diff mbox

[U-Boot,v2,5/5] RFC: tegra: Convert to using environment files

Message ID 1372106765-18401-6-git-send-email-sjg@chromium.org
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Simon Glass June 24, 2013, 8:46 p.m. UTC 
This seems more intuitive that the current #define way of doing things.
The resulting code is shorter, avoids the quoting and line continuation
pain, and also improves the clumsy way that stdio variables are created:

 #ifdef CONFIG_VIDEO_TEGRA
 #define STDOUT_LCD ",lcd"
 #else
 #define STDOUT_LCD ""
 #endif

...
 #define TEGRA_DEVICE_SETTINGS \
	"stdout=serial" STDOUT_LCD "\0" \
	...

The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
it depends on the SOC type and we probably don't want to add .emv files
for each board at this stage.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to illustrate the impact on Tegra environment

 board/nvidia/env/common.env         |  79 ++++++++++++++++++++++++
 include/configs/tegra-common-post.h | 120 +-----------------------------------
 2 files changed, 80 insertions(+), 119 deletions(-)
 create mode 100644 board/nvidia/env/common.env

Comments

Stephen Warren June 26, 2013, 8:16 p.m. UTC | #1 
On 06/24/2013 02:46 PM, Simon Glass wrote:
> This seems more intuitive that the current #define way of doing things.
> The resulting code is shorter, avoids the quoting and line continuation
> pain, and also improves the clumsy way that stdio variables are created:
> 
>  #ifdef CONFIG_VIDEO_TEGRA
>  #define STDOUT_LCD ",lcd"
>  #else
>  #define STDOUT_LCD ""
>  #endif
> 
> ...
>  #define TEGRA_DEVICE_SETTINGS \
> 	"stdout=serial" STDOUT_LCD "\0" \
> 	...
> 
> The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
> it depends on the SOC type and we probably don't want to add .emv files
> for each board at this stage.

Presumably e.g. seaboard.env could #include "tegra20.env" in order to
allow *.env to define MEM_LAYOUT_ENV_SETTINGS?

BTW, what's the #include -I path for these files? Perhaps it's worth
setting it up as board/$vendor/$board/env board/$vendor/env
arch/$arch/env to match the dtc+cpp patches I posted?

> diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

> +bootcmd_mmc0=setenv devnum 0; run mmc_boot
> +bootcmd_mmc1=setenv devnum 1; run mmc_booxt
> +boot_targets+= mmc1 mmc0

The first two lines there have no space after = and the last has space
after +=. Does that get stripped? Should it?

> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h

> -#ifdef CONFIG_BOOTCOMMAND
> -
> -#define BOOTCMDS_COMMON ""
> -
> -#else
...

Overall this change seems reasonable, but as I alluded to earlier,
removing this from tegra-common-post.h will break Tegra boards for
vendors other than NVIDIA. I guess that's part of why this patch is RFC.
Simon Glass Oct. 20, 2013, 9:15 p.m. UTC | #2 
Hi Stephen,

On Wed, Jun 26, 2013 at 2:16 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/24/2013 02:46 PM, Simon Glass wrote:
>> This seems more intuitive that the current #define way of doing things.
>> The resulting code is shorter, avoids the quoting and line continuation
>> pain, and also improves the clumsy way that stdio variables are created:
>>
>>  #ifdef CONFIG_VIDEO_TEGRA
>>  #define STDOUT_LCD ",lcd"
>>  #else
>>  #define STDOUT_LCD ""
>>  #endif
>>
>> ...
>>  #define TEGRA_DEVICE_SETTINGS \
>>       "stdout=serial" STDOUT_LCD "\0" \
>>       ...
>>
>> The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since
>> it depends on the SOC type and we probably don't want to add .emv files
>> for each board at this stage.
>
> Presumably e.g. seaboard.env could #include "tegra20.env" in order to
> allow *.env to define MEM_LAYOUT_ENV_SETTINGS?

Hmmm maybe.

>
> BTW, what's the #include -I path for these files? Perhaps it's worth
> setting it up as board/$vendor/$board/env board/$vendor/env
> arch/$arch/env to match the dtc+cpp patches I posted?

We could do that. I can't think of a down-side, but we need to decide
on where these files should go and what each directory should contain.
I am thinking that perhaps arch/arm/env might make sense and
arch/arm/cpu/armv7/env also.

