[U-Boot] [PATCH 1/3] init_func: Add fundamental framework

[Graeme Russ]

Hi Marek,

On Thu, May 3, 2012 at 1:18 PM, Marek Vasut <marex at denx.de> wrote:
> Dear Graeme Russ,
>
>> Hi Marek,
>>
>> Thanks for taking a look
>>
>> >> +The INIT_FUNC macro allows initialisation functions (i.e. functions
>> >> which are +executed before the main loop) to be easily added to the
>> >> init sequence +
>> >> +
>> >> +Specifying an Initialisation Function and is Dependencies
>> >> +---------------------------------------------------------
>> >> +The format of the INIT_FUNC macro is:
>> >> +
>> >> +INIT_FUNC(fn, grp, man_reqs, pre_reqs, pst_reqs)
>> >> +
>> >> +fn is the name of the init function to call. This function must have
>> >> the +following prototype:
>> >> +
>> >> +int foo(void);
>> >
>> > What if I want to pass some data to such a func ? Clearly, I can think of
>> > this being doable, but extra hard.
>>
>> The idea is that no changes are being made to the existing init
>> methodology.
>
> Well ... what if I want to pass eg. pdata?

Do what we do now - Global Data is the only way to pass data between
initialisation functions. Sure, this _might_ change in the future (but I
doubt it) but we can deal with that later

>> >> +
>> >> +Each init function must return 0 to indicate success - any other return
>> >> value +indicates failure and the init sequence will stop
>> >> +
>> >> +grp is the name of the group that the init function belongs to. grp may
>> >> be +the same as fn for any individual init function, but between init
>> >> functions, +fn and grp must be unique.
>> >> +
>> >> +The purpose of groups is to allow functions to be grouped together so
>> >> other +functions can specify the group as a whole as a dependency rather
>> >> than having +to list every function in the group in the dependency list
>> >> +
>> >> +man_reqs is a space seperated list of functions or groups that MUST
>> >> exist and +MUST run BEFORE fn
>> >> +
>> >> +pre_reqs is a space seperated list of functions or groups that MAY
>> >> exist and +(if they do) MUST run BEFORE fn
>> >> +
>> >> +pst_reqs is a space seperated list of functions or groups that MAY
>> >> exist and +(if they do) MUST run AFTER fn
>> >
>> > What's the point? Can't you create a kind of proxy object that the
>> > pst_reqs will have as a pre_req ?
>> >
>> > Maybe you should create this:
>> >
>> > INIT_FUNC(fn, grp, prereqs, postreqs) and for each function from prereqs
>> > and postreqs, specify per-function attributes via the GCC
>> > __attribute__(()) directive, like if the function must run before
>> > something or may run before something etc?
>>
>> Eep, I would like to see that implemented - I'm sure it would be
>> 'interesting'
>
> Pervy and sadistic at least :-)
>
>> The way INIT_FUNC and friends work is to simply build a list of static
>> strings (which we just happen to shove in a seperate section so they can
>> be filtered in/out based on the link stage)
>
> Yes, I got the understanding of it. I was more bothered by the man_reqs and
> pre_reqs, what if we soon need functions that are executed under another weird
> condition? And if you look at it the other way -- if you need function that's
> executed conditionally, why not wrap the condition into the pre_req (or some
> proxy object, whatever).

It's not about being executed conditionally - It's about 'I need to be
initialised after 'X', but if 'X' does not exist, I can still initialise'

Network is one example - If you have a PCI network adapter, it must be
intialised after PCI. But if the adapter is not PCI, you still want to be
able to initialise it

Timer is another - Hopefully the timer infrastructure will soon be an arch
independent API. But each arch (and potentially down to the board level)
may have a role to play in getting the timer infrastucture running (FPGA,
PLL, PIT intialiastion). So potentially, the common driver code would be
intialised with:

INIT_FUNC(timer_api, timer, arch_timer, board_timer, );

