From 28f52b9fae9feba369ff0d773e0b0e610c0aa6f8 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Fri, 6 May 2016 18:19:06 +0100 Subject: 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 Cc: Ryan Harkin Signed-off-by: Mark Rutland Contributed-under: TianoCore Contribution Agreement 1.0 Reviewed-by: Ard Biesheuvel --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 9 +++--- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 40 +++++++++++-------------- 2 files changed, 23 insertions(+), 26 deletions(-) (limited to 'EmbeddedPkg/Drivers') 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; } -- cgit v1.1