On the other hand, I'm really not keen to take this all to far on a
first pass, and gain experience with it before building in lots of
bells and whistles.

>
>> diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env
>
>> +bootcmd_mmc0=setenv devnum 0; run mmc_boot
>> +bootcmd_mmc1=setenv devnum 1; run mmc_booxt
>> +boot_targets+= mmc1 mmc0
>
> The first two lines there have no space after = and the last has space
> after +=. Does that get stripped? Should it?

We do need that space. There is no stripping done here as it reduces
flexibility.

>
>> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
>
>> -#ifdef CONFIG_BOOTCOMMAND
>> -
>> -#define BOOTCMDS_COMMON ""
>> -
>> -#else
> ...
>
> Overall this change seems reasonable, but as I alluded to earlier,
> removing this from tegra-common-post.h will break Tegra boards for
> vendors other than NVIDIA. I guess that's part of why this patch is RFC.

Yes, that patch would need a bit of testing plus coming up with a
solution to the problem you identified (see my comments above).

Regards,
Simon
diff mbox

Patch

diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env
new file mode 100644
index 0000000..aca36a5
--- /dev/null
+++ b/board/nvidia/env/common.env
@@ -0,0 +1,79 @@ 
+#ifndef CONFIG_BOOTCOMMAND
+rootpart=1
+script_boot=
+	if load ${devtype} ${devnum}:${rootpart}
+			${scriptaddr} ${prefix}${script}; then
+		echo ${script} found! Executing ...
+		source ${scriptaddr}
+	fi
+
+scan_boot=
+	echo Scanning ${devtype} ${devnum}...
+	for prefix in ${boot_prefixes}; do
+		for script in ${boot_scripts}; do
+			run script_boot
+		done
+	done
+
+boot_targets=
+boot_prefixes=/ /boot/
+boot_scripts=boot.scr.uimg boot.scr
+
+#ifdef CONFIG_CMD_MMC
+mmc_boot=
+	setenv devtype mmc
+		if mmc dev ${devnum}; then
+			run scan_boot
+		fi
+bootcmd_mmc0=setenv devnum 0; run mmc_boot
+bootcmd_mmc1=setenv devnum 1; run mmc_booxt
+boot_targets+= mmc1 mmc0
+#endif
+
+#ifdef CONFIG_CMD_USB
+#define BOOTCMD_INIT_USB run usb_init
+usb_init=
+	if ${usb_need_init}; then
+		set usb_need_init false
+		usb start 0
+	fi
+usb_boot=
+	setenv devtype usb
+	BOOTCMD_INIT_USB
+	if usb dev ${devnum}; then
+		run scan_boot
+	fi
+bootcmd_usb0=setenv devnum 0; run usb_boot
+boot_targets+= usb0
+#else
+#define BOOTCMD_INIT_USB
+#endif
+
+#ifdef CONFIG_CMD_DHCP
+bootcmd_dhcp=
+	BOOTCMD_INIT_USB
+	if dhcp ${scriptaddr} boot.scr.uimg; then
+		source ${scriptaddr}
+	fi
+boot_targets+= dhcp
+#endif
+
+bootcmd=for target in ${boot_targets}; do run bootcmd_${target}; done
+#endif
+
+/* Decide on values for stdio */
+stdin=serial
+stdout=serial
+stderr=serial
+
+#ifdef CONFIG_TEGRA_KEYBOARD
+stdin+=,tegra-kbc
+#endif
+
+#ifdef CONFIG_USB_KEYBOARD
+stdin+=,usbkbd
+#endif
+
+#ifdef CONFIG_VIDEO_TEGRA
+stdout+=,lcd
+#endif
diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index 6ed2fde..e14a0e1 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -24,131 +24,13 @@ 
 #ifndef __TEGRA_COMMON_POST_H
 #define __TEGRA_COMMON_POST_H
 
