[U-Boot] [PATCH V2] imx: Define common routines to set cpu and board environment variables

[Eric Nelson]

Thanks Tom,

On 11/15/2013 12:15 PM, Tom Rini wrote:
> On Fri, Nov 15, 2013 at 12:30:12AM +0100, Marek Vasut wrote:
>> Hi Eric,
>>
>>> Thanks Marek,
>>>
>>> On 11/14/2013 02:22 PM, Marek Vasut wrote:
>>>> Dear Eric Nelson,
>>>>
>>>> +Albert, Tom
>>>>
>>>>> These can be used in bootcmd to produce DTB file names.
>>>>>
>>>>> set_board_env() allows over-ride for use when a developing custom
>>>>> DTB files.
>>>>>
>>>>> Signed-off-by: Eric Nelson <eric.nelson at boundarydevices.com>
>>>>> ---
>>>>> V2 adds void to the function declarations and definitions and adds
>>>>> a blank to keep checkpatch clean.
>>>>>
>>>>> I'm feeling like I missed something here. Routines in imx-common/cpu.c
>>>>> are shared between various i.MX processors, but there doesn't appear
>>>>> to be a common header file.
>>>>>
>>>>> It seems that arch/arm/include/asm/imx-common.h should be present
>>>>> but isn't. Am I missing something?
>>>>>
>>>>> I also think there should be a way we could pull this into multiple
>>>>> boards without adding a full-up board_late_init() function into
>>>>> each board file, but tracing the init sequence, I'm not seeing an
>>>>> architecture-specific spot after env_init.
>>>>>
>>>>>    arch/arm/imx-common/cpu.c                 | 15 +++++++++++++--
>>>>>    arch/arm/include/asm/arch-mx6/sys_proto.h |  9 +++++++++
>>>>>    2 files changed, 22 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>>>>> index 0cd2538..4214946 100644
>>>>> --- a/arch/arm/imx-common/cpu.c
>>>>> +++ b/arch/arm/imx-common/cpu.c
>>>>> @@ -99,8 +99,6 @@ unsigned imx_ddr_size(void)
>>>>>
>>>>>    }
>>>>>    #endif
>>>>>
>>>>> -#if defined(CONFIG_DISPLAY_CPUINFO)
>>>>> -
>>>>>
>>>>>    const char *get_imx_type(u32 imxtype)
>>>>>    {
>>>>>
>>>>>    	switch (imxtype) {
>>>>>
>>>>> @@ -121,6 +119,19 @@ const char *get_imx_type(u32 imxtype)
>>>>>
>>>>>    	}
>>>>>
>>>>>    }
>>>>>
>>>>> +void __weak set_cpu_env(void)
>>>>> +{
>>>>> +	setenv("cpu", get_imx_type(cpu_type(get_cpu_rev())));
>>>>> +}
>>>>
>>>> I'd say we should have a U-Boot wide thing here + CPU-specific overrides
>>>> where applicable.
>>>
>>> I'll follow your lead on this.
>>
>> I'd wait for Tom's/Albert's opinion on this one too btw.
>
> So, CONFIG_ENV_VARS_UBOOT_CONFIG says that these variables are
> _build_time_.  If you want to whack their values, update the README on
> CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG to say so.  Having the i.MX code
> that updates this at run-time is fine to live in the i.MX code area.
>
I wasn't aware of these two options, and they seem useful.

They also suggest that using "cpu" and "board" are probably wrong,
since CONFIG_SYS_CPU and environment "cpu" are already set to "armv7"
and "board" is configured with the build-time board name.

Overriding "board_name" seems to be in-line with the README if
we can just figure out the right spot for initialization.

Some other variable ("cpu_name" or "imx_type" perhaps?) should probably
be used for the CPU variant.

Chasing down how "board_name" is initialized in get_board_revision()
leads me to arch_misc_init() is the right place for this on i.MX,
since it's called after the environment is initialized.

> [snip]
>>> This could be even easier on i.MX6 if we had more formalized (and
>>> lower-case) strings returned by get_imx_type(), but I wanted this
>>> to be very small.
>>>
>>> I'm not sure how consistent the DTB naming is for other machines
>>> or if there's always the equivalent of get_imx_type().
>>
>> In the worst-case scenario, you might end up with some mapping
>> function full of strcmp()s ;-)
>
> Note that in TI-land we do this with 'findfdt' in the environment and
> yes, some string compares.  But we set board_name in C so that we can do
> what we need to, to determine the board.  Maybe something like that
> would help here?
>
Thanks. That does help.

Regards,


Eric