[PATCH] cmd: fdt: Add support for reading stringlist property values

[PATCH] cmd: fdt: Add support for reading stringlist property values

[Marek Vasut]

The fdt command currently handles stringlists as strings in 'fdt get value'
subcommand. Since strings in FDT stringlists are separated by '\0', only
the first value gets inserted into the environment variable passed to the
'fdt get value' command.

Example, consider the following DT snippet:

/ { compatible = "foo", "bar" };

The following command only reports the first string in stringlist:
=> fdt get value var / compatible ; print var
foo

It is not possible to assign list of null-terminated strings into U-Boot
environment variable. Add optional 'index' parameter to the subcommand
'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
string within the stringlist should be assigned into the 'var' variable.
The default value of 'index' is 0 in case it is not present. This way the
'fdt' command API does not change and existing scripts are not broken.

The following command now reports the Nth string in stringlist, counting
from zero:
=> fdt get value var / compatible 1 ; print var
bar

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 842e6cb634b..6fbd9205d38 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -48,11 +48,27 @@ void set_working_fdt_addr(ulong addr)
 /*
  * Get a value from the fdt and format it to be set in the environment
  */
-static int fdt_value_env_set(const void *nodep, int len, const char *var)
+static int fdt_value_env_set(const void *nodep, int len,
+			     const char *var, int index)
 {
-	if (is_printable_string(nodep, len))
-		env_set(var, (void *)nodep);
-	else if (len == 4) {
+	if (is_printable_string(nodep, len)) {
+		const char *nodec = (const char *)nodep;
+		int i;
+
+		/*
+		 * Iterate over all members in stringlist and find the one at
+		 * offset $index. If no such index exists, indicate failure.
+		 */
+		for (i = 0; i < len; i += strlen(nodec) + 1) {
+			if (index-- > 0)
+				continue;
+
+			env_set(var, nodec + i);
+			return 0;
+		}
+
+		return 1;
+	} else if (len == 4) {
 		char buf[11];
 
 		sprintf(buf, "0x%08X", fdt32_to_cpu(*(fdt32_t *)nodep));
@@ -426,10 +442,14 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 				return 0;
 			} else if (nodep && len > 0) {
 				if (subcmd[0] == 'v') {
+					int index = 0;
 					int ret;
 
+					if (argc == 7)
+						index = simple_strtoul(argv[6], NULL, 10);
+
 					ret = fdt_value_env_set(nodep, len,
-								var);
+								var, index);
 					if (ret != 0)
 						return ret;
 				} else if (subcmd[0] == 'a') {
@@ -1085,7 +1105,9 @@ static char fdt_help_text[] =
 	"fdt resize [<extrasize>]            - Resize fdt to size + padding to 4k addr + some optional <extrasize> if needed\n"
 	"fdt print  <path> [<prop>]          - Recursive print starting at <path>\n"
 	"fdt list   <path> [<prop>]          - Print one level starting at <path>\n"
-	"fdt get value <var> <path> <prop>   - Get <property> and store in <var>\n"
+	"fdt get value <var> <path> <prop> [<index>] - Get <property> and store in <var>\n"
+	"                                      In case of stringlist property, use optional <index>\n"
+	"                                      to select string within the stringlist. Default is 0.\n"
 	"fdt get name <var> <path> <index>   - Get name of node <index> and store in <var>\n"
 	"fdt get addr <var> <path> <prop>    - Get start address of <property> and store in <var>\n"
 	"fdt get size <var> <path> [<prop>]  - Get size of [<property>] or num nodes and store in <var>\n"
-- 
2.35.1

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

Hi Marek,

On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
>
> The fdt command currently handles stringlists as strings in 'fdt get value'
> subcommand. Since strings in FDT stringlists are separated by '\0', only
> the first value gets inserted into the environment variable passed to the
> 'fdt get value' command.
>
> Example, consider the following DT snippet:
>
> / { compatible = "foo", "bar" };
>
> The following command only reports the first string in stringlist:
> => fdt get value var / compatible ; print var
> foo
>
> It is not possible to assign list of null-terminated strings into U-Boot
> environment variable. Add optional 'index' parameter to the subcommand
> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
> string within the stringlist should be assigned into the 'var' variable.
> The default value of 'index' is 0 in case it is not present. This way the
> 'fdt' command API does not change and existing scripts are not broken.
>
> The following command now reports the Nth string in stringlist, counting
> from zero:
> => fdt get value var / compatible 1 ; print var
> bar
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)

Can you please add docs and a test?

I am happy to do a starting point for a test if you like.

Regards,
Simon

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Marek Vasut]

On 7/12/22 12:58, Simon Glass wrote:
> Hi Marek,
> 
> On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
>>
>> The fdt command currently handles stringlists as strings in 'fdt get value'
>> subcommand. Since strings in FDT stringlists are separated by '\0', only
>> the first value gets inserted into the environment variable passed to the
>> 'fdt get value' command.
>>
>> Example, consider the following DT snippet:
>>
>> / { compatible = "foo", "bar" };
>>
>> The following command only reports the first string in stringlist:
>> => fdt get value var / compatible ; print var
>> foo
>>
>> It is not possible to assign list of null-terminated strings into U-Boot
>> environment variable. Add optional 'index' parameter to the subcommand
>> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
>> string within the stringlist should be assigned into the 'var' variable.
>> The default value of 'index' is 0 in case it is not present. This way the
>> 'fdt' command API does not change and existing scripts are not broken.
>>
>> The following command now reports the Nth string in stringlist, counting
>> from zero:
>> => fdt get value var / compatible 1 ; print var
>> bar
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>   cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
>>   1 file changed, 28 insertions(+), 6 deletions(-)
> 
> Can you please add docs and a test?

I was expecting this kind of question ... there is no test for the FDT 
command.

> I am happy to do a starting point for a test if you like.

You can implement the test for this while at it.

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

Hi Marek,

On Tue, 12 Jul 2022 at 05:47, Marek Vasut <marex@denx.de> wrote:
>
> On 7/12/22 12:58, Simon Glass wrote:
> > Hi Marek,
> >
> > On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
> >>
> >> The fdt command currently handles stringlists as strings in 'fdt get value'
> >> subcommand. Since strings in FDT stringlists are separated by '\0', only
> >> the first value gets inserted into the environment variable passed to the
> >> 'fdt get value' command.
> >>
> >> Example, consider the following DT snippet:
> >>
> >> / { compatible = "foo", "bar" };
> >>
> >> The following command only reports the first string in stringlist:
> >> => fdt get value var / compatible ; print var
> >> foo
> >>
> >> It is not possible to assign list of null-terminated strings into U-Boot
> >> environment variable. Add optional 'index' parameter to the subcommand
> >> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
> >> string within the stringlist should be assigned into the 'var' variable.
> >> The default value of 'index' is 0 in case it is not present. This way the
> >> 'fdt' command API does not change and existing scripts are not broken.
> >>
> >> The following command now reports the Nth string in stringlist, counting
> >> from zero:
> >> => fdt get value var / compatible 1 ; print var
> >> bar
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >>   cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
> >>   1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > Can you please add docs and a test?
>
> I was expecting this kind of question ... there is no test for the FDT
> command.
>
> > I am happy to do a starting point for a test if you like.
>
> You can implement the test for this while at it.

I sent a starting point for you, both docs and test.

Regards,
Simon

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Marek Vasut]

