[U-Boot] [PATCH V3 0/5] imx: Define common routines to set cpu and board environment variables

[U-Boot] [PATCH V3 0/5] imx: Define common routines to set cpu and board environment variables

[Eric Nelson]

This series adds support for two environment variables that are
useful for use in scripting startup of an O/S in general, and
specifically in computing DTB names.

Patches 1 and 2 implement the feature through arch_misc_init(), but
don't enable it.

Patch 3 overrides board_name for the nitrogen6x board file, which is
used for both Nitrogen6X and SABRE Lite boards.

Patch 4 enables the feature for nitrogen6x

Patch 5 enables the feature for Freescale mx6*sabre* boards

V3 reworks things completely based on feedback from the mailing list.
In particular:

--	the variable names were changes from "cpu" and "board" to
	"imx_type" and "board_name"
--	the arch_misc_init() routine was added
--	SABRE Lite environment was changed to use the feature

Eric Nelson (5):
  i.MX5x: define cpu_type() to return processor portion of cpu rev.
  imx: Define common routines to set cpu and board environment variables
  i.MX6: nitrogen6x/sabrelite: override set_board_name()
  i.MX6: nitrogen6x/sabrelite: initialize imx_type and board_name values
  i.MX6: mx6*sabre*: initialize imx_type and board_name values

 arch/arm/imx-common/cpu.c                 | 24 ++++++++++++++++++++++--
 arch/arm/include/asm/arch-mx5/sys_proto.h |  4 ++++
 board/boundary/nitrogen6x/nitrogen6x.c    | 14 +++++++++++++-
 include/configs/mx6sabre_common.h         |  1 +
 include/configs/nitrogen6x.h              |  9 ++++++++-
 5 files changed, 48 insertions(+), 4 deletions(-)

-- 
1.8.1.2

[U-Boot] [PATCH V3 1/5] i.MX5x: define cpu_type() to return processor portion of cpu rev.

[Eric Nelson]

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
This patch is new in V3

 arch/arm/include/asm/arch-mx5/sys_proto.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
index 9949ad1..9dad5fc 100644
--- a/arch/arm/include/asm/arch-mx5/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
@@ -17,6 +17,10 @@
 
 #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
 u32 get_cpu_rev(void);
+
+/* returns MXC_CPU_ value */
+#define cpu_type(rev) (((rev) >> 12)&0xff)
+
 unsigned imx_ddr_size(void);
 void sdelay(unsigned long);
 void set_chipselect_size(int const);
-- 
1.8.1.2

[U-Boot] [PATCH V3 1/5] i.MX5x: define cpu_type() to return processor portion of cpu rev.

[Marek Vasut]

Hi Eric,

> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> ---
> This patch is new in V3
> 
>  arch/arm/include/asm/arch-mx5/sys_proto.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h
> b/arch/arm/include/asm/arch-mx5/sys_proto.h index 9949ad1..9dad5fc 100644
> --- a/arch/arm/include/asm/arch-mx5/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
> @@ -17,6 +17,10 @@
> 
>  #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
>  u32 get_cpu_rev(void);
> +
> +/* returns MXC_CPU_ value */
> +#define cpu_type(rev) (((rev) >> 12)&0xff)

Maybe you can implement it the same way as get_cpu_rev() and make it call 
get_cpu_rev() internally ? Then it'd be a function (with all the typechecking 
and stuff) and you won't need to pass extra param.

Best regards,
Marek Vasut

[U-Boot] [PATCH V3 1/5] i.MX5x: define cpu_type() to return processor portion of cpu rev.

[Stefano Babic]

Hi Eric,

On 17/11/2013 18:17, Eric Nelson wrote:
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> ---
> This patch is new in V3
> 
>  arch/arm/include/asm/arch-mx5/sys_proto.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
> index 9949ad1..9dad5fc 100644
> --- a/arch/arm/include/asm/arch-mx5/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
> @@ -17,6 +17,10 @@
>  
>  #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
>  u32 get_cpu_rev(void);
> +
> +/* returns MXC_CPU_ value */
> +#define cpu_type(rev) (((rev) >> 12)&0xff)
> +

There is already a get_cpu_type() for other architectures (OMAP). We do
not need to reinvent the wheel this time, and it is correct to add
get_cpu_type(void) to sys_proto.h.

This lets also easier to understand the code because it can be directly
derived from the User's Manual: shifting 12 bit in your macro is only
because this is done in get_cpu_rev(), not because this is the offset in
the i.MX6 register.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH V3 1/5] i.MX5x: define cpu_type() to return processor portion of cpu rev.

[Eric Nelson]

