[PATCH v3 0/4] riscv: dts: starfive: simplify binman config

[PATCH v3 0/4] riscv: dts: starfive: simplify binman config

[Heinrich Schuchardt]

All StarFive JH7110 are supported via starfive_visionfive2_defconfig.
Up to now we have explicitly specified the multi-device-tree FIT
configuration. This more complicated than needed.

Binman can use the information from CONFIG_OF_LIST to automatically
generated a FIT image containing the necessary device-trees and
configurations.

This also removes the non-functional default configuration without a
device-tree that binman added up to now.

Before the patch set booting on an unsupported device would fail in main
U-Boot due to the lack of a device-tree in the default configuration.

With this patch set booting will fail in SPL because there is no
default configuration.

v3:
	Use CONFIG_MULTI_DTB_FIT instead of CONFIG_OF_LIST in the binman
	include for identifying the use case.
	Remove default configuration for CONFIG_MULTI_DTB_FIT in the
	RISC-V binman include instead of a change on board level.
v2:
	do not use a default configuration

Heinrich Schuchardt (4):
  riscv: dts: add OF_LIST handling to binman.dtsi
  riscv: dts: no default configuration for MULTI_DTB_FIT
  board: starfive: spl: strip off 'starfive/' prefix
  riscv: dts: starfive: remove duplicate itb entries

 arch/riscv/dts/binman.dtsi                    | 10 ++-
 .../dts/starfive-visionfive2-binman.dtsi      | 76 -------------------
 board/starfive/visionfive2/spl.c              |  4 +
 3 files changed, 11 insertions(+), 79 deletions(-)

-- 
2.47.1

[PATCH v3 1/4] riscv: dts: add OF_LIST handling to binman.dtsi

[Heinrich Schuchardt]

Binman can automatically generate device-tree and configuration entries in
the FIT image based on CONFIG_MULTI_DTB_FIT if the binman node includes the
right sub-nodes.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	Use CONFIG_MULTI_DTB_FIT instead of CONFIG_OF_LIST to avoid
	build failure for QEMU defconfigs.
v2:
	no change
---
 arch/riscv/dts/binman.dtsi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
index 0405faca574..782ef037f7a 100644
--- a/arch/riscv/dts/binman.dtsi
+++ b/arch/riscv/dts/binman.dtsi
@@ -82,8 +82,9 @@
 					};
 				};
 
