Re: [PATCH 1/4] board: starfive: function to read eMMC size

On 4/16/24 06:09, E Shattow wrote:

On Mon, Apr 15, 2024 at 4:50 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

The EEPROM provides information about the size of the EEPROM.

"The EEPROM provides information about the size of the eMMC."

Thanks for catching this.

...

Provide a new function get_mmc_size_from_eeprom() to read it.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

arch/riscv/include/asm/arch-jh7110/eeprom.h | 7 +++++++ board/starfive/visionfive2/Kconfig | 9 +++++++++ .../visionfive2/visionfive2-i2c-eeprom.c | 18 ++++++++++++++++++ 3 files changed, 34 insertions(+)

diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h b/arch/riscv/include/asm/arch-jh7110/eeprom.h index 62d184aeb57..17395d4269e 100644 --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h @@ -12,6 +12,13 @@ u8 get_pcb_revision_from_eeprom(void); u32 get_ddr_size_from_eeprom(void);

+/**

• • get_mmc_size_from_eeprom() - read MMC size form EEPROM
• • @return: size in GiB or 0 on error.
• */

+u32 get_mmc_size_from_eeprom(void);

• /**
  • get_product_id_from_eeprom - get product ID string

diff --git a/board/starfive/visionfive2/Kconfig b/board/starfive/visionfive2/Kconfig index 2186a939646..d7e8a7a7d78 100644 --- a/board/starfive/visionfive2/Kconfig +++ b/board/starfive/visionfive2/Kconfig @@ -50,4 +50,13 @@ config BOARD_SPECIFIC_OPTIONS # dummy imply PHY_LIB imply PHY_MSCC

+config STARFIVE_NO_EMMC

•   bool "Report eMMC size as zero"
  

•   help
  

•     The serial number string in the EEPROM is meant to report the
  

•     size of onboard eMMC. Unfortunately some Milk-V Mars CM Lite
  

•     modules without eMMC show a non-zero size here.
  

•     Set to 'Y' if you have a Mars CM Lite module.
  

• endif

diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c index ddef7d61235..cd3d8bd51a6 100644 --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c @@ -548,6 +548,24 @@ u32 get_ddr_size_from_eeprom(void) return hextoul(&pbuf.eeprom.atom1.data.pstr[14], NULL); }

+u32 get_mmc_size_from_eeprom(void) +{

•   u32 size;
  

•   if (IS_ENABLED(CONFIG_STARFIVE_NO_EMMC))
  

•           return 0;
  

•   if (read_eeprom())
  

•           return 0;
  

•   size = dectoul(&pbuf.eeprom.atom1.data.pstr[19], NULL);
  

•   if (pbuf.eeprom.atom1.data.pstr[21] == 'T')
  

•           size <<= 10;
  

•   return size;
  

+}

• U_BOOT_LONGHELP(mac, "\n" " - display EEPROM content\n"

-- 2.43.0

Fixed-position parsing on a data format of ordered variable length hyphen-delimited fields. Notable is that some Pine64 Star64 and Milk-V Mars CM Lite boards shipped with uninitialized or wrong EEPROM data; further the EEPROM Write Protect can be trivially disabled and arbitrary data written i.e. with a paperclip then `mac` command. Could this code be generalized to split fields on hyphen character better expressing the expected data format or is that unwanted complexity and code size?

The boards that I have seen up to now are consistent in the string layout of the boards that I have received up to now:

VisionFive 2 1.2B VF7110A1-2239-D004E000-xxxxxxxx VisionFive 2 1.3B VF7110B1-2253-D004E000-xxxxxxxx VisionFive 2 1.3B VF7110B1-2253-D008E000-xxxxxxxx Pine64 Pinetab V VF7110B1-2230-D008E000-xxxxxxxx Pine64 Star64 v1 STAR64V1-2310-D008E000-xxxxxxxx Milk-V Mars MARS-V11-2340-D008E000-xxxxxxxx Milk-V Mars CM Lite MARC-V10-2340-D002E016-xxxxxxxx

The example of the Pinetab V and of the first batch of the Milk V CM Lite shows that the trustworthiness of the EEPROM data is limited.

As you mentioned such deviations can be fixed by rewriting the EEPROM. I don't think that this is problematic for the parsing of the string. It might be an issue for warranty claims.

Retrieving the RAM size already relies on positional data. The eMMC size is just a sub-field of what is read in get_ddr_size_from_eeprom().

Changes would have to start there if wanted by the StarFive maintainer.

Best regards

Heinrich