From a4626006bbf86113453aeb7920895e66cdd04737 Mon Sep 17 00:00:00 2001 From: Ryan Harkin Date: Tue, 9 Feb 2016 08:52:32 +0000 Subject: EmbeddedPkg/Lan9118Dxe: use MemoryFence When reviewing my LAN9118 driver PCD patch [1], Ard Biesheuvel noted that most calls to gBS->Stall() in this driver seem to be used to prevent timing issues between the device updating data and the host reading the values. And that replacing most of these calls with a MemoryFence() would be more robust. The only exceptions are the stalls that are enclosed inside retry loops: - in the AutoNegotiate() function. This stall is waiting for the link to negotiate, which may require stalling until it is ready. - in the Lan9118Initialize() function. These two stalls are waiting for devices and time out after a number of retries. - in the SoftReset() function. This stall is inside a loop where the comment states: "If time taken exceeds 100us, then there was an error condition" In these instances, I kept the stall, but also added a MemoryFence(). [1] http://article.gmane.org/gmane.comp.bios.edk2.devel/7389 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ryan Harkin Reviewed-by: Ard Biesheuvel --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 9 +++--- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 40 ++++++++++++++----------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c index 4de5204899..79bee3f47c 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c @@ -307,8 +307,7 @@ SnpInitialize ( // Write the current configuration to the register MmioWrite32 (LAN9118_PMT_CTRL, PmConf); - gBS->Stall (LAN9118_STALL); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Configure GPIO and HW Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp); @@ -431,7 +430,7 @@ SnpReset ( // Write the current configuration to the register MmioWrite32 (LAN9118_PMT_CTRL, PmConf); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Reactivate the LEDs Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp); @@ -446,7 +445,7 @@ SnpReset ( HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer); // assign size chosen in SnpInitialize MmioWrite32 (LAN9118_HW_CFG, HwConf); // Write the conf - gBS->Stall (LAN9118_STALL); + MemoryFence(); } // Enable the receiver and transmitter and clear their contents @@ -701,7 +700,7 @@ SnpReceiveFilters ( // Write the options to the MAC_CSR // IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCSRValue); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // // If we have to retrieve something, start packet reception. diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c index 9531b0ba2a..2ef1ecbb28 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c @@ -236,7 +236,7 @@ IndirectEEPROMRead32 ( // Write to Eeprom command register MmioWrite32 (LAN9118_E2P_CMD, EepromCmd); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Wait until operation has completed while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY); @@ -284,7 +284,7 @@ IndirectEEPROMWrite32 ( // Write to Eeprom command register MmioWrite32 (LAN9118_E2P_CMD, EepromCmd); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Wait until operation has completed while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY); @@ -362,13 +362,14 @@ Lan9118Initialize ( if (((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PM_MODE_MASK) >> 12) != 0) { DEBUG ((DEBUG_NET, "Waking from reduced power state.\n")); MmioWrite32 (LAN9118_BYTE_TEST, 0xFFFFFFFF); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } // Check that device is active Timeout = 20; while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Timeout) { gBS->Stall (LAN9118_STALL); + MemoryFence(); } if (!Timeout) { return EFI_TIMEOUT; @@ -378,6 +379,7 @@ Lan9118Initialize ( Timeout = 20; while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Timeout){ gBS->Stall (LAN9118_STALL); + MemoryFence(); } if (!Timeout) { return EFI_TIMEOUT; @@ -447,11 +449,12 @@ SoftReset ( // Write the configuration MmioWrite32 (LAN9118_HW_CFG, HwConf); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Wait for reset to complete while (MmioRead32 (LAN9118_HW_CFG) & HWCFG_SRST) { + MemoryFence(); gBS->Stall (LAN9118_STALL); ResetTime += 1; @@ -500,7 +503,7 @@ PhySoftReset ( // Wait for completion while (MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PHY_RST) { - gBS->Stall (LAN9118_STALL); + MemoryFence(); } // PHY Basic Control Register reset } else if (Flags & PHY_RESET_BCR) { @@ -508,7 +511,7 @@ PhySoftReset ( // Wait for completion while (IndirectPHYRead32 (PHY_INDEX_BASIC_CTRL) & PHYCR_RESET) { - gBS->Stall (LAN9118_STALL); + MemoryFence(); } } @@ -542,7 +545,7 @@ ConfigureHardware ( // Write the configuration MmioWrite32 (LAN9118_GPIO_CFG, GpioConf); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } return EFI_SUCCESS; @@ -585,6 +588,7 @@ AutoNegotiate ( // Wait until it is up or until Time Out TimeOut = 2000; while ((IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS) & PHYSTS_LINK_STS) == 0) { + MemoryFence(); gBS->Stall (LAN9118_STALL); TimeOut--; if (!TimeOut) { @@ -671,7 +675,7 @@ StopTx ( TxCfg = MmioRead32 (LAN9118_TX_CFG); TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } // Check if already stopped @@ -690,7 +694,7 @@ StopTx ( if (TxCfg & TXCFG_TX_ON) { TxCfg |= TXCFG_STOP_TX; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); // Wait for Tx to finish transmitting while (MmioRead32 (LAN9118_TX_CFG) & TXCFG_STOP_TX); @@ -725,7 +729,7 @@ StopRx ( RxCfg = MmioRead32 (LAN9118_RX_CFG); RxCfg |= RXCFG_RX_DUMP; MmioWrite32 (LAN9118_RX_CFG, RxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP); } @@ -751,28 +755,28 @@ StartTx ( TxCfg = MmioRead32 (LAN9118_TX_CFG); TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } // Check if tx was started from MAC and enable if not if (Flags & START_TX_MAC) { MacCsr = IndirectMACRead32 (INDIRECT_MAC_INDEX_CR); - gBS->Stall (LAN9118_STALL); + MemoryFence(); if ((MacCsr & MACCR_TX_EN) == 0) { MacCsr |= MACCR_TX_EN; IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } } // Check if tx was started from TX_CFG and enable if not if (Flags & START_TX_CFG) { TxCfg = MmioRead32 (LAN9118_TX_CFG); - gBS->Stall (LAN9118_STALL); + MemoryFence(); if ((TxCfg & TXCFG_TX_ON) == 0) { TxCfg |= TXCFG_TX_ON; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } } @@ -802,14 +806,14 @@ StartRx ( RxCfg = MmioRead32 (LAN9118_RX_CFG); RxCfg |= RXCFG_RX_DUMP; MmioWrite32 (LAN9118_RX_CFG, RxCfg); - gBS->Stall (LAN9118_STALL); + MemoryFence(); while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP); } MacCsr |= MACCR_RX_EN; IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr); - gBS->Stall (LAN9118_STALL); + MemoryFence(); } return EFI_SUCCESS; @@ -999,7 +1003,7 @@ ChangeFifoAllocation ( HwConf &= ~(0xF0000); HwConf |= ((TxFifoOption & 0xF) << 16); MmioWrite32 (LAN9118_HW_CFG, HwConf); - gBS->Stall (LAN9118_STALL); + MemoryFence(); return EFI_SUCCESS; } -- cgit v1.2.3 From b7b7fb3decf5fa5663ad34cbfc186a36d1f6a02c Mon Sep 17 00:00:00 2001 From: Ryan Harkin Date: Mon, 8 Feb 2016 14:38:05 +0000 Subject: EmbeddedPkg/Lan9118Dxe: add PCD for negotiation timeout Add a PCD for the link negotiation timeout so the platform can over-ride the default value. The code previously did 2000 iterations of the loop with a 2us stall, so the code has been changed subtly to set the number of iterations equal to the PCD value divided by the stall time. Since the stall time has not changed, the default PCD value is set at 4000 so the original behaviour is not changed. The problems were discovered when the ARM Juno Development Platform used 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 Laszlo 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. The default PCD value does not work for Juno. Setting the PCD to a larger value works 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 Reviewed-by: Ard Biesheuvel --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf | 1 + EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 2 +- EmbeddedPkg/EmbeddedPkg.dec | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf index 9e5f98b4bd..3c2246fb8a 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf @@ -51,6 +51,7 @@ [FixedPcd] gEmbeddedTokenSpaceGuid.PcdLan9118DxeBaseAddress gEmbeddedTokenSpaceGuid.PcdLan9118DefaultMacAddress + gEmbeddedTokenSpaceGuid.PcdLan9118DefaultNegotiationTimeout [Depex] TRUE diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c index 2ef1ecbb28..c57c7ceff1 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c @@ -586,7 +586,7 @@ AutoNegotiate ( // Check that link is up first if ((PhyStatus & PHYSTS_LINK_STS) == 0) { // Wait until it is up or until Time Out - TimeOut = 2000; + TimeOut = FixedPcdGet32 (PcdLan9118DefaultNegotiationTimeout) / LAN9118_STALL; while ((IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS) & PHYSTS_LINK_STS) == 0) { MemoryFence(); gBS->Stall (LAN9118_STALL); diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec index f557527281..cd0d96f79f 100644 --- a/EmbeddedPkg/EmbeddedPkg.dec +++ b/EmbeddedPkg/EmbeddedPkg.dec @@ -145,6 +145,7 @@ # LAN9118 Ethernet Driver PCDs gEmbeddedTokenSpaceGuid.PcdLan9118DxeBaseAddress|0x0|UINT32|0x00000025 gEmbeddedTokenSpaceGuid.PcdLan9118DefaultMacAddress|0x0|UINT64|0x00000026 + gEmbeddedTokenSpaceGuid.PcdLan9118DefaultNegotiationTimeout|4000|UINT32|0x00000027 # # Android FastBoot -- cgit v1.2.3 From c64aa7c4091d63d85723aac728e0690d7bf98400 Mon Sep 17 00:00:00 2001 From: Ryan Harkin Date: Tue, 9 Feb 2016 10:07:13 +0000 Subject: EmbeddedPkg/Lan9118Dxe: minor DEBUG tidyup This patch makes a few minor DEBUG output changes: - Fix typo in DEBUG output: Negociation->Negotiation - Change DEBUG occurrences of "Lan9118" to "LAN9118" to make grepping the log output easier. - Change the warning that auto-negotiation is not supported when AutoNegotiate() returns an error. The function already reports if the feature is supported or not and can also return an error for other reasons. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ryan Harkin Reviewed-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c index 79bee3f47c..d0bf7beefe 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c @@ -142,7 +142,7 @@ Lan9118DxeEntry ( // Power up the device so we can find the MAC address Status = Lan9118Initialize (Snp); if (EFI_ERROR (Status)) { - DEBUG ((EFI_D_ERROR, "Lan9118: Error initialising hardware\n")); + DEBUG ((EFI_D_ERROR, "LAN9118: Error initialising hardware\n")); return EFI_DEVICE_ERROR; } @@ -342,7 +342,7 @@ SnpInitialize ( // Do auto-negotiation if supported Status = AutoNegotiate (AUTO_NEGOTIATE_ADVERTISE_ALL, Snp); if (EFI_ERROR(Status)) { - DEBUG ((EFI_D_WARN, "Lan9118: Auto Negociation not supported.\n")); + DEBUG ((EFI_D_WARN, "LAN9118: Auto Negotiation failed.\n")); } // Configure flow control depending on speed capabilities @@ -767,7 +767,7 @@ SnpStationAddress ( New = (EFI_MAC_ADDRESS *) PermAddr; Lan9118SetMacAddress ((EFI_MAC_ADDRESS *) PermAddr, Snp); } else { - DEBUG ((EFI_D_ERROR, "Lan9118: Warning: No valid MAC address in EEPROM, using fallback\n")); + DEBUG ((EFI_D_ERROR, "LAN9118: Warning: No valid MAC address in EEPROM, using fallback\n")); New = (EFI_MAC_ADDRESS*) (FixedPcdGet64 (PcdLan9118DefaultMacAddress)); } } else { -- cgit v1.2.3 From bbff41c11fd374c7818c96f69c28b51c9caba619 Mon Sep 17 00:00:00 2001 From: Ryan Harkin Date: Tue, 9 Feb 2016 17:10:02 +0000 Subject: EmbeddedPkg/Lan9118Dxe: rename TimeOut to Retries The variable TimeOut is actually a retry, not a timeout, so I renamed the variable accordingly. This patch makes no functional change. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ryan Harkin Reviewed-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c index c57c7ceff1..3ef98ef901 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c @@ -355,7 +355,7 @@ Lan9118Initialize ( IN EFI_SIMPLE_NETWORK_PROTOCOL *Snp ) { - UINTN Timeout; + UINTN Retries; UINT64 DefaultMacAddress; // Attempt to wake-up the device if it is in a lower power state @@ -366,22 +366,22 @@ Lan9118Initialize ( } // Check that device is active - Timeout = 20; - while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Timeout) { + Retries = 20; + while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Retries) { gBS->Stall (LAN9118_STALL); MemoryFence(); } - if (!Timeout) { + if (!Retries) { return EFI_TIMEOUT; } // Check that EEPROM isn't active - Timeout = 20; - while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Timeout){ + Retries = 20; + while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Retries){ gBS->Stall (LAN9118_STALL); MemoryFence(); } - if (!Timeout) { + if (!Retries) { return EFI_TIMEOUT; } @@ -574,7 +574,7 @@ AutoNegotiate ( UINT32 PhyControl; UINT32 PhyStatus; UINT32 Features; - UINT32 TimeOut; + UINT32 Retries; // First check that auto-negotiation is supported PhyStatus = IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS); @@ -586,12 +586,12 @@ AutoNegotiate ( // Check that link is up first if ((PhyStatus & PHYSTS_LINK_STS) == 0) { // Wait until it is up or until Time Out - TimeOut = FixedPcdGet32 (PcdLan9118DefaultNegotiationTimeout) / LAN9118_STALL; + Retries = FixedPcdGet32 (PcdLan9118DefaultNegotiationTimeout) / LAN9118_STALL; while ((IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS) & PHYSTS_LINK_STS) == 0) { MemoryFence(); gBS->Stall (LAN9118_STALL); - TimeOut--; - if (!TimeOut) { + Retries--; + if (!Retries) { DEBUG ((EFI_D_ERROR, "Link timeout in auto-negotiation.\n")); return EFI_TIMEOUT; } -- cgit v1.2.3