Re: [Linaro-uefi] [PATCH v2 1/4] EmbeddedPkg/Lan9118Dxe: use MemoryFence

On 9 February 2016 at 12:41, Ryan Harkin ryan.harkin@linaro.org 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 ryan.harkin@linaro.org

Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org

---

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

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

}

2.1.4