[PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Luis R. Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

Based on advise from Jeff, as described in the e-mail:

[PATCH] Clarify pci_iomap() usage for MMIO-only devices

we shouldn't use ioread32()/iowrite32() unless we really have a need
to otherwise we're creating an unnecessary branch on read/write. Lets
revert this back to readl()/writel() as our devices so far have been
MMIO-only. This reverts 9202ec15da36ca060722c363575e0e390d85fb71 on
ath5k. This patch is intended for the ath5k branch of wireless-dev.

Changes-licensed-under: ISC
Singed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>

---

 drivers/net/wireless/ath5k_hw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath5k_hw.c b/drivers/net/wireless/ath5k_hw.c
index 07ad127..3501b4c 100644
--- a/drivers/net/wireless/ath5k_hw.c
+++ b/drivers/net/wireless/ath5k_hw.c
@@ -219,7 +219,7 @@ static inline unsigned int
ath5k_hw_clocktoh(unsigned int clock, bool turbo)
  */
 static inline u32 ath5k_hw_reg_read(struct ath_hw *hal, u16 reg)
 {
-       return ioread32(hal->ah_sh + reg);
+       return readl(hal->ah_sh + reg);
 }

 /*
@@ -227,7 +227,7 @@ static inline u32 ath5k_hw_reg_read(struct ath_hw
*hal, u16 reg)
  */
 static inline void ath5k_hw_reg_write(struct ath_hw *hal, u32 val, u16 reg)
 {
-       iowrite32(val, hal->ah_sh + reg);
+       writel(val, hal->ah_sh + reg);
 }

 /*

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ath5k-revert-ioread-write.diff --]
[-- Type: text/x-patch; name="ath5k-revert-ioread-write.diff", Size: 684 bytes --]

diff --git a/drivers/net/wireless/ath5k_hw.c b/drivers/net/wireless/ath5k_hw.c
index 07ad127..3501b4c 100644
--- a/drivers/net/wireless/ath5k_hw.c
+++ b/drivers/net/wireless/ath5k_hw.c
@@ -219,7 +219,7 @@ static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
  */
 static inline u32 ath5k_hw_reg_read(struct ath_hw *hal, u16 reg)
 {
-	return ioread32(hal->ah_sh + reg);
+	return readl(hal->ah_sh + reg);
 }
 
 /*
@@ -227,7 +227,7 @@ static inline u32 ath5k_hw_reg_read(struct ath_hw *hal, u16 reg)
  */
 static inline void ath5k_hw_reg_write(struct ath_hw *hal, u32 val, u16 reg)
 {
-	iowrite32(val, hal->ah_sh + reg);
+	writel(val, hal->ah_sh + reg);
 }
 
 /*

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Jiri Slaby]

On 09/17/2007 10:34 PM, Luis R. Rodriguez wrote:
> Based on advise from Jeff, as described in the e-mail:
> 
> [PATCH] Clarify pci_iomap() usage for MMIO-only devices
> 
> we shouldn't use ioread32()/iowrite32() unless we really have a need
> to otherwise we're creating an unnecessary branch on read/write. Lets
> revert this back to readl()/writel() as our devices so far have been
> MMIO-only. This reverts 9202ec15da36ca060722c363575e0e390d85fb71 on
> ath5k. This patch is intended for the ath5k branch of wireless-dev.
> 
> Changes-licensed-under: ISC
> Singed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>
> 
> ---
> 
>  drivers/net/wireless/ath5k_hw.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath5k_hw.c b/drivers/net/wireless/ath5k_hw.c
> index 07ad127..3501b4c 100644
> --- a/drivers/net/wireless/ath5k_hw.c
> +++ b/drivers/net/wireless/ath5k_hw.c
> @@ -219,7 +219,7 @@ static inline unsigned int
> ath5k_hw_clocktoh(unsigned int clock, bool turbo)
>   */
>  static inline u32 ath5k_hw_reg_read(struct ath_hw *hal, u16 reg)
>  {
> -       return ioread32(hal->ah_sh + reg);
> +       return readl(hal->ah_sh + reg);
>  }
> 
>  /*
> @@ -227,7 +227,7 @@ static inline u32 ath5k_hw_reg_read(struct ath_hw
> *hal, u16 reg)
>   */
>  static inline void ath5k_hw_reg_write(struct ath_hw *hal, u32 val, u16 reg)
>  {
> -       iowrite32(val, hal->ah_sh + reg);
> +       writel(val, hal->ah_sh + reg);
>  }
> 
>  /*
> 

NACK, this is wrong. iomap returns platform dependant return value, which may or
may not correspond to what is accepted by readX/writeX. (but is 100% accepted by
ioreadXX/iowriteXX).

regards,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Jeff Garzik]

Jiri Slaby wrote:
> NACK, this is wrong. iomap returns platform dependant return value, which may or

Incorrect.  readl() and writel() work just fine on all existing 
platforms where Atheros may be used.

If you want pci_iomap() usage removed as well, that's a fair request.

	Jeff

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Luis R. Rodriguez]

On 9/17/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/17/2007 10:34 PM, Luis R. Rodriguez wrote:
> > Based on advise from Jeff, as described in the e-mail:
> >
> > [PATCH] Clarify pci_iomap() usage for MMIO-only devices
> >
> > we shouldn't use ioread32()/iowrite32() unless we really have a need
> > to otherwise we're creating an unnecessary branch on read/write. Lets
> > revert this back to readl()/writel() as our devices so far have been
> > MMIO-only. This reverts 9202ec15da36ca060722c363575e0e390d85fb71 on
> > ath5k. This patch is intended for the ath5k branch of wireless-dev.
> >
> > Changes-licensed-under: ISC
> > Singed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>
> >
> > ---
> >
> >  drivers/net/wireless/ath5k_hw.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath5k_hw.c b/drivers/net/wireless/ath5k_hw.c
> > index 07ad127..3501b4c 100644
> > --- a/drivers/net/wireless/ath5k_hw.c
> > +++ b/drivers/net/wireless/ath5k_hw.c
> > @@ -219,7 +219,7 @@ static inline unsigned int
> > ath5k_hw_clocktoh(unsigned int clock, bool turbo)
> >   */
> >  static inline u32 ath5k_hw_reg_read(struct ath_hw *hal, u16 reg)
> >  {
> > -       return ioread32(hal->ah_sh + reg);
> > +       return readl(hal->ah_sh + reg);
> >  }
> >
> >  /*
> > @@ -227,7 +227,7 @@ static inline u32 ath5k_hw_reg_read(struct ath_hw
> > *hal, u16 reg)
> >   */
> >  static inline void ath5k_hw_reg_write(struct ath_hw *hal, u32 val, u16 reg)
> >  {
> > -       iowrite32(val, hal->ah_sh + reg);
> > +       writel(val, hal->ah_sh + reg);
> >  }
> >
> >  /*
> >
>
> NACK, this is wrong. iomap returns platform dependant return value, which may or
> may not correspond to what is accepted by readX/writeX. (but is 100% accepted by
> ioreadXX/iowriteXX).

Actually it does work 100% of the times if all we have is MMIO-only
devices and hence the change. I've asked around and checked some of my
really old Atheros cards and can't find any using PIO. Are you aware
of any Atheros card using PIO?

  Luis

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Jiri Slaby]

On 09/17/2007 10:59 PM, Jeff Garzik wrote:
> Jiri Slaby wrote:
>> NACK, this is wrong. iomap returns platform dependant return value,
>> which may or
> 
> Incorrect.  readl() and writel() work just fine on all existing
> platforms where Atheros may be used.

Ok, this is what Alan Cox wrote about that and you didn't reply to it, so I
thought he's right. Anyway I wouldn't rely on iomap that it will never be
changed even on x86 -- what's the (performance) impact of having ioread instead
of readl? How much data are transferred this way?

<cite from="http://lkml.org/lkml/2007/8/25/50">
On Sat, 25 Aug 2007 04:56:19 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> If the driver knows its MMIO, using readX/writeX after pci_iomap() is
> just fine, for all current implementations, and it makes sense that way.

There is nothing that guarantees this is permitted, any more than there
is anything saying not to use outb/outl. Some of the implementations do
quite strange things. It may happen to work but its not in the
documentation or the comments.

If you want to change this then you need to check the existing usages and
update all the docs if its safe, oh and tell the sparc64 pcmcia people to
take a hike, which is probably not a big problem.
</cite>

Please, can anybody clarify it?

thanks,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Luis R. Rodriguez]

On 9/17/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/17/2007 10:59 PM, Jeff Garzik wrote:
> > Jiri Slaby wrote:
> >> NACK, this is wrong. iomap returns platform dependant return value,
> >> which may or
> >
> > Incorrect.  readl() and writel() work just fine on all existing
> > platforms where Atheros may be used.
>
> Ok, this is what Alan Cox wrote about that and you didn't reply to it, so I
> thought he's right. Anyway I wouldn't rely on iomap that it will never be
> changed even on x86 -- what's the (performance) impact of having ioread instead
> of readl? How much data are transferred this way?
>
> <cite from="http://lkml.org/lkml/2007/8/25/50">
> On Sat, 25 Aug 2007 04:56:19 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
>
> > If the driver knows its MMIO, using readX/writeX after pci_iomap() is
> > just fine, for all current implementations, and it makes sense that way.
>
> There is nothing that guarantees this is permitted, any more than there
> is anything saying not to use outb/outl. Some of the implementations do
> quite strange things. It may happen to work but its not in the
> documentation or the comments.

I posted a patch to update the documentation with this.

> Please, can anybody clarify it?

Based on Alan's 'Review-by' remarks on my patch for updating the
documenation on pci_iomap() it seems this validates this patch. Please
see thread with subject:

[PATCH] Clarify pci_iomap() usage for MMIO-only devices

   Luis

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Luis R. Rodriguez]

On 9/18/07, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On 9/17/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> > On 09/17/2007 10:59 PM, Jeff Garzik wrote:
> > > Jiri Slaby wrote:
> > >> NACK, this is wrong. iomap returns platform dependant return value,
> > >> which may or
> > >
> > > Incorrect.  readl() and writel() work just fine on all existing
> > > platforms where Atheros may be used.
> >
> > Ok, this is what Alan Cox wrote about that and you didn't reply to it, so I
> > thought he's right. Anyway I wouldn't rely on iomap that it will never be
> > changed even on x86 -- what's the (performance) impact of having ioread instead
> > of readl? How much data are transferred this way?
> >
> > <cite from="http://lkml.org/lkml/2007/8/25/50">
> > On Sat, 25 Aug 2007 04:56:19 -0400
> > Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> > > If the driver knows its MMIO, using readX/writeX after pci_iomap() is
> > > just fine, for all current implementations, and it makes sense that way.
> >
> > There is nothing that guarantees this is permitted, any more than there
> > is anything saying not to use outb/outl. Some of the implementations do
> > quite strange things. It may happen to work but its not in the
> > documentation or the comments.
>
> I posted a patch to update the documentation with this.
>
> > Please, can anybody clarify it?
>
> Based on Alan's 'Review-by' remarks on my patch for updating the
> documenation on pci_iomap() it seems this validates this patch. Please
> see thread with subject:
>
> [PATCH] Clarify pci_iomap() usage for MMIO-only devices

Well spoke too soon. Now Linus disagrees. We'll see the outcome on the
other thread then.

  Luis

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Benjamin Herrenschmidt]

> > > > If the driver knows its MMIO, using readX/writeX after pci_iomap() is
> > > > just fine, for all current implementations, and it makes sense that way.
> > >
> > > There is nothing that guarantees this is permitted, any more than there
> > > is anything saying not to use outb/outl. Some of the implementations do
> > > quite strange things. It may happen to work but its not in the
> > > documentation or the comments.
> >
> > I posted a patch to update the documentation with this.
> >
> > > Please, can anybody clarify it?
> >
> > Based on Alan's 'Review-by' remarks on my patch for updating the
> > documenation on pci_iomap() it seems this validates this patch. Please
> > see thread with subject:
> >
> > [PATCH] Clarify pci_iomap() usage for MMIO-only devices
> 
> Well spoke too soon. Now Linus disagrees. We'll see the outcome on the
> other thread then.

To be more precise, a platform has every right to return some kind of
"token" from ioport_map/pci_iomap that encodes the type of address, and
that is -different- from what a normal ioremap does. In which case, you
will -not- be able to use readb/writeb & cie on such a token.

The fact that current implementations seem to return something for MMIO
that is equivalent to what ioremap returns is an accident and cannot be
relied upon.

Cheers,
Ben.

Re: [PATCH] revert ath5k ioread32()/iowrite32() usage - use readl()/writel(), we're MMIO-only

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
> To be more precise, a platform has every right to return some kind of
> "token" from ioport_map/pci_iomap that encodes the type of address, and
> that is -different- from what a normal ioremap does. In which case, you
> will -not- be able to use readb/writeb & cie on such a token.
> 
> The fact that current implementations seem to return something for MMIO
> that is equivalent to what ioremap returns is an accident and cannot be
> relied upon.


Fair enough.  It's easy enough to change ath5k to using ioremap (or 
pci_ioremap).

	Jeff