Patchwork [U-Boot,RFC] New init sequence processing without init_sequence array

login
register
mail settings
Submitter Graeme Russ
Date Aug. 17, 2011, 1:22 p.m.
Message ID <1313587343-3693-1-git-send-email-graeme.russ@gmail.com>
Download mbox | patch
Permalink /patch/110316/
State Superseded
Headers show

Comments

Graeme Russ - Aug. 17, 2011, 1:22 p.m.
I have been thinking about the problem of the pesky init_sequence arrays
and the inevitable #ifdefs and empty stub functions that result so I
thought I'de have a crack at a more dynamic implementation. And like all
good programmers, I stole the solution ;). This implementation is based
on Linux's __initcall(fn) et. al. macros

If this works cross-platform, we can finally move board_init_* into
/lib/ - Wouldn't that be nice

Thoughts?

Regards,

Graeme

P.S Compile tested on x86 gcc 4.5.2 only - haven't fired it up yet
---
 arch/x86/cpu/sc520/sc520.c       |    1 +
 arch/x86/cpu/sc520/sc520_sdram.c |    1 +
 arch/x86/cpu/u-boot.lds          |    5 +++++
 arch/x86/lib/board.c             |   36 +++++++++++++++---------------------
 board/eNET/eNET.c                |    1 +
 common/console.c                 |    1 +
 common/env_flash.c               |    2 ++
 common/serial.c                  |    1 +
 include/common.h                 |    8 ++++++++
 9 files changed, 35 insertions(+), 21 deletions(-)

--
1.7.5.2.317.g391b14
Wolfgang Denk - Aug. 22, 2011, 8:10 p.m.
Dear Graeme Russ,

In message <1313587343-3693-1-git-send-email-graeme.russ@gmail.com> you wrote:
> I have been thinking about the problem of the pesky init_sequence arrays
> and the inevitable #ifdefs and empty stub functions that result so I
> thought I'de have a crack at a more dynamic implementation. And like all
> good programmers, I stole the solution ;). This implementation is based
> on Linux's __initcall(fn) et. al. macros
> 
> If this works cross-platform, we can finally move board_init_* into
> /lib/ - Wouldn't that be nice
> 
> Thoughts?

My initial thoughts are these two:

1. I think we should change the code in a different order.  I would
   prefer to first unify the init code across architectures (the big
   ARM reorganization we just seem to have overcome was an important
   prerequisite for this), before we start changing the init code.

2. One of the advantages of the current implementation is that there
   is a central place in the code (well, at least per architecture,
   untill we unify these as mentioned above) where you can see exactly
   which steps get performed in which sequence.

   I understand (and appreciate) your intentions, does the initcall
   concept not mean that we will have some sort of sequence numbers
   distributed all over the code?  Maybe I'm mising something - but
   does this not make it really difficult to actually see (from the
   source code) what the exact init sequence is?

3. Hardware initialization in inherently very much hardware depen-
   dent ;-)  We have some boards where PCI must be initialized early
   because it is needed to access some other peripherals like memory.
   And we have other boards where PCI can only be initialized late
   because it depends on a lot of other functions that must be working
   first.

   I explained this a number of times before: the current code was
   designed to allow even for completely board specific init
   sequences, by simply adding #define for the list of init functions
   to the board config file - see for example here:

   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33951/focus=36436

   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136

   If you look at the current code - heavily larded with #ifdefs of
   all shapes and colors - I cannot see any good way to transform this
   into an initcall (and thus sequence number based) implementation.
   Do you have any specific ideas how to address this?

Best regards,

Wolfgang Denk
Graeme Russ - Aug. 22, 2011, 11:20 p.m.
Hi Wolfgang,

On Tue, Aug 23, 2011 at 6:10 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <1313587343-3693-1-git-send-email-graeme.russ@gmail.com> you wrote:
>> I have been thinking about the problem of the pesky init_sequence arrays
>> and the inevitable #ifdefs and empty stub functions that result so I
>> thought I'de have a crack at a more dynamic implementation. And like all
>> good programmers, I stole the solution ;). This implementation is based
>> on Linux's __initcall(fn) et. al. macros
>>
>> If this works cross-platform, we can finally move board_init_* into
>> /lib/ - Wouldn't that be nice
>>
>> Thoughts?
>
> My initial thoughts are these two:
>
> 1. I think we should change the code in a different order.  I would
>   prefer to first unify the init code across architectures (the big
>   ARM reorganization we just seem to have overcome was an important
>   prerequisite for this), before we start changing the init code.
>

I agree - I am currently auditing the init sequences and teasing out any
common ordering and identifying where arches call an otherwise common
init function out-of-order to the others. This is, generally speaking,
very ugly - There are:

 - Two archictectures that do not use the init loop
 - Two that do not relocate and therefore have a mix of (what other arches
   consider) _f and _r int functions in a single loop
 - Functions that could be made init functions after the init loop in
   board_init_r
 - Some arches have an equivalent init_f loop, others have an init_r loop
   x86 is the only one that does both _f anf _r init sequences as loops

I think, as a first step, all arches need to implement an init_f and
init_r loop and collapse board_init_f and board_init_r into trival
loop processing functions - Combine them into one function (which takes
a pointer to the relavent init_function array) and move it into
/lib/init.c - Easy ;)

> 2. One of the advantages of the current implementation is that there
>   is a central place in the code (well, at least per architecture,
>   untill we unify these as mentioned above) where you can see exactly
>   which steps get performed in which sequence.

This new method can have the same by putting all the INIT_FUNC()
in a single location, much like the existing array. I don't think this
is a good idea though. I prefer good clear documentation.

>   I understand (and appreciate) your intentions, does the initcall
>   concept not mean that we will have some sort of sequence numbers
>   distributed all over the code?  Maybe I'm mising something - but
>   does this not make it really difficult to actually see (from the
>   source code) what the exact init sequence is?

Well the bulk of the init sequence will be defined by init steps clearly
defined in include/init.h. Where there is as arch, SoC or board which
needs an init function called immediately after a particular point in the
standard sequence, then there will be:

	#define CUSTOM_INIT_STEP	STANDARD_INIT_STEP + 1

> 3. Hardware initialization in inherently very much hardware depen-
>   dent ;-)  We have some boards where PCI must be initialized early
>   because it is needed to access some other peripherals like memory.
>   And we have other boards where PCI can only be initialized late
>   because it depends on a lot of other functions that must be working
>   first.

Yes, I am seeing that in my audit

>   I explained this a number of times before: the current code was
>   designed to allow even for completely board specific init
>   sequences, by simply adding #define for the list of init functions
>   to the board config file - see for example here:
>
>   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33951/focus=36436
>
>   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136

Yes, I saw some of the macro'd init functions in my audit, and I must say
that I am not a fan. If we continue down this path, we will have an init
sequence array made up of purely of macros and then it will be a PITA to
search throught the code resolving the macro for your particular arch /
Soc / board. Also, it does not allow the functions to be put in an
arbitrary order in a central location - Do do so will require a really bad
mess of #ifdef's - This will be a big barrier to centralising the whole
init sequence in say /lib/init.c as each arch will still need to put the
critical init function array in /arch/lib/board.c

>
>   If you look at the current code - heavily larded with #ifdefs of
>   all shapes and colors - I cannot see any good way to transform this
>   into an initcall (and thus sequence number based) implementation.
>   Do you have any specific ideas how to address this?

The _really_ nice thing about initcall is that if you take something like
PCI, you put the INIT_FUNC() in pci.c and it only gets included if you
define CONFIG_PCI - No #ifdefs. The problem, as you quite rightly point
out, is how to clearly define INIT_STEP_PCI. From what I am seeing, these
are mostly corner cases that can be clearly documented.

Now to answer some points in your other email:

