[edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile


Benjamin Doron
 

Follow AcpiPlatformDxe (from which this module is derived) and install
all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c | =
1 +
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | =
2 ++
2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAc=
piDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiD=
xe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
// Platform ACPI Tables=0D
//=0D
PublishAcpiTablesFromFv (&gEfiCallerIdGuid);=0D
+ PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));=0D
=0D
//=0D
// This protocol publish must be done after PublishAcpiTablesFromFv.=0D
diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAc=
piDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcp=
iDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.i=
nf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.i=
nf
@@ -62,6 +62,8 @@
gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase=0D
gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit=0D
=0D
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile=0D
+=0D
[Depex]=0D
gEfiAcpiTableProtocolGuid AND=0D
gEfiFirmwareVolume2ProtocolGuid AND=0D
--=20
2.31.1


Chaganty, Rangasai V
 

Hi Benjamin,
The problem statement is not clear to me.
Could you share any background for this change - RFC, Bugzilla etc?

Thanks,
Sai Chaganty

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com>
Sent: Wednesday, August 04, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Kethi Reddy, Deepika <deepika.kethi.reddy@intel.com>; Esakkithevar, Kathappan <kathappan.esakkithevar@intel.com>
Subject: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c | 1 +
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
// Platform ACPI Tables // PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+ PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile)); // // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.inf
@@ -62,6 +62,8 @@
gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex] gEfiAcpiTableProtocolGuid AND gEfiFirmwareVolume2ProtocolGuid AND--
2.31.1


Benjamin Doron
 

Hi Sai,
There may be a requirement to install board-specific ACPI tables. For example, to support an embedded controller in laptops, or other controllers in servers and desktops.

In a conversation with Nate about a board port in KabylakeOpenBoardPkg, I said that the options for such boards would be to include AcpiPlatformDxe, or, since BoardAcpiDxe seems to be a superset of AcpiPlatformDxe functionality, add it here by calling PublishAcpiTablesFromFv() again with this GUID.

Perhaps an RFC first would have been best. I don't mind dropping this patchset, but can we wait for comments first?

Best regards,
Benjamin


Nate DeSimone
 

Another thing that was interesting to me is that PcdAcpiTableStorageFile is actually defined in MdeModulePkg and used in MdeModulePkg/Universal/Acpi/AcpiPlatformDxe. Our MinPlatform board packages do not use that driver, we have our own BoardAcpiDxe that varies with each silicon generation. For that reason I’d be willing to call it a core feature that we have not implemented correctly in our MinPlatform *OpenBoardPkg’s.

 

It may be a good idea to see if MdeModulePkg/Universal/Acpi/AcpiPlatformDxe is usable in the MinPlatform *OpenBoardPkg’s but that seems like a task for another time.

 

Thanks,

Nate

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, August 4, 2021 2:51 PM
To: Chaganty; Chaganty, Rangasai V <rangasai.v.chaganty@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

 

Hi Sai,
There may be a requirement to install board-specific ACPI tables. For example, to support an embedded controller in laptops, or other controllers in servers and desktops.

In a conversation with Nate about a board port in KabylakeOpenBoardPkg, I said that the options for such boards would be to include AcpiPlatformDxe, or, since BoardAcpiDxe seems to be a superset of AcpiPlatformDxe functionality, add it here by calling PublishAcpiTablesFromFv() again with this GUID.

Perhaps an RFC first would have been best. I don't mind dropping this patchset, but can we wait for comments first?

Best regards,
Benjamin


Nate DeSimone
 

Hi Benjamin,

It would be good to add a check on the status code returned from PublishAcpiTablesFromFv() to the debug log. Something like this:

Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Error: PublishAcpiTablesFromFv[%g] - %r", &gEfiCallerIdGuid, Status));
}
Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
if (EFI_ERROR (Status)) {
DEBUG ((
DEBUG_ERROR,
"Error: PublishAcpiTablesFromFv[%g] - %r",
PcdGetPtr (PcdAcpiTableStorageFile),
Status
));
}

I know the original code doesn't do it, but I imagine this will be helpful to some poor soul who is trying to debug issues with this later on.

Thanks,
Nate

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com>
Sent: Wednesday, August 4, 2021 12:59 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Kethi Reddy, Deepika <deepika.kethi.reddy@intel.com>; Esakkithevar, Kathappan <kathappan.esakkithevar@intel.com>
Subject: [edk2-platforms][PATCH v1 2/4] CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all PcdAcpiTableStorageFile

Follow AcpiPlatformDxe (from which this module is derived) and install all ACPI tables added by board.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Deepika Kethi Reddy <deepika.kethi.reddy@intel.com>
Cc: Kathappan Esakkithevar <kathappan.esakkithevar@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c | 1 +
Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf | 2 ++
2 files changed, 3 insertions(+)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..11c1d619d1ea 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.c
@@ -271,6 +271,7 @@ InstallAcpiBoard (
// Platform ACPI Tables // PublishAcpiTablesFromFv (&gEfiCallerIdGuid);+ PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile)); // // This protocol publish must be done after PublishAcpiTablesFromFv.diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 09b67376fbc1..e201964a6896 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDx
+++ e.inf
@@ -62,6 +62,8 @@
gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile+ [Depex] gEfiAcpiTableProtocolGuid AND gEfiFirmwareVolume2ProtocolGuid AND--
2.31.1


Benjamin Doron
 

Another reason to install these tables with BoardAcpiDxe is that it calls a board-implemented hook to patch the tables, if necessary. AcpiPlatformDxe only installs the tables as they are found.