On Tue, Feb 09, 2016 at 11:41:02AM +0000, Ryan Harkin wrote:
> 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 exception is the Stall() in the AutoNegotiate() function. This
> stall is waiting for the link to negotiate, which may require stalling
> until it is ready. In this instance, I kept the stall, but also added a
> MemoryFence().
>
> [1] http://article.gmane.org/gmane.comp.bios.edk2.devel/7389
So, this is clearly a fix, and I think it should go in.
But as a word of warning, this could break on any platforms
predating Cortex-A15/A7, with external system caches.
However, this is technically a bug in MemoryFence() on those
platforms, and with this driver using hard-wired base addresses and
stuff I don't expect a lot of users outside of the platforms you are
looking after wourself.
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ryan Harkin <[email protected]>
> ---
> EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 9 +++---
> EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 43
> +++++++++++++------------
> 2 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> index 4de5204..79bee3f 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 9531b0b..8398c10 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,13 @@ 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;
> @@ -377,7 +377,7 @@ Lan9118Initialize (
> // Check that EEPROM isn't active
> Timeout = 20;
> while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Timeout){
> - gBS->Stall (LAN9118_STALL);
> + MemoryFence();
> }
> if (!Timeout) {
> return EFI_TIMEOUT;
> @@ -447,12 +447,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) {
>
> - gBS->Stall (LAN9118_STALL);
> + MemoryFence();
> ResetTime += 1;
>
> // If time taken exceeds 100us, then there was an error condition
> @@ -500,7 +500,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 +508,7 @@ PhySoftReset (
>
> // Wait for completion
> while (IndirectPHYRead32 (PHY_INDEX_BASIC_CTRL) & PHYCR_RESET) {
> - gBS->Stall (LAN9118_STALL);
> + MemoryFence();
> }
> }
>
> @@ -542,7 +542,7 @@ ConfigureHardware (
>
> // Write the configuration
> MmioWrite32 (LAN9118_GPIO_CFG, GpioConf);
> - gBS->Stall (LAN9118_STALL);
> + MemoryFence();
> }
>
> return EFI_SUCCESS;
> @@ -585,6 +585,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 +672,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 +691,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 +726,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 +752,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 +803,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 +1000,7 @@ ChangeFifoAllocation (
> HwConf &= ~(0xF0000);
> HwConf |= ((TxFifoOption & 0xF) << 16);
> MmioWrite32 (LAN9118_HW_CFG, HwConf);
> - gBS->Stall (LAN9118_STALL);
> + MemoryFence();
>
> return EFI_SUCCESS;
> }
> --
> 2.1.4
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
