comments below: On 02/09/16 12:41, 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 > > 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();
You removed two Stall() calls, but only added one MemoryFence(). Why? Just kidding. :) I won't try to review this patch in depth without actually knowing how to program the device, but the idea seems clear, from skimming the patch. Acked-by: Laszlo Ersek <[email protected]> Thanks Laszlo > > // 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; > } > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
