[edk2-devel][edk2-platforms][PATCH v1 0/3]

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [edk2-devel][edk2-platforms][PATCH v1 0/3]
@ 2022-09-06 17:26 Benjamin Doron
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Benjamin Doron @ 2022-09-06 17:26 UTC (permalink / raw)
  To: devel

Enable the HDMI debug port to be used for SEC phase debug. Commit my
build configuration, which successfully ouputs debug messages in all
phases and boot-flows: SEC, PEI, DXE and SMM, with handling for the
special case that is BootScriptExecutorDxe.

This patch series comprises the work product for my second planned GSoC
2022 project.

These are enhancements to Nate's series here:
https://edk2.groups.io/g/devel/message/90591.

Benjamin Doron (3):
  MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib:
    First BoardInitLib
  [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local
  KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to
    build

 .../AspireVn7Dash572G/OpenBoardPkg.dsc        | 85 +++++++++++++----
 .../AspireVn7Dash572G/OpenBoardPkg.fdf        | 11 +--
 .../AspireVn7Dash572G/OpenBoardPkgPcd.dsc     | 54 ++++++++++-
 ...ptExecutorDxeI2cHdmiDebugSerialPortLib.inf | 48 ++++++++++
 .../DxeI2cHdmiDebugSerialPortLib.inf          |  8 +-
 .../DxeSmmI2cHdmiDebugSerialPortLib.c         |  2 -
 .../Library/I2cHdmiDebugSerialPortLib/Gmbus.c | 39 ++++----
 .../I2cDebugPortProtocol.c                    |  2 -
 .../I2cDebugPortTplDxe.c                      |  9 ++
 .../I2cDebugPortTplRuntimeDxe.c               | 93 +++++++++++++++++++
 .../I2cHdmiDebugSerialPortLib.c               |  3 -
 .../I2cHdmiDebugSerialPortLib/IgfxI2c.c       |  9 +-
 .../PeiI2cHdmiDebugSerialPortLib.c            |  1 -
 .../PeiI2cHdmiDebugSerialPortLib.inf          |  5 +-
 .../RuntimeDxeI2cHdmiDebugSerialPortLib.inf   | 51 ++++++++++
 .../SecI2cHdmiDebugSerialPortLib.c            |  1 -
 .../SecI2cHdmiDebugSerialPortLib.inf          |  7 +-
 .../SmmI2cHdmiDebugSerialPortLib.inf          |  6 +-
 .../PlatformInit.c                            |  8 +-
 .../PlatformInit.c                            |  8 +-
 20 files changed, 364 insertions(+), 86 deletions(-)
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf

-- 
2.37.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib
  2022-09-06 17:26 [edk2-devel][edk2-platforms][PATCH v1 0/3] Benjamin Doron
@ 2022-09-06 17:26 ` Benjamin Doron
  2022-09-09 14:38   ` Isaac Oram
  2022-09-09 23:09   ` Nate DeSimone
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local Benjamin Doron
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 10+ messages in thread
From: Benjamin Doron @ 2022-09-06 17:26 UTC (permalink / raw)
  To: devel
  Cc: Sai Chaganty, Isaac Oram, Nate DeSimone, Ankit Sinha, Chasel Chiu,
	Liming Gao, Eric Dong

SecBoardInitLib is called to enable serial port before
SerialPortInitialize and DEBUG().

This is strongly assumed to be necessary for I2cHdmiDebugSerialPortLib
in SEC phase, which presently initialises this way.

No testing was performed before, it's assumed too risky unless the GPIO
happens to have the desired default. Presently, DEBUG() works in SEC
phase.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Chasel Chiu <chasel.chiu@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>
---
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index ef89e3f31018..d74b07bc062b 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange
   )
 {
+  //
+  // Board/Silicon initialization
+  // Prepare controllers before enabling serial port
+  //
+  BoardAfterTempRamInit ();
+
   //
   // Platform initialization
   // Enable Serial port here
@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));
   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange));
 
-  BoardAfterTempRamInit ();
-
   TestPointTempMemoryFunction (StartOfRange, EndOfRange);
 }
diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index 486c8c72616e..53f95c29bde5 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange
   )
 {
+  //
+  // Board/Silicon initialization
+  // Prepare controllers before enabling serial port
+  //
+  BoardAfterTempRamInit ();
+
   //
   // Platform initialization
   // Enable Serial port here
@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));
   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange));
 
-  BoardAfterTempRamInit ();
-
   TestPointTempMemoryFunction (StartOfRange, EndOfRange);
 }
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local
  2022-09-06 17:26 [edk2-devel][edk2-platforms][PATCH v1 0/3] Benjamin Doron
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
@ 2022-09-06 17:26 ` Benjamin Doron
  2022-09-09 23:09   ` Nate DeSimone
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build Benjamin Doron
  2022-09-09 23:09 ` [edk2-devel][edk2-platforms][PATCH v1 0/3] Nate DeSimone
  3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Doron @ 2022-09-06 17:26 UTC (permalink / raw)
  To: devel; +Cc: Sai Chaganty, Isaac Oram, Nate DeSimone, Ankit Sinha, Chasel Chiu

While the key patches here should probably be merged into the initial
patches for this library, it appears it isn't being merged soon.
Therefore, commit the patches that improve the library for S3 use.

Other than edits to INF LibraryClasses and header `#include`s, the
primary patch here assists runtime by creating events to toggle a
boolean such that gBS is not used after end-of-BS. This is necessary for
DebugLibSerialPort, but not RSC, which uninstalls the serial port
handler at end-of-BS.

For S3 resume, a key finding was that this is still insufficient. The
image is copied into the lockbox for its own security at
DxeSmmReadyToLock, so toggling booleans in the data section is
ineffective. Early DXE is fairly single-threaded and testing indicates
that simply consuming ..TplNull.c is workable.

Also, GCC 12 requires a patch to a `switch` block that
improved flow coherency.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 ...ptExecutorDxeI2cHdmiDebugSerialPortLib.inf | 48 ++++++++++
 .../DxeI2cHdmiDebugSerialPortLib.inf          |  8 +-
 .../DxeSmmI2cHdmiDebugSerialPortLib.c         |  2 -
 .../Library/I2cHdmiDebugSerialPortLib/Gmbus.c | 39 ++++----
 .../I2cDebugPortProtocol.c                    |  2 -
 .../I2cDebugPortTplDxe.c                      |  9 ++
 .../I2cDebugPortTplRuntimeDxe.c               | 93 +++++++++++++++++++
 .../I2cHdmiDebugSerialPortLib.c               |  3 -
 .../I2cHdmiDebugSerialPortLib/IgfxI2c.c       |  9 +-
 .../PeiI2cHdmiDebugSerialPortLib.c            |  1 -
 .../PeiI2cHdmiDebugSerialPortLib.inf          |  5 +-
 .../RuntimeDxeI2cHdmiDebugSerialPortLib.inf   | 51 ++++++++++
 .../SecI2cHdmiDebugSerialPortLib.c            |  1 -
 .../SecI2cHdmiDebugSerialPortLib.inf          |  7 +-
 .../SmmI2cHdmiDebugSerialPortLib.inf          |  6 +-
 15 files changed, 231 insertions(+), 53 deletions(-)
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
 create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
new file mode 100644
index 000000000000..995e67bde7d4
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
@@ -0,0 +1,48 @@
+### @file
+# Component description file for Serial I/O Port library for the HDMI I2C Debug Port
+#
+# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = RuntimeDxeI2cHdmiDebugSerialPortLib
+  FILE_GUID                      = 7E514680-470B-409C-8FC4-2FE62BF010BC
+  VERSION_STRING                 = 1.0
+  MODULE_TYPE                    = DXE_DRIVER
+  LIBRARY_CLASS                  = SerialPortLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64
+#
+
+[LibraryClasses]
+  BaseMemoryLib
+  PcdLib
+  TimerLib
+  IoLib
+  PciLib
+  UefiLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  KabylakeOpenBoardPkg/OpenBoardPkg.dec
+
+[Sources]
+  DxeSmmI2cHdmiDebugSerialPortLib.c
+  Gmbus.c
+  Gmbus.h
+  I2cDebugPortProtocol.c
+  I2cDebugPortProtocol.h
+  I2cDebugPortTplNull.c
+  I2cHdmiDebugSerialPortLib.c
+  IgfxI2c.c
+  IgfxI2c.h
+
+[Pcd]
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
index 5403d8ae0fd7..5eeee504c7ec 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
@@ -21,11 +21,13 @@
 #
 
 [LibraryClasses]
-  BaseLib
   BaseMemoryLib
   PcdLib
   TimerLib
+  IoLib
   PciLib
+  UefiBootServicesTableLib
+  UefiLib
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -42,10 +44,6 @@
   IgfxI2c.c
   IgfxI2c.h
 
-[Ppis]
-
-[Guids]
-
 [Pcd]
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
index 5556e09a7419..46827c6cefae 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
@@ -8,10 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
 #include <Base.h>
-#include <Library/BaseLib.h>
 #include <Library/SerialPortLib.h>
 #include <Library/PcdLib.h>
-#include <Library/TimerLib.h>
 
 #include <IgfxI2c.h>
 #include <I2cDebugPortProtocol.h>
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
index c04bcd285060..df5dfd70a5f2 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
@@ -7,8 +7,7 @@
 **/
 
 #include <Uefi.h>
-#include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
+//#include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 #include <Library/IoLib.h>
 #include <Library/PciLib.h>
@@ -33,6 +32,7 @@ GmbusGetGttMmAdr (
   // Check if GTT Memory Mapped BAR has been already assigned, initialize if not
   //
   GttMmPciAddress = PCI_LIB_ADDRESS (SA_IGD_BUS, SA_IGD_DEV, SA_IGD_FUN_0, R_SA_IGD_GTTMMADR);
+  // TODO(benjamindoron): TigerLake has 64-bit BAR
   GttMmAdr = PciRead32 (GttMmPciAddress) & 0xFFFFFFF0;
   //DEBUG ((DEBUG_INFO, "GttMmPciAddress = %x\n", (UINTN) GttMmPciAddress)); //@TODO
   //DEBUG ((DEBUG_INFO, "GttMmAdr = %x\n", (UINTN) GttMmAdr)); //@TODO
@@ -361,6 +361,7 @@ GmbusPrepare (
   }
   //
   // Wait for GMBUS to complete any pending commands
+  // - TODO(benjamindoron): GmbusRecoverError()
   //
   Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_INUSE, FALSE);
   if (EFI_ERROR (Status)) {
@@ -488,28 +489,28 @@ GmbusRead (
   // Input Validation
   //
   if ((*ByteCount) <= 0) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is 0, no bytes to read.\n", __FUNCTION__));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is 0, no bytes to read.\n", __FUNCTION__));
     return EFI_INVALID_PARAMETER;
   }
   if ((*ByteCount) > GMBUS_MAX_BYTES) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is greater than GMBUS_MAX_BYTES[%d].\n", __FUNCTION__, GMBUS_MAX_BYTES));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is greater than GMBUS_MAX_BYTES[%d].\n", __FUNCTION__, GMBUS_MAX_BYTES));
     return EFI_INVALID_PARAMETER;
   }
   if (ReadBuffer == NULL) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - ReadBuffer is NULL.\n", __FUNCTION__));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - ReadBuffer is NULL.\n", __FUNCTION__));
     return EFI_INVALID_PARAMETER;
   }
   if ((SlaveAddress & BIT0) != BIT0) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - BIT0 of SlaveAddress should be set for an I2C read.\n", __FUNCTION__));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - BIT0 of SlaveAddress should be set for an I2C read.\n", __FUNCTION__));
     return EFI_INVALID_PARAMETER;
   }
 
   //
   // Configure Gmbus port and clock speed
