Patchwork [2/2] mtd/maps: uclinux: support Blackfin systems

login
register
mail settings
Submitter Mike Frysinger
Date May 26, 2009, 9:46 a.m.
Message ID <1243331191-11445-2-git-send-email-vapier@gentoo.org>
Download mbox | patch
Permalink /patch/27638/
State New, archived
Headers show

Comments

Mike Frysinger - May 26, 2009, 9:46 a.m.
Due to a processor anomaly (05000263 to be exact), most Blackfin parts
cannot keep the embedded filesystem image directly after the kernel in
RAM.  Instead, the filesystem needs to be relocated to the end of memory.
As such, we need to tweak the initial filesystem address for Blackfin
systems.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
CC: Greg Ungerer <gerg@uclinux.org>
CC: uclinux-dev@uclinux.org
CC: linux-mtd@lists.infradead.org
---
 drivers/mtd/maps/uclinux.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Paul Mundt - May 26, 2009, 11:31 a.m.
On Tue, May 26, 2009 at 05:46:31AM -0400, Mike Frysinger wrote:
> diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
> index 57699c2..dcb552f 100644
> --- a/drivers/mtd/maps/uclinux.c
> +++ b/drivers/mtd/maps/uclinux.c
> @@ -55,8 +55,13 @@ static int __init uclinux_mtd_init(void)
>  {
>  	struct mtd_info *mtd;
>  	struct map_info *mapp;
> +#ifdef CONFIG_BLACKFIN
> +	extern unsigned long memory_mtd_start;
> +	unsigned long addr = (unsigned long) memory_mtd_start;
> +#else
>  	extern char _ebss;
>  	unsigned long addr = (unsigned long) &_ebss;
> +#endif
>  
>  	mapp = &uclinux_ram_map;
>  	mapp->phys = addr;

NAK.

I know there's no accounting for taste, but it would be nice to at least
see some minimal amount of effort going in to fixing these things
sanely rather than just lazily shoving ifdefs in wherever possible.

In this case you should just kill all of that crap off, and have the
platforms that use this set uclinux_ram_map up themselves, it's already
a global. Of course you can use _ebss as a default value for
uclinux_ram_map.phys and just override it in your platform.
Mike Frysinger - May 26, 2009, 4:42 p.m.
On Tue, May 26, 2009 at 07:31, Paul Mundt wrote:
> On Tue, May 26, 2009 at 05:46:31AM -0400, Mike Frysinger wrote:
>> diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
>> index 57699c2..dcb552f 100644
>> --- a/drivers/mtd/maps/uclinux.c
>> +++ b/drivers/mtd/maps/uclinux.c
>> @@ -55,8 +55,13 @@ static int __init uclinux_mtd_init(void)
>>  {
>>       struct mtd_info *mtd;
>>       struct map_info *mapp;
>> +#ifdef CONFIG_BLACKFIN
>> +     extern unsigned long memory_mtd_start;
>> +     unsigned long addr = (unsigned long) memory_mtd_start;
>> +#else
>>       extern char _ebss;
>>       unsigned long addr = (unsigned long) &_ebss;
>> +#endif
>>
>>       mapp = &uclinux_ram_map;
>>       mapp->phys = addr;
>
> NAK.
>
> I know there's no accounting for taste, but it would be nice to at least
> see some minimal amount of effort going in to fixing these things
> sanely rather than just lazily shoving ifdefs in wherever possible.
>
> In this case you should just kill all of that crap off, and have the
> platforms that use this set uclinux_ram_map up themselves, it's already
> a global. Of course you can use _ebss as a default value for
> uclinux_ram_map.phys and just override it in your platform.

i would agree if it were a board-specific issue, but it's an arch
issue, so pushing it to the boards level is wrong.  i can however
replace the addr with a global weak and add a symbol into the Blackfin
arch code to override it.

the structs being global in this file is simply wrong from what i can
tell ... everything in there should be static.  i'll send a patch for
that later on.
-mike
Paul Mundt - May 26, 2009, 4:47 p.m.
On Tue, May 26, 2009 at 12:42:48PM -0400, Mike Frysinger wrote:
> On Tue, May 26, 2009 at 07:31, Paul Mundt wrote:
> > On Tue, May 26, 2009 at 05:46:31AM -0400, Mike Frysinger wrote:
> >> diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
> >> index 57699c2..dcb552f 100644
> >> --- a/drivers/mtd/maps/uclinux.c
> >> +++ b/drivers/mtd/maps/uclinux.c
> >> @@ -55,8 +55,13 @@ static int __init uclinux_mtd_init(void)
> >> ??{
> >> ?? ?? ?? struct mtd_info *mtd;
> >> ?? ?? ?? struct map_info *mapp;
> >> +#ifdef CONFIG_BLACKFIN
> >> + ?? ?? extern unsigned long memory_mtd_start;
> >> + ?? ?? unsigned long addr = (unsigned long) memory_mtd_start;
> >> +#else
> >> ?? ?? ?? extern char _ebss;
> >> ?? ?? ?? unsigned long addr = (unsigned long) &_ebss;
> >> +#endif
> >>
> >> ?? ?? ?? mapp = &uclinux_ram_map;
> >> ?? ?? ?? mapp->phys = addr;
> >
> > NAK.
> >
> > I know there's no accounting for taste, but it would be nice to at least
> > see some minimal amount of effort going in to fixing these things
> > sanely rather than just lazily shoving ifdefs in wherever possible.
> >
> > In this case you should just kill all of that crap off, and have the
> > platforms that use this set uclinux_ram_map up themselves, it's already
> > a global. Of course you can use _ebss as a default value for
> > uclinux_ram_map.phys and just override it in your platform.
> 
> i would agree if it were a board-specific issue, but it's an arch
> issue, so pushing it to the boards level is wrong.  i can however
> replace the addr with a global weak and add a symbol into the Blackfin
> arch code to override it.
> 
I obviously meant architectures setting up the address, not the board
code, as this has nothing at all to do with boards. There are already
plenty of cases in setup_arch() for filling in uclinux mtd data, one more
isn't going to make a difference.

I don't see anything wrong with keeping uclinux_ram_map as a global
however, particularly since platforms that need to special case the
mapping can easily do this under the existing ifdef. Adding weak symbols
for something like this just seems silly.
Mike Frysinger - May 26, 2009, 4:50 p.m.
On Tue, May 26, 2009 at 12:47, Paul Mundt wrote:
> On Tue, May 26, 2009 at 12:42:48PM -0400, Mike Frysinger wrote:
>> On Tue, May 26, 2009 at 07:31, Paul Mundt wrote:
>> > On Tue, May 26, 2009 at 05:46:31AM -0400, Mike Frysinger wrote:
>> >> diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
>> >> index 57699c2..dcb552f 100644
>> >> --- a/drivers/mtd/maps/uclinux.c
>> >> +++ b/drivers/mtd/maps/uclinux.c
>> >> @@ -55,8 +55,13 @@ static int __init uclinux_mtd_init(void)
>> >> ??{
>> >> ?? ?? ?? struct mtd_info *mtd;
>> >> ?? ?? ?? struct map_info *mapp;
>> >> +#ifdef CONFIG_BLACKFIN
>> >> + ?? ?? extern unsigned long memory_mtd_start;
>> >> + ?? ?? unsigned long addr = (unsigned long) memory_mtd_start;
>> >> +#else
>> >> ?? ?? ?? extern char _ebss;
>> >> ?? ?? ?? unsigned long addr = (unsigned long) &_ebss;
>> >> +#endif
>> >>
>> >> ?? ?? ?? mapp = &uclinux_ram_map;
>> >> ?? ?? ?? mapp->phys = addr;
>> >
>> > NAK.
>> >
>> > I know there's no accounting for taste, but it would be nice to at least
>> > see some minimal amount of effort going in to fixing these things
>> > sanely rather than just lazily shoving ifdefs in wherever possible.
>> >
>> > In this case you should just kill all of that crap off, and have the
>> > platforms that use this set uclinux_ram_map up themselves, it's already
>> > a global. Of course you can use _ebss as a default value for
>> > uclinux_ram_map.phys and just override it in your platform.
>>
>> i would agree if it were a board-specific issue, but it's an arch
>> issue, so pushing it to the boards level is wrong.  i can however
>> replace the addr with a global weak and add a symbol into the Blackfin
>> arch code to override it.
>>
> I obviously meant architectures setting up the address, not the board
> code, as this has nothing at all to do with boards. There are already
> plenty of cases in setup_arch() for filling in uclinux mtd data, one more
> isn't going to make a difference.
>
> I don't see anything wrong with keeping uclinux_ram_map as a global
> however, particularly since platforms that need to special case the
> mapping can easily do this under the existing ifdef. Adding weak symbols
> for something like this just seems silly.

the point of the weak symbol was so that the map could provide a sane
default that works for most everyone out there without having to copy
& paste the same code to every arch, and to make new arch porters
worry about what needs to be done to use this very trivial map
-mike

Patch

diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c
index 57699c2..dcb552f 100644
--- a/drivers/mtd/maps/uclinux.c
+++ b/drivers/mtd/maps/uclinux.c
@@ -55,8 +55,13 @@  static int __init uclinux_mtd_init(void)
 {
 	struct mtd_info *mtd;
 	struct map_info *mapp;
+#ifdef CONFIG_BLACKFIN
+	extern unsigned long memory_mtd_start;
+	unsigned long addr = (unsigned long) memory_mtd_start;
+#else
 	extern char _ebss;
 	unsigned long addr = (unsigned long) &_ebss;
+#endif
 
 	mapp = &uclinux_ram_map;
 	mapp->phys = addr;