Re: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[E Shattow <e@freeshell.de>]

On 9/2/25 00:44, Hal Feng wrote:
>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
>> On 29.08.25 08:09, Hal Feng wrote:
>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
>>> Move the function description to the header file.
>>> Remove the function calls in board/starfive/visionfive2/.
>>>
>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>> ---
>>>   arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
>>>   board/starfive/visionfive2/spl.c              | 18 ++++++-----------
>>>   .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
>>>   .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
>>>   4 files changed, 19 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> index 6d0a0ba0c4a..025f1d32c49 100644
>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> @@ -9,6 +9,11 @@
>>>
>>>   #include <linux/types.h>
>>>
>>> +/**
>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
>>> + *
>>> + * @return: the PCB revision or 0xFF on error.
>>> + */

No. Inverted logic. See below comment about body of function.

>>>   u8 get_pcb_revision_from_eeprom(void);
>>>
>>>   /**
>>> diff --git a/board/starfive/visionfive2/spl.c
>> b/board/starfive/visionfive2/spl.c
>>> index 3dfa931b655..901e7b58f36 100644
>>> --- a/board/starfive/visionfive2/spl.c
>>> +++ b/board/starfive/visionfive2/spl.c
>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
>> *name)
>>>   		    !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;
>>> -		}
>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
>> ||
>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
>> {
>>> +		return 0;
>>>   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b")
>> &&
>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>> -		switch (get_pcb_revision_from_eeprom()) {
>>> -		case 'b':
>>> -		case 'B':
>>> -			return 0;
>>> -		}
>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
>> ||
>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
>> {
>>> +		return 0;
>>>   	}
>>>
>>>   	return -EINVAL;

I agree it is good to delete the case statement and drop lowercase
compare as suggested, also...

>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
>> b/board/starfive/visionfive2/starfive_visionfive2.c
>>> index bfbb11a2ee7..f38433e94ac 100644
>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
>>>   		fdtfile = "starfive/jh7110-milkv-mars.dtb";
>>>   	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>   		fdtfile = "starfive/jh7110-pine64-star64.dtb";
>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>> -		switch (get_pcb_revision_from_eeprom()) {
>>> -		case 'a':
>>> -		case 'A':
>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>> v1.2a.dtb";
>>> -			break;
>>> -		case 'b':
>>> -		case 'B':
>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>> v1.3b.dtb";
>>> -			break;
>>> -		default:
>>> -			log_err("Unknown revision\n");
>>> -			return;
>>> -		}
>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
>>
>> Where boards both with 'a' and 'b' ever shipped?
>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
> 
> You're right. There are no boards marked "VF7110a" or "VF7110b". Let's remove
> the comparison of "VF7110a" and "VF7110b".
> 
> Best regards,
> Hal

... again here too delete the case statement and drop lowercase compare.
Thanks.

> 
>>
>> The change looks technically correct.
>>
>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>
>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
>>>   	} else {
>>>   		log_err("Unknown product\n");
>>>   		return;

>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> index 3866d07f9d4..43b8af4fc59 100644
>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
>>>   	return 0;
>>>   }
>>>
>>> -/**
>>> - * get_pcb_revision_from_eeprom - get the PCB revision
>>> - *
>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
>>> - */
>>>   u8 get_pcb_revision_from_eeprom(void)
>>>   {
>>> -	u8 pv = 0xFF;
>>> -
>>>   	if (read_eeprom())
>>> -		return pv;
>>> +		return 0xFF;
>>>
>>> -	return pbuf.eeprom.atom1.data.pstr[6];
>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
>>>   }
>>>
>>>   u8 get_ddr_size_from_eeprom(void)
> 

No. I do not want to have to guess what error does "0xFF" have the
meaning of, it makes the code annoying to read every function header
documentation to learn this information.

Since we remove all users of mac_read_from_eeprom() it is certainly not
a problem we care about now anymore. Default PCB revision in case of
error may be zero. It is not important to make distinction of additional
failure detection, this is already resolved by read_eeprom() where any
error message may be presented to the user.

I appreciate the effort to be more technically correct and have a unique
error value but with u8 return value 0xFF it is an additional layer of
complication not needed. If you insist on this approach then the return
value should be arbitrarily defined as a compiler macro, say
CMD_EEPROM_FAIL (255) and consistently used. In fact I would not
complain if it improved code readability.

-E