| Message ID | 1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Rejected, archived |
| Headers | show |
Dear Joakim Tjernlund, In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote: > init_sequence is an array with function pointers which > are really hard to follow when you need to debug this area. > Turn it into plain function calls instead which makes > the code a bit uglier but I find the simpler debugging > much more valuable. This is indeed much uglier. What exactly is your problem with debugging the existing code? Best regards, Wolfgang Denk
Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47: > > Dear Joakim Tjernlund, > > In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote: > > init_sequence is an array with function pointers which > > are really hard to follow when you need to debug this area. > > Turn it into plain function calls instead which makes > > the code a bit uglier but I find the simpler debugging > > much more valuable. > > This is indeed much uglier. What exactly is your problem with > debugging the existing code? Whenever I screw up so that one of the init funcs crashes, often without any trace on the RS232 port you don't know which one. Single stepping though the loop is cumbersome and not easy as BDI tends to flush the cache when it stops so you loose your stack. The other way is to look up one those funs and set a BP there and hope for the best. Then repeat with the next function and so on. Compare that with just setting a BP in the new init_sequence(), it is fast and easy to move around. I don't think you have been chasing bugs in this area for a long time, if you had, you would appreciate how easy it is with functions compared with a bunch of function ptrs. I don't think this is much uglier, just a bit, but far more useful and I have a hard time buying into "beautiful trumps usefulness". Jocke
On Mon, 6 Dec 2010 21:42:28 +0100 Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47: > > > > Dear Joakim Tjernlund, > > > > In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote: > > > init_sequence is an array with function pointers which > > > are really hard to follow when you need to debug this area. > > > Turn it into plain function calls instead which makes > > > the code a bit uglier but I find the simpler debugging > > > much more valuable. > > > > This is indeed much uglier. What exactly is your problem with > > debugging the existing code? > > Whenever I screw up so that one of the init funcs crashes, often without > any trace on the RS232 port you don't know which one. Single stepping > though the loop is cumbersome and not easy as BDI tends to > flush the cache when it stops so you loose your stack. > The other way is to look up one those funs and set a BP there and hope > for the best. Then repeat with the next function and so on. > Compare that with just setting a BP in the new init_sequence(), it is > fast and easy to move around. > > I don't think you have been chasing bugs in this area for a long time, > if you had, you would appreciate how easy it is with functions > compared with a bunch of function ptrs. > I don't think this is much uglier, just a bit, but far more > useful and I have a hard time buying into "beautiful trumps usefulness". I think it's easier with the function pointers -- if you want to debug a hang in that phase of the boot, just have the loop print the address of each function before it calls it. -Scott
Dear Joakim Tjernlund, In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote: > > > This is indeed much uglier. What exactly is your problem with > > debugging the existing code? > > Whenever I screw up so that one of the init funcs crashes, often without > any trace on the RS232 port you don't know which one. Single stepping Well, we turn on the serial ports as early as possible, so ther eis only a pretty small collection that is candidate for such a situation. > though the loop is cumbersome and not easy as BDI tends to > flush the cache when it stops so you loose your stack. Does it? On which architecture / processor is this? > The other way is to look up one those funs and set a BP there and hope > for the best. Then repeat with the next function and so on. > Compare that with just setting a BP in the new init_sequence(), it is > fast and easy to move around. ...but ugly to read. And if you really want to introduce this style, it has to be done for all architectures, as I want to see this code to become common across architectures. > I don't think you have been chasing bugs in this area for a long time, > if you had, you would appreciate how easy it is with functions > compared with a bunch of function ptrs. I haven't, lately, that's true, or when I had, I always knew pretty well where I had to expect the issues. But I've been there before, many, many times. > I don't think this is much uglier, just a bit, but far more > useful and I have a hard time buying into "beautiful trumps usefulness". I'm not convinced that your code is actually a general improvement. I understand that it's useful for you, and your style of debugging. If you are really hanging in this area, then maybe a debug() is more prowerfull - and less invasive. Best regards, Wolfgang Denk
On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: > On Mon, 6 Dec 2010 21:42:28 +0100 > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > >> Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47: >> > >> > Dear Joakim Tjernlund, >> > >> > In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote: >> > > init_sequence is an array with function pointers which >> > > are really hard to follow when you need to debug this area. >> > > Turn it into plain function calls instead which makes >> > > the code a bit uglier but I find the simpler debugging >> > > much more valuable. >> > >> > This is indeed much uglier. What exactly is your problem with >> > debugging the existing code? >> >> Whenever I screw up so that one of the init funcs crashes, often without >> any trace on the RS232 port you don't know which one. Single stepping >> though the loop is cumbersome and not easy as BDI tends to >> flush the cache when it stops so you loose your stack. >> The other way is to look up one those funs and set a BP there and hope >> for the best. Then repeat with the next function and so on. >> Compare that with just setting a BP in the new init_sequence(), it is >> fast and easy to move around. >> >> I don't think you have been chasing bugs in this area for a long time, >> if you had, you would appreciate how easy it is with functions >> compared with a bunch of function ptrs. >> I don't think this is much uglier, just a bit, but far more >> useful and I have a hard time buying into "beautiful trumps usefulness". > > I think it's easier with the function pointers -- if you want to debug > a hang in that phase of the boot, just have the loop print the address > of each function before it calls it. I agree, but you can't print the address before you have console output. I notice that console_init_f() can be up to 13th in the list of initialisation functions - How often is that the case? There seems to be a lot of SDRAM initialisation prior to getting console output which, to me, seems a little strange - surely console output can be achieved earlier (even if it is using a hard-coded baud rate) I've now got x86 Cache-As-RAM working and console output is in about position four in the init sequence - from there I debug using printf() I don't think it will be long before board_init_f() gets unified into common code and I would hate to see this uglyness spread to all arches. Regards, Graeme
On Tue, 7 Dec 2010 09:36:40 +1100 Graeme Russ <graeme.russ@gmail.com> wrote: > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: > > I think it's easier with the function pointers -- if you want to debug > > a hang in that phase of the boot, just have the loop print the address > > of each function before it calls it. > > I agree, but you can't print the address before you have console output. It's usually not too hard to hack something together, even if it's too early for normal console output -- but I'd expect most problems to be either before the initialization list, or after the console is working. > I notice that console_init_f() can be up to 13th in the list of initialisation > functions - How often is that the case? There seems to be a lot of SDRAM > initialisation prior to getting console output which, to me, seems a little > strange - surely console output can be achieved earlier (even if it is using > a hard-coded baud rate) I don't see "a lot of SDRAM initialization" -- there's adjust_sdram_tbs_8xx, but that's really just setting up a couple clock registers (and it's only for 8xx). It's not the real SDRAM init. -Scott
On Tue, Dec 7, 2010 at 9:49 AM, Scott Wood <scottwood@freescale.com> wrote: > On Tue, 7 Dec 2010 09:36:40 +1100 > Graeme Russ <graeme.russ@gmail.com> wrote: > >> On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: >> > I think it's easier with the function pointers -- if you want to debug >> > a hang in that phase of the boot, just have the loop print the address >> > of each function before it calls it. >> >> I agree, but you can't print the address before you have console output. > > It's usually not too hard to hack something together, even if it's too > early for normal console output -- but I'd expect most problems to be > either before the initialization list, or after the console is working. > >> I notice that console_init_f() can be up to 13th in the list of initialisation >> functions - How often is that the case? There seems to be a lot of SDRAM >> initialisation prior to getting console output which, to me, seems a little >> strange - surely console output can be achieved earlier (even if it is using >> a hard-coded baud rate) > > I don't see "a lot of SDRAM initialization" -- there's > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock > registers (and it's only for 8xx). It's not the real SDRAM init. And sdram_adjust_866() - True, not 'a lot' But then there is dpram_init() as well which does not look like a pre-req to console Regards, Graeme
On Tue, Dec 7, 2010 at 9:36 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Joakim Tjernlund, > > In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote: >> [snip] >> The other way is to look up one those funs and set a BP there and hope >> for the best. Then repeat with the next function and so on. >> Compare that with just setting a BP in the new init_sequence(), it is >> fast and easy to move around. > > ...but ugly to read. And if you really want to introduce this style, > it has to be done for all architectures, as I want to see this code to > become common across architectures. > Moving away from the array based init sequence also promotes the posibility to break away from the very strict function prototype (void parameter returning int) which will lead to even more ugliness in the future Regards, Graeme
Dear Graeme Russ, In message <AANLkTikH3O9gZWsuEaKn-BK+8iAhUeRyk2iL+ezZz-y1@mail.gmail.com> you wrote: > > I agree, but you can't print the address before you have console output. I > notice that console_init_f() can be up to 13th in the list of initialisation > functions - How often is that the case? There seems to be a lot of SDRAM > initialisation prior to getting console output which, to me, seems a little No, this is only on broken system that don't play by the rules. See http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#6_Keep_it_Debuggable Board / architecture maintainers that ignore the design principles deserve some punishment ;-) > I don't think it will be long before board_init_f() gets unified into common > code and I would hate to see this uglyness spread to all arches. Full ACK. Best regards, Wolfgang Denk
On Tue, 7 Dec 2010 10:04:23 +1100 Graeme Russ <graeme.russ@gmail.com> wrote: > And sdram_adjust_866() - True, not 'a lot' > > But then there is dpram_init() as well which does not look like a > pre-req to console DPRAM is not general purpose memory -- it actually is used by the serial hardware. And dpram_init() is virtually too simple to fail -- just setting two gd_t fields to constants. -Scott
Dear Graeme Russ, In message <AANLkTimuPTiR6USC7k9srnGtSXYep+0DxGz9FWHVsgJd@mail.gmail.com> you wrote: > > And sdram_adjust_866() - True, not 'a lot' > > But then there is dpram_init() as well which does not look like a > pre-req to console It is. On some 8xx system you have to load microcode and thus need to initialize the DPRAM before you can configure the console port. Best regards, Wolfgang Denk
Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04: > > On Tue, 7 Dec 2010 09:36:40 +1100 > Graeme Russ <graeme.russ@gmail.com> wrote: > > > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: > > > I think it's easier with the function pointers -- if you want to debug > > > a hang in that phase of the boot, just have the loop print the address > > > of each function before it calls it. > > > > I agree, but you can't print the address before you have console output. > > It's usually not too hard to hack something together, even if it's too > early for normal console output -- but I'd expect most problems to be > either before the initialization list, or after the console is working. > > > I notice that console_init_f() can be up to 13th in the list of initialisation > > functions - How often is that the case? There seems to be a lot of SDRAM > > initialisation prior to getting console output which, to me, seems a little > > strange - surely console output can be achieved earlier (even if it is using > > a hard-coded baud rate) > > I don't see "a lot of SDRAM initialization" -- there's > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock > registers (and it's only for 8xx). It's not the real SDRAM init. Scott, listen to yourself. You are proposing that one should turn the code inside out and scan the map files just do this simple thing. What is so valuable with func ptrs that you think it is worth it? Jocke
On Tue, 7 Dec 2010 01:07:30 +0100 Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04: > > > > On Tue, 7 Dec 2010 09:36:40 +1100 > > Graeme Russ <graeme.russ@gmail.com> wrote: > > > > > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: > > > > I think it's easier with the function pointers -- if you want to debug > > > > a hang in that phase of the boot, just have the loop print the address > > > > of each function before it calls it. > > > > > > I agree, but you can't print the address before you have console output. > > > > It's usually not too hard to hack something together, even if it's too > > early for normal console output -- but I'd expect most problems to be > > either before the initialization list, or after the console is working. > > > > > I notice that console_init_f() can be up to 13th in the list of initialisation > > > functions - How often is that the case? There seems to be a lot of SDRAM > > > initialisation prior to getting console output which, to me, seems a little > > > strange - surely console output can be achieved earlier (even if it is using > > > a hard-coded baud rate) > > > > I don't see "a lot of SDRAM initialization" -- there's > > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock > > registers (and it's only for 8xx). It's not the real SDRAM init. > > Scott, listen to yourself. You are proposing that one should turn the > code inside out and scan the map files just do this simple thing. It looks like you're the one turning this code inside out. :-) No need to scan anything yourself; let the tools (gdb/addr2line/etc) do it. > What is so valuable with func ptrs that you think it is worth it? As I said, I think it's at least as easy to debug the way it is. And you admitted you found the new way uglier... Plus, maybe someday we'll get real section-list initfuncs. -Scott
Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 23:36:21: > > Dear Joakim Tjernlund, > > In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote: > > > > > This is indeed much uglier. What exactly is your problem with > > > debugging the existing code? > > > > Whenever I screw up so that one of the init funcs crashes, often without > > any trace on the RS232 port you don't know which one. Single stepping > > Well, we turn on the serial ports as early as possible, so ther eis > only a pretty small collection that is candidate for such a situation. But that won't help much. When you don't have any output it can be any reason. I really want to see where it breaks down. That will tell me more about what I did wrong earlier. It is easy to guess where when you a playing with these function, but when you are trying to understand why your latest relocation fixes doesn't work, it is a different game. > > > though the loop is cumbersome and not easy as BDI tends to > > flush the cache when it stops so you loose your stack. > > Does it? On which architecture / processor is this? MPC8321 and BDI2000. I have to play games with some internal BDI command called SAP. Maybe they have fixed this in later models? > > > The other way is to look up one those funs and set a BP there and hope > > for the best. Then repeat with the next function and so on. > > Compare that with just setting a BP in the new init_sequence(), it is > > fast and easy to move around. > > ...but ugly to read. And if you really want to introduce this style, > it has to be done for all architectures, as I want to see this code to > become common across architectures. That is the next issue, lets not stray from the path .. > > > I don't think you have been chasing bugs in this area for a long time, > > if you had, you would appreciate how easy it is with functions > > compared with a bunch of function ptrs. > > I haven't, lately, that's true, or when I had, I always knew pretty > well where I had to expect the issues. But I've been there before, > many, many times. And you never found it annoying when you hit these function ptrs? > > > I don't think this is much uglier, just a bit, but far more > > useful and I have a hard time buying into "beautiful trumps usefulness". > > I'm not convinced that your code is actually a general improvement. I > understand that it's useful for you, and your style of debugging. > If you are really hanging in this area, then maybe a debug() is more > prowerfull - and less invasive. Now you are proposing that I instrument the code, much like Scott and his print of address idea. This is exactly what I want to avoid as it would probably have to be adapted from case to case and there is always the case when there is no output and then I am back to square zero again. The argument that it is uglier alone is not a very good one. Image the roles were reversed, I am sure you would dismiss me as "less beautiful" is really not an argument alone. Jocke
Scott Wood <scottwood@freescale.com> wrote on 2010/12/07 01:21:44: > > On Tue, 7 Dec 2010 01:07:30 +0100 > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > > > Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04: > > > > > > On Tue, 7 Dec 2010 09:36:40 +1100 > > > Graeme Russ <graeme.russ@gmail.com> wrote: > > > > > > > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote: > > > > > I think it's easier with the function pointers -- if you want to debug > > > > > a hang in that phase of the boot, just have the loop print the address > > > > > of each function before it calls it. > > > > > > > > I agree, but you can't print the address before you have console output. > > > > > > It's usually not too hard to hack something together, even if it's too > > > early for normal console output -- but I'd expect most problems to be > > > either before the initialization list, or after the console is working. > > > > > > > I notice that console_init_f() can be up to 13th in the list of initialisation > > > > functions - How often is that the case? There seems to be a lot of SDRAM > > > > initialisation prior to getting console output which, to me, seems a little > > > > strange - surely console output can be achieved earlier (even if it is using > > > > a hard-coded baud rate) > > > > > > I don't see "a lot of SDRAM initialization" -- there's > > > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock > > > registers (and it's only for 8xx). It's not the real SDRAM init. > > > > Scott, listen to yourself. You are proposing that one should turn the > > code inside out and scan the map files just do this simple thing. > > It looks like you're the one turning this code inside out. :-) Some days it feels so :) > > No need to scan anything yourself; let the tools (gdb/addr2line/etc) do > it. gdb yes, addr2line/etc no. Needing to resort to external tools when gdb and a sensible code layout would do the trick? No thanks. > > > What is so valuable with func ptrs that you think it is worth it? > > As I said, I think it's at least as easy to debug the way it is. And > you admitted you found the new way uglier... You haven't tried my way yet. Suppose we littered the code with function ptrs, would still feel it is easier using addr2line/etc each and every function call? > > Plus, maybe someday we'll get real section-list initfuncs. One day that hasn't come fore years yet and may never come and probably fixable with weak functions.
On Tue, 7 Dec 2010 01:41:22 +0100 Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > Scott Wood <scottwood@freescale.com> wrote on 2010/12/07 01:21:44: > > > > On Tue, 7 Dec 2010 01:07:30 +0100 > > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > > > Scott, listen to yourself. You are proposing that one should turn the > > > code inside out and scan the map files just do this simple thing. > > > > It looks like you're the one turning this code inside out. :-) > > Some days it feels so :) > > > > > No need to scan anything yourself; let the tools (gdb/addr2line/etc) do > > it. > > gdb yes, addr2line/etc no. Needing to resort to external tools > when gdb and a sensible code layout would do the trick? No thanks. gdb is an external tool for me most of the time. :-) It's what I use for lookup anyway (usually I'll want more context), I just mentioned addr2line as an alternative. Either one beats looking at the map file (why?). > > > What is so valuable with func ptrs that you think it is worth it? > > > > As I said, I think it's at least as easy to debug the way it is. And > > you admitted you found the new way uglier... > > You haven't tried my way yet. I usually don't even have a BDI hooked up. :-P I've done breakpoint/step debugging. Sometimes it's the right tool. Usually printf works better for me, particularly if there's overhead to setting up a debugger. > > Plus, maybe someday we'll get real section-list initfuncs. > > One day that hasn't come fore years yet and may never come I can still hope. :-) > and probably fixable with weak functions. Weak functions are not a good substitute. -Scott
Dear Joakim Tjernlund, In message <OF9FF97B1B.1FC2BD05-ONC12577F2.0000D034-C12577F2.0002F125@transmode.se> you wrote: > > > > though the loop is cumbersome and not easy as BDI tends to > > > flush the cache when it stops so you loose your stack. > > > > Does it? On which architecture / processor is this? > > MPC8321 and BDI2000. I have to play games with some internal > BDI command called SAP. Maybe they have fixed this in later models? Are you using the latest firmware, i. . 1.31 from October 2009? > > ...but ugly to read. And if you really want to introduce this style, > > it has to be done for all architectures, as I want to see this code to > > become common across architectures. > > That is the next issue, lets not stray from the path .. Right, that's the next issue, lets not stray from the path and put additional road blocks into our way when we already know where we are heading to. > > I haven't, lately, that's true, or when I had, I always knew pretty > > well where I had to expect the issues. But I've been there before, > > many, many times. > > And you never found it annoying when you hit these function ptrs? Actually, no. In an earlier life I've been programming in Forth for some time, so I find a list of addresses to process quite natural and efficient. > Now you are proposing that I instrument the code, much like Scott and his > print of address idea. This is exactly what I want to avoid as it would > probably have to be adapted from case to case and there is always the > case when there is no output and then I am back to square zero again. Don't you use oneliners in the shell that you just make up as needed and then throw away, and reinvent when you need them again? Same here. Adding a printf() or similar to some code under test is something I do several times a day. Even when running under a debugger, and even when I'm not executing a list of function pointers. I see no issue with adding this on a case-by-case base, had-tailored to the specific problem I'm tracking down. > The argument that it is uglier alone is not a very good one. > Image the roles were reversed, I am sure you would dismiss me as > "less beautiful" is really not an argument alone. Ugliness was only one of several arguments, and there were several votes againt yours. Please consider the case closed. I already marked the patch as NAKed. Best regards, Wolfgang Denk
Wolfgang Denk <wd@denx.de> wrote on 2010/12/07 07:34:49: > > Dear Joakim Tjernlund, > > In message <OF9FF97B1B.1FC2BD05-ONC12577F2.0000D034-C12577F2.0002F125@transmode.se> you wrote: > > > > > > though the loop is cumbersome and not easy as BDI tends to > > > > flush the cache when it stops so you loose your stack. > > > > > > Does it? On which architecture / processor is this? > > > > MPC8321 and BDI2000. I have to play games with some internal > > BDI command called SAP. Maybe they have fixed this in later models? > > Are you using the latest firmware, i. . 1.31 from October 2009? No, got an earlier one, 1.24 > > > > ...but ugly to read. And if you really want to introduce this style, > > > it has to be done for all architectures, as I want to see this code to > > > become common across architectures. > > > > That is the next issue, lets not stray from the path .. > > Right, that's the next issue, lets not stray from the path and put > additional road blocks into our way when we already know where we are > heading to. hmm, not sure how to read this. Irony? I was not trying to dodge the subject, just wanted to keep the discussion focused. Of course this can be adapted on all archs. > > > > I haven't, lately, that's true, or when I had, I always knew pretty > > > well where I had to expect the issues. But I've been there before, > > > many, many times. > > > > And you never found it annoying when you hit these function ptrs? > > Actually, no. > > In an earlier life I've been programming in Forth for some time, so I > find a list of addresses to process quite natural and efficient. > > > Now you are proposing that I instrument the code, much like Scott and his > > print of address idea. This is exactly what I want to avoid as it would > > probably have to be adapted from case to case and there is always the > > case when there is no output and then I am back to square zero again. > > Don't you use oneliners in the shell that you just make up as needed > and then throw away, and reinvent when you need them again? > > Same here. Adding a printf() or similar to some code under test is > something I do several times a day. Even when running under a > debugger, and even when I'm not executing a list of function pointers. > I see no issue with adding this on a case-by-case base, had-tailored > to the specific problem I'm tracking down. Yes, this is a useful technique but it costs time. You don't add these printout unless you need them, right? This is my point. If you move to pure function calls you don't have to add them just to identify what function crashes or lookup addresses etc. Just move a BPs around without recompiling and reloading the code. > > > The argument that it is uglier alone is not a very good one. > > Image the roles were reversed, I am sure you would dismiss me as > > "less beautiful" is really not an argument alone. > > Ugliness was only one of several arguments, and there were several > votes againt yours. There was votes against with false claims that is is just as easy to debug function ptrs. And there was Scott which one day hope to have "real section-list initfuncs" Bottom line argument is that the new code is uglier and that trumps any of my arguments. > > Please consider the case closed. I already marked the patch as NAKed. I see, thanks for your time. Jocke
Dear Joakim Tjernlund, In message <OFB493743B.4A802F8E-ONC12577F2.002BC478-C12577F2.003241B1@transmode.se> you wrote: > > > > MPC8321 and BDI2000. I have to play games with some internal > > > BDI command called SAP. Maybe they have fixed this in later models? > > > > Are you using the latest firmware, i. . 1.31 from October 2009? > > No, got an earlier one, 1.24 Well, 1.24 is from April 2006. Information about the MPC8321 must have been pretty limited at that time. Please update. [Let me know if you want me to send you a quote for the latest firmware :-) ] Best regards, Wolfgang Denk
Wolfgang Denk <wd@denx.de> wrote on 2010/12/07 14:07:56: > > Dear Joakim Tjernlund, > > In message <OFB493743B.4A802F8E-ONC12577F2.002BC478-C12577F2.003241B1@transmode.se> you wrote: > > > > > > MPC8321 and BDI2000. I have to play games with some internal > > > > BDI command called SAP. Maybe they have fixed this in later models? > > > > > > Are you using the latest firmware, i. . 1.31 from October 2009? > > > > No, got an earlier one, 1.24 > > Well, 1.24 is from April 2006. Information about the MPC8321 must No surprise there considering we were beta testers of MPC8321. We actually had to nag on Freescale to send Abatron an eval board so we could buy a BDI2000 with MPC832x support :) > have been pretty limited at that time. Please update. [Let me know > if you want me to send you a quote for the latest firmware :-) ] He, I sent an email to Abatron and they already replied with the firmware we are entitled to which is somewhat newer than 1.24 so I will try that one first :) Jocke
diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 765f97a..f0160e6 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -157,7 +157,6 @@ ulong monitor_flash_len; * argument, and returns an integer return code, where 0 means * "continue" and != 0 means "fatal error, hang the system". */ -typedef int (init_fnc_t) (void); /************************************************************************ * Init Utilities * @@ -236,17 +235,17 @@ static int init_func_watchdog_init (void) WATCHDOG_RESET (); return (0); } -# define INIT_FUNC_WATCHDOG_INIT init_func_watchdog_init, +# define INIT_FUNC_WATCHDOG_INIT init_func_watchdog_init() static int init_func_watchdog_reset (void) { WATCHDOG_RESET (); return (0); } -# define INIT_FUNC_WATCHDOG_RESET init_func_watchdog_reset, +# define INIT_FUNC_WATCHDOG_RESET init_func_watchdog_reset() #else -# define INIT_FUNC_WATCHDOG_INIT /* undef */ -# define INIT_FUNC_WATCHDOG_RESET /* undef */ +# define INIT_FUNC_WATCHDOG_INIT 0 /* undef */ +# define INIT_FUNC_WATCHDOG_RESET 0 /* undef */ #endif /* CONFIG_WATCHDOG */ /************************************************************************ @@ -254,76 +253,110 @@ static int init_func_watchdog_reset (void) ************************************************************************ */ -init_fnc_t *init_sequence[] = { +void init_sequence(void) +{ #if defined(CONFIG_MPC85xx) || defined(CONFIG_MPC86xx) - probecpu, + if (probecpu()) + goto err_out; #endif #if defined(CONFIG_BOARD_EARLY_INIT_F) - board_early_init_f, + if (board_early_init_f()) + goto err_out; #endif #if !defined(CONFIG_8xx_CPUCLK_DEFAULT) - get_clocks, /* get CPU and bus clocks (etc.) */ + if (get_clocks()) + goto err_out; /* get CPU and bus clocks (etc.) */ #if defined(CONFIG_TQM8xxL) && !defined(CONFIG_TQM866M) \ && !defined(CONFIG_TQM885D) - adjust_sdram_tbs_8xx, + if (adjust_sdram_tbs_8xx()) + goto err_out; #endif - init_timebase, + if (init_timebase()) + goto err_out; #endif #ifdef CONFIG_SYS_ALLOC_DPRAM #if !defined(CONFIG_CPM2) - dpram_init, + if (dpram_init()) + goto err_out; #endif #endif #if defined(CONFIG_BOARD_POSTCLK_INIT) - board_postclk_init, + if (board_postclk_init()) + goto err_out; #endif - env_init, + if (env_init()) + goto err_out; #if defined(CONFIG_8xx_CPUCLK_DEFAULT) - get_clocks_866, /* get CPU and bus clocks according to the environment variable */ - sdram_adjust_866, /* adjust sdram refresh rate according to the new clock */ - init_timebase, -#endif - init_baudrate, - serial_init, - console_init_f, - display_options, + if (get_clocks_866()) + goto err_out; /* get CPU and bus clocks according to the environment variable */ + if (sdram_adjust_866()) + goto err_out; /* adjust sdram refresh rate according to the new clock */ + if (init_timebase()) + goto err_out; +#endif + if (init_baudrate()) + goto err_out; + if (serial_init()) + goto err_out; + if (console_init_f()) + goto err_out; + if (display_options()) + goto err_out; #if defined(CONFIG_8260) - prt_8260_rsr, - prt_8260_clks, + if (prt_8260_rsr()) + goto err_out; + if (prt_8260_clks()) + goto err_out; #endif /* CONFIG_8260 */ #if defined(CONFIG_MPC83xx) - prt_83xx_rsr, + if (prt_83xx_rsr()) + goto err_out; #endif - checkcpu, + if (checkcpu()) + goto err_out; #if defined(CONFIG_MPC5xxx) - prt_mpc5xxx_clks, + if (prt_mpc5xxx_clks()) + goto err_out; #endif /* CONFIG_MPC5xxx */ #if defined(CONFIG_MPC8220) - prt_mpc8220_clks, + if (prt_mpc8220_clks()) + goto err_out; #endif - checkboard, - INIT_FUNC_WATCHDOG_INIT + if (checkboard()) + goto err_out; + if (INIT_FUNC_WATCHDOG_INIT) + goto err_out; #if defined(CONFIG_MISC_INIT_F) - misc_init_f, + if (misc_init_f()) + goto err_out; #endif - INIT_FUNC_WATCHDOG_RESET + if (INIT_FUNC_WATCHDOG_RESET) + goto err_out; #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) - init_func_i2c, + if (init_func_i2c()) + goto err_out; #endif #if defined(CONFIG_HARD_SPI) - init_func_spi, + if (init_func_spi()) + goto err_out; #endif #ifdef CONFIG_POST - post_init_f, + if (post_init_f()) + goto err_out; #endif - INIT_FUNC_WATCHDOG_RESET - init_func_ram, + if (INIT_FUNC_WATCHDOG_RESET) + goto err_out; + if (init_func_ram()) + goto err_out; #if defined(CONFIG_SYS_DRAM_TEST) - testdram, + if (testdram()) + goto err_out; #endif /* CONFIG_SYS_DRAM_TEST */ - INIT_FUNC_WATCHDOG_RESET - - NULL, /* Terminate this list */ + if (INIT_FUNC_WATCHDOG_RESET) + goto err_out; + return; +err_out: + hang(); }; ulong get_effective_memsize(void) @@ -366,7 +399,6 @@ void board_init_f (ulong bootflag) ulong len, addr, addr_sp; ulong *s; gd_t *id; - init_fnc_t **init_fnc_ptr; #ifdef CONFIG_PRAM int i; ulong reg; @@ -383,12 +415,7 @@ void board_init_f (ulong bootflag) /* Clear initial global data */ memset ((void *) gd, 0, sizeof (gd_t)); #endif - - for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) { - if ((*init_fnc_ptr) () != 0) { - hang (); - } - } + init_sequence(); /* * Now that we have DRAM mapped and working, we can
init_sequence is an array with function pointers which are really hard to follow when you need to debug this area. Turn it into plain function calls instead which makes the code a bit uglier but I find the simpler debugging much more valuable. An added bonus is that it is smaller too: text data bss dec hex filename 1268 212 0 1480 5c8 lib_ppc/board.org 1224 92 0 1316 524 lib_ppc/board.new Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- So I had to do debug this area again and I am getting really tiered of following function pointers so here goes my old patch again. arch/powerpc/lib/board.c | 123 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 75 insertions(+), 48 deletions(-)