From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andre Przywara <andre.przywara@arm.com>
Date: Thu, 30 Jun 2016 16:43:58 +0100
Subject: [U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM
In-Reply-To: <af86054b-8fcc-3b58-6889-7af02cd7a9fc@redhat.com>
References: <20160630002500.2817-1-andre.przywara@arm.com>
 <af86054b-8fcc-3b58-6889-7af02cd7a9fc@redhat.com>
Message-ID: <7306b594-ad75-8efe-83b6-dc7b2fea5cc4@arm.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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
>>
>