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

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

[Eric Nelson]

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@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())));
+}
+
+void __weak set_board_env(void)
+{
+	char *old = getenv("board");
+	if (!old)
+		setenv("board", CONFIG_SYS_BOARD);
+}
+
+#if defined(CONFIG_DISPLAY_CPUINFO)
 int print_cpuinfo(void)
 {
 	u32 cpurev;
diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h
index 8c21364..f58ebc3 100644
--- a/arch/arm/include/asm/arch-mx6/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
@@ -27,8 +27,17 @@ u32 get_cpu_rev(void);
 #define is_cpu_type(cpu) (cpu_type(get_cpu_rev()) == cpu)
 
 const char *get_imx_type(u32 imxtype);
+
 unsigned imx_ddr_size(void);
 
+/* Set standard board and cpu environment variables.
+ * for use in loading DTB files.
+ *
+ * Call these in board_late_init if needed
+ */
+void set_cpu_env(void);
+void set_board_env(void);
+
 void set_vddsoc(u32 mv);
 
 /*
-- 
1.8.1.2

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

[Marek Vasut]

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

> +void __weak set_board_env(void)
> +{
> +	char *old = getenv("board");
> +	if (!old)
> +		setenv("board", CONFIG_SYS_BOARD);
> +}

This might go into arch/FOO/lib/board.c ?
[...]

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

[Eric Nelson]

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@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'm just hoping to get a trivial amount of machinery so a boot
script can figure out which of a set of DTBs to load:

	imx6q-nitrogen6x.dtb,
	imx6dl-nitrogen6x.dtb, or
	imx6s-nitrogen6x.dtb

Fabio's recent patch for SABRE SD showed the same need.

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

Regards,


Eric

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

[Marek Vasut]

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

> I'm just hoping to get a trivial amount of machinery so a boot
> script can figure out which of a set of DTBs to load:
> 
> 	imx6q-nitrogen6x.dtb,
> 	imx6dl-nitrogen6x.dtb, or
> 	imx6s-nitrogen6x.dtb
> 
> Fabio's recent patch for SABRE SD showed the same need.

Full agreement here.

> 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 ;-)

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

[Eric Nelson]

On 11/14/2013 04:30 PM, Marek Vasut wrote:
> Hi Eric,
>> Thanks Marek,
>> On 11/14/2013 02:22 PM, Marek Vasut wrote:
>>> Dear Eric Nelson,
>
 > <snip>
>
>> I'm just hoping to get a trivial amount of machinery so a boot
>> script can figure out which of a set of DTBs to load:
>>
>> 	imx6q-nitrogen6x.dtb,
>> 	imx6dl-nitrogen6x.dtb, or
>> 	imx6s-nitrogen6x.dtb
>>
>> Fabio's recent patch for SABRE SD showed the same need.
>
> Full agreement here.
>
>> 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 ;-)
>

This is much easier for us to do, since we defer much of the
scripting into a boot script.

It will be harder for Fabio to shoe-horn in until we get environment
settings in an external file...

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

[Marek Vasut]

Dear Eric Nelson,

> On 11/14/2013 04:30 PM, Marek Vasut wrote:
> > Hi Eric,
> > 
> >> Thanks Marek,
> >> 
> >> On 11/14/2013 02:22 PM, Marek Vasut wrote:
> >>> Dear Eric Nelson,
>  > 
>  > <snip>
>  > 
> >> I'm just hoping to get a trivial amount of machinery so a boot
> >> 
> >> script can figure out which of a set of DTBs to load:
> >> 	imx6q-nitrogen6x.dtb,
> >> 	imx6dl-nitrogen6x.dtb, or
> >> 	imx6s-nitrogen6x.dtb
> >> 
> >> Fabio's recent patch for SABRE SD showed the same need.
> > 
> > Full agreement here.
> > 
> >> 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 ;-)
> 
> This is much easier for us to do, since we defer much of the
> scripting into a boot script.
> 
> It will be harder for Fabio to shoe-horn in until we get environment
> settings in an external file...

Yeah, patching the env unto u-boot is the way to go here I guess. What's the 
progress on this anyway, Tom ?

Best regards,
Marek Vasut

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

[Tom Rini]

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

[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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131115/8d6b431e/attachment.pgp>

[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@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

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

[Fabio Estevam]

Hi Eric,

On Thu, Nov 14, 2013 at 7:38 PM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

> I'm just hoping to get a trivial amount of machinery so a boot
> script can figure out which of a set of DTBs to load:
>
>         imx6q-nitrogen6x.dtb,
>         imx6dl-nitrogen6x.dtb, or
>         imx6s-nitrogen6x.dtb

imx6dl-nitrogen6x.dtb should cover both dl and solo variants, so no
need for a imx6s-nitrogen6x.dtb version.

Regards,

Fabio Estevam

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

[Eric Nelson]

On 11/16/2013 07:03 AM, Fabio Estevam wrote:
> Hi Eric,
>
> On Thu, Nov 14, 2013 at 7:38 PM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
>
>> I'm just hoping to get a trivial amount of machinery so a boot
>> script can figure out which of a set of DTBs to load:
>>
>>          imx6q-nitrogen6x.dtb,
>>          imx6dl-nitrogen6x.dtb, or
>>          imx6s-nitrogen6x.dtb
>
> imx6dl-nitrogen6x.dtb should cover both dl and solo variants, so no
> need for a imx6s-nitrogen6x.dtb version.
>

Thanks Fabio.