| Message ID | 20110822164740.5682541b@queued.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 23, 2011 at 7:47 AM, Andres Salomon <dilinger@queued.net> wrote: > The sram code allocates memory with ioremap, which assumes MT_DEVICE > for memory protections. This explodes when we map sram for power > management purposes and then attempt to execute it (jump_to_lp_sram) > on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which > doesn't set the L_PTE_XN bit. > > Signed-off-by: Andres Salomon <dilinger@queued.net> > --- > arch/arm/mach-mmp/sram.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > Eric, this patch is against the devel branch of your pxa tree. > > diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c > index 4304f95..ca4d3c1 100644 > --- a/arch/arm/mach-mmp/sram.c > +++ b/arch/arm/mach-mmp/sram.c > @@ -21,6 +21,7 @@ > #include <linux/err.h> > #include <linux/slab.h> > #include <linux/genalloc.h> > +#include <asm/mach/map.h> > > #include <mach/sram.h> > > @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct platform_device *pdev) > > info->sram_phys = (phys_addr_t)res->start; > info->sram_size = resource_size(res); > - info->sram_virt = ioremap(info->sram_phys, info->sram_size); > + info->sram_virt = __arm_ioremap(info->sram_phys, info->sram_size, > + MT_MEMORY); I doubt MT_MEMORY is intended for use with __arm_ioremap(). There could be other way around to the L_PTE_XN bit. One other way I'm actually thinking of is to add the SRAM mapping to mmp_map_io(). The difference of SRAM offset/size may result the separation of mmp_map_io() into {pxa168,pxa910,mmp2}_map_io() if necessary. > info->pool_name = kstrdup(pdata->pool_name, GFP_KERNEL); > info->granularity = pdata->granularity; > > -- > 1.7.2.5 > >
On Mon, Aug 22, 2011 at 04:47:40PM -0700, Andres Salomon wrote: > The sram code allocates memory with ioremap, which assumes MT_DEVICE > for memory protections. This explodes when we map sram for power > management purposes and then attempt to execute it (jump_to_lp_sram) > on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which > doesn't set the L_PTE_XN bit. > > Signed-off-by: Andres Salomon <dilinger@queued.net> > --- > arch/arm/mach-mmp/sram.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > Eric, this patch is against the devel branch of your pxa tree. > > diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c > index 4304f95..ca4d3c1 100644 > --- a/arch/arm/mach-mmp/sram.c > +++ b/arch/arm/mach-mmp/sram.c > @@ -21,6 +21,7 @@ > #include <linux/err.h> > #include <linux/slab.h> > #include <linux/genalloc.h> > +#include <asm/mach/map.h> > > #include <mach/sram.h> > > @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct platform_device *pdev) > > info->sram_phys = (phys_addr_t)res->start; > info->sram_size = resource_size(res); > - info->sram_virt = ioremap(info->sram_phys, info->sram_size); > + info->sram_virt = __arm_ioremap(info->sram_phys, info->sram_size, > + MT_MEMORY); Not a good idea fiddling about under the covers like that. The reason that MT_MEMORY is not in asm/io.h is to stop it being used like this - MT_MEMORY etc are not meant for general purpose use. It needs to be looked at properly rather than working behind the APIs, and making my life a misery by doing so, preventing me from making changes where necessary by this kind of back-door use. I guess we need a new ioremap_xxx() variant to cope with this.
On Tue, 23 Aug 2011 08:07:41 +0800 Eric Miao <eric.y.miao@gmail.com> wrote: > On Tue, Aug 23, 2011 at 7:47 AM, Andres Salomon <dilinger@queued.net> > wrote: > > The sram code allocates memory with ioremap, which assumes MT_DEVICE > > for memory protections. This explodes when we map sram for power > > management purposes and then attempt to execute it (jump_to_lp_sram) > > on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which > > doesn't set the L_PTE_XN bit. > > > > Signed-off-by: Andres Salomon <dilinger@queued.net> > > --- > > arch/arm/mach-mmp/sram.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > Eric, this patch is against the devel branch of your pxa tree. > > > > diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c > > index 4304f95..ca4d3c1 100644 > > --- a/arch/arm/mach-mmp/sram.c > > +++ b/arch/arm/mach-mmp/sram.c > > @@ -21,6 +21,7 @@ > > #include <linux/err.h> > > #include <linux/slab.h> > > #include <linux/genalloc.h> > > +#include <asm/mach/map.h> > > > > #include <mach/sram.h> > > > > @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct > > platform_device *pdev) > > > > info->sram_phys = (phys_addr_t)res->start; > > info->sram_size = resource_size(res); > > - info->sram_virt = ioremap(info->sram_phys, > > info->sram_size); > > + info->sram_virt = __arm_ioremap(info->sram_phys, > > info->sram_size, > > + MT_MEMORY); > > I doubt MT_MEMORY is intended for use with __arm_ioremap(). There > could be other way around to the L_PTE_XN bit. > > One other way I'm actually thinking of is to add the SRAM mapping to > mmp_map_io(). The difference of SRAM offset/size may result the > separation of mmp_map_io() into {pxa168,pxa910,mmp2}_map_io() > if necessary. > I guess I don't follow. I think you're talking about adding it to the standard_io_desc array, but that would require having it pre-mapped and knowing the virtual address. Or were you planning to ioremap it?
On Tue, 23 Aug 2011 01:07:55 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: [...] > > @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct > > platform_device *pdev) > > info->sram_phys = (phys_addr_t)res->start; > > info->sram_size = resource_size(res); > > - info->sram_virt = ioremap(info->sram_phys, > > info->sram_size); > > + info->sram_virt = __arm_ioremap(info->sram_phys, > > info->sram_size, > > + MT_MEMORY); > > Not a good idea fiddling about under the covers like that. The reason > that MT_MEMORY is not in asm/io.h is to stop it being used like this - > MT_MEMORY etc are not meant for general purpose use. > > It needs to be looked at properly rather than working behind the APIs, > and making my life a misery by doing so, preventing me from making > changes where necessary by this kind of back-door use. > > I guess we need a new ioremap_xxx() variant to cope with this. Something like ioremap_exec()? I have no idea what the related MT_ entry would be (as someone who's new to the ARM world, it's not entirely clear what the semantic distinctions are between the various MT_ entries).
On 08/23/2011 10:08 AM, Andres Salomon wrote: > On Tue, 23 Aug 2011 08:07:41 +0800 > Eric Miao<eric.y.miao@gmail.com> wrote: > >> On Tue, Aug 23, 2011 at 7:47 AM, Andres Salomon<dilinger@queued.net> >> wrote: >>> The sram code allocates memory with ioremap, which assumes MT_DEVICE >>> for memory protections. This explodes when we map sram for power >>> management purposes and then attempt to execute it (jump_to_lp_sram) >>> on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which >>> doesn't set the L_PTE_XN bit. >>> >>> Signed-off-by: Andres Salomon<dilinger@queued.net> >>> --- >>> arch/arm/mach-mmp/sram.c | 4 +++- >>> 1 files changed, 3 insertions(+), 1 deletions(-) >>> >>> Eric, this patch is against the devel branch of your pxa tree. >>> >>> diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c >>> index 4304f95..ca4d3c1 100644 >>> --- a/arch/arm/mach-mmp/sram.c >>> +++ b/arch/arm/mach-mmp/sram.c >>> @@ -21,6 +21,7 @@ >>> #include<linux/err.h> >>> #include<linux/slab.h> >>> #include<linux/genalloc.h> >>> +#include<asm/mach/map.h> >>> >>> #include<mach/sram.h> >>> >>> @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct >>> platform_device *pdev) >>> >>> info->sram_phys = (phys_addr_t)res->start; >>> info->sram_size = resource_size(res); >>> - info->sram_virt = ioremap(info->sram_phys, >>> info->sram_size); >>> + info->sram_virt = __arm_ioremap(info->sram_phys, >>> info->sram_size, >>> + MT_MEMORY); >> >> I doubt MT_MEMORY is intended for use with __arm_ioremap(). There >> could be other way around to the L_PTE_XN bit. >> >> One other way I'm actually thinking of is to add the SRAM mapping to >> mmp_map_io(). The difference of SRAM offset/size may result the >> separation of mmp_map_io() into {pxa168,pxa910,mmp2}_map_io() >> if necessary. >> > > I guess I don't follow. I think you're talking about adding it to the > standard_io_desc array, but that would require having it pre-mapped and > knowing the virtual address. Or were you planning to ioremap it? I missed the L_PTE_XN bit. The patch is originally for audio sram, so use the ioremap is ok for that. But for the internal sram we should need the different mapping property. so far, the standard_io_desc is shared by pxa168/pxa910/mmp2; we can not add the sram's entry into it for now sram is only dedicated to mmp2; just like Eric's suggestion, we need mmp2_map_io() only for mmp2, and add the sram's entries into the structure. if so, we need transfer the mapped info into the sram module, and sram module just keep the info and do not need remap it again. so what's your opinion?
On Tue, 2011-08-23 at 00:48 -0700, Leo Yan wrote: > > On 08/23/2011 10:08 AM, Andres Salomon wrote: > > On Tue, 23 Aug 2011 08:07:41 +0800 > > Eric Miao<eric.y.miao@gmail.com> wrote: > > > >> On Tue, Aug 23, 2011 at 7:47 AM, Andres Salomon<dilinger@queued.net> > >> wrote: > >>> The sram code allocates memory with ioremap, which assumes MT_DEVICE > >>> for memory protections. This explodes when we map sram for power > >>> management purposes and then attempt to execute it (jump_to_lp_sram) > >>> on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which > >>> doesn't set the L_PTE_XN bit. > >>> > >>> Signed-off-by: Andres Salomon<dilinger@queued.net> > >>> --- > >>> arch/arm/mach-mmp/sram.c | 4 +++- > >>> 1 files changed, 3 insertions(+), 1 deletions(-) > >>> > >>> Eric, this patch is against the devel branch of your pxa tree. > >>> > >>> diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c > >>> index 4304f95..ca4d3c1 100644 > >>> --- a/arch/arm/mach-mmp/sram.c > >>> +++ b/arch/arm/mach-mmp/sram.c > >>> @@ -21,6 +21,7 @@ > >>> #include<linux/err.h> > >>> #include<linux/slab.h> > >>> #include<linux/genalloc.h> > >>> +#include<asm/mach/map.h> > >>> > >>> #include<mach/sram.h> > >>> > >>> @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct > >>> platform_device *pdev) > >>> > >>> info->sram_phys = (phys_addr_t)res->start; > >>> info->sram_size = resource_size(res); > >>> - info->sram_virt = ioremap(info->sram_phys, > >>> info->sram_size); > >>> + info->sram_virt = __arm_ioremap(info->sram_phys, > >>> info->sram_size, > >>> + MT_MEMORY); > >> > >> I doubt MT_MEMORY is intended for use with __arm_ioremap(). There > >> could be other way around to the L_PTE_XN bit. > >> > >> One other way I'm actually thinking of is to add the SRAM mapping to > >> mmp_map_io(). The difference of SRAM offset/size may result the > >> separation of mmp_map_io() into {pxa168,pxa910,mmp2}_map_io() > >> if necessary. > >> > > > > I guess I don't follow. I think you're talking about adding it to the > > standard_io_desc array, but that would require having it pre-mapped and > > knowing the virtual address. Or were you planning to ioremap it? > > I missed the L_PTE_XN bit. > The patch is originally for audio sram, so use the ioremap is ok for > that. But for the internal sram we should need the different mapping > property. > > so far, the standard_io_desc is shared by pxa168/pxa910/mmp2; > we can not add the sram's entry into it for now sram is only dedicated > to mmp2; just like Eric's suggestion, we need mmp2_map_io() only for > mmp2, and add the sram's entries into the structure. > if so, we need transfer the mapped info into the sram module, > and sram module just keep the info and do not need remap it again. > > so what's your opinion? Could we use resource or platform data to specify sram is used for device or memory? If so, it may be helpful to customize.
diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c index 4304f95..ca4d3c1 100644 --- a/arch/arm/mach-mmp/sram.c +++ b/arch/arm/mach-mmp/sram.c @@ -21,6 +21,7 @@ #include <linux/err.h> #include <linux/slab.h> #include <linux/genalloc.h> +#include <asm/mach/map.h> #include <mach/sram.h> @@ -87,7 +88,8 @@ static int __devinit sram_probe(struct platform_device *pdev) info->sram_phys = (phys_addr_t)res->start; info->sram_size = resource_size(res); - info->sram_virt = ioremap(info->sram_phys, info->sram_size); + info->sram_virt = __arm_ioremap(info->sram_phys, info->sram_size, + MT_MEMORY); info->pool_name = kstrdup(pdata->pool_name, GFP_KERNEL); info->granularity = pdata->granularity;
The sram code allocates memory with ioremap, which assumes MT_DEVICE for memory protections. This explodes when we map sram for power management purposes and then attempt to execute it (jump_to_lp_sram) on the OLPC XO-1.75. Instead, we want to specify MT_MEMORY, which doesn't set the L_PTE_XN bit. Signed-off-by: Andres Salomon <dilinger@queued.net> --- arch/arm/mach-mmp/sram.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) Eric, this patch is against the devel branch of your pxa tree.