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

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

[Uwe Kleine-König]

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

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

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

[Greg Ungerer]

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


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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Greg Ungerer]

Hi Uwe,

On 10/09/2012 06:44 PM, Uwe Kleine-König wrote:
> 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.

Sure. All three patches I sent to the linux-mtd list are here:

The following changes since commit 979570e02981d4a8fc20b3cc8fd651856c98ee9d:
   Linus Torvalds (1):
         Linux 3.6-rc7

are available in the git repository at:

   git://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu.git cfmmu

Greg Ungerer (3):
       mtd: fix wrong usage of ioremap_nocache() in uclinux.c map driver
       mtd: allow uclinux map driver to be used on any ColdFire CPU platform
       mtd: clean up uclinux.c map driver

  drivers/mtd/maps/Kconfig   |    2 +-
  drivers/mtd/maps/uclinux.c |   42 
+++++++++++++++++++-----------------------
  2 files changed, 20 insertions(+), 24 deletions(-)


As far as I can see Artem has pushed the first 2 into for linux-next,
but not the third. I am hoping he hops in here and lets us know what
his intentions are with the third...

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close,                            FAX:         +61 7 3891 3630
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

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

[Uwe Kleine-König]

Hello,

On Tue, Oct 09, 2012 at 08:37:09PM +1000, Greg Ungerer wrote:
> On 10/09/2012 06:44 PM, Uwe Kleine-König wrote:
> >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.
> 
> Sure. All three patches I sent to the linux-mtd list are here:
> 
> The following changes since commit 979570e02981d4a8fc20b3cc8fd651856c98ee9d:
>   Linus Torvalds (1):
>         Linux 3.6-rc7
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu.git cfmmu
> 
> Greg Ungerer (3):
>       mtd: fix wrong usage of ioremap_nocache() in uclinux.c map driver
>       mtd: allow uclinux map driver to be used on any ColdFire CPU platform
>       mtd: clean up uclinux.c map driver
> 
>  drivers/mtd/maps/Kconfig   |    2 +-
>  drivers/mtd/maps/uclinux.c |   42
> +++++++++++++++++++-----------------------
>  2 files changed, 20 insertions(+), 24 deletions(-)
> 
> 
> As far as I can see Artem has pushed the first 2 into for linux-next,
> but not the third. I am hoping he hops in here and lets us know what
> his intentions are with the third...
This branch merged into my tree still works fine.

Something I noticed is that the printk cleanups could look a bit nicer
if you used

	#define pr_fmt(fmt)	"uclinux[mtd]: " fmt

and then pr_notice et al. But maybe this is only subjective.

Also I'm not sure if all the comments are still correct with my change.

(And btw, your mailer is broken regarding line breaking and non-ascii
characters.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Mike Frysinger]

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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Mike Frysinger]

On Tue, Oct 9, 2012 at 6:37 AM, Greg Ungerer <gerg@snapgear.com> wrote:
> On 10/09/2012 06:44 PM, Uwe Kleine-König wrote:
>> 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.
>
> Sure. All three patches I sent to the linux-mtd list are here:
>
> The following changes since commit 979570e02981d4a8fc20b3cc8fd651856c98ee9d:
>   Linus Torvalds (1):
>         Linux 3.6-rc7
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu.git cfmmu
>
> Greg Ungerer (3):
>       mtd: fix wrong usage of ioremap_nocache() in uclinux.c map driver
>       mtd: allow uclinux map driver to be used on any ColdFire CPU platform
>       mtd: clean up uclinux.c map driver
>
>  drivers/mtd/maps/Kconfig   |    2 +-
>  drivers/mtd/maps/uclinux.c |   42
> +++++++++++++++++++-----------------------
>  2 files changed, 20 insertions(+), 24 deletions(-)
>
>
> As far as I can see Artem has pushed the first 2 into for linux-next,
> but not the third. I am hoping he hops in here and lets us know what
> his intentions are with the third...

so to be clear, Uwe's patch hasn't been picked up by anyone yet so we
can get it fixed first ?
-mike

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

[Greg Ungerer]