> >  For a board to add an arbitrary initialisation function is trivial -
> >  simply add a INIT_FUNC() entry with an appropriate sequence number
>
> This is actually hte biffest voncern I have: I cannot imagine how you
> will try to figure out the exact init sequence when writing the code
> (or, even worse, when reading somebody else's code).

Good point - The init sequence audit I am doing now will help. It will
become a lot clearer when the init sequences are unified. For the rest,
good clear documentation will help, but I think we will only know how it
looks and feels when a complete patchs hits the list.

One nicity of putting INIT_FUNC() after a function body (or just before
it which is easier to see if the body is longer that a screen) is that
you can see immediately that the function is involved in the init
sequence - I kind of like that.

> >  I imagine the sequence numbers could be #defined, but I don't exactly
> >  now what the linker will do - I use leading zeros in the sequence numbers
> >  to ensure correct ordering.
>
> This is another concern: what exactly will the linker do, and
> different versions of linkers with differen options and/or levels of
> optimization?

I have another patch now that #define's the step numbers. As long as the
step numbers are 100-999 (or 1000-9999, 10000-99999 etc) then everything
works fine. The linker MUST support SORT_BY_NAME, KEEP and DISCARD - can
anyone think of a linker that does not? Optimization will only be an issue
if the linker disobeys SORT_BY_NAME, KEEP or DISCARD as part of the
optimisation (which I would think would be brain-dead as these directives
are clearly telling the linker to do something very particular)

OK, so what do I see as adventageous about using initcall versus current
implementation...

 - Any arch, SoC or board can cleanly inject an init function anywhere in
   the init sequence without touching a single line of code outside that
   arch/SoC/board. The current system requires a mod to /arch/lib/board.c
   with the potential addittion of ugly #ifdef's
 - The _f init function pointers are put in a section outside of the data
   copied to RAM during relocation, slightly reducing the RAM footprint
 - No more #ifdef in the init sequence array
 - sub-systems (PCI, IDE, PCMCIA, SCSI) define the init function in their
   respective .c files and it gets pulled in when the Makefile pulls in
   the .c file
 - It's more like Linux :)

And the bad...
 - Init sequence not clearly obvious in a single location
 - Maybe brain-dead linkers will do something wrong

And the neutral
 - Dealing with specific arch/Soc/board requiring an init function at a
   different sequence step than everything else

Some neat features that are trivial to implement with initcall and
difficult or ugly with init array:
 - The names of the function can be (conditionally by DEBUG) added and
   printed out as the init sequence is being run - Very nice for debugging
   the init sequence
 - Boot profiling - Store the step number and time to execute then after
   bootup completes, a simple 'show boot profile information' can print
   each function name and the time taken to execute

Honestly, after having a good play with this and fully converting x86 to
trivial board_init_f and board_init_r functions, I really like the initcall
method - It really does 'just work' like a dream :)

Regards,

Graeme
Graeme Russ - Aug. 23, 2011, 12:17 a.m.
Hi Wolfgang,

On Tue, Aug 23, 2011 at 6:10 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <1313587343-3693-1-git-send-email-graeme.russ@gmail.com> you wrote:
>> I have been thinking about the problem of the pesky init_sequence arrays
>> and the inevitable #ifdefs and empty stub functions that result so I
>> thought I'de have a crack at a more dynamic implementation. And like all
>> good programmers, I stole the solution ;). This implementation is based
>> on Linux's __initcall(fn) et. al. macros
>>
>> If this works cross-platform, we can finally move board_init_* into
>> /lib/ - Wouldn't that be nice
>>
>> Thoughts?

>   I explained this a number of times before: the current code was
>   designed to allow even for completely board specific init
>   sequences, by simply adding #define for the list of init functions
>   to the board config file - see for example here:
>
>   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33951/focus=36436
>
>   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136

Hmm, this last thread includes this little gem:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136

<quote>
Somewhat offtopic, but you could add a few weak empty dummy functions at
strategic places in the board_X funcs. Any board that needs some extra
init sequence could define the appropriate function which will replace
the weak one.
</quote>

There is an incling that there may be a requirement to have more
flexibility in the init sequence at the board level. Your response here:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136

<quote>
The idea is that boards that want such contrrol can redefine the whole
init sequence list - adding what they really need, and omitting what they
don't. Zero overhead.
</quote>

I would agree with Detlev - For a board to redefine the entire init
sequence just to inject a single init function seems like gross overkill.
Of course, this has already been realised and the solution was to #ifdef
the init sequence array itself.

I think we are starting to see that the init array methodology is getting
a little restrictive and in order to break free, some rather unsavoury
coding behaviour has started to creep in

Regards,

Graeme
Mike Frysinger - Aug. 23, 2011, 1 a.m.
On Monday, August 22, 2011 16:10:23 Wolfgang Denk wrote:
> 3. Hardware initialization in inherently very much hardware depen-
>    dent ;-)  We have some boards where PCI must be initialized early
>    because it is needed to access some other peripherals like memory.
>    And we have other boards where PCI can only be initialized late
>    because it depends on a lot of other functions that must be working
>    first.
> 
>    I explained this a number of times before: the current code was
>    designed to allow even for completely board specific init
>    sequences, by simply adding #define for the list of init functions
>    to the board config file - see for example here:
> 
>    http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33951/focus=36436
> 
>    http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136
> 
>    If you look at the current code - heavily larded with #ifdefs of
>    all shapes and colors - I cannot see any good way to transform this
>    into an initcall (and thus sequence number based) implementation.
>    Do you have any specific ideas how to address this?

if the funcs get merged by the linker script, the board still has full control 
over the init order by writing their own linker script
-mike
Graeme Russ - Aug. 23, 2011, 1:10 a.m.
Hi Mike

On Tue, Aug 23, 2011 at 11:00 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday, August 22, 2011 16:10:23 Wolfgang Denk wrote:
>> 3. Hardware initialization in inherently very much hardware depen-
>>    dent ;-)  We have some boards where PCI must be initialized early
>>    because it is needed to access some other peripherals like memory.
>>    And we have other boards where PCI can only be initialized late
>>    because it depends on a lot of other functions that must be working
>>    first.
>>
>>    I explained this a number of times before: the current code was
>>    designed to allow even for completely board specific init
>>    sequences, by simply adding #define for the list of init functions
>>    to the board config file - see for example here:
>>
>>    http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33951/focus=36436
>>
>>    http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/72131/focus=72136
>>
>>    If you look at the current code - heavily larded with #ifdefs of
>>    all shapes and colors - I cannot see any good way to transform this
>>    into an initcall (and thus sequence number based) implementation.
>>    Do you have any specific ideas how to address this?
>
> if the funcs get merged by the linker script, the board still has full control
> over the init order by writing their own linker script

Hmm, I hadn't thought of that as a solution. It would work, but i think it
is very fragile. The way I have done the INIT_FUNC macro, each function
gets assigned to an (otherwise unused) variable which is given it's own
section in the link with each section being named with a .<step number>
at the end. Now doing SORT_BY_NAME makes sure all the sections are in the
right order and that all sections (and therefore variables, and therefore
init functions) are included.

To re-order the function in the linker script would mean individually
specifying each and every section in the boards linker script. If another
function get added at the arch level, there is a great chance that this new
linker section would not get included in any board specific linker script

Regards,

Graeme
Wolfgang Denk - Aug. 23, 2011, 11:49 a.m.
Dear Graeme Russ,

In message <CALButC+W1ekU6XmaW5FvVOFc6Y4bhchVWvFGBV_FQPw=pUSQdA@mail.gmail.com> you wrote:
> 
> > 1. I think we should change the code in a different order.  I would
> >   prefer to first unify the init code across architectures (the big
> >   ARM reorganization we just seem to have overcome was an important
> >   prerequisite for this), before we start changing the init code.
> 
> I agree - I am currently auditing the init sequences and teasing out any
> common ordering and identifying where arches call an otherwise common
> init function out-of-order to the others. This is, generally speaking,
> very ugly - There are:
> 
>  - Two archictectures that do not use the init loop
>  - Two that do not relocate and therefore have a mix of (what other arches
>    consider) _f and _r int functions in a single loop
>  - Functions that could be made init functions after the init loop in
>    board_init_r
>  - Some arches have an equivalent init_f loop, others have an init_r loop
>    x86 is the only one that does both _f anf _r init sequences as loops

I guess this will not be a single big change, but a step by step
approach.  We will probably start by merging Power and ARM
architectures, and probably x86.  Others can follow one by one.

> I think, as a first step, all arches need to implement an init_f and
> init_r loop and collapse board_init_f and board_init_r into trival
> loop processing functions - Combine them into one function (which takes
> a pointer to the relavent init_function array) and move it into
> /lib/init.c - Easy ;)

OK with me - eventually we will have some kind of architecture
cleanup, dropping dead architectures on the way ;-)

> > 2. One of the advantages of the current implementation is that there
> >   is a central place in the code (well, at least per architecture,
> >   untill we unify these as mentioned above) where you can see exactly
> >   which steps get performed in which sequence.
> 
> This new method can have the same by putting all the INIT_FUNC()
> in a single location, much like the existing array. I don't think this
> is a good idea though. I prefer good clear documentation.

The problem is that "clear documentation" is no replacement for
clear (and easily readable) code - if you maintain such information in
two different places (doc and code) you have a guarantee that these
two get out of sync rather sooner than later.

> >   I understand (and appreciate) your intentions, does the initcall
> >   concept not mean that we will have some sort of sequence numbers
> >   distributed all over the code?  Maybe I'm mising something - but
> >   does this not make it really difficult to actually see (from the
> >   source code) what the exact init sequence is?
> 
> Well the bulk of the init sequence will be defined by init steps clearly
> defined in include/init.h. Where there is as arch, SoC or board which
> needs an init function called immediately after a particular point in the
> standard sequence, then there will be:
> 
> 	#define CUSTOM_INIT_STEP	STANDARD_INIT_STEP + 1

Hm... I don't really consider this an improvement.

> >   If you look at the current code - heavily larded with #ifdefs of
> >   all shapes and colors - I cannot see any good way to transform this
> >   into an initcall (and thus sequence number based) implementation.
> >   Do you have any specific ideas how to address this?
> 
> The _really_ nice thing about initcall is that if you take something like
> PCI, you put the INIT_FUNC() in pci.c and it only gets included if you
> define CONFIG_PCI - No #ifdefs. The problem, as you quite rightly point
> out, is how to clearly define INIT_STEP_PCI. From what I am seeing, these
> are mostly corner cases that can be clearly documented.

