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

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

[Adam Ford]

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@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
@@ -513,7 +513,7 @@ static const struct ehci_ops mx6_ehci_ops = {
 
 static int ehci_usb_phy_mode(struct udevice *dev)
 {
-	struct usb_plat *plat = dev_get_plat(dev);
+	struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
 	void *__iomem addr = dev_read_addr_ptr(dev);
 	void *__iomem phy_ctrl, *__iomem phy_status;
 	const void *blob = gd->fdt_blob;
@@ -540,18 +540,18 @@ static int ehci_usb_phy_mode(struct udevice *dev)
 		val = readl(phy_ctrl);
 
 		if (val & USBPHY_CTRL_OTG_ID)
-			plat->init_type = USB_INIT_DEVICE;
+			priv->init_type = USB_INIT_DEVICE;
 		else
-			plat->init_type = USB_INIT_HOST;
-	} else if (is_mx7()) {
+			priv->init_type = USB_INIT_HOST;
+	} else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
 		phy_status = (void __iomem *)(addr +
 					      USBNC_PHY_STATUS_OFFSET);
 		val = readl(phy_status);
 
 		if (val & USBNC_PHYSTATUS_ID_DIG)
-			plat->init_type = USB_INIT_DEVICE;
+			priv->init_type = USB_INIT_DEVICE;
 		else
-			plat->init_type = USB_INIT_HOST;
+			priv->init_type = USB_INIT_HOST;
 	} else {
 		return -EINVAL;
 	}
@@ -559,19 +559,19 @@ static int ehci_usb_phy_mode(struct udevice *dev)
 	return 0;
 }
 
-static int ehci_usb_of_to_plat(struct udevice *dev)
+static int ehci_usb_dr_mode(struct udevice *dev)
 {
-	struct usb_plat *plat = dev_get_plat(dev);
+	struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
 	enum usb_dr_mode dr_mode;
 
 	dr_mode = usb_get_dr_mode(dev_ofnode(dev));
 
 	switch (dr_mode) {
 	case USB_DR_MODE_HOST:
-		plat->init_type = USB_INIT_HOST;
+		priv->init_type = USB_INIT_HOST;
 		break;
 	case USB_DR_MODE_PERIPHERAL:
-		plat->init_type = USB_INIT_DEVICE;
+		priv->init_type = USB_INIT_DEVICE;
 		break;
 	case USB_DR_MODE_OTG:
 	case USB_DR_MODE_UNKNOWN:
@@ -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;
-	priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
 
 #if CONFIG_IS_ENABLED(CLK)
 	ret = clk_get_by_index(dev, 0, &priv->clk);
@@ -677,6 +673,11 @@ static int ehci_usb_probe(struct udevice *dev)
 	mdelay(1);
 #endif
 
+	ret = ehci_usb_dr_mode(dev);
+	if (ret)
+		goto err_clk;
+	priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
+
 #if CONFIG_IS_ENABLED(DM_REGULATOR)
 	ret = device_get_supply_regulator(dev, "vbus-supply",
 					  &priv->vbus_supply);
@@ -700,7 +701,7 @@ static int ehci_usb_probe(struct udevice *dev)
 #if CONFIG_IS_ENABLED(DM_REGULATOR)
 	if (priv->vbus_supply) {
 		ret = regulator_set_enable(priv->vbus_supply,
-					   (type == USB_INIT_DEVICE) ?
+					   (priv->init_type == USB_INIT_DEVICE) ?
 					   false : true);
 		if (ret && ret != -ENOSYS) {
 			printf("Error enabling VBUS supply (ret=%i)\n", ret);
@@ -785,11 +786,9 @@ U_BOOT_DRIVER(usb_mx6) = {
 	.name	= "ehci_mx6",
 	.id	= UCLASS_USB,
 	.of_match = mx6_usb_ids,
-	.of_to_plat = ehci_usb_of_to_plat,
 	.probe	= ehci_usb_probe,
 	.remove = ehci_usb_remove,
 	.ops	= &ehci_usb_ops,
-	.plat_auto	= sizeof(struct usb_plat),
 	.priv_auto	= sizeof(struct ehci_mx6_priv_data),
 	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
 };
-- 
2.32.0

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

[Fabio Estevam]

Hi Adam,

On Thu, Dec 23, 2021 at 11:08 AM Adam Ford <aford173@gmail.com> 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@gmail.com>

Tested "ums 0 mmc 0" on a imx7s-warp. It still works fine:

Tested-by: Fabio Estevam <festevam@gmail.com>

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

[Tim Harvey]

On Thu, Dec 23, 2021 at 6:08 AM Adam Ford <aford173@gmail.com> 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@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
> @@ -513,7 +513,7 @@ static const struct ehci_ops mx6_ehci_ops = {
>
>  static int ehci_usb_phy_mode(struct udevice *dev)
>  {
> -       struct usb_plat *plat = dev_get_plat(dev);
> +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
>         void *__iomem addr = dev_read_addr_ptr(dev);
>         void *__iomem phy_ctrl, *__iomem phy_status;
>         const void *blob = gd->fdt_blob;
> @@ -540,18 +540,18 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>                 val = readl(phy_ctrl);
>
>                 if (val & USBPHY_CTRL_OTG_ID)
> -                       plat->init_type = USB_INIT_DEVICE;
> +                       priv->init_type = USB_INIT_DEVICE;
>                 else
> -                       plat->init_type = USB_INIT_HOST;
> -       } else if (is_mx7()) {
> +                       priv->init_type = USB_INIT_HOST;
> +       } else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
>                 phy_status = (void __iomem *)(addr +
>                                               USBNC_PHY_STATUS_OFFSET);
>                 val = readl(phy_status);
>
>                 if (val & USBNC_PHYSTATUS_ID_DIG)
> -                       plat->init_type = USB_INIT_DEVICE;
> +                       priv->init_type = USB_INIT_DEVICE;
>                 else
> -                       plat->init_type = USB_INIT_HOST;
> +                       priv->init_type = USB_INIT_HOST;
>         } else {
>                 return -EINVAL;
>         }
> @@ -559,19 +559,19 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>         return 0;
>  }
>
> -static int ehci_usb_of_to_plat(struct udevice *dev)
> +static int ehci_usb_dr_mode(struct udevice *dev)
>  {
> -       struct usb_plat *plat = dev_get_plat(dev);
> +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
>         enum usb_dr_mode dr_mode;
>
>         dr_mode = usb_get_dr_mode(dev_ofnode(dev));
>
>         switch (dr_mode) {
>         case USB_DR_MODE_HOST:
> -               plat->init_type = USB_INIT_HOST;
> +               priv->init_type = USB_INIT_HOST;
>                 break;
>         case USB_DR_MODE_PERIPHERAL:
> -               plat->init_type = USB_INIT_DEVICE;
> +               priv->init_type = USB_INIT_DEVICE;
>                 break;
>         case USB_DR_MODE_OTG:
>         case USB_DR_MODE_UNKNOWN:
> @@ -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;
> -       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
>
>  #if CONFIG_IS_ENABLED(CLK)
>         ret = clk_get_by_index(dev, 0, &priv->clk);
> @@ -677,6 +673,11 @@ static int ehci_usb_probe(struct udevice *dev)
>         mdelay(1);
>  #endif
>
> +       ret = ehci_usb_dr_mode(dev);
> +       if (ret)
> +               goto err_clk;
> +       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
> +
>  #if CONFIG_IS_ENABLED(DM_REGULATOR)
>         ret = device_get_supply_regulator(dev, "vbus-supply",
>                                           &priv->vbus_supply);
> @@ -700,7 +701,7 @@ static int ehci_usb_probe(struct udevice *dev)
>  #if CONFIG_IS_ENABLED(DM_REGULATOR)
>         if (priv->vbus_supply) {
>                 ret = regulator_set_enable(priv->vbus_supply,
> -                                          (type == USB_INIT_DEVICE) ?
> +                                          (priv->init_type == USB_INIT_DEVICE) ?
>                                            false : true);
>                 if (ret && ret != -ENOSYS) {
>                         printf("Error enabling VBUS supply (ret=%i)\n", ret);
> @@ -785,11 +786,9 @@ U_BOOT_DRIVER(usb_mx6) = {
>         .name   = "ehci_mx6",
>         .id     = UCLASS_USB,
>         .of_match = mx6_usb_ids,
> -       .of_to_plat = ehci_usb_of_to_plat,
>         .probe  = ehci_usb_probe,
>         .remove = ehci_usb_remove,
>         .ops    = &ehci_usb_ops,
> -       .plat_auto      = sizeof(struct usb_plat),
>         .priv_auto      = sizeof(struct ehci_mx6_priv_data),
>         .flags  = DM_FLAG_ALLOC_PRIV_DMA,
>  };
> --
> 2.32.0
>

Adam,

How are you testing this for IMX8MM or IMX8MN?

Best Regards,

Tim

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

[Adam Ford]

On Mon, Jan 3, 2022 at 10:20 PM Tim Harvey <tharvey@gateworks.com> wrote:

> On Thu, Dec 23, 2021 at 6:08 AM Adam Ford <aford173@gmail.com> 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@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
> > @@ -513,7 +513,7 @@ static const struct ehci_ops mx6_ehci_ops = {
> >
> >  static int ehci_usb_phy_mode(struct udevice *dev)
> >  {
> > -       struct usb_plat *plat = dev_get_plat(dev);
> > +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
> >         void *__iomem addr = dev_read_addr_ptr(dev);
> >         void *__iomem phy_ctrl, *__iomem phy_status;
> >         const void *blob = gd->fdt_blob;
> > @@ -540,18 +540,18 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> >                 val = readl(phy_ctrl);
> >
> >                 if (val & USBPHY_CTRL_OTG_ID)
> > -                       plat->init_type = USB_INIT_DEVICE;
> > +                       priv->init_type = USB_INIT_DEVICE;
> >                 else
> > -                       plat->init_type = USB_INIT_HOST;
> > -       } else if (is_mx7()) {
> > +                       priv->init_type = USB_INIT_HOST;
> > +       } else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
> >                 phy_status = (void __iomem *)(addr +
> >                                               USBNC_PHY_STATUS_OFFSET);
> >                 val = readl(phy_status);
> >
> >                 if (val & USBNC_PHYSTATUS_ID_DIG)
> > -                       plat->init_type = USB_INIT_DEVICE;
> > +                       priv->init_type = USB_INIT_DEVICE;
> >                 else
> > -                       plat->init_type = USB_INIT_HOST;
> > +                       priv->init_type = USB_INIT_HOST;
> >         } else {
> >                 return -EINVAL;
> >         }
> > @@ -559,19 +559,19 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> >         return 0;
> >  }
> >
> > -static int ehci_usb_of_to_plat(struct udevice *dev)
> > +static int ehci_usb_dr_mode(struct udevice *dev)
> >  {
> > -       struct usb_plat *plat = dev_get_plat(dev);
> > +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
> >         enum usb_dr_mode dr_mode;
> >
> >         dr_mode = usb_get_dr_mode(dev_ofnode(dev));
> >
> >         switch (dr_mode) {
> >         case USB_DR_MODE_HOST:
> > -               plat->init_type = USB_INIT_HOST;
> > +               priv->init_type = USB_INIT_HOST;
> >                 break;
> >         case USB_DR_MODE_PERIPHERAL:
> > -               plat->init_type = USB_INIT_DEVICE;
> > +               priv->init_type = USB_INIT_DEVICE;
> >                 break;
> >         case USB_DR_MODE_OTG:
> >         case USB_DR_MODE_UNKNOWN:
> > @@ -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;
> > -       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
> >
> >  #if CONFIG_IS_ENABLED(CLK)
> >         ret = clk_get_by_index(dev, 0, &priv->clk);
> > @@ -677,6 +673,11 @@ static int ehci_usb_probe(struct udevice *dev)
> >         mdelay(1);
> >  #endif
> >
> > +       ret = ehci_usb_dr_mode(dev);
> > +       if (ret)
> > +               goto err_clk;
> > +       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
> > +
> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
> >         ret = device_get_supply_regulator(dev, "vbus-supply",
> >                                           &priv->vbus_supply);
> > @@ -700,7 +701,7 @@ static int ehci_usb_probe(struct udevice *dev)
> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
> >         if (priv->vbus_supply) {
> >                 ret = regulator_set_enable(priv->vbus_supply,
> > -                                          (type == USB_INIT_DEVICE) ?
> > +                                          (priv->init_type ==
> USB_INIT_DEVICE) ?
> >                                            false : true);
> >                 if (ret && ret != -ENOSYS) {
> >                         printf("Error enabling VBUS supply (ret=%i)\n",
> ret);
> > @@ -785,11 +786,9 @@ U_BOOT_DRIVER(usb_mx6) = {
> >         .name   = "ehci_mx6",
> >         .id     = UCLASS_USB,
> >         .of_match = mx6_usb_ids,
> > -       .of_to_plat = ehci_usb_of_to_plat,
> >         .probe  = ehci_usb_probe,
> >         .remove = ehci_usb_remove,
> >         .ops    = &ehci_usb_ops,
> > -       .plat_auto      = sizeof(struct usb_plat),
> >         .priv_auto      = sizeof(struct ehci_mx6_priv_data),
> >         .flags  = DM_FLAG_ALLOC_PRIV_DMA,
> >  };
> > --
> > 2.32.0
> >
>
> Adam,
>
> How are you testing this for IMX8MM or IMX8MN?
>

I ran "ums 0 mmc 2" to  test peripheral mode and verified that I could see
the corresponding MMC device appear on my host machine.  I actually burned
an image to the eMMC and I was able to get Linux to run the eMMC I burned.

I connected a USB host adapter that grounds the ID pin and ran "usb start"
to test for mass storage devices, and did simple tests like "fatls usb 0"
to verify I could read the thumb drive.

I had to enable power-domain support, USB, mass storage, and a few other
items in my config file to get it all to work.  I'm traveling, so I won't
be able to give you my exact config options that I enabled.

adam


>
> Best Regards,
>
> Tim
>

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

[Tim Harvey]

On Mon, Jan 3, 2022 at 2:32 PM Adam Ford <aford173@gmail.com> wrote:
>
>
>
> On Mon, Jan 3, 2022 at 10:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
>>
>> On Thu, Dec 23, 2021 at 6:08 AM Adam Ford <aford173@gmail.com> 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@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
>> > @@ -513,7 +513,7 @@ static const struct ehci_ops mx6_ehci_ops = {
>> >
>> >  static int ehci_usb_phy_mode(struct udevice *dev)
>> >  {
>> > -       struct usb_plat *plat = dev_get_plat(dev);
>> > +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
>> >         void *__iomem addr = dev_read_addr_ptr(dev);
>> >         void *__iomem phy_ctrl, *__iomem phy_status;
>> >         const void *blob = gd->fdt_blob;
>> > @@ -540,18 +540,18 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>> >                 val = readl(phy_ctrl);
>> >
>> >                 if (val & USBPHY_CTRL_OTG_ID)
>> > -                       plat->init_type = USB_INIT_DEVICE;
>> > +                       priv->init_type = USB_INIT_DEVICE;
>> >                 else
>> > -                       plat->init_type = USB_INIT_HOST;
>> > -       } else if (is_mx7()) {
>> > +                       priv->init_type = USB_INIT_HOST;
>> > +       } else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
>> >                 phy_status = (void __iomem *)(addr +
>> >                                               USBNC_PHY_STATUS_OFFSET);
>> >                 val = readl(phy_status);
>> >
>> >                 if (val & USBNC_PHYSTATUS_ID_DIG)
>> > -                       plat->init_type = USB_INIT_DEVICE;
>> > +                       priv->init_type = USB_INIT_DEVICE;
>> >                 else
>> > -                       plat->init_type = USB_INIT_HOST;
>> > +                       priv->init_type = USB_INIT_HOST;
>> >         } else {
>> >                 return -EINVAL;
>> >         }
>> > @@ -559,19 +559,19 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>> >         return 0;
>> >  }
>> >
>> > -static int ehci_usb_of_to_plat(struct udevice *dev)
>> > +static int ehci_usb_dr_mode(struct udevice *dev)
>> >  {
>> > -       struct usb_plat *plat = dev_get_plat(dev);
>> > +       struct ehci_mx6_priv_data *priv = dev_get_priv(dev);
>> >         enum usb_dr_mode dr_mode;
>> >
>> >         dr_mode = usb_get_dr_mode(dev_ofnode(dev));
>> >
>> >         switch (dr_mode) {
>> >         case USB_DR_MODE_HOST:
>> > -               plat->init_type = USB_INIT_HOST;
>> > +               priv->init_type = USB_INIT_HOST;
>> >                 break;
>> >         case USB_DR_MODE_PERIPHERAL:
>> > -               plat->init_type = USB_INIT_DEVICE;
>> > +               priv->init_type = USB_INIT_DEVICE;
>> >                 break;
>> >         case USB_DR_MODE_OTG:
>> >         case USB_DR_MODE_UNKNOWN:
>> > @@ -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;
>> > -       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
>> >
>> >  #if CONFIG_IS_ENABLED(CLK)
>> >         ret = clk_get_by_index(dev, 0, &priv->clk);
>> > @@ -677,6 +673,11 @@ static int ehci_usb_probe(struct udevice *dev)
>> >         mdelay(1);
>> >  #endif
>> >
>> > +       ret = ehci_usb_dr_mode(dev);
>> > +       if (ret)
>> > +               goto err_clk;
>> > +       priv->phy_type = usb_get_phy_mode(dev_ofnode(dev));
>> > +
>> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
>> >         ret = device_get_supply_regulator(dev, "vbus-supply",
>> >                                           &priv->vbus_supply);
>> > @@ -700,7 +701,7 @@ static int ehci_usb_probe(struct udevice *dev)
>> >  #if CONFIG_IS_ENABLED(DM_REGULATOR)
>> >         if (priv->vbus_supply) {
>> >                 ret = regulator_set_enable(priv->vbus_supply,
>> > -                                          (type == USB_INIT_DEVICE) ?
>> > +                                          (priv->init_type == USB_INIT_DEVICE) ?
>> >                                            false : true);
>> >                 if (ret && ret != -ENOSYS) {
>> >                         printf("Error enabling VBUS supply (ret=%i)\n", ret);
>> > @@ -785,11 +786,9 @@ U_BOOT_DRIVER(usb_mx6) = {
>> >         .name   = "ehci_mx6",
>> >         .id     = UCLASS_USB,
>> >         .of_match = mx6_usb_ids,
>> > -       .of_to_plat = ehci_usb_of_to_plat,
>> >         .probe  = ehci_usb_probe,
>> >         .remove = ehci_usb_remove,
>> >         .ops    = &ehci_usb_ops,
>> > -       .plat_auto      = sizeof(struct usb_plat),
>> >         .priv_auto      = sizeof(struct ehci_mx6_priv_data),
>> >         .flags  = DM_FLAG_ALLOC_PRIV_DMA,
>> >  };
>> > --
>> > 2.32.0
>> >
>>
>> Adam,
>>
>> How are you testing this for IMX8MM or IMX8MN?
>
>
> I ran "ums 0 mmc 2" to  test peripheral mode and verified that I could see the corresponding MMC device appear on my host machine.  I actually burned an image to the eMMC and I was able to get Linux to run the eMMC I burned.
>
> I connected a USB host adapter that grounds the ID pin and ran "usb start" to test for mass storage devices, and did simple tests like "fatls usb 0" to verify I could read the thumb drive.
>
> I had to enable power-domain support, USB, mass storage, and a few other items in my config file to get it all to work.  I'm traveling, so I won't be able to give you my exact config options that I enabled.
>

Adam,

Thanks - It was the various configs that I was having trouble with...
there are a bunch of dependencies that are not obvious making me think
something else was still missing.

Tested-By: Tim Harvey <tharvey@gateworks.com> on imx8mm-venice-gw73xx-0x

I tested gadget mode via ums as well as host via asix_eth and this was
my defconfig diff for testing:
diff --git a/configs/imx8mm_venice_defconfig b/configs/imx8mm_venice_defconfig
index 822324766843..36a16866f1a0 100644
--- a/configs/imx8mm_venice_defconfig
+++ b/configs/imx8mm_venice_defconfig
@@ -48,6 +48,8 @@ CONFIG_CMD_FUSE=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_USB=y
+CONFIG_CMD_USB_MASS_STORAGE=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_UUID=y
@@ -97,6 +99,8 @@ CONFIG_MII=y
 CONFIG_PINCTRL=y
 CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_IMX8M=y
+CONFIG_POWER_DOMAIN=y
+CONFIG_IMX8M_POWER_DOMAIN=y
 CONFIG_DM_PMIC=y
 CONFIG_DM_PMIC_BD71837=y
 CONFIG_SPL_DM_PMIC_BD71837=y
@@ -112,5 +116,21 @@ CONFIG_SYSRESET_PSCI=y
 CONFIG_SYSRESET_WATCHDOG=y
 CONFIG_DM_THERMAL=y
 CONFIG_IMX_TMU=y
+CONFIG_USB=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_HOST_ETHER=y
+CONFIG_USB_ETHER_ASIX=y
+CONFIG_USB_ETHER_ASIX88179=y
+CONFIG_USB_ETHER_LAN75XX=y
+CONFIG_USB_ETHER_LAN78XX=y
+CONFIG_USB_ETHER_MCS7830=y
+CONFIG_USB_ETHER_RTL8152=y
+CONFIG_USB_ETHER_SMSC95XX=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_MANUFACTURER="Gateworks"
+CONFIG_USB_GADGET_VENDOR_NUM=0x0525
+CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5
+CONFIG_CI_UDC=y
+CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_IMX_WATCHDOG=y
 CONFIG_HEXDUMP=y

Best Regards,

Tim

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

[Michael Walle]

Hi,

> 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@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;

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

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.

-michael

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

[Adam Ford]

On Tue, Jan 4, 2022 at 2:32 AM Michael Walle <michael@walle.cc> wrote:
>
> Hi,
>
> > 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@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));
>
> 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.  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.

>
> -michael

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

[Michael Walle]

Hi,

Am 2022-01-11 15:20, schrieb Adam Ford:
> On Tue, Jan 4, 2022 at 2:32 AM Michael Walle <michael@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@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?

>  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.

-michael

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

[Adam Ford]

On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael@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@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@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.

>
> >  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?

adam
>
> -michael

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

[Michael Walle]

Am 2022-01-11 17:52, schrieb Adam Ford:
> On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael@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@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@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?
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

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

[Adam Ford]

On Wed, Jan 12, 2022 at 2:17 AM Michael Walle <michael@walle.cc> wrote:
>
> Am 2022-01-11 17:52, schrieb Adam Ford:
> > On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael@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@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@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.

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

Re: [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@gmail.com> wrote:
>
> On Wed, Jan 12, 2022 at 2:17 AM Michael Walle <michael@walle.cc> wrote:
> >
> > Am 2022-01-11 17:52, schrieb Adam Ford:
> > > On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael@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@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@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@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

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

[Adam Ford]

On Thu, Jan 13, 2022 at 6:49 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Thu, Jan 13, 2022 at 6:35 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 2:17 AM Michael Walle <michael@walle.cc> wrote:
> > >
> > > Am 2022-01-11 17:52, schrieb Adam Ford:
> > > > On Tue, Jan 11, 2022 at 9:31 AM Michael Walle <michael@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@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@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@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.
>

Michael,

It appears that the usb_setup_ehci_gadget() forces the USB driver into
peripheral mode regardless
of the device tree and/or the state of the register that detects the
pin, so I'm going to submit a V3 that keeps the helper function.
The helper function will not read the register to determine the
host/peripheral mode.  Instead, it will return UNKNOWN if the
device tree does not explicitly state the dr_mode.

If the device type is UNKNOWN, then the probe will call
ehci_usb_phy_mode after the clocks are started which will query
the register for the IP block to determine the state.  This way, we
don't break some of the functionality for forcing
peripheral mode if we intentionally activate a peripheral.

Tim & Fabio,

Would you mind re-testing?  I'm going to drop the added tags on the V3
because there is enough changes here and a small change
in behavior that I think it warrants re-test and/or re-review.

adam

> 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