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

login
register
mail settings
Submitter Uwe Kleine-König
Date Oct. 12, 2012, 7:41 a.m.
Message ID <1350027693-19528-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/191066/
State New
Headers show

Comments

Uwe Kleine-König - Oct. 12, 2012, 7:41 a.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>
--
Changed since (implicit) v1:

 - don't make uclinux_ram_map static as pointed out by Mike and Greg.

 drivers/mtd/maps/Kconfig   |    2 +-
 drivers/mtd/maps/uclinux.c |   28 ++++++++++++++++++++++------
 2 files changed, 23 insertions(+), 7 deletions(-)
Greg Ungerer - Oct. 15, 2012, 4:14 a.m.
Hi Uwe,

On 12/10/12 17:41, 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
                ^^^^
                safe

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

Unfortunately email to me goes through an exchange server and openchange,
and it manages to often mangle anything more than 7bit ascii. Not too
much I can do about it, sorry.


> Changed since (implicit) v1:
>
>   - don't make uclinux_ram_map static as pointed out by Mike and Greg.
>
>   drivers/mtd/maps/Kconfig   |    2 +-
>   drivers/mtd/maps/uclinux.c |   28 ++++++++++++++++++++++------
>   2 files changed, 23 insertions(+), 7 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..f5e8e9a 100644
> --- a/drivers/mtd/maps/uclinux.c
> +++ b/drivers/mtd/maps/uclinux.c
> @@ -24,11 +24,12 @@
>   /****************************************************************************/
>
>   struct map_info uclinux_ram_map = {
> -	.name = "RAM",
> -	.phys = (unsigned long)__bss_stop,
>   	.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);

Is there any value in saying "RAM/ROM"?
Why don't we just drop those chars altogether.


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

Do we really want this message?
My predominate usage of this code is in RAM mappings. Network
loading kernel+filesystem images on bare boards. Anyone who wants
to know and is looking in the kernel boot messages will see
something like:

     Creating 1 MTD partitions on "RAM":
     0x000000000000-0x0000000d8000 : "ROMfs"

So they will know what type of mapping it was loaded from.


> +	}
> +
>   	if (!mtd) {
> -		printk("uclinux[mtd]: failed to find a mapping?\n");
> +		printk("uclinux[mtd]: failed to find a rom/ram mapping?\n");

Again, is there any point in changing this message?
Adding "rom/ram" doesn't add any value here.

Regards
Greg



>   		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. 15, 2012, 6:58 a.m.
Hello,

On Mon, Oct 15, 2012 at 02:14:55PM +1000, Greg Ungerer wrote:
> Hi Uwe,
> 
> On 12/10/12 17:41, 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
>                ^^^^
>                safe
> 
> >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>
> 
> Unfortunately email to me goes through an exchange server and openchange,
> and it manages to often mangle anything more than 7bit ascii. Not too
> much I can do about it, sorry.
hehe, so openchange is really MS compatible now? :-)
 
> >Changed since (implicit) v1:
> >
> >  - don't make uclinux_ram_map static as pointed out by Mike and Greg.
> >
> >  drivers/mtd/maps/Kconfig   |    2 +-
> >  drivers/mtd/maps/uclinux.c |   28 ++++++++++++++++++++++------
> >  2 files changed, 23 insertions(+), 7 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..f5e8e9a 100644
> >--- a/drivers/mtd/maps/uclinux.c
> >+++ b/drivers/mtd/maps/uclinux.c
> >@@ -24,11 +24,12 @@
> >  /****************************************************************************/
> >
> >  struct map_info uclinux_ram_map = {
> >-	.name = "RAM",
> >-	.phys = (unsigned long)__bss_stop,
> >  	.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);
> 
> Is there any value in saying "RAM/ROM"?
> Why don't we just drop those chars altogether.
ok.

> >  	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");
> 
> Do we really want this message?
> My predominate usage of this code is in RAM mappings. Network
> loading kernel+filesystem images on bare boards. Anyone who wants
> to know and is looking in the kernel boot messages will see
> something like:
> 
>     Creating 1 MTD partitions on "RAM":
>     0x000000000000-0x0000000d8000 : "ROMfs"
> 
> So they will know what type of mapping it was loaded from.
I want it because if nobody reports it it might well be possible to drop
map_ram support.

Best regards
Uwe
Greg Ungerer - Oct. 16, 2012, 1:13 a.m.
Hi Uwe,

On 15/10/12 16:58, Uwe Kleine-König wrote:
> On Mon, Oct 15, 2012 at 02:14:55PM +1000, Greg Ungerer wrote:
>>>   	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");
>>
>> Do we really want this message?
>> My predominate usage of this code is in RAM mappings. Network
>> loading kernel+filesystem images on bare boards. Anyone who wants
>> to know and is looking in the kernel boot messages will see
>> something like:
>>
>>      Creating 1 MTD partitions on "RAM":
>>      0x000000000000-0x0000000d8000 : "ROMfs"
>>
>> So they will know what type of mapping it was loaded from.
> I want it because if nobody reports it it might well be possible to drop
> map_ram support.

I am not sure I follow. The message is only printed if both ROM and
RAM mappings are enabled. Many of the configs I use only have RAM
mappings enabled.

Regards
Greg

  
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com
Uwe Kleine-König - Oct. 16, 2012, 6:56 a.m.
Hello Greg,

On Tue, Oct 16, 2012 at 11:13:05AM +1000, Greg Ungerer wrote:
> On 15/10/12 16:58, Uwe Kleine-König wrote:
> >On Mon, Oct 15, 2012 at 02:14:55PM +1000, Greg Ungerer wrote:
> >>>  	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");
> >>
> >>Do we really want this message?
> >>My predominate usage of this code is in RAM mappings. Network
> >>loading kernel+filesystem images on bare boards. Anyone who wants
> >>to know and is looking in the kernel boot messages will see
> >>something like:
> >>
> >>     Creating 1 MTD partitions on "RAM":
> >>     0x000000000000-0x0000000d8000 : "ROMfs"
> >>
> >>So they will know what type of mapping it was loaded from.
> >I want it because if nobody reports it it might well be possible to drop
> >map_ram support.
> 
> I am not sure I follow. The message is only printed if both ROM and
> RAM mappings are enabled. Many of the configs I use only have RAM
> mappings enabled.
Yeah, in this case it doesn't help. I also thought about adding a
warning for !IS_ENABLED(CONFIG_MTD_ROM). But this seemed too harsh, so I
dropped the idea.

Still, the theory is that MTD_ROM should be enough for the uclinux map.
If it's not it's at least not silently fixed up and we might get a
report about what was wrong with my assumption.

Best regards
Uwe

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..f5e8e9a 100644
--- a/drivers/mtd/maps/uclinux.c
+++ b/drivers/mtd/maps/uclinux.c
@@ -24,11 +24,12 @@ 
 /****************************************************************************/
 
 struct map_info uclinux_ram_map = {
-	.name = "RAM",
-	.phys = (unsigned long)__bss_stop,
 	.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");
 
 /****************************************************************************/