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

[E Shattow <e@freeshell.de>]

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