Re: [edk2-devel][PATCH 1/1] Platform/RPi: Move away from AcpiPlatformDxe for loading ACPI tables

["Pete Batard" <pete@akeo.ie>]

Points noted. Will look to avoid these in future patches.

Regards,

/Pete

On 2020.03.02 18:30, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 18:21, Pete Batard <pete@akeo.ie> wrote:
>>
>> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>>
>> Instead use ConfigDxe. This will allow selective loading/patching
>> to enable different SBBR/EBBR profiles.
>>
> 
> For future patches, please don't break sentences in between the title
> and the commit log body. In my email window, this starts with 'Instead
> use ConfigDxe', and it takes me more cycles than necessary to figure
> out what this patch is about. The commit log should make sense by
> itself, and the title is just a summary of that.
> 
> 
> 
> 
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c   | 10 ++++++++++
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf |  2 ++
>>   Platform/RaspberryPi/RPi3/RPi3.dsc                   |  2 +-
>>   Platform/RaspberryPi/RPi3/RPi3.fdf                   |  1 -
>>   Platform/RaspberryPi/RPi4/RPi4.dsc                   |  2 +-
>>   Platform/RaspberryPi/RPi4/RPi4.fdf                   |  1 -
>>   6 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index f92ac709a3d8..5c86b6dd12b1 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -11,6 +11,7 @@
>>   #include <Library/HiiLib.h>
>>   #include <Library/DebugLib.h>
>>   #include <Library/IoLib.h>
>> +#include <Library/AcpiLib.h>
> 
> I know this is not ordered to begin with, but we usually try not to
> make things worse. I moved this include to the top.
> 
>>   #include <Library/UefiBootServicesTableLib.h>
>>   #include <Library/UefiRuntimeServicesTableLib.h>
>>   #include <Library/DevicePathLib.h>
>> @@ -26,6 +27,12 @@ extern UINT8 ConfigDxeStrings[];
>>
>>   STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
>>
>> +/*
>> + * The GUID inside Platform/RaspberryPi/RPi3/AcpiTables/AcpiTables.inf and
>> + * Platform/RaspberryPi/RPi4/AcpiTables/AcpiTables.inf _must_ match below.
>> + */
>> +STATIC CONST EFI_GUID mAcpiTableFile = { 0x7E374E25, 0x8E01, 0x4FEE, { 0x87, 0xf2, 0x39, 0x0C, 0x23, 0xC6, 0x06, 0xCD } };
> 
> This line is 122 characters long. I fixed that up for you.
> 
>> +
>>   typedef struct {
>>     VENDOR_DEVICE_PATH VendorDevicePath;
>>     EFI_DEVICE_PATH_PROTOCOL End;
>> @@ -408,5 +415,8 @@ ConfigInitialize (
>>       DEBUG ((DEBUG_ERROR, "Couldn't install ConfigDxe configuration pages: %r\n", Status));
>>     }
>>
>> +  Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>>     return EFI_SUCCESS;
>>   }
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> index 817cb98c1933..dc726cc6d934 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> @@ -35,6 +35,7 @@ [Packages]
>>     MdeModulePkg/MdeModulePkg.dec
>>     Silicon/Broadcom/Bcm283x/Bcm283x.dec
>>     Platform/RaspberryPi/RaspberryPi.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>>
>>   [LibraryClasses]
>>     BaseLib
>> @@ -46,6 +47,7 @@ [LibraryClasses]
>>     UefiDriverEntryPoint
>>     HiiLib
>>     GpioLib
>> +  AcpiLib
>>
>>   [Guids]
>>     gConfigDxeFormSetGuid
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index 304bc3dfeadf..df5b246af1f8 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -346,6 +346,7 @@ [LibraryClasses.common]
>>     PlatformBootManagerLib|Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>>     CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
>>     FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
>> +  AcpiLib|EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
>>
>>   [LibraryClasses.common.UEFI_DRIVER]
>>     UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>> @@ -548,7 +549,6 @@ [Components.common]
>>     # ACPI Support
>>     #
>>     MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> -  MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
>>     MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>>     Platform/RaspberryPi/AcpiTables/AcpiTables.inf
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> index ec3742c83729..66c2cbada59b 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> @@ -240,7 +240,6 @@ [FV.FvMain]
>>     # ACPI Support
>>     #
>>     INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> -  INF MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
>>     INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>>     INF RuleOverride = ACPITABLE Platform/RaspberryPi/AcpiTables/AcpiTables.inf
>>
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index c039f6df2eb4..94e0d91ede2f 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -355,6 +355,7 @@ [LibraryClasses.common]
>>     PlatformBootManagerLib|Platform/RaspberryPi/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>>     CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
>>     FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
>> +  AcpiLib|EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
>>
>>   [LibraryClasses.common.UEFI_DRIVER]
>>     UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>> @@ -574,7 +575,6 @@ [Components.common]
>>     # ACPI Support
>>     #
>>     MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> -  MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
>>     MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>>     Platform/RaspberryPi/AcpiTables/AcpiTables.inf
>>
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf
>> index b2a6ac9e6c66..ee57cc0dac89 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.fdf
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
>> @@ -236,7 +236,6 @@ [FV.FvMain]
>>     # ACPI Support
>>     #
>>     INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>> -  INF MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
>>     INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>>     INF RuleOverride = ACPITABLE Platform/RaspberryPi/AcpiTables/AcpiTables.inf
>>
> 
> Pushed as 07cc442f7212..cd6474cbed30
> 
> Thanks,
>