[U-Boot] [PATCH 1/3] arm: add CONFIG_MACH_TYPE option and documentation

[Igor Grinberg]

On 07/07/11 20:46, Albert ARIBAUD wrote:
> Le 07/07/2011 18:51, Igor Grinberg a écrit :
>
>>>> If we have this option and it is documented, then any new board can use it
>>>> instead of thinking (although it is simple) where and how to dereference
>>>> the bi_arch_number.
>>>
>>> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it?
>>
>> For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.),
>> If you need Christopher's patch, then there are cases when the machid is not set, right?
>> When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then
>> goes to fix the code, but again he thinks, where is the best place to put it?
>> For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial.
>> With this patch, you get the explanation and also a place to put the machid definition.
>> With this patch, you just define the configuration "variable" and the whole thing will be done for you.
>> Another example would be the board/nvidia/*, the code is shared as much as possible,
>> and the mach_type is set in the common code. That is something I would expect to be done
>> for all ARM boards, not just for nvidia...
>
> I see your point.
>
> Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(),

Vast majority of boards set their bi_arch_number in board_init().
I went through all the boards and there is only one that set it in board_early_init_f()
and several that do this in checkboard().

This makes me think of v2 of this patch which will set the bi_arch_number in board_init_f()
just before the init_sequence[] array is run.

> but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged.

Well, I don't like to force people do something by breaking their builds...
In general, I think that any change should not break any existing code (at least not on purpose).
At least, this is how it works in Linux.

>
> IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity.
>
> Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either.

Well, I can do the change board/* wide, but it will take some time to accomplish.
Also, we still don't have an exact list of boards for removal, so I'd like to wait until
the removal takes place, so there will be less boards to consider.

>
> Any suggestion for ensuring adoption of the feature wherever it can be used?

Currently, I can think of:
1) Changing all relevant boards to use it - will eliminate "bad" examples.
2) Pointing to the use of CONFIG_MACH_TYPE during code review.
3) Introduce one more config option, like CONFIG_DYNAMIC_MACH_TYPE
and change the patch to something like:
#ifndef CONFIG_DYNAMIC_MACH_TYPE
bi_arch_number = CONFIG_MACH_TYPE;
#endif

If we decide to go for 3), it would integrate nicely with Christopher's patch.


-- 
Regards,
Igor.