| Message ID | 1350424209-11186-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Stephen, On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > When -ffunction-sections or -fdata-section are used, symbols are placed > into sections such as .data.eserial1_device and .bss.serial_current. > Update the linker script to explicitly include these. Without this > change (at least with my gcc-4.5.3 built using crosstool-ng), I see that > the sections do end up being included, but __bss_end__ gets set to the > same value as __bss_start. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > This series fixes an SPL size overflow problem on Tegra. Tom Warren is > out on vacation until Oct 25th, so he certainly won't be able to review > this. Perhaps it could be applied directly to the ARM tree if enough > Tegra people ack the series? > > Note that this series is not enough to make Tegra support work; either > you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call > serial_initialize() right before serial_init() in preloader_console_init() > or wait for Allen Martin to rework Tegra's SPL support using the common > SPL code. Are you going to submit a patch to enable function-sections, or is that a separate discussion? > > arch/arm/cpu/u-boot.lds | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index e49ca0c..ae04a6e 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -35,7 +35,7 @@ SECTIONS > { > __image_copy_start = .; > CPUDIR/start.o (.text) > - *(.text) > + *(.text*) > } > > . = ALIGN(4); > @@ -43,14 +43,14 @@ SECTIONS > > . = ALIGN(4); > .data : { > - *(.data) > + *(.data*) > } > > . = ALIGN(4); > > . = .; > __u_boot_cmd_start = .; > - .u_boot_cmd : { *(.u_boot_cmd) } > + .u_boot_cmd : { *(.u_boot_cmd*) } I don't think this line is needed? > __u_boot_cmd_end = .; > > . = ALIGN(4); > @@ -65,7 +65,7 @@ SECTIONS > > .dynsym : { > __dynsym_start = .; > - *(.dynsym) > + *(.dynsym*) Nor this one? > } > > _end = .; > @@ -81,7 +81,7 @@ SECTIONS > > .bss __rel_dyn_start (OVERLAY) : { > __bss_start = .; > - *(.bss) > + *(.bss*) > . = ALIGN(4); > __bss_end__ = .; > } > -- > 1.7.0.4 > Regards, Simon
On 10/17/2012 05:58 PM, Simon Glass wrote: > Hi Stephen, > > On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> When -ffunction-sections or -fdata-section are used, symbols are placed >> into sections such as .data.eserial1_device and .bss.serial_current. >> Update the linker script to explicitly include these. Without this >> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that >> the sections do end up being included, but __bss_end__ gets set to the >> same value as __bss_start. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> This series fixes an SPL size overflow problem on Tegra. Tom Warren is >> out on vacation until Oct 25th, so he certainly won't be able to review >> this. Perhaps it could be applied directly to the ARM tree if enough >> Tegra people ack the series? >> >> Note that this series is not enough to make Tegra support work; either >> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call >> serial_initialize() right before serial_init() in preloader_console_init() >> or wait for Allen Martin to rework Tegra's SPL support using the common >> SPL code. > > Are you going to submit a patch to enable function-sections, or is > that a separate discussion? For the SPL on Tegra, those flags were already on; this patch fixes a bug rather than prepares for new functionality. >> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds >> - .u_boot_cmd : { *(.u_boot_cmd) } >> + .u_boot_cmd : { *(.u_boot_cmd*) } > > I don't think this line is needed? > ... >> - *(.dynsym) >> + *(.dynsym*) > > Nor this one? Possibly. I changed all the section names to be future-proof. Perhaps a more targeted patch is warranted.
Hi Stephen, On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/17/2012 05:58 PM, Simon Glass wrote: > > Hi Stephen, > > > > On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> From: Stephen Warren <swarren@nvidia.com> > >> > >> When -ffunction-sections or -fdata-section are used, symbols are placed > >> into sections such as .data.eserial1_device and .bss.serial_current. > >> Update the linker script to explicitly include these. Without this > >> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that > >> the sections do end up being included, but __bss_end__ gets set to the > >> same value as __bss_start. > >> > >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >> --- > >> This series fixes an SPL size overflow problem on Tegra. Tom Warren is > >> out on vacation until Oct 25th, so he certainly won't be able to review > >> this. Perhaps it could be applied directly to the ARM tree if enough > >> Tegra people ack the series? > >> > >> Note that this series is not enough to make Tegra support work; either > >> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call > >> serial_initialize() right before serial_init() in preloader_console_init() > >> or wait for Allen Martin to rework Tegra's SPL support using the common > >> SPL code. > > > > Are you going to submit a patch to enable function-sections, or is > > that a separate discussion? > > For the SPL on Tegra, those flags were already on; this patch fixes a > bug rather than prepares for new functionality. > > >> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > > >> - .u_boot_cmd : { *(.u_boot_cmd) } > >> + .u_boot_cmd : { *(.u_boot_cmd*) } > > > > I don't think this line is needed? > > > ... > >> - *(.dynsym) > >> + *(.dynsym*) > > > > Nor this one? > > Possibly. I changed all the section names to be future-proof. Perhaps a > more targeted patch is warranted. Has this been (at least build-)tested on all boards which have -ffunction-sections or -fdata-sections? Amicalement,
On 10/18/2012 02:36 PM, Albert ARIBAUD wrote: > Hi Stephen, > > On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren > <swarren@wwwdotorg.org> wrote: > >> On 10/17/2012 05:58 PM, Simon Glass wrote: >>> Hi Stephen, >>> >>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> When -ffunction-sections or -fdata-section are used, symbols are placed >>>> into sections such as .data.eserial1_device and .bss.serial_current. >>>> Update the linker script to explicitly include these. Without this >>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that >>>> the sections do end up being included, but __bss_end__ gets set to the >>>> same value as __bss_start. >>>> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>> --- >>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is >>>> out on vacation until Oct 25th, so he certainly won't be able to review >>>> this. Perhaps it could be applied directly to the ARM tree if enough >>>> Tegra people ack the series? >>>> >>>> Note that this series is not enough to make Tegra support work; either >>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call >>>> serial_initialize() right before serial_init() in preloader_console_init() >>>> or wait for Allen Martin to rework Tegra's SPL support using the common >>>> SPL code. >>> >>> Are you going to submit a patch to enable function-sections, or is >>> that a separate discussion? >> >> For the SPL on Tegra, those flags were already on; this patch fixes a >> bug rather than prepares for new functionality. >> >>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds >> >>>> - .u_boot_cmd : { *(.u_boot_cmd) } >>>> + .u_boot_cmd : { *(.u_boot_cmd*) } >>> >>> I don't think this line is needed? >>> >> ... >>>> - *(.dynsym) >>>> + *(.dynsym*) >>> >>> Nor this one? >> >> Possibly. I changed all the section names to be future-proof. Perhaps a >> more targeted patch is warranted. > > Has this been (at least build-)tested on all boards which have > -ffunction-sections or -fdata-sections? Yes; those flags both appear to be turned on when building the SPL for Tegra (although strangely, not when building the main U-Boot): xxx-gcc -M -g -Os -fno-common -ffixed-r8 -msoft-float -D__KERNEL__ -ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000 -DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD -I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding -nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm -mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c > xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts
On 10/18/2012 02:58 PM, Stephen Warren wrote: > On 10/18/2012 02:36 PM, Albert ARIBAUD wrote: >> Hi Stephen, >> >> On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren >> <swarren@wwwdotorg.org> wrote: >> >>> On 10/17/2012 05:58 PM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> From: Stephen Warren <swarren@nvidia.com> >>>>> >>>>> When -ffunction-sections or -fdata-section are used, symbols are placed >>>>> into sections such as .data.eserial1_device and .bss.serial_current. >>>>> Update the linker script to explicitly include these. Without this >>>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that >>>>> the sections do end up being included, but __bss_end__ gets set to the >>>>> same value as __bss_start. >>>>> >>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>> --- >>>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is >>>>> out on vacation until Oct 25th, so he certainly won't be able to review >>>>> this. Perhaps it could be applied directly to the ARM tree if enough >>>>> Tegra people ack the series? >>>>> >>>>> Note that this series is not enough to make Tegra support work; either >>>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call >>>>> serial_initialize() right before serial_init() in preloader_console_init() >>>>> or wait for Allen Martin to rework Tegra's SPL support using the common >>>>> SPL code. >>>> >>>> Are you going to submit a patch to enable function-sections, or is >>>> that a separate discussion? >>> >>> For the SPL on Tegra, those flags were already on; this patch fixes a >>> bug rather than prepares for new functionality. >>> >>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds >>> >>>>> - .u_boot_cmd : { *(.u_boot_cmd) } >>>>> + .u_boot_cmd : { *(.u_boot_cmd*) } >>>> >>>> I don't think this line is needed? >>>> >>> ... >>>>> - *(.dynsym) >>>>> + *(.dynsym*) >>>> >>>> Nor this one? >>> >>> Possibly. I changed all the section names to be future-proof. Perhaps a >>> more targeted patch is warranted. >> >> Has this been (at least build-)tested on all boards which have >> -ffunction-sections or -fdata-sections? > > Yes; those flags both appear to be turned on when building the SPL for > Tegra (although strangely, not when building the main U-Boot): > > xxx-gcc -M -g -Os -fno-common -ffixed-r8 -msoft-float -D__KERNEL__ > -ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000 > -DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD > -I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding > -nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm > -mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ > xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c > > xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts I tried to test this more widely. I found that only arch/arm/cpu/ixp/config.mk enables this on ARM, with the comment: > # -fdata-sections triggers "section .bss overlaps section .rel.dyn" linker error > PLATFORM_RELFLAGS += -ffunction-sections > LDFLAGS_u-boot += --gc-sections Indeed, if I edit arch/arm/cpu/armv7/config.mk to enable both -ffunction-sections -fdata-sections, I do see that same section overlap error building for Tegra. Then, if I apply the .lds patch we're discussing, Tegra builds (and runs) OK with those linker flags enabled. So, this patch seems to fix two issues not just one - bonus:-)
Hi Stephen, On Thu, 18 Oct 2012 15:17:52 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/18/2012 02:58 PM, Stephen Warren wrote: > > On 10/18/2012 02:36 PM, Albert ARIBAUD wrote: > >> Hi Stephen, > >> > >> On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren > >> <swarren@wwwdotorg.org> wrote: > >> > >>> On 10/17/2012 05:58 PM, Simon Glass wrote: > >>>> Hi Stephen, > >>>> > >>>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >>>>> From: Stephen Warren <swarren@nvidia.com> > >>>>> > >>>>> When -ffunction-sections or -fdata-section are used, symbols are placed > >>>>> into sections such as .data.eserial1_device and .bss.serial_current. > >>>>> Update the linker script to explicitly include these. Without this > >>>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that > >>>>> the sections do end up being included, but __bss_end__ gets set to the > >>>>> same value as __bss_start. > >>>>> > >>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >>>>> --- > >>>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is > >>>>> out on vacation until Oct 25th, so he certainly won't be able to review > >>>>> this. Perhaps it could be applied directly to the ARM tree if enough > >>>>> Tegra people ack the series? > >>>>> > >>>>> Note that this series is not enough to make Tegra support work; either > >>>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call > >>>>> serial_initialize() right before serial_init() in preloader_console_init() > >>>>> or wait for Allen Martin to rework Tegra's SPL support using the common > >>>>> SPL code. > >>>> > >>>> Are you going to submit a patch to enable function-sections, or is > >>>> that a separate discussion? > >>> > >>> For the SPL on Tegra, those flags were already on; this patch fixes a > >>> bug rather than prepares for new functionality. > >>> > >>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > >>> > >>>>> - .u_boot_cmd : { *(.u_boot_cmd) } > >>>>> + .u_boot_cmd : { *(.u_boot_cmd*) } > >>>> > >>>> I don't think this line is needed? > >>>> > >>> ... > >>>>> - *(.dynsym) > >>>>> + *(.dynsym*) > >>>> > >>>> Nor this one? > >>> > >>> Possibly. I changed all the section names to be future-proof. Perhaps a > >>> more targeted patch is warranted. > >> > >> Has this been (at least build-)tested on all boards which have > >> -ffunction-sections or -fdata-sections? > > > > Yes; those flags both appear to be turned on when building the SPL for > > Tegra (although strangely, not when building the main U-Boot): > > > > xxx-gcc -M -g -Os -fno-common -ffixed-r8 -msoft-float -D__KERNEL__ > > -ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000 > > -DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD > > -I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding > > -nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm > > -mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ > > xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c > > > xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts > > I tried to test this more widely. I found that only > arch/arm/cpu/ixp/config.mk enables this on ARM, with the comment: > > > # -fdata-sections triggers "section .bss overlaps section .rel.dyn" linker error > > PLATFORM_RELFLAGS += -ffunction-sections > > LDFLAGS_u-boot += --gc-sections > > Indeed, if I edit arch/arm/cpu/armv7/config.mk to enable both > -ffunction-sections -fdata-sections, I do see that same section overlap > error building for Tegra. > > Then, if I apply the .lds patch we're discussing, Tegra builds (and > runs) OK with those linker flags enabled. So, this patch seems to fix > two issues not just one - bonus:-) Unless someone opposes, I will pick this one in u-boot-arm/master later today, as I will be doing some .lds merging on ARM (and then some start.S merging). Once this is done, the Tegra tree (Tom already Cc:) should merge u-boot-arm/master to get this first commit in, then apply commits 2 to 5. Amicalement,
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index e49ca0c..ae04a6e 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -35,7 +35,7 @@ SECTIONS { __image_copy_start = .; CPUDIR/start.o (.text) - *(.text) + *(.text*) } . = ALIGN(4); @@ -43,14 +43,14 @@ SECTIONS . = ALIGN(4); .data : { - *(.data) + *(.data*) } . = ALIGN(4); . = .; __u_boot_cmd_start = .; - .u_boot_cmd : { *(.u_boot_cmd) } + .u_boot_cmd : { *(.u_boot_cmd*) } __u_boot_cmd_end = .; . = ALIGN(4); @@ -65,7 +65,7 @@ SECTIONS .dynsym : { __dynsym_start = .; - *(.dynsym) + *(.dynsym*) } _end = .; @@ -81,7 +81,7 @@ SECTIONS .bss __rel_dyn_start (OVERLAY) : { __bss_start = .; - *(.bss) + *(.bss*) . = ALIGN(4); __bss_end__ = .; }