[PATCH 1/2] dt-bindings: usb: dwc3: Document role-switch-reset-quirk

[PATCH 1/2] dt-bindings: usb: dwc3: Document role-switch-reset-quirk

[Sven Peter]

The dwc3 controller on the Apple M1 must be reset whenever a
device is unplugged from the root port and triggers a role
switch notification. Document the quirk to enable this behavior.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 25ac2c93dc6c..9635e20cab68 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -226,6 +226,12 @@ properties:
       avoid -EPROTO errors with usbhid on some devices (Hikey 970).
     type: boolean
 
+  snps,role-switch-reset-quirk:
+    description:
+      When set, DWC3 will be reset and reinitialized whenever a role switch
+      is performed.
+    type: boolean
+
   snps,is-utmi-l1-suspend:
     description:
       True when DWC3 asserts output signal utmi_l1_suspend_n, false when
-- 
2.25.1

[PATCH 2/2] usb: dwc3: Add role-switch-reset-quirk logic

[Sven Peter]

As mad as it sounds, the dwc3 controller present on the Apple M1 must be
reset and reinitialized whenever a device is unplugged from the root port
and triggers a role switch notification from the USB PD controller.

This is required for at least two reasons:

  - The USB2 D+/D- lines are connected through a stateful eUSB2 repeater
    which in turn is controlled by a variant of the TI TPS6598x USB PD
    chip. When the USB PD controller detects a hotplug event it resets
    the eUSB2 repeater. Afterwards, no new device is recognized before
    the DWC3 core and PHY are reset as well.

  - It's possible to completely break the dwc3 controller by switching
    it to device mode and unplugging the cable at just the wrong time.
    Even a CORESOFTRESET is not enough to allow new devices again.
    The only workaround is to trigger a hard reset of the entire
    dwc3 core. This also happens when running macOS on these
    machines.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/dwc3/core.c | 40 +++++++++++++++++++++++++++++++++++++---
 drivers/usb/dwc3/core.h |  6 ++++++
 drivers/usb/dwc3/drd.c  |  7 +++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 643239d7d370..444b45e9cb92 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -116,6 +116,8 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 }
 
 static int dwc3_core_soft_reset(struct dwc3 *dwc);
+static void dwc3_core_exit(struct dwc3 *dwc);
+static int dwc3_core_init_for_resume(struct dwc3 *dwc);
 
 static void __dwc3_set_mode(struct work_struct *work)
 {
@@ -131,10 +133,11 @@ static void __dwc3_set_mode(struct work_struct *work)
 	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)
 		dwc3_otg_update(dwc, 0);
 
-	if (!dwc->desired_dr_role)
+	if (!dwc->desired_dr_role && !dwc->role_switch_reset_quirk)
 		goto out;
 
-	if (dwc->desired_dr_role == dwc->current_dr_role)
+	if (dwc->desired_dr_role == dwc->current_dr_role &&
+			!dwc->role_switch_reset_quirk)
 		goto out;
 
 	if (dwc->desired_dr_role == DWC3_GCTL_PRTCAP_OTG && dwc->edev)
@@ -159,6 +162,34 @@ static void __dwc3_set_mode(struct work_struct *work)
 		break;
 	}
 
