| Message ID | 20240303130122.29983-3-heinrich.schuchardt@canonical.com |
|---|---|
| State | RFC |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: add support for Milk-V Mars board | expand |
Sharing the info from Mars CM Lite 4GB: --------EEPROM INFO-------- Vendor : MILK-V Product full SN: MARC-V10-2340-D004E016-000006DF data version: 0x2 PCB revision: 0xc1 BOM revision: A Ethernet MAC0 address: 6c:cf:39:00:83:11 Ethernet MAC1 address: 6c:cf:39:00:83:12 --------EEPROM INFO-------- I do not like this practice of force setting fdtfile environment variable. If I set fdtfile from u-boot prompt and saveenv, I expect that my action as the user will persist over a reboot but instead it gets ignored and overwritten by this strategy. Is this necessary to happen before the boot scripts? On Sun, Mar 3, 2024 at 5:02 AM Heinrich Schuchardt < heinrich.schuchardt@canonical.com> wrote: > Set environment variable fdtfile to the correct value for the Milk-V Mars > board. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > .../visionfive2/starfive_visionfive2.c | 43 +++++++++++++------ > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/board/starfive/visionfive2/starfive_visionfive2.c > b/board/starfive/visionfive2/starfive_visionfive2.c > index 78e118d5a05..9970e309690 100644 > --- a/board/starfive/visionfive2/starfive_visionfive2.c > +++ b/board/starfive/visionfive2/starfive_visionfive2.c > @@ -9,6 +9,7 @@ > #include <dm.h> > #include <fdt_support.h> > #include <env.h> > +#include <log.h> > #include <asm/arch/eeprom.h> > #include <asm/io.h> > #include <asm/sections.h> > @@ -17,6 +18,8 @@ > DECLARE_GLOBAL_DATA_PTR; > #define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 > #define JH7110_L2_PREFETCHER_HART_OFFSET 0x2000 > +#define FDTFILE_MILK_V_MARS \ > + "starfive/jh7110-milkv-mars.dtb" > #define FDTFILE_VISIONFIVE2_1_2A \ > "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb" > #define FDTFILE_VISIONFIVE2_1_3B \ > @@ -48,20 +51,34 @@ static void set_fdtfile(void) > { > u8 version; > const char *fdtfile; > + const char *product_id; > > - version = get_pcb_revision_from_eeprom(); > - switch (version) { > - case 'a': > - case 'A': > - fdtfile = FDTFILE_VISIONFIVE2_1_2A; > - break; > - > - case 'b': > - case 'B': > - default: > - fdtfile = FDTFILE_VISIONFIVE2_1_3B; > - break; > - }; > + product_id = get_product_id_from_eeprom(); > + if (!product_id) { > + log_err("Can't read EEPROM\n"); > + return; > + } > + if (!strncmp(product_id, "MARS", 4)) { > + fdtfile = FDTFILE_MILK_V_MARS; > + } else if (!strncmp(product_id, "VF7110", 6)) { > + version = get_pcb_revision_from_eeprom(); > + > + switch (version) { > + case 'a': > + case 'A': > + fdtfile = FDTFILE_VISIONFIVE2_1_2A; > + break; > + > + case 'b': > + case 'B': > + default: > + fdtfile = FDTFILE_VISIONFIVE2_1_3B; > + break; > + } > + } else { > + log_err("Unknown product\n"); > + return; > + } > > env_set("fdtfile", fdtfile); > } > -- > 2.43.0 > >
On 3/8/24 23:37, E Shattow wrote: > Sharing the info from Mars CM Lite 4GB: > --------EEPROM INFO-------- > Vendor : MILK-V > Product full SN: MARC-V10-2340-D004E016-000006DF > data version: 0x2 > PCB revision: 0xc1 > BOM revision: A > Ethernet MAC0 address: 6c:cf:39:00:83:11 > Ethernet MAC1 address: 6c:cf:39:00:83:12 > --------EEPROM INFO-------- > > I do not like this practice of force setting fdtfile environment > variable. If I set fdtfile from u-boot prompt and saveenv, I expect that > my action as the user will persist over a reboot but instead it gets > ignored and overwritten by this strategy. Is this necessary to happen > before the boot scripts? We could check if $fdtfile is already set before the update. This would require that the default environment does not include it. Cf. include/configs/starfive-visionfive2.h:51: "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ Best regards Heinrich > > On Sun, Mar 3, 2024 at 5:02 AM Heinrich Schuchardt > <heinrich.schuchardt@canonical.com > <mailto:heinrich.schuchardt@canonical.com>> wrote: > > Set environment variable fdtfile to the correct value for the Milk-V > Mars > board. > > Signed-off-by: Heinrich Schuchardt > <heinrich.schuchardt@canonical.com > <mailto:heinrich.schuchardt@canonical.com>> > --- > .../visionfive2/starfive_visionfive2.c | 43 +++++++++++++------ > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/board/starfive/visionfive2/starfive_visionfive2.c > b/board/starfive/visionfive2/starfive_visionfive2.c > index 78e118d5a05..9970e309690 100644 > --- a/board/starfive/visionfive2/starfive_visionfive2.c > +++ b/board/starfive/visionfive2/starfive_visionfive2.c > @@ -9,6 +9,7 @@ > #include <dm.h> > #include <fdt_support.h> > #include <env.h> > +#include <log.h> > #include <asm/arch/eeprom.h> > #include <asm/io.h> > #include <asm/sections.h> > @@ -17,6 +18,8 @@ > DECLARE_GLOBAL_DATA_PTR; > #define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 > #define JH7110_L2_PREFETCHER_HART_OFFSET 0x2000 > +#define FDTFILE_MILK_V_MARS \ > + "starfive/jh7110-milkv-mars.dtb" > #define FDTFILE_VISIONFIVE2_1_2A \ > "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb" > #define FDTFILE_VISIONFIVE2_1_3B \ > @@ -48,20 +51,34 @@ static void set_fdtfile(void) > { > u8 version; > const char *fdtfile; > + const char *product_id; > > - version = get_pcb_revision_from_eeprom(); > - switch (version) { > - case 'a': > - case 'A': > - fdtfile = FDTFILE_VISIONFIVE2_1_2A; > - break; > - > - case 'b': > - case 'B': > - default: > - fdtfile = FDTFILE_VISIONFIVE2_1_3B; > - break; > - }; > + product_id = get_product_id_from_eeprom(); > + if (!product_id) { > + log_err("Can't read EEPROM\n"); > + return; > + } > + if (!strncmp(product_id, "MARS", 4)) { > + fdtfile = FDTFILE_MILK_V_MARS; > + } else if (!strncmp(product_id, "VF7110", 6)) { > + version = get_pcb_revision_from_eeprom(); > + > + switch (version) { > + case 'a': > + case 'A': > + fdtfile = FDTFILE_VISIONFIVE2_1_2A; > + break; > + > + case 'b': > + case 'B': > + default: > + fdtfile = FDTFILE_VISIONFIVE2_1_3B; > + break; > + } > + } else { > + log_err("Unknown product\n"); > + return; > + } > > env_set("fdtfile", fdtfile); > } > -- > 2.43.0 >
On Fri, Mar 8, 2024 at 2:57 PM Heinrich Schuchardt < heinrich.schuchardt@canonical.com> wrote: ... > > I do not like this practice of force setting fdtfile environment > > variable. If I set fdtfile from u-boot prompt and saveenv, I expect that > > my action as the user will persist over a reboot but instead it gets > > ignored and overwritten by this strategy. Is this necessary to happen > > before the boot scripts? > > We could check if $fdtfile is already set before the update. > > This would require that the default environment does not include it. Cf. > > include/configs/starfive-visionfive2.h:51: > "fdtfile=" CONFIG_DEFAULT_FDT_FILE "\0" \ > ... The default environment is expected to have a sensible default, and implicitly is only used when there is a checksum error (so no existing valid fdtfile environment variable). It is not nice to remove capability from userspace i.e. Linux mtd interface access to update u-boot environment variables; setting fdtfile as such has no effect as it will always be force set by this routine. The workaround now is to set fdtfile from a boot.scr but that adds a dependency to reside on storage (which is not always true for all boot situations). What of "fdtfile=${fdtfile_board}" for the default environment and this routine instead updates $fdtfile_board (or similar) ? I'm not sure what to suggest, or if this is a common practice for u-boot board support. Thanks - E
diff --git a/board/starfive/visionfive2/starfive_visionfive2.c b/board/starfive/visionfive2/starfive_visionfive2.c index 78e118d5a05..9970e309690 100644 --- a/board/starfive/visionfive2/starfive_visionfive2.c +++ b/board/starfive/visionfive2/starfive_visionfive2.c @@ -9,6 +9,7 @@ #include <dm.h> #include <fdt_support.h> #include <env.h> +#include <log.h> #include <asm/arch/eeprom.h> #include <asm/io.h> #include <asm/sections.h> @@ -17,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR; #define JH7110_L2_PREFETCHER_BASE_ADDR 0x2030000 #define JH7110_L2_PREFETCHER_HART_OFFSET 0x2000 +#define FDTFILE_MILK_V_MARS \ + "starfive/jh7110-milkv-mars.dtb" #define FDTFILE_VISIONFIVE2_1_2A \ "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb" #define FDTFILE_VISIONFIVE2_1_3B \ @@ -48,20 +51,34 @@ static void set_fdtfile(void) { u8 version; const char *fdtfile; + const char *product_id; - version = get_pcb_revision_from_eeprom(); - switch (version) { - case 'a': - case 'A': - fdtfile = FDTFILE_VISIONFIVE2_1_2A; - break; - - case 'b': - case 'B': - default: - fdtfile = FDTFILE_VISIONFIVE2_1_3B; - break; - }; + product_id = get_product_id_from_eeprom(); + if (!product_id) { + log_err("Can't read EEPROM\n"); + return; + } + if (!strncmp(product_id, "MARS", 4)) { + fdtfile = FDTFILE_MILK_V_MARS; + } else if (!strncmp(product_id, "VF7110", 6)) { + version = get_pcb_revision_from_eeprom(); + + switch (version) { + case 'a': + case 'A': + fdtfile = FDTFILE_VISIONFIVE2_1_2A; + break; + + case 'b': + case 'B': + default: + fdtfile = FDTFILE_VISIONFIVE2_1_3B; + break; + } + } else { + log_err("Unknown product\n"); + return; + } env_set("fdtfile", fdtfile); }
Set environment variable fdtfile to the correct value for the Milk-V Mars board. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- .../visionfive2/starfive_visionfive2.c | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-)