-  //
+  //GMBUS_CLOCK_RATE_100K @todo
   Status = GmbusPrepare (GMBUS_CLOCK_RATE_50K, (DdcBusPinPair & B_SA_GTTMMADR_GMBUS0_PIN_PAIR_MASK));
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusPrepare() failed - %r\n", __FUNCTION__, Status));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusPrepare() failed - %r\n", __FUNCTION__, Status));
     goto Done;
   }
 
@@ -534,7 +535,7 @@ GmbusRead (
   //
   Status = SetGmbus1Command (GmbusCmdSts);
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - SetGmbus1Command() failed - %r\n", __FUNCTION__, Status));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - SetGmbus1Command() failed - %r\n", __FUNCTION__, Status));
     goto Done;
   }
 
@@ -556,7 +557,7 @@ GmbusRead (
     //
     Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_HW_RDY, TRUE);
     if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
+      //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
     }
     //
     // Check the GMBUS2 register for error conditions (NACK or Slave Stall Timeout)
@@ -564,13 +565,13 @@ GmbusRead (
     Status2 = GetGmbus2Status (&GmbusStatus);
     if (EFI_ERROR (Status2)) {
       Status = Status2;
-      DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus2Status() failed - %r\n", __FUNCTION__, Status));
+      //DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus2Status() failed - %r\n", __FUNCTION__, Status));
       goto Done;
     }
     if (EFI_ERROR (Status) && ((GmbusStatus & B_SA_GTTMMADR_GMBUS2_NACK_INDICATOR) == 0)) {
-      DEBUG ((DEBUG_INFO, "Error: %a() - Unexpected behavior detected!\n", __FUNCTION__));
-      DEBUG ((DEBUG_INFO, "The GMBUS controller did not encounter a NACK and it did not set the HW_RDY bit.\n"));
-      DEBUG ((DEBUG_INFO, "Status = %r\n", Status));
+      //DEBUG ((DEBUG_INFO, "Error: %a() - Unexpected behavior detected!\n", __FUNCTION__));
+      //DEBUG ((DEBUG_INFO, "The GMBUS controller did not encounter a NACK and it did not set the HW_RDY bit.\n"));
+      //DEBUG ((DEBUG_INFO, "Status = %r\n", Status));
       Status = EFI_DEVICE_ERROR;
       goto Done;
     }
@@ -580,10 +581,10 @@ GmbusRead (
       // If a NACK or Slave Stall Timeout occurs, then a bus error has occurred.
       // In the event of a bus error, one must reset the GMBUS controller to resume normal operation.
       //
-      DEBUG ((DEBUG_INFO, "Error: %a() - NACK occurred during read operation.\n", __FUNCTION__));
+      //DEBUG ((DEBUG_INFO, "Error: %a() - NACK occurred during read operation.\n", __FUNCTION__));
       Status = GmbusRecoverError ();
       if (EFI_ERROR (Status)) {
-        DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRecoverError() failed - %r\n", __FUNCTION__, Status));
+        //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRecoverError() failed - %r\n", __FUNCTION__, Status));
         goto Done;
       }
       Status = EFI_DEVICE_ERROR;
@@ -594,7 +595,7 @@ GmbusRead (
     //
     Status = GetGmbus3Data (&GmbusData);
     if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus3Data() failed - %r\n", __FUNCTION__, Status));
+      //DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus3Data() failed - %r\n", __FUNCTION__, Status));
       goto Done;
     }
     for (Index = 0; (Index < sizeof (UINT32)) && (BytesRead < (*ByteCount)); Index++) {
@@ -608,7 +609,7 @@ GmbusRead (
   //
   Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_BUS_ACTIVE, FALSE);
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
     return Status;
   }
 
@@ -616,7 +617,7 @@ Done:
   Status2 = GmbusRelease ();
   if (EFI_ERROR (Status2)) {
     Status = Status2;
-    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRelease() failed - %r\n", __FUNCTION__, Status));
+    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRelease() failed - %r\n", __FUNCTION__, Status));
   }
   GmbusResetBusMaster ();
 
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
index 1a31c98347db..51eeadd75af9 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
@@ -7,9 +7,7 @@
 **/
 
 #include <Base.h>
-#include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
-#include <Library/PcdLib.h>
 #include <Library/TimerLib.h>
 
 #include <IgfxI2c.h>
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
index 9d69c0365795..d92b8d262793 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
@@ -22,6 +22,11 @@ RaiseTplForI2cDebugPortAccess (
   VOID
   )
 {
+  // DebugLibSerialPort exposes potential DEBUG bugs, such as early assertions
+  if (gBS == NULL) {
+    return;
+  }
+
   if (EfiGetCurrentTpl () < TPL_NOTIFY) {
     mPreviousTpl = gBS->RaiseTPL (TPL_NOTIFY);
   }
@@ -37,6 +42,10 @@ RestoreTplAfterI2cDebugPortAccess (
   VOID
   )
 {
+  if (gBS == NULL) {
+    return;
+  }
+
   if (mPreviousTpl > 0) {
     gBS->RestoreTPL (mPreviousTpl);
     mPreviousTpl = 0;
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
new file mode 100644
index 000000000000..7aa95157d734
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
@@ -0,0 +1,93 @@
+/** @file
+  Serial I/O Port library implementation for the HDMI I2C Debug Port
+  DXE Library implementation
+
+Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+
+STATIC EFI_TPL    mPreviousTpl        = 0;
+STATIC EFI_EVENT  mEvent              = NULL;
+STATIC UINT8      mEndOfBootServices  = 0;
+
+/**
+  Exit Boot Services Event notification handler.
+
+  @param[in]  Event     Event whose notification function is being invoked
+  @param[in]  Context   Pointer to the notification function's context
+
+**/
+VOID
+EFIAPI
+OnExitBootServices (
+  IN      EFI_EVENT                 Event,
+  IN      VOID                      *Context
+  )
+{
+  mEndOfBootServices = 1;
+
+  gBS->CloseEvent (Event);
+}
+
+/**
+  For boot phases that utilize task priority levels (TPLs), this function raises
+  the TPL to the appriopriate level needed to execute I/O to the I2C Debug Port
+**/
+VOID
+RaiseTplForI2cDebugPortAccess (
+  VOID
+  )
+{
+  // DebugLibSerialPort exposes potential DEBUG bugs, such as early assertions
+  if (gBS == NULL || mEndOfBootServices == 1) {
+    return;
+  }
+
+  // An event is required for a boolean to bypass TPL modification after
+  // exit-BS. RSC obviates this, requiring it for runtime DebugLibSerialPort.
+  // - Consider creating a TplRuntimeDxe, although UefiRuntimeLib uses gST?
+  // - BootScriptExecutorDxe is a special-case, where booleans are ineffective
+  //
+  // A constructor would cycle, SerialPortInitialize() takes no arguments,
+  // and no BootServicesTableLib can be called by AutoGen early enough.
+  // Therefore, we generate the event here.
+  if (mEvent == NULL) {
+    gBS->CreateEventEx (
+           EVT_NOTIFY_SIGNAL,
+           TPL_NOTIFY,
+           OnExitBootServices,
+           NULL,
+           &gEfiEventExitBootServicesGuid,
+           &mEvent
+           );
+  }
+
+  if (EfiGetCurrentTpl () < TPL_NOTIFY) {
+    mPreviousTpl = gBS->RaiseTPL (TPL_NOTIFY);
+  }
+}
+
+/**
+  For boot phases that utilize task priority levels (TPLs), this function
+  restores the TPL to the previous level after I/O to the I2C Debug Port is
+  complete
+**/
+VOID
+RestoreTplAfterI2cDebugPortAccess (
+  VOID
+  )
+{
+  if (gBS == NULL || mEndOfBootServices == 1) {
+    return;
+  }
+
+  if (mPreviousTpl > 0) {
+    gBS->RestoreTPL (mPreviousTpl);
+    mPreviousTpl = 0;
+  }
+}
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
index 89a01b868da3..402b5a3033a9 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
@@ -7,10 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
 #include <Base.h>
-#include <Library/BaseLib.h>
 #include <Library/SerialPortLib.h>
-#include <Library/PcdLib.h>
-#include <Library/TimerLib.h>
 
 #include <IgfxI2c.h>
 #include <I2cDebugPortProtocol.h>
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
index b1273c7a5d10..886351ad4297 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
@@ -7,8 +7,6 @@
 **/
 
 #include <Uefi.h>
-#include <Library/BaseLib.h>
-#include <Library/IoLib.h>
 #include <Library/PciLib.h>
 #include <IgfxI2c.h>
 
@@ -84,7 +82,6 @@ GetGmbusBusPinPair (
         default:
           return EFI_INVALID_PARAMETER;
       }
-      break;
     // The PCH design lineage from newer CoffeeLake & WhiskeyLake
     case PchTypeCnlLp:
     case PchTypeCnlH:
@@ -105,8 +102,8 @@ GetGmbusBusPinPair (
         default:
           return EFI_INVALID_PARAMETER;
       }
-      break;
-  }
 
-  return EFI_UNSUPPORTED;
+    default:
+      return EFI_UNSUPPORTED;
+  }
 }
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
index c99821367354..05b2d31bbfc2 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
@@ -10,7 +10,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Base.h>
 #include <PiPei.h>
 
-#include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/SerialPortLib.h>
 #include <Library/HobLib.h>
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
index 62b3cd3e1e49..64d8f682b786 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
@@ -21,10 +21,11 @@
 #
 
 [LibraryClasses]
-  BaseLib
   BaseMemoryLib
   PcdLib
+  HobLib
   TimerLib
+  IoLib
   PciLib
 
 [Packages]
@@ -42,8 +43,6 @@
   IgfxI2c.c
   IgfxI2c.h
 
-[Ppis]
-
 [Guids]
   gI2cHdmiDebugHobGuid
 
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
new file mode 100644
index 000000000000..eefe85f2814c
--- /dev/null
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
@@ -0,0 +1,51 @@
+### @file
+# Component description file for Serial I/O Port library for the HDMI I2C Debug Port
+#
+# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = RuntimeDxeI2cHdmiDebugSerialPortLib
+  FILE_GUID                      = 08891D97-994C-48E9-9983-E99D622D32C8
+  VERSION_STRING                 = 1.0
+  MODULE_TYPE                    = DXE_DRIVER
+  LIBRARY_CLASS                  = SerialPortLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64
+#
+
+[LibraryClasses]
+  BaseMemoryLib
+  PcdLib
+  TimerLib
+  IoLib
+  PciLib
+  UefiLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  KabylakeOpenBoardPkg/OpenBoardPkg.dec
+
+[Sources]
+  DxeSmmI2cHdmiDebugSerialPortLib.c
+  Gmbus.c
+  Gmbus.h
+  I2cDebugPortProtocol.c
+  I2cDebugPortProtocol.h
+  I2cDebugPortTplRuntimeDxe.c
+  I2cHdmiDebugSerialPortLib.c
+  IgfxI2c.c
+  IgfxI2c.h
+
+[Guids]
+  gEfiEventExitBootServicesGuid                                           ## CONSUMES  ## Event
+
+[Pcd]
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
index 416114d4363c..e4a65a66d9c1 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
@@ -8,7 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
 #include <Base.h>
-#include <Library/BaseLib.h>
 #include <Library/SerialPortLib.h>
 #include <Library/PcdLib.h>
 
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
index 3ae724926fdd..a9780decd1a7 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
@@ -21,10 +21,9 @@
 #
 
 [LibraryClasses]
-  BaseLib
   BaseMemoryLib
-  PcdLib
   TimerLib
+  IoLib
   PciLib
 
 [Packages]
@@ -42,10 +41,6 @@
   IgfxI2c.h
   SecI2cHdmiDebugSerialPortLib.c
 
-[Ppis]
-
-[Guids]
-
 [Pcd]
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
index dcbf43b886c1..65f2b4f5f731 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
@@ -21,10 +21,10 @@
 #
 
 [LibraryClasses]
-  BaseLib
   BaseMemoryLib
   PcdLib
   TimerLib
+  IoLib
   PciLib
 
 [Packages]
@@ -42,10 +42,6 @@
   IgfxI2c.c
   IgfxI2c.h
 
-[Ppis]
-
-[Guids]
-
 [Pcd]
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build
  2022-09-06 17:26 [edk2-devel][edk2-platforms][PATCH v1 0/3] Benjamin Doron
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local Benjamin Doron
@ 2022-09-06 17:26 ` Benjamin Doron
  2022-09-09 23:09   ` Nate DeSimone
  2022-09-09 23:09 ` [edk2-devel][edk2-platforms][PATCH v1 0/3] Nate DeSimone
  3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Doron @ 2022-09-06 17:26 UTC (permalink / raw)
  To: devel; +Cc: Chasel Chiu, Sai Chaganty, Isaac Oram, Nate DeSimone, Ankit Sinha

HDMI port can be used with I2cHdmiDebugSerialPortLib, for debugging in
all phases.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 .../AspireVn7Dash572G/OpenBoardPkg.dsc        | 85 +++++++++++++++----
 .../AspireVn7Dash572G/OpenBoardPkg.fdf        | 11 ++-
 .../AspireVn7Dash572G/OpenBoardPkgPcd.dsc     | 54 ++++++++++--
 3 files changed, 121 insertions(+), 29 deletions(-)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
index 261f141056f5..c71b7169a38a 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
@@ -25,9 +25,10 @@
   #
   # Debug logging
   #
+  DEFINE USE_HDMI_DEBUG_PORT  = FALSE
   DEFINE USE_PEI_SPI_LOGGING  = FALSE
   DEFINE USE_MEMORY_LOGGING   = FALSE
-  DEFINE RELEASE_LOGGING      = ($(USE_PEI_SPI_LOGGING) || $(USE_MEMORY_LOGGING))
+  DEFINE RELEASE_LOGGING      = ($(USE_HDMI_DEBUG_PORT) || $(USE_PEI_SPI_LOGGING) || $(USE_MEMORY_LOGGING))
   DEFINE TESTING              = TRUE
 
   PLATFORM_NAME                               = $(PLATFORM_PACKAGE)
@@ -205,6 +206,15 @@
   #######################################
   DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
 
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+  DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+
+  #######################################
+  # Board-specific/Silicon Package
+  #######################################
+  SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
+!endif
+
   #######################################
   # Platform Package
   #######################################
@@ -277,7 +287,7 @@
   # Edk2 Packages
   #######################################
 # In-memory logging may require too many services for early core debug output
-!if $(USE_MEMORY_LOGGING) == TRUE
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
   DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
 !endif
 
@@ -285,7 +295,7 @@
   #######################################
   # Edk2 Packages
   #######################################
-!if $(USE_MEMORY_LOGGING) == TRUE
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
   DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
 !endif
 
@@ -322,7 +332,7 @@
   #######################################
   # Edk2 Packages
   #######################################
-!if $(USE_MEMORY_LOGGING) == TRUE
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
   DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
 !endif
 
@@ -336,7 +346,7 @@
   # Edk2 Packages
   #######################################
 # In-memory logging may require too many services for early core debug output
-!if $(USE_MEMORY_LOGGING) == TRUE
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
   DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
 !endif
 
@@ -344,7 +354,7 @@
   #######################################
   # Edk2 Packages
   #######################################
-!if $(USE_MEMORY_LOGGING) == TRUE
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
   DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
 !endif
 
@@ -363,7 +373,21 @@
   TestPointCheckLib|$(PLATFORM_PACKAGE)/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf
 !endif
 
-# TODO: DebugLib override for UEFI_DRIVER and UEFI_APPLICATION?
+[LibraryClasses.common.UEFI_DRIVER]
+  #######################################
+  # Edk2 Packages
+  #######################################
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
+  DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
+!endif
+
+[LibraryClasses.common.UEFI_APPLICATION]
+  #######################################
+  # Edk2 Packages
+  #######################################
+!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
+  DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
+!endif
 
 # TODO: Add and improve feature support
 #######################################
@@ -389,6 +413,9 @@
 !if $(USE_MEMORY_LOGGING) == TRUE
       SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/PeiSerialPortLibMem.inf
 !endif
+!endif
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
 !endif
     <PcdsFixedAtBuild>
       gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(RELEASE_LOGGING)
@@ -517,14 +544,24 @@
   #######################################
   # Edk2 Packages
   #######################################
+  MdeModulePkg/Core/Dxe/DxeMain.inf {
+    <LibraryClasses>
+      # Can debug CpuExceptionHandlerLib
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
+!endif
+  }
   MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
     <LibraryClasses>
       DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
 !if $(USE_MEMORY_LOGGING) == TRUE
       SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/DxeSerialPortLibMem.inf
+!endif
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
 !endif
     <PcdsFixedAtBuild>
-      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(USE_MEMORY_LOGGING)
+      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|($(USE_MEMORY_LOGGING) || $(USE_HDMI_DEBUG_PORT))
       gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|512
   }
   # TODO: Still requires a little more thought
@@ -533,9 +570,12 @@
       DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
 !if $(USE_MEMORY_LOGGING) == TRUE
       SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/SmmSerialPortLibMem.inf
+!endif
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
 !endif
     <PcdsFixedAtBuild>
-      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(USE_MEMORY_LOGGING)
+      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|($(USE_MEMORY_LOGGING) || $(USE_HDMI_DEBUG_PORT))
       gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|512
   }
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
@@ -548,12 +588,24 @@
   MdeModulePkg/Universal/BdsDxe/BdsDxe.inf {
     <LibraryClasses>
       NULL|BoardModulePkg/Library/BdsPs2KbcLib/BdsPs2KbcLib.inf
+!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
+      NULL|BoardModulePkg/Library/BdsSerialPortTerminalLib/BdsSerialPortTerminalLib.inf
+!endif
   }
+!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
+  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
+    <LibraryClasses>
+      DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
+  }
+  MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+!endif
+
   UefiCpuPkg/CpuDxe/CpuDxe.inf {
     <LibraryClasses>
-!if $(USE_MEMORY_LOGGING) == TRUE
-# TODO/TEST
-#      SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/DxeSerialPortLibMem.inf
+      # Can debug CpuExceptionHandlerLib
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
 !endif
   }
 
@@ -589,12 +641,9 @@
     <PcdsPatchableInModule>
       gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80080046
     <LibraryClasses>
-      !if $(TARGET) == DEBUG
-        DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
-      !endif
-!if $(USE_MEMORY_LOGGING) == TRUE
-# TODO/TEST
-#      SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/SmmSerialPortLibMem.inf
+        # Can debug CpuExceptionHandlerLib
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+        SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
 !endif
   }
 !endif
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
index 9eb37e7d230e..864d5561d7d8 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
@@ -353,6 +353,10 @@ INF  MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutputDxe.inf
 INF  BoardModulePkg/LegacySioDxe/LegacySioDxe.inf
 INF  MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
 INF  MdeModulePkg/Bus/Isa/Ps2MouseDxe/Ps2MouseDxe.inf
+!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
+  INF  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+  INF  MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+!endif
 INF  BoardModulePkg/BoardBdsHookDxe/BoardBdsHookDxe.inf
 
 INF  ShellPkg/Application/Shell/Shell.inf
@@ -584,12 +588,7 @@ INF  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
 
 !if gMinPlatformPkgTokenSpaceGuid.PcdBootToShellOnly == FALSE
 
-INF $(PLATFORM_SI_PACKAGE)/Hsti/Dxe/HstiSiliconDxe.inf
-
-!endif
-
-!if gMinPlatformPkgTokenSpaceGuid.PcdBootToShellOnly == FALSE
-
+INF  $(PLATFORM_SI_PACKAGE)/Hsti/Dxe/HstiSiliconDxe.inf
 INF  $(PLATFORM_PACKAGE)/Hsti/HstiIbvPlatformDxe/HstiIbvPlatformDxe.inf
 
 !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
index a9d531a269a5..a4ea524e26bc 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
@@ -228,7 +228,7 @@
   gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
 !else
   gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x0
-  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x3
+  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x03
 !endif # $(RELEASE_LOGGING)
 !else
   # FIXME: More than just compiler optimisation is hooked to DEBUG builds.
@@ -264,6 +264,8 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE
 !if $(TARGET) == RELEASE
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
+  # Determine RTS/CTS requirement
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
 !else
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
@@ -407,7 +409,37 @@
   #  3: DDC channel C
   #  4: DDC channel D
   # @Prompt DDC I2C channel to claim as the HDMI debug port
-  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel|0x00  #@todo - Set to correct value for VN7-572G
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel|0x02
+
+  ## Enable usage the HDMI DDC channel as a debug port - Causes the BIOS debug log
+  #  to be written to the HDMI DDC channel.
+  #  The value is defined as below.
+  #  FALSE: Do NOT use the HDMI DDC channel as a debug port
+  #  TRUE:  Use the HDMI DDC channel as a debug port
+  # @Prompt Enable usage the HDMI DDC channel as a debug port
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortEnable|$(USE_HDMI_DEBUG_PORT)
+
+  ## Enable usage the HDMI DDC channel as a serial terminal - Enables usage of the
+  #  HDMI DDC channel to display BIOS Setup, UEFI Shell, etc. using a terminal
+  #  emulator. Useful for cases where video is not operating correctly.
+  #
+  #  The value is defined as below.
+  #  FALSE: Do NOT use the HDMI DDC channel as a debug port
+  #  TRUE:  Use the HDMI DDC channel as a debug port
+  # @Prompt Enable usage the HDMI DDC channel as a debug port
+  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable|FALSE
+
+  ## Indicates the type of terminal to use.
+  #  If PcdI2cHdmiDebugPortSerialTerminalEnable is TRUE, this PCD will be used
+  #  to determine which terminal protocol to use.
+  #  0 - PCANSI
+  #  1 - VT100
+  #  2 - VT100+
+  #  3 - UTF8
+  #  4 - TTYTERM
+  # @Prompt Default Terminal Type.
+  # @ValidRange 0x80000001 | 0 - 4
+  gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|3
 
 [PcdsFixedAtBuild.IA32]
   ######################################
@@ -433,7 +465,16 @@
   ######################################
   # Edk2 Configuration
   ######################################
-  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80000046  # 0x804800C7/0x806A15CF give useful information, but is very noisy
+  # TODO: Dynamic in HII
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80000046
+!if FALSE
+  # Filtered DEBUG_POOL, DEBUG_PAGE, DEBUG_GCD and DEBUG_CACHE
+  # - Unused: DEBUG_VARIABLE, DEBUG_BM and DEBUG_LOADFILE
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x804950CF
+!endif
+!if ($(TESTING) == TRUE && $(USE_HDMI_DEBUG_PORT) == FALSE)
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80400046
+!endif
 
   ######################################
   # Silicon Configuration
@@ -446,9 +487,9 @@
   # Platform Configuration
   ######################################
 !if $(TARGET) == DEBUG
-  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|1
+  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|TRUE
 !else
-  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|0
+  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|FALSE
 !endif
 
 [PcdsDynamicDefault]
@@ -528,6 +569,9 @@
 !else
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|5 # Variable: L"Timeout"
 !endif
+!if $(USE_HDMI_DEBUG_PORT) == TRUE
+  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|15 # Variable: L"Timeout"
+!endif
 !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
   gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
   gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
@ 2022-09-09 14:38   ` Isaac Oram
  2022-09-09 17:47     ` Nate DeSimone
  2022-09-09 23:09   ` Nate DeSimone
  1 sibling, 1 reply; 10+ messages in thread
From: Isaac Oram @ 2022-09-09 14:38 UTC (permalink / raw)
  To: Benjamin Doron, devel@edk2.groups.io
  Cc: Chaganty, Rangasai V, Desimone, Nathaniel L, Sinha, Ankit,
	Chiu, Chasel, Gao, Liming, Dong, Eric

Series should have a cover letter.  Please rebase the series and send V2 now that Nate's I2CHdmiDebugSerialPortLib has been merged.

Thanks,
Isaac

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Tuesday, September 6, 2022 10:27 AM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib

SecBoardInitLib is called to enable serial port before SerialPortInitialize and DEBUG().

This is strongly assumed to be necessary for I2cHdmiDebugSerialPortLib in SEC phase, which presently initialises this way.

No testing was performed before, it's assumed too risky unless the GPIO happens to have the desired default. Presently, DEBUG() works in SEC phase.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Chasel Chiu <chasel.chiu@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>
---
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index ef89e3f31018..d74b07bc062b 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
+++ formSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange   ) {+  //+  // Board/Silicon initialization+  // Prepare controllers before enabling serial port+  //+  BoardAfterTempRamInit ();+   //   // Platform initialization   // Enable Serial port here@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange)); -  BoardAfterTempRamInit ();-   TestPointTempMemoryFunction (StartOfRange, EndOfRange); }diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index 486c8c72616e..53f95c29bde5 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library
+++ /SecFspWrapperPlatformSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange   ) {+  //+  // Board/Silicon initialization+  // Prepare controllers before enabling serial port+  //+  BoardAfterTempRamInit ();+   //   // Platform initialization   // Enable Serial port here@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange)); -  BoardAfterTempRamInit ();-   TestPointTempMemoryFunction (StartOfRange, EndOfRange); }-- 
2.37.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib
  2022-09-09 14:38   ` Isaac Oram
@ 2022-09-09 17:47     ` Nate DeSimone
  0 siblings, 0 replies; 10+ messages in thread
From: Nate DeSimone @ 2022-09-09 17:47 UTC (permalink / raw)
  To: Oram, Isaac W, Benjamin Doron, devel@edk2.groups.io
  Cc: Chaganty, Rangasai V, Sinha, Ankit, Chiu, Chasel, Gao, Liming,
	Dong, Eric

It does kind of have a cover letter: https://edk2.groups.io/g/devel/message/93281

But there is no title on the cover letter and the reviewers where not CC'd either. Please remember to do both in the future 😊.

Thanks,
Nate

-----Original Message-----
From: Oram, Isaac W <isaac.w.oram@intel.com> 
Sent: Friday, September 9, 2022 7:38 AM
To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib

Series should have a cover letter.  Please rebase the series and send V2 now that Nate's I2CHdmiDebugSerialPortLib has been merged.

Thanks,
Isaac

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Tuesday, September 6, 2022 10:27 AM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib

SecBoardInitLib is called to enable serial port before SerialPortInitialize and DEBUG().

This is strongly assumed to be necessary for I2cHdmiDebugSerialPortLib in SEC phase, which presently initialises this way.

No testing was performed before, it's assumed too risky unless the GPIO happens to have the desired default. Presently, DEBUG() works in SEC phase.

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Chasel Chiu <chasel.chiu@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>
---
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index ef89e3f31018..d74b07bc062b 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
+++ formSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange   ) {+  //+  // Board/Silicon initialization+  // Prepare controllers before enabling serial port+  //+  BoardAfterTempRamInit ();+   //   // Platform initialization   // Enable Serial port here@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange)); -  BoardAfterTempRamInit ();-   TestPointTempMemoryFunction (StartOfRange, EndOfRange); }diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
index 486c8c72616e..53f95c29bde5 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library
+++ /SecFspWrapperPlatformSecLib/PlatformInit.c
@@ -28,6 +28,12 @@ PlatformInit (
   IN VOID                 *EndOfRange   ) {+  //+  // Board/Silicon initialization+  // Prepare controllers before enabling serial port+  //+  BoardAfterTempRamInit ();+   //   // Platform initialization   // Enable Serial port here@@ -41,7 +47,5 @@ PlatformInit (
   DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));   DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange)); -  BoardAfterTempRamInit ();-   TestPointTempMemoryFunction (StartOfRange, EndOfRange); }-- 
