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

[Adam Ford]

On Mon, Feb 7, 2022 at 5:00 AM Michael Walle <michael at walle.cc> wrote:
>
> Hi Adam,
>
> it's nice to include people who made review comments in the follow-up
> patches. I had to pull this out of the mailinglist again.

Sorry.  I didn't purposefully leave you out.  I have a little script I
run to get the list of people involved, and I forgot to manually add
you back in.

>
> > 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>
> > Tested-by: Tim Harvey <tharvey at gateworks.com>
> > ---
> > V4:  Fix 'err_clk' reference, so it is not encapsulated in
> >      an ifdef making it available at all times.
> > 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 t
> >
> > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > index 1bd6147c76..060b02accc 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:
>
>
> Don't you want to propagate the OTG mode to the init_type field? Maybe
> it will be useful sometime and IMHO makes it easier to read the code...

I was thinking the OTG case was included with the default, since this
function is  really only looking for whether or not we're host or
peripheral.  Every other case is considered unknown since we have to
defer the reading of the USB state until later.

adam
>
> > -             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) {
>
> Because this implicitly also contains OTG mode, right? So despite
> the comment, if the device tree includes OTG mode, this branch is
> also taken.

Yes, because the device tree didn't specify host or peripheral, all
other instances are assumed to be OTG and deferred until we get here.

>
> Otherwise looks good to me:
>
> Reviewed-by: Michael Walle <michael at walle.cc>
>
> > +             ret = ehci_usb_phy_mode(dev);
> > +             if (ret)
> > +                     goto err_clk;
> > +             else
> > +                     priv->init_type = plat->init_type;
> > +     }
> > +
> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
> >       ret = device_get_supply_regulator(dev, "vbus-supply",
> >                                         &priv->vbus_supply);
> > @@ -741,8 +754,8 @@ err_regulator:
> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
> >       if (priv->vbus_supply)
> >               regulator_set_enable(priv->vbus_supply, false);
> > -err_clk:
> >  #endif
> > +err_clk:
> >  #if CONFIG_IS_ENABLED(CLK)
> >       clk_disable(&priv->clk);
> >  #else
> > diff --git a/include/usb.h b/include/usb.h
> > index f032de8af9..7e3796bd5b 100644
> > --- a/include/usb.h
> > +++ b/include/usb.h
> > @@ -163,7 +163,8 @@ struct int_queue;
> >   */
> >  enum usb_init_type {
> >       USB_INIT_HOST,
> > -     USB_INIT_DEVICE
> > +     USB_INIT_DEVICE,
> > +     USB_INIT_UNKNOWN,
> >  };
> >
> >  /**********************************************************************