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

[Albert ARIBAUD]

Hi Igor,

On 28/07/2011 10:58, Igor Grinberg wrote:
> On 07/28/11 11:19, Chander Kashyap wrote:
>> On 28 July 2011 13:29, Igor Grinberg<grinberg at compulab.co.il>  wrote:
>>>
>>> On 07/28/11 09:41, Chander Kashyap wrote:
>>>> Dear Igor,
>>>>
>>>>
>>>> On 27 July 2011 18:34, Igor Grinberg<grinberg at compulab.co.il>  wrote:
>>>>> On 07/27/11 13:31, Chander Kashyap wrote:
>>>>>> dear Igor,
>>>>>>
>>>>>>
>>>>>> On 14 July 2011 21:15, Igor Grinberg<grinberg at compulab.co.il>  wrote:
>>>>>>> CONFIG_MACH_TYPE is used to set the machine type number in the
>>>>>>> common arm code instead of setting it in the board code.
>>>>>>> Boards with dynamically discoverable machine types can still set the
>>>>>>> machine type number in the board code.
>>>>>>>
>>>>>>> Signed-off-by: Igor Grinberg<grinberg at compulab.co.il>
>>>>>>> ---
>>>>>>> v2:     Document the option as mandatory.
>>>>>>>         Move the bi_arch_number setting to board_init_f()
>>>>>>>
>>>>>>>   README               |   10 ++++++++++
>>>>>>>   arch/arm/lib/board.c |    4 ++++
>>>>>>>   2 files changed, 14 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/README b/README
>>>>>>> index 446966d..0b6802d 100644
>>>>>>> --- a/README
>>>>>>> +++ b/README
>>>>>>> @@ -442,6 +442,16 @@ 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][mandatory]
>>>>>>> +
>>>>>>> +               This setting is mandatory for all boards that have only one
>>>>>>> +               machine type and must 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/).
>>>>>>> +               Only boards that have multiple machine types supported
>>>>>>> +               in a single configuration file and the machine type is
>>>>>>> +               runtime discoverable, do not have to use this setting.
>>>>>>> +
>>>>>>>   - 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..9901694 100644
>>>>>>> --- a/arch/arm/lib/board.c
>>>>>>> +++ b/arch/arm/lib/board.c
>>>>>>> @@ -281,6 +281,10 @@ void board_init_f (ulong bootflag)
>>>>>>>
>>>>>>>         gd->mon_len = _bss_end_ofs;
>>>>>>>
>>>>>>> +#ifdef CONFIG_MACH_TYPE
>>>>>>> +       gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>>>>>> +#endif
>>>>>>> +
>>>>>> bd structure is not initialised by this time.
>>>>>> It leads to u-boot hanging for my board.
>>>>>> I fixed this problem but modifying it. Below is the patch attached for the same.
>>>>> Then how does it work for boards setting the gd->bd->bi_arch_number
>>>>> in board_early_init_f() function?
>>>> can you please point out any board which sets in board_early_init_f() ?
>>> board/esd/otc570/otc570.c
>>>
>>> Also, I don't think we should restrict setting it to board_init() and later functions.
>
> I've looked into the code a bit more deeply...
> Currently, I don't see how the bd initialization can be done earlier than it is right now,
> to let boards use it in board_early_init_f() function and other early functions.
> I have not found any other initialization of bd on that architecture,
> so this makes the otc570 misuse the bd pointer
> (unless 0 is a valid pointer on that architecture, but then it is a total mess...)
>
>>>>>>>         for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
>>>>>>>                 if ((*init_fnc_ptr)() != 0) {
>>>>>>>                         hang ();
>>>>>>> --
>>>>>>> 1.7.3.4
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> U-Boot mailing list
>>>>>>> U-Boot at lists.denx.de
>>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>>>
>>>>>> > From d8df2f0ca9f08470c0cb88307fea4a66f41147a5 Mon Sep 17 00:00:00 2001
>>>>>> From: Chander Kashyap<chander.kashyap at linaro.org>
>>>>>> Date: Wed, 27 Jul 2011 15:10:59 +0530
>>>>>> Subject: [PATCH] ARM: Fix wrong initialisation of bi_arch_number
>>>>>>
>>>>>> bi_arch_number is initialised using
>>>>>> @arch/arm/lib/board.c
>>>>>> \#ifdef CONFIG_MACH_TYPE
>>>>>>          gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>>>>> \#endif
>>>>>>
>>>>>> bd structure is not intialized by this time.
>>>>>> This leads to u-boot hanging when CONFIG_MACH_TYPE is defined.
>>>>>>
>>>>>> Signed-off-by: Chander Kashyap<chander.kashyap at linaro.org>
>>>>>> ---
>>>>>>   arch/arm/lib/board.c |    7 +++----
>>>>>>   1 files changed, 3 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>>>>>> index bcbf697..98a9bcc 100644
>>>>>> --- a/arch/arm/lib/board.c
>>>>>> +++ b/arch/arm/lib/board.c
>>>>>> @@ -281,10 +281,6 @@ void board_init_f (ulong bootflag)
>>>>>>
>>>>>>        gd->mon_len = _bss_end_ofs;
>>>>>>
>>>>>> -#ifdef CONFIG_MACH_TYPE
>>>>>> -     gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>>>>> -#endif
>>>>>> -
>>>>>>        for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
>>>>>>                if ((*init_fnc_ptr)() != 0) {
>>>>>>                        hang ();
>>>>>> @@ -380,6 +376,9 @@ void board_init_f (ulong bootflag)
>>>>>>        gd->bd = bd;
>>>>>>        debug ("Reserving %zu Bytes for Board Info at: %08lx\n",
>>>>>>                        sizeof (bd_t), addr_sp);
>>>>>> +#ifdef CONFIG_MACH_TYPE
>>>>>> +     gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>>>>> +#endif
>>>>> This is problematic...
>>>>> There are boards that rely on this setting in early init function calls.
>>>>> For them it should be set before the init_sequence array is run.
>>>>> I will rethink this once again.
>>>> as per my understanding board_init_f() is the first initialisation call.
>>> Yes, but there is the init_sequence[] array, which calls early board functions...
>>> Also your proposed patch moves the initialization of bi_arch_number inside
>>> #ifndef CONFIG_PRELOADER which is IMHO not right.
>> CONFIG_PRELOADER is only defined when building SPL.
>
> If I recall correctly there was an attempt to boot Linux straight from SPL code,
> but I'm not sure...
> Anyway, if we move the bi_arch_number initialization after the init_sequence[] array,
> then it can be moved further till after the POST.
> I'll send a patch for this in a minute.
> Can you please test it?

Should I revert this patch?

Amicalement,
-- 
Albert.