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

[Graeme Russ]

Hi Wolfgang,

On Tue, Aug 23, 2011 at 9:49 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButC+W1ekU6XmaW5FvVOFc6Y4bhchVWvFGBV_FQPw=pUSQdA at 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