[RFC] riscv: visionfive2: use OF_BOARD_SETUP
U-Boot already has a mechanism to fix up the DT before OS boot. This avoids the excessive duplication of data and work proposed by the explicit separation of 1.2a and 1.3b board revisions. It will also, to a good degree, improve the user experience, as pointed out by Matthias.
The defconfig changes required (in diffconfig format) are
-I2C n -NET_RANDOM_ETHADDR y +CMD_I2C y +CMD_MISC y +DM_I2C y +I2C_EEPROM y +MISC y +MISC_INIT_R y +OF_BOARD_SETUP y +SPL_DM_I2C n +SPL_MISC n +SYS_I2C_DW y +SYS_I2C_EEPROM_ADDR 0x0
along with the patch below. It has the neat side effect of providing the network with the proper MAC addresses ;)
I take advantage of the fact that I²C-5 is also required to talk to the PMIC, so it must already be initialised by OpenSBI. All that's required is to declare the EEPROM and to pull in the drivers.
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
The last hunk, to the i2c Makefile, is IMHO an independent fix, because the implication PCI => ACPI in designware_i2c_pci is invalid, and the VisionFive2 config proves it. Use this quick hack for now.
Signed-off-by: Torsten Duwe duwe@suse.de
--- diff --git a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi index ff9df56ec2..fd3a1d057a 100644 --- a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi +++ b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi @@ -119,6 +119,12 @@ pinctrl-names = "default"; pinctrl-0 = <&i2c5_pins>; status = "okay"; + + eeprom@50 { + compatible = "atmel,24c04"; + reg = <0x50>; + pagesize = <0x10>; + }; };
&i2c6 { diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c index 613fe793c4..d7f846a357 100644 --- a/board/starfive/visionfive2/starfive_visionfive2.c +++ b/board/starfive/visionfive2/starfive_visionfive2.c @@ -7,6 +7,10 @@ #include <common.h> #include <asm/io.h> #include <cpu_func.h> +#include <dm/uclass.h> +#include <fdt_support.h> +#include <i2c_eeprom.h> +#include <net.h> #include <linux/bitops.h>
#define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 @@ -38,3 +42,62 @@ int board_init(void)
return 0; } + +#ifdef CONFIG_MISC_INIT_R +int misc_init_r(void) +{ + int ret = 0; + +#ifdef CONFIG_I2C_EEPROM + struct udevice *dev; + char mac_addr[6]; + unsigned char pcb_rev, BOM; + + ret = uclass_first_device_err(UCLASS_I2C_EEPROM, &dev); + if (ret) + goto out; + + if (eth_env_get_enetaddr("ethaddr", mac_addr) == 0) { + int i; + for (i=0; i<2; i++) { + ret = i2c_eeprom_read(dev, 0x78+6*i, mac_addr, 6); + if (!ret && is_valid_ethaddr(mac_addr)) + eth_env_set_enetaddr_by_index("eth", i, mac_addr); + } + } + + ret = i2c_eeprom_read(dev, 0x76, &pcb_rev, 1); + if (!ret) + env_set_hex("board_revision", pcb_rev); + + ret = i2c_eeprom_read(dev, 0x77, &BOM, 1); + + out: +#endif + return ret; +} +#endif + +#ifdef CONFIG_OF_BOARD_SETUP +static void adjust_for_rev13b(void * fdt) +{ + do_fixup_by_path(fdt, "/soc/ethernet@16040000", + "phy-mode", "rgmii-id", 9, 0); + /* + ... other fixups ... + + */ +} + +int ft_board_setup(void *fdt, struct bd_info *bdip) +{ + unsigned char pcb_rev = 0; + + pcb_rev = env_get_hex("board_revision", pcb_rev); + if (pcb_rev >= 0xB2) { + printf("Adjusting FDT for v1.3B board rev\n"); + adjust_for_rev13b(fdt); + } + return 0; +} +#endif diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 99545df2e5..828856e40d 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -19,8 +19,10 @@ obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o obj-$(CONFIG_SYS_I2C_DAVINCI) += davinci_i2c.o obj-$(CONFIG_SYS_I2C_DW) += designware_i2c.o ifdef CONFIG_PCI +ifdef CONFIG_ACPIGEN obj-$(CONFIG_SYS_I2C_DW) += designware_i2c_pci.o endif +endif obj-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o obj-$(CONFIG_SYS_I2C_IHS) += ihs_i2c.o obj-$(CONFIG_SYS_I2C_INTEL) += intel_i2c.o
On 19/04/2023 13:28, Torsten Duwe wrote:
U-Boot already has a mechanism to fix up the DT before OS boot. This avoids the excessive duplication of data and work proposed by the explicit separation of 1.2a and 1.3b board revisions. It will also, to a good degree, improve the user experience, as pointed out by Matthias.
The defconfig changes required (in diffconfig format) are
-I2C n -NET_RANDOM_ETHADDR y +CMD_I2C y +CMD_MISC y +DM_I2C y +I2C_EEPROM y +MISC y +MISC_INIT_R y +OF_BOARD_SETUP y +SPL_DM_I2C n +SPL_MISC n +SYS_I2C_DW y +SYS_I2C_EEPROM_ADDR 0x0
along with the patch below. It has the neat side effect of providing the network with the proper MAC addresses ;)
I take advantage of the fact that I²C-5 is also required to talk to the PMIC, so it must already be initialised by OpenSBI. All that's required is to declare the EEPROM and to pull in the drivers.
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
The last hunk, to the i2c Makefile, is IMHO an independent fix, because the implication PCI => ACPI in designware_i2c_pci is invalid, and the VisionFive2 config proves it. Use this quick hack for now.
Looks like a neat approach to enable a signle U-Boot binary to boot on both platforms.
Thanks for investigating this.
Signed-off-by: Torsten Duwe duwe@suse.de
diff --git a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi index ff9df56ec2..fd3a1d057a 100644 --- a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi +++ b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi @@ -119,6 +119,12 @@ pinctrl-names = "default"; pinctrl-0 = <&i2c5_pins>; status = "okay";
eeprom@50 {
compatible = "atmel,24c04";reg = <0x50>;pagesize = <0x10>;}; };
&i2c6 {
diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c index 613fe793c4..d7f846a357 100644 --- a/board/starfive/visionfive2/starfive_visionfive2.c +++ b/board/starfive/visionfive2/starfive_visionfive2.c @@ -7,6 +7,10 @@ #include <common.h> #include <asm/io.h> #include <cpu_func.h> +#include <dm/uclass.h> +#include <fdt_support.h> +#include <i2c_eeprom.h> +#include <net.h> #include <linux/bitops.h>
#define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 @@ -38,3 +42,62 @@ int board_init(void)
return 0; }
+#ifdef CONFIG_MISC_INIT_R
As this is will be enabled for the board config I think we don't need the ifedef.
+int misc_init_r(void) +{
- int ret = 0;
+#ifdef CONFIG_I2C_EEPROM
struct udevice *dev;- char mac_addr[6];
- unsigned char pcb_rev, BOM;
- ret = uclass_first_device_err(UCLASS_I2C_EEPROM, &dev);
- if (ret)
goto out;- if (eth_env_get_enetaddr("ethaddr", mac_addr) == 0) {
int i;for (i=0; i<2; i++) {ret = i2c_eeprom_read(dev, 0x78+6*i, mac_addr, 6);if (!ret && is_valid_ethaddr(mac_addr))eth_env_set_enetaddr_by_index("eth", i, mac_addr);}- }
- ret = i2c_eeprom_read(dev, 0x76, &pcb_rev, 1);
- if (!ret)
env_set_hex("board_revision", pcb_rev);- ret = i2c_eeprom_read(dev, 0x77, &BOM, 1);
- out:
+#endif
return ret;+} +#endif
+#ifdef CONFIG_OF_BOARD_SETUP
Same here.
Regards, Matthias
+static void adjust_for_rev13b(void * fdt) +{
- do_fixup_by_path(fdt, "/soc/ethernet@16040000",
"phy-mode", "rgmii-id", 9, 0);- /*
... other fixups ...*/+}
+int ft_board_setup(void *fdt, struct bd_info *bdip) +{
- unsigned char pcb_rev = 0;
- pcb_rev = env_get_hex("board_revision", pcb_rev);
- if (pcb_rev >= 0xB2) {
printf("Adjusting FDT for v1.3B board rev\n");adjust_for_rev13b(fdt);- }
- return 0;
+} +#endif diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 99545df2e5..828856e40d 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -19,8 +19,10 @@ obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o obj-$(CONFIG_SYS_I2C_DAVINCI) += davinci_i2c.o obj-$(CONFIG_SYS_I2C_DW) += designware_i2c.o ifdef CONFIG_PCI +ifdef CONFIG_ACPIGEN obj-$(CONFIG_SYS_I2C_DW) += designware_i2c_pci.o endif +endif obj-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o obj-$(CONFIG_SYS_I2C_IHS) += ihs_i2c.o obj-$(CONFIG_SYS_I2C_INTEL) += intel_i2c.o
Hi, Torsten, Matthias,
On Wed, Apr 19, 2023 at 02:34:03PM +0200, Matthias Brugger wrote:
On 19/04/2023 13:28, Torsten Duwe wrote:
U-Boot already has a mechanism to fix up the DT before OS boot. This avoids the excessive duplication of data and work proposed by the explicit separation of 1.2a and 1.3b board revisions. It will also, to a good degree, improve the user experience, as pointed out by Matthias.
The defconfig changes required (in diffconfig format) are
-I2C n -NET_RANDOM_ETHADDR y +CMD_I2C y +CMD_MISC y +DM_I2C y +I2C_EEPROM y +MISC y +MISC_INIT_R y +OF_BOARD_SETUP y +SPL_DM_I2C n +SPL_MISC n +SYS_I2C_DW y +SYS_I2C_EEPROM_ADDR 0x0
along with the patch below. It has the neat side effect of providing the network with the proper MAC addresses ;)
I take advantage of the fact that I²C-5 is also required to talk to the PMIC, so it must already be initialised by OpenSBI. All that's required is to declare the EEPROM and to pull in the drivers.
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
The last hunk, to the i2c Makefile, is IMHO an independent fix, because the implication PCI => ACPI in designware_i2c_pci is invalid, and the VisionFive2 config proves it. Use this quick hack for now.
Looks like a neat approach to enable a signle U-Boot binary to boot on both platforms.
Thanks for investigating this.
LGTM as well!
Best regards, Leo
Signed-off-by: Torsten Duwe duwe@suse.de
diff --git a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi index ff9df56ec2..fd3a1d057a 100644 --- a/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi +++ b/arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi @@ -119,6 +119,12 @@ pinctrl-names = "default"; pinctrl-0 = <&i2c5_pins>; status = "okay";
- eeprom@50 {
compatible = "atmel,24c04";reg = <0x50>;pagesize = <0x10>;- }; }; &i2c6 {
diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c index 613fe793c4..d7f846a357 100644 --- a/board/starfive/visionfive2/starfive_visionfive2.c +++ b/board/starfive/visionfive2/starfive_visionfive2.c @@ -7,6 +7,10 @@ #include <common.h> #include <asm/io.h> #include <cpu_func.h> +#include <dm/uclass.h> +#include <fdt_support.h> +#include <i2c_eeprom.h> +#include <net.h> #include <linux/bitops.h> #define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 @@ -38,3 +42,62 @@ int board_init(void) return 0; }
+#ifdef CONFIG_MISC_INIT_R
As this is will be enabled for the board config I think we don't need the ifedef.
+int misc_init_r(void) +{
- int ret = 0;
+#ifdef CONFIG_I2C_EEPROM
struct udevice *dev;- char mac_addr[6];
- unsigned char pcb_rev, BOM;
- ret = uclass_first_device_err(UCLASS_I2C_EEPROM, &dev);
- if (ret)
goto out;- if (eth_env_get_enetaddr("ethaddr", mac_addr) == 0) {
int i;for (i=0; i<2; i++) {ret = i2c_eeprom_read(dev, 0x78+6*i, mac_addr, 6);if (!ret && is_valid_ethaddr(mac_addr))eth_env_set_enetaddr_by_index("eth", i, mac_addr);}- }
- ret = i2c_eeprom_read(dev, 0x76, &pcb_rev, 1);
- if (!ret)
env_set_hex("board_revision", pcb_rev);- ret = i2c_eeprom_read(dev, 0x77, &BOM, 1);
- out:
+#endif
return ret;+} +#endif
+#ifdef CONFIG_OF_BOARD_SETUP
Same here.
Regards, Matthias
+static void adjust_for_rev13b(void * fdt) +{
- do_fixup_by_path(fdt, "/soc/ethernet@16040000",
"phy-mode", "rgmii-id", 9, 0);- /*
... other fixups ...*/+}
+int ft_board_setup(void *fdt, struct bd_info *bdip) +{
- unsigned char pcb_rev = 0;
- pcb_rev = env_get_hex("board_revision", pcb_rev);
- if (pcb_rev >= 0xB2) {
printf("Adjusting FDT for v1.3B board rev\n");adjust_for_rev13b(fdt);- }
- return 0;
+} +#endif diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 99545df2e5..828856e40d 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -19,8 +19,10 @@ obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o obj-$(CONFIG_SYS_I2C_DAVINCI) += davinci_i2c.o obj-$(CONFIG_SYS_I2C_DW) += designware_i2c.o ifdef CONFIG_PCI +ifdef CONFIG_ACPIGEN obj-$(CONFIG_SYS_I2C_DW) += designware_i2c_pci.o endif +endif obj-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o obj-$(CONFIG_SYS_I2C_IHS) += ihs_i2c.o obj-$(CONFIG_SYS_I2C_INTEL) += intel_i2c.o
Hi Leo, thanks for the quick reply!
On Thu, 20 Apr 2023 06:33:57 +0000 Leo Liang ycliang@andestech.com wrote:
Hi, Torsten, Matthias,
On Wed, Apr 19, 2023 at 02:34:03PM +0200, Matthias Brugger wrote:
On 19/04/2023 13:28, Torsten Duwe wrote:
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
LGTM as well!
Thank you very much! Again, this is only a PoC; if you agree with the concept, I clean it up and fill in the complete DT patching.
Questions: shall I default to 1.3B and patch older 1.2A, or vice versa, or do it like your (starfive) patch set and start with something "neutral" and then patch both ways? And, more important, what is the correct interpretation of the board revision byte -- I assume it's offset 0x76 in the EEPROM? Is it always? Is ">= 0xB2" the correct discriminator?
Torsten
On 2023/4/20 15:43, Torsten Duwe wrote:
Hi Leo, thanks for the quick reply!
On Thu, 20 Apr 2023 06:33:57 +0000 Leo Liang ycliang@andestech.com wrote:
Hi, Torsten, Matthias,
On Wed, Apr 19, 2023 at 02:34:03PM +0200, Matthias Brugger wrote:
On 19/04/2023 13:28, Torsten Duwe wrote:
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
LGTM as well!
Thank you very much! Again, this is only a PoC; if you agree with the concept, I clean it up and fill in the complete DT patching.
Questions: shall I default to 1.3B and patch older 1.2A, or vice versa, or do it like your (starfive) patch set and start with something "neutral" and then patch both ways? And, more important, what is the correct interpretation of the board revision byte -- I assume it's offset 0x76 in the EEPROM? Is it always? Is ">= 0xB2" the correct discriminator?
Hi Torsten,
The attached is the driver of VisionFive2 EEPROM, which is only a preliminary draft, and it is still being improved [the function has been tested, it is normal], and it contains the definition of the EEPROM data format.
Torsten
Hi Torsten,
On 20/04/2023 09:43, Torsten Duwe wrote:
Hi Leo, thanks for the quick reply!
On Thu, 20 Apr 2023 06:33:57 +0000 Leo Liang ycliang@andestech.com wrote:
Hi, Torsten, Matthias,
On Wed, Apr 19, 2023 at 02:34:03PM +0200, Matthias Brugger wrote:
On 19/04/2023 13:28, Torsten Duwe wrote:
This is only a proof of concept; let me know if you like it and I can add the other 12 DT patches to adjust_for_rev13b(), or maybe start with 1.3b as the default and go the other way, or something in between.
LGTM as well!
Thank you very much! Again, this is only a PoC; if you agree with the concept, I clean it up and fill in the complete DT patching.
Questions: shall I default to 1.3B and patch older 1.2A, or vice versa, or do it like your (starfive) patch set and start with something "neutral" and then patch both ways? And, more important, what is the
I would go with the common case (I suppose that's version 1.3B) and detect and patch older version 1.2A.
Regards, Matthias
correct interpretation of the board revision byte -- I assume it's offset 0x76 in the EEPROM? Is it always? Is ">= 0xB2" the correct discriminator?
Torsten
participants (4)
-
Leo Liang -
Matthias Brugger -
Torsten Duwe -
yanhong wang