So the arch_timer function MUST exist and it MUST run before timer_api. The
board specific timer initialisation is optional. If the board has timer
related code, it MUST run before timer_api but timer_api can still run if
board_timer does not exist (i.e. all the timer init done at the arch level)

Now depending on the arch and board, board_timer may be either:

INIT_FUNC(board_timer, timer, arch_timer, , timer_api);

i.e. arch -> board -> api

or

INIT_FUNC(board_timer, timer, , , arch_timer timer_api);

i.e. board -> arch -> api

NOTE: In both of the above example, including timer_api as a post-req
is redundant. The timer_api INIT_FUNC definition already mandated the
order. This is not a problem and including or excluding it makes no
difference to the output. Of course, including timer_api in the above
examples means we could have done:

INIT_FUNC(timer_api, timer, arch_timer, , );

But adding the redundant references makes it clearer when you are looking
at that bit of code. The 'black-box' tool will warn you if you created a
circular reference and will print out what the init sequence is that
created the circular reference.

Now, here is where the fun begins :) - Lets say you need timer support
early, but you can't get the low level infrastructure for hi-res timers
running until later...

board/vendor_me/common/timer.c
INIT_FUNC(me_timer_init, me_late_timer, me_pit_init, , );
INIT_FUNC(me_pit_init, me_late_timer, , fpga_init pll_init, me_timer_init);

(again, me_late_timer in the second INIT_FUNC is redundant)

board/vendor_me/board_a/timer.c
INIT_FUNC(fpga_init, me_late_timer, , , me_pit_init);

board/vendor_me/board_b/timer.c
INIT_FUNC(pll_init, me_late_timer, , , me_pit_init);

(again, me_pit_init in these INIT_FUNCs is redundant)

board/vendor_me/board_c/timer.c
/*
 * This board has the PIT hard-wired to a crystal oscillator so there
 * is now board_level init function - So we can actually initialise
 * the PIT earlier :)
 */
int me_pit_early_init(void)
{
        return me_pit_init();
}
INIT_FUNC(me_pit_early_init, timer, arch_timer, , );

/*
 * Don't need any late timer init - This will skip both me_timer_init and
 * me_pit_init as they have been defined in the me_late_timer group
 */
INIT_SKIP(me_late_timer);

The idea is that me_late_timer() will perform some tweak to the timer API
to redirect it to the hi-res clock source. The hi-res clock source is
driven by a PIT common to all the boards manufacture by this vendor. But
the PIT has a different raw clock source depending on a particular board
Board A has an FPGA, board B has a PLL and board C uses a hard-wired
crystal oscillator

>> >> +
>> >> +Skipping or Replacing a Function or Group
>> >> +-----------------------------------------
>> >> +Occassionally, a board may provide a completely seperate implementation
>> >> for +an initialisation function that is provided in the common arch, SoC
>> >> or +common code.
>> >> +
>> >> +SKIP_INIT(fn_or_group)
>> >> +
>> >> +After the initialisation function dependencies are calculated, all
>> >> functions +and groups listed in any SKIP_INITs are removed - This may
>> >> result in +dependent functions being removed - It is up to the board
>> >> code developer +to ensure suitable replacements are in place
>> >> +
>> >> +REPLACE_INIT(old_fn_or_group, new_fn_or_group)
>> >> +
>> >> +Like SKIP_INIT but replaces on function with another (or one group with
>> >> +another)
>> >> +
>> >> +Example: In the SoC code yoy may have
>> >
>> > Yoy :)
>>
>> Yes. The ultimate goal is to remove a heap of '#ifdef mess' and delegate
>> the selection of initialiasation functions to where is is most appropriate.
>> CPU init in ARCH code with board specific init in board code with the
>> ARCH code 100% unaware of what the board is doing. This will also make it
>> a lot easier for vendors to implement multi-arch solutions :)
>
> Correct, alongside kbuild and driver model, we'll make an awesome bootloader.
> But fix that s/yoy/you/ please ;-)

Ah, I see now... Will do

