[RFC/PATCH 0/2] dwc3/rockchip orientation fixes

[RFC/PATCH 0/2] dwc3/rockchip orientation fixes

[John Keeping]

This series attempts to fix an issue with using the USB PHY on RK3588 as
an orientation switch.  The phy driver only updates its internal state
and does not write to the hardware when notified of an orientation
switch.

An early patch addressing this issue [1] updated the hardware directly,
but this changes the phy state underneath the USB controller when it is
not expecting the state to change.  That was addressed in [2] but my
testing of that shows runtime PM overflows that cause the device to be
left disabled unexpectedly.

This approach updates the dwc3 driver so that it always signals a phy
mode change when a plug event occurs, allowing the phy to reset safely
at a point when the USB controller expects it to do so.

The dwc3 changes seem to work in my testing, but I have no idea of the
full implications of setting an "unsupported" role.

[1] https://lore.kernel.org/r/20250226103810.3746018-1-heiko@sntech.de
[2] https://lore.kernel.org/r/20250610-rk3576-sige5-usb-v4-0-7e7f779619c1@collabora.com

John Keeping (2):
  usb: dwc3: disable for USB_ROLE_NONE
  phy: rockchip: usbdp: implement .set_mode

 drivers/phy/rockchip/phy-rockchip-usbdp.c | 14 ++++++++++++++
 drivers/usb/dwc3/core.c                   |  3 ---
 drivers/usb/dwc3/drd.c                    |  5 +----
 3 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.50.0

[RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[John Keeping]

When the phy is acting as a Type C mux, it may need to reset when the
cable direction changes.  But this should not happen while DWC3 is
trying to use the USB connection.

In this case, there must be a connection manager to notify the phy of
the orientation change and tcpm_mux_set() ensures this happens before
DWC3's role switch is informed of a change.

It should not be possible to go directly from device->device or
host->host with a change in orientation without transitioning through
the "none" role as the cable is unplugged.  So ensuring that DWC3 always
informs the phy of the new mode whenever a plug is detected should be
sufficient for the phy to safely reset itself at a time that is safe for
DWC3.

Lifting the special-case for desired_dr_mode==0 in __dwc3_set_mode() is
sufficient to allow using the unset mode for USB_ROLE_NONE.  The
handling already disables the old mode and then simply does not enable a
new one.

If an external device is notifying USB role switches, then it is not
necessary to set the default role when USB_ROLE_NONE is passed.

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 drivers/usb/dwc3/core.c | 3 ---
 drivers/usb/dwc3/drd.c  | 5 +----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8002c23a5a02a..6573cca0eeaf5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -177,9 +177,6 @@ static void __dwc3_set_mode(struct work_struct *work)
 	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)
 		dwc3_otg_update(dwc, 0);
 
-	if (!desired_dr_role)
-		goto out;
-
 	if (desired_dr_role == dwc->current_dr_role)
 		goto out;
 
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 7977860932b14..8f427afa8eb93 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -457,10 +457,7 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
 		mode = DWC3_GCTL_PRTCAP_DEVICE;
 		break;
 	default:
-		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
-			mode = DWC3_GCTL_PRTCAP_HOST;
-		else
-			mode = DWC3_GCTL_PRTCAP_DEVICE;
+		mode = 0;
 		break;
 	}
 
-- 
2.50.0

