[PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn

[Adam Ford]

On Thu, Feb 3, 2022 at 2:59 PM Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Tue, Jan 25, 2022 at 12:44 AM Marcel Ziswiler
> <marcel.ziswiler at toradex.com> wrote:
> >
> > On Mon, 2022-01-17 at 18:21 -0600, Adam Ford wrote:
> > > The imx8mm and imx8mn appear compatible with imx7d-usb
> > > flags in the OTG driver.  If the dr_mode is defined as
> > > host or peripheral, the device appears to operate correctly,
> > > however the auto host/peripheral detection results in an error.
> > >
> > > The solution isn't just adding checks for imx8mm and imx8mn to
> > > the check for imx7, because the USB clock needs to be running
> > > to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
> > >
> > > Marek requested that I not enable the clocks in ehci_usb_of_to_plat,
> > > so I modified that function to return an unknown state if the
> > > device tree does not explicitly state whether it is a host
> > > or a peripheral.
> > >
> > > When the driver probes, it looks to see if it's in the unknown
> > > state, and only then will it read the register to auto-detect.
> > >
> > > Signed-off-by: Adam Ford <aford173 at gmail.com>
> > > ---
> > > V3:  Keep ehci_usb_of_to_plat but add the ability to return
> > >      and unknown state instead of reading the register.
> > >      If the probe determines the states is unknown, it will
> > >      query the register after the clocks have been enabled.
> > >      Because of the slight behavior change, I removed any
> > >      review or tested tags.
> > >
> > > V2:  Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it
> > >      from the probe after the clocks are enabled, but before
> > >      the data is needed.
> > >
> > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > > index 1bd6147c76..cf44e53ff7 100644
> > > --- a/drivers/usb/host/ehci-mx6.c
> > > +++ b/drivers/usb/host/ehci-mx6.c
> > > @@ -543,7 +543,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() || is_imx8mn()) {
> > >                 phy_status = (void __iomem *)(addr +
> > >                                               USBNC_PHY_STATUS_OFFSET);
> > >                 val = readl(phy_status);
> > > @@ -573,9 +573,8 @@ static int ehci_usb_of_to_plat(struct udevice *dev)
> > >         case USB_DR_MODE_PERIPHERAL:
> > >                 plat->init_type = USB_INIT_DEVICE;
> > >                 break;
> > > -       case USB_DR_MODE_OTG:
> > > -       case USB_DR_MODE_UNKNOWN:
> > > -               return ehci_usb_phy_mode(dev);
> > > +       default:
> > > +               plat->init_type = USB_INIT_UNKNOWN;
> > >         };
> > >
> > >         return 0;
> > > @@ -677,6 +676,20 @@ static int ehci_usb_probe(struct udevice *dev)
> > >         mdelay(1);
> > >  #endif
> > >
> > > +       /*
> > > +        * If the device tree didn't specify host or device,
> > > +        * the default is USB_INIT_UNKNOWN, so we need to check
> > > +        * the register. For imx8mm and imx8mn, the clocks need to be
> > > +        * running first, so we defer the check until they are.
> > > +        */
> > > +       if (priv->init_type == USB_INIT_UNKNOWN) {
> > > +               ret = ehci_usb_phy_mode(dev);
> > > +               if (ret)
> > > +                       goto err_clk;
> >
> > Hm, unfortunately, that label is gated by an ifdef leading to the following on colibri_imx7:
> >
> > drivers/usb/host/ehci-mx6.c: In function ‘ehci_usb_probe’:
> > drivers/usb/host/ehci-mx6.c:688:4: error: label ‘err_clk’ used but not defined
> >   688 |    goto err_clk;
> >       |    ^~~~
> > make[1]: *** [scripts/Makefile.build:254: drivers/usb/host/ehci-mx6.o] Error 1
> >
> > I have to admit that maybe we should indeed run it with DM_REGULATOR but that won't change anything on this
> > issue. I guess one should really not have any gotos to a label err_clk being ifdefed from somewhere not
> > ifdefed.
> >
>
> Adam,
>
> Can you re-submit this with an ifdef to handle the correct goto when
> DM_REGULATOR is undefined?
>
> It looks like Fabio figured out his issue with his board and it was
> unrelated to this (fixed by 'mmc: fsl_esdhc_imx: fix watermark level
> in dma') so I think Marcel's point is the only blocker on this patch
> unless there is more discussion needed with Michael or some feedback
> needed from Marek?

Tim,
Not a problem. I was waiting for feedback from Marek in case there
were more issues, but I can fix the goto reference and resubmit.  I'm
build-testing the colibri_imx7 now.

I'm just moving the 'err_clk' reference out of the #ifdef so the same
reference can be used.  It doesn't look like it should be inside the
#ifdef anyway.

You should see something shortly, and I'll add you to the CC list.

adam

>
> Best regards,
>
> Tim