> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]>
> >>>
> >>>> + 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.
Best regards,
Hal
