Re: [PATCH V2 4/6] PcAtChipsetPkg PcRtc: Use new EfiLocateFirstAcpiTable()

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 9/14/2018 12:41 PM, Ni, Ruiyu wrote:
> On 9/13/2018 6:26 PM, Star Zeng wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=967
>> Request to add a library function for GetAcpiTable() in order
>> to get ACPI table using signature as input.
>>
>> After evaluation, we found there are many duplicated code to
>> find ACPI table by signature in different modules.
>>
>> This patch updates PcatRealTimeClockRuntimeDxe to use new
>> EfiLocateFirstAcpiTable() and remove the duplicated code.
>>
>> Cc: Younas khan <pmdyounaskhan786@gmail.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 80 
>> +---------------------
>>   1 file changed, 3 insertions(+), 77 deletions(-)
>>
>> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c 
>> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
>> index 2105acf35f7b..7965eb8aa55b 100644
>> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
>> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
>> @@ -1203,49 +1203,6 @@ IsWithinOneDay (
>>   }
>>   /**
>> -  This function find ACPI table with the specified signature in RSDT 
>> or XSDT.
>> -
>> -  @param Sdt              ACPI RSDT or XSDT.
>> -  @param Signature        ACPI table signature.
>> -  @param TablePointerSize Size of table pointer: 4 or 8.
>> -
>> -  @return ACPI table or NULL if not found.
>> -**/
>> -VOID *
>> -ScanTableInSDT (
>> -  IN EFI_ACPI_DESCRIPTION_HEADER    *Sdt,
>> -  IN UINT32                         Signature,
>> -  IN UINTN                          TablePointerSize
>> -  )
>> -{
>> -  UINTN                          Index;
>> -  UINTN                          EntryCount;
>> -  UINTN                          EntryBase;
>> -  EFI_ACPI_DESCRIPTION_HEADER    *Table;
>> -
>> -  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / 
>> TablePointerSize;
>> -
>> -  EntryBase = (UINTN) (Sdt + 1);
>> -  for (Index = 0; Index < EntryCount; Index++) {
>> -    //
>> -    // When TablePointerSize is 4 while sizeof (VOID *) is 8, make 
>> sure the upper 4 bytes are zero.
>> -    //
>> -    Table = 0;
>> -    CopyMem (&Table, (VOID *) (EntryBase + Index * TablePointerSize), 
>> TablePointerSize);
>> -
>> -    if (Table == NULL) {
>> -      continue;
>> -    }
>> -
>> -    if (Table->Signature == Signature) {
>> -      return Table;
>> -    }
>> -  }
>> -
>> -  return NULL;
>> -}
>> -
>> -/**
>>     Get the century RTC address from the ACPI FADT table.
>>     @return  The century RTC address or 0 if not found.
>> @@ -1255,42 +1212,11 @@ GetCenturyRtcAddress (
>>     VOID
>>     )
>>   {
>> -  EFI_STATUS                                    Status;
>> -  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
>>     EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *Fadt;
>> -  Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID 
>> **) &Rsdp);
>> -  if (EFI_ERROR (Status)) {
>> -    Status = EfiGetSystemConfigurationTable (&gEfiAcpi10TableGuid, 
>> (VOID **) &Rsdp);
>> -  }
>> -
>> -  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
>> -    return 0;
>> -  }
>> -
>> -  Fadt = NULL;
>> -
>> -  //
>> -  // Find FADT in XSDT
>> -  //
>> -  if (Rsdp->Revision >= 
>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION && 
>> Rsdp->XsdtAddress != 0) {
>> -    Fadt = ScanTableInSDT (
>> -             (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress,
>> -             EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
>> -             sizeof (UINTN)
>> -             );
>> -  }
>> -
>> -  //
>> -  // Find FADT in RSDT
>> -  //
>> -  if (Fadt == NULL && Rsdp->RsdtAddress != 0) {
>> -    Fadt = ScanTableInSDT (
>> -             (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress,
>> -             EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
>> -             sizeof (UINT32)
>> -             );
>> -  }
>> +  Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) 
>> EfiLocateFirstAcpiTable (
>> +                                                         
>> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE
>> +                                                         );
>>     if ((Fadt != NULL) &&
>>         (Fadt->Century > RTC_ADDRESS_REGISTER_D) && (Fadt->Century < 
>> 0x80)
>>
> 
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> 
By the way, can we remove the Acpi10/Acpi20 GUID references from INF file?

-- 
Thanks,
Ray