On 8 February 2016 at 19:36, Laszlo Ersek <[email protected]> wrote: > On 02/08/16 18:59, Ryan Harkin wrote: >> Add a PCD for the default link negotiation timeout so the platform can >> over-ride the default value. >> >> When the ARM Juno Development Platform uses the "EFI Network" option >> with then LAN9118 driver, it fails to boot the first time and so the >> board drops back to Shell again: >> >> Warning: LAN9118 Driver in stopped state >> Link timeout in auto-negotiation. >> Lan9118: Auto Negociation not supported. >> EhcExecTransfer: transfer failed with 2 >> EhcControlTransfer: error - Device Error, transfer - 2 >> Buffer: EFI Hard Drive >> Booting EFI Misc Device >> Booting EFI Misc Device 1 >> Booting EFI Hard Drive >> Booting EFI Network >> Warning: LAN9118 Driver not initialized >> Link timeout in auto-negotiation. >> Lan9118: Auto Negociation not supported. >> Booting EFI Internal Shell >> >> Exiting Shell drops the user back to the Intel BDS UI. Selecting >> "Continue" then succeeds in booting from the EFI Network: >> >> Booting EFI Misc Device >> Booting EFI Misc Device 1 >> Booting EFI Hard Drive >> Booting EFI Network >> ..MnpFreeTxBuf: Duplicated recycle report from SNP. >> MnpFreeTxBuf: Duplicated recycle report from SNP. >> [snip repeated errors] >> >> Discussion on the edk2-devel mailing list [1] prompted Laszo Ersek to >> suggest the time taken for the NIC to negotiate was causing a problem. >> He suggested the solution contained in this patch to provide a PCD >> configurable by the platform. >> >> Setting the PCD to 2000 seems to work for Juno R0, R1 and R2. >> >> [1] http://article.gmane.org/gmane.comp.bios.edk2.devel/7341 >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ryan Harkin <[email protected]> >> --- >> EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.h | 2 +- >> EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf | 1 + >> EmbeddedPkg/EmbeddedPkg.dec | 1 + >> 3 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.h >> b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.h >> index cc883e8..e5318da 100644 >> --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.h >> +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.h >> @@ -38,7 +38,7 @@ >> #include "Lan9118DxeUtil.h" >> #include "Lan9118DxeHw.h" >> >> -#define LAN9118_STALL 2 >> +#define LAN9118_STALL (FixedPcdGet64 >> (PcdLan9118DefaultNegotiationTimeout)) >> >> #define LAN9118_DEFAULT_MAC_ADDRL 0x00F70200 >> #define LAN9118_DEFAULT_MAC_ADDRH 0x00009040 >> diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf >> b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf >> index 9e5f98b..3c2246f 100644 >> --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf >> +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf >> @@ -51,6 +51,7 @@ [Protocols] >> [FixedPcd] >> gEmbeddedTokenSpaceGuid.PcdLan9118DxeBaseAddress >> gEmbeddedTokenSpaceGuid.PcdLan9118DefaultMacAddress >> + gEmbeddedTokenSpaceGuid.PcdLan9118DefaultNegotiationTimeout >> >> [Depex] >> TRUE >> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec >> index f557527..338bdd0 100644 >> --- a/EmbeddedPkg/EmbeddedPkg.dec >> +++ b/EmbeddedPkg/EmbeddedPkg.dec >> @@ -145,6 +145,7 @@ [PcdsFixedAtBuild.common] >> # LAN9118 Ethernet Driver PCDs >> gEmbeddedTokenSpaceGuid.PcdLan9118DxeBaseAddress|0x0|UINT32|0x00000025 >> gEmbeddedTokenSpaceGuid.PcdLan9118DefaultMacAddress|0x0|UINT64|0x00000026 >> + >> gEmbeddedTokenSpaceGuid.PcdLan9118DefaultNegotiationTimeout|2|UINT64|0x00000027 >> >> # >> # Android FastBoot >> > > (1) There's a serious problem with this patch. > > You made a typo in my name. :) >
Again!?!? At least this time it was a typo rather than me actually not knowing how to spell it. Not that it helps. > (Thanks for the credit though :)) > Thanks for the idea! > (2) I noticed LAN9118_STALL is used in a bunch of places in this driver. > Since those all use the same macro, I think it makes sense to set the > macro from a PCD for all of them too. So that's good. > > (3) gBS->Stall() takes a UINTN. I propose to make the PCD UINT32, in > order to match 32-bit builds better. > Of course, that makes sense. Even more so as I've been trying to get TC2 (32-bit) working without any luck. It's a different problem in TC2, mind you. > (4) 2000 looks like a pretty big value, which may not be appropriate for > all uses of LAN9118_STALL. OTOH that value is only mentioned in the > commit message, it's not a code change. So that's fine too I think. > Yeah, I thought I'd tune down it a bit before submitting a Juno patch to OpenPlatformPkg. I'll change the commit message to be more vague when I submit v2. > Thanks! > Laszlo "sneaky name" Ersek Thanks, Ryan "gammy hands" Harkin _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
