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

[Heinrich Schuchardt]

On 9/2/25 23: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).
> 
>>> + */
>>> +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.
> 
>>
>> 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

I wrote 1 GiB because this is the smallest size that can be encoded in 
the current EEPROM scheme. 1 GiB is big enough for reaching the U-Boot 
console and reprogramming the EEPROM or even booting into Linux.

> 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.

We have get_ram_size(). According to a comment 
arch/arm/mach-stm32mp/stm32mp1/spl.c that function has a problem with 
speculative access.

I don't see an urgent necessity to move away from EEPROM as the only 
method to determine the RAM size.

Best regards

Heinrich

> 
> 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.
> 
> -E