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.schucha...@canonical.com>
>>>>> Reviewed-by: Leo Yu-Chi Liang <ycli...@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
