Re: ioremap definition in generic io.h

Arnd Bergmann
2010-10-01T08:43:38Z

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)