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

[Adam Ford]

On Thu, Jan 13, 2022 at 6:35 PM Adam Ford <aford173 at gmail.com> wrote:
>
> On Wed, Jan 12, 2022 at 2:17 AM Michael Walle <michael at walle.cc> wrote:
> >
> > Am 2022-01-11 17:52, schrieb Adam Ford:
> > > On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael at walle.cc> wrote:
> > >>
> > >> Hi,
> > >>
> > >> Am 2022-01-11 15:20, schrieb Adam Ford:
> > >> > On Tue, Jan 4, 2022 at 2:32 AM Michael Walle <michael at walle.cc> 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.
> > >> >> >
> > >> >> > The init_type in both priv and plat data are the same, so it doesn't
> > >> >> > make sense to configure the data in the plat data and copy the
> > >> >> > data to priv when priv can be configured directly.  Instead, rename
> > >> >> > ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the
> > >> >> > probe functions after the clocks are enabled, but before the data
> > >> >> > is required.
> > >> >> >
> > >> >> > With that added, the additional checks for imx8mm and imx8mn will
> > >> >> > allow reading the register to automatically determine the state
> > >> >> > (host or device) of the OTG controller.
> > >> >> >
> > >> >> > Signed-off-by: Adam Ford <aford173 at gmail.com>
> > >> >> > ---
> > >> >> > 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..f2a34b7f06 100644
> > >> >> > --- a/drivers/usb/host/ehci-mx6.c
> > >> >> > +++ b/drivers/usb/host/ehci-mx6.c
> > >> >>
> > >> >> ..
> > >> >>
> > >> >> > @@ -639,10 +639,8 @@ static int mx6_parse_dt_addrs(struct udevice *dev)
> > >> >> >
> > >> >> >  static int ehci_usb_probe(struct udevice *dev)
> > >> >> >  {
> > >> >> > -     struct usb_plat *plat = dev_get_plat(dev);
> > >> >> >       struct usb_ehci *ehci = dev_read_addr_ptr(dev);
> > >> >> >       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
> > >> >> > -     enum usb_init_type type = plat->init_type;
> > >> >> >       struct ehci_hccr *hccr;
> > >> >> >       struct ehci_hcor *hcor;
> > >> >> >       int ret;
> > >> >> > @@ -660,8 +658,6 @@ static int ehci_usb_probe(struct udevice *dev)
> > >> >> >               return ret;
> > >> >> >
> > >> >> >       priv->ehci = ehci;
> > >> >> > -     priv->init_type = type;
> > >> >>
> > >> >
> > >> > Michael,
> > >> >
> > >> >> I'm not sure this is correct. There is also this:
> > >> >> https://elixir.bootlin.com/u-boot/v2022.01-rc4/source/drivers/usb/host/usb-uclass.c#L407
> > >> >>
> > >> >
> > >> > Further down in the code, you should see:
> > >> >     priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
> > >>
> > >> But that is just fetching the mode from the device tree, the
> > >> usb_setup_ehci_gadget() referenced above, change the mode during
> > >> runtime by writing the plat->init_type and reprobing the device.
> > >>
> > >> >> Which won't work anymore. usb_setup_ehci_gadget() is called from
> > >> >> usb_gadget_register_driver() in ci_udc.c. The latter is the one used
> > >> >> on the imx, right? But I might be wrong. I'm still trying to figure
> > >> >> out how this all works together, because I also try to get OTG
> > >> >> running on the dwc3 driver. It looks like the ci_udc.c is special
> > >> >> here, and I wonder how a transition to UCLASS_USB_GADGET_GENERIC
> > >> >> might look like for this driver.
> > >> >
> > >> > This driver really isn't changing anything, it's just an order of
> > >> > operations.
> > >>
> > >> It doesn't use the plat->init_type at all anymore, no?
> > >
> > > From what I could tell, the only thing that plat->init_type did was to
> > > set priv->init_type.
> > > The priv structure appears to do most of the work.
> >
> > but plat->init_type seems to change during runtime (by
> > usb_setup_ehci_gadget()) and with this patch applied, it doesn't
> > do that anymore.
> >
> > >> >  Previously there was a separate that was being called to
> > >> > determine the init_type as being either a peripheral or host.  If it
> > >> > wasn't explicitly set in the device tree, the helper function would
> > >> > query a register, however, on the imx8mm and imx8mn platforms, the
> > >> > clocks were not running which resulted in a hang.  What I've done is
> > >> > simply move the helper function into the probe and have it run after
> > >> > the clocks start.  In theory the register values will be the same as
> > >> > they would be with the help function still in place.
> > >> >
> > >> > Both Tim Harvey and I have tested on an imx8mm and Fabio tested it on
> > >> > an imx7.  For me, the peripheral mode worked when the ID pin of the
> > >> > USB-OTG was not grounded.  When it was grounded, the system corrected
> > >> > identified it as a host and I was able to mount a USB drive.
> > >>
> > >> Again, I'm missing something here, but the only user of
> > >> usb_setup_ehci_gadget() is usb/gadget/ci_udc.c.
> > >
> > > I can't speak to what those functions do.   What are you suggesting
> > > that I do differently?
> >
> > I'd start by seeing if/when usb_setup_ehci_gadget() is called and
> > see if plat->init_type changes. If its not called, is it dead code
> > then?
>
> I commented out the call to usb_setup_ehci_gadget(), and the gadget
> driver still enumerated.
> I'll push a V2 to remove it along with some subsequent patches to
> update the defconfig, so others who want to use/test it don't have to
> guess what config options are necessary.
>
> Thanks for catching that.
>

Simon,

i went go to remove usb_setup_ehci_gadget(), but the header file has a
TODO with your name on it, but the ci_udc river appears to be the only
function to call it, and from what I can tell, the CI-UDC driver
doesn't need this.

/**
 * usb_setup_ehci_gadget() - Set up a USB device as a gadget
 *
 * TODO(sjg at chromium.org): Tidy this up when USB gadgets can use driver model
 *
 * This provides a way to tell a controller to start up as a USB device
 * instead of as a host. It is untested.
 */

If I remove the function call from the ci_udc.c driver, do you want
this left in the header?  As it's written, the usb_setup_ehci_gadget()
appears to attempt to override the plat->init_type, but this change is
eliminating the need for the plat structure since it is pointless.
>From what I can tell the function is trying to force a re-probe with
the plat->init_type redefined as a peripheral, but the code I want to
use auto-detects the host/peripheral mode, rendering this pointless.

adam

> adam
>
> > Sorry I just noticed this, while reviewing how this particular driver
> > works, because I still like to find a working OTG driver in u-boot.
> > I don't have any imx boards.
> >
> > -michael