diff mbox

[U-Boot] NAND: Allow nand_ids and nand_bbt to be compiled in SPL

Message ID 201201080456.54992.vapier@gentoo.org
State Rejected
Delegated to: Scott Wood
Headers show

Commit Message

Mike Frysinger Jan. 8, 2012, 9:56 a.m. UTC 
On Wednesday 04 January 2012 18:56:23 Scott Wood wrote:
> On 12/05/2011 05:17 PM, Marek Vasut wrote:
> > This will be beneficial for the PXA3XX NAND driver, which uses the NAND
> > IDs to identify the chip and configure the controller accordingly.
> > 
> > --- a/drivers/mtd/nand/Makefile
> > +++ b/drivers/mtd/nand/Makefile
> > 
> >  endif
> >  else
> >  COBJS-y += nand.o
> > -COBJS-y += nand_bbt.o
> > -COBJS-y += nand_ids.o
> >  COBJS-y += nand_util.o
> >  endif
> > +COBJS-y += nand_bbt.o
> > +COBJS-y += nand_ids.o
> >  COBJS-y += nand_ecc.o
> >  COBJS-y += nand_base.o
> 
> So, in theory with gc-sections this shouldn't increase the size of any
> SPL that currently successfully links (at least in the absence of things
> like weak symbols).  However, I observed a devkit8000 build go from this:
> 
>    text    data     bss     dec     hex filename
>   40709    1792  197764  240265   3aa89 /tmp/u-boot-arm/spl/u-boot-spl
> 
> to this:
> 
>    text    data     bss     dec     hex filename
>   42277    1792  197764  241833   3b0a9 /tmp/u-boot-arm/spl/u-boot-spl
> 
> I verified that --function-sections/-fdata-sections/--gc-sections are
> being used on the SPL.  It looks like strings are not getting dropped.

specifically, "anonymous" strings (or whatever the term is).  it could be made 
to work, but it'd be ugly.  something like:

-mike

Comments

