[RFC] dove: fix __io() definition to use bus based offset

[RFC] dove: fix __io() definition to use bus based offset

[Eric Miao]

    [ARM] dove: fix __io() definition to use bus based offset

    __io() should be based on DOVE_PCIE0_IO_BUS_BASE instead of _PHYS_BASE,
    and use __typesafe_io() for consistency.

    Cc: Nicolas Pitre <nico@fluxnic.net>
    Cc: Lennert Buijtenhek <buytenh@wantstofly.org>
    Cc: Saeed Bishara <saeed@marvell.com>
    Signed-off-by: Eric Miao <eric.miao@canonical.com>

diff --git a/arch/arm/mach-dove/include/mach/io.h
b/arch/arm/mach-dove/include/mach/io.h
index 3b3e472..067435e 100644
--- a/arch/arm/mach-dove/include/mach/io.h
+++ b/arch/arm/mach-dove/include/mach/io.h
@@ -11,10 +11,9 @@

 #include "dove.h"

-#define IO_SPACE_LIMIT		0xffffffff
-
-#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
-				   DOVE_PCIE0_IO_VIRT_BASE))
-#define __mem_pci(a)		(a)
+#define IO_SPACE_LIMIT	0xffffffff
+#define __io(a)		__typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
+					    DOVE_PCIE0_IO_VIRT_BASE)
+#define __mem_pci(a)	(a)

 #endif

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Thursday 29 July 2010, Eric Miao wrote:
> diff --git a/arch/arm/mach-dove/include/mach/io.h
> b/arch/arm/mach-dove/include/mach/io.h
> index 3b3e472..067435e 100644
> --- a/arch/arm/mach-dove/include/mach/io.h
> +++ b/arch/arm/mach-dove/include/mach/io.h
> @@ -11,10 +11,9 @@
> 
>  #include "dove.h"
> 
> -#define IO_SPACE_LIMIT         0xffffffff
> -
> -#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
> -                                  DOVE_PCIE0_IO_VIRT_BASE))
> -#define __mem_pci(a)           (a)
> +#define IO_SPACE_LIMIT 0xffffffff
> +#define __io(a)                __typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
> +                                           DOVE_PCIE0_IO_VIRT_BASE)
> +#define __mem_pci(a)   (a)
> 
>  #endif
> 

The IO_SPACE_LIMIT still looks wrong, AFAICT it should be 

#define IO_SPACE_LIMIT (DOVE_PCIE0_IO_SIZE + DOVE_PCIE1_IO_SIZE - 1)

	Arnd

[RFC] dove: fix __io() definition to use bus based offset

[Eric Miao]

On Thu, Jul 29, 2010 at 11:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 29 July 2010, Eric Miao wrote:
>> diff --git a/arch/arm/mach-dove/include/mach/io.h
>> b/arch/arm/mach-dove/include/mach/io.h
>> index 3b3e472..067435e 100644
>> --- a/arch/arm/mach-dove/include/mach/io.h
>> +++ b/arch/arm/mach-dove/include/mach/io.h
>> @@ -11,10 +11,9 @@
>>
>> ?#include "dove.h"
>>
>> -#define IO_SPACE_LIMIT ? ? ? ? 0xffffffff
>> -
>> -#define __io(a) ?((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?DOVE_PCIE0_IO_VIRT_BASE))
>> -#define __mem_pci(a) ? ? ? ? ? (a)
>> +#define IO_SPACE_LIMIT 0xffffffff
>> +#define __io(a) ? ? ? ? ? ? ? ?__typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DOVE_PCIE0_IO_VIRT_BASE)
>> +#define __mem_pci(a) ? (a)
>>
>> ?#endif
>>
>
> The IO_SPACE_LIMIT still looks wrong, AFAICT it should be
>
> #define IO_SPACE_LIMIT (DOVE_PCIE0_IO_SIZE + DOVE_PCIE1_IO_SIZE - 1)
>

And it looks like PCIE1_IO space is not used as indicated in its __io() macro.

> ? ? ? ?Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Thursday 29 July 2010, Eric Miao wrote:
> On Thu, Jul 29, 2010 at 11:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>  #include "dove.h"
> >>
> >> -#define IO_SPACE_LIMIT         0xffffffff
> >> -
> >> -#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
> >> -                                  DOVE_PCIE0_IO_VIRT_BASE))
> >> -#define __mem_pci(a)           (a)
> >> +#define IO_SPACE_LIMIT 0xffffffff
> >> +#define __io(a)                __typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
> >> +                                           DOVE_PCIE0_IO_VIRT_BASE)
> >> +#define __mem_pci(a)   (a)
> >>
> >>  #endif
> >>
> >
> > The IO_SPACE_LIMIT still looks wrong, AFAICT it should be
> >
> > #define IO_SPACE_LIMIT (DOVE_PCIE0_IO_SIZE + DOVE_PCIE1_IO_SIZE - 1)
> >
> 
> And it looks like PCIE1_IO space is not used as indicated in its __io() macro.

DOVE_PCIE1_IO_VIRT_BASE directly follows (DOVE_PCIE0_IO_VIRT_BASE +
DOVE_PCIE0_IO_SIZE), so the macro is the same, isn't it?

However, I believe you also need the patch below to make thinks like
PCI-ISA bridges and VGA adapters as well as /dev/port access work.

