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

[E Shattow <e@freeshell.de>]

On 10/21/25 02:20, Hal Feng wrote:
>> 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@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.
> 

Also note that Heinrich mentions this could become signed int for
returning a proper error code. I do not have any more opinion about
"zero" or "negative error code", either is okay. This can become signed
int in future if we will care to make a distinction between the types of
errors.

>>
>>>> + */
>>>> +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@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

No, it is missing cpufreq driver and there is wrong behavior of the
memory driver, so this problem would repeat again when JH7110S is added.

Do you have a suggestion of OPP table values that are safe for both
JH7110 and JH7110S ? Can you implement cpufreq in U-Boot as mentioned
previously (on LKML discussion)?

Also, "2133" is not a frequency in Hz and anyway the memory driver in
U-Boot needs to be improved to use Common Clock Framework to get the
correct frequency.

Look for the two "FIXME" comments in
arch/riscv/dts/starfive-visionfive2-u-boot.dtsi from U-Boot
origin/master for location of what I am asking about.

When these issue are fixed then it will be easy to have a basic "jh711x"
dts configuration as the default until DTS selection is completed. This
will allow use of EEPROM update command from U-Boot.

Thank you, Hal.

-E