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

Heinrich Schuchardt
2025-08-29T07:33:57Z

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.
> + */
> +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;

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)