Not-at-all-tested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
 
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -59,10 +59,10 @@ static int __init dove_pcie_setup(int nr, struct pci_sys_data *sys)
 	pp->io_space_name[sizeof(pp->io_space_name) - 1] = 0;
 	pp->res[0].name = pp->io_space_name;
 	if (pp->index == 0) {
-		pp->res[0].start = DOVE_PCIE0_IO_PHYS_BASE;
+		pp->res[0].start = DOVE_PCIE0_IO_BUS_BASE;
 		pp->res[0].end = pp->res[0].start + DOVE_PCIE0_IO_SIZE - 1;
 	} else {
-		pp->res[0].start = DOVE_PCIE1_IO_PHYS_BASE;
+		pp->res[0].start = DOVE_PCIE1_IO_BUS_BASE;
 		pp->res[0].end = pp->res[0].start + DOVE_PCIE1_IO_SIZE - 1;
 	}
 	pp->res[0].flags = IORESOURCE_IO;

[RFC] dove: fix __io() definition to use bus based offset

[Russell King - ARM Linux]

On Thu, Jul 29, 2010 at 01:45:35PM +0800, Eric Miao wrote:
> diff --git a/arch/arm/mach-dove/include/mach/io.h
> b/arch/arm/mach-dove/include/mach/io.h
> index 3b3e472..067435e 100644
> --- a/arch/arm/mach-dove/include/mach/io.h
> +++ b/arch/arm/mach-dove/include/mach/io.h
> @@ -11,10 +11,9 @@
> 
>  #include "dove.h"
> 
> -#define IO_SPACE_LIMIT		0xffffffff
> -
> -#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
> -				   DOVE_PCIE0_IO_VIRT_BASE))
> -#define __mem_pci(a)		(a)
> +#define IO_SPACE_LIMIT	0xffffffff
> +#define __io(a)		__typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
> +					    DOVE_PCIE0_IO_VIRT_BASE)

I recommend against this use of __typesafe_io():

http://lists.arm.linux.org.uk/lurker/message/20090214.154245.6325bc9d.en.html

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Saturday 31 July 2010 13:08:02 Russell King - ARM Linux wrote:
> On Thu, Jul 29, 2010 at 01:45:35PM +0800, Eric Miao wrote:
> > diff --git a/arch/arm/mach-dove/include/mach/io.h
> > b/arch/arm/mach-dove/include/mach/io.h
> > index 3b3e472..067435e 100644
> > --- a/arch/arm/mach-dove/include/mach/io.h
> > +++ b/arch/arm/mach-dove/include/mach/io.h
> > @@ -11,10 +11,9 @@
> > 
> >  #include "dove.h"
> > 
> > -#define IO_SPACE_LIMIT               0xffffffff
> > -
> > -#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
> > -                                DOVE_PCIE0_IO_VIRT_BASE))
> > -#define __mem_pci(a)         (a)
> > +#define IO_SPACE_LIMIT       0xffffffff
> > +#define __io(a)              __typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
> > +                                         DOVE_PCIE0_IO_VIRT_BASE)
> 
> I recommend against this use of __typesafe_io():
> 
> http://lists.arm.linux.org.uk/lurker/message/20090214.154245.6325bc9d.en.html
> 

On a related note, is there a particular reason why most of the
*_*_VIRT_BASE macros are just numbers instead of void __iomem
pointers?

My feeling is that we could save a lot of these typecasts and conversion
macros if the base addresses were already pointers in the right address
space.

I could do some cleanup and try to get the pointers right in the arm
architecture tree if you think that's a good idea.

	Arnd

[RFC] dove: fix __io() definition to use bus based offset

[Russell King - ARM Linux]

On Sat, Jul 31, 2010 at 09:21:40PM +0200, Arnd Bergmann wrote:
> On Saturday 31 July 2010 13:08:02 Russell King - ARM Linux wrote:
> > On Thu, Jul 29, 2010 at 01:45:35PM +0800, Eric Miao wrote:
> > > diff --git a/arch/arm/mach-dove/include/mach/io.h
> > > b/arch/arm/mach-dove/include/mach/io.h
> > > index 3b3e472..067435e 100644
> > > --- a/arch/arm/mach-dove/include/mach/io.h
> > > +++ b/arch/arm/mach-dove/include/mach/io.h
> > > @@ -11,10 +11,9 @@
> > > 
> > >  #include "dove.h"
> > > 
> > > -#define IO_SPACE_LIMIT               0xffffffff
> > > -
> > > -#define __io(a)  ((void __iomem *)(((a) - DOVE_PCIE0_IO_PHYS_BASE) +\
> > > -                                DOVE_PCIE0_IO_VIRT_BASE))
> > > -#define __mem_pci(a)         (a)
> > > +#define IO_SPACE_LIMIT       0xffffffff
> > > +#define __io(a)              __typesafe_io((a) - DOVE_PCIE0_IO_BUS_BASE + \
> > > +                                         DOVE_PCIE0_IO_VIRT_BASE)
> > 
> > I recommend against this use of __typesafe_io():
> > 
> > http://lists.arm.linux.org.uk/lurker/message/20090214.154245.6325bc9d.en.html
> > 
> 
> On a related note, is there a particular reason why most of the
> *_*_VIRT_BASE macros are just numbers instead of void __iomem
> pointers?

Let me expand a little, and then I'll answer your question.

