ioremap definition in generic io.h

ioremap definition in generic io.h

[Jonas Bonn]

Hi,

I'm wondering about the usefulness of the definition of ioremap and
__ioremap in asm-generic/io.h.  How is this intended to be used?  How
are the page tables for this mapping supposed to be constructed?

The definition is as follows:

/*
 * Change "struct page" to physical address.
 */
static inline void __iomem *ioremap(phys_addr_t offset, unsigned long
size)
{
        return (void __iomem*) (unsigned long)offset;
}

#define __ioremap(offset, size, flags)  ioremap(offset, size)

Regards,
Jonas

Re: ioremap definition in generic io.h

[Jiri Slaby]

On 09/29/2010 09:59 AM, Jonas Bonn wrote:
> I'm wondering about the usefulness of the definition of ioremap and
> __ioremap in asm-generic/io.h.  How is this intended to be used?  How
> are the page tables for this mapping supposed to be constructed?

Which page tables? The functions there are for machines w/o MMU with 1:1
phys:virt mapping.

regards,
-- 
js

Re: ioremap definition in generic io.h

[Jonas Bonn]

On Wed, 2010-09-29 at 12:53 +0200, Jiri Slaby wrote:
> On 09/29/2010 09:59 AM, Jonas Bonn wrote:
> > I'm wondering about the usefulness of the definition of ioremap and
> > __ioremap in asm-generic/io.h.  How is this intended to be used?  How
> > are the page tables for this mapping supposed to be constructed?
> 
> Which page tables? The functions there are for machines w/o MMU with 1:1
> phys:virt mapping.
> 

OK, thanks.  It's not clear from asm-generic/io.h that it's intended for
NOMMU systems only.  Aside from the ioremap definition, everything in
that file should be applicable generically... or am I missing something?

On another note, looking at the definitions of ioread32/iowrite32, they
imply a little-endian bus.  Some architectures (e.g. Microblaze) define
these to use host-native byte ordering instead.  Is there a _correct_
way these functions should be defined?

/Jonas

Re: ioremap definition in generic io.h

[Arnd Bergmann]

On Wednesday 29 September 2010, Jonas Bonn wrote:
> On another note, looking at the definitions of ioread32/iowrite32, they
> imply a little-endian bus.  Some architectures (e.g. Microblaze) define
> these to use host-native byte ordering instead.  Is there a correct
> way these functions should be defined?

ioread32/iowrite32 are accessor functions for PCI byte order which is
little endian. If microblaze does this differently, that is a microblaze
bug. Any code that needs big-endian I/O should use ioread32be/iowrite32be.

	Arnd

Re: ioremap definition in generic io.h

[Jonas Bonn]

On Thu, 2010-09-30 at 13:45 +0200, Arnd Bergmann wrote:
> On Wednesday 29 September 2010, Jonas Bonn wrote:
> > On another note, looking at the definitions of ioread32/iowrite32, they
> > imply a little-endian bus.  Some architectures (e.g. Microblaze) define
> > these to use host-native byte ordering instead.  Is there a correct
> > way these functions should be defined?
> 
> ioread32/iowrite32 are accessor functions for PCI byte order which is
> little endian. If microblaze does this differently, that is a microblaze
> bug. Any code that needs big-endian I/O should use ioread32be/iowrite32be.
> 

So what's the correct way to do host-native access?  For example, big
endian access on a big endian processor.  I think I'm missing something
fundamental here...

/Jonas

Re: ioremap definition in generic io.h

[Arnd Bergmann]

On Thursday 30 September 2010, Jonas Bonn wrote:
> On Thu, 2010-09-30 at 13:45 +0200, Arnd Bergmann wrote:
> > On Wednesday 29 September 2010, Jonas Bonn wrote:
> > > On another note, looking at the definitions of ioread32/iowrite32, they
> > > imply a little-endian bus.  Some architectures (e.g. Microblaze) define
> > > these to use host-native byte ordering instead.  Is there a correct
> > > way these functions should be defined?
> > 
> > ioread32/iowrite32 are accessor functions for PCI byte order which is
> > little endian. If microblaze does this differently, that is a microblaze
> > bug. Any code that needs big-endian I/O should use ioread32be/iowrite32be.
> > 
> 
> So what's the correct way to do host-native access?  For example, big
> endian access on a big endian processor.  I think I'm missing something
> fundamental here...

The I/O accessors in Linux are written under the assumption that the I/O
device is fixed endian and may be used on both kinds of CPUs.

I assume that microblaze is a special case here because you synthesize
the I/O devices together with the CPU core and choose a common endianess
for both, right?

We have the __raw_readl()/__raw_writel() functions which are defined as
host-endian, but I would not recommend using them in general because they
also mean slightly different things depending on the architecture.