2.37.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 0/3]
  2022-09-06 17:26 [edk2-devel][edk2-platforms][PATCH v1 0/3] Benjamin Doron
                   ` (2 preceding siblings ...)
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build Benjamin Doron
@ 2022-09-09 23:09 ` Nate DeSimone
  3 siblings, 0 replies; 10+ messages in thread
From: Nate DeSimone @ 2022-09-09 23:09 UTC (permalink / raw)
  To: devel@edk2.groups.io, benjamin.doron00@gmail.com

Hi Benjamin,

I have provided some comments for you on patches 2 and 3. In addition, please rebase this patch series with latest as there has been some changes since my initial patch series from June.

Thanks,
Nate

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Benjamin Doron
> Sent: Tuesday, September 6, 2022 10:27 AM
> To: devel@edk2.groups.io
> Subject: [edk2-devel][edk2-platforms][PATCH v1 0/3]
> 
> Enable the HDMI debug port to be used for SEC phase debug. Commit my
> build configuration, which successfully ouputs debug messages in all phases
> and boot-flows: SEC, PEI, DXE and SMM, with handling for the special case
> that is BootScriptExecutorDxe.
> 
> This patch series comprises the work product for my second planned GSoC
> 2022 project.
> 
> These are enhancements to Nate's series here:
> https://edk2.groups.io/g/devel/message/90591.
> 
> Benjamin Doron (3):
> 
> MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib:
>     First BoardInitLib
>   [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local
>   KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to
>     build
> 
>  .../AspireVn7Dash572G/OpenBoardPkg.dsc        | 85 +++++++++++++----
>  .../AspireVn7Dash572G/OpenBoardPkg.fdf        | 11 +--
>  .../AspireVn7Dash572G/OpenBoardPkgPcd.dsc     | 54 ++++++++++-
>  ...ptExecutorDxeI2cHdmiDebugSerialPortLib.inf | 48 ++++++++++
>  .../DxeI2cHdmiDebugSerialPortLib.inf          |  8 +-
>  .../DxeSmmI2cHdmiDebugSerialPortLib.c         |  2 -
>  .../Library/I2cHdmiDebugSerialPortLib/Gmbus.c | 39 ++++----
>  .../I2cDebugPortProtocol.c                    |  2 -
>  .../I2cDebugPortTplDxe.c                      |  9 ++
>  .../I2cDebugPortTplRuntimeDxe.c               | 93 +++++++++++++++++++
>  .../I2cHdmiDebugSerialPortLib.c               |  3 -
>  .../I2cHdmiDebugSerialPortLib/IgfxI2c.c       |  9 +-
>  .../PeiI2cHdmiDebugSerialPortLib.c            |  1 -
>  .../PeiI2cHdmiDebugSerialPortLib.inf          |  5 +-
>  .../RuntimeDxeI2cHdmiDebugSerialPortLib.inf   | 51 ++++++++++
>  .../SecI2cHdmiDebugSerialPortLib.c            |  1 -
>  .../SecI2cHdmiDebugSerialPortLib.inf          |  7 +-
>  .../SmmI2cHdmiDebugSerialPortLib.inf          |  6 +-
>  .../PlatformInit.c                            |  8 +-
>  .../PlatformInit.c                            |  8 +-
>  20 files changed, 364 insertions(+), 86 deletions(-)  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/
> BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
>  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I
> 2cDebugPortTplRuntimeDxe.c
>  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/
> RuntimeDxeI2cHdmiDebugSerialPortLib.inf
> 
> --
> 2.37.2
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
  2022-09-09 14:38   ` Isaac Oram
@ 2022-09-09 23:09   ` Nate DeSimone
  1 sibling, 0 replies; 10+ messages in thread
From: Nate DeSimone @ 2022-09-09 23:09 UTC (permalink / raw)
  To: Benjamin Doron, devel@edk2.groups.io
  Cc: Chaganty, Rangasai V, Oram, Isaac W, Sinha, Ankit, Chiu, Chasel,
	Gao, Liming, Dong, Eric

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

> -----Original Message-----
> From: Benjamin Doron <benjamin.doron00@gmail.com>
> Sent: Tuesday, September 6, 2022 10:27 AM
> To: devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>;
> Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH v1 1/3]
> MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib:
> First BoardInitLib
> 
> SecBoardInitLib is called to enable serial port before SerialPortInitialize and
> DEBUG().
> 
> This is strongly assumed to be necessary for I2cHdmiDebugSerialPortLib in
> SEC phase, which presently initialises this way.
> 
> No testing was performed before, it's assumed too risky unless the GPIO
> happens to have the desired default. Presently, DEBUG() works in SEC
> phase.
> 
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Ankit Sinha <ankit.sinha@intel.com>
> Cc: Chasel Chiu <chasel.chiu@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>
> ---
>  .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
>  .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/PlatformInit.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/PlatformInit.c
> index ef89e3f31018..d74b07bc062b 100644
> ---
>  .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
>  .../Library/SecFspWrapperPlatformSecLib/PlatformInit.c    | 8 ++++++--
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> index ef89e3f31018..d74b07bc062b 100644
> --- a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> @@ -28,6 +28,12 @@ PlatformInit (
>    IN VOID                 *EndOfRange
>    )
>  {
> +  //
> +  // Board/Silicon initialization
> +  // Prepare controllers before enabling serial port
> +  //
> +  BoardAfterTempRamInit ();
> +
>    //
>    // Platform initialization
>    // Enable Serial port here
> @@ -41,7 +47,5 @@ PlatformInit (
>    DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));
>    DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange));
>  
> -  BoardAfterTempRamInit ();
> -
>    TestPointTempMemoryFunction (StartOfRange, EndOfRange);
>  }
> diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> index 486c8c72616e..53f95c29bde5 100644
> --- a/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> +++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme/FspWrapper/Library/SecFspWrapperPlatformSecLib/PlatformInit.c
> @@ -28,6 +28,12 @@ PlatformInit (
>    IN VOID                 *EndOfRange
>    )
>  {
> +  //
> +  // Board/Silicon initialization
> +  // Prepare controllers before enabling serial port
> +  //
> +  BoardAfterTempRamInit ();
> +
>    //
>    // Platform initialization
>    // Enable Serial port here
> @@ -41,7 +47,5 @@ PlatformInit (
>    DEBUG ((DEBUG_INFO, "StartOfRange - 0x%x\n", StartOfRange));
>    DEBUG ((DEBUG_INFO, "EndOfRange - 0x%x\n", EndOfRange));
>  
> -  BoardAfterTempRamInit ();
> -
>    TestPointTempMemoryFunction (StartOfRange, EndOfRange);
>  }
> -- 
> 2.37.2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local Benjamin Doron
@ 2022-09-09 23:09   ` Nate DeSimone
  0 siblings, 0 replies; 10+ messages in thread
From: Nate DeSimone @ 2022-09-09 23:09 UTC (permalink / raw)
  To: Benjamin Doron, devel@edk2.groups.io
  Cc: Chaganty, Rangasai V, Oram, Isaac W, Sinha, Ankit, Chiu, Chasel



> -----Original Message-----
> From: Benjamin Doron <benjamin.doron00@gmail.com>
> Sent: Tuesday, September 6, 2022 10:27 AM
> To: devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>;
> Chiu, Chasel <chasel.chiu@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP]
> KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local
> 
> While the key patches here should probably be merged into the initial
> patches for this library, it appears it isn't being merged soon.
> Therefore, commit the patches that improve the library for S3 use.
> 
> Other than edits to INF LibraryClasses and header `#include`s, the primary
> patch here assists runtime by creating events to toggle a boolean such that
> gBS is not used after end-of-BS. This is necessary for DebugLibSerialPort, but
> not RSC, which uninstalls the serial port handler at end-of-BS.
> 
> For S3 resume, a key finding was that this is still insufficient. The image is
> copied into the lockbox for its own security at DxeSmmReadyToLock, so
> toggling booleans in the data section is ineffective. Early DXE is fairly single-
> threaded and testing indicates that simply consuming ..TplNull.c is workable.
> 
> Also, GCC 12 requires a patch to a `switch` block that improved flow
> coherency.
> 
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Ankit Sinha <ankit.sinha@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
> ---
>  ...ptExecutorDxeI2cHdmiDebugSerialPortLib.inf | 48 ++++++++++
>  .../DxeI2cHdmiDebugSerialPortLib.inf          |  8 +-
>  .../DxeSmmI2cHdmiDebugSerialPortLib.c         |  2 -
>  .../Library/I2cHdmiDebugSerialPortLib/Gmbus.c | 39 ++++----
>  .../I2cDebugPortProtocol.c                    |  2 -
>  .../I2cDebugPortTplDxe.c                      |  9 ++
>  .../I2cDebugPortTplRuntimeDxe.c               | 93 +++++++++++++++++++
>  .../I2cHdmiDebugSerialPortLib.c               |  3 -
>  .../I2cHdmiDebugSerialPortLib/IgfxI2c.c       |  9 +-
>  .../PeiI2cHdmiDebugSerialPortLib.c            |  1 -
>  .../PeiI2cHdmiDebugSerialPortLib.inf          |  5 +-
>  .../RuntimeDxeI2cHdmiDebugSerialPortLib.inf   | 51 ++++++++++
>  .../SecI2cHdmiDebugSerialPortLib.c            |  1 -
>  .../SecI2cHdmiDebugSerialPortLib.inf          |  7 +-
>  .../SmmI2cHdmiDebugSerialPortLib.inf          |  6 +-
>  15 files changed, 231 insertions(+), 53 deletions(-)
>  create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
>  create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
>  create mode 100644 Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
> 
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
> new file mode 100644
> index 000000000000..995e67bde7d4
> --- /dev/null
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/BootScriptExecutorDxeI2cHdmiDebugSerialPortLib.inf
> @@ -0,0 +1,48 @@
> +### @file
> +# Component description file for Serial I/O Port library for the HDMI I2C Debug Port
> +#
> +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = RuntimeDxeI2cHdmiDebugSerialPortLib
> +  FILE_GUID                      = 7E514680-470B-409C-8FC4-2FE62BF010BC
> +  VERSION_STRING                 = 1.0
> +  MODULE_TYPE                    = DXE_DRIVER
> +  LIBRARY_CLASS                  = SerialPortLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64
> +#
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  PcdLib
> +  TimerLib
> +  IoLib
> +  PciLib
> +  UefiLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  KabylakeOpenBoardPkg/OpenBoardPkg.dec
> +
> +[Sources]
> +  DxeSmmI2cHdmiDebugSerialPortLib.c
> +  Gmbus.c
> +  Gmbus.h
> +  I2cDebugPortProtocol.c
> +  I2cDebugPortProtocol.h
> +  I2cDebugPortTplNull.c
> +  I2cHdmiDebugSerialPortLib.c
> +  IgfxI2c.c
> +  IgfxI2c.h
> +
> +[Pcd]
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
> index 5403d8ae0fd7..5eeee504c7ec 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
> @@ -21,11 +21,13 @@
>  #
>  
>  [LibraryClasses]
> -  BaseLib
>    BaseMemoryLib
>    PcdLib
>    TimerLib
> +  IoLib
>    PciLib
> +  UefiBootServicesTableLib
> +  UefiLib
>  
>  [Packages]
>    MdePkg/MdePkg.dec
> @@ -42,10 +44,6 @@
>    IgfxI2c.c
>    IgfxI2c.h
>  
> -[Ppis]
> -
> -[Guids]
> -
>  [Pcd]
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
> index 5556e09a7419..46827c6cefae 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/DxeSmmI2cHdmiDebugSerialPortLib.c
> @@ -8,10 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>  
>  #include <Base.h>
> -#include <Library/BaseLib.h>
>  #include <Library/SerialPortLib.h>
>  #include <Library/PcdLib.h>
> -#include <Library/TimerLib.h>
>  
>  #include <IgfxI2c.h>
>  #include <I2cDebugPortProtocol.h>
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
> index c04bcd285060..df5dfd70a5f2 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/Gmbus.c
> @@ -7,8 +7,7 @@
>  **/
>  
>  #include <Uefi.h>
> -#include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
> +//#include <Library/DebugLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/PciLib.h>
> @@ -33,6 +32,7 @@ GmbusGetGttMmAdr (
>    // Check if GTT Memory Mapped BAR has been already assigned, initialize if not
>    //
>    GttMmPciAddress = PCI_LIB_ADDRESS (SA_IGD_BUS, SA_IGD_DEV, SA_IGD_FUN_0, R_SA_IGD_GTTMMADR);
> +  // TODO(benjamindoron): TigerLake has 64-bit BAR

Please delete this comment. It lacks relevance to the scope of the current patch series. If you would like to port this to Tiger Lake at some point in the future, I would recommend filing a Bugzilla, assigning it to yourself, and making a note of the 64-bit BAR being one of the issues to be addressed when porting to newer silicon.

>    GttMmAdr = PciRead32 (GttMmPciAddress) & 0xFFFFFFF0;
>    //DEBUG ((DEBUG_INFO, "GttMmPciAddress = %x\n", (UINTN) GttMmPciAddress)); //@TODO
>    //DEBUG ((DEBUG_INFO, "GttMmAdr = %x\n", (UINTN) GttMmAdr)); //@TODO
> @@ -361,6 +361,7 @@ GmbusPrepare (
>    }
>    //
>    // Wait for GMBUS to complete any pending commands
> +  // - TODO(benjamindoron): GmbusRecoverError()

Please remove the TODO and either put the call to GmbusRecoverError() in... or not. I think it is a good idea to add a call to GmbusRecoverError() in the case of GmbusWaitForReady() returning EFI_TIMEOUT.

>    //
>    Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_INUSE, FALSE);
>    if (EFI_ERROR (Status)) {
> @@ -488,28 +489,28 @@ GmbusRead (
>    // Input Validation
>    //
>    if ((*ByteCount) <= 0) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is 0, no bytes to read.\n", __FUNCTION__));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is 0, no bytes to read.\n", __FUNCTION__));
>      return EFI_INVALID_PARAMETER;
>    }
>    if ((*ByteCount) > GMBUS_MAX_BYTES) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is greater than GMBUS_MAX_BYTES[%d].\n", __FUNCTION__, GMBUS_MAX_BYTES));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - ByteCount is greater than GMBUS_MAX_BYTES[%d].\n", __FUNCTION__, GMBUS_MAX_BYTES));
>      return EFI_INVALID_PARAMETER;
>    }
>    if (ReadBuffer == NULL) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - ReadBuffer is NULL.\n", __FUNCTION__));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - ReadBuffer is NULL.\n", __FUNCTION__));
>      return EFI_INVALID_PARAMETER;
>    }
>    if ((SlaveAddress & BIT0) != BIT0) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - BIT0 of SlaveAddress should be set for an I2C read.\n", __FUNCTION__));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - BIT0 of SlaveAddress should be set for an I2C read.\n", __FUNCTION__));
>      return EFI_INVALID_PARAMETER;
>    }
>  
>    //
>    // Configure Gmbus port and clock speed
> -  //
> +  //GMBUS_CLOCK_RATE_100K @todo
>    Status = GmbusPrepare (GMBUS_CLOCK_RATE_50K, (DdcBusPinPair & B_SA_GTTMMADR_GMBUS0_PIN_PAIR_MASK));
>    if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusPrepare() failed - %r\n", __FUNCTION__, Status));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusPrepare() failed - %r\n", __FUNCTION__, Status));
>      goto Done;
>    }
>  
> @@ -534,7 +535,7 @@ GmbusRead (
>    //
>    Status = SetGmbus1Command (GmbusCmdSts);
>    if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - SetGmbus1Command() failed - %r\n", __FUNCTION__, Status));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - SetGmbus1Command() failed - %r\n", __FUNCTION__, Status));
>      goto Done;
>    }
>  
> @@ -556,7 +557,7 @@ GmbusRead (
>      //
>      Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_HW_RDY, TRUE);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
> +      //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
>      }
>      //
>      // Check the GMBUS2 register for error conditions (NACK or Slave Stall Timeout)
> @@ -564,13 +565,13 @@ GmbusRead (
>      Status2 = GetGmbus2Status (&GmbusStatus);
>      if (EFI_ERROR (Status2)) {
>        Status = Status2;
> -      DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus2Status() failed - %r\n", __FUNCTION__, Status));
> +      //DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus2Status() failed - %r\n", __FUNCTION__, Status));
>        goto Done;
>      }
>      if (EFI_ERROR (Status) && ((GmbusStatus & B_SA_GTTMMADR_GMBUS2_NACK_INDICATOR) == 0)) {
> -      DEBUG ((DEBUG_INFO, "Error: %a() - Unexpected behavior detected!\n", __FUNCTION__));
> -      DEBUG ((DEBUG_INFO, "The GMBUS controller did not encounter a NACK and it did not set the HW_RDY bit.\n"));
> -      DEBUG ((DEBUG_INFO, "Status = %r\n", Status));
> +      //DEBUG ((DEBUG_INFO, "Error: %a() - Unexpected behavior detected!\n", __FUNCTION__));
> +      //DEBUG ((DEBUG_INFO, "The GMBUS controller did not encounter a NACK and it did not set the HW_RDY bit.\n"));
> +      //DEBUG ((DEBUG_INFO, "Status = %r\n", Status));
>        Status = EFI_DEVICE_ERROR;
>        goto Done;
>      }
> @@ -580,10 +581,10 @@ GmbusRead (
>        // If a NACK or Slave Stall Timeout occurs, then a bus error has occurred.
>        // In the event of a bus error, one must reset the GMBUS controller to resume normal operation.
>        //
> -      DEBUG ((DEBUG_INFO, "Error: %a() - NACK occurred during read operation.\n", __FUNCTION__));
> +      //DEBUG ((DEBUG_INFO, "Error: %a() - NACK occurred during read operation.\n", __FUNCTION__));
>        Status = GmbusRecoverError ();
>        if (EFI_ERROR (Status)) {
> -        DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRecoverError() failed - %r\n", __FUNCTION__, Status));
> +        //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRecoverError() failed - %r\n", __FUNCTION__, Status));
>          goto Done;
>        }
>        Status = EFI_DEVICE_ERROR;
> @@ -594,7 +595,7 @@ GmbusRead (
>      //
>      Status = GetGmbus3Data (&GmbusData);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus3Data() failed - %r\n", __FUNCTION__, Status));
> +      //DEBUG ((DEBUG_INFO, "Error: %a() - GetGmbus3Data() failed - %r\n", __FUNCTION__, Status));
>        goto Done;
>      }
>      for (Index = 0; (Index < sizeof (UINT32)) && (BytesRead < (*ByteCount)); Index++) {
> @@ -608,7 +609,7 @@ GmbusRead (
>    //
>    Status = GmbusWaitForReady (B_SA_GTTMMADR_GMBUS2_BUS_ACTIVE, FALSE);
>    if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusWaitForReady() failed - %r\n", __FUNCTION__, Status));
>      return Status;
>    }
>  
> @@ -616,7 +617,7 @@ Done:
>    Status2 = GmbusRelease ();
>    if (EFI_ERROR (Status2)) {
>      Status = Status2;
> -    DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRelease() failed - %r\n", __FUNCTION__, Status));
> +    //DEBUG ((DEBUG_INFO, "Error: %a() - GmbusRelease() failed - %r\n", __FUNCTION__, Status));
>    }
>    GmbusResetBusMaster ();
>  
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
> index 1a31c98347db..51eeadd75af9 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortProtocol.c
> @@ -7,9 +7,7 @@
>  **/
>  
>  #include <Base.h>
> -#include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
> -#include <Library/PcdLib.h>
>  #include <Library/TimerLib.h>
>  
>  #include <IgfxI2c.h>
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
> index 9d69c0365795..d92b8d262793 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplDxe.c
> @@ -22,6 +22,11 @@ RaiseTplForI2cDebugPortAccess (
>    VOID
>    )
>  {
> +  // DebugLibSerialPort exposes potential DEBUG bugs, such as early assertions
> +  if (gBS == NULL) {
> +    return;
> +  }
> +
>    if (EfiGetCurrentTpl () < TPL_NOTIFY) {
>      mPreviousTpl = gBS->RaiseTPL (TPL_NOTIFY);
>    }
> @@ -37,6 +42,10 @@ RestoreTplAfterI2cDebugPortAccess (
>    VOID
>    )
>  {
> +  if (gBS == NULL) {
> +    return;
> +  }
> +
>    if (mPreviousTpl > 0) {
>      gBS->RestoreTPL (mPreviousTpl);
>      mPreviousTpl = 0;
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
> new file mode 100644
> index 000000000000..7aa95157d734
> --- /dev/null
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cDebugPortTplRuntimeDxe.c
> @@ -0,0 +1,93 @@
> +/** @file
> +  Serial I/O Port library implementation for the HDMI I2C Debug Port
> +  DXE Library implementation
> +
> +Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +STATIC EFI_TPL    mPreviousTpl        = 0;
> +STATIC EFI_EVENT  mEvent              = NULL;
> +STATIC UINT8      mEndOfBootServices  = 0;
> +
> +/**
> +  Exit Boot Services Event notification handler.
> +
> +  @param[in]  Event     Event whose notification function is being invoked
> +  @param[in]  Context   Pointer to the notification function's context
> +
> +**/
> +VOID
> +EFIAPI
> +OnExitBootServices (
> +  IN      EFI_EVENT                 Event,
> +  IN      VOID                      *Context
> +  )
> +{
> +  mEndOfBootServices = 1;
> +
> +  gBS->CloseEvent (Event);
> +}
> +
> +/**
> +  For boot phases that utilize task priority levels (TPLs), this function raises
> +  the TPL to the appriopriate level needed to execute I/O to the I2C Debug Port
> +**/
> +VOID
> +RaiseTplForI2cDebugPortAccess (
> +  VOID
> +  )
> +{
> +  // DebugLibSerialPort exposes potential DEBUG bugs, such as early assertions
> +  if (gBS == NULL || mEndOfBootServices == 1) {
> +    return;
> +  }
> +
> +  // An event is required for a boolean to bypass TPL modification after
> +  // exit-BS. RSC obviates this, requiring it for runtime DebugLibSerialPort.
> +  // - Consider creating a TplRuntimeDxe, although UefiRuntimeLib uses gST?
> +  // - BootScriptExecutorDxe is a special-case, where booleans are ineffective
> +  //
> +  // A constructor would cycle, SerialPortInitialize() takes no arguments,
> +  // and no BootServicesTableLib can be called by AutoGen early enough.
> +  // Therefore, we generate the event here.
> +  if (mEvent == NULL) {
> +    gBS->CreateEventEx (
> +           EVT_NOTIFY_SIGNAL,
> +           TPL_NOTIFY,
> +           OnExitBootServices,
> +           NULL,
> +           &gEfiEventExitBootServicesGuid,
> +           &mEvent
> +           );
> +  }
> +
> +  if (EfiGetCurrentTpl () < TPL_NOTIFY) {
> +    mPreviousTpl = gBS->RaiseTPL (TPL_NOTIFY);
> +  }
> +}
> +
> +/**
> +  For boot phases that utilize task priority levels (TPLs), this function
> +  restores the TPL to the previous level after I/O to the I2C Debug Port is
> +  complete
> +**/
> +VOID
> +RestoreTplAfterI2cDebugPortAccess (
> +  VOID
> +  )
> +{
> +  if (gBS == NULL || mEndOfBootServices == 1) {
> +    return;
> +  }
> +
> +  if (mPreviousTpl > 0) {
> +    gBS->RestoreTPL (mPreviousTpl);
> +    mPreviousTpl = 0;
> +  }
> +}
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
> index 89a01b868da3..402b5a3033a9 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/I2cHdmiDebugSerialPortLib.c
> @@ -7,10 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>  
>  #include <Base.h>
> -#include <Library/BaseLib.h>
>  #include <Library/SerialPortLib.h>
> -#include <Library/PcdLib.h>
> -#include <Library/TimerLib.h>
>  
>  #include <IgfxI2c.h>
>  #include <I2cDebugPortProtocol.h>
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
> index b1273c7a5d10..886351ad4297 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/IgfxI2c.c
> @@ -7,8 +7,6 @@
>  **/
>  
>  #include <Uefi.h>
> -#include <Library/BaseLib.h>
> -#include <Library/IoLib.h>
>  #include <Library/PciLib.h>
>  #include <IgfxI2c.h>
>  
> @@ -84,7 +82,6 @@ GetGmbusBusPinPair (
>          default:
>            return EFI_INVALID_PARAMETER;
>        }
> -      break;

I realize that the break is redundant because all possible paths above here return. Still, it is best practice to keep it just in case someone adds a new case to the nested switch statement in the future that does not contain a return.

>      // The PCH design lineage from newer CoffeeLake & WhiskeyLake
>      case PchTypeCnlLp:
>      case PchTypeCnlH:
> @@ -105,8 +102,8 @@ GetGmbusBusPinPair (
>          default:
>            return EFI_INVALID_PARAMETER;
>        }
> -      break;

I realize that the break is redundant because all possible paths above here return. Still, it is best practice to keep it just in case someone adds a new case to the nested switch statement in the future that does not contain a return.

> -  }
>  
> -  return EFI_UNSUPPORTED;
> +    default:
> +      return EFI_UNSUPPORTED;
> +  }
>  }
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
> index c99821367354..05b2d31bbfc2 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.c
> @@ -10,7 +10,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Base.h>
>  #include <PiPei.h>
>  
> -#include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/SerialPortLib.h>
>  #include <Library/HobLib.h>
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
> index 62b3cd3e1e49..64d8f682b786 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
> @@ -21,10 +21,11 @@
>  #
>  
>  [LibraryClasses]
> -  BaseLib
>    BaseMemoryLib
>    PcdLib
> +  HobLib
>    TimerLib
> +  IoLib
>    PciLib
>  
>  [Packages]
> @@ -42,8 +43,6 @@
>    IgfxI2c.c
>    IgfxI2c.h
>  
> -[Ppis]
> -
>  [Guids]
>    gI2cHdmiDebugHobGuid
>  
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
> new file mode 100644
> index 000000000000..eefe85f2814c
> --- /dev/null
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
> @@ -0,0 +1,51 @@
> +### @file
> +# Component description file for Serial I/O Port library for the HDMI I2C Debug Port
> +#
> +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = RuntimeDxeI2cHdmiDebugSerialPortLib
> +  FILE_GUID                      = 08891D97-994C-48E9-9983-E99D622D32C8
> +  VERSION_STRING                 = 1.0
> +  MODULE_TYPE                    = DXE_DRIVER
> +  LIBRARY_CLASS                  = SerialPortLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64
> +#
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  PcdLib
> +  TimerLib
> +  IoLib
> +  PciLib
> +  UefiLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  KabylakeOpenBoardPkg/OpenBoardPkg.dec
> +
> +[Sources]
> +  DxeSmmI2cHdmiDebugSerialPortLib.c
> +  Gmbus.c
> +  Gmbus.h
> +  I2cDebugPortProtocol.c
> +  I2cDebugPortProtocol.h
> +  I2cDebugPortTplRuntimeDxe.c
> +  I2cHdmiDebugSerialPortLib.c
> +  IgfxI2c.c
> +  IgfxI2c.h
> +
> +[Guids]
> +  gEfiEventExitBootServicesGuid                                           ## CONSUMES  ## Event
> +
> +[Pcd]
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
> index 416114d4363c..e4a65a66d9c1 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.c
> @@ -8,7 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
>  
>  #include <Base.h>
> -#include <Library/BaseLib.h>
>  #include <Library/SerialPortLib.h>
>  #include <Library/PcdLib.h>
>  
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
> index 3ae724926fdd..a9780decd1a7 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
> @@ -21,10 +21,9 @@
>  #
>  
>  [LibraryClasses]
> -  BaseLib
>    BaseMemoryLib
> -  PcdLib
>    TimerLib
> +  IoLib
>    PciLib
>  
>  [Packages]
> @@ -42,10 +41,6 @@
>    IgfxI2c.h
>    SecI2cHdmiDebugSerialPortLib.c
>  
> -[Ppis]
> -
> -[Guids]
> -
>  [Pcd]
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
> index dcbf43b886c1..65f2b4f5f731 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
> @@ -21,10 +21,10 @@
>  #
>  
>  [LibraryClasses]
> -  BaseLib
>    BaseMemoryLib
>    PcdLib
>    TimerLib
> +  IoLib
>    PciLib
>  
>  [Packages]
> @@ -42,10 +42,6 @@
>    IgfxI2c.c
>    IgfxI2c.h
>  
> -[Ppis]
> -
> -[Guids]
> -
>  [Pcd]
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel    ## CONSUMES
>    gKabylakeOpenBoardPkgTokenSpaceGuid.PcdGttMmAddress                     ## CONSUMES
> -- 
> 2.37.2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build
  2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build Benjamin Doron
@ 2022-09-09 23:09   ` Nate DeSimone
  0 siblings, 0 replies; 10+ messages in thread
From: Nate DeSimone @ 2022-09-09 23:09 UTC (permalink / raw)
  To: Benjamin Doron, devel@edk2.groups.io
  Cc: Chiu, Chasel, Chaganty, Rangasai V, Oram, Isaac W, Sinha, Ankit

Hi Benjamin,

Please see comments below inline.

Thanks,
Nate

> -----Original Message-----
> From: Benjamin Doron <benjamin.doron00@gmail.com>
> Sent: Tuesday, September 6, 2022 10:27 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH v1 3/3]
> KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to
> build
> 
> HDMI port can be used with I2cHdmiDebugSerialPortLib, for debugging in all
> phases.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Ankit Sinha <ankit.sinha@intel.com>
> Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
> ---
>  .../AspireVn7Dash572G/OpenBoardPkg.dsc        | 85 +++++++++++++++----
>  .../AspireVn7Dash572G/OpenBoardPkg.fdf        | 11 ++-
>  .../AspireVn7Dash572G/OpenBoardPkgPcd.dsc     | 54 ++++++++++--
>  3 files changed, 121 insertions(+), 29 deletions(-)
> 
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
> index 261f141056f5..c71b7169a38a 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
> @@ -25,9 +25,10 @@
>    #
>    # Debug logging
>    #
> +  DEFINE USE_HDMI_DEBUG_PORT  = FALSE

This build flag is redundant with PcdI2cHdmiDebugPortEnable. Please remove it.

>    DEFINE USE_PEI_SPI_LOGGING  = FALSE
>    DEFINE USE_MEMORY_LOGGING   = FALSE
> -  DEFINE RELEASE_LOGGING      = ($(USE_PEI_SPI_LOGGING) || $(USE_MEMORY_LOGGING))
> +  DEFINE RELEASE_LOGGING      = ($(USE_HDMI_DEBUG_PORT) || $(USE_PEI_SPI_LOGGING) || $(USE_MEMORY_LOGGING))

This will turn into:

!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortEnable == TRUE
  DEFINE RELEASE_LOGGING      = TRUE
!else
  DEFINE RELEASE_LOGGING      = ($(USE_PEI_SPI_LOGGING) || $(USE_MEMORY_LOGGING))
!endif

>    DEFINE TESTING              = TRUE
>  
>    PLATFORM_NAME                               = $(PLATFORM_PACKAGE)
> @@ -205,6 +206,15 @@
>    #######################################
>    DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
>  
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE

All of these would be replaced with:

!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortEnable == TRUE

> +  DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> +
> +  #######################################
> +  # Board-specific/Silicon Package
> +  #######################################
> +  SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SecI2cHdmiDebugSerialPortLib.inf
> +!endif
> +
>    #######################################
>    # Platform Package
>    #######################################
> @@ -277,7 +287,7 @@
>    # Edk2 Packages
>    #######################################
>  # In-memory logging may require too many services for early core debug output
> -!if $(USE_MEMORY_LOGGING) == TRUE
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)

!if ($(USE_MEMORY_LOGGING) == TRUE || gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortEnable == TRUE

>    DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>  !endif
>  
> @@ -285,7 +295,7 @@
>    #######################################
>    # Edk2 Packages
>    #######################################
> -!if $(USE_MEMORY_LOGGING) == TRUE
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
>    DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>  !endif
>  
> @@ -322,7 +332,7 @@
>    #######################################
>    # Edk2 Packages
>    #######################################
> -!if $(USE_MEMORY_LOGGING) == TRUE
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
>    DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>  !endif
>  
> @@ -336,7 +346,7 @@
>    # Edk2 Packages
>    #######################################
>  # In-memory logging may require too many services for early core debug output
> -!if $(USE_MEMORY_LOGGING) == TRUE
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
>    DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>  !endif
>  
> @@ -344,7 +354,7 @@
>    #######################################
>    # Edk2 Packages
>    #######################################
> -!if $(USE_MEMORY_LOGGING) == TRUE
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
>    DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>  !endif
>  
> @@ -363,7 +373,21 @@
>    TestPointCheckLib|$(PLATFORM_PACKAGE)/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf
>  !endif
>  
> -# TODO: DebugLib override for UEFI_DRIVER and UEFI_APPLICATION?
> +[LibraryClasses.common.UEFI_DRIVER]
> +  #######################################
> +  # Edk2 Packages
> +  #######################################
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
> +  DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> +!endif
> +
> +[LibraryClasses.common.UEFI_APPLICATION]
> +  #######################################
> +  # Edk2 Packages
> +  #######################################
> +!if ($(USE_MEMORY_LOGGING) == TRUE || $(USE_HDMI_DEBUG_PORT) == TRUE)
> +  DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
> +!endif
>  
>  # TODO: Add and improve feature support
>  #######################################
> @@ -389,6 +413,9 @@
>  !if $(USE_MEMORY_LOGGING) == TRUE
>        SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/PeiSerialPortLibMem.inf
>  !endif
> +!endif
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/PeiI2cHdmiDebugSerialPortLib.inf
>  !endif
>      <PcdsFixedAtBuild>
>        gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(RELEASE_LOGGING)
> @@ -517,14 +544,24 @@
>    #######################################
>    # Edk2 Packages
>    #######################################
> +  MdeModulePkg/Core/Dxe/DxeMain.inf {
> +    <LibraryClasses>
> +      # Can debug CpuExceptionHandlerLib
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
> +!endif
> +  }

I recommend doing this LibraryClass override more generally using a [LibraryClasses.common.DXE_CORE] instead of overriding this specific instance of DxeMain.

>    MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
>      <LibraryClasses>
>        DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
>  !if $(USE_MEMORY_LOGGING) == TRUE
>        SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/DxeSerialPortLibMem.inf
> +!endif
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/RuntimeDxeI2cHdmiDebugSerialPortLib.inf
>  !endif
>      <PcdsFixedAtBuild>
> -      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(USE_MEMORY_LOGGING)
> +      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|($(USE_MEMORY_LOGGING) || $(USE_HDMI_DEBUG_PORT))
>        gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|512
>    }
>    # TODO: Still requires a little more thought
> @@ -533,9 +570,12 @@
>        DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
>  !if $(USE_MEMORY_LOGGING) == TRUE
>        SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/SmmSerialPortLibMem.inf
> +!endif
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
>  !endif
>      <PcdsFixedAtBuild>
> -      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|$(USE_MEMORY_LOGGING)
> +      gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|($(USE_MEMORY_LOGGING) || $(USE_HDMI_DEBUG_PORT))
>        gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|512
>    }
>    MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> @@ -548,12 +588,24 @@
>    MdeModulePkg/Universal/BdsDxe/BdsDxe.inf {
>      <LibraryClasses>
>        NULL|BoardModulePkg/Library/BdsPs2KbcLib/BdsPs2KbcLib.inf
> +!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
> +      NULL|BoardModulePkg/Library/BdsSerialPortTerminalLib/BdsSerialPortTerminalLib.inf
> +!endif

In the most recent version of my patch series, this has been replaced with the more generic:

!if gMinPlatformPkgTokenSpaceGuid.PcdSerialTerminalEnable == TRUE
      NULL|MinPlatformPkg/Library/SerialPortTerminalLib/SerialPortTerminalLib.inf
!endif

Please do the same here.

>    }
> +!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
> +  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
> +    <LibraryClasses>
> +      DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
> +  }
> +  MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
> +!endif
> +
>    UefiCpuPkg/CpuDxe/CpuDxe.inf {
>      <LibraryClasses>
> -!if $(USE_MEMORY_LOGGING) == TRUE
> -# TODO/TEST
> -#      SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/DxeSerialPortLibMem.inf
> +      # Can debug CpuExceptionHandlerLib
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +      SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/DxeI2cHdmiDebugSerialPortLib.inf
>  !endif
>    }
>  
> @@ -589,12 +641,9 @@
>      <PcdsPatchableInModule>
>        gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80080046
>      <LibraryClasses>
> -      !if $(TARGET) == DEBUG
> -        DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> -      !endif
> -!if $(USE_MEMORY_LOGGING) == TRUE
> -# TODO/TEST
> -#      SerialPortLib|MdeModulePkg/Library/PeiDxeSerialPortLibMem/SmmSerialPortLibMem.inf
> +        # Can debug CpuExceptionHandlerLib
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +        SerialPortLib|$(PLATFORM_BOARD_PACKAGE)/Library/I2cHdmiDebugSerialPortLib/SmmI2cHdmiDebugSerialPortLib.inf
>  !endif
>    }
>  !endif
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
> index 9eb37e7d230e..864d5561d7d8 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.fdf
> @@ -353,6 +353,10 @@ INF  MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutputDxe.inf
>  INF  BoardModulePkg/LegacySioDxe/LegacySioDxe.inf
>  INF  MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
>  INF  MdeModulePkg/Bus/Isa/Ps2MouseDxe/Ps2MouseDxe.inf
> +!if gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable == TRUE
> +  INF  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
> +  INF  MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
> +!endif

With the new PcdSerialTerminalEnable this should not be necessary anymore. MinPlatformPkg handles this with the included FDF files now.

>  INF  BoardModulePkg/BoardBdsHookDxe/BoardBdsHookDxe.inf
>  
>  INF  ShellPkg/Application/Shell/Shell.inf
> @@ -584,12 +588,7 @@ INF  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
>  
>  !if gMinPlatformPkgTokenSpaceGuid.PcdBootToShellOnly == FALSE
>  
> -INF $(PLATFORM_SI_PACKAGE)/Hsti/Dxe/HstiSiliconDxe.inf
> -
> -!endif
> -
> -!if gMinPlatformPkgTokenSpaceGuid.PcdBootToShellOnly == FALSE
> -
> +INF  $(PLATFORM_SI_PACKAGE)/Hsti/Dxe/HstiSiliconDxe.inf
>  INF  $(PLATFORM_PACKAGE)/Hsti/HstiIbvPlatformDxe/HstiIbvPlatformDxe.inf
>  
>  !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
> diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
> index a9d531a269a5..a4ea524e26bc 100644
> --- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
> +++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkgPcd.dsc
> @@ -228,7 +228,7 @@
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>  !else
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x0
> -  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x3
> +  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x03
>  !endif # $(RELEASE_LOGGING)
>  !else
>    # FIXME: More than just compiler optimisation is hooked to DEBUG builds.
> @@ -264,6 +264,8 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE
>  !if $(TARGET) == RELEASE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
> +  # Determine RTS/CTS requirement

There is no flow control in the HDMI DDC case since the I2C bus does not implement any flow control mechanisms. Therefore I think we can consider this comment resolved and always set the PCD to FALSE.

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
>  !else
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl|FALSE
> @@ -407,7 +409,37 @@
>    #  3: DDC channel C
>    #  4: DDC channel D
>    # @Prompt DDC I2C channel to claim as the HDMI debug port
> -  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel|0x00  #@todo - Set to correct value for VN7-572G
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortDdcI2cChannel|0x02
> +
> +  ## Enable usage the HDMI DDC channel as a debug port - Causes the BIOS debug log
> +  #  to be written to the HDMI DDC channel.
> +  #  The value is defined as below.
> +  #  FALSE: Do NOT use the HDMI DDC channel as a debug port
> +  #  TRUE:  Use the HDMI DDC channel as a debug port
> +  # @Prompt Enable usage the HDMI DDC channel as a debug port
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortEnable|$(USE_HDMI_DEBUG_PORT)
> +
> +  ## Enable usage the HDMI DDC channel as a serial terminal - Enables usage of the
> +  #  HDMI DDC channel to display BIOS Setup, UEFI Shell, etc. using a terminal
> +  #  emulator. Useful for cases where video is not operating correctly.
> +  #
> +  #  The value is defined as below.
> +  #  FALSE: Do NOT use the HDMI DDC channel as a debug port
> +  #  TRUE:  Use the HDMI DDC channel as a debug port
> +  # @Prompt Enable usage the HDMI DDC channel as a debug port
> +  gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable|FALSE

When you rebase up to latest, don't forget to include this from my most recent patch series:
gMinPlatformPkgTokenSpaceGuid.PcdSerialTerminalEnable|gKabylakeOpenBoardPkgTokenSpaceGuid.PcdI2cHdmiDebugPortSerialTerminalEnable

> +
> +  ## Indicates the type of terminal to use.
> +  #  If PcdI2cHdmiDebugPortSerialTerminalEnable is TRUE, this PCD will be used
> +  #  to determine which terminal protocol to use.
> +  #  0 - PCANSI
> +  #  1 - VT100
> +  #  2 - VT100+
> +  #  3 - UTF8
> +  #  4 - TTYTERM
> +  # @Prompt Default Terminal Type.
> +  # @ValidRange 0x80000001 | 0 - 4
> +  gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|3
>  
>  [PcdsFixedAtBuild.IA32]
>    ######################################
> @@ -433,7 +465,16 @@
>    ######################################
>    # Edk2 Configuration
>    ######################################
> -  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80000046  # 0x804800C7/0x806A15CF give useful information, but is very noisy
> +  # TODO: Dynamic in HII

I have converted this TODO into a proper Bugzilla and assigned it to you:

https://bugzilla.tianocore.org/show_bug.cgi?id=4056

Please delete this comment.

> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80000046
> +!if FALSE
> +  # Filtered DEBUG_POOL, DEBUG_PAGE, DEBUG_GCD and DEBUG_CACHE
> +  # - Unused: DEBUG_VARIABLE, DEBUG_BM and DEBUG_LOADFILE
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x804950CF
> +!endif
> +!if ($(TESTING) == TRUE && $(USE_HDMI_DEBUG_PORT) == FALSE)
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80400046
> +!endif
>  
>    ######################################
>    # Silicon Configuration
> @@ -446,9 +487,9 @@
>    # Platform Configuration
>    ######################################
>  !if $(TARGET) == DEBUG
> -  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|1
> +  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|TRUE
>  !else
> -  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|0
> +  gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable|FALSE
>  !endif
>  
>  [PcdsDynamicDefault]
> @@ -528,6 +569,9 @@
>  !else
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|5 # Variable: L"Timeout"
>  !endif
> +!if $(USE_HDMI_DEBUG_PORT) == TRUE
> +  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|15 # Variable: L"Timeout"
> +!endif
>  !if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
>    gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
> -- 
> 2.37.2

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-09-09 23:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-06 17:26 [edk2-devel][edk2-platforms][PATCH v1 0/3] Benjamin Doron
2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 1/3] MinPlatformPkg,WhiskeylakeOpenBoardPkg/SecFspWrapperPlatformSecLib: First BoardInitLib Benjamin Doron
2022-09-09 14:38   ` Isaac Oram
2022-09-09 17:47     ` Nate DeSimone
2022-09-09 23:09   ` Nate DeSimone
2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 2/3] [WIP] KabylakeOpenBoardPkg/I2cHdmiDebugSerialPortLib: Commit local Benjamin Doron
2022-09-09 23:09   ` Nate DeSimone
2022-09-06 17:26 ` [edk2-devel][edk2-platforms][PATCH v1 3/3] KabylakeOpenBoardPkg/AspireVn7Dash572G: Hook-up HDMI debug port to build Benjamin Doron
2022-09-09 23:09   ` Nate DeSimone
2022-09-09 23:09 ` [edk2-devel][edk2-platforms][PATCH v1 0/3] Nate DeSimone

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox