[PATCH net-next] dmfe: enforce consistent timing delay.

[PATCH net-next] dmfe: enforce consistent timing delay.

[Francois Romieu]

The driver does not always use the same timing for what looks like
the same operations.

- DCR0
  Use the same udelay everywhere for reset. Upper bound is 100 us.
- DCR9
  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
  clock range.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---

I noticed those while staring at the driver.

Grant, regarding your previous remarks, I do not see where the problem
could be with posted writes MMIO accesses:
- the driver has always used the first (#0) bar register. It's hardcoded.
  The driver still uses this same bar register.
- the driver has never checked whether bar #0 was related to I/O or
  memory space. It assumed an I/O decoder and issued out* instructions.
- the DM9102 datasheet states BAR #0 is an IO only BAR. No idea for the
  DM9132, 9100 and 9009 though.

My changes have replaced in/out* accesses with ioread/write* - mostly
to help detecting places where type checking would have uncloaked a
forgotten net_device.base_addr.

Thus, if someone has a dmfe device including a MMIO space decoder behind
bar #0, iowrite* will now emit memory write instructions where the driver
previously used I/O operations. It may not work very well due to posted
writes issues, sure. However I wonder on which platform - if any - the
former MMIO space + I/O accesses (!) combo would have behaved correctly.

?

 drivers/net/ethernet/dec/tulip/dmfe.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c b/drivers/net/ethernet/dec/tulip/dmfe.c
index 0ef5b68..4d6fe60 100644
--- a/drivers/net/ethernet/dec/tulip/dmfe.c
+++ b/drivers/net/ethernet/dec/tulip/dmfe.c
@@ -767,7 +767,7 @@ static int dmfe_stop(struct DEVICE *dev)
 
 	/* Reset & stop DM910X board */
 	dw32(DCR0, DM910X_RESET);
-	udelay(5);
+	udelay(100);
 	phy_write(ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
 
 	/* free interrupt */
@@ -1601,7 +1601,9 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	int i;
 
 	dw32(DCR9, CR9_SROM_READ);
+	udelay(5);
 	dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
+	udelay(5);
 
 	/* Send the Read Command 110b */
 	srom_clk_write(ioaddr, SROM_DATA_1);
@@ -1615,6 +1617,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	}
 
 	dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
+	udelay(5);
 
 	for (i = 16; i > 0; i--) {
 		dw32(DCR9, CR9_SROM_READ | CR9_SRCS | CR9_SRCLK);
@@ -1626,6 +1629,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	}
 
 	dw32(DCR9, CR9_SROM_READ);
+	udelay(5);
 	return srom_data;
 }
 
-- 
1.7.7.6

Re: [PATCH net-next] dmfe: enforce consistent timing delay.

[Grant Grundler]

On Tue, Apr 17, 2012 at 2:11 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> The driver does not always use the same timing for what looks like
> the same operations.
>
> - DCR0
>  Use the same udelay everywhere for reset. Upper bound is 100 us.
> - DCR9
>  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
>  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
>  clock range.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>


Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

> ---
>
> I noticed those while staring at the driver.
>
> Grant, regarding your previous remarks, I do not see where the problem
> could be with posted writes MMIO accesses:
> - the driver has always used the first (#0) bar register. It's hardcoded.
>  The driver still uses this same bar register.
> - the driver has never checked whether bar #0 was related to I/O or
>  memory space. It assumed an I/O decoder and issued out* instructions.
> - the DM9102 datasheet states BAR #0 is an IO only BAR. No idea for the
>  DM9132, 9100 and 9009 though.

I didn't realize BAR0 is hard coded in the driver. BAR0 unlikely to
become MMIO for other devices. I think you are right - there is no
problem.

> My changes have replaced in/out* accesses with ioread/write* - mostly
> to help detecting places where type checking would have uncloaked a
> forgotten net_device.base_addr.
>
> Thus, if someone has a dmfe device including a MMIO space decoder behind
> bar #0, iowrite* will now emit memory write instructions where the driver
> previously used I/O operations. It may not work very well due to posted
> writes issues, sure.

unlikely. I'll worry about it when someone does in fact find this configuration.

> However I wonder on which platform - if any - the
> former MMIO space + I/O accesses (!) combo would have behaved correctly.
> ?

MMIO and I/O can interoperate to the same device since the device
registers are just having transactions sent to them via different
address decoders. Drivers typically only use one or the other though
if both are available.

thanks,
grant

>
>  drivers/net/ethernet/dec/tulip/dmfe.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c b/drivers/net/ethernet/dec/tulip/dmfe.c
> index 0ef5b68..4d6fe60 100644
> --- a/drivers/net/ethernet/dec/tulip/dmfe.c
> +++ b/drivers/net/ethernet/dec/tulip/dmfe.c
> @@ -767,7 +767,7 @@ static int dmfe_stop(struct DEVICE *dev)
>
>        /* Reset & stop DM910X board */
>        dw32(DCR0, DM910X_RESET);
> -       udelay(5);
> +       udelay(100);
>        phy_write(ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
>
>        /* free interrupt */
> @@ -1601,7 +1601,9 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        int i;
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        /* Send the Read Command 110b */
>        srom_clk_write(ioaddr, SROM_DATA_1);
> @@ -1615,6 +1617,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        for (i = 16; i > 0; i--) {
>                dw32(DCR9, CR9_SROM_READ | CR9_SRCS | CR9_SRCLK);
> @@ -1626,6 +1629,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        return srom_data;
>  }
>
> --
> 1.7.7.6
>

Re: [PATCH net-next] dmfe: enforce consistent timing delay.

[David Miller]

From: Grant Grundler <grantgrundler@gmail.com>
Date: Tue, 17 Apr 2012 14:45:38 -0700

> On Tue, Apr 17, 2012 at 2:11 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
>> The driver does not always use the same timing for what looks like
>> the same operations.
>>
>> - DCR0
>>  Use the same udelay everywhere for reset. Upper bound is 100 us.
>> - DCR9
>>  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
>>  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
>>  clock range.
>>
>> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> 
> 
> Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

Applied, thanks.