Do cpu-endian MMIO accessors exist?

Do cpu-endian MMIO accessors exist?

[Pekka Paalanen]

Hi,

This is for Nouveau, where the hardware is configured to have the
same endianess as the cpu. Well, it's configured BE for PPC and
LE otherwise.

I'm looking for to drop the following kind of #ifdefs:

#if defined(__powerpc__)
static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
{
        struct drm_nouveau_private *dev_priv = dev->dev_private;
        return in_be32((void __force __iomem *)dev_priv->mmio->handle + reg);
}
#else
static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
{
        struct drm_nouveau_private *dev_priv = dev->dev_private;
        return readl((void __force __iomem *)dev_priv->mmio->handle + reg);
}
#endif

Please, don't mind about the other ugliness, that will be fixed.

Do cpu-endian MMIO accessors exist? What should I use, or do I just
have to use #ifdef __BIG_ENDIAN?


Thanks.
Please, Cc me.

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: Do cpu-endian MMIO accessors exist?

[Jiri Slaby]

On 07/21/2009 10:42 PM, Pekka Paalanen wrote:
> This is for Nouveau, where the hardware is configured to have the
> same endianess as the cpu. Well, it's configured BE for PPC and
> LE otherwise.

But not only ppc is BE.

> Do cpu-endian MMIO accessors exist? What should I use, or do I just
> have to use #ifdef __BIG_ENDIAN?

There is __raw_readl which doesn't switch bytes.

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Tuesday 21 July 2009, Jiri Slaby wrote:
> > Do cpu-endian MMIO accessors exist? What should I use, or do I just
> > have to use #ifdef __BIG_ENDIAN?
> 
> There is __raw_readl which doesn't switch bytes.

__raw_readl is not a replacement for real accessors like
ioread32 or in_be32, because it does not synchronize with the
instruction stream.

The best solution would be if you could find a way to set
the hardware into little-endian mode on all architectures.
>From the description, it sounds like the hardware should allow that.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Christoph Hellwig]

On Tue, Jul 21, 2009 at 11:15:49PM +0200, Arnd Bergmann wrote:
> The best solution would be if you could find a way to set
> the hardware into little-endian mode on all architectures.
> >From the description, it sounds like the hardware should allow that.

Why would you want to do that?  That just means a useless byteswap.
We really should have a generic native-endian MMIO-access API as there
is quite a bit of hardware with features like that, and currently we
have a miriad of hacks using __raw_* and manual barriers, the ppc
specific accessors and god knows what.

Re: Do cpu-endian MMIO accessors exist?

[Jiri Slaby]

On 07/21/2009 11:15 PM, Arnd Bergmann wrote:
> __raw_readl is not a replacement for real accessors like
> ioread32 or in_be32, because it does not synchronize with the
> instruction stream.

Aha, I see now. I guess it's a bug that ioread/write* on sh are not with
barriers?

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Tuesday 21 July 2009, Christoph Hellwig wrote:
> Why would you want to do that?  That just means a useless byteswap.
> We really should have a generic native-endian MMIO-access API as there
> is quite a bit of hardware with features like that, and currently we
> have a miriad of hacks using __raw_* and manual barriers, the ppc
> specific accessors and god knows what.

The byte swap on powerpc I/O instructions is practically free
on all the interesting CPUs, and on the others it is still
swamped by the overhead of the synchronization. If you care
about the latency of MMIO instructions, going to explicit
synchronization would help much more, saving hundreds of
cycles per I/O rather than one cycle for a saved byte swap.

The powerpc in_le32 style functions are a completely different
story, they are basically defined to operate only on on-chip
components, while ioread32 and readl do work on PCI devices.

No portable code should ever use the __raw_* functions and
architecture specific barriers.

It would of course be easy to just define an API extension
to ioread along the lines of

#ifdef __BIG_ENDIAN
#define ioread16_native ioread16be
#define ioread32_native ioread32be
#define iowrite16_native iowrite16be
#define iowrite32_native iowrite32be
#else
#define ioread16_native ioread16
#define ioread32_native ioread32
#define iowrite16_native iowrite16
#define iowrite32_native iowrite32
#endif