Scott Wood Jan. 9, 2012, 7:41 p.m. UTC | #1 
On 01/08/2012 03:56 AM, Mike Frysinger wrote:
> On Wednesday 04 January 2012 18:56:23 Scott Wood wrote:
>> On 12/05/2011 05:17 PM, Marek Vasut wrote:
>>> This will be beneficial for the PXA3XX NAND driver, which uses the NAND
>>> IDs to identify the chip and configure the controller accordingly.
>>>
>>> --- a/drivers/mtd/nand/Makefile
>>> +++ b/drivers/mtd/nand/Makefile
>>>
>>>  endif
>>>  else
>>>  COBJS-y += nand.o
>>> -COBJS-y += nand_bbt.o
>>> -COBJS-y += nand_ids.o
>>>  COBJS-y += nand_util.o
>>>  endif
>>> +COBJS-y += nand_bbt.o
>>> +COBJS-y += nand_ids.o
>>>  COBJS-y += nand_ecc.o
>>>  COBJS-y += nand_base.o
>>
>> So, in theory with gc-sections this shouldn't increase the size of any
>> SPL that currently successfully links (at least in the absence of things
>> like weak symbols).  However, I observed a devkit8000 build go from this:
>>
>>    text    data     bss     dec     hex filename
>>   40709    1792  197764  240265   3aa89 /tmp/u-boot-arm/spl/u-boot-spl
>>
>> to this:
>>
>>    text    data     bss     dec     hex filename
>>   42277    1792  197764  241833   3b0a9 /tmp/u-boot-arm/spl/u-boot-spl
>>
>> I verified that --function-sections/-fdata-sections/--gc-sections are
>> being used on the SPL.  It looks like strings are not getting dropped.
> 
> specifically, "anonymous" strings (or whatever the term is).  it could be made 
> to work, but it'd be ugly.  something like:
> 
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -22,6 +22,12 @@
>  +	256	256 Byte page size
>  *	512	512 Byte page size
>  */
> +
> +static const char nand_16mib_18v_8bit[] = "NAND 16MiB 1,8V 8-bit";
> +static const char nand_16mib_33v_8bit[] = "NAND 16MiB 3,3V 8-bit";
> +static const char nand_16mib_18v_16bit[] = "NAND 16MiB 1,8V 16-bit";
> +static const char nand_16mib_33v_16bit[] = "NAND 16MiB 3,3V 16-bit";
> +
>  const struct nand_flash_dev nand_flash_ids[] = {
>  
>  #ifdef CONFIG_MTD_NAND_MUSEUM_IDS
> @@ -42,10 +48,10 @@
>  	{"NAND 8MiB 3,3V 16-bit",	0x59, 512, 8, 0x2000, NAND_BUSWIDTH_16},
>  #endif
>  
> -	{"NAND 16MiB 1,8V 8-bit",	0x33, 512, 16, 0x4000, 0},
> -	{"NAND 16MiB 3,3V 8-bit",	0x73, 512, 16, 0x4000, 0},
> -	{"NAND 16MiB 1,8V 16-bit",	0x43, 512, 16, 0x4000, NAND_BUSWIDTH_16},
> -	{"NAND 16MiB 3,3V 16-bit",	0x53, 512, 16, 0x4000, NAND_BUSWIDTH_16},
> +	{nand_16mib_18v_8bit,	0x33, 512, 16, 0x4000, 0},
> +	{nand_16mib_33v_8bit,	0x73, 512, 16, 0x4000, 0},
> +	{nand_16mib_18v_16bit,	0x43, 512, 16, 0x4000, NAND_BUSWIDTH_16},
> +	{nand_16mib_33v_16bit,	0x53, 512, 16, 0x4000, NAND_BUSWIDTH_16},
>  
>  	{"NAND 32MiB 1,8V 8-bit",	0x35, 512, 32, 0x4000, 0},
>  	{"NAND 32MiB 3,3V 8-bit",	0x75, 512, 32, 0x4000, 0},

Ugly, and we'd get regressions every time someone adds a new anonymous
string that pushes an SPL over the limit.

Unless/until the toolchain can properly GC anonymous strings, I think
finer-grained conditional compilation is the way to go.

-Scott
Mike Frysinger Jan. 9, 2012, 9:21 p.m. UTC | #2 
On Monday 09 January 2012 14:41:07 Scott Wood wrote:
> Unless/until the toolchain can properly GC anonymous strings, I think
> finer-grained conditional compilation is the way to go.

i think we should use GC when it works (we've found 1 case so far where it 
doesn't), and add fine grained compilation when it doesn't (with a note with 
expected working versions).  i think this is a bug in the linker and have been 
trying to make a reduced test case that i can post to the GNU binutils peeps.
-mike
Tom Rini Jan. 9, 2012, 9:23 p.m. UTC | #3 
On Mon, Jan 9, 2012 at 2:21 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 09 January 2012 14:41:07 Scott Wood wrote:
>> Unless/until the toolchain can properly GC anonymous strings, I think
>> finer-grained conditional compilation is the way to go.
>
> i think we should use GC when it works (we've found 1 case so far where it
> doesn't), and add fine grained compilation when it doesn't (with a note with
> expected working versions).  i think this is a bug in the linker and have been
> trying to make a reduced test case that i can post to the GNU binutils peeps.

We've found two cases, at least.  Functions aren't being dropped out either.
Tom Rini Jan. 9, 2012, 9:25 p.m. UTC | #4 
On Mon, Jan 9, 2012 at 2:23 PM, Tom Rini <tom.rini@gmail.com> wrote:
> On Mon, Jan 9, 2012 at 2:21 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> On Monday 09 January 2012 14:41:07 Scott Wood wrote:
>>> Unless/until the toolchain can properly GC anonymous strings, I think
>>> finer-grained conditional compilation is the way to go.
>>
>> i think we should use GC when it works (we've found 1 case so far where it
>> doesn't), and add fine grained compilation when it doesn't (with a note with
>> expected working versions).  i think this is a bug in the linker and have been
>> trying to make a reduced test case that i can post to the GNU binutils peeps.
>
> We've found two cases, at least.  Functions aren't being dropped out either.

To be clear, https://github.com/trini/u-boot/commit/3a7c313125eff000ac73e38d220e5f9615c1abbe
shows switching everything to the normal nand codebase, but with a
gain of only 170 bytes.  The reason it's such an otherwise small delta
is that I added some #ifndef CONFIG_SPL_BUILD to drop out unused code
that the linker should have been dropping for us, but wasn't.
Scott Wood Jan. 9, 2012, 9:27 p.m. UTC | #5 
On 01/09/2012 03:21 PM, Mike Frysinger wrote:
> On Monday 09 January 2012 14:41:07 Scott Wood wrote:
>> Unless/until the toolchain can properly GC anonymous strings, I think
>> finer-grained conditional compilation is the way to go.
> 
> i think we should use GC when it works (we've found 1 case so far where it 
> doesn't), and add fine grained compilation when it doesn't (with a note with 
> expected working versions).

Anonymous strings are hardly a rare occurance.  Plus, doing it on a case
by cases invites breakage from people who don't/can't test the SPLs that
are affected, when they add code that moves a certain file from being
not-problematic to problematic.

> i think this is a bug in the linker and have been 
> trying to make a reduced test case that i can post to the GNU binutils peeps.

If and when they fix it, and the updated toolchains are in sufficiently
wide use that we can stop supporting the old ones (at least for small
SPL targets), we can revisit the decision.

-Scott
Mike Frysinger Jan. 10, 2012, 6:25 p.m. UTC | #6 
On Monday 09 January 2012 16:27:44 Scott Wood wrote:
> On 01/09/2012 03:21 PM, Mike Frysinger wrote:
> > On Monday 09 January 2012 14:41:07 Scott Wood wrote:
> >> Unless/until the toolchain can properly GC anonymous strings, I think
> >> finer-grained conditional compilation is the way to go.
> > 
> > i think we should use GC when it works (we've found 1 case so far where
> > it doesn't), and add fine grained compilation when it doesn't (with a
> > note with expected working versions).
> 
> Anonymous strings are hardly a rare occurance.

it isn't all anonymous strings.  there's something more going on here that is 
tickling the bug.
-mike
Mike Frysinger Jan. 10, 2012, 6:25 p.m. UTC | #7 
On Monday 09 January 2012 16:25:41 Tom Rini wrote:
> On Mon, Jan 9, 2012 at 2:23 PM, Tom Rini <tom.rini@gmail.com> wrote:
> > On Mon, Jan 9, 2012 at 2:21 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> >> On Monday 09 January 2012 14:41:07 Scott Wood wrote:
> >>> Unless/until the toolchain can properly GC anonymous strings, I think
> >>> finer-grained conditional compilation is the way to go.
> >> 
> >> i think we should use GC when it works (we've found 1 case so far where
> >> it doesn't), and add fine grained compilation when it doesn't (with a
> >> note with expected working versions).  i think this is a bug in the
> >> linker and have been trying to make a reduced test case that i can post
> >> to the GNU binutils peeps.
> > 
> > We've found two cases, at least.  Functions aren't being dropped out
> > either.
> 
> To be clear,
> https://github.com/trini/u-boot/commit/3a7c313125eff000ac73e38d220e5f9615c
> 1abbe shows switching everything to the normal nand codebase, but with a
> gain of only 170 bytes.  The reason it's such an otherwise small delta is
> that I added some #ifndef CONFIG_SPL_BUILD to drop out unused code that
> the linker should have been dropping for us, but wasn't.

how exactly do i reproduce the issue ?  what do i `make xxx` ?
-mike
Tom Rini Jan. 10, 2012, 6:36 p.m. UTC | #8 
On Tue, Jan 10, 2012 at 11:25 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 09 January 2012 16:25:41 Tom Rini wrote:
>> On Mon, Jan 9, 2012 at 2:23 PM, Tom Rini <tom.rini@gmail.com> wrote:
>> > On Mon, Jan 9, 2012 at 2:21 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> >> On Monday 09 January 2012 14:41:07 Scott Wood wrote:
>> >>> Unless/until the toolchain can properly GC anonymous strings, I think
>> >>> finer-grained conditional compilation is the way to go.
>> >>
>> >> i think we should use GC when it works (we've found 1 case so far where
>> >> it doesn't), and add fine grained compilation when it doesn't (with a
>> >> note with expected working versions).  i think this is a bug in the
>> >> linker and have been trying to make a reduced test case that i can post
>> >> to the GNU binutils peeps.
>> >
>> > We've found two cases, at least.  Functions aren't being dropped out
>> > either.
>>
>> To be clear,
>> https://github.com/trini/u-boot/commit/3a7c313125eff000ac73e38d220e5f9615c
>> 1abbe shows switching everything to the normal nand codebase, but with a
>> gain of only 170 bytes.  The reason it's such an otherwise small delta is
>> that I added some #ifndef CONFIG_SPL_BUILD to drop out unused code that
>> the linker should have been dropping for us, but wasn't.
>
> how exactly do i reproduce the issue ?  what do i `make xxx` ?

On that tree, build for devkit8000.
diff mbox

Patch

--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -22,6 +22,12 @@ 
 +	256	256 Byte page size
 *	512	512 Byte page size
 */
+
+static const char nand_16mib_18v_8bit[] = "NAND 16MiB 1,8V 8-bit";
+static const char nand_16mib_33v_8bit[] = "NAND 16MiB 3,3V 8-bit";
+static const char nand_16mib_18v_16bit[] = "NAND 16MiB 1,8V 16-bit";
+static const char nand_16mib_33v_16bit[] = "NAND 16MiB 3,3V 16-bit";
+
 const struct nand_flash_dev nand_flash_ids[] = {
 
 #ifdef CONFIG_MTD_NAND_MUSEUM_IDS
@@ -42,10 +48,10 @@ 
 	{"NAND 8MiB 3,3V 16-bit",	0x59, 512, 8, 0x2000, NAND_BUSWIDTH_16},
 #endif
 
-	{"NAND 16MiB 1,8V 8-bit",	0x33, 512, 16, 0x4000, 0},
-	{"NAND 16MiB 3,3V 8-bit",	0x73, 512, 16, 0x4000, 0},
-	{"NAND 16MiB 1,8V 16-bit",	0x43, 512, 16, 0x4000, NAND_BUSWIDTH_16},
-	{"NAND 16MiB 3,3V 16-bit",	0x53, 512, 16, 0x4000, NAND_BUSWIDTH_16},
+	{nand_16mib_18v_8bit,	0x33, 512, 16, 0x4000, 0},
+	{nand_16mib_33v_8bit,	0x73, 512, 16, 0x4000, 0},
+	{nand_16mib_18v_16bit,	0x43, 512, 16, 0x4000, NAND_BUSWIDTH_16},
+	{nand_16mib_33v_16bit,	0x53, 512, 16, 0x4000, NAND_BUSWIDTH_16},
 
 	{"NAND 32MiB 1,8V 8-bit",	0x35, 512, 32, 0x4000, 0},
 	{"NAND 32MiB 3,3V 8-bit",	0x75, 512, 32, 0x4000, 0},