On Mon, Feb 9, 2009 at 8:21 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Ühel kenal päeval, E, 2009-02-09 kell 08:04, kirjutas ron minnich:
nice to see people using the dts now!
It's a bit hard to use it for the LBAR setup as well (the code in chipset_flash_setup() in cs5536.c), as it needs to happen before VSA init, and VSA init is done in northbridge domain phase2_fixup - so we don't have a phase for NAND device to run before VSA is inited. Otherwise I'd use the NAND initialization from chipsetinit() to some nand_phase1 as well.
Acked-by: Ronald G. Minnich rminnich@gmail.com
I'm going to have to NAK this myself, as it doesn't compile with my gcc if there is no NAND device in the mainboard dts, as there is no southbridge_amd_cs5536_nand_config in statictree.h then, and gcc complains about /home/leio/dev/coreboot-v3/southbridge/amd/cs5536/cs5536.c:119: error: dereferencing pointer to incomplete type
The problem is that you have to have a separate compilation unit if you have seperate dts objects.
If you really want to separate out the nand dts, as you have done: /config/("southbridge/amd/cs5536/nand");
then you need to have a southbridge/amd/cs5536/nand.c. See the other parts for example.
That means also I'm going to have two places where to enable or disable devices - dts and Makefile. And I need to keep them in sync. And I need to list southbridge C files in mainboard specific Makefile instead of in the same directory. It isn't very nice and scalable and neat. Could we perhaps have dtc give me a DTS_HAVE_CS5536_NAND or similar preprocessor definitions if the device is present in the static tree?
If you want to keep the code in cs5536.c, then you'll have to put the settings in the cs5536 dts.
That seems like an abuse of the dts system
How should I approach this?
See what I did in the sb600.
For the sake of argument, lets say every kilobyte matters as I might want to also fit in a LAB in a 1MB limit. We could be talking about a whole lot more complex code that is unconditionally compiled in unless doing ugly C file listing in mainboard dir Makefiles than we are dealing with in this instance.
On another note, can we cleanly avoid compiling all this NAND init code in for the boards that surely do not have a NAND device as it can't be a dynamic device?
Yes. put it in its own file. That's what makefiles are for. Then those boards that have nand:
add nand.c to the Makefile use the nand dts you set up.
The problem is that that listing is in a completely different Makefile than where the file is at.
(w.r.t. IDE)
Less important though, as the code footprint is probably negligible.
This is the really key point. We decided in a heated discussion a few months back that on, e.g., sb600, we'd compile all code in, whether used or not. But in your case, with this nand stuff, you might as seperate out the code in nand.c
As a completely off-topic slightly related to the above side note, I'm still slightly annoyed at all loglevel strings getting compiled in now, no questions asked. A maximum supported loglevel option would be nice. We need no complete spew loglevel messages in a production ROM we hypothetically burn into units sold to customers. Just haven't gotten around to implement that after those changes I didn't manage to raise a NAK on time :)
Regards, Mart Raudsepp
On Mon, Feb 9, 2009 at 9:00 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Could we perhaps have dtc give me a DTS_HAVE_CS5536_NAND or similar preprocessor definitions if the device is present in the static tree?
sure. We could also have it create a Make variable such that the nand.c is conditionally compiled in. I prefer that to #ifdef goo in .c files.
Or we could do something like this: STAGE2_SOURCE="somefile.c"
and have dtc add that source to e.g. the stage2 source. Of course we are moving far beyond what dts was intended to do; we're really starting to put things into it that are in the v2 config tool.
For the sake of argument, lets say every kilobyte matters as I might want to also fit in a LAB in a 1MB limit. We could be talking about a whole lot more complex code that is unconditionally compiled in unless doing ugly C file listing in mainboard dir Makefiles than we are dealing with in this instance.
yep. Once again, the v2 config system comes out looking pretty good on this score.
As a completely off-topic slightly related to the above side note, I'm still slightly annoyed at all loglevel strings getting compiled in now, no questions asked. A maximum supported loglevel option would be nice. We need no complete spew loglevel messages in a production ROM we hypothetically burn into units sold to customers.
In many cases the lack of such compiled in messages was a huge problem on deployed systems. That's why we went to having them all in as default. There were many complaints about leaving out messages in v2 ...
ron
Ühel kenal päeval, E, 2009-02-09 kell 09:56, kirjutas ron minnich:
On Mon, Feb 9, 2009 at 9:00 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Could we perhaps have dtc give me a DTS_HAVE_CS5536_NAND or similar preprocessor definitions if the device is present in the static tree?
sure. We could also have it create a Make variable such that the nand.c is conditionally compiled in. I prefer that to #ifdef goo in .c files.
Or it would be the #define given by dtc and we can just #ifdef at the top of nand.c or similar files if they warrant a separate file, and unconditionally have it in Makefile without any mess.
Or we could do something like this: STAGE2_SOURCE="somefile.c"
and have dtc add that source to e.g. the stage2 source. Of course we are moving far beyond what dts was intended to do; we're really starting to put things into it that are in the v2 config tool.
I'm getting a feeling the dts wasn't intended to do a whole lot it has the power to do, maybe I should re-read Newboot.
For the sake of argument, lets say every kilobyte matters as I might want to also fit in a LAB in a 1MB limit. We could be talking about a whole lot more complex code that is unconditionally compiled in unless doing ugly C file listing in mainboard dir Makefiles than we are dealing with in this instance.
yep. Once again, the v2 config system comes out looking pretty good on this score.
So could dtc imho.
As a completely off-topic slightly related to the above side note, I'm still slightly annoyed at all loglevel strings getting compiled in now, no questions asked. A maximum supported loglevel option would be nice. We need no complete spew loglevel messages in a production ROM we hypothetically burn into units sold to customers.
In many cases the lack of such compiled in messages was a huge problem on deployed systems. That's why we went to having them all in as default. There were many complaints about leaving out messages in v2 ...
That's all fair, just now we have no option to declare which level messages should be compiled in - it's always DEBUG_SPEW. With an option to give the maximum compiled in level, defaulting to SPEW, would be a win-win :)
Regards, Mart Raudsepp
On Tue, Feb 10, 2009 at 1:36 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Or it would be the #define given by dtc and we can just #ifdef at the top of nand.c or similar files if they warrant a separate file, and unconditionally have it in Makefile without any mess.
I really dislike #ifdef'ing code, again because it confuses the hell out of tools like kscope, ctags, doxygen and friends. As much as possible we should make coreboot friendly to those sorts of tools. I found the bug in flashrom in a few seconds with kscope; kscope has already saved me tons of time walking code in v3 (as in, e.g., Myles improved device code).
Linux solved this problem years ago and we have too. Use the Makefile. That's what it's there for.
I'm getting a feeling the dts wasn't intended to do a whole lot it has the power to do, maybe I should re-read Newboot.
it's out of date now. It needs a thorough going-over and fixup.
For the sake of argument, lets say every kilobyte matters as I might want to also fit in a LAB in a 1MB limit. We could be talking about a whole lot more complex code that is unconditionally compiled in unless doing ugly C file listing in mainboard dir Makefiles than we are dealing with in this instance.
yep. Once again, the v2 config system comes out looking pretty good on this score.
So could dtc imho.
I agree. I think you and I like dts over all, as do others. But the standard dts from open boot is not quite enough to do what we want.
That's all fair, just now we have no option to declare which level messages should be compiled in - it's always DEBUG_SPEW. With an option to give the maximum compiled in level, defaulting to SPEW, would be a win-win :)
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting. But you're a vendor, and you need this, so if you have a patch then we ought to take a look. I'm sensitive to size issues.
ron
ron minnich wrote:
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting.
But it shouldn't be too bad, right?
Just one printk function should need changing, and only at the definition.
I'm also looking forward to seeing this. :)
//Peter
On Tue, Feb 10, 2009 at 8:46 AM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting.
But it shouldn't be too bad, right?
Just one printk function should need changing, and only at the definition.
It should not be too bad. I just want to make sure that everyone on the list understands what we are going to change.
Getting rid of the printk macro, and having all printk messages always available, was viewed as a reform from v2. We're essentially reforming the reform :-)
ron
Ühel kenal päeval, T, 2009-02-10 kell 08:49, kirjutas ron minnich:
On Tue, Feb 10, 2009 at 8:46 AM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting.
But it shouldn't be too bad, right?
Just one printk function should need changing, and only at the definition.
The log level is now given with an argument, so it's trickier to get the code excluded from the binary completely. Likely can coerce gcc to do that with specially crafted code.
It should not be too bad. I just want to make sure that everyone on the list understands what we are going to change.
Getting rid of the printk macro, and having all printk messages always available, was viewed as a reform from v2. We're essentially reforming the reform :-)
We are giving more possibilities and restoring the possibility of having a more limited set of debug strings compiled in. Having all loglevel messages compiled in would still be a possibility and the default for all I care.
Regards, Mart Raudsepp
Mart Raudsepp wrote:
The log level is now given with an argument, so it's trickier to get the code excluded from the binary completely.
Maybe split it up then? (But _only_ statics within the printk code!)
Likely can coerce gcc to do that with specially crafted code.
I would prefer to avoid that.
//Peter
On Tue, Feb 10, 2009 at 9:03 AM, Peter Stuge peter@stuge.se wrote:
Mart Raudsepp wrote:
The log level is now given with an argument, so it's trickier to get the code excluded from the binary completely.
Maybe split it up then? (But _only_ statics within the printk code!)
Actually it's not that hard.
You have a global , MAXLOGLEVEL. Turn printk into something like this:
#define printk(a, b, ...) if (a <= MAXLOGLEVEL) print(blah blah)
and then let the compiler optimize it all out.
It's kind of like if (0) { etc. etc. }
Likely can coerce gcc to do that with specially crafted code.
I would prefer to avoid that.
This is very non-special
ron
ron minnich wrote:
The log level is now given with an argument, so it's trickier to get the code excluded from the binary completely.
Maybe split it up then? (But _only_ statics within the printk code!)
Actually it's not that hard.
Yep, that's right.
You have a global , MAXLOGLEVEL. Turn printk into something like this:
#define printk(a, b, ...) if (a <= MAXLOGLEVEL) print(blah blah)
and then let the compiler optimize it all out.
That's what I thought of at first too, but I was discouraged by Mart. Do you see a real problem with this, Mart?
Likely can coerce gcc to do that with specially crafted code.
I would prefer to avoid that.
This is very non-special
Yep, it's all at cpp time and not at all strange. I would prefer avoiding compiler tricks when we can reach the goal just using cpp.
//Peter
Ühel kenal päeval, T, 2009-02-10 kell 18:14, kirjutas Peter Stuge:
ron minnich wrote:
The log level is now given with an argument, so it's trickier to get the code excluded from the binary completely.
Maybe split it up then? (But _only_ statics within the printk code!)
Actually it's not that hard.
Yep, that's right.
You have a global , MAXLOGLEVEL. Turn printk into something like this:
#define printk(a, b, ...) if (a <= MAXLOGLEVEL) print(blah blah)
and then let the compiler optimize it all out.
That's what I thought of at first too, but I was discouraged by Mart. Do you see a real problem with this, Mart?
Nope. that's similar to what I had in mind.
Likely can coerce gcc to do that with specially crafted code.
I would prefer to avoid that.
This is very non-special
Yep, it's all at cpp time and not at all strange. I would prefer avoiding compiler tricks when we can reach the goal just using cpp.
Yeah, and if(0) and the like is coercing gcc :)
However I don't think using cpp for this really flies well if we are loosing __attribute__((format (printf, 2, 3))) checks then.
Mart
Mart Raudsepp wrote:
#define printk(a, b, ...) if (a <= MAXLOGLEVEL) print(blah blah)
That's what I thought of at first too, but I was discouraged by Mart. Do you see a real problem with this, Mart?
Nope. that's similar to what I had in mind.
..
I would prefer avoiding compiler tricks when we can reach the goal just using cpp.
Yeah, and if(0) and the like is coercing gcc :)
Yes. I didn't write exactly what I meant. I was actually thinking even one more layer of cpp stuff, to check the condition. In theory like this;
#define printk(a, b...) \ #if (a) <= CONFIG_MAXLOGINCLUDE do_printk((a),(b)) \ #endif
Maybe that's too evil?
However I don't think using cpp for this really flies well if we are loosing __attribute__((format (printf, 2, 3))) checks then.
Agree.
//Peter
On Tue, Feb 10, 2009 at 10:10 AM, Peter Stuge peter@stuge.se wrote:
#define printk(a, b...) \ #if (a) <= CONFIG_MAXLOGINCLUDE do_printk((a),(b)) \ #endif
Maybe that's too evil?
let's just let gcc do its job.
ron
On 10.02.2009 17:46 Uhr, Peter Stuge wrote:
ron minnich wrote:
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting.
But it shouldn't be too bad, right?
Just one printk function should need changing, and only at the definition.
I'm also looking forward to seeing this. :)
What would it be good for? Does it safe a considerable amount of space? How much?
I think the binaries should always contain _all_ debug messages so if a user gets a binary from somewhere she always has the chance to see the full debug output, no matter what the default is.
Stefan
Ühel kenal päeval, T, 2009-02-10 kell 08:36, kirjutas ron minnich:
On Tue, Feb 10, 2009 at 1:36 AM, Mart Raudsepp mart.raudsepp@artecdesign.ee wrote:
Or it would be the #define given by dtc and we can just #ifdef at the top of nand.c or similar files if they warrant a separate file, and unconditionally have it in Makefile without any mess.
I really dislike #ifdef'ing code, again because it confuses the hell out of tools like kscope, ctags, doxygen and friends. As much as possible we should make coreboot friendly to those sorts of tools.
And not having an #ifdef can confuse a human being, because the presence of the file on the first look would suggest its code gets compiled and is active, while it isn't. I guess if we can keep the source file listing in the same dirs Makefile somehow... One idea is that Kconfig would know about it from selecting of the board defining HAVE_CS5536_NAND, but then it's still the problem of listing that fact in two places (Kconfig and dts), so that's not better.
I found the bug in flashrom in a few seconds with kscope; kscope has already saved me tons of time walking code in v3 (as in, e.g., Myles improved device code).
Isn't using it coupled with debugging a specific board or the like?
Linux solved this problem years ago and we have too. Use the Makefile. That's what it's there for.
I guess I can use the Makefile and add prominent comments in nand.c about this file being used only if dts has a device for it, but that's not a possibility right now either without digging into unfamiliar dtc territory.
There are theoretical cases where you really can't move it over to a different file however, and the dts is limiting us there. If it would be giving those #define's it would be just a matter of good code design, not a technical limitation, to have it in a separate file when appropriate.
I'm getting a feeling the dts wasn't intended to do a whole lot it has the power to do, maybe I should re-read Newboot.
it's out of date now. It needs a thorough going-over and fixup.
For the sake of argument, lets say every kilobyte matters as I might want to also fit in a LAB in a 1MB limit. We could be talking about a whole lot more complex code that is unconditionally compiled in unless doing ugly C file listing in mainboard dir Makefiles than we are dealing with in this instance.
yep. Once again, the v2 config system comes out looking pretty good on this score.
So could dtc imho.
I agree. I think you and I like dts over all, as do others. But the standard dts from open boot is not quite enough to do what we want.
That's all fair, just now we have no option to declare which level messages should be compiled in - it's always DEBUG_SPEW. With an option to give the maximum compiled in level, defaulting to SPEW, would be a win-win :)
You have to rewrite all the printk functions to be macros again. It's a bit more work than changing one setting. But you're a vendor, and you need this, so if you have a patch then we ought to take a look. I'm sensitive to size issues.
Maybe we can coerce gcc to optimize out the code to printk calls for debug levels that aren't supported via that additional config option.
Regards, Mart Raudsepp
-
Mart Raudsepp -
Peter Stuge -
ron minnich -
Stefan Reinauer