On 9 February 2016 at 12:04, Laszlo Ersek <[email protected]> wrote: > 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 know, when I saw that, I aged just a little faster :) > 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]> > Sure, thanks for checking and acking it. > 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