On 12/10/12 14:46, Mike Frysinger wrote:
> On Tue, Oct 9, 2012 at 6:37 AM, Greg Ungerer <gerg@snapgear.com> wrote:
>> On 10/09/2012 06:44 PM, Uwe Kleine-K÷nig wrote:
>>> 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.
>>
>> Sure. All three patches I sent to the linux-mtd list are here:
>>
>> The following changes since commit 979570e02981d4a8fc20b3cc8fd651856c98ee9d:
>>    Linus Torvalds (1):
>>          Linux 3.6-rc7
>>
>> are available in the git repository at:
>>
>>    git://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu.git cfmmu
>>
>> Greg Ungerer (3):
>>        mtd: fix wrong usage of ioremap_nocache() in uclinux.c map driver
>>        mtd: allow uclinux map driver to be used on any ColdFire CPU platform
>>        mtd: clean up uclinux.c map driver
>>
>>   drivers/mtd/maps/Kconfig   |    2 +-
>>   drivers/mtd/maps/uclinux.c |   42
>> +++++++++++++++++++-----------------------
>>   2 files changed, 20 insertions(+), 24 deletions(-)
>>
>>
>> As far as I can see Artem has pushed the first 2 into for linux-next,
>> but not the third. I am hoping he hops in here and lets us know what
>> his intentions are with the third...
>
> so to be clear, Uwe's patch hasn't been picked up by anyone yet so we
> can get it fixed first ?

I haven't picked it up. I assumed Artem would, but I have not seen
any response from him on this yet.

I thought you where going to put a comment in there,
http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051873.html

In any case, Uwe, that thread states what the issue is when it came
up a few months back.

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

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

[Uwe Kleine-König]

Hello,

On Fri, Oct 12, 2012 at 03:57:59PM +1000, Greg Ungerer wrote:
> >so to be clear, Uwe's patch hasn't been picked up by anyone yet so we
> >can get it fixed first ?
ok, so for blackfin uclinux_ram_map must not be static because it is
used in arch/blackfin/kernel/setup.c. If I tried to introduce such code
for an ARM platform people would run away scared maybe after telling me
I should fix my boot loader.  I really don't know the situation on
blackfin, but this really has potential for generalisation. But I don't
even know if you have/need a boot loader at all there. Here is what I
found quickly:

diff --git a/arch/blackfin/kernel/setup.c b/arch/blackfin/kernel/setup.c
index fb96e60..24478a2 100644
--- a/arch/blackfin/kernel/setup.c
+++ b/arch/blackfin/kernel/setup.c
@@ -583,7 +583,7 @@ static __init void memory_setup(void)
 
 #ifdef CONFIG_MPU
 	/* Round up to multiple of 4MB */
-	memory_start = (_ramstart + 0x3fffff) & ~0x3fffff;
+	memory_start = ALIGN(_ramstart, 0x400000);
 #else
 	memory_start = PAGE_ALIGN(_ramstart);
 #endif
@@ -593,6 +593,13 @@ static __init void memory_setup(void)
 	memory_mtd_end = memory_end;
 
 	mtd_phys = _ramstart;
+
+	/*
+	 * Since the default MTD_UCLINUX has no magic number, we just blindly
+	 * read 8 past the end of the kernel's image, and look at it.
+	 * When no image is attached, mtd_size is set to a random number.
+	 * So do some basic sanity checks before operating on things.
+	 */
 	mtd_size = PAGE_ALIGN(*((unsigned long *)(mtd_phys + 8)));
 
 # if defined(CONFIG_EXT2_FS) || defined(CONFIG_EXT3_FS)
@@ -621,11 +628,6 @@ static __init void memory_setup(void)
 	}
 # endif				/* CONFIG_ROMFS_FS */
 
-	/* Since the default MTD_UCLINUX has no magic number, we just blindly
-	 * read 8 past the end of the kernel's image, and look at it.
-	 * When no image is attached, mtd_size is set to a random number
-	 * Do some basic sanity checks before operating on things
-	 */
 	if (mtd_size == 0 || memory_end <= mtd_size) {
 		pr_emerg("Could not find valid ram mtd attached.\n");
 	} else {

If you had a boot loader, my patch (making the location of the uclinux
map settable by a kernel parameter) should be a move in the right
direction for you, too.

I will fix my patch keeping uclinux_ram_map non-static and resend.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

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

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

[PATCH 2/2] mtd/uclinux: add a comment about why uclinux_ram_map must not be static

[Uwe Kleine-König]

I was (at least) the second person trying to fix a warning by sparse, so
document in the code why this is a bad idea.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

it's a bit exaggerated to claim that sparse is happy now, but at least
we have one warning less :-)

Uwe
 drivers/mtd/maps/uclinux.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
index f5e8e9a..18cf6d0 100644
--- a/drivers/mtd/maps/uclinux.c
+++ b/drivers/mtd/maps/uclinux.c
@@ -23,6 +23,12 @@
 
 /****************************************************************************/
 
+/*
+ * Blackfin uses uclinux_ram_map during startup, so it must not be static.
+ * Provide a dummy declaration to make sparse happy.
+ */
+extern struct map_info uclinux_ram_map;
+
 struct map_info uclinux_ram_map = {
 	.size = 0,
 };
-- 
1.7.10.4

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

[Mike Frysinger]

On Fri, Oct 12, 2012 at 3:22 AM, Uwe Kleine-König wrote:
> On Fri, Oct 12, 2012 at 03:57:59PM +1000, Greg Ungerer wrote:
>> >so to be clear, Uwe's patch hasn't been picked up by anyone yet so we
>> >can get it fixed first ?
>
> ok, so for blackfin uclinux_ram_map must not be static because it is
> used in arch/blackfin/kernel/setup.c. If I tried to introduce such code
> for an ARM platform people would run away scared maybe after telling me
> I should fix my boot loader.

i would point out that code merged many many years ago had a much
lower bar than today.  also, this isn't the boot loader passing the
rootfs into the kernel.  when the kernel is created, the build system
appends the rootfs to it.

> I really don't know the situation on
> blackfin, but this really has potential for generalisation.

sure ... we're open to doing it differently as long as it's backwards compatible

> But I don't even know if you have/need a boot loader at all there.

we run u-boot like everyone else, but u-boot doesn't handle passing
the rootfs in.

> Here is what I found quickly:

updated comments here look nice, thanks
-mike

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

[Mike Frysinger]

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

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

[Mike Frysinger]

On Fri, Oct 12, 2012 at 12:09 PM, Mike Frysinger wrote:
> On Fri, Oct 12, 2012 at 3:22 AM, Uwe Kleine-König wrote:
>> I really don't know the situation on
>> blackfin, but this really has potential for generalisation.
>
> sure ... we're open to doing it differently as long as it's backwards compatible

thinking a bit more, i think the answer here is:
 - move all the mtd_size checks out of Blackfin's memory_setup() into
the uclinux mtd map's uclinux_mtd_init() logic
 - rename your new physaddr to "uclinux_ram_map_physaddr", drop the
"static", and add a comment that this needs to stay externally visible
 - change Blackfin's memory_setup() logic to set
"uclinux_ram_map_physaddr" rather than "uclinux_ram_map"
-mike

Re: [PATCH 2/2] mtd/uclinux: add a comment about why uclinux_ram_map must not be static

[Mike Frysinger]

On Fri, Oct 12, 2012 at 3:41 AM, Uwe Kleine-König wrote:
> I was (at least) the second person trying to fix a warning by sparse, so
> document in the code why this is a bad idea.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

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

[Greg Ungerer]

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


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

Re: [PATCH 2/2] mtd/uclinux: add a comment about why uclinux_ram_map must not be static

[Greg Ungerer]

On 12/10/12 17:41, Uwe Kleine-König wrote:
> I was (at least) the second person trying to fix a warning by sparse, so
> document in the code why this is a bad idea.
>
> Signed-off-by: Uwe Kleine-K÷nig <u.kleine-koenig@pengutronix.de>

Acked-by: Greg Ungerer <gerg@uclinux.org>


> ---
> Hello,
>
> it's a bit exaggerated to claim that sparse is happy now, but at least
> we have one warning less :-)
>
> Uwe
>   drivers/mtd/maps/uclinux.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
> index f5e8e9a..18cf6d0 100644
> --- a/drivers/mtd/maps/uclinux.c
> +++ b/drivers/mtd/maps/uclinux.c
> @@ -23,6 +23,12 @@
>
>   /****************************************************************************/
>
> +/*
> + * Blackfin uses uclinux_ram_map during startup, so it must not be static.
> + * Provide a dummy declaration to make sparse happy.
> + */
> +extern struct map_info uclinux_ram_map;
> +
>   struct map_info uclinux_ram_map = {
>   	.size = 0,
>   };
>


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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

On Fri, 2012-10-12 at 15:57 +1000, Greg Ungerer wrote:
> I haven't picked it up. I assumed Artem would, but I have not seen
> any response from him on this yet. 

I did not pick up anything. Also, your patches are now in Linuses tree.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Greg Ungerer]

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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |