[coreboot] [PATCH] v3: move CAR base and CAR size to invisible Kconfig
Stefan Reinauer
stepan at coresystems.de
Mon Feb 11 03:42:59 CET 2008
Carl-Daniel Hailfinger wrote:
>> The config
>> system is not there to set code internal only variables. That's what the
>> arch/x86 cpu dependent include files are there for.
>>
>
> Actually, this is one of the good aspects of v2: setting
> subarch-dependant variables dependant on subarch and/or mainboard.
>
We start reveiling too much information in the configuration process,
implying this is something that could be changed on a per-board basis.
In reality it is just a part of the code that we source out now. The CAR
code for a given platform should know how to get CAR working on that
platform.
My prognosis is we will end up with different settings per mainboard,
like we had in v2. And in 6 months noone will know anymore why we have
these different settings. In v2 you need a larger CAR area if you want
to enable some features triggered by other CONFIG variables. So there's
a dependency, but it is up to the user to manually resolve it. Just
enabling those features will not work anymore, because you need to know
that you also have to increase CAR.
The next step is to pack all kinds of logic into the config mechanism
that does not belong there. In v2 we even calculated the offsets of
normal and fallback in the config file. You enable a small piece of code
on the Xth board using a given mainboard and things will fall apart.
I think CAR_BASE and CAR_SIZE should be a property encapsulated in the
CAR code component. If some component needs to change those values, they
should provide a "reason" rather than just values. As the complexity of
v3 increases, the CAR component can then locally look at all the
"reasons" influencing required CAR size and react upon that. Locally.
Not globally. Kconfig stuff is global. Global is bad.
> How do you define "code internal only variables"? I think variables with
> arch-dependent value (as opposed to arch-dependent existence) should be
> set by Kconfig. There is no reason to have stuff like LX_NUM_CACHELINES
> in Kconfig because it doesn't even exist on other subarches.
>
CONFIG stuff should be feature driven, not code driven. A feature is
something like the "printk log buffer". If you want that, you might need
to increase the CAR size. The code needs to know that, and change the
CAR values accordingly.
Otherwise the fragility of the code is increased by the fragility of the
Kconfig mechanism. If someone thinks it is necessary to adjust CAR_BASE
and CAR_SIZE via Kconfig, the logic of the approach is broken. It means
the code needs fixing. Not hotfixing.
There is indeed no reason for LX_NUM_CACHELINES in Kconfig either.
Keeping it there is as broken as your last change.
That's not a configuration aspekt. You can't configure your board to
have more cache lines.
Packing all this stuff into Kconfig looks very developer friendly at
first, while it is going to betray us in the same way it did and does in
v2. It reduces the testability and reusability of the code while
implicating that everything got more flexible.
See also http://en.wikipedia.org/wiki/Feature_Driven_Development
Stefan
--
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20080211/c160dd53/attachment.sig>
More information about the coreboot
mailing list