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

login
register
mail settings
Submitter Simon Glass
Date March 15, 2012, 2:16 a.m.
Message ID <1331777784-8528-8-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/146798/
State Changes Requested, archived
Headers show

Comments

Simon Glass - March 15, 2012, 2:16 a.m.
We want to unify the global_data structure. Most fields are common across
architectures, but there are a fair number of SOC-specific additions. It
isn't clear how best to deal with these, but for now we just use #ifdef.

Checkpatch warnings here might be unavoidable:

warning: include/asm-generic/global_data.h,43: do not add new typedefs
warning: include/asm-generic/global_data.h,117: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
warning: include/asm-generic/global_data.h,121: storage class should be at the beginning of the declaration

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/asm-generic/global_data.h |  115 +++++++++++++++++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-generic/global_data.h
Graeme Russ - March 15, 2012, 2:30 a.m.
Hi Simon,

On Thu, Mar 15, 2012 at 1:16 PM, Simon Glass <sjg@chromium.org> wrote:
> We want to unify the global_data structure. Most fields are common across
> architectures, but there are a fair number of SOC-specific additions. It
> isn't clear how best to deal with these, but for now we just use #ifdef.
>
> Checkpatch warnings here might be unavoidable:
>
> warning: include/asm-generic/global_data.h,43: do not add new typedefs
> warning: include/asm-generic/global_data.h,117: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> warning: include/asm-generic/global_data.h,121: storage class should be at the beginning of the declaration

IMHO global data does not need to be (and should not be) typedef'd

Regards,

Graeme
Graeme Russ - March 15, 2012, 2:35 a.m.
Hi Simon,

On Thu, Mar 15, 2012 at 1:16 PM, Simon Glass <sjg@chromium.org> wrote:
> We want to unify the global_data structure. Most fields are common across
> architectures, but there are a fair number of SOC-specific additions. It
> isn't clear how best to deal with these, but for now we just use #ifdef.
>
> Checkpatch warnings here might be unavoidable:
>
> warning: include/asm-generic/global_data.h,43: do not add new typedefs
> warning: include/asm-generic/global_data.h,117: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
> warning: include/asm-generic/global_data.h,121: storage class should be at the beginning of the declaration
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>

> +#ifdef CONFIG_AT91FAMILY
> +       /* "static data" needed by at91's clock.c */
> +       unsigned long   cpu_clk_rate_hz;
> +       unsigned long   main_clk_rate_hz;
> +       unsigned long   mck_rate_hz;
> +       unsigned long   plla_rate_hz;
> +       unsigned long   pllb_rate_hz;
> +       unsigned long   at91_pllb_usb_init;
> +#endif
> +#ifdef CONFIG_ARM
> +       /* "static data" needed by most of timer.c on ARM platforms */
> +       unsigned long   timer_rate_hz;
> +       unsigned long   tbl;
> +       unsigned long   tbu;
> +       unsigned long long      timer_reset_value;
> +       unsigned long   lastinc;
> +#endif

IMHO, global data should contain only globally common members and an arch-
specific struct and ditch (most of) the #ifdefs

Regards,

Graeme
Simon Glass - March 15, 2012, 2:50 a.m.
Hi Graeme,

On Wed, Mar 14, 2012 at 7:35 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Thu, Mar 15, 2012 at 1:16 PM, Simon Glass <sjg@chromium.org> wrote:
>> We want to unify the global_data structure. Most fields are common across
>> architectures, but there are a fair number of SOC-specific additions. It
>> isn't clear how best to deal with these, but for now we just use #ifdef.
>>
>> Checkpatch warnings here might be unavoidable:
>>
>> warning: include/asm-generic/global_data.h,43: do not add new typedefs
>> warning: include/asm-generic/global_data.h,117: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
>> warning: include/asm-generic/global_data.h,121: storage class should be at the beginning of the declaration
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>
>> +#ifdef CONFIG_AT91FAMILY
>> +       /* "static data" needed by at91's clock.c */
>> +       unsigned long   cpu_clk_rate_hz;
>> +       unsigned long   main_clk_rate_hz;
>> +       unsigned long   mck_rate_hz;
>> +       unsigned long   plla_rate_hz;
>> +       unsigned long   pllb_rate_hz;
>> +       unsigned long   at91_pllb_usb_init;
>> +#endif
>> +#ifdef CONFIG_ARM
>> +       /* "static data" needed by most of timer.c on ARM platforms */
>> +       unsigned long   timer_rate_hz;
>> +       unsigned long   tbl;
>> +       unsigned long   tbu;
>> +       unsigned long long      timer_reset_value;
>> +       unsigned long   lastinc;
>> +#endif
>
> 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?