As already covered, __io() is the macro to enable the PC-like IO port
macros, and if there's a simple 1:1 mapping, defining it to be the
__typesafe_io() function is what's required.  __typesafe_io() should
not appear anywhere else, and as I've said in the URL above, it
shouldn't be aliased to __io() with offsets.  So I wouldn't want to
see stuff using __io() nor __typesafe_io() for other stuff (such as
_VIRT_BASE macros.)

Now, when it comes to _VIRT_BASE macros, they tend to end up being
used not only in C code, but also assembly.  In C, it's useful to
have virtual addresses typed correctly - which as you say are void
__iomem pointers.  When it comes to assembly, to avoid defining a
set of additional pointers, I've suggested in the past to do this:

#ifndef __ASSEMBLY__
#define IOMEM(x)	((void __iomem __force *)(x))
#else
#define IOMEM(x)	(x)
#endif

#define FOO_VIRT_BASE	IOMEM(0xfeedface)

which gives us the behaviour we desire.  I'd ideally like IOMEM() to
be in some generic header, but I don't think asm/io.h makes much sense
for that - neither does creating a new header just for it.  What do
other architectures do for accessing system device registers (excluding
x86 which of course uses ISA IO macros)?  I wonder if somewhere under
linux/ would be appropriate for it.

There is one place in our architecture code where this goes wrong, and
that's the static map structures (struct map_desc) where the virtual
address is an unsigned long - this is because the underlying code really
wants it as a number and not a pointer (it wants to do stuff with page
tables.)  Converting this to a pointer results in casts-to-pointers
appearing elsewhere in the code, so you can't win on that.

However, for the vast majority of definitions (being registers) then
defining them to be void __iomem pointer like is definitely the right
thing.

I've toyed in the past with making the IO macros have tigher type-
checking, but I've found each time I've tried it that it causes gcc
to become less efficient with code generation, resulting in larger
kernels.

If someone can come up with a script or put the work into fixing the
integer-likeness of some of the _VIRT_BASE stuff and create a set of
reasonable patches, I'm personally all for it.

[RFC] dove: fix __io() definition to use bus based offset

[Saeed Bishara]

 
>> >> -
>> >> -#define __io(a)  ((void __iomem *)(((a) - 
>DOVE_PCIE0_IO_PHYS_BASE) +\
>> >> -                                  DOVE_PCIE0_IO_VIRT_BASE))
>> >> -#define __mem_pci(a)           (a)
>> >> +#define IO_SPACE_LIMIT 0xffffffff
>> >> +#define __io(a)                __typesafe_io((a) - 
>DOVE_PCIE0_IO_BUS_BASE + \
>> >> +                                           
>DOVE_PCIE0_IO_VIRT_BASE)
>> >> +#define __mem_pci(a)   (a)
Using DOVE_PCIE0_IO_BUS_BASE instead of DOVE_PCIE0_IO_PHYS_BASE approved by me
>> >>
>> >>  #endif
>> >>
>> >
>> > The IO_SPACE_LIMIT still looks wrong, AFAICT it should be
>> >
>> > #define IO_SPACE_LIMIT (DOVE_PCIE0_IO_SIZE + 
>DOVE_PCIE1_IO_SIZE - 1)
>> >
>> 
>> And it looks like PCIE1_IO space is not used as indicated in 
>its __io() macro.
>
>DOVE_PCIE1_IO_VIRT_BASE directly follows (DOVE_PCIE0_IO_VIRT_BASE +
>DOVE_PCIE0_IO_SIZE), so the macro is the same, isn't it?
That's right, the same macro should work for PCIE1 and PCIE0.
>
>However, I believe you also need the patch below to make thinks like
>PCI-ISA bridges and VGA adapters as well as /dev/port access work.
>
>Not-at-all-tested-by: Arnd Bergmann <arnd@arndb.de>
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
>--- a/arch/arm/mach-dove/pcie.c
>+++ b/arch/arm/mach-dove/pcie.c
>@@ -59,10 +59,10 @@ static int __init dove_pcie_setup(int nr, 
>struct pci_sys_data *sys)
> 	pp->io_space_name[sizeof(pp->io_space_name) - 1] = 0;
> 	pp->res[0].name = pp->io_space_name;
> 	if (pp->index == 0) {
>-		pp->res[0].start = DOVE_PCIE0_IO_PHYS_BASE;
>+		pp->res[0].start = DOVE_PCIE0_IO_BUS_BASE;
> 		pp->res[0].end = pp->res[0].start + 
>DOVE_PCIE0_IO_SIZE - 1;
> 	} else {
>-		pp->res[0].start = DOVE_PCIE1_IO_PHYS_BASE;
>+		pp->res[0].start = DOVE_PCIE1_IO_BUS_BASE;
> 		pp->res[0].end = pp->res[0].start + 
>DOVE_PCIE1_IO_SIZE - 1;
> 	}
> 	pp->res[0].flags = IORESOURCE_IO;
I agree also on this part.
>

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Saturday 31 July 2010, Russell King - ARM Linux wrote:
> On Sat, Jul 31, 2010 at 09:21:40PM +0200, Arnd Bergmann wrote:
> > 
> > On a related note, is there a particular reason why most of the
> > *_*_VIRT_BASE macros are just numbers instead of void __iomem
> > pointers?
> 
> Let me expand a little, and then I'll answer your question.
> 
> As already covered, __io() is the macro to enable the PC-like IO port
> macros, and if there's a simple 1:1 mapping, defining it to be the
> __typesafe_io() function is what's required.  __typesafe_io() should
> not appear anywhere else, and as I've said in the URL above, it
> shouldn't be aliased to __io() with offsets.  So I wouldn't want to
> see stuff using __io() nor __typesafe_io() for other stuff (such as
> _VIRT_BASE macros.)

Right, this absolutely makes sense and we already talked about it.
I would even go further and try to remove the definitions that
contain just

#define __io(a) __typesafe_io(a)

For platforms that have no PCI/ISA/PCMCIA support, because they
do not have support for I/O space either. The definition is present
in a number of platforms right now and lets those build device drivers
that use the inb/outb family of functions, but actually calling such
a function with a hardcoded small number like 0x3f8 for the serial
port will result in a NULL pointer exception.

Ideally those functions should only be called when CONFIG_HAS_IOPORT
is set (I've started driver patches for that) and we should
set CONFIG_NO_IOPORT when there is no support for PIO, rather than
defining a bogus mapping to low memory.

> Now, when it comes to _VIRT_BASE macros, they tend to end up being
> used not only in C code, but also assembly.  In C, it's useful to
> have virtual addresses typed correctly - which as you say are void
> __iomem pointers.  When it comes to assembly, to avoid defining a
> set of additional pointers, I've suggested in the past to do this:
> 
> #ifndef __ASSEMBLY__
> #define IOMEM(x)	((void __iomem __force *)(x))
> #else
> #define IOMEM(x)	(x)
> #endif
> 
> #define FOO_VIRT_BASE	IOMEM(0xfeedface)

Ok.

> which gives us the behaviour we desire.  I'd ideally like IOMEM() to
> be in some generic header, but I don't think asm/io.h makes much sense
> for that - neither does creating a new header just for it.  What do
> other architectures do for accessing system device registers (excluding
> x86 which of course uses ISA IO macros)?  I wonder if somewhere under
> linux/ would be appropriate for it.

>From what I've seen in other architectures, few others really use
static memory space mappings, except for the legacy ISA range that
includes the VGA memory space and the PCI/ISA IO space mapping if that
is memory mapped. Both are normally only defined in one place though,
since they are not board specific, so there is no need for a macro
to abstract that.

> There is one place in our architecture code where this goes wrong, and
> that's the static map structures (struct map_desc) where the virtual
> address is an unsigned long - this is because the underlying code really
> wants it as a number and not a pointer (it wants to do stuff with page
> tables.)  Converting this to a pointer results in casts-to-pointers
> appearing elsewhere in the code, so you can't win on that.

I was thinking about making that an anonymous union for a transition
time, like

struct map_desc {
        union {
                unsigned long virtual;
                void __iomem *virt_base;
        };
        unsigned long pfn;
        unsigned long length;
        unsigned int type;
};

Then we can do one platform at a time, moving all definitions from
integer constants to pointer constants.

When the last use of map_desc->virtual is gone, we can remove the
union again (that could be in the same patch series).

> However, for the vast majority of definitions (being registers) then
> defining them to be void __iomem pointer like is definitely the right
> thing.
> 
> I've toyed in the past with making the IO macros have tigher type-
> checking, but I've found each time I've tried it that it causes gcc
> to become less efficient with code generation, resulting in larger
> kernels.

Ok, I'll make sure that whatever I come up with doesn't regress
on code size.

	Arnd

[RFC] dove: fix __io() definition to use bus based offset

[Russell King - ARM Linux]

On Mon, Aug 02, 2010 at 12:59:58PM +0200, Arnd Bergmann wrote:
> For platforms that have no PCI/ISA/PCMCIA support, because they
> do not have support for I/O space either. The definition is present
> in a number of platforms right now and lets those build device drivers
> that use the inb/outb family of functions, but actually calling such
> a function with a hardcoded small number like 0x3f8 for the serial
> port will result in a NULL pointer exception.
> 
> Ideally those functions should only be called when CONFIG_HAS_IOPORT
> is set (I've started driver patches for that) and we should
> set CONFIG_NO_IOPORT when there is no support for PIO, rather than
> defining a bogus mapping to low memory.

Be very careful here - I don't think these mean quite what you think
they do.  They were introduced by Al Viro back in 2007 to work around
the mess that the devres stuff caused.

It's basically sorting out whether an architecture has support for
ioport_map(), and therefore whether devm_ioport_map() should be defined.

We, in ARM land, have several platforms where we support the standard
PC IO based stuff, but not the ioport_map stuff for one reason or
another.  Such cases tend to be that PC IO peripherals aren't connected
to an ISA bus, but directly onto the 32-bit system bus and this results
in them having a base offset plus a register spacing of 4 instead of 2.

Even more weird setups involve 8-bit IO peripherals having a spacing
of 4 between each register, but 16-bit IO peripherals having registers
at offset 0,1,2,3 at 0,1,4,5 on the bus - and games played to access
the odd address registers.  These don't support ioport mapping.

These platforms, Al's patch added a 'select NO_IOPORT' to - but this
does not mean they don't have ISA IO support.

> > which gives us the behaviour we desire.  I'd ideally like IOMEM() to
> > be in some generic header, but I don't think asm/io.h makes much sense
> > for that - neither does creating a new header just for it.  What do
> > other architectures do for accessing system device registers (excluding
> > x86 which of course uses ISA IO macros)?  I wonder if somewhere under
> > linux/ would be appropriate for it.
> 
> From what I've seen in other architectures, few others really use
> static memory space mappings, except for the legacy ISA range that
> includes the VGA memory space and the PCI/ISA IO space mapping if that
> is memory mapped. Both are normally only defined in one place though,
> since they are not board specific, so there is no need for a macro
> to abstract that.

Do no other architectures have system peripherals in MMIO space then?
I guess those which do have their system peripherals standardized
across all supported platforms?

> > There is one place in our architecture code where this goes wrong, and
> > that's the static map structures (struct map_desc) where the virtual
> > address is an unsigned long - this is because the underlying code really
> > wants it as a number and not a pointer (it wants to do stuff with page
> > tables.)  Converting this to a pointer results in casts-to-pointers
> > appearing elsewhere in the code, so you can't win on that.
> 
> I was thinking about making that an anonymous union for a transition
> time, like
> 
> struct map_desc {
>         union {
>                 unsigned long virtual;
>                 void __iomem *virt_base;
>         };
>         unsigned long pfn;
>         unsigned long length;
>         unsigned int type;
> };
> 
> Then we can do one platform at a time, moving all definitions from
> integer constants to pointer constants.
> 
> When the last use of map_desc->virtual is gone, we can remove the
> union again (that could be in the same patch series).

I think you missed my point.

map_desc->virtual really wants to be an integer type:

static void __init create_mapping(struct map_desc *md)
{
        if (md->virtual != vectors_base() && md->virtual < TASK_SIZE) {

        if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
            md->virtual >= PAGE_OFFSET && md->virtual < VMALLOC_END) {

        addr = md->virtual & PAGE_MASK;
}

static void __init devicemaps_init(struct machine_desc *mdesc)
{
        map.virtual = MODULES_VADDR;
        map.virtual = FLUSH_BASE;
        map.virtual = FLUSH_BASE_MINICACHE;
        map.virtual = 0xffff0000;
                map.virtual = 0;
}

all end up needing casts.  If we make MODULES_VADDR a void __iomem pointer,
then (a) it's wrong because it's not a pointer to IOMEM, and (b):

arch/arm/mm/init.c:     BUILD_BUG_ON(TASK_SIZE                          > MODULES_VADDR);
arch/arm/mm/init.c:     BUG_ON(TASK_SIZE                                > MODULES_VADDR);

would need casts to unsigned long, as would:

        for (addr = 0; addr < MODULES_VADDR; addr += PGDIR_SIZE)
                pmd_clear(pmd_off_k(addr));

and so the game of chasing casts around the kernel will continue.

static inline void map_memory_bank(struct membank *bank)
{
        map.virtual = __phys_to_virt(bank_phys_start(bank));
}

can just become phys_to_virt() - but then sparse will complain about
differing address spaces.

There's no real answer here - whatever we do, we'll end up with casts
_somewhere_ for this structure.  As the underlying code naturally wants
it to be unsigned long, I think that's the correct type for it.

Maybe the solution is to have a macro to initialize its entries, which
contain the cast to unsigned long for 'virtual' ?

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Monday 02 August 2010, Russell King - ARM Linux wrote:
> On Mon, Aug 02, 2010 at 12:59:58PM +0200, Arnd Bergmann wrote:
>
> > Ideally those functions should only be called when CONFIG_HAS_IOPORT
> > is set (I've started driver patches for that) and we should
> > set CONFIG_NO_IOPORT when there is no support for PIO, rather than
> > defining a bogus mapping to low memory.
>
> Even more weird setups involve 8-bit IO peripherals having a spacing
> of 4 between each register, but 16-bit IO peripherals having registers
> at offset 0,1,2,3 at 0,1,4,5 on the bus - and games played to access
> the odd address registers.  These don't support ioport mapping.
> 
> These platforms, Al's patch added a 'select NO_IOPORT' to - but this
> does not mean they don't have ISA IO support.

Ok, I see. It seems that some platforms are working around this by
calling the inb/outb functions from ioread/iowrite when a special
magic I/O space token is passed in, which probably would work on
those as well that currently set NO_IOPORT, but doing so would mean
more work across all architectures, so I'll leave it alone for now.

> > > which gives us the behaviour we desire.  I'd ideally like IOMEM() to
> > > be in some generic header, but I don't think asm/io.h makes much sense
> > > for that - neither does creating a new header just for it.  What do
> > > other architectures do for accessing system device registers (excluding
> > > x86 which of course uses ISA IO macros)?  I wonder if somewhere under
> > > linux/ would be appropriate for it.
> > 
> > From what I've seen in other architectures, few others really use
> > static memory space mappings, except for the legacy ISA range that
> > includes the VGA memory space and the PCI/ISA IO space mapping if that
> > is memory mapped. Both are normally only defined in one place though,
> > since they are not board specific, so there is no need for a macro
> > to abstract that.
> 
> Do no other architectures have system peripherals in MMIO space then?
> I guess those which do have their system peripherals standardized
> across all supported platforms?

I think that is mostly done on nommu architectures, but the far more
common pattern that I have seen is to use ioremap() for everything,
even if that all that does is a type conversion like __typesafe_io().

> > Then we can do one platform at a time, moving all definitions from
> > integer constants to pointer constants.
> > 
> > When the last use of map_desc->virtual is gone, we can remove the
> > union again (that could be in the same patch series).
> 
> I think you missed my point.
> 
> map_desc->virtual really wants to be an integer type:
> 
> static void __init create_mapping(struct map_desc *md)
> {
>         if (md->virtual != vectors_base() && md->virtual < TASK_SIZE) {
> 
>         if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
>             md->virtual >= PAGE_OFFSET && md->virtual < VMALLOC_END) {
> 
>         addr = md->virtual & PAGE_MASK;
> }
> 
> static void __init devicemaps_init(struct machine_desc *mdesc)
> {
>         map.virtual = MODULES_VADDR;
>         map.virtual = FLUSH_BASE;
>         map.virtual = FLUSH_BASE_MINICACHE;
>         map.virtual = 0xffff0000;
>                 map.virtual = 0;
> }

Right, I didn't realize that map_desc is used for both MT_DEVICE and
non-IO memory remapping.

> all end up needing casts.  If we make MODULES_VADDR a void __iomem pointer,
> then (a) it's wrong because it's not a pointer to IOMEM, and (b):
> 
> arch/arm/mm/init.c:     BUILD_BUG_ON(TASK_SIZE                          > MODULES_VADDR);
> arch/arm/mm/init.c:     BUG_ON(TASK_SIZE                                > MODULES_VADDR);
> 
> would need casts to unsigned long, as would:
> 
>         for (addr = 0; addr < MODULES_VADDR; addr += PGDIR_SIZE)
>                 pmd_clear(pmd_off_k(addr));
> 
> and so the game of chasing casts around the kernel will continue.
> 
> static inline void map_memory_bank(struct membank *bank)
> {
>         map.virtual = __phys_to_virt(bank_phys_start(bank));
> }
> 
> can just become phys_to_virt() - but then sparse will complain about
> differing address spaces.
> 
> There's no real answer here - whatever we do, we'll end up with casts
> _somewhere_ for this structure.  As the underlying code naturally wants
> it to be unsigned long, I think that's the correct type for it.
> 
> Maybe the solution is to have a macro to initialize its entries, which
> contain the cast to unsigned long for 'virtual' ?

Yes, that would work, though I would prefer to find a solution that works
without new macros.

In other places in the kernel, we try hard to avoid mixing __iomem pointers
with other pointers. Maybe we can change iotable_init/map_desc to only operate
on actual __iomem regions.

Since there are very few places that actually need a non-__iomem remapping
outside of mmu.c, we could slightly reorganize the code so we only
need a single cast in iotable_init(), and perhaps find a different way
to map the omap/davinci sram.

The example patch below unfortunately grows the kernel image by 56 bytes,
but also shrinks the source by a few lines and doing the same without
growing the image would require more source code.

	Arnd

---
 arch/arm/include/asm/mach/map.h |    2 +
 arch/arm/kernel/tcm.c           |   25 ++-------
 arch/arm/mm/mmu.c               |  108 +++++++++++++++++---------------------
 3 files changed, 55 insertions(+), 80 deletions(-)

diff --git a/arch/arm/include/asm/mach/map.h b/arch/arm/include/asm/mach/map.h
index 742c2aa..1f0ceb5 100644
--- a/arch/arm/include/asm/mach/map.h
+++ b/arch/arm/include/asm/mach/map.h
@@ -30,6 +30,8 @@ struct map_desc {
 
 #ifdef CONFIG_MMU
 extern void iotable_init(struct map_desc *, int);
+void __init create_mapping(unsigned long virtual, unsigned long pfn,
+			   size_t length, unsigned int mtype);
 
 struct mem_type;
 extern const struct mem_type *get_mem_type(unsigned int type);
diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
index e503038..dd9ca37 100644
--- a/arch/arm/kernel/tcm.c
+++ b/arch/arm/kernel/tcm.c
@@ -48,24 +48,6 @@ static struct resource itcm_res = {
 	.flags = IORESOURCE_MEM
 };
 
-static struct map_desc dtcm_iomap[] __initdata = {
-	{
-		.virtual	= DTCM_OFFSET,
-		.pfn		= __phys_to_pfn(DTCM_OFFSET),
-		.length		= (DTCM_END - DTCM_OFFSET + 1),
-		.type		= MT_UNCACHED
-	}
-};
-
-static struct map_desc itcm_iomap[] __initdata = {
-	{
-		.virtual	= ITCM_OFFSET,
-		.pfn		= __phys_to_pfn(ITCM_OFFSET),
-		.length		= (ITCM_END - ITCM_OFFSET + 1),
-		.type		= MT_UNCACHED
-	}
-};
-
 /*
  * Allocate a chunk of TCM memory
  */
@@ -162,7 +144,8 @@ void __init tcm_init(void)
 		setup_tcm_bank(0, DTCM_OFFSET,
 			       (DTCM_END - DTCM_OFFSET + 1) >> 10);
 		request_resource(&iomem_resource, &dtcm_res);
-		iotable_init(dtcm_iomap, 1);
+		create_mapping(DTCM_OFFSET, __phys_to_pfn(DTCM_OFFSET),
+				(DTCM_END - DTCM_OFFSET + 1), MT_UNCACHED);
 		/* Copy data from RAM to DTCM */
 		start = &__sdtcm_data;
 		end   = &__edtcm_data;
@@ -176,7 +159,9 @@ void __init tcm_init(void)
 		setup_tcm_bank(1, ITCM_OFFSET,
 			       (ITCM_END - ITCM_OFFSET + 1) >> 10);
 		request_resource(&iomem_resource, &itcm_res);
-		iotable_init(itcm_iomap, 1);
+		create_mapping(ITCM_OFFSET, __phys_to_pfn(ITCM_OFFSET),
+			       (ITCM_END - ITCM_OFFSET + 1),
+			       MT_UNCACHED);
 		/* Copy code from RAM to ITCM */
 		start = &__sitcm_text;
 		end   = &__eitcm_text;
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 2858941..08ca541 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -539,20 +539,20 @@ static void __init alloc_init_section(pgd_t *pgd, unsigned long addr,
 	}
 }
 
-static void __init create_36bit_mapping(struct map_desc *md,
+static void __init create_36bit_mapping(unsigned long addr,
+					unsigned long pfn, size_t length,
 					const struct mem_type *type)
 {
-	unsigned long phys, addr, length, end;
+	unsigned long phys, end;
 	pgd_t *pgd;
 
-	addr = md->virtual;
-	phys = (unsigned long)__pfn_to_phys(md->pfn);
-	length = PAGE_ALIGN(md->length);
+	phys = (unsigned long)__pfn_to_phys(pfn);
+	length = PAGE_ALIGN(length);
 
 	if (!(cpu_architecture() >= CPU_ARCH_ARMv6 || cpu_is_xsc3())) {
 		printk(KERN_ERR "MM: CPU does not support supersection "
 		       "mapping for 0x%08llx at 0x%08lx\n",
-		       __pfn_to_phys((u64)md->pfn), addr);
+		       __pfn_to_phys((u64)pfn), addr);
 		return;
 	}
 
@@ -565,14 +565,14 @@ static void __init create_36bit_mapping(struct map_desc *md,
 	if (type->domain) {
 		printk(KERN_ERR "MM: invalid domain in supersection "
 		       "mapping for 0x%08llx at 0x%08lx\n",
-		       __pfn_to_phys((u64)md->pfn), addr);
+		       __pfn_to_phys((u64)pfn), addr);
 		return;
 	}
 
-	if ((addr | length | __pfn_to_phys(md->pfn)) & ~SUPERSECTION_MASK) {
+	if ((addr | length | __pfn_to_phys(pfn)) & ~SUPERSECTION_MASK) {
 		printk(KERN_ERR "MM: cannot create mapping for "
 		       "0x%08llx at 0x%08lx invalid alignment\n",
-		       __pfn_to_phys((u64)md->pfn), addr);
+		       __pfn_to_phys((u64)pfn), addr);
 		return;
 	}
 
@@ -580,7 +580,7 @@ static void __init create_36bit_mapping(struct map_desc *md,
 	 * Shift bits [35:32] of address into bits [23:20] of PMD
 	 * (See ARMv6 spec).
 	 */
-	phys |= (((md->pfn >> (32 - PAGE_SHIFT)) & 0xF) << 20);
+	phys |= (((pfn >> (32 - PAGE_SHIFT)) & 0xF) << 20);
 
 	pgd = pgd_offset_k(addr);
 	end = addr + length;
@@ -604,44 +604,45 @@ static void __init create_36bit_mapping(struct map_desc *md,
  * offsets, and we take full advantage of sections and
  * supersections.
  */
-static void __init create_mapping(struct map_desc *md)
+void __init create_mapping(unsigned long virtual, unsigned long pfn,
+			   size_t length, unsigned int mtype)
 {
-	unsigned long phys, addr, length, end;
+	unsigned long phys, addr, end;
 	const struct mem_type *type;
 	pgd_t *pgd;
 
-	if (md->virtual != vectors_base() && md->virtual < TASK_SIZE) {
+	if (virtual != vectors_base() && virtual < TASK_SIZE) {
 		printk(KERN_WARNING "BUG: not creating mapping for "
 		       "0x%08llx@0x%08lx in user region\n",
-		       __pfn_to_phys((u64)md->pfn), md->virtual);
+		       __pfn_to_phys((u64)pfn), virtual);
 		return;
 	}
 
-	if ((md->type == MT_DEVICE || md->type == MT_ROM) &&
-	    md->virtual >= PAGE_OFFSET && md->virtual < VMALLOC_END) {
+	if ((mtype == MT_DEVICE || mtype == MT_ROM) &&
+	    virtual >= PAGE_OFFSET && virtual < VMALLOC_END) {
 		printk(KERN_WARNING "BUG: mapping for 0x%08llx@0x%08lx "
 		       "overlaps vmalloc space\n",
-		       __pfn_to_phys((u64)md->pfn), md->virtual);
+		       __pfn_to_phys((u64)pfn), virtual);
 	}
 
-	type = &mem_types[md->type];
+	type = &mem_types[mtype];
 
 	/*
 	 * Catch 36-bit addresses
 	 */
-	if (md->pfn >= 0x100000) {
-		create_36bit_mapping(md, type);
+	if (pfn >= 0x100000) {
+		create_36bit_mapping(virtual, pfn, length, type);
 		return;
 	}
 
-	addr = md->virtual & PAGE_MASK;
-	phys = (unsigned long)__pfn_to_phys(md->pfn);
-	length = PAGE_ALIGN(md->length + (md->virtual & ~PAGE_MASK));
+	addr = virtual & PAGE_MASK;
+	phys = (unsigned long)__pfn_to_phys(pfn);
+	length = PAGE_ALIGN(length + (virtual & ~PAGE_MASK));
 
 	if (type->prot_l1 == 0 && ((addr | phys | length) & ~SECTION_MASK)) {
 		printk(KERN_WARNING "BUG: map for 0x%08lx at 0x%08lx can not "
 		       "be mapped using pages, ignoring.\n",
-		       __pfn_to_phys(md->pfn), addr);
+		       __pfn_to_phys(pfn), addr);
 		return;
 	}
 
@@ -665,9 +666,14 @@ void __init iotable_init(struct map_desc *io_desc, int nr)
 	int i;
 
 	for (i = 0; i < nr; i++)
-		create_mapping(io_desc + i);
+		create_mapping((unsigned long)io_desc->virtual,
+			       io_desc->pfn,
+			       io_desc->length,
+			       io_desc->type);
 }
 
+
+
 static unsigned long __initdata vmalloc_reserve = SZ_128M;
 
 /*
@@ -933,7 +939,6 @@ void __init reserve_node_zero(pg_data_t *pgdat)
  */
 static void __init devicemaps_init(struct machine_desc *mdesc)
 {
-	struct map_desc map;
 	unsigned long addr;
 	void *vectors;
 
@@ -950,29 +955,23 @@ static void __init devicemaps_init(struct machine_desc *mdesc)
 	 * It is always first in the modulearea.
 	 */
 #ifdef CONFIG_XIP_KERNEL
-	map.pfn = __phys_to_pfn(CONFIG_XIP_PHYS_ADDR & SECTION_MASK);
-	map.virtual = MODULES_VADDR;
-	map.length = ((unsigned long)_etext - map.virtual + ~SECTION_MASK) & SECTION_MASK;
-	map.type = MT_ROM;
-	create_mapping(&map);
+	create_mapping(MODULES_VADDR,
+		       __phys_to_pfn(CONFIG_XIP_PHYS_ADDR & SECTION_MASK),
+		       ((unsigned long)_etext - virtual + ~SECTION_MASK) & SECTION_MASK,
+		       MT_ROM);
 #endif
 
 	/*
 	 * Map the cache flushing regions.
 	 */
 #ifdef FLUSH_BASE
-	map.pfn = __phys_to_pfn(FLUSH_BASE_PHYS);
-	map.virtual = FLUSH_BASE;
-	map.length = SZ_1M;
-	map.type = MT_CACHECLEAN;
-	create_mapping(&map);
+	create_mapping(FLUSH_BASE, __phys_to_pfn(FLUSH_BASE_PHYS), SZ_1M
+			MT_CACHECLEAN);
 #endif
 #ifdef FLUSH_BASE_MINICACHE
-	map.pfn = __phys_to_pfn(FLUSH_BASE_PHYS + SZ_1M);
-	map.virtual = FLUSH_BASE_MINICACHE;
-	map.length = SZ_1M;
-	map.type = MT_MINICLEAN;
-	create_mapping(&map);
+	create_mapping(FLUSH_BASE_MINICACHE,
+		       __phys_to_pfn(FLUSH_BASE_PHYS + SZ_1M), SZ_1M,
+		       MT_MINICLEAN);
 #endif
 
 	/*
@@ -980,17 +979,12 @@ static void __init devicemaps_init(struct machine_desc *mdesc)
 	 * location (0xffff0000).  If we aren't using high-vectors, also
 	 * create a mapping at the low-vectors virtual address.
 	 */
-	map.pfn = __phys_to_pfn(virt_to_phys(vectors));
-	map.virtual = 0xffff0000;
-	map.length = PAGE_SIZE;
-	map.type = MT_HIGH_VECTORS;
-	create_mapping(&map);
-
-	if (!vectors_high()) {
-		map.virtual = 0;
-		map.type = MT_LOW_VECTORS;
-		create_mapping(&map);
-	}
+	create_mapping(0xffff0000, __phys_to_pfn(virt_to_phys(vectors)),
+		       PAGE_SIZE, MT_HIGH_VECTORS);
+
+	if (!vectors_high())
+		create_mapping(0, __phys_to_pfn(virt_to_phys(vectors)),
+		       PAGE_SIZE, MT_LOW_VECTORS);
 
 	/*
 	 * Ask the machine support to map in the statically mapped devices.
@@ -1021,14 +1015,8 @@ static void __init kmap_init(void)
 
 static inline void map_memory_bank(struct membank *bank)
 {
-	struct map_desc map;
-
-	map.pfn = bank_pfn_start(bank);
-	map.virtual = __phys_to_virt(bank_phys_start(bank));
-	map.length = bank_phys_size(bank);
-	map.type = MT_MEMORY;
-
-	create_mapping(&map);
+	create_mapping(__phys_to_virt(bank_phys_start(bank)),
+		       bank_pfn_start(bank), bank_phys_size(bank), MT_MEMORY);
 }
 
 static void __init map_lowmem(void)

[RFC] dove: fix __io() definition to use bus based offset

[Arnd Bergmann]

On Monday 02 August 2010, Arnd Bergmann wrote:
> > Maybe the solution is to have a macro to initialize its entries, which
> > contain the cast to unsigned long for 'virtual' ?
> 
> Yes, that would work, though I would prefer to find a solution that works
> without new macros.

Well, scratch that. On second thought, the macro based solution seems to
have more advantages. arch/arm/mach-bcmring/mm.c already has a variant,
but I think I can come up with a more typesafe version that warns if
you pass in an integer for the virtual address of an MT_DEVICE mapping,
or a pointer for a RAM mapping.

	Arnd