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 <ryan.harkin@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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;

 }