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

[Igor Grinberg <grinberg@compulab.co.il>]

On 07/07/11 19:07, Albert ARIBAUD wrote:

> Hi Igor,
>
> Le 06/07/2011 22:05, Igor Grinberg a ?crit :
>> On 07/06/11 21:53, Albert ARIBAUD wrote:
>>
>>> Hi Igor,
>>>
>>> Le 04/07/2011 11:00, Igor Grinberg a ?crit :
>>>> CONFIG_MACH_TYPE can be used to set the machine type number in the
>>>> common arm code instead of setting it in the board code.
>>>>
>>>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il>
>>>> ---
>>>>    README               |   12 ++++++++++++
>>>>    arch/arm/lib/board.c |    5 +++++
>>>>    2 files changed, 17 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/README b/README
>>>> index 446966d..a9ccb0a 100644
>>>> --- a/README
>>>> +++ b/README
>>>> @@ -442,6 +442,18 @@ The following options need to be configured:
>>>>            crash. This is needed for buggy hardware (uc101) where
>>>>            no pull down resistor is connected to the signal IDE5V_DD7.
>>>>
>>>> +        CONFIG_MACH_TYPE        [relevant for ARM only]
>>>> +
>>>> +        This option can be used to specify the machine type number
>>>> +        as it appears in the ARM machine registry
>>>> +        (see http://www.arm.linux.org.uk/developer/machines/).
>>>> +        If this option is not defined, then your board code
>>>> +        will have to set this up like:
>>>> +        gd->bd->bi_arch_number =<mach type>;
>>>> +        Note: This option is not suitable if you have multiple
>>>> +        boards supported in a single configuration file and the
>>>> +        machine type is runtime discoverable.
>>>> +
>>>>    - vxWorks boot parameters:
>>>>
>>>>            bootvx constructs a valid bootline using the following
>>>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>>>> index 169dfeb..ee77d05 100644
>>>> --- a/arch/arm/lib/board.c
>>>> +++ b/arch/arm/lib/board.c
>>>> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr)
>>>>
>>>>        monitor_flash_len = _end_ofs;
>>>>        debug ("monitor flash len: %08lX\n", monitor_flash_len);
>>>> +
>>>> +#ifdef CONFIG_MACH_TYPE
>>>> +    bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>>> +#endif
>>>> +
>>>>        board_init();    /* Setup chipselects */
>>>>
>>>>    #ifdef CONFIG_SERIAL_MULTI
>>>
>>> I don't really see the added value of having this configuration option. It is used in only one place for one line of code, which is a sign to me that it does not bring any substantial benefits.
>>
>> Well, this is something that came up when I tried to get rid of machine_is_*
>> macros (patch 3/3), so we could change/cut/simplify the overgrown mach-types.h
>> if we want/need to...
>
> You've read and responded to this thread: http://old.nabble.com/-U-Boot--Update-and-Cut-down-mach-types-td31432177.html, so you know that mach-types.h should not be changed, cut or simplified, but should only be generated from the mach-types list pulled from the ARM Linux machine repository just like Linux' mach-types.h is generated (though we use the full list whereas Linux uses a reduced list, because U-Boot supports some ARM machines which Linux does not). If your goal is reducing the size of mach-types.h, there is a branch in u-boot-ti that is about this (Sandeep: has it been posted as patches for review?)

Right, I've just tried to explain where it was originated from.

It helps to simplify the mach_type logic for omap1610innovator and omap1610h2.
Also, variants of it used locally by some other boards (e.g. all nvidia boards, smdk6400).
Don't you think that generalizing this stuff is an added value?
Instead of having multiple and undocumented variants of the same feature,
we'd better provide a common and documented one, no?

>
>> I think there is an added value, may be it is hard to see it right now.
>
> If it is hard to see, then maybe it is too small a value to be worth the effort -- that's what I am trying to sort out.

I've done it already... The effort is already made... Is there any other effort that I'm not aware of
(besides of picking it up and merging)?

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

>
>
>> Also, it can come in handy, in the configuration system (I think it is called Kbuild?),
>> so the value can be chosen from the list or something.
>
> AFAICT, the Linux build system does fine with the generated mach-type.

Here, I'm not talking about the mach_types, but about the config option.
It can be set by the build system and no need to do it in board code!



-- 
Regards,
Igor.