-#ifndef CONFIG_OF_BOARD
+#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
 				@fdt-SEQ {
+					fit,operation = "gen-fdt-nodes";
 					description = "NAME";
 					type = "flat_dt";
 					compression = "none";
@@ -94,7 +95,7 @@
 			configurations {
 				default = "conf-1";
 
-#ifndef CONFIG_OF_BOARD
+#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
 				@conf-SEQ {
 #else
 				conf-1 {
@@ -115,7 +116,7 @@
 #endif
 #endif /* CONFIG_OPTEE */
 
-#ifndef CONFIG_OF_BOARD
+#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
 					fdt = "fdt-SEQ";
 #endif
 				};
-- 
2.47.1

Re: [PATCH v3 1/4] riscv: dts: add OF_LIST handling to binman.dtsi

[E Shattow]

On 2/21/25 01:58, Heinrich Schuchardt wrote:
> Binman can automatically generate device-tree and configuration entries in
> the FIT image based on CONFIG_MULTI_DTB_FIT if the binman node includes the
> right sub-nodes.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	Use CONFIG_MULTI_DTB_FIT instead of CONFIG_OF_LIST to avoid
> 	build failure for QEMU defconfigs.
> v2:
> 	no change
> ---
>  arch/riscv/dts/binman.dtsi | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
> index 0405faca574..782ef037f7a 100644
> --- a/arch/riscv/dts/binman.dtsi
> +++ b/arch/riscv/dts/binman.dtsi
> @@ -82,8 +82,9 @@
>  					};
>  				};
>  
> -#ifndef CONFIG_OF_BOARD
> +#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
>  				@fdt-SEQ {
> +					fit,operation = "gen-fdt-nodes";
>  					description = "NAME";
>  					type = "flat_dt";
>  					compression = "none";
> @@ -94,7 +95,7 @@
>  			configurations {
>  				default = "conf-1";
>  
> -#ifndef CONFIG_OF_BOARD
> +#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
>  				@conf-SEQ {
>  #else
>  				conf-1 {
> @@ -115,7 +116,7 @@
>  #endif
>  #endif /* CONFIG_OPTEE */
>  
> -#ifndef CONFIG_OF_BOARD
> +#if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
>  					fdt = "fdt-SEQ";
>  #endif
>  				};

Reviewed-by: E Shattow <e@freeshell.de>

[PATCH v3 2/4] riscv: dts: no default configuration for MULTI_DTB_FIT

[Heinrich Schuchardt]

JH7110 boards are currently the only use case for multi DTB FIT images
on RISC-V.

Booting JH7110 systems with a VisionFive 2 device-tree used to kind of
work without causing harm to the hardware. But there is no guarantee
that this will hold true in future. So we should not rely on it.

Before the current patch series booting failed on unsupported boards due
to the lack of a device-tree in the binman generated default configuration
when reaching main U-Boot.

By not setting a default configuration booting will now fail on
unsupported boards already in SPL. This allows SPL to
continue with the next boot source for a possible recovery.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	new patch
---
 arch/riscv/dts/binman.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
index 782ef037f7a..ceb916b74a7 100644
--- a/arch/riscv/dts/binman.dtsi
+++ b/arch/riscv/dts/binman.dtsi
@@ -93,7 +93,10 @@
 			};
 
 			configurations {
+
+#ifndef CONFIG_MULTI_DTB_FIT
 				default = "conf-1";
+#endif
 
 #if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
 				@conf-SEQ {
-- 
2.47.1

Re: [PATCH v3 2/4] riscv: dts: no default configuration for MULTI_DTB_FIT

[E Shattow]

On 2/21/25 01:58, Heinrich Schuchardt wrote:
> JH7110 boards are currently the only use case for multi DTB FIT images
> on RISC-V.
> 
> Booting JH7110 systems with a VisionFive 2 device-tree used to kind of
> work without causing harm to the hardware. But there is no guarantee
> that this will hold true in future. So we should not rely on it.
> 
> Before the current patch series booting failed on unsupported boards due
> to the lack of a device-tree in the binman generated default configuration
> when reaching main U-Boot.
> 
> By not setting a default configuration booting will now fail on
> unsupported boards already in SPL. This allows SPL to
> continue with the next boot source for a possible recovery.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	new patch
> ---
>  arch/riscv/dts/binman.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
> index 782ef037f7a..ceb916b74a7 100644
> --- a/arch/riscv/dts/binman.dtsi
> +++ b/arch/riscv/dts/binman.dtsi
> @@ -93,7 +93,10 @@
>  			};
>  
>  			configurations {
> +
> +#ifndef CONFIG_MULTI_DTB_FIT
>  				default = "conf-1";
> +#endif
>  
>  #if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
>  				@conf-SEQ {

Reviewed-by: E Shattow <e@freeshell.de>

[PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[Heinrich Schuchardt]

The configuration descriptions generated by binman contain the vendor
device-tree directory. Instead of adding it to all match strings just strip
it off.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
---
v3:
	no change
v2:
	no change
---
 board/starfive/visionfive2/spl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
index 22afd76c6b9..d63eb1abe6a 100644
--- a/board/starfive/visionfive2/spl.c
+++ b/board/starfive/visionfive2/spl.c
@@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
 
 	product_id = get_product_id_from_eeprom();
 
+	/* Strip off prefix */
+	if (strncmp(name, "starfive/", 9))
+		return -EINVAL;
+	name += 9;
 	if (!strncmp(product_id, "VF7110", 6)) {
 		version = get_pcb_revision_from_eeprom();
 		if ((version == 'b' || version == 'B') &&
-- 
2.47.1

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[E Shattow]

On 2/21/25 01:58, Heinrich Schuchardt wrote:
> The configuration descriptions generated by binman contain the vendor
> device-tree directory. Instead of adding it to all match strings just strip
> it off.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
> ---
> v3:
> 	no change
> v2:
> 	no change
> ---
>  board/starfive/visionfive2/spl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
> index 22afd76c6b9..d63eb1abe6a 100644
> --- a/board/starfive/visionfive2/spl.c
> +++ b/board/starfive/visionfive2/spl.c
> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>  
>  	product_id = get_product_id_from_eeprom();
>  
> +	/* Strip off prefix */
> +	if (strncmp(name, "starfive/", 9))
> +		return -EINVAL;
> +	name += 9;
>  	if (!strncmp(product_id, "VF7110", 6)) {
>  		version = get_pcb_revision_from_eeprom();
>  		if ((version == 'b' || version == 'B') &&

Let's insist on logic statements in board_fit_config_name_match()
callback that begin with literal items (no pointer math trickery) from
configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that order:

#if CONFIG_IS_ENABLED(LOAD_FIT)
int board_fit_config_name_match(const char *name)
{
	if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
	   !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
		return 0;
	} else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
	   !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
		return 0;
	} else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a")) &&
	   !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
		switch (get_pcb_revision_from_eeprom()) {
		case 'a':
		case 'A':
			return 0;
		}
	} else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2b")) &&
	   !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
		switch (get_pcb_revision_from_eeprom()) {
		case 'b':
		case 'B':
			return 0;
		}
	}

	return -EINVAL;
}
#endif

Not sure about code style so that is simply an example of keeping the
sort order the same as how it exists in
configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST

Mars CM (and CM Lite) logic may be dropped since those targets do not
exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
however, I do anticipate to submit for review into Linux upstream soon
and to begin that process. A donation board was sent to me so I now have
Mars CM to test as well as Mars CM Lite.

Are the duplicate string definitions and logic in
board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
still appropriate, could those now be factored out? I think Simon's
suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace that
functionality? -E

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[Heinrich Schuchardt]

On 2/23/25 02:33, E Shattow wrote:
> 
> 
> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>> The configuration descriptions generated by binman contain the vendor
>> device-tree directory. Instead of adding it to all match strings just strip
>> it off.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>> ---
>> v3:
>> 	no change
>> v2:
>> 	no change
>> ---
>>   board/starfive/visionfive2/spl.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
>> index 22afd76c6b9..d63eb1abe6a 100644
>> --- a/board/starfive/visionfive2/spl.c
>> +++ b/board/starfive/visionfive2/spl.c
>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>   
>>   	product_id = get_product_id_from_eeprom();
>>   
>> +	/* Strip off prefix */
>> +	if (strncmp(name, "starfive/", 9))
>> +		return -EINVAL;
>> +	name += 9;
>>   	if (!strncmp(product_id, "VF7110", 6)) {
>>   		version = get_pcb_revision_from_eeprom();
>>   		if ((version == 'b' || version == 'B') &&
> 
> Let's insist on logic statements in board_fit_config_name_match()
> callback that begin with literal items (no pointer math trickery) from
> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that order:

Thank you for reviewing.

Unfortunately your sentence starting with "Let's insist" does not 
provide insight into your reasoning.

Why do you want to add the 'starfive/' to each of the strings we compare 
instead of checking the common prefix first and the remainder next which 
results in a smaller binary?

> 
> #if CONFIG_IS_ENABLED(LOAD_FIT)
> int board_fit_config_name_match(const char *name)
> {
> 	if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
> 	   !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
> 		return 0;
> 	} else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
> 	   !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> 		return 0;
> 	} else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a")) &&
> 	   !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> 		switch (get_pcb_revision_from_eeprom()) {
> 		case 'a':
> 		case 'A':
> 			return 0;
> 		}
> 	} else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2b")) &&
> 	   !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> 		switch (get_pcb_revision_from_eeprom()) {
> 		case 'b':
> 		case 'B':
> 			return 0;
> 		}
> 	}
> 
> 	return -EINVAL;
> }
> #endif
> 
> Not sure about code style so that is simply an example of keeping the
> sort order the same as how it exists in
> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
> 
> Mars CM (and CM Lite) logic may be dropped since those targets do not
> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
> however, I do anticipate to submit for review into Linux upstream soon
> and to begin that process. A donation board was sent to me so I now have
> Mars CM to test as well as Mars CM Lite.

As you are planning to upstream the boards I suggest to keep those 
lines. You could already use them by manually copying the device-trees 
into the upstream dtb folder and adding the files to CONFIG_OF_LIST.

> 
> Are the duplicate string definitions and logic in
> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
> still appropriate, could those now be factored out? I think Simon's
> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace that
> functionality? -E

$fdtfile is used for loading a device-tree from the ESP. I can't see how 
CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is 
related. We cannot set $fdtfile from SPL.

Best regards

Heinrich

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[E Shattow]

On 2/23/25 04:55, Heinrich Schuchardt wrote:
> On 2/23/25 02:33, E Shattow wrote:
>>
>>
>> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>>> The configuration descriptions generated by binman contain the vendor
>>> device-tree directory. Instead of adding it to all match strings just
>>> strip
>>> it off.
>>>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>>> ---
>>> v3:
>>>     no change
>>> v2:
>>>     no change
>>> ---
>>>   board/starfive/visionfive2/spl.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/
>>> visionfive2/spl.c
>>> index 22afd76c6b9..d63eb1abe6a 100644
>>> --- a/board/starfive/visionfive2/spl.c
>>> +++ b/board/starfive/visionfive2/spl.c
>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>>         product_id = get_product_id_from_eeprom();
>>>   +    /* Strip off prefix */
>>> +    if (strncmp(name, "starfive/", 9))
>>> +        return -EINVAL;
>>> +    name += 9;
>>>       if (!strncmp(product_id, "VF7110", 6)) {
>>>           version = get_pcb_revision_from_eeprom();
>>>           if ((version == 'b' || version == 'B') &&
>>
>> Let's insist on logic statements in board_fit_config_name_match()
>> callback that begin with literal items (no pointer math trickery) from
>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that order:
> 
> Thank you for reviewing.
> 
> Unfortunately your sentence starting with "Let's insist" does not
> provide insight into your reasoning.
> 

On first sight I do not like "code golf" of pointer math on the function
parameter in-place. It seems to be okay here but does get my attention
to look closer.

On closer look what I do want to see is 1:1 continuity between what is
in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall
form of this logic block so that it is uncomplicated to add more
variants to this board target. We assume there is a pattern "starfive/"
here but there is no such thing, this is wrong to do that. The
originating list in the Makefile may contain literals that do not have a
"starfive/" prefix.

> Why do you want to add the 'starfive/' to each of the strings we compare
> instead of checking the common prefix first and the remainder next which
> results in a smaller binary?

What are the limits on binary size, here? How important is the need to
create this assumption of "starfive/" prefix?

> 
>>
>> #if CONFIG_IS_ENABLED(LOAD_FIT)
>> int board_fit_config_name_match(const char *name)
>> {
>>     if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
>>        !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
>>         return 0;
>>     } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
>>        !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>         return 0;
>>     } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>> v1.2a")) &&
>>        !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>         switch (get_pcb_revision_from_eeprom()) {
>>         case 'a':
>>         case 'A':
>>             return 0;
>>         }
>>     } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>> v1.2b")) &&
>>        !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>         switch (get_pcb_revision_from_eeprom()) {
>>         case 'b':
>>         case 'B':
>>             return 0;
>>         }
>>     }
>>
>>     return -EINVAL;
>> }
>> #endif
>>
>> Not sure about code style so that is simply an example of keeping the
>> sort order the same as how it exists in
>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>
>> Mars CM (and CM Lite) logic may be dropped since those targets do not
>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
>> however, I do anticipate to submit for review into Linux upstream soon
>> and to begin that process. A donation board was sent to me so I now have
>> Mars CM to test as well as Mars CM Lite.
> 
> As you are planning to upstream the boards I suggest to keep those
> lines. You could already use them by manually copying the device-trees
> into the upstream dtb folder and adding the files to CONFIG_OF_LIST.

Yes, this dead code can be some removed with other cleanup, it does not
have to be this series.

> 
>>
>> Are the duplicate string definitions and logic in
>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
>> still appropriate, could those now be factored out? I think Simon's
>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace that
>> functionality? -E
> 
> $fdtfile is used for loading a device-tree from the ESP. I can't see how
> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is
> related. We cannot set $fdtfile from SPL.
> 
> Best regards
> 
> Heinrich

I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested:

--- a/configs/starfive_visionfive2_defconfig
+++ b/configs/starfive_visionfive2_defconfig
@@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y
 # CONFIG_OF_BOARD_FIXUP is not set
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_FIT=y
+CONFIG_FIT_BEST_MATCH=y
 CONFIG_BOOTSTD_DEFAULTS=y
 CONFIG_BOOTSTAGE=y
 CONFIG_QSPI_BOOT=y

also delete function
board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and
where it is called from within board_late_init(). Then $fdtfile is set
not defined so the best fit is selected, or, if $fdtfile is user-defined
then the user-defined path is loaded (same as before). The difference is
there is not any $fdtfile env variable defined for the typical situation
but it does apparently have some kind of heuristic and loads something
appropriate, I am not sure if it is from U-Boot or if it is from the
search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow
we can drop a lot of unnecessary duplicate logic and code this way. That
change could as part of this series or as a follow-up.

-E

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[Heinrich Schuchardt]

On 2/23/25 15:18, E Shattow wrote:
> 
> On 2/23/25 04:55, Heinrich Schuchardt wrote:
>> On 2/23/25 02:33, E Shattow wrote:
>>>
>>>
>>> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>>>> The configuration descriptions generated by binman contain the vendor
>>>> device-tree directory. Instead of adding it to all match strings just
>>>> strip
>>>> it off.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>>>> ---
>>>> v3:
>>>>      no change
>>>> v2:
>>>>      no change
>>>> ---
>>>>    board/starfive/visionfive2/spl.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/
>>>> visionfive2/spl.c
>>>> index 22afd76c6b9..d63eb1abe6a 100644
>>>> --- a/board/starfive/visionfive2/spl.c
>>>> +++ b/board/starfive/visionfive2/spl.c
>>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>>>          product_id = get_product_id_from_eeprom();
>>>>    +    /* Strip off prefix */
>>>> +    if (strncmp(name, "starfive/", 9))
>>>> +        return -EINVAL;
>>>> +    name += 9;
>>>>        if (!strncmp(product_id, "VF7110", 6)) {
>>>>            version = get_pcb_revision_from_eeprom();
>>>>            if ((version == 'b' || version == 'B') &&
>>>
>>> Let's insist on logic statements in board_fit_config_name_match()
>>> callback that begin with literal items (no pointer math trickery) from
>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that order:
>>
>> Thank you for reviewing.
>>
>> Unfortunately your sentence starting with "Let's insist" does not
>> provide insight into your reasoning.
>>
> 
> On first sight I do not like "code golf" of pointer math on the function
> parameter in-place. It seems to be okay here but does get my attention
> to look closer.
> 
> On closer look what I do want to see is 1:1 continuity between what is
> in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall
> form of this logic block so that it is uncomplicated to add more
> variants to this board target. We assume there is a pattern "starfive/"
> here but there is no such thing, this is wrong to do that. The
> originating list in the Makefile may contain literals that do not have a
> "starfive/" prefix.
> 
>> Why do you want to add the 'starfive/' to each of the strings we compare
>> instead of checking the common prefix first and the remainder next which
>> results in a smaller binary?
> 
> What are the limits on binary size, here? How important is the need to
> create this assumption of "starfive/" prefix?
> 
>>
>>>
>>> #if CONFIG_IS_ENABLED(LOAD_FIT)
>>> int board_fit_config_name_match(const char *name)
>>> {
>>>      if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
>>>         !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
>>>          return 0;
>>>      } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>          return 0;
>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>> v1.2a")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>          switch (get_pcb_revision_from_eeprom()) {
>>>          case 'a':
>>>          case 'A':
>>>              return 0;
>>>          }
>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>> v1.2b")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>          switch (get_pcb_revision_from_eeprom()) {
>>>          case 'b':
>>>          case 'B':
>>>              return 0;
>>>          }
>>>      }
>>>
>>>      return -EINVAL;
>>> }
>>> #endif
>>>
>>> Not sure about code style so that is simply an example of keeping the
>>> sort order the same as how it exists in
>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>
>>> Mars CM (and CM Lite) logic may be dropped since those targets do not
>>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>> however, I do anticipate to submit for review into Linux upstream soon
>>> and to begin that process. A donation board was sent to me so I now have
>>> Mars CM to test as well as Mars CM Lite.
>>
>> As you are planning to upstream the boards I suggest to keep those
>> lines. You could already use them by manually copying the device-trees
>> into the upstream dtb folder and adding the files to CONFIG_OF_LIST.
> 
> Yes, this dead code can be some removed with other cleanup, it does not
> have to be this series.
> 
>>
>>>
>>> Are the duplicate string definitions and logic in
>>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
>>> still appropriate, could those now be factored out? I think Simon's
>>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace that
>>> functionality? -E
>>
>> $fdtfile is used for loading a device-tree from the ESP. I can't see how
>> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is
>> related. We cannot set $fdtfile from SPL.
>>
>> Best regards
>>
>> Heinrich
> 
> I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested:
> 
> --- a/configs/starfive_visionfive2_defconfig
> +++ b/configs/starfive_visionfive2_defconfig
> @@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y
>   # CONFIG_OF_BOARD_FIXUP is not set
>   # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>   CONFIG_FIT=y
> +CONFIG_FIT_BEST_MATCH=y
>   CONFIG_BOOTSTD_DEFAULTS=y
>   CONFIG_BOOTSTAGE=y
>   CONFIG_QSPI_BOOT=y
> 
> also delete function
> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and
> where it is called from within board_late_init(). Then $fdtfile is set
> not defined so the best fit is selected, or, if $fdtfile is user-defined
> then the user-defined path is loaded (same as before). The difference is
> there is not any $fdtfile env variable defined for the typical situation
> but it does apparently have some kind of heuristic and loads something

 From what you write above I take that variable $fdtfile has not been set.

Have you successfully booted using the EFI boot manager and verified 
that it is picking up the correct device-tree from the ESP and not the 
one from u-boot.itb?

I wouldn't know how that should work as in the EFI boot manager we rely 
on environment variable $fdtfile:

lib/efi_loader/efi_fdt.c:55:
     fdt_fname = env_get("fdtfile");

Best regards

Heinrich

> appropriate, I am not sure if it is from U-Boot or if it is from the
> search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow
> we can drop a lot of unnecessary duplicate logic and code this way. That
> change could as part of this series or as a follow-up.
> 

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[E Shattow]

On 2/23/25 07:39, Heinrich Schuchardt wrote:
> On 2/23/25 15:18, E Shattow wrote:
>>
>> On 2/23/25 04:55, Heinrich Schuchardt wrote:
>>> On 2/23/25 02:33, E Shattow wrote:
>>>>
>>>>
>>>> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>>>>> The configuration descriptions generated by binman contain the vendor
>>>>> device-tree directory. Instead of adding it to all match strings just
>>>>> strip
>>>>> it off.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>>>>> ---
>>>>> v3:
>>>>>      no change
>>>>> v2:
>>>>>      no change
>>>>> ---
>>>>>    board/starfive/visionfive2/spl.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/
>>>>> visionfive2/spl.c
>>>>> index 22afd76c6b9..d63eb1abe6a 100644
>>>>> --- a/board/starfive/visionfive2/spl.c
>>>>> +++ b/board/starfive/visionfive2/spl.c
>>>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>>>>          product_id = get_product_id_from_eeprom();
>>>>>    +    /* Strip off prefix */
>>>>> +    if (strncmp(name, "starfive/", 9))
>>>>> +        return -EINVAL;
>>>>> +    name += 9;
>>>>>        if (!strncmp(product_id, "VF7110", 6)) {
>>>>>            version = get_pcb_revision_from_eeprom();
>>>>>            if ((version == 'b' || version == 'B') &&
>>>>
>>>> Let's insist on logic statements in board_fit_config_name_match()
>>>> callback that begin with literal items (no pointer math trickery) from
>>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that
>>>> order:
>>>
>>> Thank you for reviewing.
>>>
>>> Unfortunately your sentence starting with "Let's insist" does not
>>> provide insight into your reasoning.
>>>
>>
>> On first sight I do not like "code golf" of pointer math on the function
>> parameter in-place. It seems to be okay here but does get my attention
>> to look closer.
>>
>> On closer look what I do want to see is 1:1 continuity between what is
>> in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall
>> form of this logic block so that it is uncomplicated to add more
>> variants to this board target. We assume there is a pattern "starfive/"
>> here but there is no such thing, this is wrong to do that. The
>> originating list in the Makefile may contain literals that do not have a
>> "starfive/" prefix.
>>
>>> Why do you want to add the 'starfive/' to each of the strings we compare
>>> instead of checking the common prefix first and the remainder next which
>>> results in a smaller binary?
>>
>> What are the limits on binary size, here? How important is the need to
>> create this assumption of "starfive/" prefix?
>>
>>>
>>>>
>>>> #if CONFIG_IS_ENABLED(LOAD_FIT)
>>>> int board_fit_config_name_match(const char *name)
>>>> {
>>>>      if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
>>>>         !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
>>>>          return 0;
>>>>      } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>>          return 0;
>>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>>> v1.2a")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>>          switch (get_pcb_revision_from_eeprom()) {
>>>>          case 'a':
>>>>          case 'A':
>>>>              return 0;
>>>>          }
>>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>>> v1.2b")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>>          switch (get_pcb_revision_from_eeprom()) {
>>>>          case 'b':
>>>>          case 'B':
>>>>              return 0;
>>>>          }
>>>>      }
>>>>
>>>>      return -EINVAL;
>>>> }
>>>> #endif
>>>>
>>>> Not sure about code style so that is simply an example of keeping the
>>>> sort order the same as how it exists in
>>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>>
>>>> Mars CM (and CM Lite) logic may be dropped since those targets do not
>>>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>> however, I do anticipate to submit for review into Linux upstream soon
>>>> and to begin that process. A donation board was sent to me so I now
>>>> have
>>>> Mars CM to test as well as Mars CM Lite.
>>>
>>> As you are planning to upstream the boards I suggest to keep those
>>> lines. You could already use them by manually copying the device-trees
>>> into the upstream dtb folder and adding the files to CONFIG_OF_LIST.
>>
>> Yes, this dead code can be some removed with other cleanup, it does not
>> have to be this series.
>>
>>>
>>>>
>>>> Are the duplicate string definitions and logic in
>>>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
>>>> still appropriate, could those now be factored out? I think Simon's
>>>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace
>>>> that
>>>> functionality? -E
>>>
>>> $fdtfile is used for loading a device-tree from the ESP. I can't see how
>>> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is
>>> related. We cannot set $fdtfile from SPL.
>>>
>>> Best regards
>>>
>>> Heinrich
>>
>> I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested:
>>
>> --- a/configs/starfive_visionfive2_defconfig
>> +++ b/configs/starfive_visionfive2_defconfig
>> @@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y
>>   # CONFIG_OF_BOARD_FIXUP is not set
>>   # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>>   CONFIG_FIT=y
>> +CONFIG_FIT_BEST_MATCH=y
>>   CONFIG_BOOTSTD_DEFAULTS=y
>>   CONFIG_BOOTSTAGE=y
>>   CONFIG_QSPI_BOOT=y
>>
>> also delete function
>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and
>> where it is called from within board_late_init(). Then $fdtfile is set
>> not defined so the best fit is selected, or, if $fdtfile is user-defined
>> then the user-defined path is loaded (same as before). The difference is
>> there is not any $fdtfile env variable defined for the typical situation
>> but it does apparently have some kind of heuristic and loads something
> 
> From what you write above I take that variable $fdtfile has not been set.
> 
> Have you successfully booted using the EFI boot manager and verified
> that it is picking up the correct device-tree from the ESP and not the
> one from u-boot.itb?
> 
> I wouldn't know how that should work as in the EFI boot manager we rely
> on environment variable $fdtfile:
> 
> lib/efi_loader/efi_fdt.c:55:
>     fdt_fname = env_get("fdtfile");
> 
> Best regards
> 
> Heinrich
> 
>> appropriate, I am not sure if it is from U-Boot or if it is from the
>> search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow
>> we can drop a lot of unnecessary duplicate logic and code this way. That
>> change could as part of this series or as a follow-up.
>>
> 

