[coreboot] [PATCH] v3: move CAR base and CAR size to invisible Kconfig

[Carl-Daniel Hailfinger]

On 11.02.2008 03:42, Stefan Reinauer wrote:
> 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.

Same could be said for serial, yet we offer configurability (in the dts) 
for serial iobase even on a mainboard level.


> My prognosis is we will end up with different settings per mainboard, 
> like we had in v2.

How so? The code I committed selects an invisible Kconfig variable based 
on the subarch alone. A mainboard porter will never see the variable nor 
does he need to know it exists.


> 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.

And manual resolution is prone to failure because the user needs to know 
the dependencies. That's why having that stuff in Kconfig in v3 makes 
things easier. Besides that, code which just fails to build (or even 
worse, fails to run) without giving a user-understandable error message 
is just broken. My CAR changes in v2 and v3 fixed some of the most 
glaring problems in that area for 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.

Oh, CARBASE must be global anyway because printk needs to know it for 
the buffer address calculation. That would make CARBASE a perfect fit 
for Kconfig.


>> 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.

CONSOLE_LOGLEVEL is code driven, yet nobody is arguing for its removal.


> 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.

CARBASE can not be adjusted with any config interface. It is a hidden 
variable. I still fail to see where the logic of the approach is broken.

> 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.

How does it reduce testability and usability over lots of #ifdefs?

> See also http://en.wikipedia.org/wiki/Feature_Driven_Development

That seems to be a bad model for us. The wikipedia page says 3% of the 
total effort are spent on design review. If we really did that, nobody 
would have had the time to question my design to have CARBASE in Kconfig.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/