[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[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

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 arch/arm/include/asm/io.h |    3 +++
 arch/arm/mm/ioremap.c     |   16 +++++++++++++++-
 arch/arm/mm/nommu.c       |    4 ++++
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index d66605d..0c0cfe0 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -246,6 +246,9 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
 #define __arch_iounmap			__iounmap
 #endif
 
+extern void __iomem* (*arch_ioremap)(unsigned long p, size_t size, unsigned int type);
+extern void (*arch_iounmap)(volatile void __iomem *addr);
+
 #define ioremap(cookie,size)		__arch_ioremap((cookie), (size), MT_DEVICE)
 #define ioremap_nocache(cookie,size)	__arch_ioremap((cookie), (size), MT_DEVICE)
 #define ioremap_cached(cookie,size)	__arch_ioremap((cookie), (size), MT_DEVICE_CACHED)
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ab50627..40ef390 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -281,9 +281,15 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size,
 }
 EXPORT_SYMBOL(__arm_ioremap_pfn);
 
+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);
+
 	return __arm_ioremap_caller(phys_addr, size, mtype,
 			__builtin_return_address(0));
 }
@@ -291,10 +297,18 @@ EXPORT_SYMBOL(__arm_ioremap);
 
 void __iounmap(volatile void __iomem *io_addr)
 {
-	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
+	void *addr;
 #ifndef CONFIG_SMP
 	struct vm_struct **p, *tmp;
+#endif
 
+	if (arch_iounmap) {
+		arch_iounmap(io_addr);
+		return;
+	}
+
+	addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
+#ifndef CONFIG_SMP
 	/*
 	 * If this is a section based mapping we need to handle it
 	 * specially as the VM subsystem does not know how to handle
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index 687d023..561f19f 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -73,6 +73,10 @@ void __iomem *__arm_ioremap_pfn_caller(unsigned long pfn, unsigned long offset,
 	return __arm_ioremap_pfn(pfn, offset, size, mtype);
 }
 
+/* declare to just avoid the ifdef in nommu case */
+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)
 {
-- 
1.7.4.1

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 arch/arm/mach-omap1/io.c             |    3 +++
 arch/arm/mach-omap2/io.c             |    4 ++++
 arch/arm/plat-omap/include/plat/io.h |    2 ++
 arch/arm/plat-omap/io.c              |    6 ++++++
 4 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap1/io.c b/arch/arm/mach-omap1/io.c
index 870886a..ca80286 100644
--- a/arch/arm/mach-omap1/io.c
+++ b/arch/arm/mach-omap1/io.c
@@ -99,6 +99,9 @@ void __init omap1_map_common_io(void)
 	local_flush_tlb_all();
 	flush_cache_all();
 
+	/* we set the arch_ioremap and arch_iounmap */
+	omap_set_ioremap();
+
 	/* We want to check CPU revision early for cpu_is_omapxxxx() macros.
 	 * IO space mapping must be initialized before we can do that.
 	 */
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 441e79d..a178f9d 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -249,6 +249,10 @@ static void __init _omap2_map_common_io(void)
 	flush_cache_all();
 
 	omap2_check_revision();
+
+	/* we set the arch_ioremap and arch_iounmap */
+	omap_set_ioremap();
+
 	omap_sram_init();
 }
 
diff --git a/arch/arm/plat-omap/include/plat/io.h b/arch/arm/plat-omap/include/plat/io.h
index d72ec85..75c7a68 100644
--- a/arch/arm/plat-omap/include/plat/io.h
+++ b/arch/arm/plat-omap/include/plat/io.h
@@ -309,6 +309,8 @@ extern void omap2_init_common_devices(struct omap_sdrc_params *sdrc_cs0,
 void __iomem *omap_ioremap(unsigned long phys, size_t size, unsigned int type);
 void omap_iounmap(volatile void __iomem *addr);
 
+void __init omap_set_ioremap(void);
+
 #endif
 
 #endif
diff --git a/arch/arm/plat-omap/io.c b/arch/arm/plat-omap/io.c
index f1ecfa9..285a0d4 100644
--- a/arch/arm/plat-omap/io.c
+++ b/arch/arm/plat-omap/io.c
@@ -23,6 +23,12 @@
 #define BETWEEN(p,st,sz)	((p) >= (st) && (p) < ((st) + (sz)))
 #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.
  */
-- 
1.7.4.1

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Arnd Bergmann]