Hi Stefano,
On 11/18/2013 03:42 AM, Stefano Babic wrote:
> Hi Eric,
>
> On 17/11/2013 18:17, Eric Nelson wrote:
>> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
>> ---
>> This patch is new in V3
>>
>>   arch/arm/include/asm/arch-mx5/sys_proto.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
>> index 9949ad1..9dad5fc 100644
>> --- a/arch/arm/include/asm/arch-mx5/sys_proto.h
>> +++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
>> @@ -17,6 +17,10 @@
>>
>>   #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
>>   u32 get_cpu_rev(void);
>> +
>> +/* returns MXC_CPU_ value */
>> +#define cpu_type(rev) (((rev) >> 12)&0xff)
>> +
>
> There is already a get_cpu_type() for other architectures (OMAP). We do
> not need to reinvent the wheel this time, and it is correct to add
> get_cpu_type(void) to sys_proto.h.
>
> This lets also easier to understand the code because it can be directly
> derived from the User's Manual: shifting 12 bit in your macro is only
> because this is done in get_cpu_rev(), not because this is the offset in
> the i.MX6 register.
>

Okay. I'll re-submit with get_cpu_type(void) implemented imx-common/cpu.c.

I still question the fact that we have two header files for i.MX5x
and i.MX6x declaring the returns implemented there.

It seems that we should have a single header for routines
implemented there.

Perhaps arch/arm/include/imx-common/cpu.h?

Please advise,


Eric

[U-Boot] [PATCH V3 1/5] i.MX5x: define cpu_type() to return processor portion of cpu rev.

[Stefano Babic]

Hi Eric,

On 19/11/2013 04:25, Eric Nelson wrote:

> I still question the fact that we have two header files for i.MX5x
> and i.MX6x declaring the returns implemented there.

Good point.

> 
> It seems that we should have a single header for routines
> implemented there.
> 
> Perhaps arch/arm/include/imx-common/cpu.h?

+1

> 
> Please advise,

Nice if you clean up also this point !

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

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

[Eric Nelson]

Defines two environment variables for use in producing DTB file
names, among other uses:

imx_type: defines the CPU variant through the get_imx_type() routine
board_name: environment variable equivalent of CONFIG_SYS_BOARD_NAME

Both can be over-ridden by a user. This is expected to be most useful
when transitioning to a custom board.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 arch/arm/imx-common/cpu.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
index 0cd2538..7c434c7 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,28 @@ const char *get_imx_type(u32 imxtype)
 	}
 }
 
+#ifdef CONFIG_ARCH_MISC_INIT
+void __weak set_imx_type(void)
+{
+	setenv("imx_type", get_imx_type(cpu_type(get_cpu_rev())));
+}
+
+void __weak set_board_name(void)
+{
+	char *old = getenv("board_name");
+	if (!old)
+		setenv("board_name", CONFIG_SYS_BOARD);
+}
+
+int arch_misc_init(void)
+{
+	set_imx_type();
+	set_board_name();
+	return 0;
+}
+#endif
+
+#if defined(CONFIG_DISPLAY_CPUINFO)
 int print_cpuinfo(void)
 {
 	u32 cpurev;
-- 
1.8.1.2

[U-Boot] [PATCH 3/5] i.MX6: nitrogen6x/sabrelite: override set_board_name()

[Eric Nelson]

Since the nitrogen6x board file auto-detects Nitrogen6x and
SABRE Lite boards, override set_board_name to produce one
of two values for board_name.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 board/boundary/nitrogen6x/nitrogen6x.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
index 616ad55..aa9717a 100644
--- a/board/boundary/nitrogen6x/nitrogen6x.c
+++ b/board/boundary/nitrogen6x/nitrogen6x.c
@@ -756,9 +756,14 @@ int board_init(void)
 	return 0;
 }
 
+static inline int is_n6x(void)
+{
+	return gpio_get_value(WL12XX_WL_IRQ_GP);
+}
+
 int checkboard(void)
 {
-	if (gpio_get_value(WL12XX_WL_IRQ_GP))
+	if (is_n6x())
 		puts("Board: Nitrogen6X\n");
 	else
 		puts("Board: SABRE Lite\n");
@@ -766,6 +771,13 @@ int checkboard(void)
 	return 0;
 }
 