I'm not talking about the documentation - I'm worried about the
implementation - especially when some board stops working at some
pooint, or when trying to understand patches submitted for a new
board.  It has always been hard enough to debug issues in the early
init code, and I don't expect this to improve with the suggested
changes.

BTW: did you check the impact on the memory footprint yet?

> > This is actually hte biffest voncern I have: I cannot imagine how you
> > will try to figure out the exact init sequence when writing the code
> > (or, even worse, when reading somebody else's code).
> 
> Good point - The init sequence audit I am doing now will help. It will
> become a lot clearer when the init sequences are unified. For the rest,
> good clear documentation will help, but I think we will only know how it
> looks and feels when a complete patchs hits the list.
> 
> One nicity of putting INIT_FUNC() after a function body (or just before
> it which is easier to see if the body is longer that a screen) is that
> you can see immediately that the function is involved in the init
> sequence - I kind of like that.

Agreed.

But for me it is _way_ more important to be able to see the _complete_
list of all init functions, and their specific order of execution for
a given board configuration.

> I have another patch now that #define's the step numbers. As long as the
> step numbers are 100-999 (or 1000-9999, 10000-99999 etc) then everything
> works fine. The linker MUST support SORT_BY_NAME, KEEP and DISCARD - can
> anyone think of a linker that does not? Optimization will only be an issue

Yes, I can.  SORT_BY_NAME() was introduced with binutils version 2.16,
so there will be problems when building with all tool chains using
older binutils.


> OK, so what do I see as adventageous about using initcall versus current
> implementation...
> 
>  - Any arch, SoC or board can cleanly inject an init function anywhere in
>    the init sequence without touching a single line of code outside that
>    arch/SoC/board. The current system requires a mod to /arch/lib/board.c
>    with the potential addittion of ugly #ifdef's

I tend to see this as a disadvantage, actually [as it makes it easily
to lose track about which code is running when, or at all].

>  - It's more like Linux :)

I don't see why this would be an advantage per se.  Let's move this to
the "neutral" part ;-)

> And the bad...
>  - Init sequence not clearly obvious in a single location
>  - Maybe brain-dead linkers will do something wrong

Agreed.

> And the neutral
>  - Dealing with specific arch/Soc/board requiring an init function at a
>    different sequence step than everything else

I don't understand what you mean here.

> Some neat features that are trivial to implement with initcall and
> difficult or ugly with init array:
>  - The names of the function can be (conditionally by DEBUG) added and
>    printed out as the init sequence is being run - Very nice for debugging
>    the init sequence

You are actually wrong here - guess why the current init loop does not
contain such a debug()?  There are some init functions that need to be
run before you can output anything (for exaple, you must initialize
the console driver, including setting the baud rate or similar
parameters from the environment).

>  - Boot profiling - Store the step number and time to execute then after
>    bootup completes, a simple 'show boot profile information' can print
>    each function name and the time taken to execute

Could be done with the same amount of effort in the init loop as well.

> Honestly, after having a good play with this and fully converting x86 to
> trivial board_init_f and board_init_r functions, I really like the initcall
> method - It really does 'just work' like a dream :)

If it works, yes.  My concern is what to do if it doesn't, how to
debug such situations, and how to review such code.

Best regards,

Wolfgang Denk
Wolfgang Denk - Aug. 23, 2011, 11:52 a.m.
Dear Mike Frysinger,

In message <201108222100.25104.vapier@gentoo.org> you wrote:
>
> >    If you look at the current code - heavily larded with #ifdefs of
> >    all shapes and colors - I cannot see any good way to transform this
> >    into an initcall (and thus sequence number based) implementation.
> >    Do you have any specific ideas how to address this?
>
> if the funcs get merged by the linker script, the board still has full control 
> over the init order by writing their own linker script

Indeed. So we have yet another point that makes debugging,
understanding and reviewing the code more difficult - now a simple
typo in the linker script also has the potential to mess up the init
sequence in not exactly funny ways.

Best regards,

Wolfgang Denk
Graeme Russ - Aug. 23, 2011, 11:20 p.m.
Hi Wolfgang,

On Tue, Aug 23, 2011 at 9:49 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButC+W1ekU6XmaW5FvVOFc6Y4bhchVWvFGBV_FQPw=pUSQdA@mail.gmail.com> you wrote:
>>

[snip]

>
>> > 2. One of the advantages of the current implementation is that there
>> >   is a central place in the code (well, at least per architecture,
>> >   untill we unify these as mentioned above) where you can see exactly
>> >   which steps get performed in which sequence.
>>
>> This new method can have the same by putting all the INIT_FUNC()
>> in a single location, much like the existing array. I don't think this
>> is a good idea though. I prefer good clear documentation.
>
> The problem is that "clear documentation" is no replacement for
> clear (and easily readable) code - if you maintain such information in
> two different places (doc and code) you have a guarantee that these
> two get out of sync rather sooner than later.

True, and that is why the primary location for the init sequence
'documentation' would be init init.h - This is where I would defined all
the sequence numbers for the 'Primary' init functions (i.e. those that
are not very board specific).

Now considering that we have a massive address space for the init sequence
(10000-99999 give 89999 discrete steps), the init sequence for every
board can be defined here with a namespace regime like:

ININT_<global/arch/Soc/board>_<step name>_[F,R]

So we end up with:

#DEFINE INIT_GLOBAL_START	10000
#DEFINE INIT_X86_CPU_F		INIT_GLOBAL_START + 1
#DEFINE INIT_ARM_CPU_F		INIT_GLOBAL_START + 1
...
#DEFINE INIT_X86_<foo>		INIT_X86_<bar> + 1
...
#DEFINE INIT_X86_TIMER		INIT_X86_<foo> + 1
#DEFINE INIT_ARM_TIMER		INIT_ARM_CPU_F + 1
...
#DEFINE INIT_ENET_TIMER		INIT_X86_TIMER + 1

So this gives you a single central location to quickly see the init
sequence of any arch, SoC, or board.

>>       #define CUSTOM_INIT_STEP        STANDARD_INIT_STEP + 1
>
> Hm... I don't really consider this an improvement.

Why not? See above - I think centralising EVERY init sequence in init.h
rather than splatering board specific #ifdef's in arch/board.c is an
improvement - YMMV

>> >   If you look at the current code - heavily larded with #ifdefs of
>> >   all shapes and colors - I cannot see any good way to transform this
>> >   into an initcall (and thus sequence number based) implementation.
>> >   Do you have any specific ideas how to address this?
>>
>> The _really_ nice thing about initcall is that if you take something like
>> PCI, you put the INIT_FUNC() in pci.c and it only gets included if you
>> define CONFIG_PCI - No #ifdefs. The problem, as you quite rightly point
>> out, is how to clearly define INIT_STEP_PCI. From what I am seeing, these
>> are mostly corner cases that can be clearly documented.
>
> I'm not talking about the documentation - I'm worried about the
> implementation - especially when some board stops working at some
> pooint, or when trying to understand patches submitted for a new
> board.  It has always been hard enough to debug issues in the early
> init code, and I don't expect this to improve with the suggested
> changes.

I mentioned before about including a DEBUG feature to print init function
names as they are being run (more comments on that below). And there have
already been complaints about how hard the current init sequence is. A
flexible init sequence is always going to be difficult  to difficult to
understand, trace, and debug.

> BTW: did you check the impact on the memory footprint yet?

No - I'll do that tonight

[snip]

>> I have another patch now that #define's the step numbers. As long as the
>> step numbers are 100-999 (or 1000-9999, 10000-99999 etc) then everything
>> works fine. The linker MUST support SORT_BY_NAME, KEEP and DISCARD - can
>> anyone think of a linker that does not? Optimization will only be an issue
>
> Yes, I can.  SORT_BY_NAME() was introduced with binutils version 2.16,
> so there will be problems when building with all tool chains using
> older binutils.

That was released in May 2005 and in U-Boot parlance 'ancient' ;) - But I
am well aware that lack of SORT_BY_NAME() is a total killer for what I am
proposing. I need to know if anyone is using a linker that does not support
SORT_BY_NAME(). If there are (and if those users have no option to use a
linker that doeS) then the whole initcall idea is dead in the water anyway

>> OK, so what do I see as adventageous about using initcall versus current
>> implementation...
>>
>>  - Any arch, SoC or board can cleanly inject an init function anywhere in
>>    the init sequence without touching a single line of code outside that
>>    arch/SoC/board. The current system requires a mod to /arch/lib/board.c
>>    with the potential addittion of ugly #ifdef's
>
> I tend to see this as a disadvantage, actually [as it makes it easily
> to lose track about which code is running when, or at all].

I cannot possibly see how getting rid of an #ifdef mess is a disadvantage.
When tracing through the init sequence, you have to be continually mindful
of what is and isn't defined. I think we can take the serial initialisation
code as a prime example of just how bad this can get

>>  - It's more like Linux :)
>
> I don't see why this would be an advantage per se.  Let's move this to
> the "neutral" part ;-)


