diff mbox

[edk2,1/4] Revert "EmbeddedPkg/Lan9118Dxe: use MemoryFence"

Message ID 1462555149-18136-2-git-send-email-mark.rutland@arm.com
State Accepted
Commit 28f52b9fae9feba369ff0d773e0b0e610c0aa6f8
Headers show

Commit Message

Mark Rutland May 6, 2016, 5:19 p.m. UTC 
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: Ard Biesheuvel <ard.biesheuvel@linaro.org>
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
---
 EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c     |  9 +++---
 EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 40 +++++++++++--------------
 2 files changed, 23 insertions(+), 26 deletions(-)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index d0bf7be..50644e7 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 3ef98ef..bd20eeb 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;
 }