arm: introduce arch_ioremap and arch_iounmap function pointer

arch/arm/plat-omap/io.c +0 -0

@@ -23,6 +23,12 @@
 #define BETWEEN(p,st,sz)       ((p) >= (st) && (p) < ((st) + (sz)))

Quoted 5 lines · On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:

arch/arm/plat-omap/io.c +0 -0

@@ -23,6 +23,12 @@
 #define BETWEEN(p,st,sz)       ((p) >= (st) && (p) < ((st) + (sz)))

arch/arm/plat-omap/io.c line 33

Quoted 10 lines

arch/arm/plat-omap/io.c +6 -0

 #define XLATE(p,pst,vst)       ((void __iomem *)((p) - (pst) + (vst)))
 
+void __init omap_set_ioremap(void)
+{
+       arch_ioremap = omap_ioremap;
+       arch_iounmap = omap_iounmap;
+}
+
 /*
  * Intercept ioremap() requests for addresses in our fixed mapping regions.

The only thing that the ioremap functions for omap do as a special case is to handle the fixed mappings. Maybe a better approach would be to make handling this in omap (and others) unnecessary, using one of two approaches:

* Remove all fixed mappings that are also ioremapped, and rely on every user calling ioremap correctly. I don't see why we would want to allow both methods anyway.

* Handle the fixed mappings in the generic ioremap code. They all go through create_mapping(), so we can save a list to walk there.

Quoted 5 lines · On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:

arch/arm/plat-omap/io.c +0 -0

@@ -23,6 +23,12 @@
  #define BETWEEN(p,st,sz)       ((p)>= (st)&&  (p)<  ((st) + (sz)))

arch/arm/plat-omap/io.c line 33

Quoted 10 lines

arch/arm/plat-omap/io.c +6 -0

  #define XLATE(p,pst,vst)       ((void __iomem *)((p) - (pst) + (vst)))
 
+void __init omap_set_ioremap(void)
+{
+       arch_ioremap = omap_ioremap;
+       arch_iounmap = omap_iounmap;
+}
+
  /*
   * Intercept ioremap() requests for addresses in our fixed mapping regions.

The only thing that the ioremap functions for omap do as a special case is to handle the fixed mappings. Maybe a better approach would be to make handling this in omap (and others) unnecessary, using one of two approaches:

* Remove all fixed mappings that are also ioremapped, and rely on every user calling ioremap correctly. I don't see why we would want to allow both methods anyway.

* Handle the fixed mappings in the generic ioremap code. They all go through create_mapping(), so we can save a list to walk there.

Quoted 22 lines · Arnd Bergmann

Quoted 5 lines · On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:

arch/arm/plat-omap/io.c +0 -0

@@ -23,6 +23,12 @@
 #define BETWEEN(p,st,sz)       ((p)>= (st)&&  (p)<  ((st) + (sz)))

arch/arm/plat-omap/io.c line 33

Quoted 10 lines

arch/arm/plat-omap/io.c +6 -0

 #define XLATE(p,pst,vst)       ((void __iomem *)((p) - (pst) + (vst)))
 
+void __init omap_set_ioremap(void)
+{
+       arch_ioremap = omap_ioremap;
+       arch_iounmap = omap_iounmap;
+}
+
 /*
  * Intercept ioremap() requests for addresses in our fixed mapping regions.

The only thing that the ioremap functions for omap do as a special case is to handle the fixed mappings. Maybe a better approach would be to make handling this in omap (and others) unnecessary, using one of two approaches:

Quoted 4 lines

* Remove all fixed mappings that are also ioremapped, and rely on every user calling ioremap correctly. I don't see why we would want to allow both methods anyway.

What do you mean by "Remove all fixed mappings that are also ioremapped" ?
Why will you ioremap() it again if the fixed mapping is already registered through iotable_init()

Quoted 8 lines

Quoted 2 lines

/*
* Intercept ioremap() requests for addresses in our fixed mapping regions.

The only thing that the ioremap functions for omap do as a special case is to handle the fixed mappings. Maybe a better approach would be to make handling this in omap (and others) unnecessary, using one of two approaches:

Quoted 4 lines

* Remove all fixed mappings that are also ioremapped, and rely on every user calling ioremap correctly. I don't see why we would want to allow both methods anyway.

What do you mean by "Remove all fixed mappings that are also ioremapped" ?

Why will you ioremap() it again if the fixed mapping is already registered through iotable_init()

Quoted 8 lines · On Sunday 22 May 2011 13:20:23 Santosh Shilimkar wrote:

Quoted 8 lines

Quoted 2 lines

/*
* Intercept ioremap() requests for addresses in our fixed mapping regions.

The only thing that the ioremap functions for omap do as a special case is to handle the fixed mappings. Maybe a better approach would be to make handling this in omap (and others) unnecessary, using one of two approaches:

Quoted 6 lines

Quoted 4 lines

* Remove all fixed mappings that are also ioremapped, and rely on every user calling ioremap correctly. I don't see why we would want to allow both methods anyway.

What do you mean by "Remove all fixed mappings that are also ioremapped" ?

I mean don't call iotable_init() for regions that is ioremapped into a device driver. Having both iotable_init and ioremap on the same area is a bit fishy anyway, so we should only have one of the two: If the ioremap specifies different attributes from the existing mapping, the code will still just return a pointer, which may not be what the driver asks for, although it avoids the problems that arise from having conflicting mappings.

Quoted 2 lines

Why will you ioremap() it again if the fixed mapping is already registered through iotable_init()

Evidently, drivers call ioremap on regions that have a mapping installed by iotable_init, otherwise the omap_ioremap function would not make any sense whatsoever.

At least on OMAP, the fixed mappings used are mainly the interconnect, infrastructure, memory controller and most of these are not associated with any driver.

On OMAP, the fixed mappings are handled directly so nothing fishy there.

Attributes are already provided via io descriptor for fixed mapping and that should remain fixed. O.w there is no meaning for the fixed mapping.

For dynamic mapping, different attributes work as usual via __ioremap().

So I don't see why you will have conflicting mapping at all?

Quoted 13 lines · On Sunday 22 May 2011 13:46:04 Santosh Shilimkar wrote:

At least on OMAP, the fixed mappings used are mainly the interconnect, infrastructure, memory controller and most of these are not associated with any driver.

On OMAP, the fixed mappings are handled directly so nothing fishy there.

Attributes are already provided via io descriptor for fixed mapping and that should remain fixed. O.w there is no meaning for the fixed mapping.

For dynamic mapping, different attributes work as usual via __ioremap().

Quoted 1 line

So I don't see why you will have conflicting mapping at all?

Ok, great!

If that's the case, then you don't need omap_ioremap at all and you can simply remove it. I assume that when we make the other platforms do the same as omap, we also won't need a per-platform ioremap function pointer.

I mean don't call iotable_init() for regions that is ioremapped into a device driver. Having both iotable_init and ioremap on the same area is a bit fishy anyway,

Quoted 3 lines · Arnd Bergmann

I mean don't call iotable_init() for regions that is ioremapped into a device driver. Having both iotable_init and ioremap on the same area is a bit fishy anyway,

That's how things used to be done. What your proposal causes is people defining virtual address constants in platform header files, and using those constants directly in drivers.

_Thankfully_ we've moved on from that and are now having SoCs intercept at the ioremap() level to eliminate that junk.

Consider an SoC where you have a block of devices known at boot time to be, lets say at 0x10000000 to 0x10100000. Lets say there's 16 devices in there, each occupying 64K for simplicity.

Does it make sense to individually ioremap(), where each ioremap() creates 16 page table entries and therefore potentially consumes up to 16 TLB entries, resulting in 256 TLB entries to cover all 16 devices, or does it make sense to map the entire region as one section at boot time, thereby only consuming one TLB entry for the entire lot?

I believe TI have done some testing in this area, and have showed that this kind of optimization is reflected in the performance figures.

Given that people are worrying about 0.2% performance gains through _elimination_ of the list prefetching due to TLB misses (see the linux-arch thread, and the proposed removal of prefetching from the list macros) I don't think anyone can justify avoiding the above kind of optimization.

Quoted 3 lines · Arnd Bergmann

I mean don't call iotable_init() for regions that is ioremapped into a device driver. Having both iotable_init and ioremap on the same area is a bit fishy anyway,

That's how things used to be done. What your proposal causes is people defining virtual address constants in platform header files, and using those constants directly in drivers.

Does it make sense to individually ioremap(), where each ioremap() creates 16 page table entries and therefore potentially consumes up to 16 TLB entries, resulting in 256 TLB entries to cover all 16 devices, or does it make sense to map the entire region as one section at boot time, thereby only consuming one TLB entry for the entire lot?

I believe TI have done some testing in this area, and have showed that this kind of optimization is reflected in the performance figures.

Given that people are worrying about 0.2% performance gains through _elimination_ of the list prefetching due to TLB misses (see the linux-arch thread, and the proposed removal of prefetching from the list macros) I don't think anyone can justify avoiding the above kind of optimization.

today we define it at runtime via __arch_ioremap and __arch_iounmap

this PATH will allow to overwrite the default ioremap at runtime to be able to compile multiple soc in the same kernel

arch/arm/mm/ioremap.c +0 -0

index ab50627..40ef390 100644
@@ -281,9 +281,15 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size,
 }
 EXPORT_SYMBOL(__arm_ioremap_pfn);

Quoted 4 lines · Jean-Christophe PLAGNIOL-VILLARD

today we define it at runtime via __arch_ioremap and __arch_iounmap

this PATH will allow to overwrite the default ioremap at runtime to be able to compile multiple soc in the same kernel

This is buggy.

Quoted 7 lines

arch/arm/mm/ioremap.c +0 -0

index ab50627..40ef390 100644
@@ -281,9 +281,15 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size,
 }
 EXPORT_SYMBOL(__arm_ioremap_pfn);

arch/arm/mm/ioremap.c __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size, · line 292

Quoted 10 lines

arch/arm/mm/ioremap.c +6 -0

 
+void __iomem* (*arch_ioremap)(unsigned long p, size_t size, unsigned int type);
+void (*arch_iounmap)(volatile void __iomem *addr);
+
 void __iomem *
 __arm_ioremap(unsigned long phys_addr, size_t size, unsigned int mtype)
 {
+	if (arch_ioremap)
+		return arch_ioremap(phys_addr, size, mtype);
+

Some SoCs call __arm_ioremap() from their ioremap() implementation, so making this hook results in infinite function recursion if you try to set this to their ioremap() implementation.

arch/arm/mm/ioremap.c EXPORT_SYMBOL(__arm_ioremap); · line 308

Quoted 12 lines

arch/arm/mm/ioremap.c +1 -1

 void __iounmap(volatile void __iomem *io_addr)
 {
-	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
+	void *addr;

#ifndef CONFIG_SMP

arch/arm/mm/ioremap.c +5 -0

 	struct vm_struct **p, *tmp;
+#endif
 
+	if (arch_iounmap) {
+		arch_iounmap(io_addr);
+		return;
+	}

And on OMAP, for similar reasons, this will also be infinite function recursion:

void omap_iounmap(volatile void __iomem *addr)

{
        unsigned long virt = (unsigned long)addr;

        if (virt >= VMALLOC_START && virt < VMALLOC_END)
                __iounmap(addr);
}

So you'll end up with a call to iounmap() calling __iounmap() which then calls omap_iounmap(), which then calls __iounmap()...

did see that and I should I did it on at91 too

so how about introduce a new ioreamap/iouremap functiom

Quoted 3 lines · Jean-Christophe PLAGNIOL-VILLARD

did see that and I should I did it on at91 too

so how about introduce a new ioreamap/iouremap functiom

How about not. How about we intercept all the ioremap() calls at a better place like at the beginning of __arm_ioremap_pfn_caller() - iow, something like this:

void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
unsigned long offset, size_t size, unsigned int mtype, void *caller)

{
        const struct mem_type *type;
        int err;
        unsigned long addr;
        struct vm_struct * area;

        /*
         * High mappings must be supersection aligned
         */
        if (pfn >= 0x100000 && (__pfn_to_phys(pfn) & ~SUPERSECTION_MASK))
                return NULL;

        /*
         * Don't allow RAM to be mapped - this causes problems with ARMv6+
         */
        if (WARN_ON(pfn_valid(pfn)))
                return NULL;

        type = get_mem_type(mtype);
        if (!type)
                return NULL;

        /*
         * Page align the mapping size, taking account of any offset.
         */
        size = PAGE_ALIGN(offset + size);

(quoted diff) +3 -0

+	addr = __arch_ioremap(pfn, size);
+	if (addr)
+		return (void __iomem *) (offset + addr);
...

and then we can have __arch_ioremap() do whatever checking of static mappings we desire.