[RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()
Hal Feng
hal.feng at starfivetech.com
Wed Oct 22 10:44:19 CEST 2025
> On 22.10.25 11:03, Hal Feng wrote:
> > On 03.09.25 06:32, Heinrich Schuchardt wrote:
> > 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.
>
> Yeah, will return a negative error code (-EIO) on failure.
> I think the reason why we set the return value to 0xFF on failure was that 0xFF
> is the default value after EEPROM is erased.
Fix the last line of comments. 0xFF may be the default value after DDR is inited,
but I am not sure about it.
Best regards,
Hal
More information about the U-Boot
mailing list