Revert "EmbeddedPkg/Lan9118Dxe: use MemoryFence"

Commit a4626006bbf86113 ("EmbeddedPkg/Lan9118Dxe: use MemoryFence")
replaced some stalls with memory fences, on the presumption that these
were erroneously being used to order memory accesses. However, this was
not the case.

LAN9118 devices require a timing delay between state-changing
reads/writes and subsequent reads, as updates to the register file are
asynchronous and the effects of state-changes are not immediately
visible to subsequent reads.

This delay cannot be ensured through the use of memory barriers, which
only enforce observable ordering, and not timing. Thus, converting these
stalls to memory fences was erroneous, and may result in stale values
being read.

This reverts commit a4626006bbf86113453aeb7920895e66cdd04737.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ryan Harkin <ryan.harkin@linaro.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

2 files changed, 23 insertions, 26 deletions

diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index d0bf7beefe..50644e7b7d 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
@@ -307,7 +307,8 @@ SnpInitialize (
 

   // Write the current configuration to the register

   MmioWrite32 (LAN9118_PMT_CTRL, PmConf);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

+  gBS->Stall (LAN9118_STALL);

 

   // Configure GPIO and HW

   Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);

@@ -430,7 +431,7 @@ SnpReset (
 

   // Write the current configuration to the register

   MmioWrite32 (LAN9118_PMT_CTRL, PmConf);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   // Reactivate the LEDs

   Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp);

@@ -445,7 +446,7 @@ SnpReset (
     HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer);    // assign size chosen in SnpInitialize

 

     MmioWrite32 (LAN9118_HW_CFG, HwConf);        // Write the conf

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   // Enable the receiver and transmitter and clear their contents

@@ -700,7 +701,7 @@ SnpReceiveFilters (
   // Write the options to the MAC_CSR

   //

   IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCSRValue);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   //

   // If we have to retrieve something, start packet reception.

diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c
index 3ef98ef901..bd20eebd04 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);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   // 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);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   // Wait until operation has completed

   while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY);

@@ -362,14 +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);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   // Check that device is active

   Retries = 20;

   while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Retries) {

     gBS->Stall (LAN9118_STALL);

-    MemoryFence();

   }

   if (!Retries) {

     return EFI_TIMEOUT;

@@ -379,7 +378,6 @@ Lan9118Initialize (
   Retries = 20;

   while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Retries){

     gBS->Stall (LAN9118_STALL);

-    MemoryFence();

   }

   if (!Retries) {

     return EFI_TIMEOUT;

@@ -449,12 +447,11 @@ SoftReset (
 

   // Write the configuration

   MmioWrite32 (LAN9118_HW_CFG, HwConf);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   // Wait for reset to complete

   while (MmioRead32 (LAN9118_HW_CFG) & HWCFG_SRST) {

 

-    MemoryFence();

     gBS->Stall (LAN9118_STALL);

     ResetTime += 1;

 

@@ -503,7 +500,7 @@ PhySoftReset (
 

     // Wait for completion

     while (MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PHY_RST) {

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

     }

   // PHY Basic Control Register reset

   } else if (Flags & PHY_RESET_BCR) {

@@ -511,7 +508,7 @@ PhySoftReset (
 

     // Wait for completion

     while (IndirectPHYRead32 (PHY_INDEX_BASIC_CTRL) & PHYCR_RESET) {

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

     }

   }

 

@@ -545,7 +542,7 @@ ConfigureHardware (
 

     // Write the configuration

     MmioWrite32 (LAN9118_GPIO_CFG, GpioConf);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   return EFI_SUCCESS;

@@ -588,7 +585,6 @@ AutoNegotiate (
     // Wait until it is up or until Time Out

     Retries = FixedPcdGet32 (PcdLan9118DefaultNegotiationTimeout) / LAN9118_STALL;

     while ((IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS) & PHYSTS_LINK_STS) == 0) {

-      MemoryFence();

       gBS->Stall (LAN9118_STALL);

       Retries--;

       if (!Retries) {

@@ -675,7 +671,7 @@ StopTx (
     TxCfg = MmioRead32 (LAN9118_TX_CFG);

     TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP;

     MmioWrite32 (LAN9118_TX_CFG, TxCfg);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   // Check if already stopped

@@ -694,7 +690,7 @@ StopTx (
     if (TxCfg & TXCFG_TX_ON) {

       TxCfg |= TXCFG_STOP_TX;

       MmioWrite32 (LAN9118_TX_CFG, TxCfg);

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

 

       // Wait for Tx to finish transmitting

       while (MmioRead32 (LAN9118_TX_CFG) & TXCFG_STOP_TX);

@@ -729,7 +725,7 @@ StopRx (
     RxCfg = MmioRead32 (LAN9118_RX_CFG);

     RxCfg |= RXCFG_RX_DUMP;

     MmioWrite32 (LAN9118_RX_CFG, RxCfg);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

 

     while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP);

   }

@@ -755,28 +751,28 @@ StartTx (
     TxCfg = MmioRead32 (LAN9118_TX_CFG);

     TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP;

     MmioWrite32 (LAN9118_TX_CFG, TxCfg);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   // Check if tx was started from MAC and enable if not

   if (Flags & START_TX_MAC) {

     MacCsr = IndirectMACRead32 (INDIRECT_MAC_INDEX_CR);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

     if ((MacCsr & MACCR_TX_EN) == 0) {

       MacCsr |= MACCR_TX_EN;

       IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr);

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

     }

   }

 

   // Check if tx was started from TX_CFG and enable if not

   if (Flags & START_TX_CFG) {

     TxCfg = MmioRead32 (LAN9118_TX_CFG);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

     if ((TxCfg & TXCFG_TX_ON) == 0) {

       TxCfg |= TXCFG_TX_ON;

       MmioWrite32 (LAN9118_TX_CFG, TxCfg);

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

     }

   }

 

@@ -806,14 +802,14 @@ StartRx (
       RxCfg = MmioRead32 (LAN9118_RX_CFG);

       RxCfg |= RXCFG_RX_DUMP;

       MmioWrite32 (LAN9118_RX_CFG, RxCfg);

-      MemoryFence();

+      gBS->Stall (LAN9118_STALL);

 

       while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP);

     }

 

     MacCsr |= MACCR_RX_EN;

     IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr);

-    MemoryFence();

+    gBS->Stall (LAN9118_STALL);

   }

 

   return EFI_SUCCESS;

@@ -1003,7 +999,7 @@ ChangeFifoAllocation (
   HwConf &= ~(0xF0000);

   HwConf |= ((TxFifoOption & 0xF) << 16);

   MmioWrite32 (LAN9118_HW_CFG, HwConf);

-  MemoryFence();

+  gBS->Stall (LAN9118_STALL);

 

   return EFI_SUCCESS;

 }