RE: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hello Amelie,

thank you for this patch - I am hoping that it will help us on Amlogic
Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
On these SoCs the ID detection is performed by the PHY IP and needs to
be polled.
I think usb_role_switch is the perfect framework for this on dwc2 side.
For the PHY driver I'm going to implement the cable state using the
extcon framework and then having a new usb-conn-extcon driver. This is
just to give you an overview why I'm interested in this.

[...]
> +static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> +{
> +	struct dwc2_hsotg *hsotg = usb_role_switch_get_drvdata(sw);
> +	unsigned long flags;
> +
> +	/* Skip session not in line with dr_mode */
> +	if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
> +	    (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
> +		return -EINVAL;
> +
> +	/* Skip session if core is in test mode */
> +	if (role == USB_ROLE_NONE && hsotg->test_mode) {
> +		dev_dbg(hsotg->dev, "Core is in test mode\n");
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
due to this spin_lock_irqsave() ...

> +	if (role == USB_ROLE_HOST) {
> +		if (dwc2_ovr_avalid(hsotg, true))
> +			goto unlock;
> +
> +		if (hsotg->dr_mode == USB_DR_MODE_OTG)
> +			/*
> +			 * This will raise a Connector ID Status Change
> +			 * Interrupt - connID A
> +			 */
> +			dwc2_force_mode(hsotg, true);
... we cannot sleep in here. the call flow is:
dwc2_drd_role_sw_set
  spin_lock_irqsave
  dwc2_force_mode
    dwc2_wait_for_mode
      usleep_range

> +	} else if (role == USB_ROLE_DEVICE) {
> +		if (dwc2_ovr_bvalid(hsotg, true))
> +			goto unlock;
> +
> +		if (hsotg->dr_mode == USB_DR_MODE_OTG)
> +			/*
> +			 * This will raise a Connector ID Status Change
> +			 * Interrupt - connID B
> +			 */
> +			dwc2_force_mode(hsotg, false);
(same sleeping issue here)

[...]
+int dwc2_drd_init(struct dwc2_hsotg *hsotg)
+{
+	struct usb_role_switch_desc role_sw_desc = {0};
+	struct usb_role_switch *role_sw;
+	int ret;
+
+	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
+		return 0;
should we also return early here if dr_mode != "otg"?

[...]
@@ -532,6 +534,13 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		dwc2_writel(hsotg, ggpio, GGPIO);
 	}
 
+	retval = dwc2_drd_init(hsotg);
+	if (retval) {
+		if (retval != -EPROBE_DEFER)
+			dev_err(hsotg->dev, "failed to initialize dual-role\n");
+		goto error_init;
+	}
+
 	if (hsotg->dr_mode != USB_DR_MODE_HOST) {
 		retval = dwc2_gadget_init(hsotg);
 		if (retval)
I think dwc2_driver_probe() needs a new label (for example named
error_drd) which then calls dwc2_drd_exit. See [0] which I have
submitted as a patch for Linux 5.8, so it's not in usb-next yet.

Also in general I think you need to submit a dt-bindings patch that
documents the usb-role-switch property. Personally I would use
Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml as
reference for that.

Can you please keep me Cc'ed on a v2 because I'm not subscribed to the
linux-usb mailing list?
I am going to test this on Amlogic SoCs - once I made "everything else"
work I can give my Tested-by as well.


Thank you!
Martin


[0] https://patchwork.kernel.org/patch/11642957/