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

[Janne Grunau <j@jannau.net>]

On Thu, Aug 21, 2025 at 11:25:51PM +0000, 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 +++++++++++++++++++++++++++++++++++++++++++++----
> >  drivers/usb/dwc3/core.h |  3 +++
> >  drivers/usb/dwc3/drd.c  | 11 ++++++++-
> >  drivers/usb/dwc3/host.c |  3 ++-
> >  4 files changed, 72 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 8002c23a5a02acb8f3e87b2662a53998a4cf4f5c..26aa507a738f001409a97ef563c6561433a1cac5 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c

...

> >  	/*
> >  	 * 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?

It reports DWC31, revision 190A and the unlisted version_types
0x736f3035 (M1), 0x736f3039 (M1 Pro/Max/Ultra) and 0x736f3130 (M2, M2
Pro/Max/Ultra).

> I'm concern how this condition is needed...
> 
> > +		(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);

Janne