On 08/21/2015 03:47 AM, Guillaume Gardet wrote:
Le 19/08/2015 05:14, Stephen Warren a écrit :
On 08/18/2015 08:03 AM, Guillaume GARDET wrote:
Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support to set 'board_rev' and 'board_name' envs.
That states what the patch does rather than why its useful to do it. Can you expand on why it's useful to set these variables?
Using boot scripts you may need to get the board version / revision infos, for example to select the right DTB since u-boot DTB names and kernel DTB files do not match.
The fix here isn't to craft all kinds of complex scripts in U-Boot based on a slew of extra variables. Rather, the set of DT files in the kernel should be expanded so that there is one DT per board design, i.e. so the filenames in the kernel match the filenames that U-Boot expects.
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c @@ -240,6 +240,12 @@ int misc_init_r(void) { set_fdtfile(); set_usbethaddr(); +#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
- char str_rev[11];
- sprintf(str_rev, "0x%X", rpi_board_rev);
- setenv("board_rev", str_rev);
- setenv("board_name", models[rpi_board_rev].name);
+#endif return 0; }
That adds a variable declaration in the middle of code. I'd suggest moving the new code into set_board_info() (a name that some other boards/SoCs that honor CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG use), and calling that function inside the ifdef in misc_init_r(). You can always make the function static and implement it before misc_init_r() so that the compiler is likely to inline it.
Ok, will do that.
I'm not sure that models[rpi_board_rev].name contains values that are useful to place into an environment variable. It depends what you expect to do with that variable. Note that the values are not unique, and contain spaces, which might make the value hard to use and/or not reliable to differentiate between all the different types of boards.
Conceptually I have no general objection to this patch, although I am a little worried about turning the board_name variable into some kind of ABI.
board_name variable should be considered to get a pretty board info print, not any kind of ABI. If people need to differentiate boards revs, just use board_rev variable.
The values in models[] are already printed when U-Boot starts.
If those are the only justifications for this patch, I'm not sure it's a good idea.