On 9 February 2016 at 14:25, Ard Biesheuvel <[email protected]> wrote:
> On 9 February 2016 at 12:41, Ryan Harkin <[email protected]> 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]>
>
> Reviewed-by: Ard Biesheuvel <[email protected]>
>
OK, not so fast. As I remarked in #linaro-enterprise as well, there
are a few instances where we may need both the MemoryFence() *and* a
small delay (please see below)
The problem here is that I don't have access to h/w specs, so I'd
prefer to err on the side of caution.
>> ---
>> 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();
>> }
Here we may have woken it from a reduced power state.
>> 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();
>> }
This may take 100 microseconds, as per the comment, so having a
Stall() here is reasonable, although it makes sense to use a constant
here.
>> // 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();
Another loop, but in this case, it is reasonable to hammer on the
actual MMIO register rather than on the generic timer counter register
to check whether the delay has expired, so this Stall() can be
removed.
>> }
>> }
>>
>> @@ -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);
This needs a MemoryFence() as well
>> }
>> @@ -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);
... and here
>> }
>>
>> 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
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
