| Message ID | 1482762041-8416-1-git-send-email-oded.gabbay@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Mashiro, Could you please also take a look at the patch I sent to add the missing __bss_size ? Thanks, Oded On Mon, Dec 26, 2016 at 4:20 PM, Oded Gabbay <oded.gabbay@gmail.com> wrote: > This patch adds a missing __bss_size symbol to the default armv8 > u-boot-spl.lds file. > Makefile.spl relies on __bss_size to be present when it creates the SPL > image. It uses that symbol to create a pad file that will be used to place > the dtb after the bss section. > In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the > pad file was always 0. As a result, the dtb was placed after > _image_binary_end, which caused a failure when loading it inside the SPL. > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > --- > arch/arm/cpu/armv8/u-boot-spl.lds | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > index cc427c3..e7799cc 100644 > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -70,6 +70,7 @@ SECTIONS > KEEP(*(.__bss_end)); > } >.sdram > > + __bss_size = __bss_end - _image_binary_end; > /DISCARD/ : { *(.dynsym) } > /DISCARD/ : { *(.dynstr*) } > /DISCARD/ : { *(.dynamic*) } > -- > 2.7.4 >
Hi. (CCing Simon because he wrote the code addressed in this discussion.) 2017-01-17 18:27 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: > Hi Mashiro, > Could you please also take a look at the patch I sent to add the > missing __bss_size ? > > Thanks, > Oded > > On Mon, Dec 26, 2016 at 4:20 PM, Oded Gabbay <oded.gabbay@gmail.com> wrote: >> This patch adds a missing __bss_size symbol to the default armv8 >> u-boot-spl.lds file. >> Makefile.spl relies on __bss_size to be present when it creates the SPL >> image. It uses that symbol to create a pad file that will be used to place >> the dtb after the bss section. >> In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the >> pad file was always 0. As a result, the dtb was placed after >> _image_binary_end, which caused a failure when loading it inside the SPL. >> >> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >> Cc: Albert Aribaud <albert.u.boot@aribaud.net> >> --- >> arch/arm/cpu/armv8/u-boot-spl.lds | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds >> index cc427c3..e7799cc 100644 >> --- a/arch/arm/cpu/armv8/u-boot-spl.lds >> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds >> @@ -70,6 +70,7 @@ SECTIONS >> KEEP(*(.__bss_end)); >> } >.sdram >> >> + __bss_size = __bss_end - _image_binary_end; >> /DISCARD/ : { *(.dynsym) } >> /DISCARD/ : { *(.dynstr*) } >> /DISCARD/ : { *(.dynamic*) } This patch seems wrong. This linker script seems to support separate two memory regions. MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE, LENGTH = CONFIG_SPL_MAX_SIZE } MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, LENGTH = CONFIG_SPL_BSS_MAX_SIZE } _image_binary_end is the end of the first region (.sram), __bss_end is the end of the second region (.sdram). So, with your patch, __bss_size = (gap between .sram and .sdram) + (size of .bss) This will append really big padding there is a gap between the two regions. (at least, it will break my boards.) If you take a look at fdtdec_setup(), # ifdef CONFIG_SPL_BUILD /* FDT is at end of BSS unless it is in a different memory region */ if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) gd->fdt_blob = (ulong *)&_image_binary_end; else gd->fdt_blob = (ulong *)&__bss_end; I think you are supposed to enable CONFIG_SPL_SEPARATE_BSS if you want to use SPL_OF_CONTROL on your arm64 boards. Checking the following commit will help you. commit 10172962479ddd6609101fdb83bde66c0719852c Author: Simon Glass <sjg@chromium.org> Date: Sat Oct 17 19:41:19 2015 -0600 dm: spl: Support device tree when BSS is in a different section At present in SPL we place the device tree immediately after BSS. This avoids needing to copy it out of the way before BSS can be used. However on some boards BSS is not placed with the image - e.g. it can be in RAM if available. Add an option to tell U-Boot that the device tree should be placed at the end of the image binary (_image_binary_end) instead of at the end of BSS. Note: A common reason to place BSS in RAM is to support the FAT filesystem. We should update the code so that it does not use so much BSS. Signed-off-by: Simon Glass <sjg@chromium.org> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Hi, On 19 January 2017 at 04:28, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi. > > (CCing Simon because he wrote the code addressed in this discussion.) > > > 2017-01-17 18:27 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: >> Hi Mashiro, >> Could you please also take a look at the patch I sent to add the >> missing __bss_size ? >> >> Thanks, >> Oded >> >> On Mon, Dec 26, 2016 at 4:20 PM, Oded Gabbay <oded.gabbay@gmail.com> wrote: >>> This patch adds a missing __bss_size symbol to the default armv8 >>> u-boot-spl.lds file. >>> Makefile.spl relies on __bss_size to be present when it creates the SPL >>> image. It uses that symbol to create a pad file that will be used to place >>> the dtb after the bss section. >>> In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the >>> pad file was always 0. As a result, the dtb was placed after >>> _image_binary_end, which caused a failure when loading it inside the SPL. >>> >>> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >>> Cc: Albert Aribaud <albert.u.boot@aribaud.net> >>> --- >>> arch/arm/cpu/armv8/u-boot-spl.lds | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds >>> index cc427c3..e7799cc 100644 >>> --- a/arch/arm/cpu/armv8/u-boot-spl.lds >>> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds >>> @@ -70,6 +70,7 @@ SECTIONS >>> KEEP(*(.__bss_end)); >>> } >.sdram >>> >>> + __bss_size = __bss_end - _image_binary_end; >>> /DISCARD/ : { *(.dynsym) } >>> /DISCARD/ : { *(.dynstr*) } >>> /DISCARD/ : { *(.dynamic*) } > > > > This patch seems wrong. > > > This linker script seems to support separate two memory regions. > > MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE, > LENGTH = CONFIG_SPL_MAX_SIZE } > MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, > LENGTH = CONFIG_SPL_BSS_MAX_SIZE } > > > _image_binary_end is the end of the first region (.sram), > __bss_end is the end of the second region (.sdram). > > So, with your patch, > > __bss_size = (gap between .sram and .sdram) + (size of .bss) > > This will append really big padding there is a gap between the two regions. > (at least, it will break my boards.) > > > If you take a look at fdtdec_setup(), > > # ifdef CONFIG_SPL_BUILD > /* FDT is at end of BSS unless it is in a different memory region */ > if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) > gd->fdt_blob = (ulong *)&_image_binary_end; > else > gd->fdt_blob = (ulong *)&__bss_end; > > I think you are supposed to enable CONFIG_SPL_SEPARATE_BSS > if you want to use SPL_OF_CONTROL on your arm64 boards. > > > > Checking the following commit will help you. > > > commit 10172962479ddd6609101fdb83bde66c0719852c > Author: Simon Glass <sjg@chromium.org> > Date: Sat Oct 17 19:41:19 2015 -0600 > > dm: spl: Support device tree when BSS is in a different section > > At present in SPL we place the device tree immediately after BSS. This > avoids needing to copy it out of the way before BSS can be used. However on > some boards BSS is not placed with the image - e.g. it can be in RAM if > available. > > Add an option to tell U-Boot that the device tree should be placed at the > end of the image binary (_image_binary_end) instead of at the end of BSS. > > Note: A common reason to place BSS in RAM is to support the FAT filesystem. > We should update the code so that it does not use so much BSS. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> Yes that's right. Thank you Masahiro. Also, consider moving to use binman to generate the image. Regards, Simon
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index cc427c3..e7799cc 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -70,6 +70,7 @@ SECTIONS KEEP(*(.__bss_end)); } >.sdram + __bss_size = __bss_end - _image_binary_end; /DISCARD/ : { *(.dynsym) } /DISCARD/ : { *(.dynstr*) } /DISCARD/ : { *(.dynamic*) }
This patch adds a missing __bss_size symbol to the default armv8 u-boot-spl.lds file. Makefile.spl relies on __bss_size to be present when it creates the SPL image. It uses that symbol to create a pad file that will be used to place the dtb after the bss section. In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the pad file was always 0. As a result, the dtb was placed after _image_binary_end, which caused a failure when loading it inside the SPL. Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> Cc: Albert Aribaud <albert.u.boot@aribaud.net> --- arch/arm/cpu/armv8/u-boot-spl.lds | 1 + 1 file changed, 1 insertion(+)