[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.dehttp://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