Patchwork [U-Boot,1/4] cam_enc_4xx: fix CONFIG_SPL_MAX_SIZE semantics

login
register
mail settings
Submitter Albert ARIBAUD
Date April 8, 2013, 7:58 p.m.
Message ID <1365451109-22030-2-git-send-email-albert.u.boot@aribaud.net>
Download mbox | patch
Permalink /patch/234876/
State Superseded
Headers show

Comments

Albert ARIBAUD - April 8, 2013, 7:58 p.m.
CONFIG_SPL_MAX_SIZE wrongly included BSS size. Split
max size between image and BSS based on sizes reported
for current build.

Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 board/ait/cam_enc_4xx/u-boot-spl.lds |    2 +-
 include/configs/cam_enc_4xx.h        |    4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)
Tom Rini - April 8, 2013, 8:43 p.m.
On Mon, Apr 08, 2013 at 09:58:26AM -0000, Albert ARIBAUD wrote:

> CONFIG_SPL_MAX_SIZE wrongly included BSS size. Split
> max size between image and BSS based on sizes reported
> for current build.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> 
> ---
> board/ait/cam_enc_4xx/u-boot-spl.lds |    2 +-
>  include/configs/cam_enc_4xx.h        |    4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds b/board/ait/cam_enc_4xx/u-boot-spl.lds
> index dd9d52d..25625dc 100644
> --- a/board/ait/cam_enc_4xx/u-boot-spl.lds
> +++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
> @@ -25,7 +25,7 @@
>   */
>  
>  MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
> -		LENGTH = CONFIG_SPL_MAX_SIZE }
> +		LENGTH = (CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE) }
>  
>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>  OUTPUT_ARCH(arm)
> diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h
> index 56528dd..df3682b 100644
> --- a/include/configs/cam_enc_4xx.h
> +++ b/include/configs/cam_enc_4xx.h
> @@ -230,7 +230,9 @@
>  #define CONFIG_SPL_STACK		(0x00010000 + 0x7f00)
>  
>  #define CONFIG_SPL_TEXT_BASE		0x00000020 /*CONFIG_SYS_SRAM_START*/
> -#define CONFIG_SPL_MAX_SIZE		12320
> +/* SPL max size is 12K -- but --pad-to requires a single decimal number */
> +#define CONFIG_SPL_MAX_SIZE		12288
> +#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)

This is wrong, you've just increased the overall limit to 16K.  I know
there's a reason that current limit is so exact, Heiko?  And also, this
shows the conceptual problem I have (and 2/2 has the same, along with
tegra).  The important limit is the combined size.  It doesn't matter if
it's 11K text/data/rodata and 1K BSS, or 8+4.  When using custom linker
scripts, we avoid this and can just comment overall (which would need
adding here) that we only care about the combined size.  But then tegra
would be wrong since it uses the generic arm spl linker script?
Heiko Schocher - April 9, 2013, 6:50 a.m.
Hello Tom,

Am 08.04.2013 22:43, schrieb Tom Rini:
> On Mon, Apr 08, 2013 at 09:58:26AM -0000, Albert ARIBAUD wrote:
> 
>> CONFIG_SPL_MAX_SIZE wrongly included BSS size. Split
>> max size between image and BSS based on sizes reported
>> for current build.
>>
>> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>
>> ---
>> board/ait/cam_enc_4xx/u-boot-spl.lds |    2 +-
>>  include/configs/cam_enc_4xx.h        |    4 +++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds b/board/ait/cam_enc_4xx/u-boot-spl.lds
>> index dd9d52d..25625dc 100644
>> --- a/board/ait/cam_enc_4xx/u-boot-spl.lds
>> +++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
>> @@ -25,7 +25,7 @@
>>   */
>>  
>>  MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
>> -		LENGTH = CONFIG_SPL_MAX_SIZE }
>> +		LENGTH = (CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE) }
>>  
>>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>>  OUTPUT_ARCH(arm)
>> diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h
>> index 56528dd..df3682b 100644
>> --- a/include/configs/cam_enc_4xx.h
>> +++ b/include/configs/cam_enc_4xx.h
>> @@ -230,7 +230,9 @@
>>  #define CONFIG_SPL_STACK		(0x00010000 + 0x7f00)
>>  
>>  #define CONFIG_SPL_TEXT_BASE		0x00000020 /*CONFIG_SYS_SRAM_START*/
>> -#define CONFIG_SPL_MAX_SIZE		12320
>> +/* SPL max size is 12K -- but --pad-to requires a single decimal number */
>> +#define CONFIG_SPL_MAX_SIZE		12288
>> +#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
> 
> This is wrong, you've just increased the overall limit to 16K.  I know
> there's a reason that current limit is so exact, Heiko?  And also, this

The cam_enc_4xx use only 12k for the SPL code. This is defined in the
UBL header, see u-boot:doc/README.davinci.nand_spl, but can be adapted
for this board. The SoC has an IRam of 32K - ~2k for RBL stack, see:

http://www.ti.com/lit/gpn/tms320dm368

I have no access anymore to this HW to do some tests :-( so I looked
into the hexdump of the current u-boot code with your patch applied, and
the code on the interesting borders (0x0, 0x800 and 0x3800) looks good
to me ...

> shows the conceptual problem I have (and 2/2 has the same, along with
> tegra).  The important limit is the combined size.  It doesn't matter if
> it's 11K text/data/rodata and 1K BSS, or 8+4.  When using custom linker
> scripts, we avoid this and can just comment overall (which would need
> adding here) that we only care about the combined size.  But then tegra
> would be wrong since it uses the generic arm spl linker script?

bye,
Heiko
Albert ARIBAUD - April 9, 2013, 9:08 a.m.
Hi Heiko,

On Tue, 09 Apr 2013 08:50:26 +0200, Heiko Schocher <hs@denx.de> wrote:

> Hello Tom,
> 
> Am 08.04.2013 22:43, schrieb Tom Rini:
> > On Mon, Apr 08, 2013 at 09:58:26AM -0000, Albert ARIBAUD wrote:
> > 
> >> CONFIG_SPL_MAX_SIZE wrongly included BSS size. Split
> >> max size between image and BSS based on sizes reported
> >> for current build.
> >>
> >> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>
> >> ---
> >> board/ait/cam_enc_4xx/u-boot-spl.lds |    2 +-
> >>  include/configs/cam_enc_4xx.h        |    4 +++-
> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds b/board/ait/cam_enc_4xx/u-boot-spl.lds
> >> index dd9d52d..25625dc 100644
> >> --- a/board/ait/cam_enc_4xx/u-boot-spl.lds
> >> +++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
> >> @@ -25,7 +25,7 @@
> >>   */
> >>  
> >>  MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
> >> -		LENGTH = CONFIG_SPL_MAX_SIZE }
> >> +		LENGTH = (CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE) }
> >>  
> >>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
> >>  OUTPUT_ARCH(arm)
> >> diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h
> >> index 56528dd..df3682b 100644
> >> --- a/include/configs/cam_enc_4xx.h
> >> +++ b/include/configs/cam_enc_4xx.h
> >> @@ -230,7 +230,9 @@
> >>  #define CONFIG_SPL_STACK		(0x00010000 + 0x7f00)
> >>  
> >>  #define CONFIG_SPL_TEXT_BASE		0x00000020 /*CONFIG_SYS_SRAM_START*/
> >> -#define CONFIG_SPL_MAX_SIZE		12320
> >> +/* SPL max size is 12K -- but --pad-to requires a single decimal number */
> >> +#define CONFIG_SPL_MAX_SIZE		12288
> >> +#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
> > 
> > This is wrong, you've just increased the overall limit to 16K.  I know
> > there's a reason that current limit is so exact, Heiko?  And also, this
> 
> The cam_enc_4xx use only 12k for the SPL code. This is defined in the
> UBL header, see u-boot:doc/README.davinci.nand_spl, but can be adapted
> for this board. The SoC has an IRam of 32K - ~2k for RBL stack, see:
> 
> http://www.ti.com/lit/gpn/tms320dm368
> 
> I have no access anymore to this HW to do some tests :-( so I looked
> into the hexdump of the current u-boot code with your patch applied, and
> the code on the interesting borders (0x0, 0x800 and 0x3800) looks good
> to me ...
> 
> > shows the conceptual problem I have (and 2/2 has the same, along with
> > tegra).  The important limit is the combined size.  It doesn't matter if
> > it's 11K text/data/rodata and 1K BSS, or 8+4.  When using custom linker
> > scripts, we avoid this and can just comment overall (which would need
> > adding here) that we only care about the combined size.  But then tegra
> > would be wrong since it uses the generic arm spl linker script?

Thanks Heiko.

I'd read about the SoC IRAM, and had chosen 16K indeed arbitrarily but
taking care not to use most of it -- half felt like safe enough.
However, I'd missed the UBL thing, thanks for pointing this out. So
either I keep 12K, split for instance 10K and 2K (5 pages and 1 page),
or I reaise the number of pages in board/ait/cam_enc_4xx/ublimage.cfg,
correct?

Let us assume I keep 12K. Here is a current build of cam_enc_4xx:

text  data   bss   dec  hex filename
439526  13148  311092  763766  ba776  ./u-boot
  9073	  840     500	10413   28ad  ./spl/u-boot-spl

And the map file gives __start = 0x20, __bss_start = 0x26e0, and
__bss_end = __image_copy_end = _end = 0x28d4, which makes the
size of the non-BSS part of the image equal to 9952 bytes (thus below
10K) and the BSS part size is 500 bytes, below 2K.

So, it seems I could just replace

#define CONFIG_SPL_MAX_SIZE		12288
#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)

with

#define CONFIG_SPL_MAX_SIZE		10240
#define CONFIG_SPL_BSS_MAX_SIZE		(2*1024)

and keep the UBL cfg file untouched -- any future size issue with
image or BSS size would imply changing these values and uptating the
UBL cfg file.

Would that be ok?

> bye,
> Heiko

Amicalement,
Heiko Schocher - April 9, 2013, 12:11 p.m.
Hello Albert,

On 09.04.2013 11:08, Albert ARIBAUD wrote:
> Hi Heiko,
> 
> On Tue, 09 Apr 2013 08:50:26 +0200, Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Tom,
>>
>> Am 08.04.2013 22:43, schrieb Tom Rini:
>>> On Mon, Apr 08, 2013 at 09:58:26AM -0000, Albert ARIBAUD wrote:
>>>
>>>> CONFIG_SPL_MAX_SIZE wrongly included BSS size. Split
>>>> max size between image and BSS based on sizes reported
>>>> for current build.
>>>>
>>>> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>>
>>>> ---
>>>> board/ait/cam_enc_4xx/u-boot-spl.lds |    2 +-
>>>>  include/configs/cam_enc_4xx.h        |    4 +++-
>>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds b/board/ait/cam_enc_4xx/u-boot-spl.lds
>>>> index dd9d52d..25625dc 100644
>>>> --- a/board/ait/cam_enc_4xx/u-boot-spl.lds
>>>> +++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
>>>> @@ -25,7 +25,7 @@
>>>>   */
>>>>  
>>>>  MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
>>>> -		LENGTH = CONFIG_SPL_MAX_SIZE }
>>>> +		LENGTH = (CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE) }
>>>>  
>>>>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>>>>  OUTPUT_ARCH(arm)
>>>> diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h
>>>> index 56528dd..df3682b 100644
>>>> --- a/include/configs/cam_enc_4xx.h
>>>> +++ b/include/configs/cam_enc_4xx.h
>>>> @@ -230,7 +230,9 @@
>>>>  #define CONFIG_SPL_STACK		(0x00010000 + 0x7f00)
>>>>  
>>>>  #define CONFIG_SPL_TEXT_BASE		0x00000020 /*CONFIG_SYS_SRAM_START*/
>>>> -#define CONFIG_SPL_MAX_SIZE		12320
>>>> +/* SPL max size is 12K -- but --pad-to requires a single decimal number */
>>>> +#define CONFIG_SPL_MAX_SIZE		12288
>>>> +#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
>>>
>>> This is wrong, you've just increased the overall limit to 16K.  I know
>>> there's a reason that current limit is so exact, Heiko?  And also, this
>>
>> The cam_enc_4xx use only 12k for the SPL code. This is defined in the
>> UBL header, see u-boot:doc/README.davinci.nand_spl, but can be adapted
>> for this board. The SoC has an IRam of 32K - ~2k for RBL stack, see:
>>
>> http://www.ti.com/lit/gpn/tms320dm368
>>
>> I have no access anymore to this HW to do some tests :-( so I looked
>> into the hexdump of the current u-boot code with your patch applied, and
>> the code on the interesting borders (0x0, 0x800 and 0x3800) looks good
>> to me ...
>>
>>> shows the conceptual problem I have (and 2/2 has the same, along with
>>> tegra).  The important limit is the combined size.  It doesn't matter if
>>> it's 11K text/data/rodata and 1K BSS, or 8+4.  When using custom linker
>>> scripts, we avoid this and can just comment overall (which would need
>>> adding here) that we only care about the combined size.  But then tegra
>>> would be wrong since it uses the generic arm spl linker script?
> 
> Thanks Heiko.
> 
> I'd read about the SoC IRAM, and had chosen 16K indeed arbitrarily but
> taking care not to use most of it -- half felt like safe enough.
> However, I'd missed the UBL thing, thanks for pointing this out. So
> either I keep 12K, split for instance 10K and 2K (5 pages and 1 page),
> or I reaise the number of pages in board/ait/cam_enc_4xx/ublimage.cfg,
> correct?

Yes, but I would prefer not to change the number of pages.

> Let us assume I keep 12K. Here is a current build of cam_enc_4xx:
> 
> text  data   bss   dec  hex filename
> 439526  13148  311092  763766  ba776  ./u-boot
>   9073	  840     500	10413   28ad  ./spl/u-boot-spl
> 
> And the map file gives __start = 0x20, __bss_start = 0x26e0, and
> __bss_end = __image_copy_end = _end = 0x28d4, which makes the
> size of the non-BSS part of the image equal to 9952 bytes (thus below
> 10K) and the BSS part size is 500 bytes, below 2K.
> 
> So, it seems I could just replace
> 
> #define CONFIG_SPL_MAX_SIZE		12288
> #define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
> 
> with
> 
> #define CONFIG_SPL_MAX_SIZE		10240
> #define CONFIG_SPL_BSS_MAX_SIZE		(2*1024)
> 
> and keep the UBL cfg file untouched -- any future size issue with
> image or BSS size would imply changing these values and uptating the
> UBL cfg file.
> 
> Would that be ok?

Yes, that seems good to me, but I could not test it ...

bye,
Heiko
Albert ARIBAUD - April 9, 2013, 12:42 p.m.
Hi Heiko,

On Tue, 09 Apr 2013 14:11:38 +0200, Heiko Schocher <hs@denx.de> wrote:

> > Let us assume I keep 12K. Here is a current build of cam_enc_4xx:
> > 
> > text  data   bss   dec  hex filename
> > 439526  13148  311092  763766  ba776  ./u-boot
> >   9073	  840     500	10413   28ad  ./spl/u-boot-spl
> > 
> > And the map file gives __start = 0x20, __bss_start = 0x26e0, and
> > __bss_end = __image_copy_end = _end = 0x28d4, which makes the
> > size of the non-BSS part of the image equal to 9952 bytes (thus below
> > 10K) and the BSS part size is 500 bytes, below 2K.
> > 
> > So, it seems I could just replace
> > 
> > #define CONFIG_SPL_MAX_SIZE		12288
> > #define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
> > 
> > with
> > 
> > #define CONFIG_SPL_MAX_SIZE		10240
> > #define CONFIG_SPL_BSS_MAX_SIZE		(2*1024)
> > 
> > and keep the UBL cfg file untouched -- any future size issue with
> > image or BSS size would imply changing these values and uptating the
> > UBL cfg file.
> > 
> > Would that be ok?
> 
> Yes, that seems good to me, but I could not test it ...

I can do a diff of the binaries with and without the patch --
normally, they should be the same except for the U-Boot version
identification. Would that do?

> bye,
> Heiko

Amicalement,
Heiko Schocher - April 9, 2013, 1:17 p.m.
Hello Albert,

on 09.04.2013 14:42, wrote Albert ARIBAUD:
> Hi Heiko,
> 
> On Tue, 09 Apr 2013 14:11:38 +0200, Heiko Schocher <hs@denx.de> wrote:
> 
>>> Let us assume I keep 12K. Here is a current build of cam_enc_4xx:
>>>
>>> text  data   bss   dec  hex filename
>>> 439526  13148  311092  763766  ba776  ./u-boot
>>>   9073	  840     500	10413   28ad  ./spl/u-boot-spl
[...]
>>> Would that be ok?
>>
>> Yes, that seems good to me, but I could not test it ...
> 
> I can do a diff of the binaries with and without the patch --
> normally, they should be the same except for the U-Boot version
> identification. Would that do?

Yes. I did this too, and see only a diff for the version string.
(But not forget to commit your change first, else the "-dirty"
in the version string will insert an offset ;-)

bye,
Heiko
Albert ARIBAUD - April 9, 2013, 2:11 p.m.
Hi Heiko,

On Tue, 09 Apr 2013 15:17:02 +0200, Heiko Schocher <hs@denx.de> wrote:

> Hello Albert,
> 
> on 09.04.2013 14:42, wrote Albert ARIBAUD:
> > Hi Heiko,
> > 
> > On Tue, 09 Apr 2013 14:11:38 +0200, Heiko Schocher <hs@denx.de> wrote:
> > 
> >>> Let us assume I keep 12K. Here is a current build of cam_enc_4xx:
> >>>
> >>> text  data   bss   dec  hex filename
> >>> 439526  13148  311092  763766  ba776  ./u-boot
> >>>   9073	  840     500	10413   28ad  ./spl/u-boot-spl
> [...]
> >>> Would that be ok?
> >>
> >> Yes, that seems good to me, but I could not test it ...
> > 
> > I can do a diff of the binaries with and without the patch --
> > normally, they should be the same except for the U-Boot version
> > identification. Would that do?
> 
> Yes. I did this too, and see only a diff for the version string.

Thanks -- I've staged the 10K+2K change for V2.

> (But not forget to commit your change first, else the "-dirty"
> in the version string will insert an offset ;-)

I actually keep a 'fake_build' branch with a single nifty commit on
it that makes almost all version information sources constant (only
mkimage resists as it collects the current date and time
programmatically, but that's ok, I don't aim for 100% faking.

So, whenever I need to do bulk comparisons, I add this commit above the
two branches to be compared, and two batch builds later, I can compare
the .bins, even if different commit ID and/or local changes are
involved. :)

> bye,
> Heiko

Amicalement,

Patch

diff --git a/board/ait/cam_enc_4xx/u-boot-spl.lds b/board/ait/cam_enc_4xx/u-boot-spl.lds
index dd9d52d..25625dc 100644
--- a/board/ait/cam_enc_4xx/u-boot-spl.lds
+++ b/board/ait/cam_enc_4xx/u-boot-spl.lds
@@ -25,7 +25,7 @@ 
  */
 
 MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
-		LENGTH = CONFIG_SPL_MAX_SIZE }
+		LENGTH = (CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE) }
 
 OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
 OUTPUT_ARCH(arm)
diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h
index 56528dd..df3682b 100644
--- a/include/configs/cam_enc_4xx.h
+++ b/include/configs/cam_enc_4xx.h
@@ -230,7 +230,9 @@ 
 #define CONFIG_SPL_STACK		(0x00010000 + 0x7f00)
 
 #define CONFIG_SPL_TEXT_BASE		0x00000020 /*CONFIG_SYS_SRAM_START*/
-#define CONFIG_SPL_MAX_SIZE		12320
+/* SPL max size is 12K -- but --pad-to requires a single decimal number */
+#define CONFIG_SPL_MAX_SIZE		12288
+#define CONFIG_SPL_BSS_MAX_SIZE		(4*1024)
 
 #ifndef CONFIG_SPL_BUILD
 #define CONFIG_SYS_TEXT_BASE		0x81080000