>> And the bad...
>>  - Init sequence not clearly obvious in a single location

Well actually, I think I can resolve this

>>  - Maybe brain-dead linkers will do something wrong
>
> Agreed.

Yep - can be a total deal breaker - Can we at least find out please?

>> And the neutral
>>  - Dealing with specific arch/Soc/board requiring an init function at a
>>    different sequence step than everything else
>
> I don't understand what you mean here.

Take timer initialisation for example - This is at a different step in
every arch. I think this is easily resolved with a good #define namespace
and putting it all in init.h

>> Some neat features that are trivial to implement with initcall and
>> difficult or ugly with init array:
>>  - The names of the function can be (conditionally by DEBUG) added and
>>    printed out as the init sequence is being run - Very nice for debugging
>>    the init sequence
>
> You are actually wrong here - guess why the current init loop does not
> contain such a debug()?  There are some init functions that need to be
> run before you can output anything (for exaple, you must initialize
> the console driver, including setting the baud rate or similar
> parameters from the environment).

The current method cannot print the init function name unless the init
function itself does so. I have included an additional component to the
INIT_FUNC macro which adds the string name of the function and builds a
second table of pointers to these strings. The init loop then simply steps
through the function name pointers while stepping throught the function
pointers. If we simply add a CONSOLE_INITIALISED flag into gd->flags then
the output of the function names can be suppressed until console output
is available.

>>  - Boot profiling - Store the step number and time to execute then after
>>    bootup completes, a simple 'show boot profile information' can print
>>    each function name and the time taken to execute
>
> Could be done with the same amount of effort in the init loop as well.

Not the inclusion of the function name strings.

>> Honestly, after having a good play with this and fully converting x86 to
>> trivial board_init_f and board_init_r functions, I really like the initcall
>> method - It really does 'just work' like a dream :)
>
> If it works, yes.  My concern is what to do if it doesn't, how to
> debug such situations, and how to review such code.

Debug is no different to now - The code is stepping through a table of
function pointers. The new method can add the printing of the function
about to be called (after console init) which makes life a little easier.
As for patches, the inclusion of INIT_FUNC() in the patch is a red-flag to
look more closely - Any new step will also include an addition to init.h

Regards,

Graeme
Wolfgang Denk - Aug. 24, 2011, 5:38 a.m.
Dear Graeme Russ,

In message <CALButCJg5BPP_Z0VUMEfQgjpRYO=WDQBvvBFho6VbNovbRjFzQ@mail.gmail.com> you wrote:
> 
> So we end up with:
> 
> #DEFINE INIT_GLOBAL_START	10000
> #DEFINE INIT_X86_CPU_F		INIT_GLOBAL_START + 1
> #DEFINE INIT_ARM_CPU_F		INIT_GLOBAL_START + 1
> ...
> #DEFINE INIT_X86_<foo>		INIT_X86_<bar> + 1
> ...
> #DEFINE INIT_X86_TIMER		INIT_X86_<foo> + 1
> #DEFINE INIT_ARM_TIMER		INIT_ARM_CPU_F + 1
> ...
> #DEFINE INIT_ENET_TIMER		INIT_X86_TIMER + 1
> 
> So this gives you a single central location to quickly see the init
> sequence of any arch, SoC, or board.

But frankly: do you consider this list above _readable_?

So how do I figure out which steps are actually being executed?
I could, for example, assume that INIT_X86_TIMER and INIT_ARM_TIMER
are run at the same time (in random order) on one board - OK, here it
is obvious from the names that this is probably not the case, but we
will have other, less obvious situations.

> >>       #define CUSTOM_INIT_STEP        STANDARD_INIT_STEP + 1
> >
> > Hm... I don't really consider this an improvement.
> 
> Why not? See above - I think centralising EVERY init sequence in init.h
> rather than splatering board specific #ifdef's in arch/board.c is an
> improvement - YMMV

You keep the definitions in one place, but in a mor or less
unreadable form.

> That was released in May 2005 and in U-Boot parlance 'ancient' ;) - But I
> am well aware that lack of SORT_BY_NAME() is a total killer for what I am
> proposing. I need to know if anyone is using a linker that does not support
> SORT_BY_NAME(). If there are (and if those users have no option to use a
> linker that doeS) then the whole initcall idea is dead in the water anyway

We do have customers who are still using ELDK 2.1.0 from April 2003
(gcc version 2.95.4, binutils version version 2.11.93.0.2).  Agreed,
these are not using recent versions of U-Boot either.

> >> And the bad...
> >>  - Init sequence not clearly obvious in a single location
> 
> Well actually, I think I can resolve this

How?  Above suggestion is not a solution, but actually shows the
problem.

> Take timer initialisation for example - This is at a different step in
> every arch. I think this is easily resolved with a good #define namespace
> and putting it all in init.h