but I'm not yet convinced that there is a potential user that
should not just be fixed in a different way.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Tuesday 21 July 2009, Jiri Slaby wrote:
> On 07/21/2009 11:15 PM, Arnd Bergmann wrote:
> > __raw_readl is not a replacement for real accessors like
> > ioread32 or in_be32, because it does not synchronize with the
> > instruction stream.
> 
> Aha, I see now. I guess it's a bug that ioread/write* on sh are not with
> barriers?

That depends on how that architecture defines its bus interface.
On many simple architectures, you do not need any synchronization
operations.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Jiri Slaby]

On 07/22/2009 12:05 AM, Arnd Bergmann wrote:
> On Tuesday 21 July 2009, Jiri Slaby wrote:
>> I guess it's a bug that ioread/write* on sh are not with
>> barriers?
> 
> That depends on how that architecture defines its bus interface.
> On many simple architectures, you do not need any synchronization
> operations.

No, I should have written this explicitly. I meant read* have a barrier,
whereas ioread* do not. Similarly for writes. Is this expected?

For example:
#define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
#define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
#define ioread32(a)    __raw_readl(a)

Re: Do cpu-endian MMIO accessors exist?

[Jiri Slaby]

On 07/22/2009 12:05 AM, Arnd Bergmann wrote:
> On Tuesday 21 July 2009, Jiri Slaby wrote:
>> I guess it's a bug that ioread/write* on sh are not with
>> barriers?
> 
> That depends on how that architecture defines its bus interface.
> On many simple architectures, you do not need any synchronization
> operations.

No, I should have written this explicitly. I meant read* have a barrier,
whereas ioread* do not. Similarly for writes. Is this expected?

For example:
#define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
#define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
#define ioread32(a)    __raw_readl(a)

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Jiri Slaby wrote:
> No, I should have written this explicitly. I meant read* have a barrier,
> whereas ioread* do not. Similarly for writes. Is this expected?
> 
> For example:
> #define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
> #define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
> #define ioread32(a)    __raw_readl(a)

No, this looks like a bug. I would have expected

#define ioread32(a)    readl(a)

in this case. Also, ioread32 should actually multiplex between
readl() and inl() based on the address token, as the code in
lib/iomap.c does. It's probably easy enough to enable
CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
macros from arch/sh/include/asm/io.h.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Jiri Slaby wrote:
> No, I should have written this explicitly. I meant read* have a barrier,
> whereas ioread* do not. Similarly for writes. Is this expected?
> 
> For example:
> #define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
> #define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
> #define ioread32(a)    __raw_readl(a)

No, this looks like a bug. I would have expected

#define ioread32(a)    readl(a)

in this case. Also, ioread32 should actually multiplex between
readl() and inl() based on the address token, as the code in
lib/iomap.c does. It's probably easy enough to enable
CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
macros from arch/sh/include/asm/io.h.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Alan Cox]

On Wed, 22 Jul 2009 10:35:49 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 22 July 2009, Jiri Slaby wrote:
> > No, I should have written this explicitly. I meant read* have a barrier,
> > whereas ioread* do not. Similarly for writes. Is this expected?
> > 
> > For example:
> > #define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
> > #define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
> > #define ioread32(a)    __raw_readl(a)
> 
> No, this looks like a bug. I would have expected
> 
> #define ioread32(a)    readl(a)
> 
> in this case. Also, ioread32 should actually multiplex between
> readl() and inl() based on the address token, as the code in
> lib/iomap.c does. It's probably easy enough to enable
> CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> macros from arch/sh/include/asm/io.h.

If your platform is purely MMIO based then ioread32 and readl can become
the same thing, which is much more efficient. Even if you have port based
devices that are mapped as MMIO surely its more efficient to do the
relevant address tweaking in the iomap not in the read ?

Re: Do cpu-endian MMIO accessors exist?

[Alan Cox]

