[PATCH V2 24/24] ARM: imx8m: verdin-imx8mm: Enable USB Host support

Tim Harvey tharvey at gateworks.com
Tue Apr 27 02:01:48 CEST 2021


On Fri, Apr 23, 2021 at 8:21 AM Marek Vasut <marex at denx.de> wrote:
>
> On 4/23/21 5:04 PM, Tim Harvey wrote:
> > On Thu, Apr 22, 2021 at 12:16 PM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 4/20/21 2:33 AM, Tim Harvey wrote:
> >>
> >> [...]
> >>
> >>>> +/* USB Configs */
> >>>> +#define CONFIG_EHCI_HCD_INIT_AFTER_RESET
> >>>> +#define CONFIG_MXC_USB_PORTSC  (PORT_PTS_UTMI | PORT_PTS_PTW)
> >>>> +#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
> >>>> +
> >>>>    #endif /*_VERDIN_IMX8MM_H */
> >>>>
> >>>> --
> >>>> 2.30.2
> >>>>
> >>>
> >>> Marek,
> >>>
> >>> Thanks for your work on USB support for IMX8M!
> >>>
> >>> I'm attempting to add USB support to the venice board following this
> >>> example but I think there are still some things missing from the dt to
> >>> make this work. I find that mx6_parse_dt_adds failes; Looks like it is
> >>> required to have an alias that points to the phy but then it fails
> >>> because the phy doesn't have a reg. Also, it would see the
> >>> CONFIG_MXC_USB_PORTSC is no longer needed as that is now the default.
> >>
> >> Have a look at this patch:
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/8203ee09275c30766a1fd1cf19c60a904ee72f8c
> >>
> >> That should fix it for you.
> >>
> >> It would be good however if you could take the mx6 ventana and test the
> >> driver there, and possibly submit fixes in case the USB is still broken
> >> there. What would be even better is if you could implement the MXS PHY
> >> driver, so all the !CONFIG_PHY stuff can be removed for MX6 too. That
> >> shouldn't be much effort.
> >
> > Marek,
> >
> > That does resolve the issue with mx6_parse_dt_adds failing and works
> > for IMX8MM host mode (and likely peripheral mode) but not for otg. For
> > otg we also need something like:
> > @@ -523,7 +568,7 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> >                          plat->init_type = USB_INIT_DEVICE;
> >                  else
> >                          plat->init_type = USB_INIT_HOST;
> > -       } else if (is_mx7()) {
> > +       } else if (is_mx7() || is_imx8mm()) {
> >                  phy_status = (void __iomem *)(addr +
> >                                                USBNC_PHY_STATUS_OFFSET);
> >                  val = readl(phy_status);
> >
> > but then the issue is we hang due to the otg power domain not being
> > enabled when ehci_usb_of_to_plat is called and I'm not quite clear why
> > that is.
>
> ehci_usb_phy_mode() is called from ehci_usb_of_to_plat(), which happens
> before probe(), so the power domain at that point is off. You would have
> to move that entire code to probe() to get OTG working properly, without
> the current workaround.
>
> > Why would the power domain get probed/enabled for the usbotg2
> > bus but not the usbotg1 bus? Here is some debugging:
> > u-boot=> usb start
> > starting USB...
> > Bus usb at 32e40000: ehci_usb_phy_mode usb at 32e40000
> > usb at 32e40000 probe ret=-22
> > probe failed, error -22
> > ^^^ probe fails here because ehci_usb_phy_mode returns EINVAL for
> > dr_mode=otg but if we try to read the phy_status reg we will hang b/c
> > power domain is not enabled yet
> > Bus usb at 32e50000: imx8m_power_domain_probe gpc at 303a0000
> > imx8m_power_domain_probe pgc
> > ^^^ why did power domain get probed on the 2nd bus and not the first?
>
> I don't know, can you have a look ?

Marek,

The reg domain does not get enabled for usbotg1 because
device_of_to_plat gets called 'before' dev_power_domain_on in
device_probe.

The following will get imx8mm USB otg working:

For OTG defer setting type until probe after clock and power have been
brought up.
index 06be9deaaa..2183ae4f9d 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -523,7 +523,7 @@ static int ehci_usb_phy_mode(struct udevice *dev)
                        plat->init_type = USB_INIT_DEVICE;
                else
                        plat->init_type = USB_INIT_HOST;
-       } else if (is_mx7()) {
+       } else if (is_mx7() || is_imx8mm()) {
                phy_status = (void __iomem *)(addr +
                                              USBNC_PHY_STATUS_OFFSET);
                val = readl(phy_status);
@@ -555,7 +555,10 @@ static int ehci_usb_of_to_plat(struct udevice *dev)
                break;
        case USB_DR_MODE_OTG:
        case USB_DR_MODE_UNKNOWN:
-               return ehci_usb_phy_mode(dev);
+               if (is_imx8mm())
+                       plat->init_type = USB_INIT_HOST;
+               else
+                       return ehci_usb_phy_mode(dev);
        };

        return 0;
@@ -657,6 +660,13 @@ static int ehci_usb_probe(struct udevice *dev)
        mdelay(1);
 #endif

+       if (is_imx8mm() && (usb_get_dr_mode(dev_ofnode(dev)) ==
USB_DR_MODE_OTG)) {
+               ret = ehci_usb_phy_mode(dev);
+               if (ret)
+                       return ret;
+               priv->init_type = plat->init_type;
+       };
+
 #if CONFIG_IS_ENABLED(DM_REGULATOR)
        ret = device_get_supply_regulator(dev, "vbus-supply",
                                          &priv->vbus_supply);


>
> > imx8m_power_domain_probe power-domain at 0
> > imx8m_power_domain_probe power-domain at 3
> > imx8m_power_domain_on power-domain at 3
> > imx8m_power_domain_on power-domain at 0
> > ehci_usb_probe usb at 32e50000
> > USB EHCI 1.00
> > usb at 32e50000 probe ret=0
> > scanning bus usb at 32e50000 for devices... imx8m_power_domain_on power-domain at 3
> > imx8m_power_domain_on power-domain at 0
> > 3 USB Device(s) found
> >         scanning usb for storage devices... 0 Storage Device(s) found
> > u-boot=>
> >
> > I also verified your patch does not break USB for IMX6Q/DL ventana as well.
>
> That's good, thank you
>
> > I am still a bit concerned that the power-domain patches you merged in
> > this series are introducing bindings that are not approved upstream:
> > d78f7d8199 ARM: dts: imx8mn: Add power domain nodes
> > f0e10e33e5 ARM: dts: imx8mm: Add power domain nodes
> >
> > Are you under the impression that is what will go upstream?
>
> Pretty much, yes. And if there are some changes further down the line,
> we can pick those later when they hit upstream. I think this is better
> than having no working USB at all.
>
> > I'm still
> > waiting to see what this 'virtual power-domain' patch looks like that
> > I believe Abel or Jacky was going to work on. Perhaps you, Adam, or
> > Frieder took part in that discussion?
>
> That is still going on, yes.


More information about the U-Boot mailing list