And this becomes easy to read and understand, then? :-(

> >>  - The names of the function can be (conditionally by DEBUG) added and
> >>    printed out as the init sequence is being run - Very nice for debugging
> >>    the init sequence
> >
> > You are actually wrong here - guess why the current init loop does not
> > contain such a debug()?  There are some init functions that need to be
> > run before you can output anything (for exaple, you must initialize
> > the console driver, including setting the baud rate or similar
> > parameters from the environment).
> 
> The current method cannot print the init function name unless the init
> function itself does so. I have included an additional component to the

It would be trivial to enable printing the names from the loop; we
can't do it because we don't have the needed prerequisites yet in most
cases.

> INIT_FUNC macro which adds the string name of the function and builds a
> second table of pointers to these strings. The init loop then simply steps
> through the function name pointers while stepping throught the function
> pointers. If we simply add a CONSOLE_INITIALISED flag into gd->flags then
> the output of the function names can be suppressed until console output
> is available.

You know that debugging becomes easy as soon as we have printf().
But how does this help you for the init steps _before_ you have a
working console?

> > If it works, yes.  My concern is what to do if it doesn't, how to
> > debug such situations, and how to review such code.
> 
> Debug is no different to now - The code is stepping through a table of
> function pointers. The new method can add the printing of the function

The difference is that I have a source file for reference.

With the init.h as above I have no such reference.

> about to be called (after console init) which makes life a little easier.
> As for patches, the inclusion of INIT_FUNC() in the patch is a red-flag to
> look more closely - Any new step will also include an addition to init.h

So init.h will quickly become an incomprehensible mess.

Best regards,

Wolfgang Denk
Graeme Russ - Aug. 24, 2011, 5:57 a.m.
Hi Wolfgang,

On Wed, Aug 24, 2011 at 3:38 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButCJg5BPP_Z0VUMEfQgjpRYO=WDQBvvBFho6VbNovbRjFzQ@mail.gmail.com> you wrote:
>>
>> So we end up with:
>>
>> #DEFINE INIT_GLOBAL_START     10000
>> #DEFINE INIT_X86_CPU_F                INIT_GLOBAL_START + 1
>> #DEFINE INIT_ARM_CPU_F                INIT_GLOBAL_START + 1
>> ...
>> #DEFINE INIT_X86_<foo>                INIT_X86_<bar> + 1
>> ...
>> #DEFINE INIT_X86_TIMER                INIT_X86_<foo> + 1
>> #DEFINE INIT_ARM_TIMER                INIT_ARM_CPU_F + 1
>> ...
>> #DEFINE INIT_ENET_TIMER               INIT_X86_TIMER + 1
>>
>> So this gives you a single central location to quickly see the init
>> sequence of any arch, SoC, or board.
>
> But frankly: do you consider this list above _readable_?
>
> So how do I figure out which steps are actually being executed?
> I could, for example, assume that INIT_X86_TIMER and INIT_ARM_TIMER
> are run at the same time (in random order) on one board - OK, here it
> is obvious from the names that this is probably not the case, but we
> will have other, less obvious situations.

grep is your friend - All you need to to is grep for GLOBAL (actually I
think COMMON is a better name) and the ARCH, SOC, and BOARD keywords in
the namespace for your board and voila - You have a sorted list of the
init sequence for you board

>> >>       #define CUSTOM_INIT_STEP        STANDARD_INIT_STEP + 1
>> >
>> > Hm... I don't really consider this an improvement.
>>
>> Why not? See above - I think centralising EVERY init sequence in init.h
>> rather than splatering board specific #ifdef's in arch/board.c is an
>> improvement - YMMV
>
> You keep the definitions in one place, but in a mor or less
> unreadable form.

I don't see how it is unreadable after grep - I would argue that it is
way more readable than the #ifdef mess we are heading down

>> That was released in May 2005 and in U-Boot parlance 'ancient' ;) - But I
>> am well aware that lack of SORT_BY_NAME() is a total killer for what I am
>> proposing. I need to know if anyone is using a linker that does not support
>> SORT_BY_NAME(). If there are (and if those users have no option to use a
>> linker that doeS) then the whole initcall idea is dead in the water anyway
>
> We do have customers who are still using ELDK 2.1.0 from April 2003
> (gcc version 2.95.4, binutils version version 2.11.93.0.2).  Agreed,
> these are not using recent versions of U-Boot either.
>
>> >> And the bad...
>> >>  - Init sequence not clearly obvious in a single location
>>
>> Well actually, I think I can resolve this
>
> How?  Above suggestion is not a solution, but actually shows the
> problem.
>
>> Take timer initialisation for example - This is at a different step in
>> every arch. I think this is easily resolved with a good #define namespace
>> and putting it all in init.h
>
> And this becomes easy to read and understand, then? :-(


>
>> >>  - The names of the function can be (conditionally by DEBUG) added and
>> >>    printed out as the init sequence is being run - Very nice for debugging
>> >>    the init sequence
>> >
>> > You are actually wrong here - guess why the current init loop does not
>> > contain such a debug()?  There are some init functions that need to be
>> > run before you can output anything (for exaple, you must initialize
>> > the console driver, including setting the baud rate or similar
>> > parameters from the environment).
>>
>> The current method cannot print the init function name unless the init
>> function itself does so. I have included an additional component to the
>
> It would be trivial to enable printing the names from the loop; we
> can't do it because we don't have the needed prerequisites yet in most
> cases.

How so? I cannot see how you could create a macro which when used in the
static array initialiser would send the function pointers to one array and
the string names (or pointers to) to another array.

>> INIT_FUNC macro which adds the string name of the function and builds a
>> second table of pointers to these strings. The init loop then simply steps
>> through the function name pointers while stepping throught the function
>> pointers. If we simply add a CONSOLE_INITIALISED flag into gd->flags then
>> the output of the function names can be suppressed until console output
>> is available.
>
> You know that debugging becomes easy as soon as we have printf().
> But how does this help you for the init steps _before_ you have a
> working console?

How does debugging in that case work now? There is no difference between
the two implementations - the only thing I am changing is:
 - Where the array of function pointers gets placed the U-Boot image
   (.initfunctions.number versus .data)
 - The fact that you can inject an arbitrary function pointer (at compile
   time) into the array without messy #ifdef's

>> > If it works, yes.  My concern is what to do if it doesn't, how to
>> > debug such situations, and how to review such code.
>>
>> Debug is no different to now - The code is stepping through a table of
>> function pointers. The new method can add the printing of the function
>
> The difference is that I have a source file for reference.

Huh? You still do in the initcall case

> With the init.h as above I have no such reference.

No reference to what? - the static array of function pointers? This is of
little use anyway as all your debug stepping will do is pick up the next
value from the array

>> about to be called (after console init) which makes life a little easier.
>> As for patches, the inclusion of INIT_FUNC() in the patch is a red-flag to
>> look more closely - Any new step will also include an addition to init.h
>
> So init.h will quickly become an incomprehensible mess.

No, it becomes a clean linear list of the init sequence from which you
can easily grep out the noise. This list will never have an init step
defined out-of-order. If INIT_YOUR_BOARD_ETHERNET occurs after
INIT_YOUR_ARCH_PCI in the list then you know your board initialises its
Ehternet after the arch has initialised PCI


Regards,

Graeme
Wolfgang Denk - Aug. 24, 2011, 6:48 a.m.
Dear Graeme Russ,

In message <CALButCLX42Q=u33gCOKA+LoZZ+RZO1Y-SXNhE8MK66DfrU9YPA@mail.gmail.com> you wrote:
> 
> > But frankly: do you consider this list above _readable_?
...
> grep is your friend - All you need to to is grep for GLOBAL (actually I
> think COMMON is a better name) and the ARCH, SOC, and BOARD keywords in
> the namespace for your board and voila - You have a sorted list of the
> init sequence for you board

Yes, this is exactly what I mean.  I will need additional tools to
be able to read the code.  I cannot - for example - print a few pages
of source code and check the lines that worked, or similar.

> > It would be trivial to enable printing the names from the loop; we
> > can't do it because we don't have the needed prerequisites yet in most
> > cases.
> 
> How so? I cannot see how you could create a macro which when used in the
> static array initialiser would send the function pointers to one array and
> the string names (or pointers to) to another array.

See the example in the CPP info pages: cpp.info, Node: Concatenation:

	#define COMMAND(NAME)  { #NAME, NAME ## _command }

> How does debugging in that case work now? There is no difference between
> the two implementations - the only thing I am changing is:

Right.  So we cannot say that the initcall code is any improvment here.

> > The difference is that I have a source file for reference.
> 
> Huh? You still do in the initcall case

Agreed.  I should have written: I have a _readable_ source file that
can be parsed without additional tools.

> > With the init.h as above I have no such reference.
> 
> No reference to what? - the static array of function pointers? This is of
> little use anyway as all your debug stepping will do is pick up the next
> value from the array

But I can _see_ what the next entry will be.

> No, it becomes a clean linear list of the init sequence from which you
> can easily grep out the noise. This list will never have an init step
> defined out-of-order. If INIT_YOUR_BOARD_ETHERNET occurs after
> INIT_YOUR_ARCH_PCI in the list then you know your board initialises its
> Ehternet after the arch has initialised PCI

If I see some INIT_YOUR_ARCH_FOO in this list - how can I see if this
is actually being executed on my specific board?  This might depend on
a number of feature selections, so it is run on one board but not on
another.

Yes, you remove the #ifdef's - but here in this case this is exactly
the information that would be needed one way or another.


I bet that rather sooner than later you will end up with some toold
that parses either the ELF file or the linker map or the symbol table
to generate a readable list for the current build.  I bet a case of
beer that you will need such a tool.  We don't need it now.

Best regards,

Wolfgang Denk
Graeme Russ - Aug. 24, 2011, 7:06 a.m.
Hi Wolfgang,

On Wed, Aug 24, 2011 at 4:48 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButCLX42Q=u33gCOKA+LoZZ+RZO1Y-SXNhE8MK66DfrU9YPA@mail.gmail.com> you wrote:
>>
>> > But frankly: do you consider this list above _readable_?
> ...
>> grep is your friend - All you need to to is grep for GLOBAL (actually I
>> think COMMON is a better name) and the ARCH, SOC, and BOARD keywords in
>> the namespace for your board and voila - You have a sorted list of the
>> init sequence for you board
>
> Yes, this is exactly what I mean.  I will need additional tools to
> be able to read the code.  I cannot - for example - print a few pages
> of source code and check the lines that worked, or similar.

Well if the use of grep irks you so, I dare you to rm grep ;)

>> > It would be trivial to enable printing the names from the loop; we
>> > can't do it because we don't have the needed prerequisites yet in most
>> > cases.
>>
>> How so? I cannot see how you could create a macro which when used in the
>> static array initialiser would send the function pointers to one array and
>> the string names (or pointers to) to another array.
>
> See the example in the CPP info pages: cpp.info, Node: Concatenation:
>
>        #define COMMAND(NAME)  { #NAME, NAME ## _command }

Ah, so turn the current array into a two element array containing a
function pointer and string pointer. I guess that would work, but that will
totally change the symantics of the static variable between debug and non
debug builds - Not an optimal scenario in my mind

>> How does debugging in that case work now? There is no difference between
>> the two implementations - the only thing I am changing is:
>
> Right.  So we cannot say that the initcall code is any improvment here.
>
>> > The difference is that I have a source file for reference.
>>
>> Huh? You still do in the initcall case
>
> Agreed.  I should have written: I have a _readable_ source file that
> can be parsed without additional tools.

I would hardly call 'grep' an extra tool - How many programmers do you
know that have never used grep?

>> > With the init.h as above I have no such reference.
>>
>> No reference to what? - the static array of function pointers? This is of
>> little use anyway as all your debug stepping will do is pick up the next
>> value from the array
>
> But I can _see_ what the next entry will be.

And by defining #DEBUG I can have a parallel list of function names so when
the loop picks up the next funtion pointer, it picks up the name as well
ready to examine before stepping in - OK, not ideal, but still, I think
the flexibility to seamlessly inject init functions at the board level
outweighs this

>> No, it becomes a clean linear list of the init sequence from which you
>> can easily grep out the noise. This list will never have an init step
>> defined out-of-order. If INIT_YOUR_BOARD_ETHERNET occurs after
>> INIT_YOUR_ARCH_PCI in the list then you know your board initialises its
>> Ehternet after the arch has initialised PCI
>
> If I see some INIT_YOUR_ARCH_FOO in this list - how can I see if this
> is actually being executed on my specific board?  This might depend on
> a number of feature selections, so it is run on one board but not on
> another.

Technically we have the same issue with weak functions - Just because you
have code that overrides them, does not mean that code gets compiled in.

The initcall solution introduces no additional 'issues' that we are not
already dealing with on a day-to-day basis.

> Yes, you remove the #ifdef's - but here in this case this is exactly
> the information that would be needed one way or another.
>
>
> I bet that rather sooner than later you will end up with some toold
> that parses either the ELF file or the linker map or the symbol table
> to generate a readable list for the current build.  I bet a case of
> beer that you will need such a tool.  We don't need it now.

Sorry, I don't have a crystal ball

Regards,

Graeme
Wolfgang Denk - Aug. 24, 2011, 12:49 p.m.
Dear Graeme Russ,