>> >> +
>> >> +INIT_FUNC(init_cpu_f, RESET, , , );
>> >> +
>> >> +In the board code, you may want a slightly tweaked version, so you
>> >> might +have:
>> >> +
>> >> +int my_new_init_cpu_f(void)
>> >> +{
>> >> +     ...
>> >> +}
>> >> +REPLACE_INIT(init_cpu_f, my_new_init_cpu_f);
>>
>> [snip]
>>
>> >> diff --git a/tools/mkinitseq.c b/tools/mkinitseq.c
>> >> new file mode 100644
>> >> index 0000000..b150de4
>> >> --- /dev/null
>> >> +++ b/tools/mkinitseq.c
>> >> @@ -0,0 +1,1512 @@
>> >
>> > Ok, this is the worst part. I think this could be reimplemented in shell
>> > ;-)
>>
>> Be my guest :) - Have a really good look at what is happening behind the
>> scenes - Lots of list processing and checking. In particular, the
>> processing of the REPLACE and SKIP macros would be particularly nasty.
>
> That's what sed can do for you, can't it ?

So below - If you can show me (i.e. write the whole thing in shell) I might
include it

>> I have no problem with this being done in shell code. BUT if I have to do
>> so in order for it to be accepted, then I'll bin this patch series right
>> now. I'm more than happy to integrate somebody elses shell-based tool into
>> the series. Sorry to be so blunt, but I do not have the time or energy to
>> re-implement this myself.
>
> Ooh, I smell flame-fuel :-) I'd certainly prefer to see this in shell, since I
> believe the substitutions and reordering of text might be easier there.

See above - be my guest ;)

>> My argument is that this is a black-box tool like gcc/ld. Once we prove it
>> to do what it does correctly, we can 'leave it alone'(tm) and test cases
>> are fairly trivial. You can even mock-up the input file in vi :)
>
> Ewwwwww ... like gcc/ld ... or maybe like Windows (TM)(C)(R)(?) :-D

No, like FLOSS gcc/ld :P

>> And is a shell based implementation going to be any easier to understand,
>> modify, debug, etc?
>
> I wonder :-)

Well, if someone manages to do it in shell, we will see - Take a look at
main() - I have been very carefull to layout the way the tool operates in
a very linear way but ther lists are global, so sharing data between the
shell stages will be a pain (pipes?)

>> >> +/*
>> >> + * (C) Copyright 2012
>> >> + * Graeme Russ <graeme.russ at gmail.com>
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or
>> >> + * modify it under the terms of the GNU General Public License as
>> >> + * published by the Free Software Foundation; either version 2 of
>> >> + * the License, or (at your option) any later version.
>> >> + *
>> >> + * This program is distributed in the hope that it will be useful,
>> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >> + * GNU General Public License for more details.
>> >> + *
>> >> + * You should have received a copy of the GNU General Public License
>> >> + * along with this program; if not, write to the Free Software
>> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> >> + * MA 02111-1307 USA
>> >> + */
>> >> +
>> >> +/**
>> >> + * container_of - cast a member of a structure out to the containing
>> >> structure + * @ptr:   the pointer to the member.
>> >> + * @type:    the type of the container struct this is embedded in.
>> >> + * @member:  the name of the member within the struct.
>> >> + *
>> >> + */
>> >> +#define container_of(ptr, type, member) ({                   \
>> >> +     const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
>> >> +     (type *)( (char *)__mptr - offsetof(type,member) );})
>> >
>> > Don't we already have this defined in uboot ?
>>
>> mkinitseq is host-code so it only brings in host headers (not U-Boot
>> headers) - If you don't have Linux kernel headers installed, you can't
>> get this macro. Actually, even though I do have them, I had a hard time
>> getting this macro in, so I gave up - Any hints?
>
> Reimplement this thing in shell :-)

You already know my answer to this ;)

>> As an aside, I wonder if the kernel headers are required for the list
>> macros?
>
> Nope.

Good :)

Regards,

Graeme