From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.20764.1665583138849527789 for ; Wed, 12 Oct 2022 06:58:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=pdXRibeS; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id CF40120F0F6A; Wed, 12 Oct 2022 06:58:57 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com CF40120F0F6A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1665583138; bh=knP5tm2cAhhv6FZafMEURR7mJRMRm2CnH3lSAVA5t7M=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=pdXRibeSmSyHTpMS33ECE8g4XsqNeZSyKgnnw8fk2qRCXEXsYKLeBNYfvGtE2Rus1 YdVCL8lh40FJ/7LPxCSL+9m1xVVlMSw6oqmCTnPHBopVZ8rJfHNjPSbPnbNn6ged/9 cpZggBiRO4Viyk/2QKFknM6xbITrFuob+PHVizx4= Message-ID: Date: Wed, 12 Oct 2022 09:58:57 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/3] CoffeelakeSiliconPkg: Fix invalid debug macros To: devel@edk2.groups.io, nathaniel.l.desimone@intel.com Cc: "Chiu, Chasel" , "Chaganty, Rangasai V" References: <20221005040648.4139-1-mikuback@linux.microsoft.com> <20221005040648.4139-2-mikuback@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Thanks, I took the approach that no one seemed to miss bad/missing information. I'll send a v2 with these suggestions. On 10/11/2022 8:37 PM, Nate DeSimone wrote: > Hi Michael, > > Please see feedback inline. > > Thanks, > Nate > >> -----Original Message----- >> From: 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 ; Chaganty, Rangasai V >> >> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/3] CoffeelakeSiliconPkg: >> Fix invalid debug macros >> >> From: Michael Kubacki >> >> 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 >> Cc: Sai Chaganty >> Signed-off-by: Michael Kubacki >> --- >> 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 > > > > >