In message <CALButCK=hN8VBmu+8EzVq7UhFFASrj_ZjN_Yz8WQT0QnXjLoog@mail.gmail.com> you wrote:
> 
> > Agreed.  I should have written: I have a _readable_ source file that
> > can be parsed without additional tools.
> 
> I would hardly call 'grep' an extra tool - How many programmers do you
> know that have never used grep?

That's not the point.  There is a significant difference between seeng
the source code in your favorite editor or in the source code window
of your debugger's GUI or even some IDE,  versus having to run
additional commands in a separate window.

> And by defining #DEBUG I can have a parallel list of function names so when
> the loop picks up the next funtion pointer, it picks up the name as well
> ready to examine before stepping in - OK, not ideal, but still, I think
> the flexibility to seamlessly inject init functions at the board level
> outweighs this

Sorry, but when reading the source code or when revioewing patches I
cannot paick up any next funtion pointers, I'm stuck with reading the
source code only.


Best regards,

Wolfgang Denk
Graeme Russ - Aug. 24, 2011, 12:56 p.m.
Hi Wolfgang,

On 24/08/11 22:49, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <CALButCK=hN8VBmu+8EzVq7UhFFASrj_ZjN_Yz8WQT0QnXjLoog@mail.gmail.com> you wrote:
>>
>>> Agreed.  I should have written: I have a _readable_ source file that
>>> can be parsed without additional tools.
>>
>> I would hardly call 'grep' an extra tool - How many programmers do you
>> know that have never used grep?
> 
> That's not the point.  There is a significant difference between seeng
> the source code in your favorite editor or in the source code window
> of your debugger's GUI or even some IDE,  versus having to run
> additional commands in a separate window.
> 
>> And by defining #DEBUG I can have a parallel list of function names so when
>> the loop picks up the next funtion pointer, it picks up the name as well
>> ready to examine before stepping in - OK, not ideal, but still, I think
>> the flexibility to seamlessly inject init functions at the board level
>> outweighs this
> 
> Sorry, but when reading the source code or when revioewing patches I
> cannot paick up any next funtion pointers, I'm stuck with reading the
> source code only.

Well, I guess I had a good shot at creating a 'better' init sequence - one
where any board, SoC, or arch can seamlessly inject a custom init step
without treading on any toes.

I must say, it was rather fun learning how to build the macros and get the
linker to do the right thing and have it all work so quickly. I'll stick it
in my bag of tricks for another day :)

Regards,

Graeme
Wolfgang Denk - Aug. 24, 2011, 1:24 p.m.
Dear Graeme Russ,

In message <4E54F501.6050706@gmail.com> you wrote:
> 
> > Sorry, but when reading the source code or when revioewing patches I
> > cannot paick up any next funtion pointers, I'm stuck with reading the
> > source code only.
> 
> Well, I guess I had a good shot at creating a 'better' init sequence - one
> where any board, SoC, or arch can seamlessly inject a custom init step
> without treading on any toes.
> 
> I must say, it was rather fun learning how to build the macros and get the
> linker to do the right thing and have it all work so quickly. I'll stick it
> in my bag of tricks for another day :)

Hey, you give up early.  Are you suffering from hot weather, too?  :-)

Seriously,  I do not think we should stop this discussion yet. I agree
that a better approach to the init code would be a good thing, but at
the same time I want to make sure the result will be really better,
and we not by means of Beelzebub expel the demons...


I already tried to lend a helping hand by suggesting to create a list
of init functions as part of the build process - OK, we still have to
build the code to get this, but at last we can then see the complete
and precise order for this specific configuration.


Also, Detlev made some interesting remarks - he noted that basicly
what we are trying to solve is a dependency issue: each init function
has a list of dependencies (other init steps) that need to be run
before.  Instead of manually assigning initcall numbers, we could try
and write down these dependencies, for example in a format that can
be digested by a tool like tsort.  We could then use this to
auto-generate the list of init calls.  This is a completely new
approach, but it has the charme of making the dependencies clear.


Best regards,

Wolfgang Denk
Detlev Zundel - Aug. 24, 2011, 1:46 p.m.
Hi Wolfgang,

[...]

> I bet that rather sooner than later you will end up with some toold
> that parses either the ELF file or the linker map or the symbol table
> to generate a readable list for the current build.  I bet a case of
> beer that you will need such a tool.  We don't need it now.

objdump -t --section=... will probably do the trick.

Cheers
  Detlev
Graeme Russ - Aug. 24, 2011, 1:58 p.m.
Hi Wolfgang,

On 24/08/11 23:24, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4E54F501.6050706@gmail.com> you wrote:
>>
>>> Sorry, but when reading the source code or when revioewing patches I
>>> cannot paick up any next funtion pointers, I'm stuck with reading the
>>> source code only.
>>
>> Well, I guess I had a good shot at creating a 'better' init sequence - one
>> where any board, SoC, or arch can seamlessly inject a custom init step
>> without treading on any toes.
>>
>> I must say, it was rather fun learning how to build the macros and get the
>> linker to do the right thing and have it all work so quickly. I'll stick it
>> in my bag of tricks for another day :)
> 
> Hey, you give up early.  Are you suffering from hot weather, too?  :-)

No, just young kids, new job and pregnant wife :)

> Seriously,  I do not think we should stop this discussion yet. I agree
> that a better approach to the init code would be a good thing, but at
> the same time I want to make sure the result will be really better,
> and we not by means of Beelzebub expel the demons...
> 
> 
> I already tried to lend a helping hand by suggesting to create a list
> of init functions as part of the build process - OK, we still have to
> build the code to get this, but at last we can then see the complete
> and precise order for this specific configuration.

Something like how we auto generate asm-offsets.h?

> Also, Detlev made some interesting remarks - he noted that basicly
> what we are trying to solve is a dependency issue: each init function
> has a list of dependencies (other init steps) that need to be run
> before.  Instead of manually assigning initcall numbers, we could try
> and write down these dependencies, for example in a format that can
> be digested by a tool like tsort.  We could then use this to
> auto-generate the list of init calls.  This is a completely new
> approach, but it has the charme of making the dependencies clear.

Hmmm, now we're talking ;) Let me ramble aimlessly for a second...

I can see this heading towards an auto-generated init-sequence.c file with
an init array specifically crafted for the build target. That would have
less linker functionality dependencies than initcall...

So if we define a file, say 'init.txt' which lists the init dependencies
and we drill-down and pre-process this to generate /common/init-sequence.c
which has the array of function pointers (and optionally the function name
strings for debug output...)

Hmmm, it _sounds_ messy on the surface, but it is a pre-compile step so
what you get code-wise for the final build is exactly what you want - A
clean array that is 'right there'

So we might have in init.txt for a board:

INIT_STEP_TIMER(board_foo_timer_init)
DEPENDS_ON(INIT_STEP_ARM_TIMER)

and in arch/arm/Soc/init.txt

INIT_STEP_ARM_TIMER(arm_timer_init)
DEPENDS(INIT_STEP_SDRAM)
DEPENDS(INIT_STEP_GPIO)

Can't say I like it much...could be xml, but still very clunky

Maybe there is something to be gleaned...

Dunno - thoughts?

Regards,

Graeme
Graeme Russ - Aug. 24, 2011, 11 p.m.
Hi Wolfgang

On Wed, Aug 24, 2011 at 11:58 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang,
>
> On 24/08/11 23:24, Wolfgang Denk wrote:
>> Dear Graeme Russ,
>>
>> In message <4E54F501.6050706@gmail.com> you wrote:

[snip]

>
> Hmmm, now we're talking ;) Let me ramble aimlessly for a second...
>
> I can see this heading towards an auto-generated init-sequence.c file with
> an init array specifically crafted for the build target. That would have
> less linker functionality dependencies than initcall...
>
> So if we define a file, say 'init.txt' which lists the init dependencies
> and we drill-down and pre-process this to generate /common/init-sequence.c
> which has the array of function pointers (and optionally the function name
> strings for debug output...)
>
> Hmmm, it _sounds_ messy on the surface, but it is a pre-compile step so
> what you get code-wise for the final build is exactly what you want - A
> clean array that is 'right there'
>
> So we might have in init.txt for a board:
>
> INIT_STEP_TIMER(board_foo_timer_init)
> DEPENDS_ON(INIT_STEP_ARM_TIMER)
>
> and in arch/arm/Soc/init.txt
>
> INIT_STEP_ARM_TIMER(arm_timer_init)
> DEPENDS(INIT_STEP_SDRAM)
> DEPENDS(INIT_STEP_GPIO)
>
> Can't say I like it much...could be xml, but still very clunky
>
> Maybe there is something to be gleaned...
>
> Dunno - thoughts?

Midnight is not the best time to come up with ideas ;) - Way to much
convaluted pre-processing, non of it in native C code (or even pre-
processor) - Yetch! All to complicated for a very simple goal:

 - Allow any arch/Soc/board to seamlessly inject an init function without
   needing to modify any other files other than its own.

