Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
On Fri, 28 Jan 2011 14:07:09 -0500 Haiying Wang 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
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: textdata bss dec hex filename 3320 520 03840 f00 nand_spl/u-boot-spl After applying that patch: textdata bss dec hex filename 3476 520 03996 f9c nand_spl/u-boot-spl Once 8572 support is getting bigger as that in BSP, the error message will be triggered. Haiying ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
On Fri, 28 Jan 2011 13:46:06 -0500 Haiying Wang 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
On Fri, 28 Jan 2011 13:08:30 -0500 Haiying Wang wrote: > On Fri, 2011-01-28 at 11:36 -0600, Scott Wood wrote: > > On Thu, 27 Jan 2011 23:58:10 -0500 > > wrote: > > > > > From: Haiying Wang > > > > > > Signed-off-by: Haiying Wang > > > --- > > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
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, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
On Fri, 2011-01-28 at 11:36 -0600, Scott Wood wrote: > On Thu, 27 Jan 2011 23:58:10 -0500 > wrote: > > > From: Haiying Wang > > > > Signed-off-by: Haiying Wang > > --- > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 7/7] add gc-sections to TPL boot
On Thu, 27 Jan 2011 23:58:10 -0500 wrote: > From: Haiying Wang > > Signed-off-by: Haiying Wang > --- > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
