Patchwork [RFC] mtd/uclinux: support ROM and allow passing the base address

login
register
mail settings
Submitter Uwe Kleine-König
Date Oct. 8, 2012, 3:25 p.m.
Message ID <1349709952-4332-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/190050/
State New
Headers show

Comments

Uwe Kleine-König - Oct. 8, 2012, 3:25 p.m.
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(-)
Greg Ungerer - Oct. 9, 2012, 3:29 a.m.
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");
>
>   /****************************************************************************/
>
Uwe Kleine-König - Oct. 9, 2012, 8:44 a.m.
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
Mike Frysinger - Oct. 11, 2012, 4:21 a.m.
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
Uwe Kleine-König - Oct. 11, 2012, 6:13 a.m.
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
Uwe Kleine-König - Oct. 12, 2012, 7:41 a.m.
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
Mike Frysinger - Oct. 12, 2012, 4:18 p.m.
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

Patch

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");
 
 /****************************************************************************/