From mboxrd@z Thu Jan  1 00:00:00 1970
From: Graeme Russ <graeme.russ@gmail.com>
Date: Wed, 24 Aug 2011 15:57:27 +1000
Subject: [U-Boot] [RFC] New init sequence processing without
	init_sequence array
In-Reply-To: <20110824053849.2317F11F9E75@gemini.denx.de>
References: <1313587343-3693-1-git-send-email-graeme.russ@gmail.com>
	<20110822201023.0F8A111EF9D9@gemini.denx.de>
	<CALButC+W1ekU6XmaW5FvVOFc6Y4bhchVWvFGBV_FQPw=pUSQdA@mail.gmail.com>
	<20110823114920.AD2ED201520@gemini.denx.de>
	<CALButCJg5BPP_Z0VUMEfQgjpRYO=WDQBvvBFho6VbNovbRjFzQ@mail.gmail.com>
	<20110824053849.2317F11F9E75@gemini.denx.de>
Message-ID: <CALButCLX42Q=u33gCOKA+LoZZ+RZO1Y-SXNhE8MK66DfrU9YPA@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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