diff mbox series

[2/2] phy: rockchip: usbdp: re-init the phy on orientation-change

Message ID 20250225184519.3586926-3-heiko@sntech.de (mailing list archive)
State New
Headers show
Series phy: rockchip: usbdp: improve typec orientation handling | expand

Commit Message

Heiko Stuebner Feb. 25, 2025, 6:45 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@cherry.de>

Until now the usbdp in the orientation-handler set the new lane setup in
its internal state variables and adapted the sbu gpios as needed.
It never actually updated the phy itself though, but relied on the
controlling usb-controller to disable and re-enable the phy.

And while on the vendor-kernel, I could see that on every unplug the dwc3
did go to its suspend and woke up on the next device plug-in event,
thus toggling the phy as needed, this does not happen in all cases and we
should not rely on that behaviour.

This results in the usb2 always working, as it's not affected by the
orientation, but usb3 only working in one direction right now.

So similar to how the update works in the power-on callback, just re-init
the phy if it's already running when the orientation-event happens.

Both the power-on/-off functions as well as the orientation-set callback
work with the usbdp-mutex held, so can't conflict.

The behaviour is similar to how the qcom qmp phys handle the orientaton
re-init - by re-initting the phy.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/phy/rockchip/phy-rockchip-usbdp.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Heiko Stuebner Feb. 25, 2025, 10:07 p.m. UTC | #1 
Am Dienstag, 25. Februar 2025, 19:45:19 MEZ schrieb Heiko Stuebner:
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 7b17c82ebcfc..b63259a90d85 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1277,6 +1277,7 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>  				 enum typec_orientation orien)
>  {
>  	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> +	int ret = 0;
>  
>  	mutex_lock(&udphy->mutex);
>  
> @@ -1292,6 +1293,12 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>  	rk_udphy_set_typec_default_mapping(udphy);
>  	rk_udphy_usb_bvalid_enable(udphy, true);
>  
> +	/* re-init the phy if already on */
> +	if (udphy->status != UDPHY_MODE_NONE) {
> +		rk_udphy_disable(udphy);
> +		ret = rk_udphy_setup(udphy);
> +	}
> +

just realized that 

	if (udphy->status != UDPHY_MODE_NONE)
		ret = rk_udphy_init(udphy);

does the same, and we really don't need to disable and re-enable the
phy's clocks that are the added via rk_udphy_disable and _setup.

So will give it a day and send a v2 then.


Heiko
Ondřej Jirman Feb. 26, 2025, 2:46 p.m. UTC | #2 
Hi Heiko,

On Tue, Feb 25, 2025 at 07:45:19PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> Until now the usbdp in the orientation-handler set the new lane setup in
> its internal state variables and adapted the sbu gpios as needed.
> It never actually updated the phy itself though, but relied on the
> controlling usb-controller to disable and re-enable the phy.
> 
> And while on the vendor-kernel, I could see that on every unplug the dwc3
> did go to its suspend and woke up on the next device plug-in event,
> thus toggling the phy as needed, this does not happen in all cases and we
> should not rely on that behaviour.

On RK3399 there's a similar issue with the equivalent type-c PHY driver.
The TRM (part 2) states that:

4.6.1 Some Special Settings before Initialization

- Set USB3.0 OTG controller AXI master setting.
- Clear USB2.0 only mode setting (bit 3 of register GRF_USB3PHY0/1_CON0 in Chapter GRF)
- USB3.0 OTG controller should be hold in reset during the initialization of the corresponding
  TypeC PHY until TypeC PHY is ready for USB operation.
- Set PHYIF to 1 to use 16-bit UTMI+ interface (see register GUSB2PHYCFG0)
- Clear ENBLSLPM to 0 to disable sleep and l1 suspend (see register GUSB2PHYCFG0)
  ...

The PHY for Superspeed signals is expected to be set up while the USB
controller is held in reset, which makes sense HW wise, and it's what downstream
kernel efectivelly does, via its RPM based hack.

RK3588 TRM doesn't have very detailed notes on this, but I expect it will be
similar.

So reconfiguring the phy here, while it's actively linked to the USB controller
without the controller driver driving the process so it reliably happens while
it's in reset, or at least so that USB controller reset happens afterwards, may
not be correct way to approach this.

Also moving this to the USB controller driver would fix the issue on both RK3399
and RK3588 and maybe elsewhere.

My own shot at this is:

https://codeberg.org/megi/linux/commit/2fee801eae4ca6c0e90e52f6a01caa3e6db28d7d

I turned out to be a bit too complicated, so I didn't submit that yet upstream,
hoping I could simplify it in the future.

I basically abused current_dr_role a bit to add a disconnected state, to get
__dwc3_set_mode() called when type-c controller updates the port state from
disconnected to connected with some orientation, and trigger phy reconfig from
there, while USB is in reset:

https://elixir.bootlin.com/linux/v6.13.4/source/drivers/usb/dwc3/core.c#L197

Kind regards,
	o.

> This results in the usb2 always working, as it's not affected by the
> orientation, but usb3 only working in one direction right now.
> 
> So similar to how the update works in the power-on callback, just re-init
> the phy if it's already running when the orientation-event happens.
> 
> Both the power-on/-off functions as well as the orientation-set callback
> work with the usbdp-mutex held, so can't conflict.
> 
> The behaviour is similar to how the qcom qmp phys handle the orientaton
> re-init - by re-initting the phy.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-usbdp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 7b17c82ebcfc..b63259a90d85 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1277,6 +1277,7 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>  				 enum typec_orientation orien)
>  {
>  	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> +	int ret = 0;
>  
>  	mutex_lock(&udphy->mutex);
>  
> @@ -1292,6 +1293,12 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>  	rk_udphy_set_typec_default_mapping(udphy);
>  	rk_udphy_usb_bvalid_enable(udphy, true);
>  
> +	/* re-init the phy if already on */
> +	if (udphy->status != UDPHY_MODE_NONE) {
> +		rk_udphy_disable(udphy);
> +		ret = rk_udphy_setup(udphy);
> +	}
> +
>  unlock_ret:
>  	mutex_unlock(&udphy->mutex);
>  	return ret;
> -- 
> 2.47.2
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Heiko Stuebner Feb. 26, 2025, 3:53 p.m. UTC | #3 
Hey Ondřej,

Am Mittwoch, 26. Februar 2025, 15:46:11 MEZ schrieb Ondřej Jirman:
> On Tue, Feb 25, 2025 at 07:45:19PM +0100, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > Until now the usbdp in the orientation-handler set the new lane setup in
> > its internal state variables and adapted the sbu gpios as needed.
> > It never actually updated the phy itself though, but relied on the
> > controlling usb-controller to disable and re-enable the phy.
> > 
> > And while on the vendor-kernel, I could see that on every unplug the dwc3
> > did go to its suspend and woke up on the next device plug-in event,
> > thus toggling the phy as needed, this does not happen in all cases and we
> > should not rely on that behaviour.
> 
> On RK3399 there's a similar issue with the equivalent type-c PHY driver.
> The TRM (part 2) states that:
> 
> 4.6.1 Some Special Settings before Initialization
> 
> - Set USB3.0 OTG controller AXI master setting.
> - Clear USB2.0 only mode setting (bit 3 of register GRF_USB3PHY0/1_CON0 in Chapter GRF)
> - USB3.0 OTG controller should be hold in reset during the initialization of the corresponding
>   TypeC PHY until TypeC PHY is ready for USB operation.
> - Set PHYIF to 1 to use 16-bit UTMI+ interface (see register GUSB2PHYCFG0)
> - Clear ENBLSLPM to 0 to disable sleep and l1 suspend (see register GUSB2PHYCFG0)
>   ...
> 
> The PHY for Superspeed signals is expected to be set up while the USB
> controller is held in reset, which makes sense HW wise, and it's what downstream
> kernel efectivelly does, via its RPM based hack.
> 
> RK3588 TRM doesn't have very detailed notes on this, but I expect it will be
> similar.
> 
> So reconfiguring the phy here, while it's actively linked to the USB controller
> without the controller driver driving the process so it reliably happens while
> it's in reset, or at least so that USB controller reset happens afterwards, may
> not be correct way to approach this.

the function here, is using infrastructure from the type-c framework.

The function in question tcpm_mux_set(), which then ends up in the
usbdp_phy only gets called from tcpm_reset_port() .

Which I think will do the right already - judging by its name ;-) .
[and also by the fact that the referenced qcom phy behaves similarly
 when talking to the type-c framework]


