@@ -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

Arnd Bergmann

2010-07-29 15:26 UTC #1 lore raw

Quoted 11 lines · Eric Miao

arch/arm/mach-dove/include/mach/io.h +0 -1

index 3b3e472..067435e 100644
@@ -11,10 +11,9 @@
 
 #include "dove.h"
 
-#define IO_SPACE_LIMIT         0xffffffff

arch/arm/mach-dove/include/mach/io.h line 19

Quoted 10 lines

arch/arm/mach-dove/include/mach/io.h +4 -4

-
-#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

Eric Miao

author

2010-07-29 15:34 UTC #2 lore raw

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · Eric Miao

arch/arm/mach-dove/include/mach/io.h +0 -0

index 3b3e472..067435e 100644
@@ -11,10 +11,9 @@

?#include "dove.h"

arch/arm/mach-dove/include/mach/io.h +4 -5

-#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)

Quoted 7 lines

Quoted 2 lines
?#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.

Quoted 7 lines

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

Arnd Bergmann

2010-07-29 15:49 UTC #3 lore raw

RFC 1/1 dove: fix __io() definition to use bus based offset

Quoted 18 lines · Eric Miao

Quoted 17 lines · Arnd Bergmann

Quoted 4 lines

#include "dove.h"
(quoted diff) +0 -1
-#define IO_SPACE_LIMIT         0xffffffff

Review

Quoted 10 lines

(quoted diff) +4 -4

-
-#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

Quoted 4 lines

Quoted 2 lines
#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 <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

arch/arm/mach-dove/pcie.c +2 -2

@@ -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;

Russell King - ARM Linux

2010-07-31 11:08 UTC #5 lore raw

In reply to Eric Miao

Quoted 7 lines · Eric Miao

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

arch/arm/mach-dove/include/mach/io.h line 16

Quoted 10 lines

arch/arm/mach-dove/include/mach/io.h +3 -5

 #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

Arnd Bergmann

2010-07-31 19:21 UTC #6 lore raw

Quoted 21 lines · On Saturday 31 July 2010 13:08:02 Russell King - ARM Linux wrote:

Quoted 7 lines · Eric Miao

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

arch/arm/mach-dove/include/mach/io.h line 16

Quoted 10 lines

arch/arm/mach-dove/include/mach/io.h +3 -5

 #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():

Quoted 2 lines

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

Russell King - ARM Linux

2010-07-31 20:47 UTC #7 lore raw

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Saturday 31 July 2010 13:08:02 Russell King - ARM Linux wrote:
Quoted 7 lines · Eric Miao
arch/arm/mach-dove/include/mach/io.h +0 -0
index 3b3e472..067435e 100644
@@ -11,10 +11,9 @@
 
arch/arm/mach-dove/include/mach/io.h line 16
Quoted 10 lines
arch/arm/mach-dove/include/mach/io.h +3 -5
 #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():

Quoted 6 lines

Quoted 2 lines
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.

Arnd Bergmann

2010-08-02 10:59 UTC #8 lore raw

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Arnd Bergmann
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.

Quoted 7 lines

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.

Quoted 6 lines

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:

Quoted 7 lines

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

Ok.

Quoted 8 lines

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.

Quoted 6 lines

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).

Quoted 8 lines

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

Russell King - ARM Linux

2010-08-02 11:24 UTC #9 lore raw

Quoted 11 lines · Arnd Bergmann

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.

Quoted 7 lines

Quoted 6 lines
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.

Quoted 6 lines

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?

Quoted 20 lines

Quoted 6 lines
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;
};

Quoted 5 lines

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' ?

Arnd Bergmann

2010-08-02 15:44 UTC #10 lore raw

RFC 1/1 dove: fix __io() definition to use bus based offset

Quoted 7 lines · Russell King - ARM Linux

Quoted 4 lines · Arnd Bergmann
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.

Quoted 7 lines

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.

Quoted 14 lines

Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
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.

Quoted 3 lines

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().

Quoted 20 lines

Quoted 5 lines
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;
}

Quoted 8 lines

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.

Quoted 21 lines

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.

Quoted 6 lines

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(-)

arch/arm/include/asm/mach/map.h +2 -0

index 742c2aa..1f0ceb5 100644
@@ -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);

arch/arm/kernel/tcm.c +5 ~~-20~~

index e503038..dd9ca37 100644
@@ -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;

arch/arm/mm/mmu.c +48 ~~-60~~

209 lines not on this page · raw message

Arnd Bergmann

2010-08-03 09:08 UTC #11 lore raw

Quoted 5 lines · Arnd Bergmann

Quoted 2 lines
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