On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> --- a/arch/arm/plat-omap/io.c
> +++ b/arch/arm/plat-omap/io.c
> @@ -23,6 +23,12 @@
>  #define BETWEEN(p,st,sz)       ((p) >= (st) && (p) < ((st) + (sz)))
>  #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.

	Arnd

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:54 Sun 22 May     , Arnd Bergmann wrote:
> On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > 
> > --- a/arch/arm/plat-omap/io.c
> > +++ b/arch/arm/plat-omap/io.c
> > @@ -23,6 +23,12 @@
> >  #define BETWEEN(p,st,sz)       ((p) >= (st) && (p) < ((st) + (sz)))
> >  #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.
not sure we can do so as imx do do so, they check on mx3 the memmory region
and fix the remapping type

Not sure it's the right to do either

Best Regards,
J.

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Santosh Shilimkar]

On 5/22/2011 3:24 PM, Arnd Bergmann wrote:
> On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:
>>
>> --- a/arch/arm/plat-omap/io.c
>> +++ b/arch/arm/plat-omap/io.c
>> @@ -23,6 +23,12 @@
>>   #define BETWEEN(p,st,sz)       ((p)>= (st)&&  (p)<  ((st) + (sz)))
>>   #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.
>
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()

> * Handle the fixed mappings in the generic ioremap code. They all go through
>    create_mapping(), so we can save a list to walk there.
>
This seems good approach in case we are ok to create and save the IO
list. This will also help to avoid un-necessary dual mappings which
can happen today if the mapping is outside the fixed mapping.

Regards
Santosh

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:50 Sun 22 May     , Santosh Shilimkar wrote:
> On 5/22/2011 3:24 PM, Arnd Bergmann wrote:
> >On Sunday 22 May 2011 11:04:40 Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>
> >>--- a/arch/arm/plat-omap/io.c
> >>+++ b/arch/arm/plat-omap/io.c
> >>@@ -23,6 +23,12 @@
> >>  #define BETWEEN(p,st,sz)       ((p)>= (st)&&  (p)<  ((st) + (sz)))
> >>  #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.
> >
> 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()
agreed on this

Best Regards,
J.

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Arnd Bergmann]

