[PATCH 1/2] imx8mn_ddr4_evk: Add USB Mass Storage support

[PATCH 1/2] imx8mn_ddr4_evk: Add USB Mass Storage support

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Add USB Mass Storage support, which is a convenient way to flash
the eMMC card, for example.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 configs/imx8mn_ddr4_evk_defconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/configs/imx8mn_ddr4_evk_defconfig b/configs/imx8mn_ddr4_evk_defconfig
index 917cdb5aa9d9..065f7392b706 100644
--- a/configs/imx8mn_ddr4_evk_defconfig
+++ b/configs/imx8mn_ddr4_evk_defconfig
@@ -42,6 +42,8 @@ CONFIG_CMD_FUSE=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_USB=y
+CONFIG_CMD_USB_MASS_STORAGE=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_REGULATOR=y
 CONFIG_CMD_EXT4_WRITE=y
@@ -82,4 +84,14 @@ CONFIG_SPL_SYSRESET=y
 CONFIG_SYSRESET_PSCI=y
 CONFIG_SYSRESET_WATCHDOG=y
 CONFIG_DM_THERMAL=y
+CONFIG_USB=y
+# CONFIG_SPL_DM_USB is not set
+CONFIG_USB_EHCI_HCD=y
+CONFIG_MXC_USB_OTG_HACTIVE=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_MANUFACTURER="FSL"
+CONFIG_USB_GADGET_VENDOR_NUM=0x0525
+CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
+CONFIG_CI_UDC=y
+CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_IMX_WATCHDOG=y
-- 
2.25.1

[PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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."

Select ENV_IS_NOWHERE globally when i.MX8MN or i.MX8MP are used,
so that serial download mode can be used by default on these SoCs.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 arch/arm/mach-imx/imx8m/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

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"
-- 
2.25.1

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Marek Vasut]

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 ?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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)

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Marek Vasut]

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 ?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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.

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Michael Nazzareno Trimarchi]

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


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Marek Vasut]

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 ?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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
-- 
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

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Michael Nazzareno Trimarchi]

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



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Adam Ford]

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?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Michael Nazzareno Trimarchi]

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



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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?

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Michael Nazzareno Trimarchi]

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

-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH 2/2] imx8mn/8mp: Select ENV_IS_NOWHERE

[Fabio Estevam]

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.