diff mbox

[U-Boot,7/7] add gc-sections to TPL boot

Message ID 1296190690-21146-5-git-send-email-Haiying.Wang@freescale.com
State Superseded
Headers show

Commit Message

Haiying Wang Jan. 28, 2011, 4:58 a.m. UTC 
From: Haiying Wang <Haiying.Wang@freescale.com>

Signed-off-by: Haiying Wang <Haiying.Wang@freescale.com>
---
 arch/powerpc/config.mk |    4 ++++
 config.mk              |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

Comments

Scott Wood Jan. 28, 2011, 5:36 p.m. UTC | #1 
On Thu, 27 Jan 2011 23:58:10 -0500
<Haiying.Wang@freescale.com> wrote:

> From: Haiying Wang <Haiying.Wang@freescale.com>
> 
> Signed-off-by: Haiying Wang <Haiying.Wang@freescale.com>
> ---
>  arch/powerpc/config.mk |    4 ++++
>  config.mk              |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)

I see patch 3/8, 4/8, 5/8, and 7/7.  Where are the rest?

> diff --git a/config.mk b/config.mk
> index 5147c35..d7bb07f 100644
> --- a/config.mk
> +++ b/config.mk
> @@ -260,8 +260,13 @@ $(obj)%.s:	%.c
>  #########################################################################
>  
>  # If the list of objects to link is empty, just create an empty built-in.o
> +ifdef CONFIG_HAS_TPL
> +cmd_link_o_target = $(if $(strip $1),\
> +		      $(LD) -r -o $@ $1,\
> +		      rm -f $@; $(AR) rcs $@ )
> +else
>  cmd_link_o_target = $(if $(strip $1),\
>  		      $(LD) $(LDFLAGS) -r -o $@ $1,\
>  		      rm -f $@; $(AR) rcs $@ )
> -
> +endif

What's going on here?

-Scott
Haiying Wang Jan. 28, 2011, 6:08 p.m. UTC | #2 
On Fri, 2011-01-28 at 11:36 -0600, Scott Wood wrote:
> On Thu, 27 Jan 2011 23:58:10 -0500
> <Haiying.Wang@freescale.com> wrote:
> 
> > From: Haiying Wang <Haiying.Wang@freescale.com>
> > 
> > Signed-off-by: Haiying Wang <Haiying.Wang@freescale.com>
> > ---
> >  arch/powerpc/config.mk |    4 ++++
> >  config.mk              |    7 ++++++-
> >  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> I see patch 3/8, 4/8, 5/8, and 7/7.  Where are the rest?
Sorry, patch 7/7 is a wrong number here. I kept the patch # as 3/8/,
4/8, 5/8, 8/8 to be consistent with the order in the patchset(v2) I
submitted in last December. I thought it would be clearer to compare
them with v2 version and review.  Patch 1/8,2/8 have been applied by
Kumar, patch 6/8, 7/8 remain the same as v2 version. This patch, is a
new patch because that TPL still needs --gc-sections in linker option to
do partial link.

If it is preferable to have new whole set of patch, I can reorder them
from 3/8-8/8 plus this one to submit.

> > diff --git a/config.mk b/config.mk
> > index 5147c35..d7bb07f 100644
> > --- a/config.mk
> > +++ b/config.mk
> > @@ -260,8 +260,13 @@ $(obj)%.s:	%.c
> >  #########################################################################
> >  
> >  # If the list of objects to link is empty, just create an empty built-in.o
> > +ifdef CONFIG_HAS_TPL
> > +cmd_link_o_target = $(if $(strip $1),\
> > +		      $(LD) -r -o $@ $1,\
> > +		      rm -f $@; $(AR) rcs $@ )
> > +else
> >  cmd_link_o_target = $(if $(strip $1),\
> >  		      $(LD) $(LDFLAGS) -r -o $@ $1,\
> >  		      rm -f $@; $(AR) rcs $@ )
> > -
> > +endif
> 
> What's going on here?
> 
For CONFIG_HAS_TPL, LDFLAGS has --gc-sections now, passing it to
cmd_link_o_target here will fail in linking stage:
"
powerpc-none-linux-gnuspe-ld: gc-sections requires either an entry or an
undefined symbol
"

