[PATCH] usb: dwc3-generic: Fix the iMX8MQ support

[PATCH] usb: dwc3-generic: Fix the iMX8MQ support

[Alban Bedel]

[-- Attachment #1: Type: text/plain, Size: 4363 bytes --]

The binding of iMX8MQ USB controller doesn't use child nodes like the
other devices supported in this driver. To support it split the child
nodes parsing to its own function and add a field to the platform data
to indicate that we should just use the top node.

Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
---
 drivers/usb/dwc3/dwc3-generic.c | 95 +++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 01bd0ca190e3..defef43ff503 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -221,6 +221,7 @@ U_BOOT_DRIVER(dwc3_generic_host) = {
 struct dwc3_glue_ops {
 	void (*select_dr_mode)(struct udevice *dev, int index,
 			       enum usb_dr_mode mode);
+	int single_node;
 };
 
 void dwc3_ti_select_dr_mode(struct udevice *dev, int index,
@@ -307,54 +308,66 @@ struct dwc3_glue_ops ti_ops = {
 	.select_dr_mode = dwc3_ti_select_dr_mode,
 };
 
+static int dwc3_glue_bind_node(struct udevice *parent, ofnode node,
+			       enum usb_dr_mode dr_mode)
+{
+	const char *name = ofnode_get_name(node);
+	const char *driver = NULL;
+	struct udevice *dev;
+	int ret;
+
+	debug("%s: subnode name: %s\n", __func__, name);
+
+	/* if the parent node doesn't have a mode check the leaf */
+	if (!dr_mode)
+		dr_mode = usb_get_dr_mode(node);
+
+	switch (dr_mode) {
+	case USB_DR_MODE_PERIPHERAL:
+	case USB_DR_MODE_OTG:
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
+		driver = "dwc3-generic-peripheral";
+#endif
+		break;
+#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
+	case USB_DR_MODE_HOST:
+		debug("%s: dr_mode: HOST\n", __func__);
+		driver = "dwc3-generic-host";
+		break;
+#endif
+	default:
+		debug("%s: unsupported dr_mode\n", __func__);
+		return -ENODEV;
+	};
+
+	if (!driver)
+		return 0;
+
+	ret = device_bind_driver_to_node(parent, driver, name,
+					 node, &dev);
+	if (ret)
+		debug("%s: not able to bind usb device mode\n",
+		      __func__);
+	return ret;
+}
+
 static int dwc3_glue_bind(struct udevice *parent)
 {
+	struct dwc3_glue_ops *ops = (struct dwc3_glue_ops *)dev_get_driver_data(parent);
 	ofnode node;
 	int ret;
 	enum usb_dr_mode dr_mode;
 
+	if (ops->single_node)
+		return dwc3_glue_bind_node(parent, dev_ofnode(parent), 0);
+
 	dr_mode = usb_get_dr_mode(dev_ofnode(parent));
 
 	ofnode_for_each_subnode(node, dev_ofnode(parent)) {
-		const char *name = ofnode_get_name(node);
-		struct udevice *dev;
-		const char *driver = NULL;
-
-		debug("%s: subnode name: %s\n", __func__, name);
-
-		/* if the parent node doesn't have a mode check the leaf */
-		if (!dr_mode)
-			dr_mode = usb_get_dr_mode(node);
-
-		switch (dr_mode) {
-		case USB_DR_MODE_PERIPHERAL:
-		case USB_DR_MODE_OTG:
-#if CONFIG_IS_ENABLED(DM_USB_GADGET)
-			debug("%s: dr_mode: OTG or Peripheral\n", __func__);
-			driver = "dwc3-generic-peripheral";
-#endif
-			break;
-#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
-		case USB_DR_MODE_HOST:
-			debug("%s: dr_mode: HOST\n", __func__);
-			driver = "dwc3-generic-host";
-			break;
-#endif
-		default:
-			debug("%s: unsupported dr_mode\n", __func__);
-			return -ENODEV;
-		};
-
-		if (!driver)
-			continue;
-
-		ret = device_bind_driver_to_node(parent, driver, name,
-						 node, &dev);
-		if (ret) {
-			debug("%s: not able to bind usb device mode\n",
-			      __func__);
+		ret = dwc3_glue_bind_node(parent, node, dr_mode);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
@@ -454,6 +467,10 @@ static int dwc3_glue_remove(struct udevice *dev)
 	return 0;
 }
 
+struct dwc3_glue_ops single_node_ops = {
+	.single_node = 1,
+};
+
 static const struct udevice_id dwc3_glue_ids[] = {
 	{ .compatible = "xlnx,zynqmp-dwc3" },
 	{ .compatible = "xlnx,versal-dwc3" },
@@ -464,7 +481,7 @@ static const struct udevice_id dwc3_glue_ids[] = {
 	{ .compatible = "rockchip,rk3328-dwc3" },
 	{ .compatible = "rockchip,rk3399-dwc3" },
 	{ .compatible = "qcom,dwc3" },
-	{ .compatible = "fsl,imx8mq-dwc3" },
+	{ .compatible = "fsl,imx8mq-dwc3", .data = (ulong)&single_node_ops },
 	{ .compatible = "intel,tangier-dwc3" },
 	{ }
 };
-- 
2.32.0

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3627 bytes --]

Re: [PATCH] usb: dwc3-generic: Fix the iMX8MQ support

[Angus Ainslie]

Hi Alban,

On 2022-04-19 01:07, Alban Bedel wrote:
> The binding of iMX8MQ USB controller doesn't use child nodes like the
> other devices supported in this driver. To support it split the child
> nodes parsing to its own function and add a field to the platform data
> to indicate that we should just use the top node.
> 

I'm not clear on what this is fixing. Doesn't the original code deal 
with a devicetree stanza that has the information in either the node or 
the parent.

Which case does this fix ?

Thanks
Angus

> Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
> ---
>  drivers/usb/dwc3/dwc3-generic.c | 95 +++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-generic.c 
> b/drivers/usb/dwc3/dwc3-generic.c
> index 01bd0ca190e3..defef43ff503 100644
> --- a/drivers/usb/dwc3/dwc3-generic.c
> +++ b/drivers/usb/dwc3/dwc3-generic.c
> @@ -221,6 +221,7 @@ U_BOOT_DRIVER(dwc3_generic_host) = {
>  struct dwc3_glue_ops {
>  	void (*select_dr_mode)(struct udevice *dev, int index,
>  			       enum usb_dr_mode mode);
> +	int single_node;
>  };
> 
>  void dwc3_ti_select_dr_mode(struct udevice *dev, int index,
> @@ -307,54 +308,66 @@ struct dwc3_glue_ops ti_ops = {
>  	.select_dr_mode = dwc3_ti_select_dr_mode,
>  };
> 
> +static int dwc3_glue_bind_node(struct udevice *parent, ofnode node,
> +			       enum usb_dr_mode dr_mode)
> +{
> +	const char *name = ofnode_get_name(node);
> +	const char *driver = NULL;
> +	struct udevice *dev;
> +	int ret;
> +
> +	debug("%s: subnode name: %s\n", __func__, name);
> +
> +	/* if the parent node doesn't have a mode check the leaf */
> +	if (!dr_mode)
> +		dr_mode = usb_get_dr_mode(node);
> +
> +	switch (dr_mode) {
> +	case USB_DR_MODE_PERIPHERAL:
> +	case USB_DR_MODE_OTG:
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
> +		driver = "dwc3-generic-peripheral";
> +#endif
> +		break;
> +#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
> +	case USB_DR_MODE_HOST:
> +		debug("%s: dr_mode: HOST\n", __func__);
> +		driver = "dwc3-generic-host";
> +		break;
> +#endif
> +	default:
> +		debug("%s: unsupported dr_mode\n", __func__);
> +		return -ENODEV;
> +	};
> +
> +	if (!driver)
> +		return 0;
> +
> +	ret = device_bind_driver_to_node(parent, driver, name,
> +					 node, &dev);
> +	if (ret)
> +		debug("%s: not able to bind usb device mode\n",
> +		      __func__);
> +	return ret;
> +}
> +
>  static int dwc3_glue_bind(struct udevice *parent)
>  {
> +	struct dwc3_glue_ops *ops = (struct dwc3_glue_ops
> *)dev_get_driver_data(parent);
>  	ofnode node;
>  	int ret;
>  	enum usb_dr_mode dr_mode;
> 
> +	if (ops->single_node)
> +		return dwc3_glue_bind_node(parent, dev_ofnode(parent), 0);
> +
>  	dr_mode = usb_get_dr_mode(dev_ofnode(parent));
> 
>  	ofnode_for_each_subnode(node, dev_ofnode(parent)) {
> -		const char *name = ofnode_get_name(node);
> -		struct udevice *dev;
> -		const char *driver = NULL;
> -
> -		debug("%s: subnode name: %s\n", __func__, name);
> -
> -		/* if the parent node doesn't have a mode check the leaf */
> -		if (!dr_mode)
> -			dr_mode = usb_get_dr_mode(node);
> -
> -		switch (dr_mode) {
> -		case USB_DR_MODE_PERIPHERAL:
> -		case USB_DR_MODE_OTG:
> -#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> -			debug("%s: dr_mode: OTG or Peripheral\n", __func__);
> -			driver = "dwc3-generic-peripheral";
> -#endif
> -			break;
> -#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
> -		case USB_DR_MODE_HOST:
> -			debug("%s: dr_mode: HOST\n", __func__);
> -			driver = "dwc3-generic-host";
> -			break;
> -#endif
> -		default:
> -			debug("%s: unsupported dr_mode\n", __func__);
> -			return -ENODEV;
> -		};
> -
> -		if (!driver)
> -			continue;
> -
> -		ret = device_bind_driver_to_node(parent, driver, name,
> -						 node, &dev);
> -		if (ret) {
> -			debug("%s: not able to bind usb device mode\n",
> -			      __func__);
> +		ret = dwc3_glue_bind_node(parent, node, dr_mode);
> +		if (ret)
>  			return ret;
> -		}
>  	}
> 
>  	return 0;
> @@ -454,6 +467,10 @@ static int dwc3_glue_remove(struct udevice *dev)
>  	return 0;
>  }
> 
> +struct dwc3_glue_ops single_node_ops = {
> +	.single_node = 1,
> +};
> +
>  static const struct udevice_id dwc3_glue_ids[] = {
>  	{ .compatible = "xlnx,zynqmp-dwc3" },
>  	{ .compatible = "xlnx,versal-dwc3" },
> @@ -464,7 +481,7 @@ static const struct udevice_id dwc3_glue_ids[] = {
>  	{ .compatible = "rockchip,rk3328-dwc3" },
>  	{ .compatible = "rockchip,rk3399-dwc3" },
>  	{ .compatible = "qcom,dwc3" },
> -	{ .compatible = "fsl,imx8mq-dwc3" },
> +	{ .compatible = "fsl,imx8mq-dwc3", .data = (ulong)&single_node_ops },
>  	{ .compatible = "intel,tangier-dwc3" },
>  	{ }
>  };

Re: [PATCH] usb: dwc3-generic: Fix the iMX8MQ support

[Bedel, Alban]

[-- Attachment #1: Type: text/plain, Size: 7854 bytes --]

On Tue, 2022-04-19 at 07:34 -0700, Angus Ainslie wrote:
> Hi Alban,
> 
> On 2022-04-19 01:07, Alban Bedel wrote:
> > The binding of iMX8MQ USB controller doesn't use child nodes like the
> > other devices supported in this driver. To support it split the child
> > nodes parsing to its own function and add a field to the platform
> > data
> > to indicate that we should just use the top node.
> > 
> 
> I'm not clear on what this is fixing. Doesn't the original code deal 
> with a devicetree stanza that has the information in either the node or
> the parent.
>
> Which case does this fix ?

It fix the case where there is no top glue node and only a standalone
node. The driver currently do nothing in this case as it only act on
the sub nodes.

Now looking at the binding[1] we could as well just replace
"fsl,imx8mq-dwc3" with "snps,dwc3" in the OF match list as nothing
specific to the iMX8MQ is done in the code.

Regards,
Alban

1: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/usb/snps%2Cdwc3.yaml

> Thanks
> Angus
> 
> > Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
> > ---
> >  drivers/usb/dwc3/dwc3-generic.c | 95 +++++++++++++++++++------------
> > --
> >  1 file changed, 56 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-generic.c 
> > b/drivers/usb/dwc3/dwc3-generic.c
> > index 01bd0ca190e3..defef43ff503 100644
> > --- a/drivers/usb/dwc3/dwc3-generic.c
> > +++ b/drivers/usb/dwc3/dwc3-generic.c
> > @@ -221,6 +221,7 @@ U_BOOT_DRIVER(dwc3_generic_host) = {
> >  struct dwc3_glue_ops {
> >         void (*select_dr_mode)(struct udevice *dev, int index,
> >                                enum usb_dr_mode mode);
> > +       int single_node;
> >  };
> > 
> >  void dwc3_ti_select_dr_mode(struct udevice *dev, int index,
> > @@ -307,54 +308,66 @@ struct dwc3_glue_ops ti_ops = {
> >         .select_dr_mode = dwc3_ti_select_dr_mode,
> >  };
> > 
> > +static int dwc3_glue_bind_node(struct udevice *parent, ofnode node,
> > +                              enum usb_dr_mode dr_mode)
> > +{
> > +       const char *name = ofnode_get_name(node);
> > +       const char *driver = NULL;
> > +       struct udevice *dev;
> > +       int ret;
> > +
> > +       debug("%s: subnode name: %s\n", __func__, name);
> > +
> > +       /* if the parent node doesn't have a mode check the leaf */
> > +       if (!dr_mode)
> > +               dr_mode = usb_get_dr_mode(node);
> > +
> > +       switch (dr_mode) {
> > +       case USB_DR_MODE_PERIPHERAL:
> > +       case USB_DR_MODE_OTG:
> > +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> > +               debug("%s: dr_mode: OTG or Peripheral\n", __func__);
> > +               driver = "dwc3-generic-peripheral";
> > +#endif
> > +               break;
> > +#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
> > +       case USB_DR_MODE_HOST:
> > +               debug("%s: dr_mode: HOST\n", __func__);
> > +               driver = "dwc3-generic-host";
> > +               break;
> > +#endif
> > +       default:
> > +               debug("%s: unsupported dr_mode\n", __func__);
> > +               return -ENODEV;
> > +       };
> > +
> > +       if (!driver)
> > +               return 0;
> > +
> > +       ret = device_bind_driver_to_node(parent, driver, name,
> > +                                        node, &dev);
> > +       if (ret)
> > +               debug("%s: not able to bind usb device mode\n",
> > +                     __func__);
> > +       return ret;
> > +}
> > +
> >  static int dwc3_glue_bind(struct udevice *parent)
> >  {
> > +       struct dwc3_glue_ops *ops = (struct dwc3_glue_ops
> > *)dev_get_driver_data(parent);
> >         ofnode node;
> >         int ret;
> >         enum usb_dr_mode dr_mode;
> > 
> > +       if (ops->single_node)
> > +               return dwc3_glue_bind_node(parent,
> > dev_ofnode(parent), 0);
> > +
> >         dr_mode = usb_get_dr_mode(dev_ofnode(parent));
> > 
> >         ofnode_for_each_subnode(node, dev_ofnode(parent)) {
> > -               const char *name = ofnode_get_name(node);
> > -               struct udevice *dev;
> > -               const char *driver = NULL;
> > -
> > -               debug("%s: subnode name: %s\n", __func__, name);
> > -
> > -               /* if the parent node doesn't have a mode check the
> > leaf */
> > -               if (!dr_mode)
> > -                       dr_mode = usb_get_dr_mode(node);
> > -
> > -               switch (dr_mode) {
> > -               case USB_DR_MODE_PERIPHERAL:
> > -               case USB_DR_MODE_OTG:
> > -#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> > -                       debug("%s: dr_mode: OTG or Peripheral\n",
> > __func__);
> > -                       driver = "dwc3-generic-peripheral";
> > -#endif
> > -                       break;
> > -#if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
> > -               case USB_DR_MODE_HOST:
> > -                       debug("%s: dr_mode: HOST\n", __func__);
> > -                       driver = "dwc3-generic-host";
> > -                       break;
> > -#endif
> > -               default:
> > -                       debug("%s: unsupported dr_mode\n", __func__);
> > -                       return -ENODEV;
> > -               };
> > -
> > -               if (!driver)
> > -                       continue;
> > -
> > -               ret = device_bind_driver_to_node(parent, driver,
> > name,
> > -                                                node, &dev);
> > -               if (ret) {
> > -                       debug("%s: not able to bind usb device
> > mode\n",
> > -                             __func__);
> > +               ret = dwc3_glue_bind_node(parent, node, dr_mode);
> > +               if (ret)
> >                         return ret;
> > -               }
> >         }
> > 
> >         return 0;
> > @@ -454,6 +467,10 @@ static int dwc3_glue_remove(struct udevice *dev)
> >         return 0;
> >  }
> > 
> > +struct dwc3_glue_ops single_node_ops = {
> > +       .single_node = 1,
> > +};
> > +
> >  static const struct udevice_id dwc3_glue_ids[] = {
> >         { .compatible = "xlnx,zynqmp-dwc3" },
> >         { .compatible = "xlnx,versal-dwc3" },
> > @@ -464,7 +481,7 @@ static const struct udevice_id dwc3_glue_ids[] =
> > {
> >         { .compatible = "rockchip,rk3328-dwc3" },
> >         { .compatible = "rockchip,rk3399-dwc3" },
> >         { .compatible = "qcom,dwc3" },
> > -       { .compatible = "fsl,imx8mq-dwc3" },
> > +       { .compatible = "fsl,imx8mq-dwc3", .data =
> > (ulong)&single_node_ops },
> >         { .compatible = "intel,tangier-dwc3" },
> >         { }
> >  };


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3627 bytes --]

Re: [PATCH] usb: dwc3-generic: Fix the iMX8MQ support

[Bedel, Alban]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

Hi Angus,
On Tue, 2022-04-19 at 07:34 -0700, Angus Ainslie wrote:
> Hi Alban,
> On 2022-04-19 01:07, Alban Bedel wrote:
> > The binding of iMX8MQ USB controller doesn't use child nodes like
> > the other devices supported in this driver. To support it split the
> > child nodes parsing to its own function and add a field to the
> > platform data to indicate that we should just use the top node.
> > 
> 
> I'm not clear on what this is fixing. Doesn't the original code deal 
> with a devicetree stanza that has the information in either the node
> or the parent.

I have now see your commit c08db05455bc ("usb: dwc3: dwc3-generic:
check the parent nodes") which added the compatible for the imx8mq and
reference the librem5 DTS. This DTS has `port` subnodes to define the
connection to a type C connector. So with the compatible string you
added the glue driver would interpret the port nodes as DWC3 nodes,
hence the need to get properties from the parent node.

I think we should revert this patch and add support for standalone DWC3
devices instead. We could also improve the glue driver to only handle
subnode with the proper compatible.

Alban

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3627 bytes --]