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

[Adam Ford]

On Tue, Apr 27, 2021 at 10:50 AM Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Mon, Apr 26, 2021, 5:35 PM Marek Vasut <marex at denx.de> wrote:
> >
> > On 4/27/21 2:01 AM, Tim Harvey wrote:
> > [...]
> > >>> 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())
> >
> > Does this mean OTG doesn't work on the 8MM then ?
>
> IMX8MM USB in general still doesn't work without your:
> usb: ehci-mx6: Limit PHY address parsing to !CONFIG_PHY
>
> With your patch, IMX8MM 'host' works but 'otg' will fail probe with
> -22 (due ehci_usb_phy_mode called from of_to_plat and it not having a
> case for 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;
> > > +       };
> >
> > I have to wonder, why not move the whole OTG/Host/Device detection to
> > probe then ?
>
> Yes, I think that is the right thing to do.
>
> >
> > Also, could you submit a regular patch ?
>
> Yes, I will post patches to fix IMX8MM OTG. Can you submit your 'usb:
> ehci-mx6: Limit PHY address parsing to !CONFIG_PHY' patch so I can go
> on top of that or can I just pull that into my series?

If you want me to test it on either a Mini or Nano, feel free to CC me
as well.  I was out of town the last week, so I wasn't in a place to
do any work.
I am a little behind, so I might need some pointers to prerequisite
patches if they're necessary.

thanks,

adam

>
> Best regards,
>
> Tim