In essance, I see three 'big' problems with the initcall implementation:

 1. Do all linkers used to compile latest mailline U-Boot support
    SORT_BY_NAME()? If not, game over
 2. There is no definative init sequence list in the code (a la the
    current init function array)
 3. Managing the init call sequence

There is nothing I can do about #1 as mentioned before

For #2, if we had a post-build step which generated the init sequence into
a text file for reference would that help? If I name the initcall sections
something like .initfunc.1000.init_cpu_f then it will be pretty trivial to
post-process u-boot.map

As for #3 - What about INIT_AFTER (and maybe INIT_BEFORE) macros so you may
have in init.h

#define INIT_START	1000

and in /arch/.../cpu.c:

INIT_AFTER(INIT_START, init_cpu_f)
int init_cpu_f(void)
{
...
}

The only problem I have with that is that you cannot define a new macro in
a macro. I would love to do:

INIT_AFTER(INIT_START, init_cpu_f, INIT_CPU_F)

which would then define INIT_CPU_F so you could then:

INIT_AFTER(INIT_CPU_F, init_timers_f, INIT_TIMERS_F)

but I cannot figure out how that would work

Regards,

Graeme
Simon Glass - Aug. 24, 2011, 11:41 p.m.
Hi Graeme,

On Wed, Aug 24, 2011 at 4:00 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Wolfgang
>
> On Wed, Aug 24, 2011 at 11:58 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Wolfgang,
>>
>> On 24/08/11 23:24, Wolfgang Denk wrote:
>>> Dear Graeme Russ,
>>>
>>> In message <4E54F501.6050706@gmail.com> you wrote:
>
> [snip]
>
>>
>> Hmmm, now we're talking ;) Let me ramble aimlessly for a second...
>>
>> I can see this heading towards an auto-generated init-sequence.c file with
>> an init array specifically crafted for the build target. That would have
>> less linker functionality dependencies than initcall...
>>
>> So if we define a file, say 'init.txt' which lists the init dependencies
>> and we drill-down and pre-process this to generate /common/init-sequence.c
>> which has the array of function pointers (and optionally the function name
>> strings for debug output...)
>>
>> Hmmm, it _sounds_ messy on the surface, but it is a pre-compile step so
>> what you get code-wise for the final build is exactly what you want - A
>> clean array that is 'right there'
>>
>> So we might have in init.txt for a board:
>>
>> INIT_STEP_TIMER(board_foo_timer_init)
>> DEPENDS_ON(INIT_STEP_ARM_TIMER)
>>
>> and in arch/arm/Soc/init.txt
>>
>> INIT_STEP_ARM_TIMER(arm_timer_init)
>> DEPENDS(INIT_STEP_SDRAM)
>> DEPENDS(INIT_STEP_GPIO)
>>
>> Can't say I like it much...could be xml, but still very clunky
>>
>> Maybe there is something to be gleaned...
>>
>> Dunno - thoughts?
>
> Midnight is not the best time to come up with ideas ;) - Way to much
> convaluted pre-processing, non of it in native C code (or even pre-
> processor) - Yetch! All to complicated for a very simple goal:
>
>  - Allow any arch/Soc/board to seamlessly inject an init function without
>   needing to modify any other files other than its own.
>
> In essance, I see three 'big' problems with the initcall implementation:
>
>  1. Do all linkers used to compile latest mailline U-Boot support
>    SORT_BY_NAME()? If not, game over
>  2. There is no definative init sequence list in the code (a la the
>    current init function array)
>  3. Managing the init call sequence
>
> There is nothing I can do about #1 as mentioned before

No, and fair enough too.

>
> For #2, if we had a post-build step which generated the init sequence into
> a text file for reference would that help? If I name the initcall sections
> something like .initfunc.1000.init_cpu_f then it will be pretty trivial to
> post-process u-boot.map

Yes indeed.

>
> As for #3 - What about INIT_AFTER (and maybe INIT_BEFORE) macros so you may
> have in init.h
>
> #define INIT_START      1000
>
> and in /arch/.../cpu.c:
>
> INIT_AFTER(INIT_START, init_cpu_f)
> int init_cpu_f(void)
> {
> ...
> }
>
> The only problem I have with that is that you cannot define a new macro in
> a macro. I would love to do:
>
> INIT_AFTER(INIT_START, init_cpu_f, INIT_CPU_F)
>
> which would then define INIT_CPU_F so you could then:
>
> INIT_AFTER(INIT_CPU_F, init_timers_f, INIT_TIMERS_F)
>
> but I cannot figure out how that would work

Ick. The more I read of this thread the more I like your original
implementation. It really doesn't seem that hard to see the order of
initcalls either from System.map or objdump | grep or similar. Write a
nice long Python script if you like! Your implementation allows
tracing of initcalls as they are made which is a big win.

Early init cannot necessarily printf() but I don't see why initcalls
must solve a problem which exists currently anyway (I have been
thinking about buffering pre-console pre-reloc output and printing it
later - it's not that hard, just a bit ugly).

Yes moving to a dependency-based init sounds lovely, but this is
U-Boot not Upstart. We do need to keep things reasonably simple - at
least with your scheme we can tell at link time what the order will
be. The other thing is that init ordering is often not critical bar a
few early routines, and most of these will be fixed by the order
selected in the arch/xxx/lib/board.c code. Board maintainers would
likely only have a few ranges within which to add their init, and
strongly discouraged from inserting an init function into the middle
of an existing stable sequence in that file.

I do tend to agree that merging the board.c code is a bigger win, but
this might be a nice vehicle for doing it, since one could imagine an
incremental approach with a CONFIG_NEW_INIT option which can be turned
on/off for each board. As problems with each architecture (then each
board) are ironed out, the option will turned on in the board config
file.

Regards,
Simon

>
> Regards,
>
> Graeme

PS: Regarding the late night work, kids, wives, etc. I am reminded of
that 'quote' from Dr Johnson about his work with his 'U-Boot':

...that has taken eighteen hours of every day for the last ten years.
My mother died; I hardly noticed. My father cut off his head and fried it
in garlic in the hope of attracting my attention; I scarcely looked up from
my work.

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Graeme Russ - Aug. 25, 2011, 2:45 a.m.
Hi Simon

On Thu, Aug 25, 2011 at 9:41 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Graeme,

[big snip]

>
> Ick. The more I read of this thread the more I like your original
> implementation. It really doesn't seem that hard to see the order of
> initcalls either from System.map or objdump | grep or similar. Write a
> nice long Python script if you like! Your implementation allows
> tracing of initcalls as they are made which is a big win.

I'de like to keep any pre/post processing as simple as possible - limited
to grep, awk, and sed would be best

> Early init cannot necessarily printf() but I don't see why initcalls
> must solve a problem which exists currently anyway (I have been
> thinking about buffering pre-console pre-reloc output and printing it
> later - it's not that hard, just a bit ugly).

Allocate a buffer in pre-reloc RAM (Cache-As-RAM), have a pointer in gd
which points to where we are currently up to and redirect putc() if the
console has not been initialised. Immediately after console init, flush
the buffer via the now unredirected putc() and viola! I don't see this as
being too ugly. I might even have a go myself :)

> Yes moving to a dependency-based init sounds lovely, but this is
> U-Boot not Upstart. We do need to keep things reasonably simple - at

Yes, my initial thoughts are that building dependencies at build time
will be messy and at run time is a waste of CPU time

> least with your scheme we can tell at link time what the order will

Yes, but I understand Wolfgang's concern that prior to doing the build, it
is not obvious what the init sequence order will be. I personally think
that the current #ifdef'ing of the init array is not 'completely obvious'
either, but I conceded it is 'more obvious' than any proposed
implementation for the initcall sequence so far.

> be. The other thing is that init ordering is often not critical bar a
> few early routines, and most of these will be fixed by the order
> selected in the arch/xxx/lib/board.c code. Board maintainers would
> likely only have a few ranges within which to add their init, and
> strongly discouraged from inserting an init function into the middle
> of an existing stable sequence in that file.

I personally don't see not having an obvious init sequence pre-build as a
huge issue for me. The init sequence is not fiddled with often, and by
using the INIT_FUNC macro, there is a big red flag in any patch that
touches the init sequence.

I did start the think along the lines of four init.h files

 - /include/init.h for the sequence shared by all architectures
 - /include/<arch>/init.h - Arch specific, step numbering based on
   #defines in /include/init.h
 - /include/<arch>/<soc>/init.h - SoC specific, step numbering based on
   #defines in /include/init.h and /include/<arch>/init.h
 - /board/<board>/init.h - Board specific, step numbering based on
   #defines in /include/init.h, /include/<arch>/init.h and
   /include/<arch>/<soc>/init.h

But that would leave the definition of the init sequence splattered all
over the place. Maybe there is scope for a pre-compile stage like
asm-offsets, but again, you will not know the init order until you
actually do the build...

Creating a namespace for init step #defines, defining the entire init
sequence for every arch, SoC, and board in /include/init.h and using
grep to filter out the noise gets the init sequence in one spot, BUT,
just because INIT_ARCH_FOO is in init.h does not mean it will be included
in the build. The init array 'solves' this with #ifdefs around array
elements. I don't think this gives any greater clarity - It's not obvious
pre-compile if FOO is defined (although most decent IDE's are smart enough
to know that FOO is not defined and grey out the code, but this is far
from foolproof (I know eclipse has trouble when the #ifdef logic gets too
complicated, and most of the #defines do not even exist until you do a
make conig. Also, defining the whole sequence in /include/init.h breaks
my idea of a custom board init step not touching code outside the board
(an edit to /include/init.h is required - but that provides another red
flag for review)

> I do tend to agree that merging the board.c code is a bigger win, but
> this might be a nice vehicle for doing it, since one could imagine an
> incremental approach with a CONFIG_NEW_INIT option which can be turned
> on/off for each board. As problems with each architecture (then each
> board) are ironed out, the option will turned on in the board config
> file.

I must say, that my audit of the init code those far has been an eye
opener. As a collective, we have all let things drift to the point where
it starts to get all too hard to bring everthing back into line and we
just let the status quo continue. Relocation, timers, init and cache
handling are prime examples. I honestly think we should take a 'one mess
per merge window approach' - Decide on a drop-dead date for a global
change and just do it. Any arches, SoCs, or boards that don't make the
deadline get purged. It worked for ARM relocation ;)

> PS: Regarding the late night work, kids, wives, etc. I am reminded of
> that 'quote' from Dr Johnson about his work with his 'U-Boot':
>
> ...that has taken eighteen hours of every day for the last ten years.
> My mother died; I hardly noticed. My father cut off his head and fried it
> in garlic in the hope of attracting my attention; I scarcely looked up from
> my work.

Don't worry - I'm doing far less U-Boot coding now than I was a year ago
and probably more than I'll be doing in another year. At least when I do
stop, I can at least be happy that I made the contributions that I did.

I would at least like to see timers and init resolved before I hang up my
hat ;)

Regards,

Graeme

Patch

diff --git a/arch/x86/cpu/sc520/sc520.c b/arch/x86/cpu/sc520/sc520.c
index e37c403..fc2996a 100644
--- a/arch/x86/cpu/sc520/sc520.c
+++ b/arch/x86/cpu/sc520/sc520.c
@@ -53,6 +53,7 @@  int cpu_init_f(void)

 	return x86_cpu_init_f();
 }