For the rk3399 I would think converting the old typec-phy-driver over to
use the actual kernel type-c framework, might just magically solve the
issue you have on it.

Rockchip actually has converted the rk3399 typec-phy to use the type-c
framework in their vendor kernel.


Heiko
Ondřej Jirman Feb. 26, 2025, 5:03 p.m. UTC | #4 
Hi Heiko,

On Wed, Feb 26, 2025 at 04:53:45PM +0100, Heiko Stübner wrote:
> Hey Ondřej,
> 
> Am Mittwoch, 26. Februar 2025, 15:46:11 MEZ schrieb Ondřej Jirman:
> > On Tue, Feb 25, 2025 at 07:45:19PM +0100, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > > 
> > > Until now the usbdp in the orientation-handler set the new lane setup in
> > > its internal state variables and adapted the sbu gpios as needed.
> > > It never actually updated the phy itself though, but relied on the
> > > controlling usb-controller to disable and re-enable the phy.
> > > 
> > > And while on the vendor-kernel, I could see that on every unplug the dwc3
> > > did go to its suspend and woke up on the next device plug-in event,
> > > thus toggling the phy as needed, this does not happen in all cases and we
> > > should not rely on that behaviour.
> > 
> > On RK3399 there's a similar issue with the equivalent type-c PHY driver.
> > The TRM (part 2) states that:
> > 
> > 4.6.1 Some Special Settings before Initialization
> > 
> > - Set USB3.0 OTG controller AXI master setting.
> > - Clear USB2.0 only mode setting (bit 3 of register GRF_USB3PHY0/1_CON0 in Chapter GRF)
> > - USB3.0 OTG controller should be hold in reset during the initialization of the corresponding
> >   TypeC PHY until TypeC PHY is ready for USB operation.
> > - Set PHYIF to 1 to use 16-bit UTMI+ interface (see register GUSB2PHYCFG0)
> > - Clear ENBLSLPM to 0 to disable sleep and l1 suspend (see register GUSB2PHYCFG0)
> >   ...
> > 
> > The PHY for Superspeed signals is expected to be set up while the USB
> > controller is held in reset, which makes sense HW wise, and it's what downstream
> > kernel efectivelly does, via its RPM based hack.
> > 
> > RK3588 TRM doesn't have very detailed notes on this, but I expect it will be
> > similar.
> > 
> > So reconfiguring the phy here, while it's actively linked to the USB controller
> > without the controller driver driving the process so it reliably happens while
> > it's in reset, or at least so that USB controller reset happens afterwards, may
> > not be correct way to approach this.
> 
> the function here, is using infrastructure from the type-c framework.
> 
> The function in question tcpm_mux_set(), which then ends up in the
> usbdp_phy only gets called from tcpm_reset_port() .
>
> Which I think will do the right already - judging by its name ;-) .

