Re: [edk2-devel] [edk2-platforms][PATCH v1 1/3] CoffeelakeSiliconPkg: Fix invalid debug macros

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Michael,

Please see feedback inline.

Thanks,
Nate

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Tuesday, October 4, 2022 9:07 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/3] CoffeelakeSiliconPkg:
> Fix invalid debug macros
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4095
> 
> Updates several debug macros in CoffeelakeSiliconPkg to correctly match
> print specifiers to actual arguments.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  Silicon/Intel/CoffeelakeSiliconPkg/Cpu/Library/PeiCpuPolicyLib/CpuPrintPolicy.c                                       | 2 +-
>  Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiDxeSmmGbeMdiLib/GbeMdiLib.c                                         | 2 +-
>  Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiOcWdtLib/PeiOcWdtLib.c                                              | 4 ++--
>  Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/Private/PeiDxeSmmPchPciExpressHelpersLib/PchPciExpressHelpersLibrary.c | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Cpu/Library/PeiCpuPolicyLib/CpuPrintPolicy.c b/Silicon/Intel/CoffeelakeSiliconPkg/Cpu/Library/PeiCpuPolicyLib/CpuPrintPolicy.c
> index 38cf383e8da2..2e50068ba193 100644
> --- a/Silicon/Intel/CoffeelakeSiliconPkg/Cpu/Library/PeiCpuPolicyLib/CpuPrintPolicy.c
> +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Cpu/Library/PeiCpuPolicyLib/CpuPrintPolicy.c
> @@ -161,7 +161,7 @@ CpuPidTestConfigPrint (
>  {
>    UINT32 Index = 0;
>    DEBUG ((DEBUG_INFO, "------------------ CPU PID Test Config ------------------\n"));
> -  DEBUG ((DEBUG_INFO, " CPU_PID_TEST_CONFIG : PidTuning : 0x%X\n", Index,  CpuPidTestConfig->PidTuning));
> +  DEBUG ((DEBUG_INFO, " CPU_PID_TEST_CONFIG : PidTuning : 0x%X\n", CpuPidTestConfig->PidTuning));
>    if ( CpuPidTestConfig->PidTuning == 1) {
>      for (Index = PID_DOMAIN_KP; Index <= PID_DOMAIN_KD; Index++) {
>          DEBUG ((DEBUG_INFO, " CPU_PID_TEST_CONFIG : Ratl[%X] : 0x%X\n", Index,  CpuPidTestConfig->Ratl[Index]));
> diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiDxeSmmGbeMdiLib/GbeMdiLib.c b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiDxeSmmGbeMdiLib/GbeMdiLib.c
> index e5aa10de3b7b..7df011269af5 100644
> --- a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiDxeSmmGbeMdiLib/GbeMdiLib.c
> +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiDxeSmmGbeMdiLib/GbeMdiLib.c
> @@ -335,7 +335,7 @@ GbeMdiGetLanPhyRevision (
>        Status = EFI_DEVICE_ERROR;
>        goto PHY_EXIT;
>      }
> -    DEBUG ((DEBUG_INFO, "GbeMdiGetLanPhyRevision failed to read Revision. Overriding LANPHYPC\n", Status));
> +    DEBUG ((DEBUG_INFO, "GbeMdiGetLanPhyRevision failed to read Revision. Overriding LANPHYPC.\n"));

That does not seem to be what the original author intended. I suspect this is the intent:

DEBUG ((DEBUG_INFO, "GbeMdiGetLanPhyRevision failed to read Revision. Overriding LANPHYPC. Status: %r\n", Status));

>      //
>      // Taking over LANPHYPC
>      // 1. SW signal override - 1st cycle.
> diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiOcWdtLib/PeiOcWdtLib.c b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiOcWdtLib/PeiOcWdtLib.c
> index 22f6fb215fcc..e2014f97e58c 100644
> --- a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiOcWdtLib/PeiOcWdtLib.c
> +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/PeiOcWdtLib/PeiOcWdtLib.c
> @@ -71,7 +71,7 @@ OcWdtResetCheck (
>    /// Timeout status bits are cleared by writing '1'
>    ///
>    if (Readback & (B_ACPI_IO_OC_WDT_CTL_ICCSURV_STS | B_ACPI_IO_OC_WDT_CTL_NO_ICCSURV_STS)) {
> -    DEBUG ((DEBUG_ERROR, "(WDT) Expiration detected.\n", Readback));
> +    DEBUG ((DEBUG_ERROR, "(WDT) Expiration detected. Read back = 0x%08x\n", Readback));
>      Readback |= B_ACPI_IO_OC_WDT_CTL_FAILURE_STS;
>      Readback |= (B_ACPI_IO_OC_WDT_CTL_ICCSURV_STS | B_ACPI_IO_OC_WDT_CTL_NO_ICCSURV_STS);
>      Readback &= ~(B_ACPI_IO_OC_WDT_CTL_UNXP_RESET_STS);
> @@ -98,7 +98,7 @@ OcWdtResetCheck (
>        ///
>        /// No WDT expiration and no unexpected reset - clear Failure status
>        ///
> -      DEBUG ((DEBUG_INFO, "(WDT) Status OK.\n", Readback));
> +      DEBUG ((DEBUG_INFO, "(WDT) Status OK.\n"));
>        Readback &= ~(B_ACPI_IO_OC_WDT_CTL_FAILURE_STS);
>        Readback |= (B_ACPI_IO_OC_WDT_CTL_ICCSURV_STS | B_ACPI_IO_OC_WDT_CTL_NO_ICCSURV_STS);
>      }
> diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/Private/PeiDxeSmmPchPciExpressHelpersLib/PchPciExpressHelpersLibrary.c b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/Private/PeiDxeSmmPchPciExpressHelpersLib/PchPciExpressHelpersLibrary.c
> index dcb43285b73e..c55fa4efe188 100644
> --- a/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/Private/PeiDxeSmmPchPciExpressHelpersLib/PchPciExpressHelpersLibrary.c
> +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Pch/Library/Private/PeiDxeSmmPchPciExpressHelpersLib/PchPciExpressHelpersLibrary.c
> @@ -1800,7 +1800,7 @@ RecursiveIoApicCheck (
>    IoApicPresent = FALSE;
>  
>    if (IsIoApicDevice (SbdfToBase (Sbdf))) {
> -    DEBUG ((DEBUG_INFO, "IoApicFound @%x:%x:%x:%x\n", Sbdf.Bus, Sbdf.Dev, Sbdf.Func));
> +    DEBUG ((DEBUG_INFO, "IoApicFound @%x:%x:%x\n", Sbdf.Bus, Sbdf.Dev, Sbdf.Func));

That does not seem to be what the original author intended. I suspect this is the intent:

DEBUG ((DEBUG_INFO, "IoApicFound @%x:%x:%x:%x\n", Sbdf.Seg, Sbdf.Bus, Sbdf.Dev, Sbdf.Func));

>      return TRUE;
>    }
>    if (HasChildBus (Sbdf, &ChildSbdf)) {
> -- 
> 2.28.0.windows.1