| Message ID | 1349709952-4332-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | RFC |
| Headers | show |
Hi Uwe, On 09/10/12 01:25, Uwe Kleine-König wrote: > This allows to put the filesystem at a defined address in ROM allowing > to save more precious RAM. > > I think it's save to default to ROM because the intention of using the > uclinux map is to use a romfs and so mtd-ram doesn't give you anything > that mtd-rom doesn't. > > Signed-off-by: Uwe Kleine-K÷nig <u.kleine-koenig@pengutronix.de> Looks good to me, so: Acked-by: Greg Ungerer <gerg@uclinux.org> Couple of things to be aware of. Artem currently has a couple of patches against this file (looks like at least 1 of them is currently in linux-next). It will probably conflict with your changes, should be easy enough to fix up. The change to this file in linux-next changes the call to ioremap to be a phys_to_virt - which is because it was always going to be in real RAM. This may no longer be true with your change, it could be in ROM. For non-MMU I expect no problems. But this could be an issue if used on systems with MMU enabled. (And one of the other changes that I generated for this allows this for at least one architecture type - ColdFire/m68k). Just flagging this, I don't expect it will cause any current users any problems. Regards Greg > --- > drivers/mtd/maps/Kconfig | 2 +- > drivers/mtd/maps/uclinux.c | 30 +++++++++++++++++++++++------- > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig > index 5ba2458..d945950 100644 > --- a/drivers/mtd/maps/Kconfig > +++ b/drivers/mtd/maps/Kconfig > @@ -443,7 +443,7 @@ config MTD_GPIO_ADDR > > config MTD_UCLINUX > bool "Generic uClinux RAM/ROM filesystem support" > - depends on MTD_RAM=y && !MMU > + depends on (MTD_RAM=y || MTD_ROM=y) && !MMU > help > Map driver to support image based filesystems for uClinux. > > diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c > index c3bb304..a8a5929 100644 > --- a/drivers/mtd/maps/uclinux.c > +++ b/drivers/mtd/maps/uclinux.c > @@ -23,12 +23,13 @@ > > /****************************************************************************/ > > -struct map_info uclinux_ram_map = { > - .name = "RAM", > - .phys = (unsigned long)__bss_stop, > +static struct map_info uclinux_ram_map = { > .size = 0, > }; > > +static unsigned long physaddr = -1; > +module_param(physaddr, ulong, S_IRUGO); > + > static struct mtd_info *uclinux_ram_mtdinfo; > > /****************************************************************************/ > @@ -60,11 +61,17 @@ static int __init uclinux_mtd_init(void) > struct map_info *mapp; > > mapp = &uclinux_ram_map; > + > + if (physaddr == -1) > + mapp->phys = (resource_size_t)__bss_stop; > + else > + mapp->phys = physaddr; > + > if (!mapp->size) > mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8)))); > mapp->bankwidth = 4; > > - printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", > + printk("uclinux[mtd]: RAM/ROM probe address=0x%x size=0x%x\n", > (int) mapp->phys, (int) mapp->size); > > mapp->virt = ioremap_nocache(mapp->phys, mapp->size); > @@ -76,9 +83,18 @@ static int __init uclinux_mtd_init(void) > > simple_map_init(mapp); > > - mtd = do_map_probe("map_ram", mapp); > + mapp->name = "ROM"; > + mtd = do_map_probe("map_rom", mapp); > + if (!mtd) { > + /* fall back to ram probing for compatibility reasons */ > + mapp->name = "RAM"; > + mtd = do_map_probe("map_ram", mapp); > + if (mtd && IS_ENABLED(CONFIG_MTD_ROM)) > + pr_err("Failed to map rom, but ram succeeded. Please report this issue!\n"); > + } > + > if (!mtd) { > - printk("uclinux[mtd]: failed to find a mapping?\n"); > + printk("uclinux[mtd]: failed to find a rom/ram mapping?\n"); > iounmap(mapp->virt); > return(-ENXIO); > } > @@ -115,6 +131,6 @@ module_exit(uclinux_mtd_cleanup); > > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Greg Ungerer <gerg@snapgear.com>"); > -MODULE_DESCRIPTION("Generic RAM based MTD for uClinux"); > +MODULE_DESCRIPTION("Generic RAM/ROM based MTD for uClinux"); > > /****************************************************************************/ >
Hi Greg, On Tue, Oct 09, 2012 at 01:29:46PM +1000, Greg Ungerer wrote: > On 09/10/12 01:25, Uwe Kleine-König wrote: > >This allows to put the filesystem at a defined address in ROM allowing > >to save more precious RAM. > > > >I think it's save to default to ROM because the intention of using the > >uclinux map is to use a romfs and so mtd-ram doesn't give you anything > >that mtd-rom doesn't. > > > >Signed-off-by: Uwe Kleine-K÷nig <u.kleine-koenig@pengutronix.de> > > Looks good to me, so: > > Acked-by: Greg Ungerer <gerg@uclinux.org> > > Couple of things to be aware of. Artem currently has a couple of > patches against this file (looks like at least 1 of them is currently > in linux-next). It will probably conflict with your changes, should > be easy enough to fix up. If you tell me a branch to test, I can do so. Thanks Uwe
On Mon, Oct 8, 2012 at 11:25 AM, Uwe Kleine-König wrote: > --- a/drivers/mtd/maps/uclinux.c > +++ b/drivers/mtd/maps/uclinux.c > @@ -23,12 +23,13 @@ > > /****************************************************************************/ > > -struct map_info uclinux_ram_map = { > - .name = "RAM", > - .phys = (unsigned long)__bss_stop, > +static struct map_info uclinux_ram_map = { > .size = 0, > }; this change will break Blackfin systems. i mentioned last time someone tried to make this and we talked about adding a comment to prevent future breakage, but that seems to not have happened. i don't think's necessary either for the larger change here. use the logic: if (physaddr != -1) mapp->phys = physaddr; and keep the default initialization. -mike
On Thu, Oct 11, 2012 at 12:21:59AM -0400, Mike Frysinger wrote: > On Mon, Oct 8, 2012 at 11:25 AM, Uwe Kleine-König wrote: > > --- a/drivers/mtd/maps/uclinux.c > > +++ b/drivers/mtd/maps/uclinux.c > > @@ -23,12 +23,13 @@ > > > > /****************************************************************************/ > > > > -struct map_info uclinux_ram_map = { > > - .name = "RAM", > > - .phys = (unsigned long)__bss_stop, > > +static struct map_info uclinux_ram_map = { > > .size = 0, > > }; > > this change will break Blackfin systems. i mentioned last time > someone tried to make this and we talked about adding a comment to > prevent future breakage, but that seems to not have happened. yeah, I really think adding a comment would be the right thing as I don't see what could be the problem here you're talking about. > i don't think's necessary either for the larger change here. use the logic: > if (physaddr != -1) > mapp->phys = physaddr; > and keep the default initialization. > -mike Thanks Uwe
On Thu, Oct 11, 2012 at 12:21:59AM -0400, Mike Frysinger wrote: > On Mon, Oct 8, 2012 at 11:25 AM, Uwe Kleine-König wrote: > > --- a/drivers/mtd/maps/uclinux.c > > +++ b/drivers/mtd/maps/uclinux.c > > @@ -23,12 +23,13 @@ > > > > /****************************************************************************/ > > > > -struct map_info uclinux_ram_map = { > > - .name = "RAM", > > - .phys = (unsigned long)__bss_stop, > > +static struct map_info uclinux_ram_map = { > > .size = 0, > > }; > > this change will break Blackfin systems. i mentioned last time > someone tried to make this and we talked about adding a comment to > prevent future breakage, but that seems to not have happened. Will send a patch in a moment. > i don't think's necessary either for the larger change here. use the logic: > if (physaddr != -1) > mapp->phys = physaddr; > and keep the default initialization. I keep my way as it seems to be more robust for possible future changes. (I don't know if that will ever happen, but consider platform device support or just a retry of instatiation after physaddr changed from something back to -1.) Best regards Uwe
On Fri, Oct 12, 2012 at 3:41 AM, Uwe Kleine-König wrote: > On Thu, Oct 11, 2012 at 12:21:59AM -0400, Mike Frysinger wrote: >> i don't think's necessary either for the larger change here. use the logic: >> if (physaddr != -1) >> mapp->phys = physaddr; >> and keep the default initialization. > > I keep my way as it seems to be more robust for possible future changes. > (I don't know if that will ever happen, but consider platform device > support or just a retry of instatiation after physaddr changed from > something back to -1.) i'm afraid that will still break things. the arch code initializes the mtd map in setup_arch() which i believe is before the module_init() (which is device_initcall() when it is compiled into). so clobbering uclinux_ram_map.phys in the module_init will undo what was done in setup_arch(). -mike
diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig index 5ba2458..d945950 100644 --- a/drivers/mtd/maps/Kconfig +++ b/drivers/mtd/maps/Kconfig @@ -443,7 +443,7 @@ config MTD_GPIO_ADDR config MTD_UCLINUX bool "Generic uClinux RAM/ROM filesystem support" - depends on MTD_RAM=y && !MMU + depends on (MTD_RAM=y || MTD_ROM=y) && !MMU help Map driver to support image based filesystems for uClinux. diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c index c3bb304..a8a5929 100644 --- a/drivers/mtd/maps/uclinux.c +++ b/drivers/mtd/maps/uclinux.c @@ -23,12 +23,13 @@ /****************************************************************************/ -struct map_info uclinux_ram_map = { - .name = "RAM", - .phys = (unsigned long)__bss_stop, +static struct map_info uclinux_ram_map = { .size = 0, }; +static unsigned long physaddr = -1; +module_param(physaddr, ulong, S_IRUGO); + static struct mtd_info *uclinux_ram_mtdinfo; /****************************************************************************/ @@ -60,11 +61,17 @@ static int __init uclinux_mtd_init(void) struct map_info *mapp; mapp = &uclinux_ram_map; + + if (physaddr == -1) + mapp->phys = (resource_size_t)__bss_stop; + else + mapp->phys = physaddr; + if (!mapp->size) mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8)))); mapp->bankwidth = 4; - printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", + printk("uclinux[mtd]: RAM/ROM probe address=0x%x size=0x%x\n", (int) mapp->phys, (int) mapp->size); mapp->virt = ioremap_nocache(mapp->phys, mapp->size); @@ -76,9 +83,18 @@ static int __init uclinux_mtd_init(void) simple_map_init(mapp); - mtd = do_map_probe("map_ram", mapp); + mapp->name = "ROM"; + mtd = do_map_probe("map_rom", mapp); + if (!mtd) { + /* fall back to ram probing for compatibility reasons */ + mapp->name = "RAM"; + mtd = do_map_probe("map_ram", mapp); + if (mtd && IS_ENABLED(CONFIG_MTD_ROM)) + pr_err("Failed to map rom, but ram succeeded. Please report this issue!\n"); + } + if (!mtd) { - printk("uclinux[mtd]: failed to find a mapping?\n"); + printk("uclinux[mtd]: failed to find a rom/ram mapping?\n"); iounmap(mapp->virt); return(-ENXIO); } @@ -115,6 +131,6 @@ module_exit(uclinux_mtd_cleanup); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Greg Ungerer <gerg@snapgear.com>"); -MODULE_DESCRIPTION("Generic RAM based MTD for uClinux"); +MODULE_DESCRIPTION("Generic RAM/ROM based MTD for uClinux"); /****************************************************************************/
This allows to put the filesystem at a defined address in ROM allowing to save more precious RAM. I think it's save to default to ROM because the intention of using the uclinux map is to use a romfs and so mtd-ram doesn't give you anything that mtd-rom doesn't. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/mtd/maps/Kconfig | 2 +- drivers/mtd/maps/uclinux.c | 30 +++++++++++++++++++++++------- 2 files changed, 24 insertions(+), 8 deletions(-)