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

Heinrich Schuchardt heinrich.schuchardt at canonical.com
Wed Sep 3 00:32:17 CEST 2025


On 9/3/25 00:12, E Shattow wrote:
> 
> 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 at 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 at 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.

There is no necessity to use u8 as the return value. We could also use 
int and return a negative error code (-EIO) on failure.

Best regards

Heinrich

> 
> -E



More information about the U-Boot mailing list