[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 at canonical.com>
>> Reviewed-by: Leo Yu-Chi Liang <ycliang at 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