On 7/13/22 17:28, Simon Glass wrote:
> Hi Marek,
> 
> On Tue, 12 Jul 2022 at 05:47, Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/12/22 12:58, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The fdt command currently handles stringlists as strings in 'fdt get value'
>>>> subcommand. Since strings in FDT stringlists are separated by '\0', only
>>>> the first value gets inserted into the environment variable passed to the
>>>> 'fdt get value' command.
>>>>
>>>> Example, consider the following DT snippet:
>>>>
>>>> / { compatible = "foo", "bar" };
>>>>
>>>> The following command only reports the first string in stringlist:
>>>> => fdt get value var / compatible ; print var
>>>> foo
>>>>
>>>> It is not possible to assign list of null-terminated strings into U-Boot
>>>> environment variable. Add optional 'index' parameter to the subcommand
>>>> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
>>>> string within the stringlist should be assigned into the 'var' variable.
>>>> The default value of 'index' is 0 in case it is not present. This way the
>>>> 'fdt' command API does not change and existing scripts are not broken.
>>>>
>>>> The following command now reports the Nth string in stringlist, counting
>>>> from zero:
>>>> => fdt get value var / compatible 1 ; print var
>>>> bar
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>>    cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
>>>>    1 file changed, 28 insertions(+), 6 deletions(-)
>>>
>>> Can you please add docs and a test?
>>
>> I was expecting this kind of question ... there is no test for the FDT
>> command.
>>
>>> I am happy to do a starting point for a test if you like.
>>
>> You can implement the test for this while at it.
> 
> I sent a starting point for you, both docs and test.