On Wed, 22 Jul 2009 10:35:49 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 22 July 2009, Jiri Slaby wrote:
> > No, I should have written this explicitly. I meant read* have a barrier,
> > whereas ioread* do not. Similarly for writes. Is this expected?
> > 
> > For example:
> > #define __raw_readl(a) (__chk_io_ptr(a), *(volatile u32 __force *)(a))
> > #define readl(a)       ({ u32 r_ = __raw_readl(a); mb(); r_; })
> > #define ioread32(a)    __raw_readl(a)
> 
> No, this looks like a bug. I would have expected
> 
> #define ioread32(a)    readl(a)
> 
> in this case. Also, ioread32 should actually multiplex between
> readl() and inl() based on the address token, as the code in
> lib/iomap.c does. It's probably easy enough to enable
> CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> macros from arch/sh/include/asm/io.h.

If your platform is purely MMIO based then ioread32 and readl can become
the same thing, which is much more efficient. Even if you have port based
devices that are mapped as MMIO surely its more efficient to do the
relevant address tweaking in the iomap not in the read ?

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Alan Cox wrote:
> 
> > in this case. Also, ioread32 should actually multiplex between
> > readl() and inl() based on the address token, as the code in
> > lib/iomap.c does. It's probably easy enough to enable
> > CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> > macros from arch/sh/include/asm/io.h.
> 
> If your platform is purely MMIO based then ioread32 and readl can become
> the same thing, which is much more efficient. Even if you have port based
> devices that are mapped as MMIO surely its more efficient to do the
> relevant address tweaking in the iomap not in the read ?

I did check that the architecture in question (sh) cannot do this,
because it actually implements board specific PIO functions in
arch/sh/boards/mach-*/io.c.

For architectures that don't need such hacks, I fully agree.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Alan Cox wrote:
> 
> > in this case. Also, ioread32 should actually multiplex between
> > readl() and inl() based on the address token, as the code in
> > lib/iomap.c does. It's probably easy enough to enable
> > CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> > macros from arch/sh/include/asm/io.h.
> 
> If your platform is purely MMIO based then ioread32 and readl can become
> the same thing, which is much more efficient. Even if you have port based
> devices that are mapped as MMIO surely its more efficient to do the
> relevant address tweaking in the iomap not in the read ?

I did check that the architecture in question (sh) cannot do this,
because it actually implements board specific PIO functions in
arch/sh/boards/mach-*/io.c.

For architectures that don't need such hacks, I fully agree.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Pekka Paalanen]

On Wed, 22 Jul 2009 00:01:59 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 21 July 2009, Christoph Hellwig wrote:
> > Why would you want to do that?  That just means a useless byteswap.
> > We really should have a generic native-endian MMIO-access API as there
> > is quite a bit of hardware with features like that, and currently we
> > have a miriad of hacks using __raw_* and manual barriers, the ppc
> > specific accessors and god knows what.

My motivation is to clean up nouveau code, and I was first thinking the
same as Christoph.

Arnd is right, there is an easy way to switch the card's endianess,
in theory. I hear there are few cases where it doesn't work yet.

> The byte swap on powerpc I/O instructions is practically free
> on all the interesting CPUs, and on the others it is still
> swamped by the overhead of the synchronization. If you care
> about the latency of MMIO instructions, going to explicit
> synchronization would help much more, saving hundreds of
> cycles per I/O rather than one cycle for a saved byte swap.

I figured something like that while reading the ppc implementation.
Therefore having the card always LE is not much different from setting
it to cpu endianness. I'll let the Nouveau developers decide which
way to do it, and keep it as is for now.

> The powerpc in_le32 style functions are a completely different
> story, they are basically defined to operate only on on-chip
> components, while ioread32 and readl do work on PCI devices.

So what should I use on BE arches when the card is in BE mode?
Not out_be32() but iowrite32be()?
I never even noticed that io*be() functions exist, thanks for the hint.

On LE arches I'll stick to {read,write}[bwl], which in my
understanding always handle the hardware as LE. Right?

> No portable code should ever use the __raw_* functions and
> architecture specific barriers.

Yeah, it felt like that, hence I asked here.

