Re: [RFC] riscv: visionfive2: use OF_BOARD_SETUP

[Leo Liang <ycliang@andestech.com>]

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