Regards,
Simon

>
> Regards,
>
> Graeme
Graeme Russ - March 15, 2012, 3:02 a.m.
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

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)" ;)

There will always be arch specific global data members - I see a few
options:

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

Regards,

Graeme
Simon Glass - March 15, 2012, 3:41 a.m.
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.

Regards,
Simon

>
> Regards,
>
> Graeme
Simon Glass - March 24, 2012, 6:40 a.m.
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>
};

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)

(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?)

Regards,
Simon
Graeme Russ - March 24, 2012, 11:14 a.m.
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
Simon Glass - March 24, 2012, 11:10 p.m.
Hi Graeme,

On Sat, Mar 24, 2012 at 4:14 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
> 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 ;)

You surprise me :-)

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

Separate issue, but I can't see why not.

But I now remember the main problem I had with this.
CONFIG_SYS_GENERIC_BOARD is an option, not mandatory, so we must not
change the C code (much, ideally not at all) to make this work. If we
move things around as you suggest then we should probably do it first
as part of the normal code, so that generic board can just use it.

So the first change might be to move arch-specific data into an agd
structure, and get everything building again. Then finally pull out
the common part of each file into a new asm-generic file. It's the
opposite way to what I have done so far, but the result will be the
same in the end. I suppose my only reservation is that it creates
patches initially for little benefit. I will have to think about how
to motivate it.

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

Probably, if we went into a deep enough. Certainly there are members
which are only used for one sub-archs, and some of those sub-archs
have a very small number of boards, perhaps 1 in some cases.

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

Yes, but I'm just saying that is a long list, and involves a lot of
code searching.

>
> Regards,
>
> Graeme
>

Regards,
Simon
Graeme Russ - March 24, 2012, 11:54 p.m.
Hi Simon,

On 03/25/2012 10:10 AM, Simon Glass wrote:
> Hi Graeme,
> 
> On Sat, Mar 24, 2012 at 4:14 AM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Simon,
>>

[snip]

>>
>> 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?
> 
> Separate issue, but I can't see why not.

OK - See below

> But I now remember the main problem I had with this.
> CONFIG_SYS_GENERIC_BOARD is an option, not mandatory, so we must not
> change the C code (much, ideally not at all) to make this work. If we
> move things around as you suggest then we should probably do it first
> as part of the normal code, so that generic board can just use it.

Sounds like a solid plan

> So the first change might be to move arch-specific data into an agd
> structure, and get everything building again. Then finally pull out
> the common part of each file into a new asm-generic file. It's the
> opposite way to what I have done so far, but the result will be the
> same in the end. I suppose my only reservation is that it creates
> patches initially for little benefit. I will have to think about how
> to motivate it.

Clean-up work that, on the surface, appears to not do much does much more
than you think. It may highlight a few nasty quibbles that you hadn't
expected that you will need to plan around.

Big changes made against good, clean, consistent code are always easier
than against bad, dirty, inconsistent code :)

And I think this is the ideal opportunity to drop the typedef

>> Q: Are there any board specific global data members anywhere?
> 
> Probably, if we went into a deep enough. Certainly there are members
> which are only used for one sub-archs, and some of those sub-archs
> have a very small number of boards, perhaps 1 in some cases.

OK, it might be worth investigating HAS_BOARD_GD. Maybe bring that through
struct arch_global_data but it's not as clear an issue (there is a symlink
to include/arch/asm but none for include/board/. For the time being, we may
just have to leave the ifdefs in the include/arch/asm/global_data.h