It's probably best to define your own functions for microblaze. Obviously
the drivers are not portable to other architectures anyway because those
might be cross-endian.

It's probably best to name the accessors by the bus that the devices
are attached to, and then define them in a a per-bus header file, like

#ifdef CONFIG_PLB_BIG_ENDIAN
#define plb_ioread32(p) ioread32be(p)
#define plb_iowrite32(p) iowrite32be(p)
#else
#define plb_ioread32(p) ioread32(p)
#define plb_iowrite32(p) iowrite32(p)
#endif

	Arnd

Re: ioremap definition in generic io.h

[Jonas Bonn]

Hi Arnd,

Thanks for the detailed explanation... it's started to make sense now.

> I assume that microblaze is a special case here because you synthesize
> the I/O devices together with the CPU core and choose a common endianess
> for both, right?

Yeah, that's exactly the case I'm looking at.  I'm actually working on
OpenRISC and not Microblaze, but that's just a detail!  The same issues
are applicable to both architectures.

> We have the __raw_readl()/__raw_writel() functions which are defined as
> host-endian, but I would not recommend using them in general because they
> also mean slightly different things depending on the architecture.

OK, I'm curious what you mean here... I would have thought that wrapping
our own functions around these would have been the right way to do
things.  What subtleties exist here that I would need to look out for?

> It's probably best to define your own functions for microblaze. Obviously
> the drivers are not portable to other architectures anyway because those
> might be cross-endian.
> 
> It's probably best to name the accessors by the bus that the devices
> are attached to, and then define them in a a per-bus header file, like
> 
> #ifdef CONFIG_PLB_BIG_ENDIAN
> #define plb_ioread32(p) ioread32be(p)
> #define plb_iowrite32(p) iowrite32be(p)
> #else
> #define plb_ioread32(p) ioread32(p)
> #define plb_iowrite32(p) iowrite32(p)
> #endif
> 

This seems like a reasonable approach.  What got me looking at all of
this was that I wanted to use asm-generic/io.h for our architecture, and
it's mostly OK except for the definition of ioremap which implies NOMMU.
Wouldn't it make sense to drop the ioremap definitions from this file,
thus allowing it to be used by archictectures with MMU?  Or do you know
of something more in this file which prohibits it from being used more
"generically"?

Thanks for your helpful feedback!
/Jonas

Re: ioremap definition in generic io.h

[Arnd Bergmann]

On Friday 01 October 2010 10:35:50 Jonas Bonn wrote:
> > We have the __raw_readl()/__raw_writel() functions which are defined as
> > host-endian, but I would not recommend using them in general because they
> > also mean slightly different things depending on the architecture.
> 
> OK, I'm curious what you mean here... I would have thought that wrapping
> our own functions around these would have been the right way to do
> things.  What subtleties exist here that I would need to look out for?

The most common problem is synchronization. On architectures with out of order
I/O, you want the accessor functions to provide serialization against each
other and against spinlocks. The __raw_* versions are typically completely
unordered.

I would expect that most architectures with a simple I/O model like
microblaze don't have this problem though because the access is always
serialized with  the instructions around it.

> > 
> > #ifdef CONFIG_PLB_BIG_ENDIAN
> > #define plb_ioread32(p) ioread32be(p)
> > #define plb_iowrite32(p) iowrite32be(p)
> > #else
> > #define plb_ioread32(p) ioread32(p)
> > #define plb_iowrite32(p) iowrite32(p)
> > #endif
> 
> This seems like a reasonable approach.  What got me looking at all of
> this was that I wanted to use asm-generic/io.h for our architecture, and
> it's mostly OK except for the definition of ioremap which implies NOMMU.
> Wouldn't it make sense to drop the ioremap definitions from this file,
> thus allowing it to be used by archictectures with MMU?  Or do you know
> of something more in this file which prohibits it from being used more
> "generically"?

The file is certainly meant to be as generic as possible, I just didn't
consider MMU based architectures with strictly ordered I/O yet.
I would suggest the patch below to let you override the defaults.

	Arnd

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 118601f..aaa5fac 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -247,12 +247,16 @@ static inline void *phys_to_virt(unsigned long address)
 /*
  * Change "struct page" to physical address.
  */
+#ifndef ioremap
 static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
 {
 	return (void __iomem*) (unsigned long)offset;
 }
+#endif
 
+#ifndef __ioremap
 #define __ioremap(offset, size, flags)	ioremap(offset, size)
+#endif
 
 #ifndef ioremap_nocache
 #define ioremap_nocache ioremap
@@ -262,9 +266,11 @@ static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
 #define ioremap_wc ioremap_nocache
 #endif
 
+#ifndef iounmap
 static inline void iounmap(void *addr)
 {
 }
+#endif
 
 #ifndef CONFIG_GENERIC_IOMAP
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)