[coreboot] [PATCH] cs5536/nand: Allow setting of NAND timing values in the dts.

Mart Raudsepp mart.raudsepp at artecdesign.ee
Tue Feb 10 17:50:48 CET 2009


Ü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 at 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





More information about the coreboot mailing list