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

[E Shattow <e@freeshell.de>]

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