> It would of course be easy to just define an API extension
> to ioread along the lines of
> 
> #ifdef __BIG_ENDIAN
> #define ioread16_native ioread16be
> #define ioread32_native ioread32be
> #define iowrite16_native iowrite16be
> #define iowrite32_native iowrite32be
> #else
> #define ioread16_native ioread16
> #define ioread32_native ioread32
> #define iowrite16_native iowrite16
> #define iowrite32_native iowrite32
> #endif
> 
> but I'm not yet convinced that there is a potential user that
> should not just be fixed in a different way.

This is how it currently is.


Thanks.

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Pekka Paalanen wrote:
> > The powerpc in_le32 style functions are a completely different
> > story, they are basically defined to operate only on on-chip
> > components, while ioread32 and readl do work on PCI devices.
> 
> So what should I use on BE arches when the card is in BE mode?
> Not out_be32() but iowrite32be()?
> I never even noticed that io*be() functions exist, thanks for the hint.

If it's a PCI/AGP/PCIe device, use iowrite32be(), otherwise it
may not work correctly on a pseries, celleb or qs20 machine.

If it's connected over a different bus (IOIF on PS3), out_be32
would be more appropriate, but AFAICT, iowrite32be should work
just as well.

> On LE arches I'll stick to {read,write}[bwl], which in my
> understanding always handle the hardware as LE. Right?

Right. For consistency you could decide to switch to iowrite32()
to go along with iowrite32be(), that should be equivalent on
MMIO based devices to writel.

> > It would of course be easy to just define an API extension
> > to ioread along the lines of
> > 
> > #ifdef __BIG_ENDIAN
> > #define ioread16_native ioread16be
> > #define ioread32_native ioread32be
> > #define iowrite16_native iowrite16be
> > #define iowrite32_native iowrite32be
> > #else
> > #define ioread16_native ioread16
> > #define ioread32_native ioread32
> > #define iowrite16_native iowrite16
> > #define iowrite32_native iowrite32
> > #endif
> > 
> > but I'm not yet convinced that there is a potential user that
> > should not just be fixed in a different way.
> 
> This is how it currently is.

Well, I meant we could add it to asm-generic/iomap.h. If we
decide that this is the right approach after all, it should
be part of the common linux API, not private to a single driver.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Arnd Bergmann]

On Wednesday 22 July 2009, Pekka Paalanen wrote:
> > The powerpc in_le32 style functions are a completely different
> > story, they are basically defined to operate only on on-chip
> > components, while ioread32 and readl do work on PCI devices.
> 
> So what should I use on BE arches when the card is in BE mode?
> Not out_be32() but iowrite32be()?
> I never even noticed that io*be() functions exist, thanks for the hint.

If it's a PCI/AGP/PCIe device, use iowrite32be(), otherwise it
may not work correctly on a pseries, celleb or qs20 machine.

If it's connected over a different bus (IOIF on PS3), out_be32
would be more appropriate, but AFAICT, iowrite32be should work
just as well.

> On LE arches I'll stick to {read,write}[bwl], which in my
> understanding always handle the hardware as LE. Right?

Right. For consistency you could decide to switch to iowrite32()
to go along with iowrite32be(), that should be equivalent on
MMIO based devices to writel.

> > It would of course be easy to just define an API extension
> > to ioread along the lines of
> > 
> > #ifdef __BIG_ENDIAN
> > #define ioread16_native ioread16be
> > #define ioread32_native ioread32be
> > #define iowrite16_native iowrite16be
> > #define iowrite32_native iowrite32be
> > #else
> > #define ioread16_native ioread16
> > #define ioread32_native ioread32
> > #define iowrite16_native iowrite16
> > #define iowrite32_native iowrite32
> > #endif
> > 
> > but I'm not yet convinced that there is a potential user that
> > should not just be fixed in a different way.
> 
> This is how it currently is.

Well, I meant we could add it to asm-generic/iomap.h. If we
decide that this is the right approach after all, it should
be part of the common linux API, not private to a single driver.

	Arnd <><