Thank you.

Do I understand your remark correctly that you're blocking this 
particular patch from going in until there is a test ?

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

Hi Marek,

On Wed, 13 Jul 2022 at 09:30, Marek Vasut <marex@denx.de> wrote:
>
> On 7/13/22 17:28, Simon Glass wrote:
> > Hi Marek,
> >
> > On Tue, 12 Jul 2022 at 05:47, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/12/22 12:58, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> The fdt command currently handles stringlists as strings in 'fdt get value'
> >>>> subcommand. Since strings in FDT stringlists are separated by '\0', only
> >>>> the first value gets inserted into the environment variable passed to the
> >>>> 'fdt get value' command.
> >>>>
> >>>> Example, consider the following DT snippet:
> >>>>
> >>>> / { compatible = "foo", "bar" };
> >>>>
> >>>> The following command only reports the first string in stringlist:
> >>>> => fdt get value var / compatible ; print var
> >>>> foo
> >>>>
> >>>> It is not possible to assign list of null-terminated strings into U-Boot
> >>>> environment variable. Add optional 'index' parameter to the subcommand
> >>>> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
> >>>> string within the stringlist should be assigned into the 'var' variable.
> >>>> The default value of 'index' is 0 in case it is not present. This way the
> >>>> 'fdt' command API does not change and existing scripts are not broken.
> >>>>
> >>>> The following command now reports the Nth string in stringlist, counting
> >>>> from zero:
> >>>> => fdt get value var / compatible 1 ; print var
> >>>> bar
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> Cc: Simon Glass <sjg@chromium.org>
> >>>> Cc: Tom Rini <trini@konsulko.com>
> >>>> ---
> >>>>    cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
> >>>>    1 file changed, 28 insertions(+), 6 deletions(-)
> >>>
> >>> Can you please add docs and a test?
> >>
> >> I was expecting this kind of question ... there is no test for the FDT
> >> command.
> >>
> >>> I am happy to do a starting point for a test if you like.
> >>
> >> You can implement the test for this while at it.
> >
> > I sent a starting point for you, both docs and test.
>
> Thank you.
>
> Do I understand your remark correctly that you're blocking this
> particular patch from going in until there is a test ?

I'm not blocking anything, but turning it around the other way, how
about having a crack at a test? You can try out 'ut fdt' to see the
current tests (using the patches I sent).

Regards,
Simon

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

Hi Marek,

