Re: [PATCH RFC 04/22] usb: dwc3: apple: Reset dwc3 during role switches

[Sven Peter <sven@kernel.org>]

On 22.08.25 01:25, Thinh Nguyen wrote:
> On Thu, Aug 21, 2025, Sven Peter wrote:
>> 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 or when the PHY mode is changed.
>>
>> This is required for at least the following 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 because the eUSB2 repeater
>>      and the PHY/dwc3 block disagree about the current state.
>>
>>    - It's possible to completely break the dwc3 controller by switching
>>      it to device mode and unplugging the cable at just the wrong time.
>>      If this happens dwc3 behaves as if no device is connected.
>>      CORESOFTRESET will also never clear after it has been set. The only
>>      workaround is to trigger a hard reset of the entire dwc3 core with
>>      its external reset line.
>>
>>    - Whenever the PHY mode is changed (to e.g. transition to DisplayPort
>>      alternate mode or USB4) dwc3 has to be shutdown and reinitialized.
>>      Otherwise the Type-C port will not be usable until the entire SoC
>>      has been reset.
>>
>> All of this can be easily worked around by respecting transitions to
>> USB_ROLE_NONE and making sure the external reset line is asserted when
>> switching roles. We additionally have to ensure that the PHY is
>> suspended during init.
>>
>> Signed-off-by: Sven Peter <sven@kernel.org>
>> ---
>>   drivers/usb/dwc3/core.c | 61 +++++++++++++++++++++++++++++++++++++++++++++---


[...]

> 
>> +			dwc3_core_exit(dwc);
>> +		}
>> +
>> +		if (desired_dr_role) {
>> +			ret = dwc3_core_init_for_resume(dwc);
> 
> The dwc3_core_init_for_resume() is for PM, reusing this with its
> current name is confusing.

Ack, I was going to clean this up later and wanted to get feedback on 
this entire approach first. Won't be used anymore when moving to a 
glue.h based approach anyway.

> 
>> +			if (ret) {
>> +				dev_err(dwc->dev,
>> +				    "failed to reinitialize core\n");
>> +				goto out;
>> +			}
>> +		} else {
>> +			goto out;
>> +		}
>> +	}
>> +
>>   	/*
>>   	 * When current_dr_role is not set, there's no role switching.
>>   	 * Only perform GCTL.CoreSoftReset when there's DRD role switching.
>>   	 */
>> -	if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
>> +	if (dwc->role_switch_reset_quirk ||
> 
> Don't override the use of GCTL.CoreSoftReset with this quirk. Not all
> controller versions should use GCTL.CoreSoftReset, the new controller
> version don't even have it. What version is this vendor using?
> 
> I'm concern how this condition is needed...

This is actually a leftover from the first attempts at making this work. 
I didn't know about the external reset line back then and had to 
soft-reset it here because it would not see new devices otherwise IIRC.
Since we're going through a hard-reset now anyway this can be dropped
and this entire commit will disappear in favor of a glue.h based driver 
anyway.

> 
>> +		(dwc->current_dr_role && ((DWC3_IP_IS(DWC3) ||
>>   			DWC3_VER_IS_PRIOR(DWC31, 190A)) &&
>> -			desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) {
>> +			desired_dr_role != DWC3_GCTL_PRTCAP_OTG))) {
>>   		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>>   		reg |= DWC3_GCTL_CORESOFTRESET;
>>   		dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> @@ -1372,6 +1394,9 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	if (ret)
>>   		goto err_exit_phy;
>>   
>> +	if (dwc->role_switch_reset_quirk)
>> +		dwc3_enable_susphy(dwc, true);
>> +
> 
> Why do you need to enable susphy here?

The only place we actually need it is when we shut down the Type-C PHY 
due some what I assume is some hardware quirk, i.e. just before 
dwc3_core_exit.

The PHY will otherwise not be able to acquire some hardware lock (which 
they call PIPEHANDLER lock in debug strings) to switch from e.g. USB3 
PHY to a dummy PHY for USB2 only. It then can't shut down cleanly 
anymore and will get stuck in a weird state where the port refuses to 
work until I reset everything.
Originally it was added because we just undid some commit where susphy 
handling was made unconditional IIRC.

I'll move this to the glue driver with a comment explaining why it's 
required.


> 
>>   	dwc3_core_setup_global_control(dwc);

[...]

>> +	if (dev->of_node) {
>> +		if (of_device_is_compatible(dev->of_node, "apple,t8103-dwc3")) {
>> +			if (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) ||
>> +			    !IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
>> +				dev_err(dev,
>> +				    "Apple DWC3 requires role switch support.\n"
>> +				    );
>> +				ret = -EINVAL;
>> +				goto err_put_psy;
>> +			}
>> +
>> +			dwc->dr_mode = USB_DR_MODE_OTG;
>> +			dwc->role_switch_reset_quirk = true;
> 
> Put this in your glue driver or device tree.

Ack.

> 
>> +		}
>> +	}

[...]

>> +
>> +	if (dwc->role_switch_reset_quirk && !dwc->current_dr_role)
>> +		role = USB_ROLE_NONE;
> 
> Don't return USB_ROLE_NONE on role_switch get. The USB_ROLE_NONE is the
> default role. The role_switch get() should return exactly which role the
> controller is currently in, and the driver can figure that out.

Ack, will also happen from inside the glue driver now.




Sven