Re: Do cpu-endian MMIO accessors exist?

[Pekka Paalanen]

On Wed, 22 Jul 2009 23:19:12 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> > > It would of course be easy to just define an API extension
> > > to ioread along the lines of
> > > 
> > > #ifdef __BIG_ENDIAN
> > > #define ioread16_native ioread16be
> > > #define ioread32_native ioread32be
> > > #define iowrite16_native iowrite16be
> > > #define iowrite32_native iowrite32be
> > > #else
> > > #define ioread16_native ioread16
> > > #define ioread32_native ioread32
> > > #define iowrite16_native iowrite16
> > > #define iowrite32_native iowrite32
> > > #endif
> > > 
> > > but I'm not yet convinced that there is a potential user that
> > > should not just be fixed in a different way.
> > 
> > This is how it currently is.
> 
> Well, I meant we could add it to asm-generic/iomap.h. If we
> decide that this is the right approach after all, it should
> be part of the common linux API, not private to a single driver.

Yes, I understood. I believe I should wait until Nouveau gets ready
to be sent upstream before proposing your extension as a real patch.


Thank you very much.

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: Do cpu-endian MMIO accessors exist?

[Paul Mundt]

On Wed, Jul 22, 2009 at 03:44:40PM +0200, Arnd Bergmann wrote:
> On Wednesday 22 July 2009, Alan Cox wrote:
> > 
> > > in this case. Also, ioread32 should actually multiplex between
> > > readl() and inl() based on the address token, as the code in
> > > lib/iomap.c does. It's probably easy enough to enable
> > > CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> > > macros from arch/sh/include/asm/io.h.
> > 
> > If your platform is purely MMIO based then ioread32 and readl can become
> > the same thing, which is much more efficient. Even if you have port based
> > devices that are mapped as MMIO surely its more efficient to do the
> > relevant address tweaking in the iomap not in the read ?
> 
> I did check that the architecture in question (sh) cannot do this,
> because it actually implements board specific PIO functions in
> arch/sh/boards/mach-*/io.c.
> 
> For architectures that don't need such hacks, I fully agree.
> 
SH doesn't really need it either, it is mostly legacy crap that was
commonly done for SuperIOs. I've tried to kill off most of it, and most
of the offenders now are in boards that very few people have available or
even use. We're at the point now where the platforms that are the worst
offenders can just be killed off, and the few that matter can be
converted. The generic iomap is a bit tricky given that most peripherals,
PCI I/O and mem space, etc. all fall under non-translatable sections of
the address space, despite being MMIO.

Re: Do cpu-endian MMIO accessors exist?

[Paul Mundt]

On Wed, Jul 22, 2009 at 03:44:40PM +0200, Arnd Bergmann wrote:
> On Wednesday 22 July 2009, Alan Cox wrote:
> > 
> > > in this case. Also, ioread32 should actually multiplex between
> > > readl() and inl() based on the address token, as the code in
> > > lib/iomap.c does. It's probably easy enough to enable
> > > CONFIG_GENERIC_IOMAP on sh, and remove the ioread*/iowrite*
> > > macros from arch/sh/include/asm/io.h.
> > 
> > If your platform is purely MMIO based then ioread32 and readl can become
> > the same thing, which is much more efficient. Even if you have port based
> > devices that are mapped as MMIO surely its more efficient to do the
> > relevant address tweaking in the iomap not in the read ?
> 
> I did check that the architecture in question (sh) cannot do this,
> because it actually implements board specific PIO functions in
> arch/sh/boards/mach-*/io.c.
> 
> For architectures that don't need such hacks, I fully agree.
> 
SH doesn't really need it either, it is mostly legacy crap that was
commonly done for SuperIOs. I've tried to kill off most of it, and most
of the offenders now are in boards that very few people have available or
even use. We're at the point now where the platforms that are the worst
offenders can just be killed off, and the few that matter can be
converted. The generic iomap is a bit tricky given that most peripherals,
PCI I/O and mem space, etc. all fall under non-translatable sections of
the address space, despite being MMIO.