| Message ID | 1313587343-3693-1-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 >
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
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