All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
@ 2016-06-30  0:29 Andre Przywara
  2016-06-30  1:54 ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2016-06-30  0:29 UTC (permalink / raw)
  To: u-boot

Probably due to some (ill-founded) fear of a large BSS all sunxi boards
forced their SPL BSS section into DRAM.
This only works if there is no usage of a .BSS variable before the DRAM
is initialised.
The recent inclusion of tiny-printf breaks this assumption (it has two
variables in .BSS), so any early printf (printing a number) hangs a board.
This in particular breaks the (WIP) Pine64 SPL, which at the moment links
Allwinner's libdram library, trying to print debug information:
DRAM:DRAM driver version: V1.0
DRAM Type = <hangs>

As it turns out the normal BSS size for sunxi is about 256 Bytes, so we
can happily remove the symbols and the linker script part that was
forcing the section into DRAM and let the linker naturally put it into
SRAM.
Tested on BananaPi M1 and Pine64(-SPL), also buildman sunxi was happy.

Thanks to Siarhei for providing helpful hints!

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---

(and now with the list in CC: as well) ...

 arch/arm/cpu/armv7/sunxi/u-boot-spl.lds | 4 +---
 include/configs/sunxi-common.h          | 4 ----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
index 53f0cbd..a90404f 100644
--- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
+++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
@@ -16,8 +16,6 @@
  */
 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 }
 
 OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
 OUTPUT_ARCH(arm)
@@ -54,5 +52,5 @@ SECTIONS
 		*(.bss*)
 		. = ALIGN(4);
 		__bss_end = .;
-	} > .sdram
+	} > .sram
 }
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 94275a7..e3fe965 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -75,7 +75,6 @@
  * since it needs to fit in with the other values. By also #defining it
  * we get warnings if the Kconfig value mismatches. */
 #define CONFIG_SPL_STACK_R_ADDR		0x2fe00000
-#define CONFIG_SPL_BSS_START_ADDR	0x2ff80000
 #else
 #define SDRAM_OFFSET(x) 0x4##x
 #define CONFIG_SYS_SDRAM_BASE		0x40000000
@@ -86,11 +85,8 @@
  * since it needs to fit in with the other values. By also #defining it
  * we get warnings if the Kconfig value mismatches. */
 #define CONFIG_SPL_STACK_R_ADDR		0x4fe00000
-#define CONFIG_SPL_BSS_START_ADDR	0x4ff80000
 #endif
 
-#define CONFIG_SPL_BSS_MAX_SIZE		0x00080000 /* 512 KiB */
-
 #if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I)
 /*
  * The A80's A1 sram starts at 0x00010000 rather then at 0x00000000 and is
-- 
2.9.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-06-30  0:29 [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM Andre Przywara
@ 2016-06-30  1:54 ` Marek Vasut
  2016-06-30  8:50   ` Andre Przywara
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2016-06-30  1:54 UTC (permalink / raw)
  To: u-boot

On 06/30/2016 02:29 AM, Andre Przywara wrote:
> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
> forced their SPL BSS section into DRAM.
> This only works if there is no usage of a .BSS variable before the DRAM
> is initialised.
> The recent inclusion of tiny-printf breaks this assumption (it has two
> variables in .BSS), so any early printf (printing a number) hangs a board.

I believe you should fix tiny-printf instead, try this patch:

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 451f4f7..5b9b0dc 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -17,7 +17,7 @@ static char *bf;
 static char zs;

 /* Current position in sprintf() output string */
-static char *outstr;
+static char *outstr __section(".data");

 static void out(char c)
 {

> This in particular breaks the (WIP) Pine64 SPL, which at the moment links
> Allwinner's libdram library, trying to print debug information:
> DRAM:DRAM driver version: V1.0
> DRAM Type = <hangs>
> 
> As it turns out the normal BSS size for sunxi is about 256 Bytes, so we
> can happily remove the symbols and the linker script part that was
> forcing the section into DRAM and let the linker naturally put it into
> SRAM.

Except SRAM is limited, which is why bss was in DRAM.

> Tested on BananaPi M1 and Pine64(-SPL), also buildman sunxi was happy.
> 
> Thanks to Siarhei for providing helpful hints!
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> 
> (and now with the list in CC: as well) ...
> 
>  arch/arm/cpu/armv7/sunxi/u-boot-spl.lds | 4 +---
>  include/configs/sunxi-common.h          | 4 ----
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
> index 53f0cbd..a90404f 100644
> --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
> +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
> @@ -16,8 +16,6 @@
>   */
>  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 }
>  
>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>  OUTPUT_ARCH(arm)
> @@ -54,5 +52,5 @@ SECTIONS
>  		*(.bss*)
>  		. = ALIGN(4);
>  		__bss_end = .;
> -	} > .sdram
> +	} > .sram
>  }
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 94275a7..e3fe965 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -75,7 +75,6 @@
>   * since it needs to fit in with the other values. By also #defining it
>   * we get warnings if the Kconfig value mismatches. */
>  #define CONFIG_SPL_STACK_R_ADDR		0x2fe00000
> -#define CONFIG_SPL_BSS_START_ADDR	0x2ff80000
>  #else
>  #define SDRAM_OFFSET(x) 0x4##x
>  #define CONFIG_SYS_SDRAM_BASE		0x40000000
> @@ -86,11 +85,8 @@
>   * since it needs to fit in with the other values. By also #defining it
>   * we get warnings if the Kconfig value mismatches. */
>  #define CONFIG_SPL_STACK_R_ADDR		0x4fe00000
> -#define CONFIG_SPL_BSS_START_ADDR	0x4ff80000
>  #endif
>  
> -#define CONFIG_SPL_BSS_MAX_SIZE		0x00080000 /* 512 KiB */
> -
>  #if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I)
>  /*
>   * The A80's A1 sram starts at 0x00010000 rather then at 0x00000000 and is
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-06-30  1:54 ` Marek Vasut
@ 2016-06-30  8:50   ` Andre Przywara
  0 siblings, 0 replies; 9+ messages in thread
From: Andre Przywara @ 2016-06-30  8:50 UTC (permalink / raw)
  To: u-boot

Hi Marek,

On 30/06/16 02:54, Marek Vasut wrote:
> On 06/30/2016 02:29 AM, Andre Przywara wrote:
>> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
>> forced their SPL BSS section into DRAM.
>> This only works if there is no usage of a .BSS variable before the DRAM
>> is initialised.
>> The recent inclusion of tiny-printf breaks this assumption (it has two
>> variables in .BSS), so any early printf (printing a number) hangs a board.
> 
> I believe you should fix tiny-printf instead, try this patch:

Mmmh, that looks like a hack to paper over another hack for me. I don't
think we should start to annotate seemingly random variables (at least
in the code) just to cover up for some linker hack.
And frankly, having bss in initially inaccessible memory without
documenting this or enforcing sanity checks to catch those cases like
tiny-printf seems quite hacky to me.
Also if I am not mistaken we don't actually clear BSS for the SPL?

I could live with some compiler switch or linker script snippet to avoid
using BSS variables for the SPL (or parts of it, at least).

Or we provide a per SoC BSS offset to utilise other SRAM locations and
use that for BSS.

> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 451f4f7..5b9b0dc 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -17,7 +17,7 @@ static char *bf;
>  static char zs;
> 
>  /* Current position in sprintf() output string */
> -static char *outstr;
> +static char *outstr __section(".data");

To prove my debugging theory I initialised _both_ those variables to 1,
that also worked. But definitely you need to cover "zs" as well.

> 
>  static void out(char c)
>  {
> 
>> This in particular breaks the (WIP) Pine64 SPL, which at the moment links
>> Allwinner's libdram library, trying to print debug information:
>> DRAM:DRAM driver version: V1.0
>> DRAM Type = <hangs>
>>
>> As it turns out the normal BSS size for sunxi is about 256 Bytes, so we
>> can happily remove the symbols and the linker script part that was
>> forcing the section into DRAM and let the linker naturally put it into
>> SRAM.
> 
> Except SRAM is limited, which is why bss was in DRAM.

Except that DRAM is not available initially ;-)

Cheers,
Andre.

> 
>> Tested on BananaPi M1 and Pine64(-SPL), also buildman sunxi was happy.
>>
>> Thanks to Siarhei for providing helpful hints!
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>
>> (and now with the list in CC: as well) ...
>>
>>  arch/arm/cpu/armv7/sunxi/u-boot-spl.lds | 4 +---
>>  include/configs/sunxi-common.h          | 4 ----
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> index 53f0cbd..a90404f 100644
>> --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> @@ -16,8 +16,6 @@
>>   */
>>  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 }
>>  
>>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>>  OUTPUT_ARCH(arm)
>> @@ -54,5 +52,5 @@ SECTIONS
>>  		*(.bss*)
>>  		. = ALIGN(4);
>>  		__bss_end = .;
>> -	} > .sdram
>> +	} > .sram
>>  }
>> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
>> index 94275a7..e3fe965 100644
>> --- a/include/configs/sunxi-common.h
>> +++ b/include/configs/sunxi-common.h
>> @@ -75,7 +75,6 @@
>>   * since it needs to fit in with the other values. By also #defining it
>>   * we get warnings if the Kconfig value mismatches. */
>>  #define CONFIG_SPL_STACK_R_ADDR		0x2fe00000
>> -#define CONFIG_SPL_BSS_START_ADDR	0x2ff80000
>>  #else
>>  #define SDRAM_OFFSET(x) 0x4##x
>>  #define CONFIG_SYS_SDRAM_BASE		0x40000000
>> @@ -86,11 +85,8 @@
>>   * since it needs to fit in with the other values. By also #defining it
>>   * we get warnings if the Kconfig value mismatches. */
>>  #define CONFIG_SPL_STACK_R_ADDR		0x4fe00000
>> -#define CONFIG_SPL_BSS_START_ADDR	0x4ff80000
>>  #endif
>>  
>> -#define CONFIG_SPL_BSS_MAX_SIZE		0x00080000 /* 512 KiB */
>> -
>>  #if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I)
>>  /*
>>   * The A80's A1 sram starts at 0x00010000 rather then at 0x00000000 and is
>>
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-07-02 11:49     ` Hans de Goede
  2016-07-03 21:15       ` Simon Glass
@ 2016-07-03 22:24       ` André Przywara
  1 sibling, 0 replies; 9+ messages in thread
From: André Przywara @ 2016-07-03 22:24 UTC (permalink / raw)
  To: u-boot

On 02/07/16 12:49, Hans de Goede wrote:

Hi Hans,

> On 30-06-16 17:24, Simon Glass wrote:
>> Hi,
>>
>> On 30 June 2016 at 03:00, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi Andre,
>>>
>>> On 30-06-16 02:25, Andre Przywara wrote:
>>>>
>>>> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
>>>> forced their SPL BSS section into DRAM.
>>>> This only works if there is no usage of a .BSS variable before the DRAM
>>>> is initialised.
>>>> The recent inclusion of tiny-printf breaks this assumption (it has two
>>>> variables in .BSS), so any early printf (printing a number) hangs a
>>>> board.
>>>> This in particular breaks the (WIP) Pine64 SPL, which at the moment
>>>> links
>>>> Allwinner's libdram library, trying to print debug information:
>>>> DRAM:DRAM driver version: V1.0
>>>> DRAM Type = <hangs>
>>>
>>>
>>> Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in
>>> DRAM, esp. since the bss use may grow over time, and the SPL space is
>>> quite
>>> small.
>>>
>>> Moreover, given that tiny-printf is specifically meant for use in SPL /
>>> restricted environments and having BSS in DRAM is not unheard of for
>>> other boards, it seems to me like this is something which should really
>>> be fixed in tinyprintf instead.
>>>
>>> Have you tried looking into fixing this at the tinyprintf level ?
>>
>> Marek's fix is one option. Another is to make use of global_data,
>> which will be available from very early and it set to zero.
> 
> I think Marek's fix is fine, we should go and merge that.

Only that it's not sufficient as it (all three variables should be
covered). I have a better version which I will send.
I found the promising -fno-zero-initialized-in-bss gcc switch, but this
apparently only works for *explicitly* initialized variables. So:
static char zs = 0;
is fine (moved to .data), but without the "= 0" it's still in BSS.
Initialising those variables is pointless and confusing, so at least
requires a comment. At this point I think we can just use the explicit
section attribute.
A more involved way would be to avoid those global variables in the
first place, but I don't think it's worth the effort.

> As for the worries about not using BSS before DRAM is initialized
> coming back to bite us. Yes that may happen, but we are not the
> only board / mach / soc code to do this.

I don't think that's a good excuse ;-)

> I agree that documenting
> this somewhere would be good (patches welcome).

I wonder if there is a way to _enforce_ this.
Can't we somehow mark code that runs before DRAM init and check that
this marked code doesn't have anything in BSS?
Or maybe tagging this code (or the files) and putting their BSS
variables in SRAM, while letting the rest of the SPL use a DRAM based BSS?
Having compile warnings or errors is much better than hard to debug breaks.

> As for just putting BSS in the sram, nack. Thanks to various efforts
> we currently have some free space for the SPL, but in the future
> we will likely add NAND support, and try to move the SPL to
> dm (with static device instantiation, no space for dt) so that we
> can get rid of the duplicate non-dm + dm gpio and uart code we
> currently have. These things combined will push things to their
> limit and may very well grow the BSS section too.

I am bit worried that U-Boot is accepting hacks that paper over other
hacks. Frankly: we shouldn't save memory beyond the point where it
breaks things and makes them close to unmaintainable. Relying on
variables to be in certain sections or not being accessed before a
certain point in time is fragile (in fact it just broke!), so please
note my protest against this approach.

We have more SRAM space available in many SoCs, so I will send another
patch that makes use of them if possible (starting with the A64).
That should be the route that we take for the future, limiting this "BSS
in DRAM" hack to just the SoCs that are in desperate need for it.

Cheers,
Andre

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-07-02 11:49     ` Hans de Goede
@ 2016-07-03 21:15       ` Simon Glass
  2016-07-03 22:24       ` André Przywara
  1 sibling, 0 replies; 9+ messages in thread
From: Simon Glass @ 2016-07-03 21:15 UTC (permalink / raw)
  To: u-boot

Hi Hans,

On 2 July 2016 at 04:49, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 30-06-16 17:24, Simon Glass wrote:
>>
>> Hi,
>>
>> On 30 June 2016 at 03:00, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Andre,
>>>
>>> On 30-06-16 02:25, Andre Przywara wrote:
>>>>
>>>>
>>>> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
>>>> forced their SPL BSS section into DRAM.
>>>> This only works if there is no usage of a .BSS variable before the DRAM
>>>> is initialised.
>>>> The recent inclusion of tiny-printf breaks this assumption (it has two
>>>> variables in .BSS), so any early printf (printing a number) hangs a
>>>> board.
>>>> This in particular breaks the (WIP) Pine64 SPL, which at the moment
>>>> links
>>>> Allwinner's libdram library, trying to print debug information:
>>>> DRAM:DRAM driver version: V1.0
>>>> DRAM Type = <hangs>
>>>
>>>
>>>
>>> Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in
>>> DRAM, esp. since the bss use may grow over time, and the SPL space is
>>> quite
>>> small.
>>>
>>> Moreover, given that tiny-printf is specifically meant for use in SPL /
>>> restricted environments and having BSS in DRAM is not unheard of for
>>> other boards, it seems to me like this is something which should really
>>> be fixed in tinyprintf instead.
>>>
>>> Have you tried looking into fixing this at the tinyprintf level ?
>>
>>
>> Marek's fix is one option. Another is to make use of global_data,
>> which will be available from very early and it set to zero.
>
>
> I think Marek's fix is fine, we should go and merge that.
>
> As for the worries about not using BSS before DRAM is initialized
> coming back to bite us. Yes that may happen, but we are not the
> only board / mach / soc code to do this. I agree that documenting
> this somewhere would be good (patches welcome).
>
> As for just putting BSS in the sram, nack. Thanks to various efforts
> we currently have some free space for the SPL, but in the future
> we will likely add NAND support, and try to move the SPL to
> dm (with static device instantiation, no space for dt) so that we
> can get rid of the duplicate non-dm + dm gpio and uart code we
> currently have. These things combined will push things to their
> limit and may very well grow the BSS section too.

FWIW dm is designed to avoid using BSS. Also if it helps, see my
series which converts the DT to C data structures.

Regards,
Simon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
       [not found]   ` <CAPnjgZ2ajoU90kSNMoHVhbXN4CdeKzs5-SQ-Hq-xePfC0brJ3A@mail.gmail.com>
@ 2016-07-02 11:49     ` Hans de Goede
  2016-07-03 21:15       ` Simon Glass
  2016-07-03 22:24       ` André Przywara
  0 siblings, 2 replies; 9+ messages in thread
From: Hans de Goede @ 2016-07-02 11:49 UTC (permalink / raw)
  To: u-boot

Hi,

On 30-06-16 17:24, Simon Glass wrote:
> Hi,
>
> On 30 June 2016 at 03:00, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Andre,
>>
>> On 30-06-16 02:25, Andre Przywara wrote:
>>>
>>> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
>>> forced their SPL BSS section into DRAM.
>>> This only works if there is no usage of a .BSS variable before the DRAM
>>> is initialised.
>>> The recent inclusion of tiny-printf breaks this assumption (it has two
>>> variables in .BSS), so any early printf (printing a number) hangs a board.
>>> This in particular breaks the (WIP) Pine64 SPL, which at the moment links
>>> Allwinner's libdram library, trying to print debug information:
>>> DRAM:DRAM driver version: V1.0
>>> DRAM Type = <hangs>
>>
>>
>> Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in
>> DRAM, esp. since the bss use may grow over time, and the SPL space is quite
>> small.
>>
>> Moreover, given that tiny-printf is specifically meant for use in SPL /
>> restricted environments and having BSS in DRAM is not unheard of for
>> other boards, it seems to me like this is something which should really
>> be fixed in tinyprintf instead.
>>
>> Have you tried looking into fixing this at the tinyprintf level ?
>
> Marek's fix is one option. Another is to make use of global_data,
> which will be available from very early and it set to zero.

I think Marek's fix is fine, we should go and merge that.

As for the worries about not using BSS before DRAM is initialized
coming back to bite us. Yes that may happen, but we are not the
only board / mach / soc code to do this. I agree that documenting
this somewhere would be good (patches welcome).

As for just putting BSS in the sram, nack. Thanks to various efforts
we currently have some free space for the SPL, but in the future
we will likely add NAND support, and try to move the SPL to
dm (with static device instantiation, no space for dt) so that we
can get rid of the duplicate non-dm + dm gpio and uart code we
currently have. These things combined will push things to their
limit and may very well grow the BSS section too.

Regards,

Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-06-30 17:40     ` Peter Korsgaard
@ 2016-06-30 21:09       ` Marek Vasut
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2016-06-30 21:09 UTC (permalink / raw)
  To: u-boot

On 06/30/2016 07:40 PM, Peter Korsgaard wrote:
>>>>>> "Andre" == Andre Przywara <andre.przywara@arm.com> writes:
> 
> Hi,
> 
>  > I really don't know. This simple bug has cost me at least two hours
>  > yesterday, since it was the rather innocent access to a variable that
>  > caused the issue. And if it wouldn't have been for Siarhei to point me
>  > in the right direction I'd have spend even more time to find a fix.
> 
> Agreed. The toolchain will complain loudly about an overflow of SRAM
> space, but not about BSS access before DRAM is available.
> 
I see two problems:
- this fixes sunxi and possibly leaves other platforms unfixed
- when the spl on sunxi grows some more, the bss won't fit and you'll
  have a problem again, except much bigger this time

I'd rather have this fixed on tiny-printf level with a bit of
documentation on why that's done the way it's done.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
  2016-06-30 15:43   ` Andre Przywara
@ 2016-06-30 17:40     ` Peter Korsgaard
  2016-06-30 21:09       ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Korsgaard @ 2016-06-30 17:40 UTC (permalink / raw)
  To: u-boot

>>>>> "Andre" == Andre Przywara <andre.przywara@arm.com> writes:

Hi,

 > I really don't know. This simple bug has cost me at least two hours
 > yesterday, since it was the rather innocent access to a variable that
 > caused the issue. And if it wouldn't have been for Siarhei to point me
 > in the right direction I'd have spend even more time to find a fix.

Agreed. The toolchain will complain loudly about an overflow of SRAM
space, but not about BSS access before DRAM is available.

-- 
Bye, Peter Korsgaard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
       [not found] ` <af86054b-8fcc-3b58-6889-7af02cd7a9fc@redhat.com>
@ 2016-06-30 15:43   ` Andre Przywara
  2016-06-30 17:40     ` Peter Korsgaard
       [not found]   ` <CAPnjgZ2ajoU90kSNMoHVhbXN4CdeKzs5-SQ-Hq-xePfC0brJ3A@mail.gmail.com>
  1 sibling, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2016-06-30 15:43 UTC (permalink / raw)
  To: u-boot

Hi Hans,

(CC:ing the list, which I accidentally forgot on this first post)

On 30/06/16 11:00, Hans de Goede wrote:
> Hi Andre,
> 
> On 30-06-16 02:25, Andre Przywara wrote:
>> Probably due to some (ill-founded) fear of a large BSS all sunxi boards
>> forced their SPL BSS section into DRAM.
>> This only works if there is no usage of a .BSS variable before the DRAM
>> is initialised.
>> The recent inclusion of tiny-printf breaks this assumption (it has two
>> variables in .BSS), so any early printf (printing a number) hangs a
>> board.
>> This in particular breaks the (WIP) Pine64 SPL, which at the moment links
>> Allwinner's libdram library, trying to print debug information:
>> DRAM:DRAM driver version: V1.0
>> DRAM Type = <hangs>
> 
> Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in
> DRAM, esp. since the bss use may grow over time, and the SPL space is quite
> small.
> 
> Moreover, given that tiny-printf is specifically meant for use in SPL /
> restricted environments and having BSS in DRAM is not unheard of for
> other boards, it seems to me like this is something which should really
> be fixed in tinyprintf instead.

I really don't know. This simple bug has cost me at least two hours
yesterday, since it was the rather innocent access to a variable that
caused the issue. And if it wouldn't have been for Siarhei to point me
in the right direction I'd have spend even more time to find a fix.

This whole BSS in DRAM construct is very fragile and not obvious to
blame for any bug that you happen to see. For instance we were assuming
it to be due to unreliable SRAM C before.
Also we *do* a printf very early, which works because it doesn't involve
any numbers.
And really I think it's quite a stretch to ask the casual programmer to
keep in mind that some variables may end up in BSS and this must not
happen before DRAM is initialised.

So either we find a way to make this break if BSS variables are involved
in functions called before DRAM init (by somehow annotating files that
are used before or during DRAM init).
Or we find other spaces for the BSS in other SRAMs.
Like I described here:
http://lists.denx.de/pipermail/u-boot/2016-June/259392.html

Cheers,
Andre.

> Have you tried looking into fixing this at the tinyprintf level ?
> 
> Regards,
> 
> Hans
> 
>>
>> As it turns out the normal BSS size for sunxi is about 256 Bytes, so we
>> can happily remove the symbols and the linker script part that was
>> forcing the section into DRAM and let the linker naturally put it into
>> SRAM.
>> Tested on BananaPi M1 and Pine64(-SPL), also buildman sunxi was happy.
>>
>> Thanks to Siarhei for providing helpful hints!
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arch/arm/cpu/armv7/sunxi/u-boot-spl.lds | 4 +---
>>  include/configs/sunxi-common.h          | 4 ----
>>  2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> index 53f0cbd..a90404f 100644
>> --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds
>> @@ -16,8 +16,6 @@
>>   */
>>  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 }
>>
>>  OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
>>  OUTPUT_ARCH(arm)
>> @@ -54,5 +52,5 @@ SECTIONS
>>          *(.bss*)
>>          . = ALIGN(4);
>>          __bss_end = .;
>> -    } > .sdram
>> +    } > .sram
>>  }
>> diff --git a/include/configs/sunxi-common.h
>> b/include/configs/sunxi-common.h
>> index 94275a7..e3fe965 100644
>> --- a/include/configs/sunxi-common.h
>> +++ b/include/configs/sunxi-common.h
>> @@ -75,7 +75,6 @@
>>   * since it needs to fit in with the other values. By also #defining it
>>   * we get warnings if the Kconfig value mismatches. */
>>  #define CONFIG_SPL_STACK_R_ADDR        0x2fe00000
>> -#define CONFIG_SPL_BSS_START_ADDR    0x2ff80000
>>  #else
>>  #define SDRAM_OFFSET(x) 0x4##x
>>  #define CONFIG_SYS_SDRAM_BASE        0x40000000
>> @@ -86,11 +85,8 @@
>>   * since it needs to fit in with the other values. By also #defining it
>>   * we get warnings if the Kconfig value mismatches. */
>>  #define CONFIG_SPL_STACK_R_ADDR        0x4fe00000
>> -#define CONFIG_SPL_BSS_START_ADDR    0x4ff80000
>>  #endif
>>
>> -#define CONFIG_SPL_BSS_MAX_SIZE        0x00080000 /* 512 KiB */
>> -
>>  #if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I)
>>  /*
>>   * The A80's A1 sram starts at 0x00010000 rather then at 0x00000000
>> and is
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-07-03 22:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30  0:29 [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM Andre Przywara
2016-06-30  1:54 ` Marek Vasut
2016-06-30  8:50   ` Andre Przywara
     [not found] <20160630002500.2817-1-andre.przywara@arm.com>
     [not found] ` <af86054b-8fcc-3b58-6889-7af02cd7a9fc@redhat.com>
2016-06-30 15:43   ` Andre Przywara
2016-06-30 17:40     ` Peter Korsgaard
2016-06-30 21:09       ` Marek Vasut
     [not found]   ` <CAPnjgZ2ajoU90kSNMoHVhbXN4CdeKzs5-SQ-Hq-xePfC0brJ3A@mail.gmail.com>
2016-07-02 11:49     ` Hans de Goede
2016-07-03 21:15       ` Simon Glass
2016-07-03 22:24       ` André Przywara

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.