[RFC 00/10] Add support for StarFive VisionFive 2 Lite board

[RFC 00/10] Add support for StarFive VisionFive 2 Lite board

[Hal Feng]

VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S industrial
SoC which can run at -40~85 degrees centigrade and up to 1.25GHz.

Board features:
- JH7110S SoC
- 2/4/8 GiB LPDDR4 DRAM
- AXP15060 PMIC
- 40 pin GPIO header
- 1x USB 3.0 host port
- 3x USB 2.0 host port
- 1x M.2 M-Key (size: 2242)
- 1x MicroSD slot (optional non-removable eMMC)
- 1x QSPI Flash
- 1x I2C EEPROM
- 1x 1Gbps Ethernet port
- SDIO-based Wi-Fi & UART-based Bluetooth
- 1x HDMI port
- 1x 2-lane DSI
- 1x 2-lane CSI

For more details, please see [1].

Note: Patch 1 and 2 are the kernel device tree picked from [2]. If [2] is
merged, they will be no more needed. Please ignore these two patches.

[1] https://www.kickstarter.com/projects/starfive/visionfive-2-lite-unlock-risc-v-sbc-at-199
[2] https://lore.kernel.org/all/20250821100930.71404-1-hal.feng@starfivetech.com/

Hal Feng (10):
  riscv: dts: starfive: jh7110-common: Move out some nodes to the board
    dts
  riscv: dts: starfive: Add VisionFive 2 Lite board device tree
  eeprom: starfive: Simplify get_ddr_size_from_eeprom()
  eeprom: starfive: Correct get_pcb_revision_from_eeprom()
  eeprom: starfive: Update eeprom data format version to 3
  pcie: starfive: Add a optional power gpio support
  riscv: dts: jh7110: Add StarFive VisionFive 2 Lite u-boot device tree
  configs: visionfive2: Add VisionFive 2 Lite DT to OF_LIST
  board: starfive: spl: Support VisionFive 2 Lite
  board: starfive: visionfive2: Add VisionFive 2 Lite fdt selection

 arch/riscv/cpu/jh7110/spl.c                   |   2 +-
 ...10s-starfive-visionfive-2-lite-u-boot.dtsi |   7 +
 arch/riscv/include/asm/arch-jh7110/eeprom.h   |  13 +-
 board/starfive/visionfive2/spl.c              |  21 ++-
 .../visionfive2/starfive_visionfive2.c        |  22 +--
 .../visionfive2/visionfive2-i2c-eeprom.c      | 119 +++++++++-----
 configs/starfive_visionfive2_defconfig        |   2 +-
 drivers/pci/pcie_starfive_jh7110.c            |   8 +
 .../src/riscv/starfive/jh7110-common.dtsi     |  22 ---
 .../jh7110-deepcomputing-fml13v01.dts         |  49 ++++++
 .../src/riscv/starfive/jh7110-milkv-mars.dts  |  49 ++++++
 .../riscv/starfive/jh7110-pine64-star64.dts   |  49 ++++++
 .../jh7110-starfive-visionfive-2.dtsi         |  46 ++++++
 dts/upstream/src/riscv/starfive/jh7110.dtsi   |  16 --
 .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
 15 files changed, 474 insertions(+), 103 deletions(-)
 create mode 100644 arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
 create mode 100644 dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts


base-commit: 3dc5e9a0108bb114175b6362f9cb22367402f624
-- 
2.43.2

[RFC 01/10] riscv: dts: starfive: jh7110-common: Move out some nodes to the board dts

[Hal Feng]

Some node in this file are not used by the upcoming VisionFive 2 Lite
board. Move them to the board dts to prepare for adding the new
VisionFive 2 Lite device tree.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../src/riscv/starfive/jh7110-common.dtsi     | 22 ---------
 .../jh7110-deepcomputing-fml13v01.dts         | 49 +++++++++++++++++++
 .../src/riscv/starfive/jh7110-milkv-mars.dts  | 49 +++++++++++++++++++
 .../riscv/starfive/jh7110-pine64-star64.dts   | 49 +++++++++++++++++++
 .../jh7110-starfive-visionfive-2.dtsi         | 46 +++++++++++++++++
 dts/upstream/src/riscv/starfive/jh7110.dtsi   | 16 ------
 6 files changed, 193 insertions(+), 38 deletions(-)

diff --git a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi b/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
index 4baeb981d4d..9d3d03ad2ed 100644
--- a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
+++ b/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
@@ -272,15 +272,9 @@
 	assigned-clock-rates = <50000000>;
 	bus-width = <8>;
 	bootph-pre-ram;
-	cap-mmc-highspeed;
-	mmc-ddr-1_8v;
-	mmc-hs200-1_8v;
-	cap-mmc-hw-reset;
 	post-power-on-delay-ms = <200>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins>;
-	vmmc-supply = <&vcc_3v3>;
-	vqmmc-supply = <&emmc_vdd>;
 	status = "okay";
 };
 
@@ -290,12 +284,7 @@
 	assigned-clock-rates = <50000000>;
 	bus-width = <4>;
 	bootph-pre-ram;
-	no-sdio;
-	no-mmc;
-	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
-	disable-wp;
 	cap-sd-highspeed;
-	post-power-on-delay-ms = <200>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc1_pins>;
 	status = "okay";
@@ -439,17 +428,6 @@
 	};
 
 	mmc0_pins: mmc0-0 {
-		 rst-pins {
-			pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
-					      GPOEN_ENABLE,
-					      GPI_NONE)>;
-			bias-pull-up;
-			drive-strength = <12>;
-			input-disable;
-			input-schmitt-disable;
-			slew-rate = <0>;
-		};
-
 		mmc-pins {
 			pinmux = <PINMUX(PAD_SD0_CLK, 0)>,
 				 <PINMUX(PAD_SD0_CMD, 0)>,
diff --git a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
index f2857d021d6..5a2a41a7e8c 100644
--- a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
+++ b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
@@ -11,6 +11,55 @@
 	compatible = "deepcomputing,fml13v01", "starfive,jh7110";
 };
 
