[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Guillaume GARDET]

Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support to set 'board_rev' and 
'board_name' envs.

Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>

Cc: Stephen Warren <swarren@wwwdotorg.org>

---
 board/raspberrypi/rpi/rpi.c  | 6 ++++++
 include/configs/rpi-common.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index d21750e..b61ff67 100644
--- 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;
 }
 
diff --git a/include/configs/rpi-common.h b/include/configs/rpi-common.h
index 8830a10..17237cf 100644
--- a/include/configs/rpi-common.h
+++ b/include/configs/rpi-common.h
@@ -133,6 +133,7 @@
 #include <config_distro_defaults.h>
 
 /* Environment */
+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
 #define ENV_DEVICE_SETTINGS \
 	"stdin=serial,lcd\0" \
 	"stdout=serial,lcd\0" \
-- 
1.8.4.5

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Stephen Warren]

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?

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

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.

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Guillaume Gardet]

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.

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


Guillaume

[U-Boot] [PATCH V2] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Guillaume GARDET]

Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support and enable it to set 
'board_rev' and 'board_name' envs.
'board_rev' can be used in scripts to determine what board we are running on 
and 'board_name' for pretty printing.


Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>

Cc: Stephen Warren <swarren@wwwdotorg.org>

---

Changes in V2:
  * Move setenvs to set_board_info() function
  * Expand patch log


 board/raspberrypi/rpi/rpi.c  | 13 +++++++++++++
 include/configs/rpi-common.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index d21750e..dc6f096 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -236,10 +236,23 @@ static void set_usbethaddr(void)
 	return;
 }
 
+#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
+static void set_board_info(void)
+{
+	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 /* CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG */
+
 int misc_init_r(void)
 {
 	set_fdtfile();
 	set_usbethaddr();
+#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
+	set_board_info();
+#endif
 	return 0;
 }
 
diff --git a/include/configs/rpi-common.h b/include/configs/rpi-common.h
index 8830a10..17237cf 100644
--- a/include/configs/rpi-common.h
+++ b/include/configs/rpi-common.h
@@ -133,6 +133,7 @@
 #include <config_distro_defaults.h>
 
 /* Environment */
+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
 #define ENV_DEVICE_SETTINGS \
 	"stdin=serial,lcd\0" \
 	"stdout=serial,lcd\0" \
-- 
1.8.4.5

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Stephen Warren]

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.

[U-Boot] [PATCH V2] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Stephen Warren]

On 08/25/2015 07:10 AM, Guillaume GARDET wrote:
> Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support and enable it to set 
> 'board_rev' and 'board_name' envs.
> 'board_rev' can be used in scripts to determine what board we are running on 
> and 'board_name' for pretty printing.

The patch itself looks clean enough now, although like I said in
response to V1 I'm still a little worried about the intended use-case,
and do think there are likely better ways of solving any issue. Still, I
guess the board_rev values are driven by the RPi firmware itself, so
they're OK as an ABI.

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Ian Lepore]

On Tue, 2015-08-25 at 19:34 -0600, Stephen Warren wrote:
> 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.
> 

You speak of "the kernel" as if there were only one.  FreeBSD also uses
dtb files, and we often have to do local patches to u-boot source to
make it emit identifying info such as soc type/model/rev and board rev
so that we can choose the right dtb file.

The way we handle dtb files is nothing like what linux does, but we use
the same dts source and dtb files as linux.  In some cases we need
u-boot to load those files, in other cases we just need some u-boot
script code to set fdt_file to the right name, then our next-stage
kernel loader handles the actual loading of the file.  Who does the
loading isn't as important as being able to choose the right file to
load, given an sdcard image that contains a single kernel that works for
a variety of related boards, but multiple dtb files, one per board.

-- Ian

[U-Boot] [PATCH] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Stephen Warren]

On 08/25/2015 07:46 PM, Ian Lepore wrote:
> On Tue, 2015-08-25 at 19:34 -0600, Stephen Warren wrote:
>> 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.
> 
> You speak of "the kernel" as if there were only one.

You're right; I should have said "DT file repository" not "kernel" here.
In practice, that's the Linux kernel at present though.

Still, I stand by my assertion that the values U-Boot expects for DTB
filenames make sense, and we should update any/all repositories of DTB
files so there is a complete set of matching files.

[U-Boot] [U-Boot, V2] ARM: rpi: Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support

[Tom Rini]

On Tue, Aug 25, 2015 at 03:10:26PM +0200, Guillaume GARDET wrote:

> Add CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG support and enable it to set 
> 'board_rev' and 'board_name' envs.
> 'board_rev' can be used in scripts to determine what board we are running on 
> and 'board_name' for pretty printing.
> 
> 
> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> 
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151024/1bb8a761/attachment.sig>