>>> (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
> 
> Yes, but I'm just saying that is a long list, and involves a lot of
> code searching.

I didn't say it would be easy ;)

The board_init_f array is a starting point.

I was planning on moving pre-relocation functions into a .init section so
they were not copied during code relocation (much like Linux marks init
functions so the memory can be free'd post init). Once this is done,
identifying 'real' global data members will be trivial

Regards,

Graeme

Patch

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
new file mode 100644
index 0000000..6199926
--- /dev/null
+++ b/include/asm-generic/global_data.h
@@ -0,0 +1,115 @@ 
+/*
+ * (C) Copyright 2002-2010
+ * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef	__ASM_GENERIC_GBL_DATA_H
+#define __ASM_GENERIC_GBL_DATA_H
+/*
+ * The following data structure is placed in some memory which is
+ * available very early after boot (like DPRAM on MPC8xx/MPC82xx, or
+ * some locked parts of the data cache) to allow for a minimum set of
+ * global variables during system initialization (until we have set
+ * up the memory controller so that we can use RAM).
+ *
+ * Keep it *SMALL* and remember to set GENERATED_GBL_DATA_SIZE > sizeof(gd_t)
+ *
+ * sjg@chromium.org: Well it would be nice to have a generic one of these
+ * since so many fields are similar. But it means that everyone architecture
+ * will want to add its own nutty fields. Perhaps that is no bad thing since
+ * it shows up inconsistences and might produce downward pressure on the
+ * number of fields.
+ */
+
+#ifndef __ASSEMBLY__
+typedef struct global_data {
+	bd_t		*bd;
+	unsigned long	flags;
+	unsigned long	baudrate;
+#if defined(CONFIG_LCD) || defined(CONFIG_VIDEO)
+	unsigned long	fb_base;	/* Base address of framebuffer mem */
+#endif
+#if defined(CONFIG_POST) || defined(CONFIG_LOGBUFFER)
+	unsigned long	post_log_word;  /* Record POST activities */
+	unsigned long	post_log_res; /* success of POST test */
+	unsigned long	post_init_f_time;  /* When post_init_f started */
+#endif
+	unsigned long	have_console;	/* serial_init() was called */
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
+	unsigned long	precon_buf_idx;	/* Pre-Console buffer index */
+#endif
+	unsigned long	env_addr;	/* Address  of Environment struct */
+	unsigned long	env_valid;	/* Checksum of Environment valid? */
+	/* Here begins ARM-specific things. Needs discussion */
+#ifdef CONFIG_AT91FAMILY
+	/* "static data" needed by at91's clock.c */
+	unsigned long	cpu_clk_rate_hz;
+	unsigned long	main_clk_rate_hz;
+	unsigned long	mck_rate_hz;
+	unsigned long	plla_rate_hz;
+	unsigned long	pllb_rate_hz;
+	unsigned long	at91_pllb_usb_init;
+#endif
+#ifdef CONFIG_ARM
+	/* "static data" needed by most of timer.c on ARM platforms */
+	unsigned long	timer_rate_hz;
+	unsigned long	tbl;
+	unsigned long	tbu;
+	unsigned long long	timer_reset_value;
+	unsigned long	lastinc;
+#endif
+#ifdef CONFIG_IXP425
+	unsigned long	timestamp;
+#endif
+	/* TODO: is this the same as relocaddr, or something else? */
+	unsigned long	dest_addr;	/* Post-relocation address of U-Boot */
+	unsigned long	dest_addr_sp;
+	unsigned long	ram_top;	/* Top address of RAM used by U-Boot */
+
+	unsigned long	relocaddr;	/* Start address of U-Boot in RAM */
+	phys_size_t	ram_size;	/* RAM size */
+	unsigned long	mon_len;	/* monitor len */
+	unsigned long	irq_sp;		/* irq stack pointer */
+	unsigned long	start_addr_sp;	/* start_addr_stackpointer */
+	unsigned long	reloc_off;
+#if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
+	unsigned long	tlb_addr;
+#endif
+	struct global_data *new_gd;	/* relocated global data */
+	const void	*fdt_blob;	/* Our device tree, NULL if none */
+	void		**jt;		/* jump table */
+	char		env_buf[32];	/* buffer for getenv() before reloc. */
+} gd_t;
+#endif
+
+/*
+ * Global Data Flags
+ */
+#define	GD_FLG_RELOC		0x00001	/* Code was relocated to RAM	   */
+#define	GD_FLG_DEVINIT		0x00002	/* Devices have been initialized   */
+#define	GD_FLG_SILENT		0x00004	/* Silent mode			   */
+#define	GD_FLG_POSTFAIL		0x00008	/* Critical POST test failed	   */
+#define	GD_FLG_POSTSTOP		0x00010	/* POST seqeunce aborted	   */
+#define	GD_FLG_LOGINIT		0x00020	/* Log Buffer has been initialized */
+#define GD_FLG_DISABLE_CONSOLE	0x00040	/* Disable console (in & out)	   */
+#define GD_FLG_ENV_READY	0x00080	/* Env. imported into hash table   */
+
+#endif /* __ASM_GENERIC_GBL_DATA_H */