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