No. This function has nothing to do with USB controller reset. And while  dwc3
driver consumes usb_role_switch interface, calling set_role on that interface
just translates to calling dwc3_set_mode(), which will do nothing if the mode
did not change from what it was previously. (and it stays the same if you just
re-plug the same device in same mode but just in different cable orientation)

Only time DWC3 reset seems to happen currently is sometimes in dwc3_set_mode()
depending on driver state, and then in dwc3_init()/exit() which is called only
at driver probe/remove time and in suspend/resume.

So if PHY driver reconfigures/"power-cycles" the PHY on its own, DWC3 driver
will not issue any reset to the controller HW, at least not based on the
set_role signal from usb_role_switch alone most of the time.

Also tcpm_orientation interface you use for PHY reset is called from many places
in TCPM driver, based on many events, mainly from tcpm_set_roles(), so you'll
be resetting the PHY quite a bit in uncoordinated manner with the dwc3 MAC
interface it's connected to in HW.

> [and also by the fact that the referenced qcom phy behaves similarly
>  when talking to the type-c framework]
> 
> 
> For the rk3399 I would think converting the old typec-phy-driver over to
> use the actual kernel type-c framework, might just magically solve the
> issue you have on it.

It will not. The problem is not getting the information about roles between the
drivers. extcon works fine for that. Problem is that your proposed solution
doesn't coordinate the PHY reset with USB controller to ensure proper ordering,
or even ensuring that any USB controller reset happens, or the controller knows
about the PHY reset.

Resulting issues may happen only occasionally, dunno. I guess it depends on
whether SS MAC will happen to be accessing USBDP PHY at the time when you're
turning off the PHY, and if this messes up the internal state of the MAC pipe
interface, so after PHY comes back up from reset state, whether that locks up
the MAC interface logic or not will depend on luck. :)

You can ask Rockchip what they think about uncoordinated PHY reset, while DWC3
is up and ready. Maybe it's ok on RK3588, but they're not doing it in their
vendor driver this way, and they forbid it on RK3399, so perhaps it's not? From
HW point of view it looks sketchy.

Kind regards,
	o.

> Rockchip actually has converted the rk3399 typec-phy to use the type-c
> framework in their vendor kernel.
> 
> 
> Heiko
> 
>
diff mbox series

Patch

diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index 7b17c82ebcfc..b63259a90d85 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -1277,6 +1277,7 @@  static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 				 enum typec_orientation orien)
 {
 	struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
+	int ret = 0;
 
 	mutex_lock(&udphy->mutex);
 
@@ -1292,6 +1293,12 @@  static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
 	rk_udphy_set_typec_default_mapping(udphy);
 	rk_udphy_usb_bvalid_enable(udphy, true);
 
+	/* re-init the phy if already on */
+	if (udphy->status != UDPHY_MODE_NONE) {
+		rk_udphy_disable(udphy);
+		ret = rk_udphy_setup(udphy);
+	}
+
 unlock_ret:
 	mutex_unlock(&udphy->mutex);
 	return ret;