[U-Boot] [PATCH v4 07/27] Introduce generic global_data

[Graeme Russ <graeme.russ@gmail.com>]

Hi Simon,

On 03/24/2012 05:40 PM, Simon Glass wrote:
> Hi Graeme,
> 
> On Wed, Mar 14, 2012 at 8:41 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Graeme,
>>
>> On Wed, Mar 14, 2012 at 8:02 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Thu, Mar 15, 2012 at 1:50 PM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Graeme,
>>>>
>>>> On Wed, Mar 14, 2012 at 7:35 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>
>>> [snip]
>>>
>>>>>
>>>>> IMHO, global data should contain only globally common members and an arch-
>>>>> specific struct and ditch (most of) the #ifdefs
>>>>
>>>> My thinking here was to try to bring everything into a single file. It
>>>> then should be clearer when things are common between different
>>>> architectures. Patches to the generic file can be made without also
>>>> having to patch the non-generic files, etc.
>>>>
>>>> A fair number of the #ifdefs are not needed, or are there because some
>>>> archs don't implement all the features of U-Boot.
>>>>
>>>> You have an example right there: cpu_clk_rate_hz is similar to cpu_clk
>>>> and core_clk.
>>>>
>>>> That said it is a bit of a daunting task to amalgamate them.
>>>>
>>>> Also there is the purely practical consideration that if we continue
>>>> to have an asm/global_data.h then we end up with two global datas. One
>>>> is for CONFIG_SYS_GENERIC_BOARD and contains just the non-common
>>>> fields. The other is for non-CONFIG_SYS_GENERIC_BOARD and contains all
>>>> fields. Ick.
>>>>
>>>> So what do you think?
>>>
>>> Do you really need to unify global data to achieve what the title of the
>>> patch series suggests (i.e. to unify the init processing loop)? Maybe you
>>> could leave global data as is (or slightly tweak the odd arch) and leave
>>> the resolution of just how bad global data is becoming for another day
>>
>> It's not that easy, because in board_f.c and board_r.c and other files
>> there are certain fields required. It doesn't make a huge amount of
>> sense to have generic code which accesses a different global structure
>> depending on what architecture it is built for. Then there are fields
>> are are only used when certain options are defined. Ick.
>>
>> If I am going to pull this off I need a bit of flexibility. I've
>> looked into this quite a bit and mapped a path through this which I
>> think will work. It requires doing things in stages, or it will never
>> happen.
>>
>>>
>>> I only say this because this is turning into "let's do a dirty hack now to
>>> partially fix it and leave the rest for later (it'll get done, really,
>>> honestly, I promise)" ;)
>>
>> It was always like that. Although I wouldn't characterise it as a
>> dirty hack. If there was a requirement to do all of this in one big
>> bang then I wouldn't have even started. It is just too hard :-(
>>
>>>
>>> There will always be arch specific global data members - I see a few
>>> options:
>>>
>>>  - Move them into bd
>>
>> I recall talk of getting rid of this (Mike?)
>>
>>>  - Move them into an arch_global_data struct inside gd
>>
>> This was Mike's idea. It is probably the easiest thing to do.
>>
>>>  - Move them into an arch_global_data struct totally seperate from gd
>>
>> I sort-of like this except it would slow down access and maybe
>> increase code size. Then again perhaps that's a good thing if it
>> provides an incentive to reduce the number of arch-specific fields.
>>
>>>  - Question how many are really required to be in gd (remember, gd is
>>>   only there to cart around writable global variable before .bss and
>>>   .data are available after relocation)
>>
>> Well yes, I feel there are far more at present than are needed. Having
>> them all there in the open feels like a nice way to draw attention to
>> the mess.
> 
> Any more comments on this thread? At this stage I am still not sure of
> the best approach for this header...none of the options is
> particularly attractive. I can imagine something horrible like:
> 
> struct global_data {
>    <common fields>
>    ...
> #include <asm/arch_global_data.h>
> };

Ick! - NAK NAK NAK NAK ;)

> 
> which would be the smallest code change (essentially no accesses would
> need to change). But it is too awful.
> 
> Of course I have a generic bd structure now, so moving things into
> there doesn't fix the problem.
> 
> So if I rewind back to your first suggestion (just leave global-data
> and bd as they are now), then I have the problem that I need to add
> quite a bit of stuff to these structures for every architecture. This
> is because at present most architectures don't support all the
> features, and so don't need all the fields. As soon as I have generic
> C code, I have references in that C code to global data members that
> only exist for some architectures. Then someone enables CONFIG_SPI,
> and breaks the board on the architecture that didn't previously have
> SPI support. I wonder if that matters?
> 
> It sort-off seems attractive from the 'less work' point of view, and
> is a stepping stone along the way, though.
> 
> Just to repeat your other ideas:
> 
>>  - Move them into bd
>>  - Move them into an arch_global_data struct inside gd
>>  - Move them into an arch_global_data struct totally seperate from gd
>>  - Question how many are really required to be in gd (remember, gd is
>>   only there to cart around writable global variable before .bss and
>>   .data are available after relocation)

IIRC the Linux driver model includes a pointer to non-generic driver
specific data which only the driver knows what to do with (the framework
ignores it)
If struct global_data has struct arch_global_data *agd in gd - Means a lot
of changing of accessors and maybe a little code increase and slight access
penalty (and maybe changes to asm startup code to handle the creation of gd
plus agd and setting the pointer)

But that would be way cleaner for stuct global_data

Having agd in gd is probably a lot easier as the auto-generated gd size
will be good to use

Hmmm..

arch/asm/global_data.h:

#define HAS_ARCH_GD

struct arch_global_data {
	...
};

include/global_data.h:


#include <arch/asm/global_data.h>

struct global_data {
	...
#ifdef HAS_ARCH_GD
	struct arch_global_data agd;
#endif
};

I like this better

P.S. Can we ditch the typedef while we are at it?

Q: Are there any board specific global data members anywhere?

> 
> (I feel the last one has to come later, though, even if unfortunately
> it would simplify things now - how on earth are we going to work out
> what things are really needed in global data?)

Look at the functions which are called prior to relocation - Anything
referenced in these functions needs to be in gd

Regards,

Graeme