-#ifdef CONFIG_BOOTCOMMAND
-
-#define BOOTCMDS_COMMON ""
-
-#else
-
-#ifdef CONFIG_CMD_MMC
-#define BOOTCMDS_MMC \
-	"mmc_boot=" \
-		"setenv devtype mmc; " \
-		"if mmc dev ${devnum}; then " \
-			"run scan_boot; " \
-		"fi\0" \
-	"bootcmd_mmc0=setenv devnum 0; run mmc_boot;\0" \
-	"bootcmd_mmc1=setenv devnum 1; run mmc_boot;\0"
-#define BOOT_TARGETS_MMC "mmc1 mmc0"
-#else
-#define BOOTCMDS_MMC ""
-#define BOOT_TARGETS_MMC ""
-#endif
-
-#ifdef CONFIG_CMD_USB
-#define BOOTCMD_INIT_USB "run usb_init; "
-#define BOOTCMDS_USB \
-	"usb_init=" \
-		"if ${usb_need_init}; then " \
-			"set usb_need_init false; " \
-			"usb start 0; " \
-		"fi\0" \
-	\
-	"usb_boot=" \
-		"setenv devtype usb; " \
-		BOOTCMD_INIT_USB \
-		"if usb dev ${devnum}; then " \
-			"run scan_boot; " \
-		"fi\0" \
-	\
-	"bootcmd_usb0=setenv devnum 0; run usb_boot;\0"
-#define BOOT_TARGETS_USB "usb0"
-#else
-#define BOOTCMD_INIT_USB ""
-#define BOOTCMDS_USB ""
-#define BOOT_TARGETS_USB ""
-#endif
-
-#ifdef CONFIG_CMD_DHCP
-#define BOOTCMDS_DHCP \
-	"bootcmd_dhcp=" \
-		BOOTCMD_INIT_USB \
-		"if dhcp ${scriptaddr} boot.scr.uimg; then "\
-			"source ${scriptaddr}; " \
-		"fi\0"
-#define BOOT_TARGETS_DHCP "dhcp"
-#else
-#define BOOTCMDS_DHCP ""
-#define BOOT_TARGETS_DHCP ""
-#endif
-
-#define BOOTCMDS_COMMON \
-	"rootpart=1\0" \
-	\
-	"script_boot="                                                    \
-		"if load ${devtype} ${devnum}:${rootpart} "               \
-				"${scriptaddr} ${prefix}${script}; then " \
-			"echo ${script} found! Executing ...;"            \
-			"source ${scriptaddr};"                           \
-		"fi;\0"                                                   \
-	\
-	"scan_boot="                                                      \
-		"echo Scanning ${devtype} ${devnum}...; "                 \
-		"for prefix in ${boot_prefixes}; do "                     \
-			"for script in ${boot_scripts}; do "              \
-				"run script_boot; "                       \
-			"done; "                                          \
-		"done;\0"                                                 \
-	\
-	"boot_targets=" \
-		BOOT_TARGETS_MMC " " \
-		BOOT_TARGETS_USB " " \
-		BOOT_TARGETS_DHCP " " \
-		"\0" \
-	\
-	"boot_prefixes=/ /boot/\0" \
-	\
-	"boot_scripts=boot.scr.uimg boot.scr\0" \
-	\
-	BOOTCMDS_MMC \
-	BOOTCMDS_USB \
-	BOOTCMDS_DHCP
-
-#define CONFIG_BOOTCOMMAND \
-	"for target in ${boot_targets}; do run bootcmd_${target}; done"
-
-#endif
-
-#ifdef CONFIG_TEGRA_KEYBOARD
-#define STDIN_KBD_KBC ",tegra-kbc"
-#else
-#define STDIN_KBD_KBC ""
-#endif
-
 #ifdef CONFIG_USB_KEYBOARD
-#define STDIN_KBD_USB ",usbkbd"
 #define CONFIG_SYS_USB_EVENT_POLL
 #define CONFIG_PREBOOT			"usb start"
-#else
-#define STDIN_KBD_USB ""
 #endif
 
-#ifdef CONFIG_VIDEO_TEGRA
-#define STDOUT_LCD ",lcd"
-#else
-#define STDOUT_LCD ""
-#endif
-
-#define TEGRA_DEVICE_SETTINGS \
-	"stdin=serial" STDIN_KBD_KBC STDIN_KBD_USB "\0" \
-	"stdout=serial" STDOUT_LCD "\0" \
-	"stderr=serial" STDOUT_LCD "\0" \
-	""
-
 #define CONFIG_EXTRA_ENV_SETTINGS \
-	TEGRA_DEVICE_SETTINGS \
-	MEM_LAYOUT_ENV_SETTINGS \
-	BOOTCMDS_COMMON
+	MEM_LAYOUT_ENV_SETTINGS
 
 #if defined(CONFIG_TEGRA20_SFLASH) || defined(CONFIG_TEGRA20_SLINK) || defined(CONFIG_TEGRA114_SPI)
 #define CONFIG_FDT_SPI