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

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

[Benjamin Doron]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 662 bytes --]

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

[Nate DeSimone]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

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@intel.com>; 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


[-- Attachment #2: Type: text/html, Size: 4022 bytes --]

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

[Benjamin Doron]

[-- Attachment #1: Type: text/plain, Size: 195 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 195 bytes --]

[edk2-devel][edk2-platforms][PATCH v1 0/4] Platform/Intel:

[Benjamin Doron]

BoardAcpiDxe is derived from AcpiPlatformDxe, but only the
silicon/platform DSDT is installed. However, boards can have tables that
must be installed too. For instance, laptops often have an EC ACPI
device. Therefore, implement support for installing the generic
PcdAcpiTableStorageFile too.

Tested with KabylakeOpenBoardPkg, the Aspire VN7-572G's BoardAcpiTables
are successfully installed.

Benjamin Doron (4):
  KabylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile
  CometlakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile
  MinPlatformPkg/Acpi: MinDsdt also installs all PcdAcpiTableStorageFile
  WhiskeylakeOpenBoardPkg/Acpi: BoardAcpiDxe also installs all
    PcdAcpiTableStorageFile

 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 .../Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 10 ++++++++++
 .../Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf |  3 +++
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 8 files changed, 61 insertions(+), 3 deletions(-)

-- 
2.37.2

[edk2-devel][edk2-platforms][PATCH v1 1/4] KabylakeOpenBoardPkg/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>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 0361ffc2fbf9..c6df1714af9a 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -288,7 +288,20 @@ InstallAcpiBoard (
   //
   // Platform ACPI Tables
   //
-  PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  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
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
 
   //
   // This protocol publish must be done after PublishAcpiTablesFromFv.
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index 5d3d4c3a2b33..7b400cf967f3 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
@@ -67,6 +67,8 @@
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.37.2

[edk2-devel][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>
---
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index 7fc71bca644a..ba59c84fd4ba 100644
--- a/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/CometlakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -270,7 +270,20 @@ InstallAcpiBoard (
   //
   // Platform ACPI Tables
   //
-  PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  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
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
 
   //
   // 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/BoardAcpiDxe.inf
@@ -62,6 +62,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.37.2

[edk2-devel][edk2-platforms][PATCH v1 3/4] MinPlatformPkg/Acpi: MinDsdt 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: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c   | 10 ++++++++++
 Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
index 9bc222453857..ac343afca7d4 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.c
@@ -227,6 +227,16 @@ InstallMinDsdt (
 
   Status = PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
   ASSERT_EFI_ERROR (Status);
+  Status = PublishAcpiTablesFromFv (PcdGetPtr (PcdAcpiTableStorageFile));
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "Error: PublishAcpiTablesFromFv[%g] - %r",
+      PcdGetPtr (PcdAcpiTableStorageFile),
+      Status
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
   
   return EFI_SUCCESS;
 }
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
index 3437bc489cd3..dc32186eb361 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/MinDsdt/MinDsdt.inf
@@ -21,6 +21,7 @@
 
 [Packages]
   MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   MinPlatformPkg/MinPlatformPkg.dec
 
 [LibraryClasses]
@@ -41,6 +42,8 @@
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemBase
   gMinPlatformPkgTokenSpaceGuid.PcdPciReservedMemLimit
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid
-- 
2.37.2

[edk2-devel][edk2-platforms][PATCH v1 4/4] WhiskeylakeOpenBoardPkg/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>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.c              | 15 ++++++++++++++-
 .../Acpi/BoardAcpiDxe/BoardAcpiDxe.inf            |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
index cb5f328a3909..c8c1e0cc2cf2 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.c
@@ -270,7 +270,20 @@ InstallAcpiBoard (
   //
   // Platform ACPI Tables
   //
-  PublishAcpiTablesFromFv (&gEfiCallerIdGuid);
+  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
+      ));
+    DEBUG ((DEBUG_INFO, "Does board contain ACPI tables?\n"));
+  }
 
   //
   // This protocol publish must be done after PublishAcpiTablesFromFv.
diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
index af5c8f1c0609..f17fb588ddb8 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/Acpi/BoardAcpiDxe/BoardAcpiDxe.inf
@@ -60,6 +60,8 @@
   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisablePassiveTripPoints
   gWhiskeylakeOpenBoardPkgTokenSpaceGuid.PcdDisableCriticalTripPoints
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
+
 [Depex]
   gEfiAcpiTableProtocolGuid           AND
   gEfiFirmwareVolume2ProtocolGuid     AND
-- 
2.37.2