| Message ID | 1429761426-26748-5-git-send-email-festevam@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Fabio, On 23/04/2015 05:57, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Instead of hardcoding the 'fdt_file' variable, let's introduce a new > function - build_dts_name(), that can build the dtb filename on the fly. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > board/solidrun/mx6cuboxi/mx6cuboxi.c | 24 ++++++++++++++++++++++++ > include/configs/mx6cuboxi.h | 3 +-- > 2 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/board/solidrun/mx6cuboxi/mx6cuboxi.c b/board/solidrun/mx6cuboxi/mx6cuboxi.c > index 83410b2..1c24a55 100644 > --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c > +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c > @@ -212,6 +212,30 @@ int checkboard(void) > return 0; > } > > +static const char *build_dts_name(void) > +{ > + char *dt_prefix = "unknown"; > + char *dt_suffix = "unknown"; > + > + if (is_cpu_type(MXC_CPU_MX6Q) || is_cpu_type(MXC_CPU_MX6D)) > + dt_prefix = "imx6q"; > + else if (is_cpu_type(MXC_CPU_MX6SOLO) || is_cpu_type(MXC_CPU_MX6DL)) > + dt_prefix = "imx6dl"; > + > + if (is_hummingboard()) > + dt_suffix = "-hummingboard.dtb"; > + else > + dt_suffix = "-cubox-i.dtb"; > + > + return strcat(dt_prefix, dt_suffix); > +} > + I admit I do not like a lot to have C code setting / fixing the environment. This has the drawback that when a user try to set the environment from the console as he wants, he cannot because the code has reverted back and it is not easy to track. I would like to propose another solution. What about to export your is_hummingboard() function as U-Boot command ? You can then use it in U-Boot scripts, and the correct fdt name can be set in the "bootcmd" variable. Something like "if is_humming;then ..." And if a user wants to use other names, he can because it is not hard-coded. > +int misc_init_r(void) > +{ > + setenv("fdt_file", build_dts_name()); > + return 0; > +} > + > #ifdef CONFIG_SPL_BUILD > #include <asm/arch/mx6-ddr.h> > static const struct mx6dq_iomux_ddr_regs mx6q_ddr_ioregs = { > diff --git a/include/configs/mx6cuboxi.h b/include/configs/mx6cuboxi.h > index 5d58b16..504a81c 100644 > --- a/include/configs/mx6cuboxi.h > +++ b/include/configs/mx6cuboxi.h > @@ -29,6 +29,7 @@ > > #define CONFIG_SYS_MALLOC_LEN (2 * SZ_1M) > #define CONFIG_BOARD_EARLY_INIT_F > +#define CONFIG_MISC_INIT_R > #define CONFIG_MXC_GPIO > #define CONFIG_MXC_UART > #define CONFIG_CMD_FUSE > @@ -81,14 +82,12 @@ > #define CONFIG_MXC_UART_BASE UART1_BASE > #define CONFIG_CONSOLE_DEV "ttymxc0" > #define CONFIG_MMCROOT "/dev/mmcblk0p2" > -#define CONFIG_DEFAULT_FDT_FILE "imx6q-hummingboard.dtb" > #define CONFIG_SYS_FSL_USDHC_NUM 1 > #define CONFIG_SYS_MMC_ENV_DEV 0 /* SDHC2 */ > > #define CONFIG_EXTRA_ENV_SETTINGS \ > "script=boot.scr\0" \ > "image=zImage\0" \ > - "fdt_file=" CONFIG_DEFAULT_FDT_FILE "\0" \ I do not exclude that the board will switch to distro environment, and we will have a strong dependency with the code then. Best regards, Stefano
Hi Stefano, On Thu, Apr 23, 2015 at 3:13 AM, Stefano Babic <sbabic@denx.de> wrote: > I admit I do not like a lot to have C code setting / fixing the > environment. This has the drawback that when a user try to set the > environment from the console as he wants, he cannot because the code has > reverted back and it is not easy to track. I would like to propose > another solution. Thanks for the suggestion. > What about to export your is_hummingboard() function as U-Boot command ? > You can then use it in U-Boot scripts, and the correct fdt name can be > set in the "bootcmd" variable. Something like "if is_humming;then ..." I am not sure how I can retrieve the returned value from is_hummingboard() as a U-boot command and use it inside a script? Maybe I did not understand your suggestion. Please advise. > And if a user wants to use other names, he can because it is not hard-coded. Yes, I understand the concern, but in this specific case we are talking about a DTB file, which is board specific and cannot really change. Regards, Fabio Estevam
Hi Fabio, On 23/04/2015 19:18, Fabio Estevam wrote: >> What about to export your is_hummingboard() function as U-Boot command ? >> You can then use it in U-Boot scripts, and the correct fdt name can be >> set in the "bootcmd" variable. Something like "if is_humming;then ..." > > I am not sure how I can retrieve the returned value from > is_hummingboard() as a U-boot command and use it inside a script? > Maybe I did not understand your suggestion. Please advise. U_BOOT_CMD returns a value that can be evaluated, exactly as we do with "if tftp.." or for other commands. So you could implement: U_BOOT_CMD(is_hummingbird, 1, 1, do_is_hummingbird, .. and the do_is_hummingbird can return CMD_RET_SUCCESS or CMD_RET_FAILURE. This is then evaluated in the script as "if is_hummingbird;then fdt_file=....;else fdt_file=...;fi" > >> And if a user wants to use other names, he can because it is not hard-coded. > > Yes, I understand the concern, but in this specific case we are > talking about a DTB file, which is board specific and cannot really > change. Well, maybe I am alone, but I am used to have several DTB files during the developmnet phase - I agree with you that at the end there should be only one DTB file. Anyway, my was only a proposal - it is also fine if you decide to maintain the current implementation. Best regards, Stefano Babic
On 2015-04-22, Fabio Estevam wrote: > Instead of hardcoding the 'fdt_file' variable, let's introduce a new > function - build_dts_name(), that can build the dtb filename on the fly. > diff --git a/board/solidrun/mx6cuboxi/mx6cuboxi.c b/board/solidrun/mx6cuboxi/mx6cuboxi.c > index 83410b2..1c24a55 100644 > --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c > +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c ... > +int misc_init_r(void) > +{ > + setenv("fdt_file", build_dts_name()); > + return 0; > +} > + Shouldn't this set fdtfile instead of fdt_file? According to top-level README, several variables are configured: Image File Name RAM Address Flash Location ----- --------- ----------- -------------- u-boot u-boot u-boot_addr_r u-boot_addr Linux kernel bootfile kernel_addr_r kernel_addr device tree blob fdtfile fdt_addr_r fdt_addr ramdisk ramdiskfile ramdisk_addr_r ramdisk_addr FWIW, I'm happy to test and review patches for cubox-i4po, hummingboard solo and hummingboard i2ex. live well, vagrant
On Thu, Apr 23, 2015 at 08:13:05AM +0200, Stefano Babic wrote: > Hi Fabio, > > On 23/04/2015 05:57, Fabio Estevam wrote: > > From: Fabio Estevam <fabio.estevam@freescale.com> > > > > Instead of hardcoding the 'fdt_file' variable, let's introduce a new > > function - build_dts_name(), that can build the dtb filename on the fly. > > > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > > --- > > board/solidrun/mx6cuboxi/mx6cuboxi.c | 24 ++++++++++++++++++++++++ > > include/configs/mx6cuboxi.h | 3 +-- > > 2 files changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/board/solidrun/mx6cuboxi/mx6cuboxi.c b/board/solidrun/mx6cuboxi/mx6cuboxi.c > > index 83410b2..1c24a55 100644 > > --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c > > +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c > > @@ -212,6 +212,30 @@ int checkboard(void) > > return 0; > > } > > > > +static const char *build_dts_name(void) > > +{ > > + char *dt_prefix = "unknown"; > > + char *dt_suffix = "unknown"; > > + > > + if (is_cpu_type(MXC_CPU_MX6Q) || is_cpu_type(MXC_CPU_MX6D)) > > + dt_prefix = "imx6q"; > > + else if (is_cpu_type(MXC_CPU_MX6SOLO) || is_cpu_type(MXC_CPU_MX6DL)) > > + dt_prefix = "imx6dl"; > > + > > + if (is_hummingboard()) > > + dt_suffix = "-hummingboard.dtb"; > > + else > > + dt_suffix = "-cubox-i.dtb"; > > + > > + return strcat(dt_prefix, dt_suffix); > > +} > > + > > I admit I do not like a lot to have C code setting / fixing the > environment. This has the drawback that when a user try to set the > environment from the console as he wants, he cannot because the code has > reverted back and it is not easy to track. I would like to propose > another solution. Yes. Please see how this is done for the various TI boards where we export into the env enough information to make these kind of decisions in the shell. This is even more applicable here as we don't have the dynamic "pick a DT" for Falcon Mode that the Solid Run tree has where this originated in.
diff --git a/board/solidrun/mx6cuboxi/mx6cuboxi.c b/board/solidrun/mx6cuboxi/mx6cuboxi.c index 83410b2..1c24a55 100644 --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c @@ -212,6 +212,30 @@ int checkboard(void) return 0; } +static const char *build_dts_name(void) +{ + char *dt_prefix = "unknown"; + char *dt_suffix = "unknown"; + + if (is_cpu_type(MXC_CPU_MX6Q) || is_cpu_type(MXC_CPU_MX6D)) + dt_prefix = "imx6q"; + else if (is_cpu_type(MXC_CPU_MX6SOLO) || is_cpu_type(MXC_CPU_MX6DL)) + dt_prefix = "imx6dl"; + + if (is_hummingboard()) + dt_suffix = "-hummingboard.dtb"; + else + dt_suffix = "-cubox-i.dtb"; + + return strcat(dt_prefix, dt_suffix); +} + +int misc_init_r(void) +{ + setenv("fdt_file", build_dts_name()); + return 0; +} + #ifdef CONFIG_SPL_BUILD #include <asm/arch/mx6-ddr.h> static const struct mx6dq_iomux_ddr_regs mx6q_ddr_ioregs = { diff --git a/include/configs/mx6cuboxi.h b/include/configs/mx6cuboxi.h index 5d58b16..504a81c 100644 --- a/include/configs/mx6cuboxi.h +++ b/include/configs/mx6cuboxi.h @@ -29,6 +29,7 @@ #define CONFIG_SYS_MALLOC_LEN (2 * SZ_1M) #define CONFIG_BOARD_EARLY_INIT_F +#define CONFIG_MISC_INIT_R #define CONFIG_MXC_GPIO #define CONFIG_MXC_UART #define CONFIG_CMD_FUSE @@ -81,14 +82,12 @@ #define CONFIG_MXC_UART_BASE UART1_BASE #define CONFIG_CONSOLE_DEV "ttymxc0" #define CONFIG_MMCROOT "/dev/mmcblk0p2" -#define CONFIG_DEFAULT_FDT_FILE "imx6q-hummingboard.dtb" #define CONFIG_SYS_FSL_USDHC_NUM 1 #define CONFIG_SYS_MMC_ENV_DEV 0 /* SDHC2 */ #define CONFIG_EXTRA_ENV_SETTINGS \ "script=boot.scr\0" \ "image=zImage\0" \ - "fdt_file=" CONFIG_DEFAULT_FDT_FILE "\0" \ "fdt_addr=0x18000000\0" \ "boot_fdt=try\0" \ "ip_dyn=yes\0" \