This works the same way, you set fdtfile to load device-tree from the
ESP. If fdtfile is not set we are already have a valid fdt for Linux in
u-boot.itb so it should not be any problem. Do I understand this
correctly? -E

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[Heinrich Schuchardt]

On 2/23/25 15:18, E Shattow wrote:
> 
> On 2/23/25 04:55, Heinrich Schuchardt wrote:
>> On 2/23/25 02:33, E Shattow wrote:
>>>
>>>
>>> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>>>> The configuration descriptions generated by binman contain the vendor
>>>> device-tree directory. Instead of adding it to all match strings just
>>>> strip
>>>> it off.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>>>> ---
>>>> v3:
>>>>      no change
>>>> v2:
>>>>      no change
>>>> ---
>>>>    board/starfive/visionfive2/spl.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/
>>>> visionfive2/spl.c
>>>> index 22afd76c6b9..d63eb1abe6a 100644
>>>> --- a/board/starfive/visionfive2/spl.c
>>>> +++ b/board/starfive/visionfive2/spl.c
>>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>>>          product_id = get_product_id_from_eeprom();
>>>>    +    /* Strip off prefix */
>>>> +    if (strncmp(name, "starfive/", 9))
>>>> +        return -EINVAL;
>>>> +    name += 9;
>>>>        if (!strncmp(product_id, "VF7110", 6)) {
>>>>            version = get_pcb_revision_from_eeprom();
>>>>            if ((version == 'b' || version == 'B') &&
>>>
>>> Let's insist on logic statements in board_fit_config_name_match()
>>> callback that begin with literal items (no pointer math trickery) from
>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that order:
>>
>> Thank you for reviewing.
>>
>> Unfortunately your sentence starting with "Let's insist" does not
>> provide insight into your reasoning.
>>
> 
> On first sight I do not like "code golf" of pointer math on the function
> parameter in-place. It seems to be okay here but does get my attention
> to look closer.
> 
> On closer look what I do want to see is 1:1 continuity between what is
> in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall
> form of this logic block so that it is uncomplicated to add more
> variants to this board target. We assume there is a pattern "starfive/"
> here but there is no such thing, this is wrong to do that. The
> originating list in the Makefile may contain literals that do not have a
> "starfive/" prefix.

The starfive/ prefix is given by the vendor name of the SoC. Linux will 
not accept device-trees for JH7110 based boards into other folders. This 
is why we will continue to have 'starfive/' in CONFIG_OF_LIST for all 
future JH7110 boards.

Binman copies the dtb name into the description field of the configurations.

> 
>> Why do you want to add the 'starfive/' to each of the strings we compare
>> instead of checking the common prefix first and the remainder next which
>> results in a smaller binary?
> 
> What are the limits on binary size, here? How important is the need to
> create this assumption of "starfive/" prefix?

The SPL size is limited by the cache size as SPL contains the DDR init 
code. See symbol CONFIG_SPL_STARFIVE_DDR.

Best regards

Heinrich

> 
>>
>>>
>>> #if CONFIG_IS_ENABLED(LOAD_FIT)
>>> int board_fit_config_name_match(const char *name)
>>> {
>>>      if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
>>>         !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
>>>          return 0;
>>>      } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>          return 0;
>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>> v1.2a")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>          switch (get_pcb_revision_from_eeprom()) {
>>>          case 'a':
>>>          case 'A':
>>>              return 0;
>>>          }
>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>> v1.2b")) &&
>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>          switch (get_pcb_revision_from_eeprom()) {
>>>          case 'b':
>>>          case 'B':
>>>              return 0;
>>>          }
>>>      }
>>>
>>>      return -EINVAL;
>>> }
>>> #endif
>>>
>>> Not sure about code style so that is simply an example of keeping the
>>> sort order the same as how it exists in
>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>
>>> Mars CM (and CM Lite) logic may be dropped since those targets do not
>>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>> however, I do anticipate to submit for review into Linux upstream soon
>>> and to begin that process. A donation board was sent to me so I now have
>>> Mars CM to test as well as Mars CM Lite.
>>
>> As you are planning to upstream the boards I suggest to keep those
>> lines. You could already use them by manually copying the device-trees
>> into the upstream dtb folder and adding the files to CONFIG_OF_LIST.
> 
> Yes, this dead code can be some removed with other cleanup, it does not
> have to be this series.
> 
>>
>>>
>>> Are the duplicate string definitions and logic in
>>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
>>> still appropriate, could those now be factored out? I think Simon's
>>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace that
>>> functionality? -E
>>
>> $fdtfile is used for loading a device-tree from the ESP. I can't see how
>> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is
>> related. We cannot set $fdtfile from SPL.
>>
>> Best regards
>>
>> Heinrich
> 
> I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested:
> 
> --- a/configs/starfive_visionfive2_defconfig
> +++ b/configs/starfive_visionfive2_defconfig
> @@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y
>   # CONFIG_OF_BOARD_FIXUP is not set
>   # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>   CONFIG_FIT=y
> +CONFIG_FIT_BEST_MATCH=y
>   CONFIG_BOOTSTD_DEFAULTS=y
>   CONFIG_BOOTSTAGE=y
>   CONFIG_QSPI_BOOT=y
> 
> also delete function
> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and
> where it is called from within board_late_init(). Then $fdtfile is set
> not defined so the best fit is selected, or, if $fdtfile is user-defined
> then the user-defined path is loaded (same as before). The difference is
> there is not any $fdtfile env variable defined for the typical situation
> but it does apparently have some kind of heuristic and loads something
> appropriate, I am not sure if it is from U-Boot or if it is from the
> search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow
> we can drop a lot of unnecessary duplicate logic and code this way. That
> change could as part of this series or as a follow-up.
> 
> -E

Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix

[E Shattow]

On 2/24/25 02:13, Heinrich Schuchardt wrote:
> On 2/23/25 15:18, E Shattow wrote:
>>
>> On 2/23/25 04:55, Heinrich Schuchardt wrote:
>>> On 2/23/25 02:33, E Shattow wrote:
>>>>
>>>>
>>>> On 2/21/25 01:58, Heinrich Schuchardt wrote:
>>>>> The configuration descriptions generated by binman contain the vendor
>>>>> device-tree directory. Instead of adding it to all match strings just
>>>>> strip
>>>>> it off.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>> Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
>>>>> ---
>>>>> v3:
>>>>>      no change
>>>>> v2:
>>>>>      no change
>>>>> ---
>>>>>    board/starfive/visionfive2/spl.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/
>>>>> visionfive2/spl.c
>>>>> index 22afd76c6b9..d63eb1abe6a 100644
>>>>> --- a/board/starfive/visionfive2/spl.c
>>>>> +++ b/board/starfive/visionfive2/spl.c
>>>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name)
>>>>>          product_id = get_product_id_from_eeprom();
>>>>>    +    /* Strip off prefix */
>>>>> +    if (strncmp(name, "starfive/", 9))
>>>>> +        return -EINVAL;
>>>>> +    name += 9;
>>>>>        if (!strncmp(product_id, "VF7110", 6)) {
>>>>>            version = get_pcb_revision_from_eeprom();
>>>>>            if ((version == 'b' || version == 'B') &&
>>>>
>>>> Let's insist on logic statements in board_fit_config_name_match()
>>>> callback that begin with literal items (no pointer math trickery) from
>>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that
>>>> order:
>>>
>>> Thank you for reviewing.
>>>
>>> Unfortunately your sentence starting with "Let's insist" does not
>>> provide insight into your reasoning.
>>>
>>
>> On first sight I do not like "code golf" of pointer math on the function
>> parameter in-place. It seems to be okay here but does get my attention
>> to look closer.
>>
>> On closer look what I do want to see is 1:1 continuity between what is
>> in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall
>> form of this logic block so that it is uncomplicated to add more
>> variants to this board target. We assume there is a pattern "starfive/"
>> here but there is no such thing, this is wrong to do that. The
>> originating list in the Makefile may contain literals that do not have a
>> "starfive/" prefix.
> 
> The starfive/ prefix is given by the vendor name of the SoC. Linux will
> not accept device-trees for JH7110 based boards into other folders. This
> is why we will continue to have 'starfive/' in CONFIG_OF_LIST for all
> future JH7110 boards.
> 
> Binman copies the dtb name into the description field of the
> configurations.
> 

Look around in the source tree, what I see is that U-Boot i.e. may have
device-trees for JH7110 based boards in other folders, or no folder
prefix at all. We don't have that now, but we could (even though we just
went to great effort to follow more exactly the Linux kernel upstream).
I have for example tested by adding dozens more (fake) compatible boards
and the binary size is still comfortably within whatever it needs to be
to execute correctly. If some assumed prefix should be dropped this is
not the place to be doing this, it will need to be a change to the
programming interface to do this before it gets to this callback or else
the programming interface is poor (or we are doing something even
worse-off inventing an anti-pattern). I could not find any example of
prefix enforcement for this callback on other board targets so in the
goal of "saving a few bytes" we're inventing something new, which is
also weird because it is really just not needed when instead we can type
out "starfive/" a few more times which makes code grep and review and
git and everything so much better for a few dozen bytes of binary
size.... so be it.

If you do not feel like what I am saying has merit then I do not want to
get in the way of your improvements either, now, overall I think it's
fine enough. I would submit a follow-up patch for my concept of how this
should be.

>>
>>> Why do you want to add the 'starfive/' to each of the strings we compare
>>> instead of checking the common prefix first and the remainder next which
>>> results in a smaller binary?
>>
>> What are the limits on binary size, here? How important is the need to
>> create this assumption of "starfive/" prefix?
> 
> The SPL size is limited by the cache size as SPL contains the DDR init
> code. See symbol CONFIG_SPL_STARFIVE_DDR.
> 
> Best regards
> 
> Heinrich
> 

I have tested by adding dozens more board variant targets with
unreasonably long device-tree names, and so this compiles and runs fine
on Star64 hardware. Whatever the limit here it does not seem to be any
actual concern if we are trimming "starfive" bytes or not for existing
and expected new variants. The code golf maneuver creates an artificial
disconnect between the literals in the Makefile and what exists in code;
that may make sense to career programmers but I look at it and it is
more complex not simple. You have a thing in the Makefile, so check
against that same thing and not some obfuscation of it. If you would
adjust to drop the pointer math and use the full literals from Makefile
(with "starfive/"*) this logic section will still be inside-out until it
follows the form of the data that originates from the Makefile. It's not
just about binary size, but knowing that binary size is why you're
sticking more pointer math into code after we had this same kind of
tactic being involved with a bug in similar code, when it's actually not
necessary here now... this makes me even more suspicious when I see this.

>>
>>>
>>>>
>>>> #if CONFIG_IS_ENABLED(LOAD_FIT)
>>>> int board_fit_config_name_match(const char *name)
>>>> {
>>>>      if(!strcmp(name, "starfive/jh7110-milkv-mars") &&
>>>>         !strncmp(get_product_id_from_eeprom(), "MARS", 4)) {
>>>>          return 0;
>>>>      } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>>          return 0;
>>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>>> v1.2a")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>>          switch (get_pcb_revision_from_eeprom()) {
>>>>          case 'a':
>>>>          case 'A':
>>>>              return 0;
>>>>          }
>>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2-
>>>> v1.2b")) &&
>>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>>          switch (get_pcb_revision_from_eeprom()) {
>>>>          case 'b':
>>>>          case 'B':
>>>>              return 0;
>>>>          }
>>>>      }
>>>>
>>>>      return -EINVAL;
>>>> }
>>>> #endif
>>>>
>>>> Not sure about code style so that is simply an example of keeping the
>>>> sort order the same as how it exists in
>>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>>
>>>> Mars CM (and CM Lite) logic may be dropped since those targets do not
>>>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST
>>>> however, I do anticipate to submit for review into Linux upstream soon
>>>> and to begin that process. A donation board was sent to me so I now
>>>> have
>>>> Mars CM to test as well as Mars CM Lite.
>>>
>>> As you are planning to upstream the boards I suggest to keep those
>>> lines. You could already use them by manually copying the device-trees
>>> into the upstream dtb folder and adding the files to CONFIG_OF_LIST.
>>
>> Yes, this dead code can be some removed with other cleanup, it does not
>> have to be this series.
>>
>>>
>>>>
>>>> Are the duplicate string definitions and logic in
>>>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc.
>>>> still appropriate, could those now be factored out? I think Simon's
>>>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace
>>>> that
>>>> functionality? -E
>>>
>>> $fdtfile is used for loading a device-tree from the ESP. I can't see how
>>> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is
>>> related. We cannot set $fdtfile from SPL.
>>>
>>> Best regards
>>>
>>> Heinrich
>>
>> I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested:
>>
>> --- a/configs/starfive_visionfive2_defconfig
>> +++ b/configs/starfive_visionfive2_defconfig
>> @@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y
>>   # CONFIG_OF_BOARD_FIXUP is not set
>>   # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>>   CONFIG_FIT=y
>> +CONFIG_FIT_BEST_MATCH=y
>>   CONFIG_BOOTSTD_DEFAULTS=y
>>   CONFIG_BOOTSTAGE=y
>>   CONFIG_QSPI_BOOT=y
>>
>> also delete function
>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and
>> where it is called from within board_late_init(). Then $fdtfile is set
>> not defined so the best fit is selected, or, if $fdtfile is user-defined
>> then the user-defined path is loaded (same as before). The difference is
>> there is not any $fdtfile env variable defined for the typical situation
>> but it does apparently have some kind of heuristic and loads something
>> appropriate, I am not sure if it is from U-Boot or if it is from the
>> search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow
>> we can drop a lot of unnecessary duplicate logic and code this way. That
>> change could as part of this series or as a follow-up.
>>
>> -E
> 

I'm not as sure about the fdtfile stuff so I hope someone (Simon?) can
address that. Vendored kernels and SD card OS images and I'm sure many
other situations I do not bother to test for so when I say it works it
is just the simple case of loading official Debian installer and install
and run the OS from NVMe / MMC / SD Card media. It seems to work fine
though without all the $fdtfile heuristic to set the variable, I set
instead $fdtfile directly in U-Boot or from userland tools in Linux the
U-Boot environment variable, or chainload GRUB for its devicetree
directive, or U-Boot EFI Fdt directive, ... does not seem to be like
this is a problem worth hanging onto a bunch of redundant code for when
we've got OF_UPSTREAM done for this board / multi-target. -E

[PATCH v3 4/4] riscv: dts: starfive: remove duplicate itb entries

[Heinrich Schuchardt]

As binman already creates nodes based on CONFIG_OF_LIST we don't need to
add extra nodes.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org> # StarFIve VisionFive 2
---
v3:
	no change
v2:
	no change
---
 .../dts/starfive-visionfive2-binman.dtsi      | 76 -------------------
 1 file changed, 76 deletions(-)

diff --git a/arch/riscv/dts/starfive-visionfive2-binman.dtsi b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
index 4cce001e80d..05787bdb92d 100644
--- a/arch/riscv/dts/starfive-visionfive2-binman.dtsi
+++ b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
@@ -13,82 +13,6 @@
 };
 
 &binman {
-	itb {
-		fit {
-			images {
-				fdt-jh7110-milkv-mars {
-					description = "jh7110-milkv-mars";
-					load = <0x40400000>;
-					compression = "none";
-
-					blob-ext {
-						filename = "dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dtb";
-					};
-				};
-
-				fdt-jh7110-pine64-star64 {
-					description = "jh7110-pine64-star64";
-					load = <0x40400000>;
-					compression = "none";
-
-					blob-ext {
-						filename = "dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dtb";
-					};
-				};
-
-				fdt-jh7110-starfive-visionfive-2-v1.2a {
-					description = "jh7110-starfive-visionfive-2-v1.2a";
-					load = <0x40400000>;
-					compression = "none";
-
-					blob-ext {
-						filename = "dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
-					};
-				};
-
-				fdt-jh7110-starfive-visionfive-2-v1.3b {
-					description = "jh7110-starfive-visionfive-2-v1.3b";
-					load = <0x40400000>;
-					compression = "none";
-
-					blob-ext {
-						filename = "dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
-					};
-				};
-			};
-
-			configurations {
-				conf-jh7110-milkv-mars {
-					description = "jh7110-milkv-mars";
-					firmware = "opensbi";
-					loadables = "uboot";
-					fdt = "fdt-jh7110-milkv-mars";
-				};
-
-				conf-jh7110-pine64-star64 {
-					description = "jh7110-pine64-star64";
-					firmware = "opensbi";
-					loadables = "uboot";
-					fdt = "fdt-jh7110-pine64-star64";
-				};
-
-				conf-jh7110-starfive-visionfive-2-v1.2a {
-					description = "jh7110-starfive-visionfive-2-v1.2a";
-					firmware = "opensbi";
-					loadables = "uboot";
-					fdt = "fdt-jh7110-starfive-visionfive-2-v1.2a";
-				};
-
-				conf-jh7110-starfive-visionfive-2-v1.3b {
-					description = "jh7110-starfive-visionfive-2-v1.3b";
-					firmware = "opensbi";
-					loadables = "uboot";
-					fdt = "fdt-jh7110-starfive-visionfive-2-v1.3b";
-				};
-			};
-		};
-	};
-
 	spl-img {
 		filename = "spl/u-boot-spl.bin.normal.out";
 
-- 
2.47.1

Re: [PATCH v3 4/4] riscv: dts: starfive: remove duplicate itb entries

[E Shattow]

On 2/21/25 01:58, Heinrich Schuchardt wrote:
> As binman already creates nodes based on CONFIG_OF_LIST we don't need to
> add extra nodes.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested-by: Simon Glass <sjg@chromium.org> # StarFIve VisionFive 2
> ---
> v3:
> 	no change
> v2:
> 	no change
> ---
>  .../dts/starfive-visionfive2-binman.dtsi      | 76 -------------------
>  1 file changed, 76 deletions(-)
> 
> diff --git a/arch/riscv/dts/starfive-visionfive2-binman.dtsi b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> index 4cce001e80d..05787bdb92d 100644
> --- a/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> +++ b/arch/riscv/dts/starfive-visionfive2-binman.dtsi
> @@ -13,82 +13,6 @@
>  };
>  
>  &binman {
> -	itb {
> -		fit {
> -			images {
> -				fdt-jh7110-milkv-mars {
> -					description = "jh7110-milkv-mars";
> -					load = <0x40400000>;
> -					compression = "none";
> -
> -					blob-ext {
> -						filename = "dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dtb";
> -					};
> -				};
> -
> -				fdt-jh7110-pine64-star64 {
> -					description = "jh7110-pine64-star64";
> -					load = <0x40400000>;
> -					compression = "none";
> -
> -					blob-ext {
> -						filename = "dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dtb";
> -					};
> -				};
> -
> -				fdt-jh7110-starfive-visionfive-2-v1.2a {
> -					description = "jh7110-starfive-visionfive-2-v1.2a";
> -					load = <0x40400000>;
> -					compression = "none";
> -
> -					blob-ext {
> -						filename = "dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> -					};
> -				};
> -
> -				fdt-jh7110-starfive-visionfive-2-v1.3b {
> -					description = "jh7110-starfive-visionfive-2-v1.3b";
> -					load = <0x40400000>;
> -					compression = "none";
> -
> -					blob-ext {
> -						filename = "dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> -					};
> -				};
> -			};
> -
> -			configurations {
> -				conf-jh7110-milkv-mars {
> -					description = "jh7110-milkv-mars";
> -					firmware = "opensbi";
> -					loadables = "uboot";
> -					fdt = "fdt-jh7110-milkv-mars";
> -				};
> -
> -				conf-jh7110-pine64-star64 {
> -					description = "jh7110-pine64-star64";
> -					firmware = "opensbi";
> -					loadables = "uboot";
> -					fdt = "fdt-jh7110-pine64-star64";
> -				};
> -
> -				conf-jh7110-starfive-visionfive-2-v1.2a {
> -					description = "jh7110-starfive-visionfive-2-v1.2a";
> -					firmware = "opensbi";
> -					loadables = "uboot";
> -					fdt = "fdt-jh7110-starfive-visionfive-2-v1.2a";
> -				};
> -
> -				conf-jh7110-starfive-visionfive-2-v1.3b {
> -					description = "jh7110-starfive-visionfive-2-v1.3b";
> -					firmware = "opensbi";
> -					loadables = "uboot";
> -					fdt = "fdt-jh7110-starfive-visionfive-2-v1.3b";
> -				};
> -			};
> -		};
> -	};
> -
>  	spl-img {
>  		filename = "spl/u-boot-spl.bin.normal.out";
>  

Reviewed-by: E Shattow <e@freeshell.de>