[U-Boot] [Patch v3 1/2] common/board_f: Preserve early global data

[Simon Glass]

+Albert

Hi,

On 1 May 2014 15:41, Scott Wood <scottwood at freescale.com> wrote:
> On Thu, 2014-05-01 at 14:29 -0700, York Sun wrote:
>> On 05/01/2014 02:21 PM, Scott Wood wrote:
>> > On Thu, 2014-05-01 at 14:16 -0700, York Sun wrote:
>> >> On 05/01/2014 08:54 AM, Simon Glass wrote:
>> >>> Hi York,
>> >>>
>> >>> On 30 April 2014 18:01, York Sun <yorksun at freescale.com> wrote:
>> >>>> Some platforms (including mpc85xx, mpc86xx) use global data before calling
>> >>>> function baord_inti_f(). This data should not be cleared later. Any arch
>> >>>> which needs to preserve global data should define CONFIG_SYS_EARLY_GD.
>> >>>>
>> >>>> Signed-off-by: York Sun <yorksun at freescale.com>
>> >>>> CC: Scott Wood <scottwood at freescale.com>
>> >>>> CC: Simon Glass <sjg at chromium.org>
>> >>>> ---
>> >>>> Change log
>> >>>>
>> >>>>  v3: Introduce CONFIG_SYS_EARLY_GD instead of using a list
>> >>>>  v2: Instead of adding back gd init for all PPC, preserve gd for mpc85xx and mpc86xx.
>> >>>>
>> >>>>  README                            |    3 +++
>> >>>>  arch/powerpc/include/asm/config.h |    2 ++
>> >>>>  arch/x86/include/asm/config.h     |    1 +
>> >>>>  common/board_f.c                  |   10 +++++-----
>> >>>>  4 files changed, 11 insertions(+), 5 deletions(-)
>> >>>>
>> >>>> diff --git a/README b/README
>> >>>> index 12758dc..1a51b11 100644
>> >>>> --- a/README
>> >>>> +++ b/README
>> >>>> @@ -440,6 +440,9 @@ The following options need to be configured:
>> >>>>                 supported, core will start to execute uboot when wakes up.
>> >>>>
>> >>>>  - Generic CPU options:
>> >>>> +               CONFIG_SYS_EARLY_GD
>> >>>> +               Defines global data is used before board_init_f().
>> >>>> +
>> >>>
>> >>> This looks like a good solution, but I believe that 'early' is in fact
>> >>> where we are headed. So perhaps you should create an inverted option
>> >>> and define it for archs that don't support it?
>> >>
>> >> I don't disagree for the long term. But since we just start to convert to
>> >> generic board, fewer archs need this than those don't. I suggest we try this and
>> >> when most archs convert to generic board, we can invert it if needed.
>> >
>> > Why wait to do it right?  Which arches actually don't need this, given
>> > that Simon said he thinks ARM needs it as well?

Thanks for working on this, York.

I looked at ARM and I think it is similar. See arch/arm/lib/crt0.S. It
sets up gd - the only problem is that it does not zero the area
unfortunately. But once that is added we can switch ARM over.

>> >
>>
>> Adding something I can't test?
>
> By that rationale nobody could ever make changes that aren't localized
> to a few boards.  That type of development produces the sort of mess
> that the generic board is meant to remedy.
>
>> Even Simon said he thinks ARM needs it, it is not what current code has. If I
>> revert this logic,
>>
>> #ifdef CONFIG_SYS_GLOBAL_DATA_STACK
>
> This looks like a global stack of data, not global data on the stack.
>
> How about CONFIG_SYS_GENERIC_GLOBAL_DATA?
>
>>        gd_dt data;
>>
>>        gd = &data;
>>        zero_global_data();
>> #endif
>>
>> I need to add #define CONFIG_SYS_GLOBAL_DATA_STACK to the board/arch who needs
>> it. I have no idea which needs in the 70+ boards with CONFIG_DISPLAY_BOARDINFO
>> defined.
>
> I see only 10 boards that currently use CONFIG_SYS_GENERIC_BOARD in
> include/configs, plus sandbox and x86 which hide it in config.mk.  I
> don't know what CONFIG_DISPLAY_BOARDINFO has to do with it.  To invert
> the symbol, add the new symbol to all of the current users.  Removal
> from ARM boards can come afterward if it's confirmed that it's the right
> thing to do.

SGTM.

Regards,
Simon