Re: [PATCH v11 07/36] phy: rockchip: usbdp: Keep clocks running on PHY re-init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The USB3 (U3) port is not re-enabled when the PHY dynamically switches from DP-only mode to DP+USB mode, breaking USB3 functionality.
- [High] PHY clock and power leak due to early exit during power off based on current mode instead of active status.
- [High] The DisplayPort PHY init function fails to replay a cached HPD (Hot Plug Detect) event if the Type-C cable was connected before the DP controller probed.
--

--- Patch [7]: [PATCH v11 07/36] phy: rockchip: usbdp: Keep clocks running on PHY re-init ---
commit 79bb98238750965863b0b02f66603b673864fa91
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Keep clocks running on PHY re-init

This commit optimizes PHY re-initialization by avoiding the redundant disable
and re-enable of clocks. Instead of calling rk_udphy_disable() and
rk_udphy_setup(), it directly calls rk_udphy_init().

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 6cb9f6b4dbf6d..e3f5a26c876a5 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -987,8 +987,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>  
> -		rk_udphy_disable(udphy);
> -		ret = rk_udphy_setup(udphy);
> +		ret = rk_udphy_init(udphy);

[Severity: High]
This isn't a bug introduced by this patch, but does this logic leave the USB3
port disabled when dynamically switching from DP-only back to DP+USB mode?

When the Type-C mux transitions from DP+USB to DP-only mode,
rk_udphy_power_on() executes rk_udphy_u3_port_disable(udphy, true) because
udphy->mode == UDPHY_MODE_DP.

If the mux later switches back to DP+USB mode (where
udphy->mode == UDPHY_MODE_DP_USB), rk_udphy_power_on() is called again with
udphy->mode_change == true, but there is no corresponding
rk_udphy_u3_port_disable(udphy, false) call here to re-enable the U3 port.

Consequently, the USB 3.0 port appears to remain disabled in the GRF registers
until a full PHY power cycle.

>  		if (ret)
>  			return ret;
>  		udphy->mode_change = false;

There are also two other pre-existing issues in this file that might need
attention.

[Severity: High]
This is a pre-existing issue, but does rk_udphy_usb3_phy_exit() permanently
leak PHY clock and power resources if the Type-C mode changes before the USB
controller exits?

If the mode changes (e.g., from DP+USB to DP-only) before the USB controller
calls phy_exit, the mode will be updated. When rk_udphy_usb3_phy_exit() is
subsequently called:

static int rk_udphy_usb3_phy_exit(struct phy *phy)
{
	...
	/* DP only or high-speed */
	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		goto unlock;

	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
	...
}

Since udphy->mode was already updated, this condition evaluates to true,
causing an early return. The UDPHY_MODE_USB bit is never cleared from
udphy->status, permanently skipping rk_udphy_disable() and leaving clocks
running even when all devices detach.

The same logic flaw exists in rk_udphy_power_off():

static void rk_udphy_power_off(struct rk_udphy *udphy, u8 mode)
{
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return;
	}
	...
}

Checking udphy->mode instead of the active udphy->status prevents the status
bit from being cleared.


[Severity: High]
This is a pre-existing issue, but does rk_udphy_dp_phy_init() silently miss
the initial connection event if the Type-C cable is connected before the
DisplayPort controller driver probes?

When a connection occurs, rk_udphy_dp_hpd_event_trigger() is called:

static void rk_udphy_dp_hpd_event_trigger(struct rk_udphy *udphy, bool hpd)
{
	...
	udphy->dp_sink_hpd_sel = true;
	udphy->dp_sink_hpd_cfg = hpd;

	if (!udphy->dp_in_use)
		return;

	rk_udphy_grfreg_write(udphy->vogrf, &cfg->vogrfcfg[udphy->id].hpd_trigger, hpd);
}

If the DP controller hasn't probed yet, the event is merely cached and not
written to the GRF register.

When the DP controller later probes and calls rk_udphy_dp_phy_init():

static int rk_udphy_dp_phy_init(struct phy *phy)
{
	...
	udphy->dp_in_use = true;
	...
	return 0;
}

The init function sets dp_in_use to true but never checks or applies the
cached HPD state (dp_sink_hpd_sel and dp_sink_hpd_cfg), leaving the display
blank until a physical replug.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-rockchip-usbdp-cleanup-v11-0-a149ac60f76c@collabora.com?part=7