+&cpu_opp {
+	opp-375000000 {
+		opp-hz = /bits/ 64 <375000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-500000000 {
+		opp-hz = /bits/ 64 <500000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-750000000 {
+		opp-hz = /bits/ 64 <750000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-1500000000 {
+		opp-hz = /bits/ 64 <1500000000>;
+		opp-microvolt = <1040000>;
+	};
+};
+
+&mmc0 {
+	cap-mmc-highspeed;
+	cap-mmc-hw-reset;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	vmmc-supply = <&vcc_3v3>;
+	vqmmc-supply = <&emmc_vdd>;
+};
+
+&mmc0_pins {
+	rst-pins {
+		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
+				      GPOEN_ENABLE,
+				      GPI_NONE)>;
+		bias-pull-up;
+		drive-strength = <12>;
+		input-disable;
+		input-schmitt-disable;
+		slew-rate = <0>;
+	};
+};
+
+&mmc1 {
+	no-sdio;
+	no-mmc;
+	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	post-power-on-delay-ms = <200>;
+};
+
 &pcie1 {
 	perst-gpios = <&sysgpio 21 GPIO_ACTIVE_LOW>;
 	phys = <&pciephy1>;
diff --git a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts b/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
index 3bd62ab7852..0c90facc4ee 100644
--- a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
+++ b/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
@@ -11,6 +11,25 @@
 	compatible = "milkv,mars", "starfive,jh7110";
 };
 
+&cpu_opp {
+	opp-375000000 {
+		opp-hz = /bits/ 64 <375000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-500000000 {
+		opp-hz = /bits/ 64 <500000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-750000000 {
+		opp-hz = /bits/ 64 <750000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-1500000000 {
+		opp-hz = /bits/ 64 <1500000000>;
+		opp-microvolt = <1040000>;
+	};
+};
+
 &gmac0 {
 	starfive,tx-use-rgmii-clk;
 	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
@@ -22,6 +41,36 @@
 	status = "okay";
 };
 
+&mmc0 {
+	cap-mmc-highspeed;
+	cap-mmc-hw-reset;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	vmmc-supply = <&vcc_3v3>;
+	vqmmc-supply = <&emmc_vdd>;
+};
+
+&mmc0_pins {
+	rst-pins {
+		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
+				      GPOEN_ENABLE,
+				      GPI_NONE)>;
+		bias-pull-up;
+		drive-strength = <12>;
+		input-disable;
+		input-schmitt-disable;
+		slew-rate = <0>;
+	};
+};
+
+&mmc1 {
+	no-sdio;
+	no-mmc;
+	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	post-power-on-delay-ms = <200>;
+};
+
 &pcie0 {
 	status = "okay";
 };
diff --git a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
index 31e825be206..c9677aef9ff 100644
--- a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
+++ b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
@@ -14,6 +14,25 @@
 	};
 };
 
+&cpu_opp {
+	opp-375000000 {
+		opp-hz = /bits/ 64 <375000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-500000000 {
+		opp-hz = /bits/ 64 <500000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-750000000 {
+		opp-hz = /bits/ 64 <750000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-1500000000 {
+		opp-hz = /bits/ 64 <1500000000>;
+		opp-microvolt = <1040000>;
+	};
+};
+
 &gmac0 {
 	starfive,tx-use-rgmii-clk;
 	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
@@ -44,6 +63,36 @@
 	status = "okay";
 };
 
+&mmc0 {
+	cap-mmc-highspeed;
+	cap-mmc-hw-reset;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	vmmc-supply = <&vcc_3v3>;
+	vqmmc-supply = <&emmc_vdd>;
+};
+
+&mmc0_pins {
+	rst-pins {
+		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
+				      GPOEN_ENABLE,
+				      GPI_NONE)>;
+		bias-pull-up;
+		drive-strength = <12>;
+		input-disable;
+		input-schmitt-disable;
+		slew-rate = <0>;
+	};
+};
+
+&mmc1 {
+	no-sdio;
+	no-mmc;
+	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	post-power-on-delay-ms = <200>;
+};
+
 &pcie1 {
 	status = "okay";
 };
diff --git a/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi b/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
index 5f14afb2c24..d1e4206f125 100644
--- a/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -13,6 +13,25 @@
 	};
 };
 
+&cpu_opp {
+	opp-375000000 {
+		opp-hz = /bits/ 64 <375000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-500000000 {
+		opp-hz = /bits/ 64 <500000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-750000000 {
+		opp-hz = /bits/ 64 <750000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-1500000000 {
+		opp-hz = /bits/ 64 <1500000000>;
+		opp-microvolt = <1040000>;
+	};
+};
+
 &gmac0 {
 	status = "okay";
 };
@@ -38,9 +57,36 @@
 };
 
 &mmc0 {
+	cap-mmc-highspeed;
+	cap-mmc-hw-reset;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
+	vmmc-supply = <&vcc_3v3>;
+	vqmmc-supply = <&emmc_vdd>;
 	non-removable;
 };
 
+&mmc0_pins {
+	rst-pins {
+		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
+				      GPOEN_ENABLE,
+				      GPI_NONE)>;
+		bias-pull-up;
+		drive-strength = <12>;
+		input-disable;
+		input-schmitt-disable;
+		slew-rate = <0>;
+	};
+};
+
+&mmc1 {
+	no-sdio;
+	no-mmc;
+	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	post-power-on-delay-ms = <200>;
+};
+
 &pcie0 {
 	status = "okay";
 };
diff --git a/dts/upstream/src/riscv/starfive/jh7110.dtsi b/dts/upstream/src/riscv/starfive/jh7110.dtsi
index 0ba74ef0467..d2463399b95 100644
--- a/dts/upstream/src/riscv/starfive/jh7110.dtsi
+++ b/dts/upstream/src/riscv/starfive/jh7110.dtsi
@@ -200,22 +200,6 @@
 	cpu_opp: opp-table-0 {
 			compatible = "operating-points-v2";
 			opp-shared;
-			opp-375000000 {
-					opp-hz = /bits/ 64 <375000000>;
-					opp-microvolt = <800000>;
-			};
-			opp-500000000 {
-					opp-hz = /bits/ 64 <500000000>;
-					opp-microvolt = <800000>;
-			};
-			opp-750000000 {
-					opp-hz = /bits/ 64 <750000000>;
-					opp-microvolt = <800000>;
-			};
-			opp-1500000000 {
-					opp-hz = /bits/ 64 <1500000000>;
-					opp-microvolt = <1040000>;
-			};
 	};
 
 	thermal-zones {
-- 
2.43.2

Re: [RFC 01/10] riscv: dts: starfive: jh7110-common: Move out some nodes to the board dts

[Heinrich Schuchardt]

On 29.08.25 08:09, Hal Feng wrote:
> Some node in this file are not used by the upcoming VisionFive 2 Lite
> board. Move them to the board dts to prepare for adding the new
> VisionFive 2 Lite device tree.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

LGTM

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> ---
>   .../src/riscv/starfive/jh7110-common.dtsi     | 22 ---------
>   .../jh7110-deepcomputing-fml13v01.dts         | 49 +++++++++++++++++++
>   .../src/riscv/starfive/jh7110-milkv-mars.dts  | 49 +++++++++++++++++++
>   .../riscv/starfive/jh7110-pine64-star64.dts   | 49 +++++++++++++++++++
>   .../jh7110-starfive-visionfive-2.dtsi         | 46 +++++++++++++++++
>   dts/upstream/src/riscv/starfive/jh7110.dtsi   | 16 ------
>   6 files changed, 193 insertions(+), 38 deletions(-)
> 
> diff --git a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi b/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
> index 4baeb981d4d..9d3d03ad2ed 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
> +++ b/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
> @@ -272,15 +272,9 @@
>   	assigned-clock-rates = <50000000>;
>   	bus-width = <8>;
>   	bootph-pre-ram;
> -	cap-mmc-highspeed;
> -	mmc-ddr-1_8v;
> -	mmc-hs200-1_8v;
> -	cap-mmc-hw-reset;
>   	post-power-on-delay-ms = <200>;
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&mmc0_pins>;
> -	vmmc-supply = <&vcc_3v3>;
> -	vqmmc-supply = <&emmc_vdd>;
>   	status = "okay";
>   };
>   
> @@ -290,12 +284,7 @@
>   	assigned-clock-rates = <50000000>;
>   	bus-width = <4>;
>   	bootph-pre-ram;
> -	no-sdio;
> -	no-mmc;
> -	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
> -	disable-wp;
>   	cap-sd-highspeed;
> -	post-power-on-delay-ms = <200>;
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&mmc1_pins>;
>   	status = "okay";
> @@ -439,17 +428,6 @@
>   	};
>   
>   	mmc0_pins: mmc0-0 {
> -		 rst-pins {
> -			pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
> -					      GPOEN_ENABLE,
> -					      GPI_NONE)>;
> -			bias-pull-up;
> -			drive-strength = <12>;
> -			input-disable;
> -			input-schmitt-disable;
> -			slew-rate = <0>;
> -		};
> -
>   		mmc-pins {
>   			pinmux = <PINMUX(PAD_SD0_CLK, 0)>,
>   				 <PINMUX(PAD_SD0_CMD, 0)>,
> diff --git a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
> index f2857d021d6..5a2a41a7e8c 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
> +++ b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
> @@ -11,6 +11,55 @@
>   	compatible = "deepcomputing,fml13v01", "starfive,jh7110";
>   };
>   
> +&cpu_opp {
> +	opp-375000000 {
> +		opp-hz = /bits/ 64 <375000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-500000000 {
> +		opp-hz = /bits/ 64 <500000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-750000000 {
> +		opp-hz = /bits/ 64 <750000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-1500000000 {
> +		opp-hz = /bits/ 64 <1500000000>;
> +		opp-microvolt = <1040000>;
> +	};
> +};
> +
> +&mmc0 {
> +	cap-mmc-highspeed;
> +	cap-mmc-hw-reset;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&emmc_vdd>;
> +};
> +
> +&mmc0_pins {
> +	rst-pins {
> +		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
> +				      GPOEN_ENABLE,
> +				      GPI_NONE)>;
> +		bias-pull-up;
> +		drive-strength = <12>;
> +		input-disable;
> +		input-schmitt-disable;
> +		slew-rate = <0>;
> +	};
> +};
> +
> +&mmc1 {
> +	no-sdio;
> +	no-mmc;
> +	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	post-power-on-delay-ms = <200>;
> +};
> +
>   &pcie1 {
>   	perst-gpios = <&sysgpio 21 GPIO_ACTIVE_LOW>;
>   	phys = <&pciephy1>;
> diff --git a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts b/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
> index 3bd62ab7852..0c90facc4ee 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
> +++ b/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
> @@ -11,6 +11,25 @@
>   	compatible = "milkv,mars", "starfive,jh7110";
>   };
>   
> +&cpu_opp {
> +	opp-375000000 {
> +		opp-hz = /bits/ 64 <375000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-500000000 {
> +		opp-hz = /bits/ 64 <500000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-750000000 {
> +		opp-hz = /bits/ 64 <750000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-1500000000 {
> +		opp-hz = /bits/ 64 <1500000000>;
> +		opp-microvolt = <1040000>;
> +	};
> +};
> +
>   &gmac0 {
>   	starfive,tx-use-rgmii-clk;
>   	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
> @@ -22,6 +41,36 @@
>   	status = "okay";
>   };
>   
> +&mmc0 {
> +	cap-mmc-highspeed;
> +	cap-mmc-hw-reset;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&emmc_vdd>;
> +};
> +
> +&mmc0_pins {
> +	rst-pins {
> +		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
> +				      GPOEN_ENABLE,
> +				      GPI_NONE)>;
> +		bias-pull-up;
> +		drive-strength = <12>;
> +		input-disable;
> +		input-schmitt-disable;
> +		slew-rate = <0>;
> +	};
> +};
> +
> +&mmc1 {
> +	no-sdio;
> +	no-mmc;
> +	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	post-power-on-delay-ms = <200>;
> +};
> +
>   &pcie0 {
>   	status = "okay";
>   };
> diff --git a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
> index 31e825be206..c9677aef9ff 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
> +++ b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
> @@ -14,6 +14,25 @@
>   	};
>   };
>   
> +&cpu_opp {
> +	opp-375000000 {
> +		opp-hz = /bits/ 64 <375000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-500000000 {
> +		opp-hz = /bits/ 64 <500000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-750000000 {
> +		opp-hz = /bits/ 64 <750000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-1500000000 {
> +		opp-hz = /bits/ 64 <1500000000>;
> +		opp-microvolt = <1040000>;
> +	};
> +};
> +
>   &gmac0 {
>   	starfive,tx-use-rgmii-clk;
>   	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
> @@ -44,6 +63,36 @@
>   	status = "okay";
>   };
>   
> +&mmc0 {
> +	cap-mmc-highspeed;
> +	cap-mmc-hw-reset;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&emmc_vdd>;
> +};
> +
> +&mmc0_pins {
> +	rst-pins {
> +		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
> +				      GPOEN_ENABLE,
> +				      GPI_NONE)>;
> +		bias-pull-up;
> +		drive-strength = <12>;
> +		input-disable;
> +		input-schmitt-disable;
> +		slew-rate = <0>;
> +	};
> +};
> +
> +&mmc1 {
> +	no-sdio;
> +	no-mmc;
> +	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	post-power-on-delay-ms = <200>;
> +};
> +
>   &pcie1 {
>   	status = "okay";
>   };
> diff --git a/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi b/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
> index 5f14afb2c24..d1e4206f125 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -13,6 +13,25 @@
>   	};
>   };
>   
> +&cpu_opp {
> +	opp-375000000 {
> +		opp-hz = /bits/ 64 <375000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-500000000 {
> +		opp-hz = /bits/ 64 <500000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-750000000 {
> +		opp-hz = /bits/ 64 <750000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-1500000000 {
> +		opp-hz = /bits/ 64 <1500000000>;
> +		opp-microvolt = <1040000>;
> +	};
> +};
> +
>   &gmac0 {
>   	status = "okay";
>   };
> @@ -38,9 +57,36 @@
>   };
>   
>   &mmc0 {
> +	cap-mmc-highspeed;
> +	cap-mmc-hw-reset;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&emmc_vdd>;
>   	non-removable;
>   };
>   
> +&mmc0_pins {
> +	rst-pins {
> +		pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
> +				      GPOEN_ENABLE,
> +				      GPI_NONE)>;
> +		bias-pull-up;
> +		drive-strength = <12>;
> +		input-disable;
> +		input-schmitt-disable;
> +		slew-rate = <0>;
> +	};
> +};
> +
> +&mmc1 {
> +	no-sdio;
> +	no-mmc;
> +	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	post-power-on-delay-ms = <200>;
> +};
> +
>   &pcie0 {
>   	status = "okay";
>   };
> diff --git a/dts/upstream/src/riscv/starfive/jh7110.dtsi b/dts/upstream/src/riscv/starfive/jh7110.dtsi
> index 0ba74ef0467..d2463399b95 100644
> --- a/dts/upstream/src/riscv/starfive/jh7110.dtsi
> +++ b/dts/upstream/src/riscv/starfive/jh7110.dtsi
> @@ -200,22 +200,6 @@
>   	cpu_opp: opp-table-0 {
>   			compatible = "operating-points-v2";
>   			opp-shared;
> -			opp-375000000 {
> -					opp-hz = /bits/ 64 <375000000>;
> -					opp-microvolt = <800000>;
> -			};
> -			opp-500000000 {
> -					opp-hz = /bits/ 64 <500000000>;
> -					opp-microvolt = <800000>;
> -			};
> -			opp-750000000 {
> -					opp-hz = /bits/ 64 <750000000>;
> -					opp-microvolt = <800000>;
> -			};
> -			opp-1500000000 {
> -					opp-hz = /bits/ 64 <1500000000>;
> -					opp-microvolt = <1040000>;
> -			};
>   	};
>   
>   	thermal-zones {

Re: [RFC 01/10] riscv: dts: starfive: jh7110-common: Move out some nodes to the board dts

[E Shattow]

Hello Hal, and Heinrich,

On 8/29/25 00:19, Heinrich Schuchardt wrote:
> On 29.08.25 08:09, Hal Feng wrote:
>> Some node in this file are not used by the upcoming VisionFive 2 Lite
>> board. Move them to the board dts to prepare for adding the new
>> VisionFive 2 Lite device tree.
>>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> 
> LGTM
> 
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 

NAK

Better to drop jh7110-common-u-boot.dtsi from U-Boot [1]. Also we can
drop jh7110-u-boot.dtsi file but that does require some additional work:

- upstream dt-binding memory-controllers/starfive,jh7110-dmc.yaml [2]
- upstream devicetree changes adding memory controller [3]
- upstream bootph-pre-ram hints [4]
- implement cpufreq driver
- refactor drivers/ram/starfive/starfive_ddr.c to use common clock
framework and delete 'clock-frequency = <2133>' in dts

I am not interested in writing U-Boot drivers. Hal, can you make the
needed improvements in U-Boot for cpufreq and memory controller drivers?

1: https://lore.kernel.org/u-boot/20250815050315.62956-1-e@freeshell.de/
2: https://lore.kernel.org/lkml/20250823100159.203925-2-e@freeshell.de/
3: https://lore.kernel.org/lkml/20250823100159.203925-3-e@freeshell.de/
4: https://lore.kernel.org/lkml/20250823100159.203925-4-e@freeshell.de/

Thanks,  -E Shattow

>> ---
>>   .../src/riscv/starfive/jh7110-common.dtsi     | 22 ---------
>>   .../jh7110-deepcomputing-fml13v01.dts         | 49 +++++++++++++++++++
>>   .../src/riscv/starfive/jh7110-milkv-mars.dts  | 49 +++++++++++++++++++
>>   .../riscv/starfive/jh7110-pine64-star64.dts   | 49 +++++++++++++++++++
>>   .../jh7110-starfive-visionfive-2.dtsi         | 46 +++++++++++++++++
>>   dts/upstream/src/riscv/starfive/jh7110.dtsi   | 16 ------
>>   6 files changed, 193 insertions(+), 38 deletions(-)
>>
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi b/dts/
>> upstream/src/riscv/starfive/jh7110-common.dtsi
>> index 4baeb981d4d..9d3d03ad2ed 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
>> +++ b/dts/upstream/src/riscv/starfive/jh7110-common.dtsi
>> @@ -272,15 +272,9 @@
>>       assigned-clock-rates = <50000000>;
>>       bus-width = <8>;
>>       bootph-pre-ram;
>> -    cap-mmc-highspeed;
>> -    mmc-ddr-1_8v;
>> -    mmc-hs200-1_8v;
>> -    cap-mmc-hw-reset;
>>       post-power-on-delay-ms = <200>;
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&mmc0_pins>;
>> -    vmmc-supply = <&vcc_3v3>;
>> -    vqmmc-supply = <&emmc_vdd>;
>>       status = "okay";
>>   };
>>   @@ -290,12 +284,7 @@
>>       assigned-clock-rates = <50000000>;
>>       bus-width = <4>;
>>       bootph-pre-ram;
>> -    no-sdio;
>> -    no-mmc;
>> -    cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
>> -    disable-wp;
>>       cap-sd-highspeed;
>> -    post-power-on-delay-ms = <200>;
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&mmc1_pins>;
>>       status = "okay";
>> @@ -439,17 +428,6 @@
>>       };
>>         mmc0_pins: mmc0-0 {
>> -         rst-pins {
>> -            pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
>> -                          GPOEN_ENABLE,
>> -                          GPI_NONE)>;
>> -            bias-pull-up;
>> -            drive-strength = <12>;
>> -            input-disable;
>> -            input-schmitt-disable;
>> -            slew-rate = <0>;
>> -        };
>> -
>>           mmc-pins {
>>               pinmux = <PINMUX(PAD_SD0_CLK, 0)>,
>>                    <PINMUX(PAD_SD0_CMD, 0)>,
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-
>> fml13v01.dts b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-
>> fml13v01.dts
>> index f2857d021d6..5a2a41a7e8c 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
>> +++ b/dts/upstream/src/riscv/starfive/jh7110-deepcomputing-fml13v01.dts
>> @@ -11,6 +11,55 @@
>>       compatible = "deepcomputing,fml13v01", "starfive,jh7110";
>>   };
>>   +&cpu_opp {
>> +    opp-375000000 {
>> +        opp-hz = /bits/ 64 <375000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-500000000 {
>> +        opp-hz = /bits/ 64 <500000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-750000000 {
>> +        opp-hz = /bits/ 64 <750000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-1500000000 {
>> +        opp-hz = /bits/ 64 <1500000000>;
>> +        opp-microvolt = <1040000>;
>> +    };
>> +};
>> +
>> +&mmc0 {
>> +    cap-mmc-highspeed;
>> +    cap-mmc-hw-reset;
>> +    mmc-ddr-1_8v;
>> +    mmc-hs200-1_8v;
>> +    vmmc-supply = <&vcc_3v3>;
>> +    vqmmc-supply = <&emmc_vdd>;
>> +};
>> +
>> +&mmc0_pins {
>> +    rst-pins {
>> +        pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
>> +                      GPOEN_ENABLE,
>> +                      GPI_NONE)>;
>> +        bias-pull-up;
>> +        drive-strength = <12>;
>> +        input-disable;
>> +        input-schmitt-disable;
>> +        slew-rate = <0>;
>> +    };
>> +};
>> +
>> +&mmc1 {
>> +    no-sdio;
>> +    no-mmc;
>> +    cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
>> +    disable-wp;
>> +    post-power-on-delay-ms = <200>;
>> +};
>> +
>>   &pcie1 {
>>       perst-gpios = <&sysgpio 21 GPIO_ACTIVE_LOW>;
>>       phys = <&pciephy1>;
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts b/
>> dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
>> index 3bd62ab7852..0c90facc4ee 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
>> +++ b/dts/upstream/src/riscv/starfive/jh7110-milkv-mars.dts
>> @@ -11,6 +11,25 @@
>>       compatible = "milkv,mars", "starfive,jh7110";
>>   };
>>   +&cpu_opp {
>> +    opp-375000000 {
>> +        opp-hz = /bits/ 64 <375000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-500000000 {
>> +        opp-hz = /bits/ 64 <500000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-750000000 {
>> +        opp-hz = /bits/ 64 <750000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-1500000000 {
>> +        opp-hz = /bits/ 64 <1500000000>;
>> +        opp-microvolt = <1040000>;
>> +    };
>> +};
>> +
>>   &gmac0 {
>>       starfive,tx-use-rgmii-clk;
>>       assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
>> @@ -22,6 +41,36 @@
>>       status = "okay";
>>   };
>>   +&mmc0 {
>> +    cap-mmc-highspeed;
>> +    cap-mmc-hw-reset;
>> +    mmc-ddr-1_8v;
>> +    mmc-hs200-1_8v;
>> +    vmmc-supply = <&vcc_3v3>;
>> +    vqmmc-supply = <&emmc_vdd>;
>> +};
>> +
>> +&mmc0_pins {
>> +    rst-pins {
>> +        pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
>> +                      GPOEN_ENABLE,
>> +                      GPI_NONE)>;
>> +        bias-pull-up;
>> +        drive-strength = <12>;
>> +        input-disable;
>> +        input-schmitt-disable;
>> +        slew-rate = <0>;
>> +    };
>> +};
>> +
>> +&mmc1 {
>> +    no-sdio;
>> +    no-mmc;
>> +    cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
>> +    disable-wp;
>> +    post-power-on-delay-ms = <200>;
>> +};
>> +
>>   &pcie0 {
>>       status = "okay";
>>   };
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
>> b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
>> index 31e825be206..c9677aef9ff 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
>> +++ b/dts/upstream/src/riscv/starfive/jh7110-pine64-star64.dts
>> @@ -14,6 +14,25 @@
>>       };
>>   };
>>   +&cpu_opp {
>> +    opp-375000000 {
>> +        opp-hz = /bits/ 64 <375000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-500000000 {
>> +        opp-hz = /bits/ 64 <500000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-750000000 {
>> +        opp-hz = /bits/ 64 <750000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-1500000000 {
>> +        opp-hz = /bits/ 64 <1500000000>;
>> +        opp-microvolt = <1040000>;
>> +    };
>> +};
>> +
>>   &gmac0 {
>>       starfive,tx-use-rgmii-clk;
>>       assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
>> @@ -44,6 +63,36 @@
>>       status = "okay";
>>   };
>>   +&mmc0 {
>> +    cap-mmc-highspeed;
>> +    cap-mmc-hw-reset;
>> +    mmc-ddr-1_8v;
>> +    mmc-hs200-1_8v;
>> +    vmmc-supply = <&vcc_3v3>;
>> +    vqmmc-supply = <&emmc_vdd>;
>> +};
>> +
>> +&mmc0_pins {
>> +    rst-pins {
>> +        pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
>> +                      GPOEN_ENABLE,
>> +                      GPI_NONE)>;
>> +        bias-pull-up;
>> +        drive-strength = <12>;
>> +        input-disable;
>> +        input-schmitt-disable;
>> +        slew-rate = <0>;
>> +    };
>> +};
>> +
>> +&mmc1 {
>> +    no-sdio;
>> +    no-mmc;
>> +    cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
>> +    disable-wp;
>> +    post-power-on-delay-ms = <200>;
>> +};
>> +
>>   &pcie1 {
>>       status = "okay";
>>   };
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110-starfive-
>> visionfive-2.dtsi b/dts/upstream/src/riscv/starfive/jh7110-starfive-
>> visionfive-2.dtsi
>> index 5f14afb2c24..d1e4206f125 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/dts/upstream/src/riscv/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -13,6 +13,25 @@
>>       };
>>   };
>>   +&cpu_opp {
>> +    opp-375000000 {
>> +        opp-hz = /bits/ 64 <375000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-500000000 {
>> +        opp-hz = /bits/ 64 <500000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-750000000 {
>> +        opp-hz = /bits/ 64 <750000000>;
>> +        opp-microvolt = <800000>;
>> +    };
>> +    opp-1500000000 {
>> +        opp-hz = /bits/ 64 <1500000000>;
>> +        opp-microvolt = <1040000>;
>> +    };
>> +};
>> +
>>   &gmac0 {
>>       status = "okay";
>>   };
>> @@ -38,9 +57,36 @@
>>   };
>>     &mmc0 {
>> +    cap-mmc-highspeed;
>> +    cap-mmc-hw-reset;
>> +    mmc-ddr-1_8v;
>> +    mmc-hs200-1_8v;
>> +    vmmc-supply = <&vcc_3v3>;
>> +    vqmmc-supply = <&emmc_vdd>;
>>       non-removable;
>>   };
>>   +&mmc0_pins {
>> +    rst-pins {
>> +        pinmux = <GPIOMUX(62, GPOUT_SYS_SDIO0_RST,
>> +                      GPOEN_ENABLE,
>> +                      GPI_NONE)>;
>> +        bias-pull-up;
>> +        drive-strength = <12>;
>> +        input-disable;
>> +        input-schmitt-disable;
>> +        slew-rate = <0>;
>> +    };
>> +};
>> +
>> +&mmc1 {
>> +    no-sdio;
>> +    no-mmc;
>> +    cd-gpios = <&sysgpio 41 GPIO_ACTIVE_LOW>;
>> +    disable-wp;
>> +    post-power-on-delay-ms = <200>;
>> +};
>> +
>>   &pcie0 {
>>       status = "okay";
>>   };
>> diff --git a/dts/upstream/src/riscv/starfive/jh7110.dtsi b/dts/
>> upstream/src/riscv/starfive/jh7110.dtsi
>> index 0ba74ef0467..d2463399b95 100644
>> --- a/dts/upstream/src/riscv/starfive/jh7110.dtsi
>> +++ b/dts/upstream/src/riscv/starfive/jh7110.dtsi
>> @@ -200,22 +200,6 @@
>>       cpu_opp: opp-table-0 {
>>               compatible = "operating-points-v2";
>>               opp-shared;
>> -            opp-375000000 {
>> -                    opp-hz = /bits/ 64 <375000000>;
>> -                    opp-microvolt = <800000>;
>> -            };
>> -            opp-500000000 {
>> -                    opp-hz = /bits/ 64 <500000000>;
>> -                    opp-microvolt = <800000>;
>> -            };
>> -            opp-750000000 {
>> -                    opp-hz = /bits/ 64 <750000000>;
>> -                    opp-microvolt = <800000>;
>> -            };
>> -            opp-1500000000 {
>> -                    opp-hz = /bits/ 64 <1500000000>;
>> -                    opp-microvolt = <1040000>;
>> -            };
>>       };
>>         thermal-zones {
> 

RE: [RFC 01/10] riscv: dts: starfive: jh7110-common: Move out some nodes to the board dts

[Hal Feng]

> On 03.09.25 00:18, E Shattow wrote:
> 
> Hello Hal, and Heinrich,
> 
> On 8/29/25 00:19, Heinrich Schuchardt wrote:
> > On 29.08.25 08:09, Hal Feng wrote:
> >> Some node in this file are not used by the upcoming VisionFive 2 Lite
> >> board. Move them to the board dts to prepare for adding the new
> >> VisionFive 2 Lite device tree.
> >>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >
> > LGTM
> >
> > Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >
> 
> NAK

Please ignore patch 1 and patch 2.

> 
> Better to drop jh7110-common-u-boot.dtsi from U-Boot [1]. Also we can drop
> jh7110-u-boot.dtsi file but that does require some additional work:
> 
> - upstream dt-binding memory-controllers/starfive,jh7110-dmc.yaml [2]
> - upstream devicetree changes adding memory controller [3]
> - upstream bootph-pre-ram hints [4]
> - implement cpufreq driver

Why do we need the cpufreq driver in u-boot? You want the u-boot to run at the
highest frequency?
I think it is more stable to keep the u-boot running at 1GHz with lower power
consumption.

> - refactor drivers/ram/starfive/starfive_ddr.c to use common clock framework
> and delete 'clock-frequency = <2133>' in dts

2133 is the DDR data rate to set. We can not get this frequency using common
clock framework.

Best regards,
Hal

> 
> I am not interested in writing U-Boot drivers. Hal, can you make the needed
> improvements in U-Boot for cpufreq and memory controller drivers?
> 
> 1: https://lore.kernel.org/u-boot/20250815050315.62956-1-e@freeshell.de/
> 2: https://lore.kernel.org/lkml/20250823100159.203925-2-e@freeshell.de/
> 3: https://lore.kernel.org/lkml/20250823100159.203925-3-e@freeshell.de/
> 4: https://lore.kernel.org/lkml/20250823100159.203925-4-e@freeshell.de/

Re: [RFC 01/10] riscv: dts: starfive: jh7110-common: Move out some nodes to the board dts

[E Shattow]

On 10/22/25 02:59, Hal Feng wrote:
>> On 03.09.25 00:18, E Shattow wrote:
>>
>> Hello Hal, and Heinrich,
>>
>> On 8/29/25 00:19, Heinrich Schuchardt wrote:
>>> On 29.08.25 08:09, Hal Feng wrote:
>>>> Some node in this file are not used by the upcoming VisionFive 2 Lite
>>>> board. Move them to the board dts to prepare for adding the new
>>>> VisionFive 2 Lite device tree.
>>>>
>>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>>
>>> LGTM
>>>
>>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>
>>
>> NAK
> 
> Please ignore patch 1 and patch 2.
> 
>>
>> Better to drop jh7110-common-u-boot.dtsi from U-Boot [1]. Also we can drop
>> jh7110-u-boot.dtsi file but that does require some additional work:
>>
>> - upstream dt-binding memory-controllers/starfive,jh7110-dmc.yaml [2]
>> - upstream devicetree changes adding memory controller [3]
>> - upstream bootph-pre-ram hints [4]
>> - implement cpufreq driver
> 
> Why do we need the cpufreq driver in u-boot? You want the u-boot to run at the
> highest frequency?
> I think it is more stable to keep the u-boot running at 1GHz with lower power
> consumption.
> 

The OPP tables are now from upstream Linux so it needs to be done. What
for higher frequency and voltage or lower frequency and voltage, that
should be determined by upstream dts not our override.

>> - refactor drivers/ram/starfive/starfive_ddr.c to use common clock framework
>> and delete 'clock-frequency = <2133>' in dts
> 
> 2133 is the DDR data rate to set. We can not get this frequency using common
> clock framework.
> 
> Best regards,
> Hal
> 

"2133" is a wrong value to have in devicetree for this property, and
this was rejected by Linux Kernel upstream:

https://lore.kernel.org/lkml/2e53fc11-49eb-420c-ab8e-57e9493c42ba@kernel.org/

Krzysztof says there "Drop clock-frequency. I doubt 2133 Hz is even
correct... This is a legacy property, use common clock framework."

>>
>> I am not interested in writing U-Boot drivers. Hal, can you make the needed
>> improvements in U-Boot for cpufreq and memory controller drivers?
>>
>> 1: https://lore.kernel.org/u-boot/20250815050315.62956-1-e@freeshell.de/
>> 2: https://lore.kernel.org/lkml/20250823100159.203925-2-e@freeshell.de/
>> 3: https://lore.kernel.org/lkml/20250823100159.203925-3-e@freeshell.de/
>> 4: https://lore.kernel.org/lkml/20250823100159.203925-4-e@freeshell.de/
> 

Thanks,

-E

[RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board device tree

[Hal Feng]

VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S SoC.

Board features:
- JH7110S SoC
- 2/4/8 GiB LPDDR4 DRAM
- AXP15060 PMIC
- 40 pin GPIO header
- 1x USB 3.0 host port
- 3x USB 2.0 host port
- 1x M.2 M-Key (size: 2242)
- 1x MicroSD slot (optional non-removable eMMC)
- 1x QSPI Flash
- 1x I2C EEPROM
- 1x 1Gbps Ethernet port
- SDIO-based Wi-Fi & UART-based Bluetooth
- 1x HDMI port
- 1x 2-lane DSI
- 1x 2-lane CSI

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
 1 file changed, 152 insertions(+)
 create mode 100644 dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts

diff --git a/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
new file mode 100644
index 00000000000..a0cb9912eb8
--- /dev/null
+++ b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2025 StarFive Technology Co., Ltd.
+ * Copyright (C) 2025 Hal Feng <hal.feng@starfivetech.com>
+ */
+
+/dts-v1/;
+#include "jh7110-common.dtsi"
+
+/ {
+	model = "StarFive VisionFive 2 Lite";
+	compatible = "starfive,visionfive-2-lite", "starfive,jh7110s";
+};
+
+&cpu_opp {
+	opp-312500000 {
+		opp-hz = /bits/ 64 <312500000>;
+		opp-microvolt = <800000>;
+	};
+	opp-417000000 {
+		opp-hz = /bits/ 64 <417000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-625000000 {
+		opp-hz = /bits/ 64 <625000000>;
+		opp-microvolt = <800000>;
+	};
+	opp-1250000000 {
+		opp-hz = /bits/ 64 <1250000000>;
+		opp-microvolt = <1000000>;
+	};
+};
+
+&gmac0 {
+	starfive,tx-use-rgmii-clk;
+	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
+	assigned-clock-parents = <&aoncrg JH7110_AONCLK_GMAC0_RMII_RTX>;
+	status = "okay";
+};
+
+&i2c0 {
+	status = "okay";
+};
+
+&mmc0 {
+	bus-width = <4>;
+	no-sdio;
+	no-mmc;
+	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_HIGH>;
+	disable-wp;
+	cap-sd-highspeed;
+};
+
+&mmc1 {
+	max-frequency = <50000000>;
+	keep-power-in-suspend;
+	non-removable;
+};
+
+&pcie1 {
+	enable-gpios = <&sysgpio 27 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+};
+
+&phy0 {
+	motorcomm,tx-clk-adj-enabled;
+	motorcomm,tx-clk-100-inverted;
+	motorcomm,tx-clk-1000-inverted;
+	motorcomm,rx-clk-drv-microamp = <3970>;
+	motorcomm,rx-data-drv-microamp = <2910>;
+	rx-internal-delay-ps = <1500>;
+	tx-internal-delay-ps = <1500>;
+};
+
+&pwm {
+	status = "okay";
+};
+
+&spi0 {
+	status = "okay";
+};
+
+&syscrg {
+	assigned-clock-rates = <0>, <0>, <0>, <0>, <500000000>, <1250000000>;
+};
+
+&sysgpio {
+	uart1_pins: uart1-0 {
+		tx-pins {
+			pinmux = <GPIOMUX(22, GPOUT_SYS_UART1_TX,
+					      GPOEN_ENABLE,
+					      GPI_NONE)>;
+			bias-disable;
+			drive-strength = <12>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		rx-pins {
+			pinmux = <GPIOMUX(23, GPOUT_LOW,
+					      GPOEN_DISABLE,
+					      GPI_SYS_UART1_RX)>;
+			bias-pull-up;
+			drive-strength = <2>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+
+		cts-pins {
+			pinmux = <GPIOMUX(24, GPOUT_LOW,
+					      GPOEN_DISABLE,
+					      GPI_SYS_UART1_CTS)>;
+			input-enable;
+		};
+
+		rts-pins {
+			pinmux = <GPIOMUX(25, GPOUT_SYS_UART1_RTS,
+					      GPOEN_ENABLE,
+					      GPI_NONE)>;
+			input-enable;
+		};
+	};
+
+	usb0_pins: usb0-0 {
+		power-pins {
+			pinmux = <GPIOMUX(26, GPOUT_HIGH,
+					      GPOEN_ENABLE,
+					      GPI_NONE)>;
+			input-disable;
+		};
+	};
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>;
+	status = "okay";
+};
+
+&usb0 {
+	dr_mode = "host";
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb0_pins>;
+	status = "okay";
+};
+
+&usb_cdns3 {
+	phys = <&usbphy0>, <&pciephy0>;
+	phy-names = "cdns3,usb2-phy", "cdns3,usb3-phy";
+};
-- 
2.43.2

Re: [RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board device tree

[E Shattow]

Hi Hal,

On 8/28/25 23:09, Hal Feng wrote:
> VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S SoC.
> 
> Board features:
> - JH7110S SoC
> - 2/4/8 GiB LPDDR4 DRAM
> - AXP15060 PMIC
> - 40 pin GPIO header
> - 1x USB 3.0 host port
> - 3x USB 2.0 host port
> - 1x M.2 M-Key (size: 2242)
> - 1x MicroSD slot (optional non-removable eMMC)
> - 1x QSPI Flash
> - 1x I2C EEPROM
> - 1x 1Gbps Ethernet port
> - SDIO-based Wi-Fi & UART-based Bluetooth
> - 1x HDMI port
> - 1x 2-lane DSI
> - 1x 2-lane CSI
> 

Prefer to see subject line and URL of recent upstream discussion or git
web commit for the new or modified dts changes from upstream of
devicetree-rebasing in this commit and not just in the cover letter.

+ From upstream LKML discussion "[RFC 3/3] riscv: dts: starfive: Add
VisionFive 2 Lite board device tree" [1].
+ 1:
https://lore.kernel.org/all/20250821100930.71404-4-hal.feng@starfivetech.com/

> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
>  1 file changed, 152 insertions(+)
>  create mode 100644 dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
> 
> diff --git a/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
> new file mode 100644
> index 00000000000..a0cb9912eb8
> --- /dev/null
> +++ b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts

No, do not directly add or modify upstream dts in dts/upstream/src
subtree. We are not permitted in U-Boot to merge out-of-band devicetree
changes to the devicetree-rebasing subtree as this creates more work for
Tom, if I understand correctly?

Instead add or modify this dts as
arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi filename.

Note: Locally for development purpose I would like to extend U-Boot to
choose a Linux source tree path for location of devicetree. Today
OF_UPSTREAM support in U-Boot build system assumes to use the
devicetree-rebasing subtree and the path is not meant to be changed. The
devicetree-rebasing scripts assume only to merge from regular Linux
release version tags and not arbitrary commit IDs. As a workaround I do
something like git recursive remove the
dts/upstream/src/$(ARCH)/$(VENDOR) and replace with symbolic link to the
Linux source tree arch/$(ARCH)/boot/dts/$(VENDOR) for development. I
would like better if it was simply an environment flag for make system
where Linux or devicetree-rebasing source tree is located, but that is
for a different discussion not in this series.

> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2025 StarFive Technology Co., Ltd.
> + * Copyright (C) 2025 Hal Feng <hal.feng@starfivetech.com>
> + */
> +
> +/dts-v1/;
> +#include "jh7110-common.dtsi"
> +
> +/ {
> +	model = "StarFive VisionFive 2 Lite";
> +	compatible = "starfive,visionfive-2-lite", "starfive,jh7110s";
> +};
> +
> +&cpu_opp {
> +	opp-312500000 {
> +		opp-hz = /bits/ 64 <312500000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-417000000 {
> +		opp-hz = /bits/ 64 <417000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-625000000 {
> +		opp-hz = /bits/ 64 <625000000>;
> +		opp-microvolt = <800000>;
> +	};
> +	opp-1250000000 {
> +		opp-hz = /bits/ 64 <1250000000>;
> +		opp-microvolt = <1000000>;
> +	};
> +};
> +
> +&gmac0 {
> +	starfive,tx-use-rgmii-clk;
> +	assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>;
> +	assigned-clock-parents = <&aoncrg JH7110_AONCLK_GMAC0_RMII_RTX>;
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&mmc0 {
> +	bus-width = <4>;
> +	no-sdio;
> +	no-mmc;
> +	cd-gpios = <&sysgpio 41 GPIO_ACTIVE_HIGH>;
> +	disable-wp;
> +	cap-sd-highspeed;
> +};
> +
> +&mmc1 {
> +	max-frequency = <50000000>;
> +	keep-power-in-suspend;
> +	non-removable;
> +};
> +
> +&pcie1 {
> +	enable-gpios = <&sysgpio 27 GPIO_ACTIVE_HIGH>;
> +	status = "okay";
> +};
> +
> +&phy0 {
> +	motorcomm,tx-clk-adj-enabled;
> +	motorcomm,tx-clk-100-inverted;
> +	motorcomm,tx-clk-1000-inverted;
> +	motorcomm,rx-clk-drv-microamp = <3970>;
> +	motorcomm,rx-data-drv-microamp = <2910>;
> +	rx-internal-delay-ps = <1500>;
> +	tx-internal-delay-ps = <1500>;
> +};
> +
> +&pwm {
> +	status = "okay";
> +};
> +
> +&spi0 {
> +	status = "okay";
> +};
> +
> +&syscrg {
> +	assigned-clock-rates = <0>, <0>, <0>, <0>, <500000000>, <1250000000>;
> +};
> +
> +&sysgpio {
> +	uart1_pins: uart1-0 {
> +		tx-pins {
> +			pinmux = <GPIOMUX(22, GPOUT_SYS_UART1_TX,
> +					      GPOEN_ENABLE,
> +					      GPI_NONE)>;
> +			bias-disable;
> +			drive-strength = <12>;
> +			input-disable;
> +			input-schmitt-disable;
> +			slew-rate = <0>;
> +		};
> +
> +		rx-pins {
> +			pinmux = <GPIOMUX(23, GPOUT_LOW,
> +					      GPOEN_DISABLE,
> +					      GPI_SYS_UART1_RX)>;
> +			bias-pull-up;
> +			drive-strength = <2>;
> +			input-enable;
> +			input-schmitt-enable;
> +			slew-rate = <0>;
> +		};
> +
> +		cts-pins {
> +			pinmux = <GPIOMUX(24, GPOUT_LOW,
> +					      GPOEN_DISABLE,
> +					      GPI_SYS_UART1_CTS)>;
> +			input-enable;
> +		};
> +
> +		rts-pins {
> +			pinmux = <GPIOMUX(25, GPOUT_SYS_UART1_RTS,
> +					      GPOEN_ENABLE,
> +					      GPI_NONE)>;
> +			input-enable;
> +		};
> +	};
> +
> +	usb0_pins: usb0-0 {
> +		power-pins {
> +			pinmux = <GPIOMUX(26, GPOUT_HIGH,
> +					      GPOEN_ENABLE,
> +					      GPI_NONE)>;
> +			input-disable;
> +		};
> +	};
> +};
> +
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
> +	status = "okay";
> +};
> +
> +&usb0 {
> +	dr_mode = "host";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb0_pins>;
> +	status = "okay";
> +};
> +
> +&usb_cdns3 {
> +	phys = <&usbphy0>, <&pciephy0>;
> +	phy-names = "cdns3,usb2-phy", "cdns3,usb3-phy";
> +};

Append the last line of this file an '#include' statement corresponding
with the next automatic dtsi inclusion order [1] as appropriate,
currently when we drop file jh7110-common-u-boot.dtsi from U-Boot:

+ #include "jh7110-u-boot.dtsi"

Or better approach in future for last line of the file would be if when
CONFIG_NAME is added to the search order [2], then this include would
instead be a hyphenated form of $(CONFIG_SYS_CONFIG_NAME)-u-boot.dtsi:

+ #include "starfive-visionfive2-u-boot.dtsi"

And drop all dts/riscv/jh7110*{.dts,.dtsi} not existing in
dts/upstream/src subtree.

Anyways I discover the build system is not working correctly for '$<'
symbol [3] and this breaks our assumption that
arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi would be
automatically included. It is needed to fix the situation separate from
this series.

1: https://docs.u-boot.org/en/latest/develop/devicetree/control.html
2: https://lore.kernel.org/u-boot/20250826214708.309271-1-e@freeshell.de/
3: https://lore.kernel.org/u-boot/20250901094506.647427-1-e@freeshell.de/

-E

Re: [RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board device tree

[Sumit Garg]

On Tue, Sep 02, 2025 at 01:03:09PM -0700, E Shattow wrote:
> Hi Hal,
> 
> On 8/28/25 23:09, Hal Feng wrote:
> > VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S SoC.
> > 
> > Board features:
> > - JH7110S SoC
> > - 2/4/8 GiB LPDDR4 DRAM
> > - AXP15060 PMIC
> > - 40 pin GPIO header
> > - 1x USB 3.0 host port
> > - 3x USB 2.0 host port
> > - 1x M.2 M-Key (size: 2242)
> > - 1x MicroSD slot (optional non-removable eMMC)
> > - 1x QSPI Flash
> > - 1x I2C EEPROM
> > - 1x 1Gbps Ethernet port
> > - SDIO-based Wi-Fi & UART-based Bluetooth
> > - 1x HDMI port
> > - 1x 2-lane DSI
> > - 1x 2-lane CSI
> > 
> 
> Prefer to see subject line and URL of recent upstream discussion or git
> web commit for the new or modified dts changes from upstream of
> devicetree-rebasing in this commit and not just in the cover letter.
> 
> + From upstream LKML discussion "[RFC 3/3] riscv: dts: starfive: Add
> VisionFive 2 Lite board device tree" [1].
> + 1:
> https://lore.kernel.org/all/20250821100930.71404-4-hal.feng@starfivetech.com/
> 
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
> >  1 file changed, 152 insertions(+)
> >  create mode 100644 dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
> > 
> > diff --git a/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
> > new file mode 100644
> > index 00000000000..a0cb9912eb8
> > --- /dev/null
> > +++ b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
> 
> No, do not directly add or modify upstream dts in dts/upstream/src
> subtree. We are not permitted in U-Boot to merge out-of-band devicetree
> changes to the devicetree-rebasing subtree as this creates more work for
> Tom, if I understand correctly?
> 
> Instead add or modify this dts as
> arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi filename.

That's right. dts/upstream subtree isn't allowed to be directly patched
but it is rather synced regularly with devicetree-rebasing. So please
follow documentation documentation here [1].

[1] https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing

-Sumit

Re: [RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board device tree

[Heinrich Schuchardt]

On 9/3/25 13:07, Sumit Garg wrote:
> On Tue, Sep 02, 2025 at 01:03:09PM -0700, E Shattow wrote:
>> Hi Hal,
>>
>> On 8/28/25 23:09, Hal Feng wrote:
>>> VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S SoC.
>>>
>>> Board features:
>>> - JH7110S SoC
>>> - 2/4/8 GiB LPDDR4 DRAM
>>> - AXP15060 PMIC
>>> - 40 pin GPIO header
>>> - 1x USB 3.0 host port
>>> - 3x USB 2.0 host port
>>> - 1x M.2 M-Key (size: 2242)
>>> - 1x MicroSD slot (optional non-removable eMMC)
>>> - 1x QSPI Flash
>>> - 1x I2C EEPROM
>>> - 1x 1Gbps Ethernet port
>>> - SDIO-based Wi-Fi & UART-based Bluetooth
>>> - 1x HDMI port
>>> - 1x 2-lane DSI
>>> - 1x 2-lane CSI
>>>
>>
>> Prefer to see subject line and URL of recent upstream discussion or git
>> web commit for the new or modified dts changes from upstream of
>> devicetree-rebasing in this commit and not just in the cover letter.
>>
>> + From upstream LKML discussion "[RFC 3/3] riscv: dts: starfive: Add
>> VisionFive 2 Lite board device tree" [1].
>> + 1:
>> https://lore.kernel.org/all/20250821100930.71404-4-hal.feng@starfivetech.com/
>>
>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>> ---
>>>   .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
>>>   1 file changed, 152 insertions(+)
>>>   create mode 100644 dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
>>>
>>> diff --git a/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
>>> new file mode 100644
>>> index 00000000000..a0cb9912eb8
>>> --- /dev/null
>>> +++ b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.dts
>>
>> No, do not directly add or modify upstream dts in dts/upstream/src
>> subtree. We are not permitted in U-Boot to merge out-of-band devicetree
>> changes to the devicetree-rebasing subtree as this creates more work for
>> Tom, if I understand correctly?
>>
>> Instead add or modify this dts as
>> arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi filename.
> 
> That's right. dts/upstream subtree isn't allowed to be directly patched
> but it is rather synced regularly with devicetree-rebasing. So please
> follow documentation documentation here [1].
> 
> [1] https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing
> 
> -Sumit

Is mixing upstream dtbs and non upstream dtbs in CONFIG_OF_LIST 
supported by buildman?
Do files in arch/risc-v/dts/ override upstream files of the same name?

Best regards

Heinrich

RE: [RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board device tree

[Hal Feng]

> On 03.09.25 19:07, Sumit Garg wrote:
> On Tue, Sep 02, 2025 at 01:03:09PM -0700, E Shattow wrote:
> > Hi Hal,
> >
> > On 8/28/25 23:09, Hal Feng wrote:
> > > VisionFive 2 Lite is a mini SBC based on the StarFive JH7110S SoC.
> > >
> > > Board features:
> > > - JH7110S SoC
> > > - 2/4/8 GiB LPDDR4 DRAM
> > > - AXP15060 PMIC
> > > - 40 pin GPIO header
> > > - 1x USB 3.0 host port
> > > - 3x USB 2.0 host port
> > > - 1x M.2 M-Key (size: 2242)
> > > - 1x MicroSD slot (optional non-removable eMMC)
> > > - 1x QSPI Flash
> > > - 1x I2C EEPROM
> > > - 1x 1Gbps Ethernet port
> > > - SDIO-based Wi-Fi & UART-based Bluetooth
> > > - 1x HDMI port
> > > - 1x 2-lane DSI
> > > - 1x 2-lane CSI
> > >
> >
> > Prefer to see subject line and URL of recent upstream discussion or
> > git web commit for the new or modified dts changes from upstream of
> > devicetree-rebasing in this commit and not just in the cover letter.
> >
> > + From upstream LKML discussion "[RFC 3/3] riscv: dts: starfive: Add
> > VisionFive 2 Lite board device tree" [1].
> > + 1:
> > https://lore.kernel.org/all/20250821100930.71404-4-hal.feng@starfivete
> > ch.com/
> >
> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > > ---
> > >  .../jh7110s-starfive-visionfive-2-lite.dts    | 152 ++++++++++++++++++
> > >  1 file changed, 152 insertions(+)
> > >  create mode 100644
> > > dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite.d
> > > ts
> > >
> > > diff --git
> > > a/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite
> > > .dts
> > > b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-lite
> > > .dts
> > > new file mode 100644
> > > index 00000000000..a0cb9912eb8
> > > --- /dev/null
> > > +++ b/dts/upstream/src/riscv/starfive/jh7110s-starfive-visionfive-2-
> > > +++ lite.dts
> >
> > No, do not directly add or modify upstream dts in dts/upstream/src
> > subtree. We are not permitted in U-Boot to merge out-of-band
> > devicetree changes to the devicetree-rebasing subtree as this creates
> > more work for Tom, if I understand correctly?
> >
> > Instead add or modify this dts as
> > arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi filename.
> 
> That's right. dts/upstream subtree isn't allowed to be directly patched but it is
> rather synced regularly with devicetree-rebasing. So please follow
> documentation documentation here [1].
> 
> [1] https://docs.u-
> boot.org/en/latest/develop/devicetree/control.html#resyncing-with-
> devicetree-rebasing

Please ignore patch 1 and patch 2 and don't merge them into u-boot.
I  had noted it in the cover-letter. To be clearer, I will point it out in the
commit messages next time.

The reason to add these two patches is that it will fail to compile without
these two patches and be more convenient for the developers to test the
series.

Best regards,
Hal

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

[Hal Feng]

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;
 
-	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)
-- 
2.43.2

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

[Heinrich Schuchardt]

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)

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

[E Shattow]

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

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

-E

Re: [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@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@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

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

[Hal Feng]

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

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

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

[E Shattow]

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

[RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Hal Feng]

pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
Move the function description to the header file.
Remove the function calls in board/starfive/visionfive2/.

Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
 board/starfive/visionfive2/spl.c              | 18 ++++++-----------
 .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
 .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
 4 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h b/arch/riscv/include/asm/arch-jh7110/eeprom.h
index 6d0a0ba0c4a..025f1d32c49 100644
--- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
+++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
@@ -9,6 +9,11 @@
 
 #include <linux/types.h>
 
+/**
+ * get_pcb_revision_from_eeprom() - get the PCB revision
+ *
+ * @return: the PCB revision or 0xFF on error.
+ */
 u8 get_pcb_revision_from_eeprom(void);
 
 /**
diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
index 3dfa931b655..901e7b58f36 100644
--- a/board/starfive/visionfive2/spl.c
+++ b/board/starfive/visionfive2/spl.c
@@ -126,19 +126,13 @@ int board_fit_config_name_match(const char *name)
 		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
 		return 0;
 	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a") &&
-		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
-		switch (get_pcb_revision_from_eeprom()) {
-		case 'a':
-		case 'A':
-			return 0;
-		}
+		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
+		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7))) {
+		return 0;
 	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b") &&
-		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
-		switch (get_pcb_revision_from_eeprom()) {
-		case 'b':
-		case 'B':
-			return 0;
-		}
+		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
+		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7))) {
+		return 0;
 	}
 
 	return -EINVAL;
diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c
index bfbb11a2ee7..f38433e94ac 100644
--- a/board/starfive/visionfive2/starfive_visionfive2.c
+++ b/board/starfive/visionfive2/starfive_visionfive2.c
@@ -59,20 +59,12 @@ static void set_fdtfile(void)
 		fdtfile = "starfive/jh7110-milkv-mars.dtb";
 	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
 		fdtfile = "starfive/jh7110-pine64-star64.dtb";
-	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
-		switch (get_pcb_revision_from_eeprom()) {
-		case 'a':
-		case 'A':
-			fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
-			break;
-		case 'b':
-		case 'B':
-			fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
-			break;
-		default:
-			log_err("Unknown revision\n");
-			return;
-		}
+	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
+		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
+		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
+	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
+		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
+		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
 	} else {
 		log_err("Unknown product\n");
 		return;
diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
index 3866d07f9d4..43b8af4fc59 100644
--- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
+++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
@@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
 	return 0;
 }
 
-/**
- * get_pcb_revision_from_eeprom - get the PCB revision
- *
- * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
- */
 u8 get_pcb_revision_from_eeprom(void)
 {
-	u8 pv = 0xFF;
-
 	if (read_eeprom())
-		return pv;
+		return 0xFF;
 
-	return pbuf.eeprom.atom1.data.pstr[6];
+	return pbuf.eeprom.atom4.data.pcb_revision;
 }
 
 u8 get_ddr_size_from_eeprom(void)
-- 
2.43.2

Re: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Heinrich Schuchardt]

On 29.08.25 08:09, Hal Feng wrote:
> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
> Move the function description to the header file.
> Remove the function calls in board/starfive/visionfive2/.
> 
> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>   arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
>   board/starfive/visionfive2/spl.c              | 18 ++++++-----------
>   .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
>   .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
>   4 files changed, 19 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> index 6d0a0ba0c4a..025f1d32c49 100644
> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> @@ -9,6 +9,11 @@
>   
>   #include <linux/types.h>
>   
> +/**
> + * get_pcb_revision_from_eeprom() - get the PCB revision
> + *
> + * @return: the PCB revision or 0xFF on error.
> + */
>   u8 get_pcb_revision_from_eeprom(void);
>   
>   /**
> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
> index 3dfa931b655..901e7b58f36 100644
> --- a/board/starfive/visionfive2/spl.c
> +++ b/board/starfive/visionfive2/spl.c
> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char *name)
>   		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>   		return 0;
>   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a") &&
> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> -		switch (get_pcb_revision_from_eeprom()) {
> -		case 'a':
> -		case 'A':
> -			return 0;
> -		}
> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7))) {
> +		return 0;
>   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b") &&
> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> -		switch (get_pcb_revision_from_eeprom()) {
> -		case 'b':
> -		case 'B':
> -			return 0;
> -		}
> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7))) {
> +		return 0;
>   	}
>   
>   	return -EINVAL;
> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c
> index bfbb11a2ee7..f38433e94ac 100644
> --- a/board/starfive/visionfive2/starfive_visionfive2.c
> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
>   		fdtfile = "starfive/jh7110-milkv-mars.dtb";
>   	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>   		fdtfile = "starfive/jh7110-pine64-star64.dtb";
> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> -		switch (get_pcb_revision_from_eeprom()) {
> -		case 'a':
> -		case 'A':
> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> -			break;
> -		case 'b':
> -		case 'B':
> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> -			break;
> -		default:
> -			log_err("Unknown revision\n");
> -			return;
> -		}
> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {

Where boards both with 'a' and 'b' ever shipped?
I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].

The change looks technically correct.

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
>   	} else {
>   		log_err("Unknown product\n");
>   		return;
> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> index 3866d07f9d4..43b8af4fc59 100644
> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
>   	return 0;
>   }
>   
> -/**
> - * get_pcb_revision_from_eeprom - get the PCB revision
> - *
> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
> - */
>   u8 get_pcb_revision_from_eeprom(void)
>   {
> -	u8 pv = 0xFF;
> -
>   	if (read_eeprom())
> -		return pv;
> +		return 0xFF;
>   
> -	return pbuf.eeprom.atom1.data.pstr[6];
> +	return pbuf.eeprom.atom4.data.pcb_revision;
>   }
>   
>   u8 get_ddr_size_from_eeprom(void)

RE: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Hal Feng]

> On 29.08.25 15:44, Heinrich Schuchardt wrote:
> On 29.08.25 08:09, Hal Feng wrote:
> > pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
> > Move the function description to the header file.
> > Remove the function calls in board/starfive/visionfive2/.
> >
> > Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >   arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
> >   board/starfive/visionfive2/spl.c              | 18 ++++++-----------
> >   .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
> >   .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
> >   4 files changed, 19 insertions(+), 35 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > index 6d0a0ba0c4a..025f1d32c49 100644
> > --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > @@ -9,6 +9,11 @@
> >
> >   #include <linux/types.h>
> >
> > +/**
> > + * get_pcb_revision_from_eeprom() - get the PCB revision
> > + *
> > + * @return: the PCB revision or 0xFF on error.
> > + */
> >   u8 get_pcb_revision_from_eeprom(void);
> >
> >   /**
> > diff --git a/board/starfive/visionfive2/spl.c
> b/board/starfive/visionfive2/spl.c
> > index 3dfa931b655..901e7b58f36 100644
> > --- a/board/starfive/visionfive2/spl.c
> > +++ b/board/starfive/visionfive2/spl.c
> > @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
> *name)
> >   		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >   		return 0;
> >   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a")
> &&
> > -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> > -		switch (get_pcb_revision_from_eeprom()) {
> > -		case 'a':
> > -		case 'A':
> > -			return 0;
> > -		}
> > +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
> ||
> > +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
> {
> > +		return 0;
> >   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b")
> &&
> > -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> > -		switch (get_pcb_revision_from_eeprom()) {
> > -		case 'b':
> > -		case 'B':
> > -			return 0;
> > -		}
> > +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
> ||
> > +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
> {
> > +		return 0;
> >   	}
> >
> >   	return -EINVAL;
> > diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
> b/board/starfive/visionfive2/starfive_visionfive2.c
> > index bfbb11a2ee7..f38433e94ac 100644
> > --- a/board/starfive/visionfive2/starfive_visionfive2.c
> > +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> > @@ -59,20 +59,12 @@ static void set_fdtfile(void)
> >   		fdtfile = "starfive/jh7110-milkv-mars.dtb";
> >   	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >   		fdtfile = "starfive/jh7110-pine64-star64.dtb";
> > -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> > -		switch (get_pcb_revision_from_eeprom()) {
> > -		case 'a':
> > -		case 'A':
> > -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> v1.2a.dtb";
> > -			break;
> > -		case 'b':
> > -		case 'B':
> > -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> v1.3b.dtb";
> > -			break;
> > -		default:
> > -			log_err("Unknown revision\n");
> > -			return;
> > -		}
> > +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
> > +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
> 
> Where boards both with 'a' and 'b' ever shipped?
> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].

You're right. There are no boards marked "VF7110a" or "VF7110b". Let's remove
the comparison of "VF7110a" and "VF7110b".

Best regards,
Hal

> 
> The change looks technically correct.
> 
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> > +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> > +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> > +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
> > +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> >   	} else {
> >   		log_err("Unknown product\n");
> >   		return;
> > diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > index 3866d07f9d4..43b8af4fc59 100644
> > --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
> >   	return 0;
> >   }
> >
> > -/**
> > - * get_pcb_revision_from_eeprom - get the PCB revision
> > - *
> > - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
> > - */
> >   u8 get_pcb_revision_from_eeprom(void)
> >   {
> > -	u8 pv = 0xFF;
> > -
> >   	if (read_eeprom())
> > -		return pv;
> > +		return 0xFF;
> >
> > -	return pbuf.eeprom.atom1.data.pstr[6];
> > +	return pbuf.eeprom.atom4.data.pcb_revision;
> >   }
> >
> >   u8 get_ddr_size_from_eeprom(void)

Re: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[E Shattow]

On 9/2/25 00:44, Hal Feng wrote:
>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
>> On 29.08.25 08:09, Hal Feng wrote:
>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
>>> Move the function description to the header file.
>>> Remove the function calls in board/starfive/visionfive2/.
>>>
>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>> ---
>>>   arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
>>>   board/starfive/visionfive2/spl.c              | 18 ++++++-----------
>>>   .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
>>>   .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
>>>   4 files changed, 19 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> index 6d0a0ba0c4a..025f1d32c49 100644
>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> @@ -9,6 +9,11 @@
>>>
>>>   #include <linux/types.h>
>>>
>>> +/**
>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
>>> + *
>>> + * @return: the PCB revision or 0xFF on error.
>>> + */

No. Inverted logic. See below comment about body of function.

>>>   u8 get_pcb_revision_from_eeprom(void);
>>>
>>>   /**
>>> diff --git a/board/starfive/visionfive2/spl.c
>> b/board/starfive/visionfive2/spl.c
>>> index 3dfa931b655..901e7b58f36 100644
>>> --- a/board/starfive/visionfive2/spl.c
>>> +++ b/board/starfive/visionfive2/spl.c
>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
>> *name)
>>>   		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>   		return 0;
>>>   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a")
>> &&
>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>> -		switch (get_pcb_revision_from_eeprom()) {
>>> -		case 'a':
>>> -		case 'A':
>>> -			return 0;
>>> -		}
>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
>> ||
>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
>> {
>>> +		return 0;
>>>   	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b")
>> &&
>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>> -		switch (get_pcb_revision_from_eeprom()) {
>>> -		case 'b':
>>> -		case 'B':
>>> -			return 0;
>>> -		}
>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
>> ||
>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
>> {
>>> +		return 0;
>>>   	}
>>>
>>>   	return -EINVAL;

I agree it is good to delete the case statement and drop lowercase
compare as suggested, also...

>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
>> b/board/starfive/visionfive2/starfive_visionfive2.c
>>> index bfbb11a2ee7..f38433e94ac 100644
>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
>>>   		fdtfile = "starfive/jh7110-milkv-mars.dtb";
>>>   	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>   		fdtfile = "starfive/jh7110-pine64-star64.dtb";
>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>> -		switch (get_pcb_revision_from_eeprom()) {
>>> -		case 'a':
>>> -		case 'A':
>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>> v1.2a.dtb";
>>> -			break;
>>> -		case 'b':
>>> -		case 'B':
>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>> v1.3b.dtb";
>>> -			break;
>>> -		default:
>>> -			log_err("Unknown revision\n");
>>> -			return;
>>> -		}
>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
>>
>> Where boards both with 'a' and 'b' ever shipped?
>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
> 
> You're right. There are no boards marked "VF7110a" or "VF7110b". Let's remove
> the comparison of "VF7110a" and "VF7110b".
> 
> Best regards,
> Hal

... again here too delete the case statement and drop lowercase compare.
Thanks.

> 
>>
>> The change looks technically correct.
>>
>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>
>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
>>>   	} else {
>>>   		log_err("Unknown product\n");
>>>   		return;

>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> index 3866d07f9d4..43b8af4fc59 100644
>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
>>>   	return 0;
>>>   }
>>>
>>> -/**
>>> - * get_pcb_revision_from_eeprom - get the PCB revision
>>> - *
>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
>>> - */
>>>   u8 get_pcb_revision_from_eeprom(void)
>>>   {
>>> -	u8 pv = 0xFF;
>>> -
>>>   	if (read_eeprom())
>>> -		return pv;
>>> +		return 0xFF;
>>>
>>> -	return pbuf.eeprom.atom1.data.pstr[6];
>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
>>>   }
>>>
>>>   u8 get_ddr_size_from_eeprom(void)
> 

No. I do not want to have to guess what error does "0xFF" have the
meaning of, it makes the code annoying to read every function header
documentation to learn this information.

Since we remove all users of mac_read_from_eeprom() it is certainly not
a problem we care about now anymore. Default PCB revision in case of
error may be zero. It is not important to make distinction of additional
failure detection, this is already resolved by read_eeprom() where any
error message may be presented to the user.

I appreciate the effort to be more technically correct and have a unique
error value but with u8 return value 0xFF it is an additional layer of
complication not needed. If you insist on this approach then the return
value should be arbitrarily defined as a compiler macro, say
CMD_EEPROM_FAIL (255) and consistently used. In fact I would not
complain if it improved code readability.

-E

Re: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Heinrich Schuchardt]

On 9/3/25 00:12, E Shattow wrote:
> 
> On 9/2/25 00:44, Hal Feng wrote:
>>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
>>> On 29.08.25 08:09, Hal Feng wrote:
>>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
>>>> Move the function description to the header file.
>>>> Remove the function calls in board/starfive/visionfive2/.
>>>>
>>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
>>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>>> ---
>>>>    arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
>>>>    board/starfive/visionfive2/spl.c              | 18 ++++++-----------
>>>>    .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
>>>>    .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
>>>>    4 files changed, 19 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>>> index 6d0a0ba0c4a..025f1d32c49 100644
>>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
>>>> @@ -9,6 +9,11 @@
>>>>
>>>>    #include <linux/types.h>
>>>>
>>>> +/**
>>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
>>>> + *
>>>> + * @return: the PCB revision or 0xFF on error.
>>>> + */
> 
> No. Inverted logic. See below comment about body of function.
> 
>>>>    u8 get_pcb_revision_from_eeprom(void);
>>>>
>>>>    /**
>>>> diff --git a/board/starfive/visionfive2/spl.c
>>> b/board/starfive/visionfive2/spl.c
>>>> index 3dfa931b655..901e7b58f36 100644
>>>> --- a/board/starfive/visionfive2/spl.c
>>>> +++ b/board/starfive/visionfive2/spl.c
>>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
>>> *name)
>>>>    		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>>    		return 0;
>>>>    	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.2a")
>>> &&
>>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>> -		switch (get_pcb_revision_from_eeprom()) {
>>>> -		case 'a':
>>>> -		case 'A':
>>>> -			return 0;
>>>> -		}
>>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
>>> ||
>>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
>>> {
>>>> +		return 0;
>>>>    	} else if (!strcmp(name, "starfive/jh7110-starfive-visionfive-2-v1.3b")
>>> &&
>>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>> -		switch (get_pcb_revision_from_eeprom()) {
>>>> -		case 'b':
>>>> -		case 'B':
>>>> -			return 0;
>>>> -		}
>>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
>>> ||
>>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
>>> {
>>>> +		return 0;
>>>>    	}
>>>>
>>>>    	return -EINVAL;
> 
> I agree it is good to delete the case statement and drop lowercase
> compare as suggested, also...
> 
>>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
>>> b/board/starfive/visionfive2/starfive_visionfive2.c
>>>> index bfbb11a2ee7..f38433e94ac 100644
>>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
>>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
>>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
>>>>    		fdtfile = "starfive/jh7110-milkv-mars.dtb";
>>>>    	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
>>>>    		fdtfile = "starfive/jh7110-pine64-star64.dtb";
>>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
>>>> -		switch (get_pcb_revision_from_eeprom()) {
>>>> -		case 'a':
>>>> -		case 'A':
>>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>>> v1.2a.dtb";
>>>> -			break;
>>>> -		case 'b':
>>>> -		case 'B':
>>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
>>> v1.3b.dtb";
>>>> -			break;
>>>> -		default:
>>>> -			log_err("Unknown revision\n");
>>>> -			return;
>>>> -		}
>>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
>>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
>>>
>>> Where boards both with 'a' and 'b' ever shipped?
>>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
>>
>> You're right. There are no boards marked "VF7110a" or "VF7110b". Let's remove
>> the comparison of "VF7110a" and "VF7110b".
>>
>> Best regards,
>> Hal
> 
> ... again here too delete the case statement and drop lowercase compare.
> Thanks.
> 
>>
>>>
>>> The change looks technically correct.
>>>
>>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>
>>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
>>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
>>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
>>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
>>>>    	} else {
>>>>    		log_err("Unknown product\n");
>>>>    		return;
> 
>>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>>> index 3866d07f9d4..43b8af4fc59 100644
>>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
>>>>    	return 0;
>>>>    }
>>>>
>>>> -/**
>>>> - * get_pcb_revision_from_eeprom - get the PCB revision
>>>> - *
>>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are illegal
>>>> - */
>>>>    u8 get_pcb_revision_from_eeprom(void)
>>>>    {
>>>> -	u8 pv = 0xFF;
>>>> -
>>>>    	if (read_eeprom())
>>>> -		return pv;
>>>> +		return 0xFF;
>>>>
>>>> -	return pbuf.eeprom.atom1.data.pstr[6];
>>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
>>>>    }
>>>>
>>>>    u8 get_ddr_size_from_eeprom(void)
>>
> 
> No. I do not want to have to guess what error does "0xFF" have the
> meaning of, it makes the code annoying to read every function header
> documentation to learn this information.
> 
> Since we remove all users of mac_read_from_eeprom() it is certainly not
> a problem we care about now anymore. Default PCB revision in case of
> error may be zero. It is not important to make distinction of additional
> failure detection, this is already resolved by read_eeprom() where any
> error message may be presented to the user.
> 
> I appreciate the effort to be more technically correct and have a unique
> error value but with u8 return value 0xFF it is an additional layer of
> complication not needed. If you insist on this approach then the return
> value should be arbitrarily defined as a compiler macro, say
> CMD_EEPROM_FAIL (255) and consistently used. In fact I would not
> complain if it improved code readability.

There is no necessity to use u8 as the return value. We could also use 
int and return a negative error code (-EIO) on failure.

Best regards

Heinrich

> 
> -E

RE: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Hal Feng]

> On 03.09.25 06:32, Heinrich Schuchardt wrote:
> On 9/3/25 00:12, E Shattow wrote:
> >
> > On 9/2/25 00:44, Hal Feng wrote:
> >>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
> >>> On 29.08.25 08:09, Hal Feng wrote:
> >>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
> >>>> Move the function description to the header file.
> >>>> Remove the function calls in board/starfive/visionfive2/.
> >>>>
> >>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM
> >>>> configuration")
> >>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >>>> ---
> >>>>    arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
> >>>>    board/starfive/visionfive2/spl.c              | 18 ++++++-----------
> >>>>    .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
> >>>>    .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
> >>>>    4 files changed, 19 insertions(+), 35 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> index 6d0a0ba0c4a..025f1d32c49 100644
> >>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> @@ -9,6 +9,11 @@
> >>>>
> >>>>    #include <linux/types.h>
> >>>>
> >>>> +/**
> >>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
> >>>> + *
> >>>> + * @return: the PCB revision or 0xFF on error.
> >>>> + */
> >
> > No. Inverted logic. See below comment about body of function.
> >
> >>>>    u8 get_pcb_revision_from_eeprom(void);
> >>>>
> >>>>    /**
> >>>> diff --git a/board/starfive/visionfive2/spl.c
> >>> b/board/starfive/visionfive2/spl.c
> >>>> index 3dfa931b655..901e7b58f36 100644
> >>>> --- a/board/starfive/visionfive2/spl.c
> >>>> +++ b/board/starfive/visionfive2/spl.c
> >>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
> >>> *name)
> >>>>    		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >>>>    		return 0;
> >>>>    	} else if (!strcmp(name,
> >>>> "starfive/jh7110-starfive-visionfive-2-v1.2a")
> >>> &&
> >>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'a':
> >>>> -		case 'A':
> >>>> -			return 0;
> >>>> -		}
> >>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
> >>> ||
> >>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
> >>> {
> >>>> +		return 0;
> >>>>    	} else if (!strcmp(name,
> >>>> "starfive/jh7110-starfive-visionfive-2-v1.3b")
> >>> &&
> >>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'b':
> >>>> -		case 'B':
> >>>> -			return 0;
> >>>> -		}
> >>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
> >>> ||
> >>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
> >>> {
> >>>> +		return 0;
> >>>>    	}
> >>>>
> >>>>    	return -EINVAL;
> >
> > I agree it is good to delete the case statement and drop lowercase
> > compare as suggested, also...
> >
> >>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
> >>> b/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> index bfbb11a2ee7..f38433e94ac 100644
> >>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
> >>>>    		fdtfile = "starfive/jh7110-milkv-mars.dtb";
> >>>>    	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >>>>    		fdtfile = "starfive/jh7110-pine64-star64.dtb";
> >>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'a':
> >>>> -		case 'A':
> >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> >>> v1.2a.dtb";
> >>>> -			break;
> >>>> -		case 'b':
> >>>> -		case 'B':
> >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> >>> v1.3b.dtb";
> >>>> -			break;
> >>>> -		default:
> >>>> -			log_err("Unknown revision\n");
> >>>> -			return;
> >>>> -		}
> >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
> >>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
> >>>
> >>> Where boards both with 'a' and 'b' ever shipped?
> >>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
> >>
> >> You're right. There are no boards marked "VF7110a" or "VF7110b".
> >> Let's remove the comparison of "VF7110a" and "VF7110b".
> >>
> >> Best regards,
> >> Hal
> >
> > ... again here too delete the case statement and drop lowercase compare.
> > Thanks.
> >
> >>
> >>>
> >>> The change looks technically correct.
> >>>
> >>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>
> >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> >>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
> >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> >>>>    	} else {
> >>>>    		log_err("Unknown product\n");
> >>>>    		return;
> >
> >>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> index 3866d07f9d4..43b8af4fc59 100644
> >>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
> >>>>    	return 0;
> >>>>    }
> >>>>
> >>>> -/**
> >>>> - * get_pcb_revision_from_eeprom - get the PCB revision
> >>>> - *
> >>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are
> >>>> illegal
> >>>> - */
> >>>>    u8 get_pcb_revision_from_eeprom(void)
> >>>>    {
> >>>> -	u8 pv = 0xFF;
> >>>> -
> >>>>    	if (read_eeprom())
> >>>> -		return pv;
> >>>> +		return 0xFF;
> >>>>
> >>>> -	return pbuf.eeprom.atom1.data.pstr[6];
> >>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
> >>>>    }
> >>>>
> >>>>    u8 get_ddr_size_from_eeprom(void)
> >>
> >
> > No. I do not want to have to guess what error does "0xFF" have the
> > meaning of, it makes the code annoying to read every function header
> > documentation to learn this information.
> >
> > Since we remove all users of mac_read_from_eeprom() it is certainly
> > not a problem we care about now anymore. Default PCB revision in case
> > of error may be zero. It is not important to make distinction of
> > additional failure detection, this is already resolved by
> > read_eeprom() where any error message may be presented to the user.
> >
> > I appreciate the effort to be more technically correct and have a
> > unique error value but with u8 return value 0xFF it is an additional
> > layer of complication not needed. If you insist on this approach then
> > the return value should be arbitrarily defined as a compiler macro,
> > say CMD_EEPROM_FAIL (255) and consistently used. In fact I would not
> > complain if it improved code readability.
> 
> There is no necessity to use u8 as the return value. We could also use int and
> return a negative error code (-EIO) on failure.

Yeah, will return a negative error code (-EIO) on failure.
I think the reason why we set the return value to 0xFF on failure was that
0xFF is the default value after EEPROM is erased.

Best regards,
Hal

RE: [RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Hal Feng]

> On 22.10.25 11:03, Hal Feng wrote:
> > On 03.09.25 06:32, Heinrich Schuchardt wrote:
> > On 9/3/25 00:12, E Shattow wrote:
> > >
> > > On 9/2/25 00:44, Hal Feng wrote:
> > >>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
> > >>> On 29.08.25 08:09, Hal Feng wrote:
> > >>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
> > >>>> Move the function description to the header file.
> > >>>> Remove the function calls in board/starfive/visionfive2/.
> > >>>>
> > >>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM
> > >>>> configuration")
> > >>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > >>>> ---
> > >>>>    arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
> > >>>>    board/starfive/visionfive2/spl.c              | 18 ++++++-----------
> > >>>>    .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
> > >>>>    .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
> > >>>>    4 files changed, 19 insertions(+), 35 deletions(-)
> > >>>>
> > >>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > >>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > >>>> index 6d0a0ba0c4a..025f1d32c49 100644
> > >>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > >>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> > >>>> @@ -9,6 +9,11 @@
> > >>>>
> > >>>>    #include <linux/types.h>
> > >>>>
> > >>>> +/**
> > >>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
> > >>>> + *
> > >>>> + * @return: the PCB revision or 0xFF on error.
> > >>>> + */
> > >
> > > No. Inverted logic. See below comment about body of function.
> > >
> > >>>>    u8 get_pcb_revision_from_eeprom(void);
> > >>>>
> > >>>>    /**
> > >>>> diff --git a/board/starfive/visionfive2/spl.c
> > >>> b/board/starfive/visionfive2/spl.c
> > >>>> index 3dfa931b655..901e7b58f36 100644
> > >>>> --- a/board/starfive/visionfive2/spl.c
> > >>>> +++ b/board/starfive/visionfive2/spl.c
> > >>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
> > >>> *name)
> > >>>>    		    !strncmp(get_product_id_from_eeprom(),
> "STAR64", 6)) {
> > >>>>    		return 0;
> > >>>>    	} else if (!strcmp(name,
> > >>>> "starfive/jh7110-starfive-visionfive-2-v1.2a")
> > >>> &&
> > >>>> -		    !strncmp(get_product_id_from_eeprom(),
> "VF7110", 6)) {
> > >>>> -		switch (get_pcb_revision_from_eeprom()) {
> > >>>> -		case 'a':
> > >>>> -		case 'A':
> > >>>> -			return 0;
> > >>>> -		}
> > >>>> +		   (!strncmp(get_product_id_from_eeprom(),
> "VF7110A", 7)
> > >>> ||
> > >>>> +		    !strncmp(get_product_id_from_eeprom(),
> "VF7110a", 7)))
> > >>> {
> > >>>> +		return 0;
> > >>>>    	} else if (!strcmp(name,
> > >>>> "starfive/jh7110-starfive-visionfive-2-v1.3b")
> > >>> &&
> > >>>> -		    !strncmp(get_product_id_from_eeprom(),
> "VF7110", 6)) {
> > >>>> -		switch (get_pcb_revision_from_eeprom()) {
> > >>>> -		case 'b':
> > >>>> -		case 'B':
> > >>>> -			return 0;
> > >>>> -		}
> > >>>> +		   (!strncmp(get_product_id_from_eeprom(),
> "VF7110B", 7)
> > >>> ||
> > >>>> +		    !strncmp(get_product_id_from_eeprom(),
> "VF7110b", 7)))
> > >>> {
> > >>>> +		return 0;
> > >>>>    	}
> > >>>>
> > >>>>    	return -EINVAL;
> > >
> > > I agree it is good to delete the case statement and drop lowercase
> > > compare as suggested, also...
> > >
> > >>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
> > >>> b/board/starfive/visionfive2/starfive_visionfive2.c
> > >>>> index bfbb11a2ee7..f38433e94ac 100644
> > >>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
> > >>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> > >>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
> > >>>>    		fdtfile = "starfive/jh7110-milkv-mars.dtb";
> > >>>>    	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64",
> 6)) {
> > >>>>    		fdtfile = "starfive/jh7110-pine64-star64.dtb";
> > >>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110",
> 6)) {
> > >>>> -		switch (get_pcb_revision_from_eeprom()) {
> > >>>> -		case 'a':
> > >>>> -		case 'A':
> > >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> > >>> v1.2a.dtb";
> > >>>> -			break;
> > >>>> -		case 'b':
> > >>>> -		case 'B':
> > >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> > >>> v1.3b.dtb";
> > >>>> -			break;
> > >>>> -		default:
> > >>>> -			log_err("Unknown revision\n");
> > >>>> -			return;
> > >>>> -		}
> > >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A",
> 7) ||
> > >>>> +		   !strncmp(get_product_id_from_eeprom(),
> "VF7110a", 7)) {
> > >>>
> > >>> Where boards both with 'a' and 'b' ever shipped?
> > >>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
> > >>
> > >> You're right. There are no boards marked "VF7110a" or "VF7110b".
> > >> Let's remove the comparison of "VF7110a" and "VF7110b".
> > >>
> > >> Best regards,
> > >> Hal
> > >
> > > ... again here too delete the case statement and drop lowercase compare.
> > > Thanks.
> > >
> > >>
> > >>>
> > >>> The change looks technically correct.
> > >>>
> > >>> Reviewed-by: Heinrich Schuchardt
> > >>> <heinrich.schuchardt@canonical.com>
> > >>>
> > >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-
> v1.2a.dtb";
> > >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B",
> 7) ||
> > >>>> +		   !strncmp(get_product_id_from_eeprom(),
> "VF7110b", 7)) {
> > >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-
> v1.3b.dtb";
> > >>>>    	} else {
> > >>>>    		log_err("Unknown product\n");
> > >>>>    		return;
> > >
> > >>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > >>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > >>>> index 3866d07f9d4..43b8af4fc59 100644
> > >>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > >>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > >>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
> > >>>>    	return 0;
> > >>>>    }
> > >>>>
> > >>>> -/**
> > >>>> - * get_pcb_revision_from_eeprom - get the PCB revision
> > >>>> - *
> > >>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are
> > >>>> illegal
> > >>>> - */
> > >>>>    u8 get_pcb_revision_from_eeprom(void)
> > >>>>    {
> > >>>> -	u8 pv = 0xFF;
> > >>>> -
> > >>>>    	if (read_eeprom())
> > >>>> -		return pv;
> > >>>> +		return 0xFF;
> > >>>>
> > >>>> -	return pbuf.eeprom.atom1.data.pstr[6];
> > >>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
> > >>>>    }
> > >>>>
> > >>>>    u8 get_ddr_size_from_eeprom(void)
> > >>
> > >
> > > No. I do not want to have to guess what error does "0xFF" have the
> > > meaning of, it makes the code annoying to read every function header
> > > documentation to learn this information.
> > >
> > > Since we remove all users of mac_read_from_eeprom() it is certainly
> > > not a problem we care about now anymore. Default PCB revision in
> > > case of error may be zero. It is not important to make distinction
> > > of additional failure detection, this is already resolved by
> > > read_eeprom() where any error message may be presented to the user.
> > >
> > > I appreciate the effort to be more technically correct and have a
> > > unique error value but with u8 return value 0xFF it is an additional
> > > layer of complication not needed. If you insist on this approach
> > > then the return value should be arbitrarily defined as a compiler
> > > macro, say CMD_EEPROM_FAIL (255) and consistently used. In fact I
> > > would not complain if it improved code readability.
> >
> > There is no necessity to use u8 as the return value. We could also use
> > int and return a negative error code (-EIO) on failure.
> 
> Yeah, will return a negative error code (-EIO) on failure.
> I think the reason why we set the return value to 0xFF on failure was that 0xFF
> is the default value after EEPROM is erased.

Fix the last line of comments. 0xFF may be the default value after DDR is inited,
but I am not sure about it.

Best regards,
Hal

[RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[Hal Feng]

Add eeprom data format v3 support.
Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify it.
Set or show eth0/1 MAC address only if the board has eth0/1.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
 1 file changed, 78 insertions(+), 13 deletions(-)

diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
index 43b8af4fc59..7352252c475 100644
--- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
+++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
@@ -11,7 +11,7 @@
 #include <u-boot/crc.h>
 #include <linux/delay.h>
 
-#define FORMAT_VERSION		0x2
+#define FORMAT_VERSION		0x3
 #define PCB_VERSION		0xB1
 #define BOM_VERSION		'A'
 /*
@@ -105,7 +105,8 @@ struct eeprom_atom4_data {
 	u8 bom_revision;		/* BOM version */
 	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
 	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
-	u8 reserved[2];
+	u8 wifi_bt;			/* WIFI/BT support flag */
+	u8 reserved;
 };
 
 struct starfive_eeprom_atom4 {
@@ -128,6 +129,35 @@ static union {
 /* Set to 1 if we've read EEPROM into memory */
 static int has_been_read __section(".data");
 
+static const char * const board_no_eth0_list[] = {
+	"FML13V01",
+};
+
+static const char * const board_no_eth1_list[] = {
+	"FML13V01",
+	"MARS",
+	"VF7110SL",
+};
+
+static bool board_have_eth(u8 eth)
+{
+	int i;
+
+	if (eth == 0) {
+		for (i = 0 ; i < ARRAY_SIZE(board_no_eth0_list); i++)
+			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth0_list[i],
+				     strlen(board_no_eth0_list[i])))
+				return false;
+	} else {
+		for (i = 0 ; i < ARRAY_SIZE(board_no_eth1_list); i++)
+			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth1_list[i],
+				     strlen(board_no_eth1_list[i])))
+				return false;
+	}
+
+	return true;
+}
+
 static inline int is_match_magic(void)
 {
 	return strncmp(pbuf.eeprom.header.signature, STARFIVE_EEPROM_HATS_SIG,
@@ -176,17 +206,22 @@ static void show_eeprom(void)
 	printf("Vendor : %s\n", pbuf.eeprom.atom1.data.vstr);
 	printf("Product full SN: %s\n", pbuf.eeprom.atom1.data.pstr);
 	printf("data version: 0x%x\n", pbuf.eeprom.atom4.data.version);
-	if (pbuf.eeprom.atom4.data.version == 2) {
+	if (pbuf.eeprom.atom4.data.version >= 2 && pbuf.eeprom.atom4.data.version <= 3) {
 		printf("PCB revision: 0x%x\n", pbuf.eeprom.atom4.data.pcb_revision);
 		printf("BOM revision: %c\n", pbuf.eeprom.atom4.data.bom_revision);
-		printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
-		       pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
-		       pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
-		       pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
-		printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
-		       pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
-		       pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
-		       pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
+		if (board_have_eth(0))
+			printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
+			pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
+			pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
+			pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
+
+		if (board_have_eth(1))
+			printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
+			pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
+			pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
+			pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
+		if (pbuf.eeprom.atom4.data.version >= 3)
+			printf("WIFI/BT support: %x\n", pbuf.eeprom.atom4.data.wifi_bt);
 	} else {
 		printf("Custom data v%d is not Supported\n", pbuf.eeprom.atom4.data.version);
 		dump_raw_eeprom();
@@ -260,6 +295,7 @@ static void init_local_copy(void)
 	pbuf.eeprom.atom4.data.bom_revision = BOM_VERSION;
 	set_mac_address(STARFIVE_DEFAULT_MAC0, 0);
 	set_mac_address(STARFIVE_DEFAULT_MAC1, 1);
+	pbuf.eeprom.atom4.data.wifi_bt = 0;
 }
 
 /**
@@ -385,6 +421,28 @@ static void set_bom_revision(char *string)
 	update_crc();
 }
 
+/**
+ * set_wifi_bt() - stores a StarFive WIFI/BT support flag into the local EEPROM copy
+ *
+ * Takes a pointer to a string representing the numeric WIFI/BT support flag in
+ * decimal ("0" or "1"), stores it in the wifi_bt field of the
+ * EEPROM local copy, and updates the CRC of the local copy.
+ */
+static void set_wifi_bt(char *string)
+{
+	u8 wifi_bt;
+
+	wifi_bt = simple_strtoul(string, &string, 16);
+	if (wifi_bt > 1) {
+		printf("WIFI/BT support flag must not be greater than 1");
+		return;
+	}
+
+	pbuf.eeprom.atom4.data.wifi_bt = wifi_bt;
+
+	update_crc();
+}
+
 /**
  * set_product_id() - stores a StarFive product ID into the local EEPROM copy
  *
@@ -478,6 +536,9 @@ int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	} else if (!strcmp(cmd, "bom_revision")) {
 		set_bom_revision(argv[2]);
 		return 0;
+	} else if (!strcmp(cmd, "wifi_bt")) {
+		set_wifi_bt(argv[2]);
+		return 0;
 	} else if (!strcmp(cmd, "product_id")) {
 		set_product_id(argv[2]);
 		return 0;
@@ -513,8 +574,10 @@ int mac_read_from_eeprom(void)
 	}
 
 	// 1, setup ethaddr env
-	eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
-	eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
+	if (board_have_eth(0))
+		eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
+	if (board_have_eth(1))
+		eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
 
 	/**
 	 * 2, setup serial# env, reference to hifive-platform-i2c-eeprom.c,
@@ -585,6 +648,8 @@ U_BOOT_LONGHELP(mac,
 	"    - stores a StarFive PCB revision into the local EEPROM copy\n"
 	"mac bom_revision <A>\n"
 	"    - stores a StarFive BOM revision into the local EEPROM copy\n"
+	"mac wifi_bt <?>\n"
+	"    - stores a StarFive WIFI/BT support flag into the local EEPROM copy\n"
 	"mac product_id <VF7110A1-2228-D008E000-xxxxxxxx>\n"
 	"    - stores a StarFive product ID into the local EEPROM copy\n"
 	"mac vendor <Vendor Name>\n"
-- 
2.43.2

Re: [RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[Heinrich Schuchardt]

On 29.08.25 08:09, Hal Feng wrote:
> Add eeprom data format v3 support.
> Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify it.
> Set or show eth0/1 MAC address only if the board has eth0/1.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>   .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
>   1 file changed, 78 insertions(+), 13 deletions(-)
> 
> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> index 43b8af4fc59..7352252c475 100644
> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> @@ -11,7 +11,7 @@
>   #include <u-boot/crc.h>
>   #include <linux/delay.h>
>   
> -#define FORMAT_VERSION		0x2
> +#define FORMAT_VERSION		0x3
>   #define PCB_VERSION		0xB1
>   #define BOM_VERSION		'A'
>   /*
> @@ -105,7 +105,8 @@ struct eeprom_atom4_data {
>   	u8 bom_revision;		/* BOM version */
>   	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
>   	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
> -	u8 reserved[2];
> +	u8 wifi_bt;			/* WIFI/BT support flag */
> +	u8 reserved;
>   };
>   
>   struct starfive_eeprom_atom4 {
> @@ -128,6 +129,35 @@ static union {
>   /* Set to 1 if we've read EEPROM into memory */
>   static int has_been_read __section(".data");
>   
> +static const char * const board_no_eth0_list[] = {
> +	"FML13V01",
> +};
> +
> +static const char * const board_no_eth1_list[] = {
> +	"FML13V01",
> +	"MARS",
> +	"VF7110SL",
> +};

Do you know in which category the Milk-V Mars CM ("MARC") falls?

Best regards

Heinrich

> +
> +static bool board_have_eth(u8 eth)
> +{
> +	int i;
> +
> +	if (eth == 0) {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth0_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth0_list[i],
> +				     strlen(board_no_eth0_list[i])))
> +				return false;
> +	} else {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth1_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth1_list[i],
> +				     strlen(board_no_eth1_list[i])))
> +				return false;
> +	}
> +
> +	return true;
> +}
> +
>   static inline int is_match_magic(void)
>   {
>   	return strncmp(pbuf.eeprom.header.signature, STARFIVE_EEPROM_HATS_SIG,
> @@ -176,17 +206,22 @@ static void show_eeprom(void)
>   	printf("Vendor : %s\n", pbuf.eeprom.atom1.data.vstr);
>   	printf("Product full SN: %s\n", pbuf.eeprom.atom1.data.pstr);
>   	printf("data version: 0x%x\n", pbuf.eeprom.atom4.data.version);
> -	if (pbuf.eeprom.atom4.data.version == 2) {
> +	if (pbuf.eeprom.atom4.data.version >= 2 && pbuf.eeprom.atom4.data.version <= 3) {
>   		printf("PCB revision: 0x%x\n", pbuf.eeprom.atom4.data.pcb_revision);
>   		printf("BOM revision: %c\n", pbuf.eeprom.atom4.data.bom_revision);
> -		printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> -		       pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> -		       pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> -		printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> -		       pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> -		       pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (board_have_eth(0))
> +			printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> +			pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> +			pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> +
> +		if (board_have_eth(1))
> +			printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> +			pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> +			pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (pbuf.eeprom.atom4.data.version >= 3)
> +			printf("WIFI/BT support: %x\n", pbuf.eeprom.atom4.data.wifi_bt);
>   	} else {
>   		printf("Custom data v%d is not Supported\n", pbuf.eeprom.atom4.data.version);
>   		dump_raw_eeprom();
> @@ -260,6 +295,7 @@ static void init_local_copy(void)
>   	pbuf.eeprom.atom4.data.bom_revision = BOM_VERSION;
>   	set_mac_address(STARFIVE_DEFAULT_MAC0, 0);
>   	set_mac_address(STARFIVE_DEFAULT_MAC1, 1);
> +	pbuf.eeprom.atom4.data.wifi_bt = 0;
>   }
>   
>   /**
> @@ -385,6 +421,28 @@ static void set_bom_revision(char *string)
>   	update_crc();
>   }
>   
> +/**
> + * set_wifi_bt() - stores a StarFive WIFI/BT support flag into the local EEPROM copy
> + *
> + * Takes a pointer to a string representing the numeric WIFI/BT support flag in
> + * decimal ("0" or "1"), stores it in the wifi_bt field of the
> + * EEPROM local copy, and updates the CRC of the local copy.
> + */
> +static void set_wifi_bt(char *string)
> +{
> +	u8 wifi_bt;
> +
> +	wifi_bt = simple_strtoul(string, &string, 16);
> +	if (wifi_bt > 1) {
> +		printf("WIFI/BT support flag must not be greater than 1");
> +		return;
> +	}
> +
> +	pbuf.eeprom.atom4.data.wifi_bt = wifi_bt;
> +
> +	update_crc();
> +}
> +
>   /**
>    * set_product_id() - stores a StarFive product ID into the local EEPROM copy
>    *
> @@ -478,6 +536,9 @@ int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	} else if (!strcmp(cmd, "bom_revision")) {
>   		set_bom_revision(argv[2]);
>   		return 0;
> +	} else if (!strcmp(cmd, "wifi_bt")) {
> +		set_wifi_bt(argv[2]);
> +		return 0;
>   	} else if (!strcmp(cmd, "product_id")) {
>   		set_product_id(argv[2]);
>   		return 0;
> @@ -513,8 +574,10 @@ int mac_read_from_eeprom(void)
>   	}
>   
>   	// 1, setup ethaddr env
> -	eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> -	eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
> +	if (board_have_eth(0))
> +		eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> +	if (board_have_eth(1))
> +		eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
>   
>   	/**
>   	 * 2, setup serial# env, reference to hifive-platform-i2c-eeprom.c,
> @@ -585,6 +648,8 @@ U_BOOT_LONGHELP(mac,
>   	"    - stores a StarFive PCB revision into the local EEPROM copy\n"
>   	"mac bom_revision <A>\n"
>   	"    - stores a StarFive BOM revision into the local EEPROM copy\n"
> +	"mac wifi_bt <?>\n"
> +	"    - stores a StarFive WIFI/BT support flag into the local EEPROM copy\n"
>   	"mac product_id <VF7110A1-2228-D008E000-xxxxxxxx>\n"
>   	"    - stores a StarFive product ID into the local EEPROM copy\n"
>   	"mac vendor <Vendor Name>\n"

RE: [RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[Hal Feng]

> On 29.08.25 15:47, Heinrich Schuchardt wrote:
> On 29.08.25 08:09, Hal Feng wrote:
> > Add eeprom data format v3 support.
> > Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify
> it.
> > Set or show eth0/1 MAC address only if the board has eth0/1.
> >
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >   .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
> >   1 file changed, 78 insertions(+), 13 deletions(-)
> >
> > diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > index 43b8af4fc59..7352252c475 100644
> > --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > @@ -11,7 +11,7 @@
> >   #include <u-boot/crc.h>
> >   #include <linux/delay.h>
> >
> > -#define FORMAT_VERSION		0x2
> > +#define FORMAT_VERSION		0x3
> >   #define PCB_VERSION		0xB1
> >   #define BOM_VERSION		'A'
> >   /*
> > @@ -105,7 +105,8 @@ struct eeprom_atom4_data {
> >   	u8 bom_revision;		/* BOM version */
> >   	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
> >   	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
> > -	u8 reserved[2];
> > +	u8 wifi_bt;			/* WIFI/BT support flag */
> > +	u8 reserved;
> >   };
> >
> >   struct starfive_eeprom_atom4 {
> > @@ -128,6 +129,35 @@ static union {
> >   /* Set to 1 if we've read EEPROM into memory */
> >   static int has_been_read __section(".data");
> >
> > +static const char * const board_no_eth0_list[] = {
> > +	"FML13V01",
> > +};
> > +
> > +static const char * const board_no_eth1_list[] = {
> > +	"FML13V01",
> > +	"MARS",
> > +	"VF7110SL",
> > +};
> 
> Do you know in which category the Milk-V Mars CM ("MARC") falls?

Milk-V Mars CM board has eth0 but no eth1.
Will add "MARC" in board_no_eth1_list. Thanks.

Best regards,
Hal

Re: [RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> Add eeprom data format v3 support.
> Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify it.
> Set or show eth0/1 MAC address only if the board has eth0/1.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
>  1 file changed, 78 insertions(+), 13 deletions(-)
> 
> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> index 43b8af4fc59..7352252c475 100644
> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> @@ -11,7 +11,7 @@
>  #include <u-boot/crc.h>
>  #include <linux/delay.h>
>  
> -#define FORMAT_VERSION		0x2
> +#define FORMAT_VERSION		0x3

No. This stays FORMAT_VERSION 0x2 for compatibility, allow to write
EEPROM in the "old" format on boards. We must allow compatible action
for vendor BSP firmware and weird old software that we do not want
responsibility for updating.

Update to 0x3 only if some data field is expressed that needs 0x3 format.

>  #define PCB_VERSION		0xB1
>  #define BOM_VERSION		'A'
>  /*
> @@ -105,7 +105,8 @@ struct eeprom_atom4_data {
>  	u8 bom_revision;		/* BOM version */
>  	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
>  	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
> -	u8 reserved[2];
> +	u8 wifi_bt;			/* WIFI/BT support flag */

Are you sure a whole u8 for a one-bit flag?

> +	u8 reserved;
>  };
>  
>  struct starfive_eeprom_atom4 {
> @@ -128,6 +129,35 @@ static union {
>  /* Set to 1 if we've read EEPROM into memory */
>  static int has_been_read __section(".data");
>  
> +static const char * const board_no_eth0_list[] = {
> +	"FML13V01",
> +};
> +
> +static const char * const board_no_eth1_list[] = {
> +	"FML13V01",
> +	"MARS",
> +	"VF7110SL",
> +};
> +
> +static bool board_have_eth(u8 eth)
> +{
> +	int i;
> +
> +	if (eth == 0) {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth0_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth0_list[i],
> +				     strlen(board_no_eth0_list[i])))
> +				return false;
> +	} else {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth1_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth1_list[i],
> +				     strlen(board_no_eth1_list[i])))
> +				return false;
> +	}
> +
> +	return true;
> +}
> +

No. Drop all this no_eth0 no_eth1 configuration, the string arrays, it
exists only to show or hide some EEPROM configuration text and can be
deleted.

>  static inline int is_match_magic(void)
>  {
>  	return strncmp(pbuf.eeprom.header.signature, STARFIVE_EEPROM_HATS_SIG,
> @@ -176,17 +206,22 @@ static void show_eeprom(void)
>  	printf("Vendor : %s\n", pbuf.eeprom.atom1.data.vstr);
>  	printf("Product full SN: %s\n", pbuf.eeprom.atom1.data.pstr);
>  	printf("data version: 0x%x\n", pbuf.eeprom.atom4.data.version);
> -	if (pbuf.eeprom.atom4.data.version == 2) {

> +	if (pbuf.eeprom.atom4.data.version >= 2 && pbuf.eeprom.atom4.data.version <= 3) {

This is not some floating point value. Compare with discrete values:

+ if (pbuf.eeprom.atom4.data.version == 2 ||
+     pbuf.eeprom.atom4.data.version == 3) {

or else I think more readable is convert to a switch case conditional:

+ switch (pbuf.eeprom.atom4.data.version) {
+ case 2:
+ case 3:
		...

>  		printf("PCB revision: 0x%x\n", pbuf.eeprom.atom4.data.pcb_revision);
>  		printf("BOM revision: %c\n", pbuf.eeprom.atom4.data.bom_revision);
> -		printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> -		       pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> -		       pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> -		printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> -		       pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> -		       pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (board_have_eth(0))
> +			printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> +			pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> +			pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> +
> +		if (board_have_eth(1))

No. There is not any purpose to hide this data from the user. In fact it
is useful when Mars CM Lite SoM installed to carrier board that has its
own additional network interface on PCIe bus, setting eth2addr
environment variable to be the same as (disabled) MAC1 from EEPROM print
output.

> +			printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> +			pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> +			pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (pbuf.eeprom.atom4.data.version >= 3)
> +			printf("WIFI/BT support: %x\n", pbuf.eeprom.atom4.data.wifi_bt);
+break;

+ default:

>  	} else {
>  		printf("Custom data v%d is not Supported\n", pbuf.eeprom.atom4.data.version);
>  		dump_raw_eeprom();
> @@ -260,6 +295,7 @@ static void init_local_copy(void)
>  	pbuf.eeprom.atom4.data.bom_revision = BOM_VERSION;
>  	set_mac_address(STARFIVE_DEFAULT_MAC0, 0);
>  	set_mac_address(STARFIVE_DEFAULT_MAC1, 1);
> +	pbuf.eeprom.atom4.data.wifi_bt = 0;
+ break;
>  }
>  
>  /**
> @@ -385,6 +421,28 @@ static void set_bom_revision(char *string)
>  	update_crc();
>  }
>  
> +/**
> + * set_wifi_bt() - stores a StarFive WIFI/BT support flag into the local EEPROM copy
> + *
> + * Takes a pointer to a string representing the numeric WIFI/BT support flag in
> + * decimal ("0" or "1"), stores it in the wifi_bt field of the
> + * EEPROM local copy, and updates the CRC of the local copy.
> + */
> +static void set_wifi_bt(char *string)
> +{
> +	u8 wifi_bt;
> +
> +	wifi_bt = simple_strtoul(string, &string, 16);
> +	if (wifi_bt > 1) {
> +		printf("WIFI/BT support flag must not be greater than 1");
> +		return;
> +	}
> +
> +	pbuf.eeprom.atom4.data.wifi_bt = wifi_bt;
> +
> +	update_crc();
> +}
> +

I am surprised this wifi_bt u8 is a 1-bit flag and does not contain any
mac address. What is the SDIO module present on VisionFive2 Lite and
does it contain memory for WiFi MAC address and Bluetooth MAC address?

>  /**
>   * set_product_id() - stores a StarFive product ID into the local EEPROM copy
>   *
> @@ -478,6 +536,9 @@ int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	} else if (!strcmp(cmd, "bom_revision")) {
>  		set_bom_revision(argv[2]);
>  		return 0;
> +	} else if (!strcmp(cmd, "wifi_bt")) {
> +		set_wifi_bt(argv[2]);
> +		return 0;
>  	} else if (!strcmp(cmd, "product_id")) {
>  		set_product_id(argv[2]);
>  		return 0;
> @@ -513,8 +574,10 @@ int mac_read_from_eeprom(void)
>  	}
>  
>  	// 1, setup ethaddr env
> -	eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> -	eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
> +	if (board_have_eth(0))
> +		eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> +	if (board_have_eth(1))
> +		eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
>  

No. Unnecessary complication to have a list of products and do all this
extra work. We should only update fields that the user has set.

>  	/**
>  	 * 2, setup serial# env, reference to hifive-platform-i2c-eeprom.c,
> @@ -585,6 +648,8 @@ U_BOOT_LONGHELP(mac,
>  	"    - stores a StarFive PCB revision into the local EEPROM copy\n"
>  	"mac bom_revision <A>\n"
>  	"    - stores a StarFive BOM revision into the local EEPROM copy\n"
> +	"mac wifi_bt <?>\n"
> +	"    - stores a StarFive WIFI/BT support flag into the local EEPROM copy\n"
>  	"mac product_id <VF7110A1-2228-D008E000-xxxxxxxx>\n"
>  	"    - stores a StarFive product ID into the local EEPROM copy\n"
>  	"mac vendor <Vendor Name>\n"

NAK to this patch.

Add the support for the new EEPROM format and keep it simple:

- It is okay if the user sees print of all valid fields of EEPROM, even
when some printed data does not have any importance for them. The format
version will hide or show some data fields - okay.

- For compatibility keep FORMAT_VERSION (0x2) default and each field
being set should do a check with pbuf.eeprom.header.version ; for
example if there is wifi_bt being set then it is appropriate to promote
format version (0x3) and print info that the format version has changed.

- For explicit write operation to EEPROM of format version 0x3 but user
did not set any data fields that need more than format version (0x2),
then the user must set format version to 0x3. Make it possible to set
the format version directly.

If you want to get complicated and add conditional for promotion of
minimum format version required by each data field, okay then, I would
like to see that.

-E

RE: [RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[Hal Feng]

> On 03.09.25 07:30, E Shattow wrote:
> On 8/28/25 23:09, Hal Feng wrote:
> > Add eeprom data format v3 support.
> > Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify
> it.
> > Set or show eth0/1 MAC address only if the board has eth0/1.
> >
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
> >  1 file changed, 78 insertions(+), 13 deletions(-)
> >
> > diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > index 43b8af4fc59..7352252c475 100644
> > --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> > @@ -11,7 +11,7 @@
> >  #include <u-boot/crc.h>
> >  #include <linux/delay.h>
> >
> > -#define FORMAT_VERSION		0x2
> > +#define FORMAT_VERSION		0x3
> 
> No. This stays FORMAT_VERSION 0x2 for compatibility, allow to write EEPROM
> in the "old" format on boards. We must allow compatible action for vendor
> BSP firmware and weird old software that we do not want responsibility for
> updating.
> 
> Update to 0x3 only if some data field is expressed that needs 0x3 format.

Agreed. Keep FORMAT_VERSION 0x2.

> 
> >  #define PCB_VERSION		0xB1
> >  #define BOM_VERSION		'A'
> >  /*
> > @@ -105,7 +105,8 @@ struct eeprom_atom4_data {
> >  	u8 bom_revision;		/* BOM version */
> >  	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
> >  	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
> > -	u8 reserved[2];
> > +	u8 wifi_bt;			/* WIFI/BT support flag */
> 
> Are you sure a whole u8 for a one-bit flag?

Use bit 0 to mark the WIFI/BT support flag. Reserve the other 7 bits.

> 
> > +	u8 reserved;
> >  };
> >
> >  struct starfive_eeprom_atom4 {
> > @@ -128,6 +129,35 @@ static union {
> >  /* Set to 1 if we've read EEPROM into memory */  static int
> > has_been_read __section(".data");
> >
> > +static const char * const board_no_eth0_list[] = {
> > +	"FML13V01",
> > +};
> > +
> > +static const char * const board_no_eth1_list[] = {
> > +	"FML13V01",
> > +	"MARS",
> > +	"VF7110SL",
> > +};
> > +
> > +static bool board_have_eth(u8 eth)
> > +{
> > +	int i;
> > +
> > +	if (eth == 0) {
> > +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth0_list); i++)
> > +			if (!strncmp(pbuf.eeprom.atom1.data.pstr,
> board_no_eth0_list[i],
> > +				     strlen(board_no_eth0_list[i])))
> > +				return false;
> > +	} else {
> > +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth1_list); i++)
> > +			if (!strncmp(pbuf.eeprom.atom1.data.pstr,
> board_no_eth1_list[i],
> > +				     strlen(board_no_eth1_list[i])))
> > +				return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> 
> No. Drop all this no_eth0 no_eth1 configuration, the string arrays, it exists
> only to show or hide some EEPROM configuration text and can be deleted.

Agreed.

> 
> >  static inline int is_match_magic(void)  {
> >  	return strncmp(pbuf.eeprom.header.signature,
> > STARFIVE_EEPROM_HATS_SIG, @@ -176,17 +206,22 @@ static void
> show_eeprom(void)
> >  	printf("Vendor : %s\n", pbuf.eeprom.atom1.data.vstr);
> >  	printf("Product full SN: %s\n", pbuf.eeprom.atom1.data.pstr);
> >  	printf("data version: 0x%x\n", pbuf.eeprom.atom4.data.version);
> > -	if (pbuf.eeprom.atom4.data.version == 2) {
> 
> > +	if (pbuf.eeprom.atom4.data.version >= 2 &&
> > +pbuf.eeprom.atom4.data.version <= 3) {
> 
> This is not some floating point value. Compare with discrete values:
> 
> + if (pbuf.eeprom.atom4.data.version == 2 ||
> +     pbuf.eeprom.atom4.data.version == 3) {

I prefer to change like this.

> 
> or else I think more readable is convert to a switch case conditional:
> 
> + switch (pbuf.eeprom.atom4.data.version) { case 2:
> + case 3:
> 		...
> 
> >  		printf("PCB revision: 0x%x\n",
> pbuf.eeprom.atom4.data.pcb_revision);
> >  		printf("BOM revision: %c\n",
> pbuf.eeprom.atom4.data.bom_revision);
> > -		printf("Ethernet MAC0
> address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> > -		       pbuf.eeprom.atom4.data.mac0_addr[0],
> pbuf.eeprom.atom4.data.mac0_addr[1],
> > -		       pbuf.eeprom.atom4.data.mac0_addr[2],
> pbuf.eeprom.atom4.data.mac0_addr[3],
> > -		       pbuf.eeprom.atom4.data.mac0_addr[4],
> pbuf.eeprom.atom4.data.mac0_addr[5]);
> > -		printf("Ethernet MAC1
> address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> > -		       pbuf.eeprom.atom4.data.mac1_addr[0],
> pbuf.eeprom.atom4.data.mac1_addr[1],
> > -		       pbuf.eeprom.atom4.data.mac1_addr[2],
> pbuf.eeprom.atom4.data.mac1_addr[3],
> > -		       pbuf.eeprom.atom4.data.mac1_addr[4],
> pbuf.eeprom.atom4.data.mac1_addr[5]);
> > +		if (board_have_eth(0))
> > +			printf("Ethernet MAC0
> address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> > +			pbuf.eeprom.atom4.data.mac0_addr[0],
> pbuf.eeprom.atom4.data.mac0_addr[1],
> > +			pbuf.eeprom.atom4.data.mac0_addr[2],
> pbuf.eeprom.atom4.data.mac0_addr[3],
> > +			pbuf.eeprom.atom4.data.mac0_addr[4],
> > +pbuf.eeprom.atom4.data.mac0_addr[5]);
> > +
> > +		if (board_have_eth(1))
> 
> No. There is not any purpose to hide this data from the user. In fact it is useful
> when Mars CM Lite SoM installed to carrier board that has its own additional
> network interface on PCIe bus, setting eth2addr environment variable to be
> the same as (disabled) MAC1 from EEPROM print output.
> 
> > +			printf("Ethernet MAC1
> address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> > +			pbuf.eeprom.atom4.data.mac1_addr[0],
> pbuf.eeprom.atom4.data.mac1_addr[1],
> > +			pbuf.eeprom.atom4.data.mac1_addr[2],
> pbuf.eeprom.atom4.data.mac1_addr[3],
> > +			pbuf.eeprom.atom4.data.mac1_addr[4],
> pbuf.eeprom.atom4.data.mac1_addr[5]);
> > +		if (pbuf.eeprom.atom4.data.version >= 3)
> > +			printf("WIFI/BT support: %x\n",
> pbuf.eeprom.atom4.data.wifi_bt);
> +break;
> 
> + default:
> 
> >  	} else {
> >  		printf("Custom data v%d is not Supported\n",
> pbuf.eeprom.atom4.data.version);
> >  		dump_raw_eeprom();
> > @@ -260,6 +295,7 @@ static void init_local_copy(void)
> >  	pbuf.eeprom.atom4.data.bom_revision = BOM_VERSION;
> >  	set_mac_address(STARFIVE_DEFAULT_MAC0, 0);
> >  	set_mac_address(STARFIVE_DEFAULT_MAC1, 1);
> > +	pbuf.eeprom.atom4.data.wifi_bt = 0;
> + break;
> >  }
> >
> >  /**
> > @@ -385,6 +421,28 @@ static void set_bom_revision(char *string)
> >  	update_crc();
> >  }
> >
> > +/**
> > + * set_wifi_bt() - stores a StarFive WIFI/BT support flag into the
> > +local EEPROM copy
> > + *
> > + * Takes a pointer to a string representing the numeric WIFI/BT
> > +support flag in
> > + * decimal ("0" or "1"), stores it in the wifi_bt field of the
> > + * EEPROM local copy, and updates the CRC of the local copy.
> > + */
> > +static void set_wifi_bt(char *string) {
> > +	u8 wifi_bt;
> > +
> > +	wifi_bt = simple_strtoul(string, &string, 16);
> > +	if (wifi_bt > 1) {
> > +		printf("WIFI/BT support flag must not be greater than 1");
> > +		return;
> > +	}
> > +
> > +	pbuf.eeprom.atom4.data.wifi_bt = wifi_bt;
> > +
> > +	update_crc();
> > +}
> > +
> 
> I am surprised this wifi_bt u8 is a 1-bit flag and does not contain any mac
> address. What is the SDIO module present on VisionFive2 Lite and does it
> contain memory for WiFi MAC address and Bluetooth MAC address?

Yeah, it's just a 1-bit flag.
In VF2 Lite,
mmc0 is used for SD card or EMMC.
mmc1 is used for WIFI & Bluetooth module.

> 
> >  /**
> >   * set_product_id() - stores a StarFive product ID into the local EEPROM
> copy
> >   *
> > @@ -478,6 +536,9 @@ int do_mac(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
> >  	} else if (!strcmp(cmd, "bom_revision")) {
> >  		set_bom_revision(argv[2]);
> >  		return 0;
> > +	} else if (!strcmp(cmd, "wifi_bt")) {
> > +		set_wifi_bt(argv[2]);
> > +		return 0;
> >  	} else if (!strcmp(cmd, "product_id")) {
> >  		set_product_id(argv[2]);
> >  		return 0;
> > @@ -513,8 +574,10 @@ int mac_read_from_eeprom(void)
> >  	}
> >
> >  	// 1, setup ethaddr env
> > -	eth_env_set_enetaddr("ethaddr",
> pbuf.eeprom.atom4.data.mac0_addr);
> > -	eth_env_set_enetaddr("eth1addr",
> pbuf.eeprom.atom4.data.mac1_addr);
> > +	if (board_have_eth(0))
> > +		eth_env_set_enetaddr("ethaddr",
> pbuf.eeprom.atom4.data.mac0_addr);
> > +	if (board_have_eth(1))
> > +		eth_env_set_enetaddr("eth1addr",
> pbuf.eeprom.atom4.data.mac1_addr);
> >
> 
> No. Unnecessary complication to have a list of products and do all this extra
> work. We should only update fields that the user has set.

Will drop.

> 
> >  	/**
> >  	 * 2, setup serial# env, reference to hifive-platform-i2c-eeprom.c,
> > @@ -585,6 +648,8 @@ U_BOOT_LONGHELP(mac,
> >  	"    - stores a StarFive PCB revision into the local EEPROM copy\n"
> >  	"mac bom_revision <A>\n"
> >  	"    - stores a StarFive BOM revision into the local EEPROM copy\n"
> > +	"mac wifi_bt <?>\n"
> > +	"    - stores a StarFive WIFI/BT support flag into the local EEPROM
> copy\n"
> >  	"mac product_id <VF7110A1-2228-D008E000-xxxxxxxx>\n"
> >  	"    - stores a StarFive product ID into the local EEPROM copy\n"
> >  	"mac vendor <Vendor Name>\n"
> 
> NAK to this patch.
> 
> Add the support for the new EEPROM format and keep it simple:
> 
> - It is okay if the user sees print of all valid fields of EEPROM, even when some
> printed data does not have any importance for them. The format version will
> hide or show some data fields - okay.
> 
> - For compatibility keep FORMAT_VERSION (0x2) default and each field being
> set should do a check with pbuf.eeprom.header.version ; for example if there
> is wifi_bt being set then it is appropriate to promote format version (0x3) and
> print info that the format version has changed.
> 
> - For explicit write operation to EEPROM of format version 0x3 but user did
> not set any data fields that need more than format version (0x2), then the
> user must set format version to 0x3. Make it possible to set the format version
> directly.
> 
> If you want to get complicated and add conditional for promotion of
> minimum format version required by each data field, okay then, I would like to
> see that.

Agreed with most of your suggestions. Thanks for your review.

Best regards,
Hal

[RFC 06/10] pcie: starfive: Add a optional power gpio support

[Hal Feng]

Get a optional power gpio and enable it if it is valid.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 drivers/pci/pcie_starfive_jh7110.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pcie_starfive_jh7110.c b/drivers/pci/pcie_starfive_jh7110.c
index 51aca7359ff..f6b50d12a90 100644
--- a/drivers/pci/pcie_starfive_jh7110.c
+++ b/drivers/pci/pcie_starfive_jh7110.c
@@ -45,6 +45,7 @@ struct starfive_pcie {
 	struct pcie_plda plda;
 	struct clk_bulk	clks;
 	struct reset_ctl_bulk	rsts;
+	struct gpio_desc	power_gpio;
 	struct gpio_desc	reset_gpio;
 	struct regmap *regmap;
 	unsigned int stg_pcie_base;
@@ -184,6 +185,10 @@ static int starfive_pcie_parse_dt(struct udevice *dev)
 		dev_err(dev, "reset-gpio is not valid\n");
 		return -EINVAL;
 	}
+
+	gpio_request_by_name(dev, "enable-gpios", 0, &priv->power_gpio,
+			     GPIOD_IS_OUT);
+
 	return 0;
 }
 
@@ -205,6 +210,9 @@ static int starfive_pcie_init_port(struct udevice *dev)
 		goto err_deassert_clk;
 	}
 
+	if (dm_gpio_is_valid(&priv->power_gpio))
+		dm_gpio_set_value(&priv->power_gpio, 1);
+
 	dm_gpio_set_value(&priv->reset_gpio, 1);
 	/* Disable physical functions except #0 */
 	for (i = 1; i < PLDA_FUNC_NUM; i++) {
-- 
2.43.2

Re: [RFC 06/10] pcie: starfive: Add a optional power gpio support

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> Get a optional power gpio and enable it if it is valid.
> 

I see this appears to be a normal thing that needs implemented in U-Boot
to have parity with Linux kernel, but the commit message should help me
understand why so I don't have to think so hard.

Expand why this is useful and how this is implemented in Linux kernel,
what is the API reference or something to give reason why U-Boot can
expect the devicetree property to be present from upstream
devicetree-rebasing.

> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  drivers/pci/pcie_starfive_jh7110.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/pcie_starfive_jh7110.c b/drivers/pci/pcie_starfive_jh7110.c
> index 51aca7359ff..f6b50d12a90 100644
> --- a/drivers/pci/pcie_starfive_jh7110.c
> +++ b/drivers/pci/pcie_starfive_jh7110.c
> @@ -45,6 +45,7 @@ struct starfive_pcie {
>  	struct pcie_plda plda;
>  	struct clk_bulk	clks;
>  	struct reset_ctl_bulk	rsts;
> +	struct gpio_desc	power_gpio;
>  	struct gpio_desc	reset_gpio;
>  	struct regmap *regmap;
>  	unsigned int stg_pcie_base;
> @@ -184,6 +185,10 @@ static int starfive_pcie_parse_dt(struct udevice *dev)
>  		dev_err(dev, "reset-gpio is not valid\n");
>  		return -EINVAL;
>  	}
> +
> +	gpio_request_by_name(dev, "enable-gpios", 0, &priv->power_gpio,
> +			     GPIOD_IS_OUT);
> +
>  	return 0;
>  }
>  
> @@ -205,6 +210,9 @@ static int starfive_pcie_init_port(struct udevice *dev)
>  		goto err_deassert_clk;
>  	}
>  
> +	if (dm_gpio_is_valid(&priv->power_gpio))
> +		dm_gpio_set_value(&priv->power_gpio, 1);
> +
>  	dm_gpio_set_value(&priv->reset_gpio, 1);
>  	/* Disable physical functions except #0 */
>  	for (i = 1; i < PLDA_FUNC_NUM; i++) {

-E

RE: [RFC 06/10] pcie: starfive: Add a optional power gpio support

[Hal Feng]

> On 03.09.25 07:48, E Shattow wrote:
> On 8/28/25 23:09, Hal Feng wrote:
> > Get a optional power gpio and enable it if it is valid.
> >
> 
> I see this appears to be a normal thing that needs implemented in U-Boot to
> have parity with Linux kernel, but the commit message should help me
> understand why so I don't have to think so hard.
> 
> Expand why this is useful and how this is implemented in Linux kernel, what is
> the API reference or something to give reason why U-Boot can expect the
> devicetree property to be present from upstream devicetree-rebasing.

OK. Will improve the commit messages. Thanks.

Best regards,
Hal

> 
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  drivers/pci/pcie_starfive_jh7110.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/pcie_starfive_jh7110.c
> > b/drivers/pci/pcie_starfive_jh7110.c
> > index 51aca7359ff..f6b50d12a90 100644
> > --- a/drivers/pci/pcie_starfive_jh7110.c
> > +++ b/drivers/pci/pcie_starfive_jh7110.c
> > @@ -45,6 +45,7 @@ struct starfive_pcie {
> >  	struct pcie_plda plda;
> >  	struct clk_bulk	clks;
> >  	struct reset_ctl_bulk	rsts;
> > +	struct gpio_desc	power_gpio;
> >  	struct gpio_desc	reset_gpio;
> >  	struct regmap *regmap;
> >  	unsigned int stg_pcie_base;
> > @@ -184,6 +185,10 @@ static int starfive_pcie_parse_dt(struct udevice
> *dev)
> >  		dev_err(dev, "reset-gpio is not valid\n");
> >  		return -EINVAL;
> >  	}
> > +
> > +	gpio_request_by_name(dev, "enable-gpios", 0, &priv->power_gpio,
> > +			     GPIOD_IS_OUT);
> > +
> >  	return 0;
> >  }
> >
> > @@ -205,6 +210,9 @@ static int starfive_pcie_init_port(struct udevice *dev)
> >  		goto err_deassert_clk;
> >  	}
> >
> > +	if (dm_gpio_is_valid(&priv->power_gpio))
> > +		dm_gpio_set_value(&priv->power_gpio, 1);
> > +
> >  	dm_gpio_set_value(&priv->reset_gpio, 1);
> >  	/* Disable physical functions except #0 */
> >  	for (i = 1; i < PLDA_FUNC_NUM; i++) {
> 
> -E

[RFC 07/10] riscv: dts: jh7110: Add StarFive VisionFive 2 Lite u-boot device tree

[Hal Feng]

Add the u-boot device tree include needed to support the
StarFive VisionFive 2 Lite board.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi     | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi

diff --git a/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi b/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
new file mode 100644
index 00000000000..46ef07a99eb
--- /dev/null
+++ b/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2025 StarFive Technology Co., Ltd.
+ */
+
+#include "jh7110-common-u-boot.dtsi"
+#include "starfive-visionfive2-binman.dtsi"
-- 
2.43.2

Re: [RFC 07/10] riscv: dts: jh7110: Add StarFive VisionFive 2 Lite u-boot device tree

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> Add the u-boot device tree include needed to support the
> StarFive VisionFive 2 Lite board.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi     | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
> 
> diff --git a/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi b/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
> new file mode 100644
> index 00000000000..46ef07a99eb
> --- /dev/null
> +++ b/arch/riscv/dts/jh7110s-starfive-visionfive-2-lite-u-boot.dtsi
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2025 StarFive Technology Co., Ltd.
> + */
> +
> +#include "jh7110-common-u-boot.dtsi"
> +#include "starfive-visionfive2-binman.dtsi"

NAK. Not needed, with some expected changes to U-Boot. See comments
about "[RFC 02/10] riscv: dts: starfive: Add VisionFive 2 Lite board
device tree" [1]
1:
https://lore.kernel.org/u-boot/3b75e20f-261e-4a23-b2b6-d4f4596b5fb3@freeshell.de/

-E

[RFC 08/10] configs: visionfive2: Add VisionFive 2 Lite DT to OF_LIST

[Hal Feng]

So the VisionFive 2 Lite DT will be built and merged into FIT.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 configs/starfive_visionfive2_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/starfive_visionfive2_defconfig b/configs/starfive_visionfive2_defconfig
index 544140c03f7..5a0e918c733 100644
--- a/configs/starfive_visionfive2_defconfig
+++ b/configs/starfive_visionfive2_defconfig
@@ -78,7 +78,7 @@ CONFIG_CMD_WDT=y
 CONFIG_CMD_WGET=y
 CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_BOARD=y
-CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01 starfive/jh7110-milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-starfive-visionfive-2-v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b"
+CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01 starfive/jh7110-milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-starfive-visionfive-2-v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b starfive/jh7110s-starfive-visionfive-2-lite"
 CONFIG_MULTI_DTB_FIT=y
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
-- 
2.43.2

Re: [RFC 08/10] configs: visionfive2: Add VisionFive 2 Lite DT to OF_LIST

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> So the VisionFive 2 Lite DT will be built and merged into FIT.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  configs/starfive_visionfive2_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configs/starfive_visionfive2_defconfig b/configs/starfive_visionfive2_defconfig
> index 544140c03f7..5a0e918c733 100644
> --- a/configs/starfive_visionfive2_defconfig
> +++ b/configs/starfive_visionfive2_defconfig
> @@ -78,7 +78,7 @@ CONFIG_CMD_WDT=y
>  CONFIG_CMD_WGET=y
>  CONFIG_CMD_BOOTSTAGE=y
>  CONFIG_OF_BOARD=y
> -CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01 starfive/jh7110-milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-starfive-visionfive-2-v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b"
> +CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01 starfive/jh7110-milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-starfive-visionfive-2-v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b starfive/jh7110s-starfive-visionfive-2-lite"
>  CONFIG_MULTI_DTB_FIT=y
>  CONFIG_ENV_OVERWRITE=y
>  CONFIG_ENV_IS_IN_SPI_FLASH=y

Okay. While we are adding a target with a different (yet similar) CPU
JH7110S, I have questions about if we can also make JH7100 a variant
here too. How much difference is allowed for one U-Boot board target?

It would be great to have a single U-Boot target
starfive_jh71xx_defconfig for example. I am aware that mainline Linux
kernel defconfig can boot on JH7100 but some peripheral drivers do not
work correctly, and so there is a requirement to compile Linux kernel as
non-portable [1]. However I want to know what prevents U-Boot from
supporting all of JH7100, JH7110, JH7110S, with the same build target?

1: https://wiki.debian.org/InstallingDebianOn/StarFive/VisionFiveV1

Anyways, this change to CONFIG_OF_LIST is sorted correctly and looks
good to me.

Reviewed-by: E Shattow <e@freeshell.de>

RE: [RFC 08/10] configs: visionfive2: Add VisionFive 2 Lite DT to OF_LIST

[Hal Feng]

> On 03.09.25 08:15, E Shattow wrote:
> On 8/28/25 23:09, Hal Feng wrote:
> > So the VisionFive 2 Lite DT will be built and merged into FIT.
> >
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  configs/starfive_visionfive2_defconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configs/starfive_visionfive2_defconfig
> > b/configs/starfive_visionfive2_defconfig
> > index 544140c03f7..5a0e918c733 100644
> > --- a/configs/starfive_visionfive2_defconfig
> > +++ b/configs/starfive_visionfive2_defconfig
> > @@ -78,7 +78,7 @@ CONFIG_CMD_WDT=y
> >  CONFIG_CMD_WGET=y
> >  CONFIG_CMD_BOOTSTAGE=y
> >  CONFIG_OF_BOARD=y
> > -CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01 starfive/jh7110-
> milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-starfive-visionfive-2-
> v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b"
> > +CONFIG_OF_LIST="starfive/jh7110-deepcomputing-fml13v01
> starfive/jh7110-milkv-mars starfive/jh7110-pine64-star64 starfive/jh7110-
> starfive-visionfive-2-v1.2a starfive/jh7110-starfive-visionfive-2-v1.3b
> starfive/jh7110s-starfive-visionfive-2-lite"
> >  CONFIG_MULTI_DTB_FIT=y
> >  CONFIG_ENV_OVERWRITE=y
> >  CONFIG_ENV_IS_IN_SPI_FLASH=y
> 
> Okay. While we are adding a target with a different (yet similar) CPU JH7110S, I
> have questions about if we can also make JH7100 a variant here too. How

No. The peripherals of JH7110S are the same as those of JH7110, but are quite
different from those of JH7100. JH7100 can not use the JH7110 device tree.

> much difference is allowed for one U-Boot board target?
> 
> It would be great to have a single U-Boot target starfive_jh71xx_defconfig for
> example. I am aware that mainline Linux kernel defconfig can boot on JH7100
> but some peripheral drivers do not work correctly, and so there is a
> requirement to compile Linux kernel as non-portable [1]. However I want to
> know what prevents U-Boot from supporting all of JH7100, JH7110, JH7110S,
> with the same build target?
> 
> 1: https://wiki.debian.org/InstallingDebianOn/StarFive/VisionFiveV1
> 
> Anyways, this change to CONFIG_OF_LIST is sorted correctly and looks good to
> me.
> 
> Reviewed-by: E Shattow <e@freeshell.de>

[RFC 09/10] board: starfive: spl: Support VisionFive 2 Lite

[Hal Feng]

Choose the matching FIT config on the VisionFive 2 Lite board.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 board/starfive/visionfive2/spl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
index 901e7b58f36..18571b482f9 100644
--- a/board/starfive/visionfive2/spl.c
+++ b/board/starfive/visionfive2/spl.c
@@ -133,6 +133,9 @@ int board_fit_config_name_match(const char *name)
 		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
 		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7))) {
 		return 0;
+	} else if (!strcmp(name, "starfive/jh7110s-starfive-visionfive-2-lite") &&
+		    !strncmp(get_product_id_from_eeprom(), "VF7110SL", 8)) {
+		return 0;
 	}
 
 	return -EINVAL;
-- 
2.43.2

Re: [RFC 09/10] board: starfive: spl: Support VisionFive 2 Lite

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> Choose the matching FIT config on the VisionFive 2 Lite board.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  board/starfive/visionfive2/spl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
> index 901e7b58f36..18571b482f9 100644
> --- a/board/starfive/visionfive2/spl.c
> +++ b/board/starfive/visionfive2/spl.c
> @@ -133,6 +133,9 @@ int board_fit_config_name_match(const char *name)
>  		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
>  		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7))) {

Deprecation of "VF7110b" and "VF7110a" mentioned previously should be in
its own patch.

>  		return 0;
> +	} else if (!strcmp(name, "starfive/jh7110s-starfive-visionfive-2-lite") &&
> +		    !strncmp(get_product_id_from_eeprom(), "VF7110SL", 8)) {
> +		return 0;
>  	}
>  
>  	return -EINVAL;

This is sorted correctly and looks good to me. With that,

Reviewed-by: E Shattow <e@freeshell.de>

[RFC 10/10] board: starfive: visionfive2: Add VisionFive 2 Lite fdt selection

[Hal Feng]

Set $fdtfile to the VisionFive 2 Lite DTB if the board is matched.

Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 board/starfive/visionfive2/starfive_visionfive2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c
index f38433e94ac..912e8cb967a 100644
--- a/board/starfive/visionfive2/starfive_visionfive2.c
+++ b/board/starfive/visionfive2/starfive_visionfive2.c
@@ -65,6 +65,8 @@ static void set_fdtfile(void)
 	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
 		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
 		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
+	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110SL", 8)) {
+		fdtfile = "starfive/jh7110s-starfive-visionfive-2-lite.dtb";
 	} else {
 		log_err("Unknown product\n");
 		return;
-- 
2.43.2

Re: [RFC 10/10] board: starfive: visionfive2: Add VisionFive 2 Lite fdt selection

[E Shattow]

On 8/28/25 23:09, Hal Feng wrote:
> Set $fdtfile to the VisionFive 2 Lite DTB if the board is matched.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  board/starfive/visionfive2/starfive_visionfive2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c
> index f38433e94ac..912e8cb967a 100644
> --- a/board/starfive/visionfive2/starfive_visionfive2.c
> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> @@ -65,6 +65,8 @@ static void set_fdtfile(void)
>  	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
>  		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
>  		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110SL", 8)) {
> +		fdtfile = "starfive/jh7110s-starfive-visionfive-2-lite.dtb";
>  	} else {
>  		log_err("Unknown product\n");
>  		return;

I continue to think it is silly we are setting $fdtfile env variable
past the moment since adopting OF_UPSTREAM. That is a discussion for
somewhere else, however.

Anyhow the sorting is correct and new code follows the existing code
pattern.

Reviewed-by: E Shattow <e@freeshell.de>

Re: [RFC 10/10] board: starfive: visionfive2: Add VisionFive 2 Lite fdt selection

[Heinrich Schuchardt]

E Shattow <e@freeshell.de> schrieb am Mi., 3. Sept. 2025, 02:26:

>
>
> On 8/28/25 23:09, Hal Feng wrote:
> > Set $fdtfile to the VisionFive 2 Lite DTB if the board is matched.
> >
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  board/starfive/visionfive2/starfive_visionfive2.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
> b/board/starfive/visionfive2/starfive_visionfive2.c
> > index f38433e94ac..912e8cb967a 100644
> > --- a/board/starfive/visionfive2/starfive_visionfive2.c
> > +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> > @@ -65,6 +65,8 @@ static void set_fdtfile(void)
> >       } else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> >                  !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
> >               fdtfile =
> "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> > +     } else if (!strncmp(get_product_id_from_eeprom(), "VF7110SL", 8)) {
> > +             fdtfile =
> "starfive/jh7110s-starfive-visionfive-2-lite.dtb";
> >       } else {
> >               log_err("Unknown product\n");
> >               return;
>
> I continue to think it is silly we are setting $fdtfile env variable
> past the moment since adopting OF_UPSTREAM. That is a discussion for
> somewhere else, however.
>

Do not expect an old kernel to boot with a newer dtb. Linux broke such
expectations repeatedly.

Do not assume that users upgrade U-Boot to keep it in sync with the kernel
they use.

Debian/Ubuntu packages flash-kernel and u-boot-menu rely on $fdtfile being
present.

Best regards

Heinrich


> Anyhow the sorting is correct and new code follows the existing code
> pattern.
>
> Reviewed-by: E Shattow <e@freeshell.de>
>