Re: [RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[Thinh Nguyen]

On Thu, Jul 10, 2025, John Keeping wrote:
> When the phy is acting as a Type C mux, it may need to reset when the
> cable direction changes.  But this should not happen while DWC3 is
> trying to use the USB connection.
> 
> In this case, there must be a connection manager to notify the phy of
> the orientation change and tcpm_mux_set() ensures this happens before
> DWC3's role switch is informed of a change.
> 
> It should not be possible to go directly from device->device or
> host->host with a change in orientation without transitioning through
> the "none" role as the cable is unplugged.  So ensuring that DWC3 always

The controller is either host or device mode. It's odd to use "none" to
indicate connection.

> informs the phy of the new mode whenever a plug is detected should be
> sufficient for the phy to safely reset itself at a time that is safe for
> DWC3.

Couldn't the phy do this as it detects connection/disconnection.

It seems what you need is a full controller initialization upon new
connection on orientation change, and you're using role_switch selecting
"none" to do so.

I'm not sure if role-switch has the right interface to do so. Perhaps we
can introduce one? I don't think we should change the behavior of the
current flow and apply that to all other platforms.

Thanks,
Thinh

> 
> Lifting the special-case for desired_dr_mode==0 in __dwc3_set_mode() is
> sufficient to allow using the unset mode for USB_ROLE_NONE.  The
> handling already disables the old mode and then simply does not enable a
> new one.
> 
> If an external device is notifying USB role switches, then it is not
> necessary to set the default role when USB_ROLE_NONE is passed.

Re: [RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[John Keeping]

On Sat, Jul 12, 2025 at 12:11:38AM +0000, Thinh Nguyen wrote:
> On Thu, Jul 10, 2025, John Keeping wrote:
> > When the phy is acting as a Type C mux, it may need to reset when the
> > cable direction changes.  But this should not happen while DWC3 is
> > trying to use the USB connection.
> > 
> > In this case, there must be a connection manager to notify the phy of
> > the orientation change and tcpm_mux_set() ensures this happens before
> > DWC3's role switch is informed of a change.
> > 
> > It should not be possible to go directly from device->device or
> > host->host with a change in orientation without transitioning through
> > the "none" role as the cable is unplugged.  So ensuring that DWC3 always
> 
> The controller is either host or device mode. It's odd to use "none" to
> indicate connection.

There is no connection in this state.  When the type C controller
indicates that the role is "none" then there cannot be a USB connection.

> > informs the phy of the new mode whenever a plug is detected should be
> > sufficient for the phy to safely reset itself at a time that is safe for
> > DWC3.
> 
> Couldn't the phy do this as it detects connection/disconnection.

I don't think the phy can detect a connection.  If it is configured for
the wrong orientation then it will not monitor the correct data lines.
The phy hardware does not signal any interrupts to the CPU for the
driver to react to.

> It seems what you need is a full controller initialization upon new
> connection on orientation change, and you're using role_switch selecting
> "none" to do so.

I'm not sure whether a complete initialization is necessary, but what I
want to avoid is the phy resetting while the controller is part-way
through device enumeration or setting up a gadget configuration.

It may be that simply avoiding resetting the phy if the orientation is
unchanged is enough to avoid some of this problem, but I suspect there
are still problems if the clocks from the phy to the controller are
stopped unexpectedly.  However, I have run some tests of this change and
it looks promising.

> I'm not sure if role-switch has the right interface to do so. Perhaps we
> can introduce one? I don't think we should change the behavior of the
> current flow and apply that to all other platforms.

I don't think it's unreasonable for the controller to be idle when there
is an external type C controller notifying the connection state.

The hardware setup looks like this, with the Linux type C code notifying
the phy driver of the orientation change and the DWC3 driver of the role
change:

                      +------+          
                      | DWC3 |<----+    
                      +--^---+     |    
                         |         |    
                      +--v--+      |    
                +---->| PHY |      |Role
                |     +--^--+      |    
    Orientation |        |         |    
                |   +----v----+    |    
                +---+ FUSB302 +----+    
                    +---------+

The advantage of using the role hook is that we are guaranteed that it
is called after the phy has been notified of the orientation change.

Do you have an idea of a new interface?  Or do you think it is safe to
reset the phy underneath the controller when that will halt the clocks
from the phy to the controller for some period of time?


Regards,
John

Re: [RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[Thinh Nguyen]

Hi,

Just got back from vacation, sorry for the delay response.

On Mon, Jul 14, 2025, John Keeping wrote:
> On Sat, Jul 12, 2025 at 12:11:38AM +0000, Thinh Nguyen wrote:
> > On Thu, Jul 10, 2025, John Keeping wrote:
> > > When the phy is acting as a Type C mux, it may need to reset when the
> > > cable direction changes.  But this should not happen while DWC3 is
> > > trying to use the USB connection.
> > > 
> > > In this case, there must be a connection manager to notify the phy of
> > > the orientation change and tcpm_mux_set() ensures this happens before
> > > DWC3's role switch is informed of a change.
> > > 
> > > It should not be possible to go directly from device->device or
> > > host->host with a change in orientation without transitioning through
> > > the "none" role as the cable is unplugged.  So ensuring that DWC3 always
> > 
> > The controller is either host or device mode. It's odd to use "none" to
> > indicate connection.
> 
> There is no connection in this state.  When the type C controller
> indicates that the role is "none" then there cannot be a USB connection.
> 

It's not about connection. It's about the current mode of the USB
controller. The mode of the controller isn't "none" on disconnection
even if you try to role-switch to that via the typec switch.

> > > informs the phy of the new mode whenever a plug is detected should be
> > > sufficient for the phy to safely reset itself at a time that is safe for
> > > DWC3.
> > 
> > Couldn't the phy do this as it detects connection/disconnection.
> 
> I don't think the phy can detect a connection.  If it is configured for
> the wrong orientation then it will not monitor the correct data lines.
> The phy hardware does not signal any interrupts to the CPU for the
> driver to react to.

Ok.

> 
> > It seems what you need is a full controller initialization upon new
> > connection on orientation change, and you're using role_switch selecting
> > "none" to do so.
> 
> I'm not sure whether a complete initialization is necessary, but what I
> want to avoid is the phy resetting while the controller is part-way
> through device enumeration or setting up a gadget configuration.
> 
> It may be that simply avoiding resetting the phy if the orientation is
> unchanged is enough to avoid some of this problem, but I suspect there
> are still problems if the clocks from the phy to the controller are
> stopped unexpectedly.  However, I have run some tests of this change and
> it looks promising.

That sounds like the the phy reset is done after asserting the vbus
valid line indicating connection to the controller. Can this be fixed in
the phy or type-c driver?

> 
> > I'm not sure if role-switch has the right interface to do so. Perhaps we
> > can introduce one? I don't think we should change the behavior of the
> > current flow and apply that to all other platforms.
> 
> I don't think it's unreasonable for the controller to be idle when there
> is an external type C controller notifying the connection state.
> 
> The hardware setup looks like this, with the Linux type C code notifying
> the phy driver of the orientation change and the DWC3 driver of the role
> change:
> 
>                       +------+          
>                       | DWC3 |<----+    
>                       +--^---+     |    
>                          |         |    
>                       +--v--+      |    
>                 +---->| PHY |      |Role
>                 |     +--^--+      |    
>     Orientation |        |         |    
>                 |   +----v----+    |    
>                 +---+ FUSB302 +----+    
>                     +---------+
> 
> The advantage of using the role hook is that we are guaranteed that it
> is called after the phy has been notified of the orientation change.
> 
> Do you have an idea of a new interface?  Or do you think it is safe to
> reset the phy underneath the controller when that will halt the clocks
> from the phy to the controller for some period of time?

The proper fix should be in the phy where it should only assert vbus
valid after processing orientation and phy reset. I'm not familiar on
the communication between your PHY and FUSB302. There should be some
indication from the phy that it's ready before you can assert vbus-valid
(that can be from a callback, a status register etc).

What you've done is a teardown and reinit of the driver, triggering a
soft-reset. This can re-sync the phy. That probably takes long enough
that a phy-reset after orientation change will be completed before the
initialization complete.

If there's no way for you to know when the phy complete its reset, we
can add a quirk for your platform to always reinit on role-switch call.

Let me know if this is reasonable.

BR,
Thinh

Re: [RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[John Keeping]

On Tue, Jul 29, 2025 at 10:10:35PM +0000, Thinh Nguyen wrote:
> On Mon, Jul 14, 2025, John Keeping wrote:
> > On Sat, Jul 12, 2025 at 12:11:38AM +0000, Thinh Nguyen wrote:
> > > On Thu, Jul 10, 2025, John Keeping wrote:
> > > > When the phy is acting as a Type C mux, it may need to reset when the
> > > > cable direction changes.  But this should not happen while DWC3 is
> > > > trying to use the USB connection.
> > > > 
> > > > In this case, there must be a connection manager to notify the phy of
> > > > the orientation change and tcpm_mux_set() ensures this happens before
> > > > DWC3's role switch is informed of a change.
> > > > 
> > > > It should not be possible to go directly from device->device or
> > > > host->host with a change in orientation without transitioning through
> > > > the "none" role as the cable is unplugged.  So ensuring that DWC3 always
> > > 
> > > The controller is either host or device mode. It's odd to use "none" to
> > > indicate connection.
> > 
> > There is no connection in this state.  When the type C controller
> > indicates that the role is "none" then there cannot be a USB connection.
> > 
> 
> It's not about connection. It's about the current mode of the USB
> controller. The mode of the controller isn't "none" on disconnection
> even if you try to role-switch to that via the typec switch.
> 
> > > > informs the phy of the new mode whenever a plug is detected should be
> > > > sufficient for the phy to safely reset itself at a time that is safe for
> > > > DWC3.
> > > 
> > > Couldn't the phy do this as it detects connection/disconnection.
> > 
> > I don't think the phy can detect a connection.  If it is configured for
> > the wrong orientation then it will not monitor the correct data lines.
> > The phy hardware does not signal any interrupts to the CPU for the
> > driver to react to.
> 
> Ok.
> 
> > 
> > > It seems what you need is a full controller initialization upon new
> > > connection on orientation change, and you're using role_switch selecting
> > > "none" to do so.
> > 
> > I'm not sure whether a complete initialization is necessary, but what I
> > want to avoid is the phy resetting while the controller is part-way
> > through device enumeration or setting up a gadget configuration.
> > 
> > It may be that simply avoiding resetting the phy if the orientation is
> > unchanged is enough to avoid some of this problem, but I suspect there
> > are still problems if the clocks from the phy to the controller are
> > stopped unexpectedly.  However, I have run some tests of this change and
> > it looks promising.
> 
> That sounds like the the phy reset is done after asserting the vbus
> valid line indicating connection to the controller. Can this be fixed in
> the phy or type-c driver?

I don't think so.  I've tried implementing this in the phy driver and it
does not work reliably.

> > > I'm not sure if role-switch has the right interface to do so. Perhaps we
> > > can introduce one? I don't think we should change the behavior of the
> > > current flow and apply that to all other platforms.
> > 
> > I don't think it's unreasonable for the controller to be idle when there
> > is an external type C controller notifying the connection state.
> > 
> > The hardware setup looks like this, with the Linux type C code notifying
> > the phy driver of the orientation change and the DWC3 driver of the role
> > change:
> > 
> >                       +------+          
> >                       | DWC3 |<----+    
> >                       +--^---+     |    
> >                          |         |    
> >                       +--v--+      |    
> >                 +---->| PHY |      |Role
> >                 |     +--^--+      |    
> >     Orientation |        |         |    
> >                 |   +----v----+    |    
> >                 +---+ FUSB302 +----+    
> >                     +---------+
> > 
> > The advantage of using the role hook is that we are guaranteed that it
> > is called after the phy has been notified of the orientation change.
> > 
> > Do you have an idea of a new interface?  Or do you think it is safe to
> > reset the phy underneath the controller when that will halt the clocks
> > from the phy to the controller for some period of time?
> 
> The proper fix should be in the phy where it should only assert vbus
> valid after processing orientation and phy reset. I'm not familiar on
> the communication between your PHY and FUSB302. There should be some
> indication from the phy that it's ready before you can assert vbus-valid
> (that can be from a callback, a status register etc).

The FUSB302 is connected to the CPU via I2C, there is no direct
communication between it and the phy, it all goes via the kernel's tcpm
subsystem.

The problem with re-initializing the phy is that this causes dwc3 gadget
configuration to fail with "failed to enable ep0out", and this happens
when VBUS valid is de-asserted.  This is 100% reproducible on boot for
me where the init scripts are setting up a gadget via configfs.

So it seems the timing of the phy setup needs to coordinate with the
dwc3 controller.

> What you've done is a teardown and reinit of the driver, triggering a
> soft-reset. This can re-sync the phy. That probably takes long enough
> that a phy-reset after orientation change will be completed before the
> initialization complete.
> 
> If there's no way for you to know when the phy complete its reset, we
> can add a quirk for your platform to always reinit on role-switch call.
> 
> Let me know if this is reasonable.

I'm happy to test patches.


Kind regards,
John

Re: [RFC/PATCH 1/2] usb: dwc3: disable for USB_ROLE_NONE

[Thinh Nguyen]

On Wed, Aug 06, 2025, John Keeping wrote:
> On Tue, Jul 29, 2025 at 10:10:35PM +0000, Thinh Nguyen wrote:
> > On Mon, Jul 14, 2025, John Keeping wrote:
> > > On Sat, Jul 12, 2025 at 12:11:38AM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jul 10, 2025, John Keeping wrote:
> > > > > When the phy is acting as a Type C mux, it may need to reset when the
> > > > > cable direction changes.  But this should not happen while DWC3 is
> > > > > trying to use the USB connection.
> > > > > 
> > > > > In this case, there must be a connection manager to notify the phy of
> > > > > the orientation change and tcpm_mux_set() ensures this happens before
> > > > > DWC3's role switch is informed of a change.
> > > > > 
> > > > > It should not be possible to go directly from device->device or
> > > > > host->host with a change in orientation without transitioning through
> > > > > the "none" role as the cable is unplugged.  So ensuring that DWC3 always
> > > > 
> > > > The controller is either host or device mode. It's odd to use "none" to
> > > > indicate connection.
> > > 
> > > There is no connection in this state.  When the type C controller
> > > indicates that the role is "none" then there cannot be a USB connection.
> > > 
> > 
> > It's not about connection. It's about the current mode of the USB
> > controller. The mode of the controller isn't "none" on disconnection
> > even if you try to role-switch to that via the typec switch.
> > 
> > > > > informs the phy of the new mode whenever a plug is detected should be
> > > > > sufficient for the phy to safely reset itself at a time that is safe for
> > > > > DWC3.
> > > > 
> > > > Couldn't the phy do this as it detects connection/disconnection.
> > > 
> > > I don't think the phy can detect a connection.  If it is configured for
> > > the wrong orientation then it will not monitor the correct data lines.
> > > The phy hardware does not signal any interrupts to the CPU for the
> > > driver to react to.
> > 
> > Ok.
> > 
> > > 
> > > > It seems what you need is a full controller initialization upon new
> > > > connection on orientation change, and you're using role_switch selecting
> > > > "none" to do so.
> > > 
> > > I'm not sure whether a complete initialization is necessary, but what I
> > > want to avoid is the phy resetting while the controller is part-way
> > > through device enumeration or setting up a gadget configuration.
> > > 
> > > It may be that simply avoiding resetting the phy if the orientation is
> > > unchanged is enough to avoid some of this problem, but I suspect there
> > > are still problems if the clocks from the phy to the controller are
> > > stopped unexpectedly.  However, I have run some tests of this change and
> > > it looks promising.
> > 
> > That sounds like the the phy reset is done after asserting the vbus
> > valid line indicating connection to the controller. Can this be fixed in
> > the phy or type-c driver?
> 
> I don't think so.  I've tried implementing this in the phy driver and it
> does not work reliably.
> 
> > > > I'm not sure if role-switch has the right interface to do so. Perhaps we
> > > > can introduce one? I don't think we should change the behavior of the
> > > > current flow and apply that to all other platforms.
> > > 
> > > I don't think it's unreasonable for the controller to be idle when there
> > > is an external type C controller notifying the connection state.
> > > 
> > > The hardware setup looks like this, with the Linux type C code notifying
> > > the phy driver of the orientation change and the DWC3 driver of the role
> > > change:
> > > 
> > >                       +------+          
> > >                       | DWC3 |<----+    
> > >                       +--^---+     |    
> > >                          |         |    
> > >                       +--v--+      |    
> > >                 +---->| PHY |      |Role
> > >                 |     +--^--+      |    
> > >     Orientation |        |         |    
> > >                 |   +----v----+    |    
> > >                 +---+ FUSB302 +----+    
> > >                     +---------+
> > > 
> > > The advantage of using the role hook is that we are guaranteed that it
> > > is called after the phy has been notified of the orientation change.
> > > 
> > > Do you have an idea of a new interface?  Or do you think it is safe to
> > > reset the phy underneath the controller when that will halt the clocks
> > > from the phy to the controller for some period of time?
> > 
> > The proper fix should be in the phy where it should only assert vbus
> > valid after processing orientation and phy reset. I'm not familiar on
> > the communication between your PHY and FUSB302. There should be some
> > indication from the phy that it's ready before you can assert vbus-valid
> > (that can be from a callback, a status register etc).
> 
> The FUSB302 is connected to the CPU via I2C, there is no direct
> communication between it and the phy, it all goes via the kernel's tcpm
> subsystem.
> 
> The problem with re-initializing the phy is that this causes dwc3 gadget
> configuration to fail with "failed to enable ep0out", and this happens
> when VBUS valid is de-asserted.  This is 100% reproducible on boot for
> me where the init scripts are setting up a gadget via configfs.
> 
> So it seems the timing of the phy setup needs to coordinate with the
> dwc3 controller.

Ok.

> 
> > What you've done is a teardown and reinit of the driver, triggering a
> > soft-reset. This can re-sync the phy. That probably takes long enough
> > that a phy-reset after orientation change will be completed before the
> > initialization complete.
> > 
> > If there's no way for you to know when the phy complete its reset, we
> > can add a quirk for your platform to always reinit on role-switch call.
> > 
> > Let me know if this is reasonable.
> 
> I'm happy to test patches.
> 

Sure. I'll need to get back on this.

BR,
Thinh

[RFC/PATCH 2/2] phy: rockchip: usbdp: implement .set_mode

[John Keeping]

When the orientation of a type C cable changes, usbdp set the new
configuration in its internal state but does not write this to the
hardware.

Make use of phy_ops::set_mode to write this new state.  This should be
called by the USB controller when it is notified of a role change
(assuming it is acting as the role switch) and will be called at a point
when the controller does not expect the phy to be operating normally.

Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index c066cc0a7b4f1..00368fb09307a 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -1335,9 +1335,23 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
 	return 0;
 }
 
+static int rk_udphy_usb3_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct rk_udphy *udphy = phy_get_drvdata(phy);
+	int ret = 0;
+
+	mutex_lock(&udphy->mutex);
+	if (udphy->mode != UDPHY_MODE_NONE)
+		ret = rk_udphy_init(udphy);
+	mutex_unlock(&udphy->mutex);
+
+	return ret;
+}
+
 static const struct phy_ops rk_udphy_usb3_phy_ops = {
 	.init		= rk_udphy_usb3_phy_init,
 	.exit		= rk_udphy_usb3_phy_exit,
+	.set_mode	= rk_udphy_usb3_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.50.0

Re: [RFC/PATCH 2/2] phy: rockchip: usbdp: implement .set_mode

[Heiko Stuebner]

Hi John,

Am Donnerstag, 10. Juli 2025, 17:22:50 Mitteleuropäische Sommerzeit schrieb John Keeping:
> When the orientation of a type C cable changes, usbdp set the new
> configuration in its internal state but does not write this to the
> hardware.
> 
> Make use of phy_ops::set_mode to write this new state.  This should be
> called by the USB controller when it is notified of a role change
> (assuming it is acting as the role switch) and will be called at a point
> when the controller does not expect the phy to be operating normally.
> 
> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>

with the comments from Ondrej in [0] the whole thing seems to be
slightly more complex


[0] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250225184519.3586926-3-heiko@sntech.de/

> ---
>  drivers/phy/rockchip/phy-rockchip-usbdp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c066cc0a7b4f1..00368fb09307a 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1335,9 +1335,23 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> +static int rk_udphy_usb3_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	struct rk_udphy *udphy = phy_get_drvdata(phy);
> +	int ret = 0;
> +
> +	mutex_lock(&udphy->mutex);
> +	if (udphy->mode != UDPHY_MODE_NONE)
> +		ret = rk_udphy_init(udphy);
> +	mutex_unlock(&udphy->mutex);
> +
> +	return ret;
> +}
> +
>  static const struct phy_ops rk_udphy_usb3_phy_ops = {
>  	.init		= rk_udphy_usb3_phy_init,
>  	.exit		= rk_udphy_usb3_phy_exit,
> +	.set_mode	= rk_udphy_usb3_phy_set_mode,
>  	.owner		= THIS_MODULE,
>  };
>  
> 

Re: [RFC/PATCH 2/2] phy: rockchip: usbdp: implement .set_mode

[John Keeping]

On Tue, Jul 15, 2025 at 12:35:47PM +0200, Heiko Stuebner wrote:
> Am Donnerstag, 10. Juli 2025, 17:22:50 Mitteleuropäische Sommerzeit schrieb John Keeping:
> > When the orientation of a type C cable changes, usbdp set the new
> > configuration in its internal state but does not write this to the
> > hardware.
> > 
> > Make use of phy_ops::set_mode to write this new state.  This should be
> > called by the USB controller when it is notified of a role change
> > (assuming it is acting as the role switch) and will be called at a point
> > when the controller does not expect the phy to be operating normally.
> > 
> > Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> 
> with the comments from Ondrej in [0] the whole thing seems to be
> slightly more complex
> 
> 
> [0] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250225184519.3586926-3-heiko@sntech.de/

I spent some more time looking at this and have found what looks like
the vendor kernel's approach to this issue [1].  It seems that Rockchip
set the autosuspend time to 100ms and hope that is sufficiently short to
allow the controller to suspend whenever a cable is unplugged.

[1] https://github.com/rockchip-linux/kernel/commit/5ac62b80f7471ee9858ab0459af07180de938b51

Having experimented some more, I was able to do something similar
without needing any changes to the phy driver by applying the changes
below on top of patch 1/2 in this thread.  This depends on runtime PM to
ensure the controller is suspended whenever a cable is unplugged, but if
CONFIG_PM is enabled then it should address those concerns because the
controller will be shutdown and the phy configured via phy_init() in the
normal sequence.

It sounds like Thinh doesn't like this approach, but if the requirement
is for the controller to be shutdown and restarted whenever the
orientation changes I don't think we can avoid some changes in dwc3.

--- >8 ---
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 6573cca0eeaf5..b707afd38cc27 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -199,6 +199,10 @@ static void __dwc3_set_mode(struct work_struct *work)
 		dwc3_otg_update(dwc, 1);
 		break;
 	default:
+		if (dwc->force_suspended) {
+			pm_runtime_force_resume(dwc->dev);
+			dwc->force_suspended = 0;
+		}
 		break;
 	}
 
@@ -272,6 +276,9 @@ static void __dwc3_set_mode(struct work_struct *work)
 		dwc3_otg_update(dwc, 0);
 		break;
 	default:
+		ret = pm_runtime_force_suspend(dwc->dev);
+		if (!ret)
+			dwc->force_suspended = 1;
 		break;
 	}
 
@@ -2485,7 +2492,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	default:
-		/* do nothing */
+		dwc3_core_exit(dwc);
 		break;
 	}
 
@@ -2552,7 +2559,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 
 		break;
 	default:
-		/* do nothing */
+		ret = dwc3_core_init_for_resume(dwc);
+		if (ret)
+			return ret;
 		break;
 	}
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index d5b985fa12f4d..7e1d480c59ae1 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1390,6 +1390,7 @@ struct dwc3 {
 	unsigned		sys_wakeup:1;
 	unsigned		wakeup_configured:1;
 	unsigned		suspended:1;
+	unsigned		force_suspended:1;
 	unsigned		susphy_state:1;
 
 	u16			imod_interval;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 8f427afa8eb93..16aac8384ca3d 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -461,6 +461,7 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
 		break;
 	}
 
+	flush_work(&dwc->drd_work);
 	dwc3_set_mode(dwc, mode);
 	return 0;
 }