Haiying
Albert ARIBAUD Jan. 28, 2011, 6:21 p.m. UTC | #3 
Le 28/01/2011 19:08, Haiying Wang a écrit :

>> I see patch 3/8, 4/8, 5/8, and 7/7.  Where are the rest?
> Sorry, patch 7/7 is a wrong number here. I kept the patch # as 3/8/,
> 4/8, 5/8, 8/8 to be consistent with the order in the patchset(v2) I
> submitted in last December. I thought it would be clearer to compare
> them with v2 version and review.  Patch 1/8,2/8 have been applied by
> Kumar, patch 6/8, 7/8 remain the same as v2 version. This patch, is a
> new patch because that TPL still needs --gc-sections in linker option to
> do partial link.
>
> If it is preferable to have new whole set of patch, I can reorder them
> from 3/8-8/8 plus this one to submit.

I would suggest to simply number patches from 1 to N for each version 
even if that means the same patch gets numbered differently across 
versions, because readers of a given version may not have read the 
previous one. A patchset should be self-sufficient and self-consistent IMO.

Amicalement,
Scott Wood Jan. 28, 2011, 6:30 p.m. UTC | #4 
On Fri, 28 Jan 2011 13:08:30 -0500
Haiying Wang <Haiying.Wang@freescale.com> wrote:

> On Fri, 2011-01-28 at 11:36 -0600, Scott Wood wrote:
> > On Thu, 27 Jan 2011 23:58:10 -0500
> > <Haiying.Wang@freescale.com> wrote:
> > 
> > > From: Haiying Wang <Haiying.Wang@freescale.com>
> > > 
> > > Signed-off-by: Haiying Wang <Haiying.Wang@freescale.com>
> > > ---
> > >  arch/powerpc/config.mk |    4 ++++
> > >  config.mk              |    7 ++++++-
> > >  2 files changed, 10 insertions(+), 1 deletions(-)
> > 
> > I see patch 3/8, 4/8, 5/8, and 7/7.  Where are the rest?
> Sorry, patch 7/7 is a wrong number here. I kept the patch # as 3/8/,
> 4/8, 5/8, 8/8 to be consistent with the order in the patchset(v2) I
> submitted in last December. I thought it would be clearer to compare
> them with v2 version and review.  Patch 1/8,2/8 have been applied by
> Kumar, patch 6/8, 7/8 remain the same as v2 version. This patch, is a
> new patch because that TPL still needs --gc-sections in linker option to
> do partial link.
> 
> If it is preferable to have new whole set of patch, I can reorder them
> from 3/8-8/8 plus this one to submit.

Just produce a new complete patchset of what still needs to be
applied.  Don't preserve the numbering.

> > > diff --git a/config.mk b/config.mk
> > > index 5147c35..d7bb07f 100644
> > > --- a/config.mk
> > > +++ b/config.mk
> > > @@ -260,8 +260,13 @@ $(obj)%.s:	%.c
> > >  #########################################################################
> > >  
> > >  # If the list of objects to link is empty, just create an empty built-in.o
> > > +ifdef CONFIG_HAS_TPL
> > > +cmd_link_o_target = $(if $(strip $1),\
> > > +		      $(LD) -r -o $@ $1,\
> > > +		      rm -f $@; $(AR) rcs $@ )
> > > +else
> > >  cmd_link_o_target = $(if $(strip $1),\
> > >  		      $(LD) $(LDFLAGS) -r -o $@ $1,\
> > >  		      rm -f $@; $(AR) rcs $@ )
> > > -
> > > +endif
> > 
> > What's going on here?
> > 
> For CONFIG_HAS_TPL, LDFLAGS has --gc-sections now, passing it to
> cmd_link_o_target here will fail in linking stage:
> "
> powerpc-none-linux-gnuspe-ld: gc-sections requires either an entry or an
> undefined symbol
> "

I think --gc-sections should go in LDFLAGS_u-boot instead.

In any case, I don't think we want different behavior here based on
whether we have TPL.  Either LDFLAGS is used in partial linking, or
it's not.

