Re: [PATCH v3 12/28] AmpereAltraPkg: Add Ac01PcieLib library instance

["Nhi Pham" <nhi@os.amperecomputing.com>]

Hi Leif,

Thanks a lot for your review. We are heading to improve the Ac01PcieLib 
and addressing your comments in the PciSegmentLib, by decoupling the 
Ac01PcieLib into hierarchical modules. Hoping it will look better to you.

Best regards,

Nhi


On 28/09/2021 17:34, Leif Lindholm wrote:
> Apart from the request to break out Ac01PcieConfigRW and
> Ac01PcieCfgIn/Out# I noticed a further thing.
>
> On Wed, Sep 15, 2021 at 22:55:11 +0700, Nhi Pham wrote:
>> +/**
>> +  Get RootBridge disable status.
>> +
>> +  @param[in]  HBIndex               Index to identify of PCIE Host bridge.
>> +  @param[in]  RBIndex               Index to identify of underneath PCIE Root bridge.
>> +
>> +  @retval BOOLEAN                   Return RootBridge disable status.
>> +**/
>> +BOOLEAN
>> +Ac01PcieCheckRootBridgeDisabled (
>> +  IN UINTN HBIndex,
>> +  IN UINTN RBIndex
>> +  )
>> +{
>> +  UINTN RCIndex;
>> +  INT8  Ret;
>> +
>> +  RCIndex = HBIndex;
>> +  Ret = !RCList[RCIndex].Active;
>> +  if (Ret) {
>> +    PciList[HBIndex] = -1;
>> +  } else {
>> +    PciList[HBIndex] = HBIndex;
>> +  }
>> +  if (HBIndex == (AC01_MAX_PCIE_ROOT_COMPLEX -1)) {
>> +    SortPciList (PciList);
>> +    if (!IsSlaveSocketPresent ()) {
>> +      AcpiPatchPciMem32 (PciList);
>> +    }
>> +    AcpiInstallMcfg (PciList);
>> +    AcpiInstallIort (PciList);
> Should a function named "Check if RootBridge is Disabled" really have
> the undocumented side effect of going off and installing ACPI tables?
>
> Please move that logic over to PciHostBridgeReadyToBootEvent.
>
> With that, I think this revision is fully reviewed.
>
> /
>      Leif
>
>> +  }
>> +  return Ret;
>> +}