From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id EA9F581E9E for ; Thu, 19 Jan 2017 17:35:20 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 77D0F707; Thu, 19 Jan 2017 17:35:20 -0800 (PST) Received: from [192.168.236.128] (unknown [10.118.32.33]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 05AB13F3D6; Thu, 19 Jan 2017 17:35:19 -0800 (PST) To: Leif Lindholm , Ryan Harkin References: <1484782032-82342-1-git-send-email-daniil.egranov@arm.com> <20170119151307.GZ25883@bivouac.eciton.net> Cc: "edk2-devel@lists.01.org" From: Daniil Egranov Message-ID: Date: Thu, 19 Jan 2017 19:34:58 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170119151307.GZ25883@bivouac.eciton.net> X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Jan 2017 01:35:21 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Hi Leif, Ryan On 01/19/2017 09:13 AM, Leif Lindholm wrote: > On Thu, Jan 19, 2017 at 01:49:04PM +0000, Ryan Harkin wrote: >> On 18 January 2017 at 23:27, Daniil Egranov wrote: >>> The Marvell Yukon MAC address load supported only on Juno R1 and R2. >>> It disabled for Juno R0 due to PCI issues on this board. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Daniil Egranov >> Tested-by: Ryan Harkin >> >>> --- >>> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c >>> index 47ff587..e9e6990 100644 >>> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c >>> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c >>> @@ -378,6 +378,7 @@ OnEndOfDxe ( >>> EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath; >>> EFI_HANDLE Handle; >>> EFI_STATUS Status; >>> + UINT32 JunoRevision; >>> >>> // >>> // PCI Root Complex initialization >>> @@ -393,8 +394,12 @@ OnEndOfDxe ( >>> Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE); >>> ASSERT_EFI_ERROR (Status); >>> >>> - Status = ArmJunoSetNicMacAddress (); >>> - ASSERT_EFI_ERROR (Status); >>> + GetJunoRevision (JunoRevision); >>> + >>> + if (JunoRevision != JUNO_REVISION_R0) { >>> + Status = ArmJunoSetNicMacAddress (); >>> + ASSERT_EFI_ERROR (Status); >> This is just an FYI, but I stacked your patch on top of mainline, like this: >> >> 5f81f61 2017-01-18 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: >> Fixed crash on Juno R0 [Daniil Egranov] >> 19ca06b 2017-01-19 OvmfPkg: Remove superfluous return statements. >> [Thomas Huth] >> >> The first time I ran this, Juno R0 worked fine, but on R1 and R2, the >> assert triggered: >> >> UEFI firmware (version 5f81f61 built at 11:56:52 on Jan 19 2017) >> [snip] >> ASSERT_EFI_ERROR (Status = Not Found) >> ASSERT [ArmJunoDxe] >> /linaro/platforms/uefi/edk2/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c(401): >> !EFI_ERROR (Status) >> >> I worked out what is happening. And it's not to do with this patch. >> It's another fall-out from the re-work you did to the previous patch. >> It's also ultimately due to a bug the firmware. >> >> With the initial version of your "Set Marvell Yukon MAC address" >> patch, this hang didn't happen. I suspect that was because your error >> checking was weaker and certain PCIe failures didn't trigger the >> assert. >> >> To reproduce the error with this commit: >> 1) power on and boot R1 or R2 into Shell >> I do this by interrupting the boot by pressing ESCAPE and using the boot menu >> 2) At the Shell prompt, run "reset -s" to shutdown >> 3) At the ARM Boot Loader "Cmd>" prompt, run "reboot" >> 4) the board will hang while booting UEFI, assuming the board firmware >> doesn't die with constant messages like this: >> >> ERROR: PCIe CSR read failed to respond >> ERROR: SMBus transaction not claimed >> >> Assuming the problem is firmware, not EDK2, what should we do about it? > OK, so instinctively, my reaction was that "the reset -s bug is a > system controller firmware bug and we shouldn't work around > it". However, since it is actually disrupting Ryan's workflow, which > frequently doesn't touch PCI at all, I think downgrading the ASSERT to > an error message is a good idea short-term. > > Daniil - could you make that change please? > > / > Leif I've been able to reproduce "PCIe CSR read failed to respond" and "SMBus transaction not claimed" errors on my Juno R2. I disabled Marvell Yukon driver (.dsc/.fdf) and removed ArmJunoDxe patch but still see the same error messages during the initial boot. Testing motherboard interfaces (FPGA build 118)... SRAM 32MB test: PASSED LAN9118 test: PASSED KMI1/2 test: PASSED MMC test: PASSED PB/LEDs test: PASSED FPGA UART test: PASSED ERROR: PCIe CSR read failed to respond ERROR: SMBus transaction not claimed ERROR: PCIe CSR read failed to respond ... Once it went through reporting these errors, the UEFI starts loading but still fails in OnEndOfDxe(): ASSERT_EFI_ERROR (Status = Not Found) ASSERT [ArmJunoDxe] /home/user/workspace/juno/uefi/edk2/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c(110): !EFI_ERROR (Status) This is the original ArmJunoDxe code: Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE); ASSERT_EFI_ERROR (Status); <---- line 110 but it actually fails here first: Status = gBS->LocateDevicePath (&gEfiPciRootBridgeIoProtocolGuid, &PciRootComplexDevicePath, &Handle); Ryan - could you try to remove Marvell patches and check if you also catching "PCIe .." and "SMBus .." errors without them and your build still fails with other ASSERTs related to PCI. Leif - in my tests, if it fails with "PCIe .." and "SMBus ..", the UEFI PCI enumeration is completely corrupted. These errors do not appear if board was reset with the nPBRESET button. It never fails with "reset -w" and "reset -c". I also loaded Debian and used the shutdown command from there and got the same "PCIe .." and "SMBus .." errors after "reboot" command from the "Cmd>" prompt. Possibly, the "reboot" command from the board shell prompt doesnot reset the board correctly so it looks like a firmware issue. Thanks, Daniil >> Prior to your "Set Marvell Yukon MAC address" patch, or with the >> earlier version, the board would boot anyway, but the Yukon device >> would be missing. >> >> Now it dies. >> >> I don't know which is worse, but I think hanging is worse than an >> ethernet port dropping out. Although hanging is a bit more obvious >> that there's a problem... >> >> >>> + } >>> } >>> >>> STATIC >>> -- >>> 2.7.4 >>> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel