[U-Boot] [PATCH] SPL: sunxi: don't force .BSS into DRAM

[André Przywara]

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