| Message ID | 20220420210757.1184891-2-festevam@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [1/2] imx8mn_ddr4_evk: Add USB Mass Storage support | expand |
On 4/20/22 23:07, Fabio Estevam wrote: > From: Fabio Estevam <festevam@denx.de> > > Currently, on i.MX8MN/i.MX8MP (Bootrom version 2) it is not possible to > load U-Boot via serial download mode, unless CONFIG_ENV_IS_NOWHERE is > selected. > > This was noticed before by Adam Ford and fixed on the imx8mn beacon > board in commit 2c7ebf7778cf ("imx: imx8mn_beacon: Fix USB booting"). > > Such commit log states: > > "The i.MX8M Nano can boot over USB using the boot ROM instead of > adding extra code to SPL to support USB drivers, etc. However, > when booting from USB, the environment doesnt' know where to load > and causes a hang. Fix this hang by supporting CONFIG_ENV_IS_NOWHERE=y. > It only falls back to this condition when booting from USB, so it > does not impact MMC booting." I suspect this happens because arch/arm/mach-imx/imx8m/soc.c env_get_location() contains " ... default: return ENVL_NOWHERE; " right ? I wonder what would happen if you were to add: case USB_BOOT: return ENVL_UNKNOWN; Maybe that would even work without CONFIG_ENV_IS_NOWHERE=y ?
Hi Marek, On Wed, Apr 20, 2022 at 6:42 PM Marek Vasut <marex@denx.de> wrote: > I suspect this happens because > > arch/arm/mach-imx/imx8m/soc.c env_get_location() > > contains > " > ... > default: > return ENVL_NOWHERE; > " > > right ? > > I wonder what would happen if you were to add: > > case USB_BOOT: > return ENVL_UNKNOWN; > > Maybe that would even work without CONFIG_ENV_IS_NOWHERE=y ? Unfortunately, with this change the boot via SDP does not complete: U-Boot SPL 2022.04-00857-g0f0a48bd2df4-dirty (Apr 20 2022 - 18:57:00 -0300) SEC0: RNG instantiated Normal Boot WDT: Started watchdog@30280000 with servicing (60s timeout) Trying to boot from BOOTROM Find img info 0x&48020a00, size 872 Need continue download 1024 Download 762080, Total size 763616 NOTICE: BL31: v2.2(release):rel_imx_5.4.47_2.2.0-0-gc949a888e909 NOTICE: BL31: Built : 16:07:45, Apr 20 2022 (it hangs here)
On 4/21/22 00:03, Fabio Estevam wrote: > Hi Marek, Hi, > On Wed, Apr 20, 2022 at 6:42 PM Marek Vasut <marex@denx.de> wrote: > >> I suspect this happens because >> >> arch/arm/mach-imx/imx8m/soc.c env_get_location() >> >> contains >> " >> ... >> default: >> return ENVL_NOWHERE; >> " >> >> right ? >> >> I wonder what would happen if you were to add: >> >> case USB_BOOT: >> return ENVL_UNKNOWN; >> >> Maybe that would even work without CONFIG_ENV_IS_NOWHERE=y ? > > Unfortunately, with this change the boot via SDP does not complete: Did you actually hit the USB_BOOT case or did you fall into the default: case ?
Hi Marek, On Wed, Apr 20, 2022 at 7:15 PM Marek Vasut <marex@denx.de> wrote: > Did you actually hit the USB_BOOT case or did you fall into the default: > case ? It does hit the USB_BOOT case. I also tested forcing to return ENVL_UNKNOWN: --- a/arch/arm/mach-imx/imx8m/soc.c +++ b/arch/arm/mach-imx/imx8m/soc.c @@ -1531,6 +1531,7 @@ void do_error(struct pt_regs *pt_regs) enum env_location env_get_location(enum env_operation op, int prio) { enum boot_device dev = get_boot_device(); + return ENVL_UNKNOWN; if (prio) return ENVL_UNKNOWN; but still, the boot does not complete.
Hi Fabio On Thu, Apr 21, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Marek, > > On Wed, Apr 20, 2022 at 7:15 PM Marek Vasut <marex@denx.de> wrote: > > > Did you actually hit the USB_BOOT case or did you fall into the default: > > case ? > > It does hit the USB_BOOT case. > > I also tested forcing to return ENVL_UNKNOWN: > > --- a/arch/arm/mach-imx/imx8m/soc.c > +++ b/arch/arm/mach-imx/imx8m/soc.c > @@ -1531,6 +1531,7 @@ void do_error(struct pt_regs *pt_regs) > enum env_location env_get_location(enum env_operation op, int prio) > { > enum boot_device dev = get_boot_device(); > + return ENVL_UNKNOWN; > > if (prio) > return ENVL_UNKNOWN; > > but still, the boot does not complete. You need only add USB_ case in MMC and NAND #ifdef CONFIG_ENV_IS_IN_NAND /* add */ case USB_BOOT: case NAND_BOOT: env_loc = ENVL_NAND; break; #endif #ifdef CONFIG_ENV_IS_IN_MMC /* add */ case USB_BOOT: case SD1_BOOT: case SD2_BOOT: case SD3_BOOT: case MMC1_BOOT: case MMC2_BOOT: case MMC3_BOOT: env_loc = ENVL_MMC; break; #endif Michael
On 4/21/22 16:17, Fabio Estevam wrote: > Hi Marek, > > On Wed, Apr 20, 2022 at 7:15 PM Marek Vasut <marex@denx.de> wrote: > >> Did you actually hit the USB_BOOT case or did you fall into the default: >> case ? > > It does hit the USB_BOOT case. > > I also tested forcing to return ENVL_UNKNOWN: Hum, sigh. Can you check where exactly this hangs ? I speculate env_select() in env/env.c returns -ENODEV and then whatever calls env_select() ends up in hang(), no ?
Hi Michael, On 21/04/2022 11:34, Michael Nazzareno Trimarchi wrote: > You need only add USB_ case in MMC and NAND > #ifdef CONFIG_ENV_IS_IN_NAND > /* add */ > case USB_BOOT: > case NAND_BOOT: > env_loc = ENVL_NAND; > break; > #endif > #ifdef CONFIG_ENV_IS_IN_MMC > /* add */ > case USB_BOOT: > case SD1_BOOT: > case SD2_BOOT: > case SD3_BOOT: > case MMC1_BOOT: > case MMC2_BOOT: > case MMC3_BOOT: > env_loc = ENVL_MMC; > break; > #endif Your suggestion works, thanks. I will prepare v2. Thanks, Fabio Estevam
Hi Fabio On Thu, Apr 21, 2022 at 6:19 PM Fabio Estevam <festevam@denx.de> wrote: > > Hi Michael, > > On 21/04/2022 11:34, Michael Nazzareno Trimarchi wrote: > > > You need only add USB_ case in MMC and NAND > > #ifdef CONFIG_ENV_IS_IN_NAND > > /* add */ > > case USB_BOOT: > > case NAND_BOOT: > > env_loc = ENVL_NAND; > > break; > > #endif > > #ifdef CONFIG_ENV_IS_IN_MMC > > /* add */ > > case USB_BOOT: > > case SD1_BOOT: > > case SD2_BOOT: > > case SD3_BOOT: > > case MMC1_BOOT: > > case MMC2_BOOT: > > case MMC3_BOOT: > > env_loc = ENVL_MMC; > > break; > > #endif > > Your suggestion works, thanks. > > I will prepare v2. That function should drop. There is not other architecture that does it. What about: register_enviroment_hook() deregister_enviroment_hook() This can replace the one default with board/arch etc Michael > > Thanks, > > Fabio Estevam > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-60 Fax: (+49)-8142-66989-80 Email: > festevam@denx.de
Hi Michael, On 21/04/2022 13:34, Michael Nazzareno Trimarchi wrote: > That function should drop. There is not other architecture that does > it. What about: I implemented your suggestion like this: --- a/arch/arm/mach-imx/imx8m/soc.c +++ b/arch/arm/mach-imx/imx8m/soc.c @@ -1536,6 +1536,14 @@ enum env_location arch_env_get_location(enum env_operation op, int prio) return ENVL_UNKNOWN; switch (dev) { + case USB_BOOT: + if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) + return ENVL_SPI_FLASH; + if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) + return ENVL_NAND; + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) + return ENVL_MMC; + return ENVL_NOWHERE; case QSPI_BOOT: case SPI_NOR_BOOT: if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) This still allows me to save the env into the eMMC when U-Boot is loaded from USB. > register_enviroment_hook() > deregister_enviroment_hook() > > This can replace the one default with board/arch etc Marek submitted these patches: https://source.denx.de/u-boot/u-boot/-/commit/de70e8879bb253f4d2a9ba9149cd41cb38b94ed8 https://source.denx.de/u-boot/u-boot/-/commit/e4dc2d0620851d6e0e784d4ef0a50f26e1e73857 Is this the mechanism that you are looking for?
On Thu, Apr 21, 2022 at 11:47 AM Fabio Estevam <festevam@denx.de> wrote: > > Hi Michael, > > On 21/04/2022 13:34, Michael Nazzareno Trimarchi wrote: > > > That function should drop. There is not other architecture that does > > it. What about: > > I implemented your suggestion like this: > > --- a/arch/arm/mach-imx/imx8m/soc.c > +++ b/arch/arm/mach-imx/imx8m/soc.c > @@ -1536,6 +1536,14 @@ enum env_location arch_env_get_location(enum > env_operation op, int prio) > return ENVL_UNKNOWN; > > switch (dev) { > + case USB_BOOT: > + if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > + return ENVL_SPI_FLASH; > + if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) > + return ENVL_NAND; > + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > + return ENVL_MMC; > + return ENVL_NOWHERE; > case QSPI_BOOT: > case SPI_NOR_BOOT: > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > > This still allows me to save the env into the eMMC when U-Boot > is loaded from USB. > > > register_enviroment_hook() > > deregister_enviroment_hook() > > > > This can replace the one default with board/arch etc > > Marek submitted these patches: > > https://source.denx.de/u-boot/u-boot/-/commit/de70e8879bb253f4d2a9ba9149cd41cb38b94ed8 > > https://source.denx.de/u-boot/u-boot/-/commit/e4dc2d0620851d6e0e784d4ef0a50f26e1e73857 > I believe those were applied today. I haven't verified it yet, but I think I saw the e-mail go by. > Is this the mechanism that you are looking for?
Hi On Thu, Apr 21, 2022 at 6:51 PM Adam Ford <aford173@gmail.com> wrote: > > On Thu, Apr 21, 2022 at 11:47 AM Fabio Estevam <festevam@denx.de> wrote: > > > > Hi Michael, > > > > On 21/04/2022 13:34, Michael Nazzareno Trimarchi wrote: > > > > > That function should drop. There is not other architecture that does > > > it. What about: > > > > I implemented your suggestion like this: > > > > --- a/arch/arm/mach-imx/imx8m/soc.c > > +++ b/arch/arm/mach-imx/imx8m/soc.c > > @@ -1536,6 +1536,14 @@ enum env_location arch_env_get_location(enum > > env_operation op, int prio) > > return ENVL_UNKNOWN; > > > > switch (dev) { > > + case USB_BOOT: > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > > + return ENVL_SPI_FLASH; > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) > > + return ENVL_NAND; > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > > + return ENVL_MMC; > > + return ENVL_NOWHERE; > > case QSPI_BOOT: > > case SPI_NOR_BOOT: > > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > > > > This still allows me to save the env into the eMMC when U-Boot > > is loaded from USB. > > > > > register_enviroment_hook() > > > deregister_enviroment_hook() > > > > > > This can replace the one default with board/arch etc > > > > Marek submitted these patches: > > > > https://source.denx.de/u-boot/u-boot/-/commit/de70e8879bb253f4d2a9ba9149cd41cb38b94ed8 > > > > https://source.denx.de/u-boot/u-boot/-/commit/e4dc2d0620851d6e0e784d4ef0a50f26e1e73857 > > > > I believe those were applied today. I haven't verified it yet, but I > think I saw the e-mail go by. > > > Is this the mechanism that you are looking for? I have seen those patches already but the cost of a function pointer and a call is more readable. If they get applied, I will be fine with them ;) Michael
On 21/04/2022 14:01, Michael Nazzareno Trimarchi wrote: > I have seen those patches already but the cost of a function pointer > and a call is more readable. If they get applied, I will be fine with > them ;) They are already in master. How do we proceed to fix the U-Boot load via USB?
Hi On Thu, Apr 21, 2022 at 7:03 PM Fabio Estevam <festevam@denx.de> wrote: > > On 21/04/2022 14:01, Michael Nazzareno Trimarchi wrote: > > > I have seen those patches already but the cost of a function pointer > > and a call is more readable. If they get applied, I will be fine with > > them ;) > > They are already in master. > > How do we proceed to fix the U-Boot load via USB? The uboot has no problem, the problem is that the function is wrong ;). When you boot from USB you should inform where to pick the environment. You can force ENV_EVERYWHERE for those architectures, you can decide to change the switch as I said, or you can add in your board and override at board level the function ;) Michael
On 21/04/2022 14:10, Michael Nazzareno Trimarchi wrote: > The uboot has no problem, the problem is that the function is wrong ;). > When you boot from USB you should inform where to pick the environment. > You can force ENV_EVERYWHERE for those architectures, you can decide > to change the switch as I said, or you can add in your board and > override I will send a patch based on your suggestion, thanks.
diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig index 55db25062a9b..ba7328aec8a1 100644 --- a/arch/arm/mach-imx/imx8m/Kconfig +++ b/arch/arm/mach-imx/imx8m/Kconfig @@ -16,10 +16,12 @@ config IMX8MM config IMX8MN bool select IMX8M + select ENV_IS_NOWHERE config IMX8MP bool select IMX8M + select ENV_IS_NOWHERE config SYS_SOC default "imx8m"