[RFC 03/10] eeprom: starfive: Simplify get_ddr_size_from_eeprom()

[Hal Feng]

> On 03.09.25 05:28, E Shattow wrote:
> On 8/29/25 00:33, Heinrich Schuchardt wrote:
> > On 29.08.25 08:09, Hal Feng wrote:
> >> Directly return the DDR size instead of the field of 'DxxxExxx'.
> >> Move the function description to the header file.
> >>
> >> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM
> >> configuration")
> >> Signed-off-by: Hal Feng <hal.feng at starfivetech.com>
> >> ---
> >>   arch/riscv/cpu/jh7110/spl.c                     |  2 +-
> >>   arch/riscv/include/asm/arch-jh7110/eeprom.h     |  8 +++++++-
> >>   .../visionfive2/visionfive2-i2c-eeprom.c        | 17
> >> +++--------------
> >>   3 files changed, 11 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/arch/riscv/cpu/jh7110/spl.c
> >> b/arch/riscv/cpu/jh7110/spl.c index 87aaf865246..3aece7d995b 100644
> >> --- a/arch/riscv/cpu/jh7110/spl.c
> >> +++ b/arch/riscv/cpu/jh7110/spl.c
> >> @@ -41,7 +41,7 @@ int spl_dram_init(void)
> >>       /* Read the definition of the DDR size from eeprom, and if not,
> >>        * use the definition in DT
> >>        */
> >> -    size = (get_ddr_size_from_eeprom() >> 16) & 0xFF;
> >> +    size = get_ddr_size_from_eeprom();
> >>       if (check_ddr_size(size))
> >>           gd->ram_size = size << 30;
> >>   diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h b/arch/
> >> riscv/include/asm/arch-jh7110/eeprom.h
> >> index 45ad2a5f7bc..6d0a0ba0c4a 100644
> >> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >> @@ -10,7 +10,13 @@
> >>   #include <linux/types.h>
> >>     u8 get_pcb_revision_from_eeprom(void);
> >> -u32 get_ddr_size_from_eeprom(void);
> >> +
> >> +/**
> >> + * get_ddr_size_from_eeprom() - read DDR size from EEPROM
> >> + *
> >> + * @return: size in GiB or 0xFF on error.
> 
> No. This is not consistent with get_mmc_size_from_eeprom() return value
> which uses 0 as short-circuit return value if read_eeprom() fails.
> 
> It is not important to make the distinction here if eeprom read failed or the
> data was not a valid size (i.e. zero from successful eeprom read).

OK. Will return 0 if read_eeprom() fails.

> 
> >> + */
> >> +u8 get_ddr_size_from_eeprom(void);
> >>     /**
> >>    * get_mmc_size_from_eeprom() - read eMMC size from EEPROM diff
> >> --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/
> >> board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >> index 17a44020bcf..3866d07f9d4 100644
> >> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >> @@ -550,23 +550,12 @@ u8 get_pcb_revision_from_eeprom(void)
> >>       return pbuf.eeprom.atom1.data.pstr[6];
> >>   }
> >>   -/**
> >> - * get_ddr_size_from_eeprom - get the DDR size
> >> - * pstr:  VF7110A1-2228-D008E000-00000001
> >> - * VF7110A1/VF7110B1 : VisionFive JH7110A /VisionFive JH7110B
> >> - * D008: 8GB LPDDR4
> >> - * E000: No emmc device, ECxx: include emmc device, xx: Capacity
> >> size[GB]
> >> - * return: the field of 'D008E000'
> >> - */
> >> -
> >> -u32 get_ddr_size_from_eeprom(void)
> >> +u8 get_ddr_size_from_eeprom(void)
> >>   {
> >> -    u32 pv = 0xFFFFFFFF;
> >> -
> >>       if (read_eeprom())
> >> -        return pv;
> >> +        return 0xFF;
> 
> No, inverted logic, see above return of 0xFF as an error condition is suspect.

Ditto.

> 
> >
> > Let's assume somebody cleared the EEPROM and the SPI flash.
> >
> > Would it make sense to assume the minimum encodable RAM size (1 GiB)
> > in this case, just to let the board boot to the console to allow
> > writing the EEPROM with valid data again?
> >
> > The current patch to simplify the code looks correct.
> >
> > Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >
> >>   -    return hextoul(&pbuf.eeprom.atom1.data.pstr[14], NULL);
> >> +    return (hextoul(&pbuf.eeprom.atom1.data.pstr[14], NULL) >> 16) &
> >> 0xFF;
> >>   }
> >>     u32 get_mmc_size_from_eeprom(void)
> >
> 
> Yes, later fallback to 2G size configuration makes sense. Compatible products
> with less than 2G do not exist and probably never will; there may be some
> developer boards with 1G but none I have ever heard of directly in
> conversations since 2023. The other common situation with zero or
> uninitialized EEPROM would be a failed read of eeprom i.e. RTC module at
> same i2c address or missing devicetree nodes at SPL phase.
> 
> 1. DDR size detection heuristic should happen first without need of eeprom, if
> possible.
> 
> 2. Else warn DDR detection failed and read size from eeprom config
> 
> 3. Else warn eeprom memory config invalid and set size set to default 2G.

Actually, if the u-boot fails to read DDR size from EEPROM, it will use the size (4GB)
defined in the memory node of device tree. 

With some debugging, I found that the reason why the SPL fails to boot when the
EEPROM crashing is that the u-boot FDT selection depends on the product ID read
from EEPROM.  There is no default FDT for VF2 u-boot to use now.

One solution is that
------------------------------------------------------------------------------------------------
diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
index b518560bb94..b1bacaf7996 100644
--- a/arch/riscv/dts/binman.dtsi
+++ b/arch/riscv/dts/binman.dtsi
@@ -92,10 +92,7 @@
                        };
 
                        configurations {
-
-#ifndef CONFIG_MULTI_DTB_FIT
                                default = "conf-1";
-#endif
 
 #if !defined(CONFIG_OF_BOARD) || defined(CONFIG_MULTI_DTB_FIT)
                                @conf-SEQ {
------------------------------------------------------------------------------------------------
which means using the first FDT (jh7110-deepcomputing-fml13v01) in OF_LIST as the
default FDT.

Best regards,
Hal