[U-Boot] [PATCH V3 0/5] imx: Define common routines to set cpu and board environment variables
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(-)
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);
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
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
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
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
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;
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;
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
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
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
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 */
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
participants (3)
-
Eric Nelson -
Marek Vasut -
Stefano Babic