+	if (dwc->role_switch_reset_quirk) {
+		if (dwc->current_dr_role) {
+			dwc->current_dr_role = 0;
+			dwc3_core_exit(dwc);
+		}
+
+		if (dwc->desired_dr_role) {
+			/*
+			 * the first call to __dwc3_set_mode comes from
+			 * dwc3_drd_init. In that case dwc3_core_init has been
+			 * called but dwc->current_dr_role is zero such that
+			 * we must not reinitialize the core again here.
+			 */
+			if (dwc->role_switch_reset_quirk_initialized) {
+				ret = dwc3_core_init_for_resume(dwc);
+				if (ret) {
+					dev_err(dwc->dev,
+					    "failed to reinitialize core\n");
+					goto out;
+				}
+			}
+
+			dwc->role_switch_reset_quirk_initialized = 1;
+		} else {
+			goto out;
+		}
+	}
+
 	/* For DRD host or device mode only */
 	if (dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
 		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
@@ -1425,6 +1456,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
 
+	dwc->role_switch_reset_quirk = device_property_read_bool(dev,
+				"snps,role-switch-reset-quirk");
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -1744,7 +1778,6 @@ static int dwc3_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
 static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 {
 	int ret;
@@ -1771,6 +1804,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 	return ret;
 }
 
+#ifdef CONFIG_PM
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ee854697c300..04b1b9c2bbed 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1086,6 +1086,9 @@ struct dwc3_scratchpad_array {
  *	3	- Reserved
  * @dis_metastability_quirk: set to disable metastability quirk.
  * @dis_split_quirk: set to disable split boundary.
+ * @role_switch_reset_quirk: set to force reinitialization after any role switch
+ * @role_switch_reset_quirk_initialized: set to true after the first role switch
+ *			which is triggered from dwc3_drd_init directly
  * @imod_interval: set the interrupt moderation interval in 250ns
  *			increments or 0 to disable.
  * @max_cfg_eps: current max number of IN eps used across all USB configs.
@@ -1299,6 +1302,9 @@ struct dwc3 {
 	unsigned		dis_split_quirk:1;
 	unsigned		async_callbacks:1;
 
+	unsigned		role_switch_reset_quirk:1;
+	unsigned		role_switch_reset_quirk_initialized:1;
+
 	u16			imod_interval;
 
 	int			max_cfg_eps;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index d7f76835137f..403e88a72f0e 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -506,6 +506,9 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
 		break;
 	}
 
+	if (dwc->role_switch_reset_quirk && role == USB_ROLE_NONE)
+		mode = 0;
+
 	dwc3_set_mode(dwc, mode);
 	return 0;
 }
@@ -534,6 +537,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
 			role = USB_ROLE_DEVICE;
 		break;
 	}
+
+	if (dwc->role_switch_reset_quirk && !dwc->current_dr_role)
+		role = USB_ROLE_NONE;
+
 	spin_unlock_irqrestore(&dwc->lock, flags);
 	return role;
 }
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: usb: dwc3: Document role-switch-reset-quirk

[Rob Herring]

On Sun, Oct 17, 2021 at 02:59:03PM +0200, Sven Peter wrote:
> The dwc3 controller on the Apple M1 must be reset whenever a
> device is unplugged from the root port and triggers a role
> switch notification. Document the quirk to enable this behavior.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 25ac2c93dc6c..9635e20cab68 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -226,6 +226,12 @@ properties:
>        avoid -EPROTO errors with usbhid on some devices (Hikey 970).
>      type: boolean
>  
> +  snps,role-switch-reset-quirk:
> +    description:
> +      When set, DWC3 will be reset and reinitialized whenever a role switch
> +      is performed.
> +    type: boolean

This binding is a example of why we don't do a property per quirk. We 
end up with a gazillion of them.

Imply this from the SoC specific compatible (I don't recall seeing one 
for the M1, so that's a problem).

Rob

Re: [PATCH 1/2] dt-bindings: usb: dwc3: Document role-switch-reset-quirk

[Sven Peter]

On Wed, Oct 27, 2021, at 01:25, Rob Herring wrote:
> On Sun, Oct 17, 2021 at 02:59:03PM +0200, Sven Peter wrote:
>> The dwc3 controller on the Apple M1 must be reset whenever a
>> device is unplugged from the root port and triggers a role
>> switch notification. Document the quirk to enable this behavior.
>> 
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>> ---
>>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index 25ac2c93dc6c..9635e20cab68 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -226,6 +226,12 @@ properties:
>>        avoid -EPROTO errors with usbhid on some devices (Hikey 970).
>>      type: boolean
>>  
>> +  snps,role-switch-reset-quirk:
>> +    description:
>> +      When set, DWC3 will be reset and reinitialized whenever a role switch
>> +      is performed.
>> +    type: boolean
>
> This binding is a example of why we don't do a property per quirk. We 
> end up with a gazillion of them.

Makes sense. I didn't think too much about this since I saw all those
other quirks and just did the same.

>
> Imply this from the SoC specific compatible (I don't recall seeing one 
> for the M1, so that's a problem).

Sure, I'll do that for v2. The compatible for the M1 doesn't exist yet
because the dwc3 nodes also don't exist in the upstream DT yet.
This was all blocked on getting I2C and the USB PD chip to work.



Sven