On Sunday 22 May 2011 13:20:23 Santosh Shilimkar wrote:
> >>   /*
> >>    * 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.
> >
> 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.

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

	Arnd

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Santosh Shilimkar]

On 5/22/2011 5:05 PM, Arnd Bergmann wrote:
> On Sunday 22 May 2011 13:20:23 Santosh Shilimkar wrote:
>>>>    /*
>>>>     * 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.
>>>
>> 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.
>
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?

Infact we have a problem today in the dynamic ioremap() code,
where if you called ioremap() with same PA, multiple times,
it will keep creating new mappings, and this is where you
can land up in problem.

But this problem is separate and needs some kind of io_list
storage for dymamic mappings too.

>> 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.
>
See above. Driver's space is generally avoided in fixed
mappings.

Regards
Santosh

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Arnd Bergmann]

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

	Arnd

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Santosh Shilimkar]

On 5/22/2011 6:26 PM, Arnd Bergmann wrote:
> 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().
>>
>> 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 didn't say that. Because if you want to get a VA for an IO
space from fixed map, ioremap() platform hook is useful.

The existing interface allows, platforms to add additional
logic for fixed mappings and still use the generic interface'
for dynamic mappings with single exported interface.

Regards
Santosh

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Russell King - ARM Linux]

On Sun, May 22, 2011 at 01:35:03PM +0200, Arnd Bergmann wrote:
> 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.

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Santosh Shilimkar]

On 5/22/2011 6:39 PM, Russell King - ARM Linux wrote:
> On Sun, May 22, 2011 at 01:35:03PM +0200, Arnd Bergmann wrote:
>> 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.

Thanks Russell for bringing this point. I forgot to mention about TLB
pressure while talking about other bits and flexibility the current
interface gives.

Regards
Santosh

[RFC PATCH 2/2] omap: switch to ioremap function pointer

[Arnd Bergmann]

On Sunday 22 May 2011 15:09:55 Russell King - ARM Linux wrote:
> On Sun, May 22, 2011 at 01:35:03PM +0200, Arnd Bergmann wrote:
> > 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.

My first proposal was to remove the fixed mapping in the cases where a
driver uses ioremap, not the other way round. The other proposal was
to handle the static mappings in the common ioremap code.

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

Ok, good to know. Can anyone point to the specific results? TLB pressure
is the obvious concern, but I didn't expect it to be measurable in this
scenario.
 
> 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.

The main reason the TLB miss for the list walk hurts so much is that it's
in the hot path for important workloads and that certain CPUs fail to install
an invalid TLB entry, so the pointless page walk gets exectuted all the time.

For the device mappings, I would assume that the accesses are typically
going to the most active devices, which consequently are still present in
the TLB. My na?ve expectation would be that there is more to gain from
using large pages ioremap (which we don't do AFAICT) whenever we map more
than a page than by pre-mapping some of the devices using large pages.

	Arnd

[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[Russell King - ARM Linux]

On Sun, May 22, 2011 at 11:04:39AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 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.

> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index ab50627..40ef390 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -281,9 +281,15 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size,
>  }
>  EXPORT_SYMBOL(__arm_ioremap_pfn);
>  
> +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.

>  void __iounmap(volatile void __iomem *io_addr)
>  {
> -	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
> +	void *addr;
>  #ifndef CONFIG_SMP
>  	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()...

[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:03 Sun 22 May     , Russell King - ARM Linux wrote:
> On Sun, May 22, 2011 at 11:04:39AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > 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.
> 
> > diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> > index ab50627..40ef390 100644
> > --- a/arch/arm/mm/ioremap.c
> > +++ b/arch/arm/mm/ioremap.c
> > @@ -281,9 +281,15 @@ __arm_ioremap_pfn(unsigned long pfn, unsigned long offset, size_t size,
> >  }
> >  EXPORT_SYMBOL(__arm_ioremap_pfn);
> >  
> > +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.
> 
> >  void __iounmap(volatile void __iomem *io_addr)
> >  {
> > -	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
> > +	void *addr;
> >  #ifndef CONFIG_SMP
> >  	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

as to be able to compile more than one soc we need to be able to specifcy the
custom ioremap at runtime
Do you agree on this?


like this patch

Best Regards,
J.

[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[Russell King - ARM Linux]

On Sun, May 22, 2011 at 01:05:43PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 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);

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

[RFC PATCH 1/2] arm: introduce arch_ioremap and arch_iounmap function pointer

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:19 Sun 22 May     , Russell King - ARM Linux wrote:
> On Sun, May 22, 2011 at 01:05:43PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > 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);
> 
> +	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.
I like that

and for ioummap

void __iounmap(volatile void __iomem *io_addr)
{
        void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
#ifndef CONFIG_SMP
        struct vm_struct **p, *tmp;
+#endif

+	__arch_iounmap(io_addr);
+
+#ifndef CONFIG_SMP
        /*
         * If this is a section based mapping we need to handle it
         * specially as the VM subsystem does not know how to handle
         * such a beast. We need the lock here b/c we need to clear
         * all the mappings before the area can be reclaimed
         * by someone else.
         */

Best Regards,
J.