On Wed, 13 Jul 2022 at 09:30, Marek Vasut <marex@denx.de> wrote:
>
> On 7/13/22 17:28, Simon Glass wrote:
> > Hi Marek,
> >
> > On Tue, 12 Jul 2022 at 05:47, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/12/22 12:58, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On Fri, 8 Jul 2022 at 15:50, Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> The fdt command currently handles stringlists as strings in 'fdt get value'
> >>>> subcommand. Since strings in FDT stringlists are separated by '\0', only
> >>>> the first value gets inserted into the environment variable passed to the
> >>>> 'fdt get value' command.
> >>>>
> >>>> Example, consider the following DT snippet:
> >>>>
> >>>> / { compatible = "foo", "bar" };
> >>>>
> >>>> The following command only reports the first string in stringlist:
> >>>> => fdt get value var / compatible ; print var
> >>>> foo
> >>>>
> >>>> It is not possible to assign list of null-terminated strings into U-Boot
> >>>> environment variable. Add optional 'index' parameter to the subcommand
> >>>> 'fdt get value <var> <path> <prop> [<index>]' which lets user specify which
> >>>> string within the stringlist should be assigned into the 'var' variable.
> >>>> The default value of 'index' is 0 in case it is not present. This way the
> >>>> 'fdt' command API does not change and existing scripts are not broken.
> >>>>
> >>>> The following command now reports the Nth string in stringlist, counting
> >>>> from zero:
> >>>> => fdt get value var / compatible 1 ; print var
> >>>> bar
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> Cc: Simon Glass <sjg@chromium.org>
> >>>> Cc: Tom Rini <trini@konsulko.com>
> >>>> ---
> >>>>    cmd/fdt.c | 34 ++++++++++++++++++++++++++++------
> >>>>    1 file changed, 28 insertions(+), 6 deletions(-)
> >>>
> >>> Can you please add docs and a test?
> >>
> >> I was expecting this kind of question ... there is no test for the FDT
> >> command.
> >>
> >>> I am happy to do a starting point for a test if you like.
> >>
> >> You can implement the test for this while at it.
> >
> > I sent a starting point for you, both docs and test.
>
> Thank you.
>
> Do I understand your remark correctly that you're blocking this
> particular patch from going in until there is a test ?

I'm not blocking anything, but turning it around the other way, how
about having a crack at a test? You can try out 'ut fdt' to see the
current tests (using the patches I sent).

Regards,
Simon

Applied to u-boot-dm, thanks!

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Marek Vasut]

On 7/17/22 10:12, Simon Glass wrote:
> Hi Marek,

Hi,

[...]

> Applied to u-boot-dm, thanks!

This patch is still not in u-boot/master ?

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

On 7/17/22 10:12, Simon Glass wrote:
> Hi Marek,

Hi,

[...]

> Applied to u-boot-dm, thanks!

This patch is still not in u-boot/master ?

Applied to u-boot-dm, thanks!

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

On 7/17/22 10:12, Simon Glass wrote:
> Hi Marek,

Hi,

[...]

> Applied to u-boot-dm, thanks!

This patch is still not in u-boot/master ?

Applied to u-boot-dm, thanks!
Applied to u-boot-dm, thanks!

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Marek Vasut]

On 9/21/22 11:50, Simon Glass wrote:
> On 7/17/22 10:12, Simon Glass wrote:
>> Hi Marek,
> 
> Hi,
> 
> [...]
> 
>> Applied to u-boot-dm, thanks!
> 
> This patch is still not in u-boot/master ?
> 
> Applied to u-boot-dm, thanks!
> Applied to u-boot-dm, thanks!

Can you point me to this patch in the u-boot-dm repository ?
I am getting these emails, but I don't see the patch applied.

Re: [PATCH] cmd: fdt: Add support for reading stringlist property values

[Simon Glass]

Hi Marek,

On Fri, 23 Sept 2022 at 09:17, Marek Vasut <marex@denx.de> wrote:
>
> On 9/21/22 11:50, Simon Glass wrote:
> > On 7/17/22 10:12, Simon Glass wrote:
> >> Hi Marek,
> >
> > Hi,
> >
> > [...]
> >
> >> Applied to u-boot-dm, thanks!
> >
> > This patch is still not in u-boot/master ?
> >
> > Applied to u-boot-dm, thanks!
> > Applied to u-boot-dm, thanks!
>
> Can you point me to this patch in the u-boot-dm repository ?
> I am getting these emails, but I don't see the patch applied.

Oh it is gone now as I pushed something new to dm/master.

But see this one in master:

13982ced2cc cmd: fdt: Add support for reading stringlist property values

Regards,
Simon