-Scott
Haiying Wang Jan. 28, 2011, 6:46 p.m. UTC | #5 
On Fri, 2011-01-28 at 12:30 -0600, Scott Wood wrote:
> > > > diff --git a/config.mk b/config.mk
> > > > index 5147c35..d7bb07f 100644
> > > > --- a/config.mk
> > > > +++ b/config.mk
> > > > @@ -260,8 +260,13 @@ $(obj)%.s:	%.c
> > > >  #########################################################################
> > > >  
> > > >  # If the list of objects to link is empty, just create an empty built-in.o
> > > > +ifdef CONFIG_HAS_TPL
> > > > +cmd_link_o_target = $(if $(strip $1),\
> > > > +		      $(LD) -r -o $@ $1,\
> > > > +		      rm -f $@; $(AR) rcs $@ )
> > > > +else
> > > >  cmd_link_o_target = $(if $(strip $1),\
> > > >  		      $(LD) $(LDFLAGS) -r -o $@ $1,\
> > > >  		      rm -f $@; $(AR) rcs $@ )
> > > > -
> > > > +endif
> > > 
> > > What's going on here?
> > > 
> > For CONFIG_HAS_TPL, LDFLAGS has --gc-sections now, passing it to
> > cmd_link_o_target here will fail in linking stage:
> > "
> > powerpc-none-linux-gnuspe-ld: gc-sections requires either an entry or an
> > undefined symbol
> > "
> 
> I think --gc-sections should go in LDFLAGS_u-boot instead.
LDFLAGS_u-boot has --gc-sections already, I did not change it. I only add --gc-sections to PLATFORM_LDFLAGS in arch/powerpc/config.mk under "ifdef CONFIG_HAS_TPL"

> In any case, I don't think we want different behavior here based on
> whether we have TPL.  Either LDFLAGS is used in partial linking, or
> it's not.
I don't understand why LDFLAGS was added here in patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html

It says "LDFLAGS sets necessary option by partial linking (use in
cmd_link_o_target)." But without this changing, the partial linking
worked well before. Please correct me if I am wrong.

So if someone can confirm LDFLAGS is not necessary to be added in
cmd_link_o_target, I prefer not add it here.

Thanks.

Haiying
Scott Wood Jan. 28, 2011, 6:58 p.m. UTC | #6 
On Fri, 28 Jan 2011 13:46:06 -0500
Haiying Wang <Haiying.Wang@freescale.com> wrote:

> On Fri, 2011-01-28 at 12:30 -0600, Scott Wood wrote:
> > I think --gc-sections should go in LDFLAGS_u-boot instead.
> LDFLAGS_u-boot has --gc-sections already, I did not change it.

It looks like LDFLAGS_u-boot may not be suitable for building SPL/TPL
images.  Since TPL is new, and we don't have to worry about breaking
any existing boards, just unconditionally use --gc-sections when
linking the final TPL image.  Or, if we want a way for
boards/cpus to add ld options that things like TPL use, introduce
LDFLAGS_FINAL that holds ld parameters used for final link of any
image, with LDFLAGS_u-boot holding things like text addresses and linker
scripts with values that only apply to the main image.

I'd prefer the latter approach, as we could make use of it in SPL as
well, which does have existing boards to worry about.

> > In any case, I don't think we want different behavior here based on
> > whether we have TPL.  Either LDFLAGS is used in partial linking, or
> > it's not.
> I don't understand why LDFLAGS was added here in patch
> http://lists.denx.de/pipermail/u-boot/2011-January/084705.html
> 
> It says "LDFLAGS sets necessary option by partial linking (use in
> cmd_link_o_target)." But without this changing, the partial linking
> worked well before. Please correct me if I am wrong.
> 
> So if someone can confirm LDFLAGS is not necessary to be added in
> cmd_link_o_target, I prefer not add it here.

Whether leaving out -n during partial link worked for you or not,
LDFLAGS is supposed to be used by partial links (that distinction is
why LDFLAGS_u-boot was created).  So don't put things in LDFPLAGS that
break partial links.

-Scott
Haiying Wang Jan. 28, 2011, 7:07 p.m. UTC | #7 
On Fri, 2011-01-28 at 13:46 -0500, Haiying Wang wrote:
> > In any case, I don't think we want different behavior here based on
> > whether we have TPL.  Either LDFLAGS is used in partial linking, or
> > it's not.
> I don't understand why LDFLAGS was added here in patch
> http://lists.denx.de/pipermail/u-boot/2011-January/084705.html
> 
> It says "LDFLAGS sets necessary option by partial linking (use in
> cmd_link_o_target)." But without this changing, the partial linking
> worked well before. Please correct me if I am wrong.
> 
> So if someone can confirm LDFLAGS is not necessary to be added in
> cmd_link_o_target, I prefer not add it here.

BTW, I doubt removing --gc-sections for PLATFORM_FLAGS by patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html may have
the risk of building failure for nand_spl, as we encountered the message
"NAND bootstrap too big" before

For example, the size for MPC8572DS_NAND_config before applying patch:

   text	   data	    bss	    dec	    hex	filename
   3320	    520	      0	   3840	    f00	nand_spl/u-boot-spl

After applying that patch:
   text	   data	    bss	    dec	    hex	filename
   3476	    520	      0	   3996	    f9c	nand_spl/u-boot-spl

Once 8572 support is getting bigger as that in BSP, the error message
will be triggered.

Haiying
Scott Wood Jan. 28, 2011, 7:12 p.m. UTC | #8 
On Fri, 28 Jan 2011 14:07:09 -0500
Haiying Wang <Haiying.Wang@freescale.com> wrote:

> On Fri, 2011-01-28 at 13:46 -0500, Haiying Wang wrote:
> > > In any case, I don't think we want different behavior here based on
> > > whether we have TPL.  Either LDFLAGS is used in partial linking, or
> > > it's not.
> > I don't understand why LDFLAGS was added here in patch
> > http://lists.denx.de/pipermail/u-boot/2011-January/084705.html
> > 
> > It says "LDFLAGS sets necessary option by partial linking (use in
> > cmd_link_o_target)." But without this changing, the partial linking
> > worked well before. Please correct me if I am wrong.
> > 
> > So if someone can confirm LDFLAGS is not necessary to be added in
> > cmd_link_o_target, I prefer not add it here.
> 
> BTW, I doubt removing --gc-sections for PLATFORM_FLAGS by patch
> http://lists.denx.de/pipermail/u-boot/2011-January/084705.html may have
> the risk of building failure for nand_spl, as we encountered the message
> "NAND bootstrap too big" before

Yes, I saw that as well -- we need gc-sections.  It just can't go in
LDFLAGS.

-Scott
diff mbox

Patch

diff --git a/arch/powerpc/config.mk b/arch/powerpc/config.mk
index 64191c7..78e53c4 100644
--- a/arch/powerpc/config.mk
+++ b/arch/powerpc/config.mk
@@ -27,7 +27,11 @@  STANDALONE_LOAD_ADDR = 0x40000
 LDFLAGS_u-boot = --gc-sections
 PLATFORM_RELFLAGS += -mrelocatable -ffunction-sections -fdata-sections
 PLATFORM_CPPFLAGS += -DCONFIG_PPC -D__powerpc__
+ifdef CONFIG_HAS_TPL
+PLATFORM_LDFLAGS  += -n --gc-sections
+else
 PLATFORM_LDFLAGS  += -n
+endif
 
 ifdef CONFIG_SYS_LDSCRIPT
 # need to strip off double quotes
diff --git a/config.mk b/config.mk
index 5147c35..d7bb07f 100644
--- a/config.mk
+++ b/config.mk
@@ -260,8 +260,13 @@  $(obj)%.s:	%.c
 #########################################################################
 
 # If the list of objects to link is empty, just create an empty built-in.o
+ifdef CONFIG_HAS_TPL
+cmd_link_o_target = $(if $(strip $1),\
+		      $(LD) -r -o $@ $1,\
+		      rm -f $@; $(AR) rcs $@ )
+else
 cmd_link_o_target = $(if $(strip $1),\
 		      $(LD) $(LDFLAGS) -r -o $@ $1,\
 		      rm -f $@; $(AR) rcs $@ )
-
+endif
 #########################################################################