+void set_board_name(void)
+{
+	char *old = getenv("board_name");
+	if (!old)
+		setenv("board_name", is_n6x() ? "nitrogen6x" : "sabrelite");
+}
+
 struct button_key {
 	char const	*name;
 	unsigned	gpnum;
-- 
1.8.1.2

[U-Boot] [PATCH 3/5] i.MX6: nitrogen6x/sabrelite: override set_board_name()

[Stefano Babic]

Hi Eric,

On 17/11/2013 18:17, Eric Nelson wrote:
> Since the nitrogen6x board file auto-detects Nitrogen6x and
> SABRE Lite boards, override set_board_name to produce one
> of two values for board_name.
> 
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> ---
>  board/boundary/nitrogen6x/nitrogen6x.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
> index 616ad55..aa9717a 100644
> --- a/board/boundary/nitrogen6x/nitrogen6x.c
> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
> @@ -756,9 +756,14 @@ int board_init(void)
>  	return 0;
>  }
>  
> +static inline int is_n6x(void)
> +{
> +	return gpio_get_value(WL12XX_WL_IRQ_GP);
> +}
> +
>  int checkboard(void)
>  {
> -	if (gpio_get_value(WL12XX_WL_IRQ_GP))
> +	if (is_n6x())
>  		puts("Board: Nitrogen6X\n");
>  	else
>  		puts("Board: SABRE Lite\n");
> @@ -766,6 +771,13 @@ int checkboard(void)
>  	return 0;
>  }
>  
> +void set_board_name(void)
> +{
> +	char *old = getenv("board_name");

Agree on the name: board_name was already introduced in u-boot.

> +	if (!old)
> +		setenv("board_name", is_n6x() ? "nitrogen6x" : "sabrelite");

I have a major question: if it is possible to detect at runtime, as you
have already implemented, which is the board where code is running, why
is it possible to override it for the operator ? I agree that forcing
environment variables inside code is bad, but in this case it is a
hardware related stuff. It is like to the processor type or the
serial-id of the processor (variable dieid# on OMAP). Overriding seems
weird.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 3/5] i.MX6: nitrogen6x/sabrelite: override set_board_name()

[Eric Nelson]

Hi Stefano,

On 11/18/2013 03:57 AM, Stefano Babic wrote:
> Hi Eric,
>
> On 17/11/2013 18:17, Eric Nelson wrote:
>> Since the nitrogen6x board file auto-detects Nitrogen6x and
>> SABRE Lite boards, override set_board_name to produce one
>> of two values for board_name.
>>
>> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
>> ---
>>   board/boundary/nitrogen6x/nitrogen6x.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
>> index 616ad55..aa9717a 100644
>> --- a/board/boundary/nitrogen6x/nitrogen6x.c
>> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
>> @@ -756,9 +756,14 @@ int board_init(void)
>>   	return 0;
>>   }
>>
>> +static inline int is_n6x(void)
>> +{
>> +	return gpio_get_value(WL12XX_WL_IRQ_GP);
>> +}
>> +
>>   int checkboard(void)
>>   {
>> -	if (gpio_get_value(WL12XX_WL_IRQ_GP))
>> +	if (is_n6x())
>>   		puts("Board: Nitrogen6X\n");
>>   	else
>>   		puts("Board: SABRE Lite\n");
>> @@ -766,6 +771,13 @@ int checkboard(void)
>>   	return 0;
>>   }
>>
>> +void set_board_name(void)user
>> +{
>> +	char *old = getenv("board_name");
>
> Agree on the name: board_name was already introduced in u-boot.
>
>> +	if (!old)
>> +		setenv("board_name", is_n6x() ? "nitrogen6x" : "sabrelite");
>
> I have a major question: if it is possible to detect at runtime, as you
> have already implemented, which is the board where code is running, why
> is it possible to override it for the operator ?
 >
First off, I want to clarify things. There are two levels of override
possible here:
	- weak functions can be over-ridden by board files
	- environment variables can be overridden through saveenv()

The first is to allow auto-detection of a "board" as shown in
nitrogen6x.c. I assume you're okay with that bit.

 > I agree that forcing environment variables inside code is bad, but
 > in this case it is a hardware related stuff. It is like to the
 > processor type or the serial-id of the processor (variable dieid#
 > on OMAP). Overriding seems weird.
>
There is a reason for this, and it mostly involves custom pin-muxing.

All of our boards support a wide variety of peripherals, and we make
assumptions about what the various connectors will be used for.

For instance, we define a particular connector for use as a parallel
(RGB) display.

Lots of customers have other needs though, and through the magic of
pin-muxing, they build their own "board" files in the kernel and
use the parallel display pins for connecting SPI devices or additional
I2C or RS-232 pins. Or simply as GPIOs.

The easiest, most maintainable way to do that is by cloning our board
files and editing the pin muxes.

With the addition of device tree support, this becomes even easier.

So is it a different board? Maybe not, but it's useful to name things
differently in the kernel tree so you can easily perform a diff
against the original or boot to the original DTB for comparison.

SOM customers blur the lines even further, and it's not uncommon
for someone to boot a different carrier with our stock U-Boot if
the changes are minor or their needs are modest.

Re-reading the patch, it seems that there's no good reason to have
set_imx_type(void) declared __weak.

Regards,


Eric

[U-Boot] [PATCH 3/5] i.MX6: nitrogen6x/sabrelite: override set_board_name()

[Stefano Babic]

Hi Eric,

On 19/11/2013 04:40, Eric Nelson wrote:

>> I have a major question: if it is possible to detect at runtime, as you
>> have already implemented, which is the board where code is running, why
>> is it possible to override it for the operator ?
>>
> First off, I want to clarify things. There are two levels of override
> possible here:
>     - weak functions can be over-ridden by board files
>     - environment variables can be overridden through saveenv()
> 
> The first is to allow auto-detection of a "board" as shown in
> nitrogen6x.c. I assume you're okay with that bit.

Right.

>> I agree that forcing environment variables inside code is bad, but
>> in this case it is a hardware related stuff. It is like to the
>> processor type or the serial-id of the processor (variable dieid#
>> on OMAP). Overriding seems weird.
>>
> There is a reason for this, and it mostly involves custom pin-muxing.
> 
> All of our boards support a wide variety of peripherals, and we make
> assumptions about what the various connectors will be used for.
> 
> For instance, we define a particular connector for use as a parallel
> (RGB) display.
> 
> Lots of customers have other needs though, and through the magic of
> pin-muxing, they build their own "board" files in the kernel and
> use the parallel display pins for connecting SPI devices or additional
> I2C or RS-232 pins. Or simply as GPIOs.
> 
> The easiest, most maintainable way to do that is by cloning our board
> files and editing the pin muxes.
> 
> With the addition of device tree support, this becomes even easier.
> 
> So is it a different board? Maybe not, but it's useful to name things
> differently in the kernel tree so you can easily perform a diff
> against the original or boot to the original DTB for comparison.
> 
> SOM customers blur the lines even further, and it's not uncommon
> for someone to boot a different carrier with our stock U-Boot if
> the changes are minor or their needs are modest.

ok, understood, thanks for clarification.

> 
> Re-reading the patch, it seems that there's no good reason to have
> set_imx_type(void) declared __weak.

Agree.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 4/5] i.MX6: nitrogen6x/sabrelite: initialize imx_type and board_name values

