[PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

There is no need to set and/or detect mode in of_to_plat and
accessing phy registers at that point before device power domain and
clock are enabled will cause hangs on platforms such as IMX8M Mini.

Move the mode set/detect from of_to_plat into the probe and remove
the unnecessary of_to_plat.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 06be9deaaa..c2dfe49012 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev)
 	return 0;
 }
 
-static int ehci_usb_of_to_plat(struct udevice *dev)
-{
-	struct usb_plat *plat = dev_get_plat(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;
-		break;
-	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);
-	};
-
-	return 0;
-}
-
 static int mx6_parse_dt_addrs(struct udevice *dev)
 {
 #if !defined(CONFIG_PHY)
@@ -623,7 +601,6 @@ 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;
@@ -641,7 +618,6 @@ static int ehci_usb_probe(struct udevice *dev)
 		return ret;
 
 	priv->ehci = ehci;
-	priv->init_type = type;
 
 #if CONFIG_IS_ENABLED(CLK)
 	ret = clk_get_by_index(dev, 0, &priv->clk);
@@ -657,6 +633,21 @@ static int ehci_usb_probe(struct udevice *dev)
 	mdelay(1);
 #endif
 
+	switch (usb_get_dr_mode(dev_ofnode(dev))) {
+	case USB_DR_MODE_HOST:
+		plat->init_type = USB_INIT_HOST;
+		break;
+	case USB_DR_MODE_PERIPHERAL:
+		plat->init_type = USB_INIT_DEVICE;
+		break;
+	case USB_DR_MODE_OTG:
+	case USB_DR_MODE_UNKNOWN:
+		ret = ehci_usb_phy_mode(dev);
+		if (ret)
+			return ret;
+	};
+	priv->init_type = plat->init_type;
+
 #if CONFIG_IS_ENABLED(DM_REGULATOR)
 	ret = device_get_supply_regulator(dev, "vbus-supply",
 					  &priv->vbus_supply);
@@ -680,7 +671,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);
@@ -764,7 +755,6 @@ 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,
-- 
2.17.1

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Tim Harvey]

Add support for determining host vs peripheral mode for IMX8MM
configured as OTG.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/usb/host/ehci-mx6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index c2dfe49012..d055d2b1fe 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);
-- 
2.17.1

[PATCH 3/3] configs: imx8mm_venice_defconfig: add usb support

[Tim Harvey]

Enable USB support for host controller and various USB ethernet devices.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 configs/imx8mm_venice_defconfig | 15 +++++++++++++++
 include/configs/imx8mm_venice.h |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/configs/imx8mm_venice_defconfig b/configs/imx8mm_venice_defconfig
index 01bf2f362a..0d16f8d1c0 100644
--- a/configs/imx8mm_venice_defconfig
+++ b/configs/imx8mm_venice_defconfig
@@ -48,6 +48,7 @@ CONFIG_CMD_FUSE=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_USB=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_UUID=y
@@ -92,6 +93,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
@@ -107,5 +110,17 @@ CONFIG_SYSRESET_PSCI=y
 CONFIG_SYSRESET_WATCHDOG=y
 CONFIG_DM_THERMAL=y
 CONFIG_IMX_TMU=y
+CONFIG_USB=y
+CONFIG_DM_USB=y
+# CONFIG_SPL_DM_USB is not set
+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_IMX_WATCHDOG=y
 CONFIG_HEXDUMP=y
diff --git a/include/configs/imx8mm_venice.h b/include/configs/imx8mm_venice.h
index 91669255e1..5e9f4c89d1 100644
--- a/include/configs/imx8mm_venice.h
+++ b/include/configs/imx8mm_venice.h
@@ -99,6 +99,10 @@
 /* UART */
 #define CONFIG_MXC_UART_BASE		UART2_BASE_ADDR
 
+/* USB */
+#define CONFIG_EHCI_HCD_INIT_AFTER_RESET
+#define CONFIG_USB_MAX_CONTROLLER_COUNT 2
+
 /* Monitor Command Prompt */
 #define CONFIG_SYS_PROMPT_HUSH_PS2	"> "
 #define CONFIG_SYS_CBSIZE		SZ_2K
-- 
2.17.1

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Marek Vasut]

On 4/27/21 7:08 PM, Tim Harvey wrote:
> Add support for determining host vs peripheral mode for IMX8MM
> configured as OTG.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>   drivers/usb/host/ehci-mx6.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index c2dfe49012..d055d2b1fe 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()) {

This likely also applies to 8mq/mm/mn/mp , i.e. all of them.

[PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Marek Vasut]

On 4/27/21 7:08 PM, Tim Harvey wrote:
> There is no need to set and/or detect mode in of_to_plat and
> accessing phy registers at that point before device power domain and
> clock are enabled will cause hangs on platforms such as IMX8M Mini.
> 
> Move the mode set/detect from of_to_plat into the probe and remove
> the unnecessary of_to_plat.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>   drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++-----------------------
>   1 file changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 06be9deaaa..c2dfe49012 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>   	return 0;
>   }
>   
> -static int ehci_usb_of_to_plat(struct udevice *dev)
> -{
> -	struct usb_plat *plat = dev_get_plat(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;
> -		break;
> -	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);
> -	};
> -
> -	return 0;
> -}
> -

[...]

> @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = {
>   	.name	= "ehci_mx6",
>   	.id	= UCLASS_USB,
>   	.of_match = mx6_usb_ids,
> -	.of_to_plat = ehci_usb_of_to_plat,

I wonder why it was implemented in of_to_plat originally , maybe there 
is some reason for that ?

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Tim Harvey]

On Tue, Apr 27, 2021 at 10:44 AM Marek Vasut <marex@denx.de> wrote:
>
> On 4/27/21 7:08 PM, Tim Harvey wrote:
> > Add support for determining host vs peripheral mode for IMX8MM
> > configured as OTG.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >   drivers/usb/host/ehci-mx6.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > index c2dfe49012..d055d2b1fe 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()) {
>
> This likely also applies to 8mq/mm/mn/mp , i.e. all of them.

Agreed. Perhaps Adam, Frieder, or Fabio have something to test with? I
only have IMX8M Mini at the moment.

Tim

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Patrick Wildt]

Am Tue, Apr 27, 2021 at 10:50:34AM -0700 schrieb Tim Harvey:
> On Tue, Apr 27, 2021 at 10:44 AM Marek Vasut <marex@denx.de> wrote:
> >
> > On 4/27/21 7:08 PM, Tim Harvey wrote:
> > > Add support for determining host vs peripheral mode for IMX8MM
> > > configured as OTG.
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > ---
> > >   drivers/usb/host/ehci-mx6.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > > index c2dfe49012..d055d2b1fe 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()) {
> >
> > This likely also applies to 8mq/mm/mn/mp , i.e. all of them.
> 
> Agreed. Perhaps Adam, Frieder, or Fabio have something to test with? I
> only have IMX8M Mini at the moment.

No, I don't think this applies to all of them, 8MM and 8MN should be
correct.

So from a historic point of view the first one that showed up is the
8MQ, which has a DesignWare xHCI controller.  There's no eHCI, so this
driver doesn't attach and there is neither the fsl,imx27-usb nor the
fsl,imx7d-usb compatible.  Hence not having imx8mq should be correct.

When the i.MX8MM showed up the biggest difference obviously was the
silicon technology, reducing temperature.  But it also replaced the
DesignWare xHCI with ... what was it, the cadence OTG controller?
This is basically a carbon copy of the fsl,imx7d-usb (which has the
i.MX6 as heritage).  Hence imx8mm is correct here.

Then the i.MX8MP showed up, which is builds on that, but it finally
gets the DesignWare xHCI back.  So in that regard it's more a better
version of i.MX8MQ.  The fsl,imx7d-usb compatible is not part of the
dts, hence not having imx8mp should be correct.

The i.MX8MN is new to me, but by grepping you can see that the MN also
has the fsl,imx7d-usb compatible.

Hence I think only 8MM and 8MN should be added, not 8MQ or 8MP.

Patrick

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Marek Vasut]

On 4/27/21 9:24 PM, Patrick Wildt wrote:
> Am Tue, Apr 27, 2021 at 10:50:34AM -0700 schrieb Tim Harvey:
>> On Tue, Apr 27, 2021 at 10:44 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 4/27/21 7:08 PM, Tim Harvey wrote:
>>>> Add support for determining host vs peripheral mode for IMX8MM
>>>> configured as OTG.
>>>>
>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>> ---
>>>>    drivers/usb/host/ehci-mx6.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>>>> index c2dfe49012..d055d2b1fe 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()) {
>>>
>>> This likely also applies to 8mq/mm/mn/mp , i.e. all of them.
>>
>> Agreed. Perhaps Adam, Frieder, or Fabio have something to test with? I
>> only have IMX8M Mini at the moment.
> 
> No, I don't think this applies to all of them, 8MM and 8MN should be
> correct.
> 
> So from a historic point of view the first one that showed up is the
> 8MQ, which has a DesignWare xHCI controller.  There's no eHCI, so this
> driver doesn't attach and there is neither the fsl,imx27-usb nor the
> fsl,imx7d-usb compatible.  Hence not having imx8mq should be correct.
> 
> When the i.MX8MM showed up the biggest difference obviously was the
> silicon technology, reducing temperature.  But it also replaced the
> DesignWare xHCI with ... what was it, the cadence OTG controller?

chipidea

> This is basically a carbon copy of the fsl,imx7d-usb (which has the
> i.MX6 as heritage).  Hence imx8mm is correct here.

Not quite, the mx8mm/mn variant is a bit more complex due to the extra 
power domain stuff. And the PHY/MISC bits changed from mx7 too.

> Then the i.MX8MP showed up, which is builds on that, but it finally
> gets the DesignWare xHCI back.  So in that regard it's more a better
> version of i.MX8MQ.  The fsl,imx7d-usb compatible is not part of the
> dts, hence not having imx8mp should be correct.
> 
> The i.MX8MN is new to me, but by grepping you can see that the MN also
> has the fsl,imx7d-usb compatible.

Seems there is always some sort of overlap between the Ms in terms of 
IPs that are reused.

> Hence I think only 8MM and 8MN should be added, not 8MQ or 8MP.

I agree, 8MP also only lists xhci (likely dwc3), like original M(Q).
MN should work, I use these patches on a board with it, in host mode only.

[PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote:
>
> On 4/27/21 7:08 PM, Tim Harvey wrote:
> > There is no need to set and/or detect mode in of_to_plat and
> > accessing phy registers at that point before device power domain and
> > clock are enabled will cause hangs on platforms such as IMX8M Mini.
> >
> > Move the mode set/detect from of_to_plat into the probe and remove
> > the unnecessary of_to_plat.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >   drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++-----------------------
> >   1 file changed, 16 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > index 06be9deaaa..c2dfe49012 100644
> > --- a/drivers/usb/host/ehci-mx6.c
> > +++ b/drivers/usb/host/ehci-mx6.c
> > @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> >       return 0;
> >   }
> >
> > -static int ehci_usb_of_to_plat(struct udevice *dev)
> > -{
> > -     struct usb_plat *plat = dev_get_plat(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;
> > -             break;
> > -     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);
> > -     };
> > -
> > -     return 0;
> > -}
> > -
>
> [...]
>
> > @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = {
> >       .name   = "ehci_mx6",
> >       .id     = UCLASS_USB,
> >       .of_match = mx6_usb_ids,
> > -     .of_to_plat = ehci_usb_of_to_plat,
>
> I wonder why it was implemented in of_to_plat originally , maybe there
> is some reason for that ?

Marek,

Looking back the commit that added the ehci_usb_ofdata_to_platdata was:
cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata")

Before that there was a board-specific function that would set the
usb_plat->init_type.

The only reason to set usb_plat->init_type in of_to_plat would be so
that drivers/usb/host/usb-uclass.c would have knowledge of it but I
only see that it is set there in usb_setup_ehci_gadget.

I added Peng, Stefano, and Simon to the thread to see if they see an
issue with doing away with of_to_plat setting the usb_plat->init_type
prior to probe.

Tim

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Tim Harvey]

On Tue, Apr 27, 2021 at 12:52 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/27/21 9:24 PM, Patrick Wildt wrote:
> > Am Tue, Apr 27, 2021 at 10:50:34AM -0700 schrieb Tim Harvey:
> >> On Tue, Apr 27, 2021 at 10:44 AM Marek Vasut <marex@denx.de> wrote:
> >>>
> >>> On 4/27/21 7:08 PM, Tim Harvey wrote:
> >>>> Add support for determining host vs peripheral mode for IMX8MM
> >>>> configured as OTG.
> >>>>
> >>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>>> ---
> >>>>    drivers/usb/host/ehci-mx6.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> >>>> index c2dfe49012..d055d2b1fe 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()) {
> >>>
> >>> This likely also applies to 8mq/mm/mn/mp , i.e. all of them.
> >>
> >> Agreed. Perhaps Adam, Frieder, or Fabio have something to test with? I
> >> only have IMX8M Mini at the moment.
> >
> > No, I don't think this applies to all of them, 8MM and 8MN should be
> > correct.
> >
> > So from a historic point of view the first one that showed up is the
> > 8MQ, which has a DesignWare xHCI controller.  There's no eHCI, so this
> > driver doesn't attach and there is neither the fsl,imx27-usb nor the
> > fsl,imx7d-usb compatible.  Hence not having imx8mq should be correct.
> >
> > When the i.MX8MM showed up the biggest difference obviously was the
> > silicon technology, reducing temperature.  But it also replaced the
> > DesignWare xHCI with ... what was it, the cadence OTG controller?
>
> chipidea
>
> > This is basically a carbon copy of the fsl,imx7d-usb (which has the
> > i.MX6 as heritage).  Hence imx8mm is correct here.
>
> Not quite, the mx8mm/mn variant is a bit more complex due to the extra
> power domain stuff. And the PHY/MISC bits changed from mx7 too.
>
> > Then the i.MX8MP showed up, which is builds on that, but it finally
> > gets the DesignWare xHCI back.  So in that regard it's more a better
> > version of i.MX8MQ.  The fsl,imx7d-usb compatible is not part of the
> > dts, hence not having imx8mp should be correct.
> >
> > The i.MX8MN is new to me, but by grepping you can see that the MN also
> > has the fsl,imx7d-usb compatible.
>
> Seems there is always some sort of overlap between the Ms in terms of
> IPs that are reused.
>
> > Hence I think only 8MM and 8MN should be added, not 8MQ or 8MP.
>
> I agree, 8MP also only lists xhci (likely dwc3), like original M(Q).
> MN should work, I use these patches on a board with it, in host mode only.

I will re-submit and add is_imx8mn().... that's what the NXP
downstream u-boot does as well.

Tim

[PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Marek Vasut]

On 4/28/21 3:55 AM, Tim Harvey wrote:
> On Tue, Apr 27, 2021 at 12:52 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/27/21 9:24 PM, Patrick Wildt wrote:
>>> Am Tue, Apr 27, 2021 at 10:50:34AM -0700 schrieb Tim Harvey:
>>>> On Tue, Apr 27, 2021 at 10:44 AM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 4/27/21 7:08 PM, Tim Harvey wrote:
>>>>>> Add support for determining host vs peripheral mode for IMX8MM
>>>>>> configured as OTG.
>>>>>>
>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>> ---
>>>>>>     drivers/usb/host/ehci-mx6.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>>>>>> index c2dfe49012..d055d2b1fe 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()) {
>>>>>
>>>>> This likely also applies to 8mq/mm/mn/mp , i.e. all of them.
>>>>
>>>> Agreed. Perhaps Adam, Frieder, or Fabio have something to test with? I
>>>> only have IMX8M Mini at the moment.
>>>
>>> No, I don't think this applies to all of them, 8MM and 8MN should be
>>> correct.
>>>
>>> So from a historic point of view the first one that showed up is the
>>> 8MQ, which has a DesignWare xHCI controller.  There's no eHCI, so this
>>> driver doesn't attach and there is neither the fsl,imx27-usb nor the
>>> fsl,imx7d-usb compatible.  Hence not having imx8mq should be correct.
>>>
>>> When the i.MX8MM showed up the biggest difference obviously was the
>>> silicon technology, reducing temperature.  But it also replaced the
>>> DesignWare xHCI with ... what was it, the cadence OTG controller?
>>
>> chipidea
>>
>>> This is basically a carbon copy of the fsl,imx7d-usb (which has the
>>> i.MX6 as heritage).  Hence imx8mm is correct here.
>>
>> Not quite, the mx8mm/mn variant is a bit more complex due to the extra
>> power domain stuff. And the PHY/MISC bits changed from mx7 too.
>>
>>> Then the i.MX8MP showed up, which is builds on that, but it finally
>>> gets the DesignWare xHCI back.  So in that regard it's more a better
>>> version of i.MX8MQ.  The fsl,imx7d-usb compatible is not part of the
>>> dts, hence not having imx8mp should be correct.
>>>
>>> The i.MX8MN is new to me, but by grepping you can see that the MN also
>>> has the fsl,imx7d-usb compatible.
>>
>> Seems there is always some sort of overlap between the Ms in terms of
>> IPs that are reused.
>>
>>> Hence I think only 8MM and 8MN should be added, not 8MQ or 8MP.
>>
>> I agree, 8MP also only lists xhci (likely dwc3), like original M(Q).
>> MN should work, I use these patches on a board with it, in host mode only.
> 
> I will re-submit and add is_imx8mn().... that's what the NXP
> downstream u-boot does as well.

Thanks

[PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Marek Vasut]

On 4/28/21 3:51 AM, Tim Harvey wrote:
> On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/27/21 7:08 PM, Tim Harvey wrote:
>>> There is no need to set and/or detect mode in of_to_plat and
>>> accessing phy registers at that point before device power domain and
>>> clock are enabled will cause hangs on platforms such as IMX8M Mini.
>>>
>>> Move the mode set/detect from of_to_plat into the probe and remove
>>> the unnecessary of_to_plat.
>>>
>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>> ---
>>>    drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++-----------------------
>>>    1 file changed, 16 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
>>> index 06be9deaaa..c2dfe49012 100644
>>> --- a/drivers/usb/host/ehci-mx6.c
>>> +++ b/drivers/usb/host/ehci-mx6.c
>>> @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev)
>>>        return 0;
>>>    }
>>>
>>> -static int ehci_usb_of_to_plat(struct udevice *dev)
>>> -{
>>> -     struct usb_plat *plat = dev_get_plat(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;
>>> -             break;
>>> -     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);
>>> -     };
>>> -
>>> -     return 0;
>>> -}
>>> -
>>
>> [...]
>>
>>> @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = {
>>>        .name   = "ehci_mx6",
>>>        .id     = UCLASS_USB,
>>>        .of_match = mx6_usb_ids,
>>> -     .of_to_plat = ehci_usb_of_to_plat,
>>
>> I wonder why it was implemented in of_to_plat originally , maybe there
>> is some reason for that ?
> 
> Marek,
> 
> Looking back the commit that added the ehci_usb_ofdata_to_platdata was:
> cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata")
> 
> Before that there was a board-specific function that would set the
> usb_plat->init_type.
> 
> The only reason to set usb_plat->init_type in of_to_plat would be so
> that drivers/usb/host/usb-uclass.c would have knowledge of it but I
> only see that it is set there in usb_setup_ehci_gadget.

So interaction with Gadget, that's what I was afraid of.

> I added Peng, Stefano, and Simon to the thread to see if they see an
> issue with doing away with of_to_plat setting the usb_plat->init_type
> prior to probe.

I added Lukasz too.

Re: [PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Tue, Apr 27, 2021 at 6:59 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/28/21 3:51 AM, Tim Harvey wrote:
> > On Tue, Apr 27, 2021 at 10:45 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 4/27/21 7:08 PM, Tim Harvey wrote:
> >>> There is no need to set and/or detect mode in of_to_plat and
> >>> accessing phy registers at that point before device power domain and
> >>> clock are enabled will cause hangs on platforms such as IMX8M Mini.
> >>>
> >>> Move the mode set/detect from of_to_plat into the probe and remove
> >>> the unnecessary of_to_plat.
> >>>
> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>> ---
> >>>    drivers/usb/host/ehci-mx6.c | 42 ++++++++++++++-----------------------
> >>>    1 file changed, 16 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> >>> index 06be9deaaa..c2dfe49012 100644
> >>> --- a/drivers/usb/host/ehci-mx6.c
> >>> +++ b/drivers/usb/host/ehci-mx6.c
> >>> @@ -539,28 +539,6 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> >>>        return 0;
> >>>    }
> >>>
> >>> -static int ehci_usb_of_to_plat(struct udevice *dev)
> >>> -{
> >>> -     struct usb_plat *plat = dev_get_plat(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;
> >>> -             break;
> >>> -     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);
> >>> -     };
> >>> -
> >>> -     return 0;
> >>> -}
> >>> -
> >>
> >> [...]
> >>
> >>> @@ -764,7 +755,6 @@ U_BOOT_DRIVER(usb_mx6) = {
> >>>        .name   = "ehci_mx6",
> >>>        .id     = UCLASS_USB,
> >>>        .of_match = mx6_usb_ids,
> >>> -     .of_to_plat = ehci_usb_of_to_plat,
> >>
> >> I wonder why it was implemented in of_to_plat originally , maybe there
> >> is some reason for that ?
> >
> > Marek,
> >
> > Looking back the commit that added the ehci_usb_ofdata_to_platdata was:
> > cccbddc38c43 ("usb: ehci-mx6: implement ofdata_to_platdata")
> >
> > Before that there was a board-specific function that would set the
> > usb_plat->init_type.
> >
> > The only reason to set usb_plat->init_type in of_to_plat would be so
> > that drivers/usb/host/usb-uclass.c would have knowledge of it but I
> > only see that it is set there in usb_setup_ehci_gadget.
>
> So interaction with Gadget, that's what I was afraid of.
>
> > I added Peng, Stefano, and Simon to the thread to see if they see an
> > issue with doing away with of_to_plat setting the usb_plat->init_type
> > prior to probe.
>
> I added Lukasz too.

Marek,

Is there something you want me to change here?

I am happy to test gadget support as my board has an OTG connector.
Could you give me some pointers on how to configure a gadget in U-Boot
in order to test?

Best regards,

Tim

Re: [PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Fabio Estevam]

Hi Tim.

On Thu, Jun 3, 2021 at 12:50 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Marek,
>
> Is there something you want me to change here?
>
> I am happy to test gadget support as my board has an OTG connector.
> Could you give me some pointers on how to configure a gadget in U-Boot
> in order to test?

One quick way to test gadget support is via the "ums 0 mmc 0" command.

This will mount the eMMC content via USB mass storage device on the PC.

You can check configs/warp7_defconfig for an example on how to select
the USB gadget options.

I applied this one and tested on a imx7s-warp board via "ums 0 mmc 0".

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

I am interested in getting USB gadget support in imx8mm too.

Thanks

Re: [PATCH 1/3] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Thu, Jul 1, 2021 at 5:41 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim.
>
> On Thu, Jun 3, 2021 at 12:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > Marek,
> >
> > Is there something you want me to change here?
> >
> > I am happy to test gadget support as my board has an OTG connector.
> > Could you give me some pointers on how to configure a gadget in U-Boot
> > in order to test?
>
> One quick way to test gadget support is via the "ums 0 mmc 0" command.
>
> This will mount the eMMC content via USB mass storage device on the PC.
>
> You can check configs/warp7_defconfig for an example on how to select
> the USB gadget options.
>
> I applied this one and tested on a imx7s-warp board via "ums 0 mmc 0".
>
> Tested-by: Fabio Estevam <festevam@gmail.com>
>
> I am interested in getting USB gadget support in imx8mm too.
>

Fabio,

Right - I finally remembered that earlier today and just sent a v2 of
this series showing that I indeed tested that on both imx6q and
imx8mm. When you get a chance please show your Tested-by on that
thread.

Tim

Re: [PATCH 2/3] usb: ehci-mx6: add IMX8MM OTG support

[Fabio Estevam]

Hi Tim,

On Tue, Apr 27, 2021 at 10:55 PM Tim Harvey <tharvey@gateworks.com> wrote:

> I will re-submit and add is_imx8mn().... that's what the NXP
> downstream u-boot does as well.

When you re-submit this, please add:

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

USB gadget works now on imx8mm-evk. Thanks