+INIT_FUNC(f, 010, cpu_init_f);

 int cpu_init_r(void)
 {
diff --git a/arch/x86/cpu/sc520/sc520_sdram.c b/arch/x86/cpu/sc520/sc520_sdram.c
index f3623f5..08674eb 100644
--- a/arch/x86/cpu/sc520/sc520_sdram.c
+++ b/arch/x86/cpu/sc520/sc520_sdram.c
@@ -57,6 +57,7 @@  int dram_init_f(void)

 	return 0;
 }
+INIT_FUNC(f, 070, dram_init_f);

 static inline void sc520_dummy_write(void)
 {
diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
index fe28030..48b5b99 100644
--- a/arch/x86/cpu/u-boot.lds
+++ b/arch/x86/cpu/u-boot.lds
@@ -57,6 +57,11 @@  SECTIONS
 	__data_end = .;

 	. = ALIGN(4);
+	__initfuncs_f_start = .;
+	.initfuncs_f : { KEEP(*(SORT_BY_NAME(.initfuncs_f*))) }
+	__initfuncs_f_end = .;
+
+	. = ALIGN(4);
 	__bss_start = ABSOLUTE(.);
 	.bss (NOLOAD) : { *(.bss) }
 	. = ALIGN(4);
diff --git a/arch/x86/lib/board.c b/arch/x86/lib/board.c
index b1b8680..de06800 100644
--- a/arch/x86/lib/board.c
+++ b/arch/x86/lib/board.c
@@ -64,6 +64,8 @@  extern ulong __rel_dyn_start;
 extern ulong __rel_dyn_end;
 extern ulong __bss_start;
 extern ulong __bss_end;
+extern ulong __initfuncs_f_start;
+extern ulong __initfuncs_f_end;

 /************************************************************************
  * Init Utilities							*
@@ -83,6 +85,7 @@  static int init_baudrate (void)

 	return (0);
 }
+INIT_FUNC(f, 040, init_baudrate);

 static int display_banner (void)
 {
@@ -152,22 +155,6 @@  static int copy_uboot_to_ram(void);
 static int clear_bss(void);
 static int do_elf_reloc_fixups(void);

-init_fnc_t *init_sequence_f[] = {
-	cpu_init_f,
-	board_early_init_f,
-	env_init,
-	init_baudrate,
-	serial_init,
-	console_init_f,
-	dram_init_f,
-	calculate_relocation_address,
-	copy_uboot_to_ram,
-	clear_bss,
-	do_elf_reloc_fixups,
-
-	NULL,
-};
-
 init_fnc_t *init_sequence_r[] = {
 	cpu_init_r,		/* basic cpu dependent setup */
 	board_early_init_r,	/* basic board dependent setup */
@@ -201,6 +188,7 @@  static int calculate_relocation_address(void)

 	return 0;
 }
+INIT_FUNC(f, 080, calculate_relocation_address);

 static int copy_uboot_to_ram(void)
 {
@@ -213,6 +201,7 @@  static int copy_uboot_to_ram(void)

 	return 0;
 }
+INIT_FUNC(f, 090, copy_uboot_to_ram);

 static int clear_bss(void)
 {
@@ -227,6 +216,7 @@  static int clear_bss(void)

 	return 0;
 }
+INIT_FUNC(f, 100, clear_bss);

 static int do_elf_reloc_fixups(void)
 {
@@ -241,18 +231,22 @@  static int do_elf_reloc_fixups(void)

 	return 0;
 }
+INIT_FUNC(f, 110, do_elf_reloc_fixups);

 /* Load U-Boot into RAM, initialize BSS, perform relocation adjustments */
 void board_init_f(ulong boot_flags)
 {
-	init_fnc_t **init_fnc_ptr;
-
 	gd->flags = boot_flags;

-	for (init_fnc_ptr = init_sequence_f; *init_fnc_ptr; ++init_fnc_ptr) {
-		if ((*init_fnc_ptr)() != 0)
+	init_fnc_t *init_fnc_ptr = (init_fnc_t *)(&__initfuncs_f_start);
+	init_fnc_t *init_func_end = (init_fnc_t *)(&__initfuncs_f_end);
+
+	do {
+		if ((init_fnc_ptr)() != 0)
 			hang();
-	}
+
+	} while (init_fnc_ptr++ < init_func_end);
+

 	gd->flags |= GD_FLG_RELOC;

diff --git a/board/eNET/eNET.c b/board/eNET/eNET.c
index 2a5636c..2167df9 100644
--- a/board/eNET/eNET.c
+++ b/board/eNET/eNET.c
@@ -106,6 +106,7 @@  int board_early_init_f(void)

 	return 0;
 }
+INIT_FUNC(f, 020, board_early_init_f);

 static void enet_setup_pars(void)
 {
diff --git a/common/console.c b/common/console.c
index 8c650e0..559c799 100644
--- a/common/console.c
+++ b/common/console.c
@@ -531,6 +531,7 @@  int console_init_f(void)

 	return 0;
 }
+INIT_FUNC(f, 060, console_init_f);

 void stdio_print_current_devices(void)
 {
diff --git a/common/env_flash.c b/common/env_flash.c
index 50ca4ffa..a63c108 100644
--- a/common/env_flash.c
+++ b/common/env_flash.c
@@ -126,6 +126,7 @@  int  env_init(void)

 	return 0;
 }
+INIT_FUNC(f, 030, env_init);

 #ifdef CMD_SAVEENV
 int saveenv(void)
@@ -252,6 +253,7 @@  int  env_init(void)
 	gd->env_valid = 0;
 	return 0;
 }
+INIT_FUNC(f, 030, env_init);

 #ifdef CMD_SAVEENV

diff --git a/common/serial.c b/common/serial.c
index 995d268..5d097d8 100644
--- a/common/serial.c
+++ b/common/serial.c
@@ -168,6 +168,7 @@  int serial_init (void)

 	return serial_current->init ();
 }
+INIT_FUNC(f, 050, serial_init);

 void serial_setbrg (void)
 {
diff --git a/include/common.h b/include/common.h
index 12a1074..61126f1 100644
--- a/include/common.h
+++ b/include/common.h
@@ -39,8 +39,16 @@  typedef volatile unsigned char	vu_char;
 #include <linux/bitops.h>
 #include <linux/types.h>
 #include <linux/string.h>
+#include <linux/compiler.h>
 #include <asm/ptrace.h>
 #include <stdarg.h>
+
+typedef int (*initfunc_t)(void);
+
+#define INIT_FUNC(stage,step,fn) \
+	static initfunc_t __initfunc_ ## fn ## stage __used \
+	__attribute__((__section__(".initfuncs_" #stage "." #step))) = fn
+
 #if defined(CONFIG_PCI) && (defined(CONFIG_4xx) && !defined(CONFIG_AP1000))
 #include <pci.h>
 #endif