On 8 February 2016 at 19:36, Laszlo Ersek lersek@redhat.com 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 ryan.harkin@linaro.org

---

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