[Eric Nelson]

Use them to produce FDT file name in mx6qsabrelite_config.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 include/configs/nitrogen6x.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/configs/nitrogen6x.h b/include/configs/nitrogen6x.h
index 3df8de0..361e69d 100644
--- a/include/configs/nitrogen6x.h
+++ b/include/configs/nitrogen6x.h
@@ -188,7 +188,6 @@
 	"console=ttymxc1\0" \
 	"fdt_high=0xffffffff\0" \
 	"initrd_high=0xffffffff\0" \
-	"fdt_file=imx6q-sabrelite.dtb\0" \
 	"fdt_addr=0x11000000\0" \
 	"boot_fdt=try\0" \
 	"ip_dyn=yes\0" \
@@ -244,6 +243,12 @@
 		"fi;\0"
 
 #define CONFIG_BOOTCOMMAND \
+	   "if itest.s x6Q == x${imx_type}; then " \
+		"fdt_file=imx6q ; " \
+	   "else " \
+		"fdt_file=imx6dl ; " \
+	   "fi; " \
+	   "fdt_file=${fdt_file}-${board_name}.dtb;" \
 	   "mmc dev ${mmcdev}; if mmc rescan; then " \
 		   "if run loadbootscript; then " \
 			   "run bootscript; " \
@@ -361,4 +366,6 @@
 #define CONFIG_SUPPORT_RAW_INITRD
 #define CONFIG_CMD_FS_GENERIC
 
+#define CONFIG_ARCH_MISC_INIT
+
 #endif	       /* __CONFIG_H */
-- 
1.8.1.2

[U-Boot] [PATCH 5/5] i.MX6: mx6*sabre*: initialize imx_type and board_name values

[Eric Nelson]

These will need to be computed if/when boards support multiple
processor variants.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 include/configs/mx6sabre_common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/mx6sabre_common.h b/include/configs/mx6sabre_common.h
index 79d1f34..092b764 100644
--- a/include/configs/mx6sabre_common.h
+++ b/include/configs/mx6sabre_common.h
@@ -27,6 +27,7 @@
 /* Size of malloc() pool */
 #define CONFIG_SYS_MALLOC_LEN		(10 * SZ_1M)
 
+#define CONFIG_ARCH_MISC_INIT
 #define CONFIG_BOARD_EARLY_INIT_F
 #define CONFIG_BOARD_LATE_INIT
 #define CONFIG_MXC_GPIO
-- 
1.8.1.2