Message-ID: <7q5yn466xd7emebhjze4ixkswgyxjjjt5rwvyww2hwbts6bamd@i5vwvegy2os6>
Date: Wed, 26 Feb 2025 15:46:11 +0100
From: Ondřej Jirman <megi@....cz>
To: Heiko Stuebner <heiko@...ech.de>
Cc: vkoul@...nel.org, kishon@...nel.org, linux-phy@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	quentin.schulz@...rry.de, sebastian.reichel@...labora.com, 
	Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH 2/2] phy: rockchip: usbdp: re-init the phy on
 orientation-change

Hi Heiko,

On Tue, Feb 25, 2025 at 07:45:19PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@...rry.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@...rry.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists