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

2024-04-16 Thread Heinrich Schuchardt 

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

On Mon, Apr 15, 2024 at 4:50 AM Heinrich Schuchardt
 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 
---
  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([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([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-
VisionFive 2 1.3B   VF7110B1-2253-D004E000-
VisionFive 2 1.3B   VF7110B1-2253-D008E000-
Pine64 Pinetab VVF7110B1-2230-D008E000-
Pine64 Star64 v1STAR64V1-2310-D008E000-
Milk-V Mars MARS-V11-2340-D008E000-
Milk-V Mars CM Lite MARC-V10-2340-D002E016-

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

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

2024-04-15 Thread E Shattow 
On Mon, Apr 15, 2024 at 4:50 AM Heinrich Schuchardt
 wrote:
>
> The EEPROM provides information about the size of the EEPROM.

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

> Provide a new function get_mmc_size_from_eeprom() to read it.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
>  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([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([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?

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

2024-04-15 Thread Heinrich Schuchardt 
The EEPROM provides information about the size of the EEPROM.
Provide a new function get_mmc_size_from_eeprom() to read it.

Signed-off-by: Heinrich Schuchardt 
---
 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([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([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