Re: [PATCH v12 10/36] phy: rockchip: usbdp: Add missing mode_change update

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] PHY mode updates in `rk_udphy_typec_mux_set` are incorrectly gated by DP Alt Mode and HPD status, breaking USB lane restoration when exiting Alt Mode.
- [High] The USB3 port remains permanently disabled in the GRF after dynamically returning from 4-lane DP mode.
- [High] HPD events received from the Type-C TCPM before the DisplayPort controller initializes the PHY are cached but never flushed, dropping the hot-plug event.
- [High] The driver leaks the `UDPHY_MODE_USB` bit in `udphy->status` when dynamically switching from DP+USB mode to DP-only mode.
--

--- Patch [10]: [PATCH v12 10/36] phy: rockchip: usbdp: Add missing mode_change update ---
commit 1907d9e7af04f2c28df8bf00ded6a9b5a290664a
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Add missing mode_change update

rk_udphy_set_typec_default_mapping() updates the available modes,
but does not set the mode_change as required. This results in
missing re-initialization and thus non-working DisplayPort.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 982315535caf7..7540c897dde13 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1361,10 +1370,7 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  			usleep_range(750, 800);
>  			rk_udphy_dp_hpd_event_trigger(udphy, true);
>  		} else if (data->status & DP_STATUS_HPD_STATE) {
> -			if (udphy->mode != mode) {
> -				udphy->mode = mode;
> -				udphy->mode_change = true;
> -			}
> +			rk_udphy_mode_set(udphy, mode);

[Severity: High]
This is a pre-existing issue, but are PHY mode updates incorrectly gated by
DP Alt Mode and HPD status here?

When the Type-C manager tears down DP Alt Mode, it calls typec_mux_set()
with state->mode = TYPEC_STATE_SAFE and state->alt = NULL. The function
computes mode = UDPHY_MODE_DP_USB, but this rk_udphy_mode_set() call is
nested inside:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
    ...
    if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
    ...
        } else if (data->status & DP_STATUS_HPD_STATE) {
            rk_udphy_mode_set(udphy, mode);
    ...
}

Does this incorrectly restrict PHY mode updates to DP Alt Mode, completely
missing state teardown paths where state->alt is NULL? This appears to break
USB lane restoration when exiting Alt Mode, causing USB to fail after
unplugging a DisplayPort adapter or exiting Alt Mode without a replug.

>  			rk_udphy_dp_hpd_event_trigger(udphy, true);
>  		} else {
>  			rk_udphy_dp_hpd_event_trigger(udphy, false);

[Severity: High]
This isn't a bug introduced by this patch, but does the USB3 port remain
permanently disabled in the GRF after dynamically returning from 4-lane DP
mode?

When dynamically switching from 4-lane DP mode back to DP+USB mode,
rk_udphy_power_on() executes the following block to reinitialize the PHY:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
    ...
    } else if (udphy->mode_change) {
        if (udphy->mode == UDPHY_MODE_DP)
            rk_udphy_u3_port_disable(udphy, true);

        ret = rk_udphy_init(udphy);
    ...
}

While this block correctly disables the U3 port for DP-only mode via
rk_udphy_u3_port_disable(udphy, true), it lacks the inverse
rk_udphy_u3_port_disable(udphy, false) call for restoring DP+USB mode.

Could this prevent the DWC3 controller from communicating with the USB3 PHY
when returning to DP+USB mode, resulting in non-working USB3?

[Severity: High]
This is a pre-existing issue, but are HPD events dropped if they are received
from the Type-C TCPM before the DisplayPort controller initializes the PHY?

If the TCPM fires an HPD event before the DP consumer probes and calls
rk_udphy_dp_phy_init(), rk_udphy_dp_hpd_event_trigger() caches the state in
udphy->dp_sink_hpd_cfg because udphy->dp_in_use is false.

When the DP consumer finally initializes the PHY, dp_in_use is set to true:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_init() {
    ...
    mutex_lock(&udphy->mutex);

    udphy->dp_in_use = true;

    mutex_unlock(&udphy->mutex);
    ...
}

However, the cached dp_sink_hpd_cfg is never written to the hardware
registers via rk_udphy_grfreg_write(). Can this drop the hot-plug event
and cause the display to remain completely undetected until the cable is
replugged?

[Severity: High]
This is a pre-existing issue, but does the driver leak the UDPHY_MODE_USB bit
in udphy->status when dynamically switching from DP+USB mode to DP-only mode?

When the type-c mux switches mode from UDPHY_MODE_DP_USB to UDPHY_MODE_DP,
udphy->mode is updated immediately. Later, when the USB subsystem processes
the disconnect and calls rk_udphy_usb3_phy_exit(), it checks the new mux mode
rather than the currently active status:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_usb3_phy_exit() {
    ...
    /* DP only or high-speed */
    if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
        goto unlock;

    rk_udphy_power_off(udphy, UDPHY_MODE_USB);
    ...
}

This condition evaluates to true because the mode was already changed,
skipping rk_udphy_power_off(udphy, UDPHY_MODE_USB).

Furthermore, the overarching rk_udphy_power_off() shares this identical
flawed check:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_off() {
    ...
    if (!(udphy->mode & mode)) {
        dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
        return;
    }
    ...
}

Could this secondary check return early if the new mode lacks the bit the
caller is legitimately trying to power off, preventing the mode bit from ever
being cleared from udphy->status? This appears to leave PHY resources
(clocks, resets) unreleased on teardown, corrupting state tracking and
leaking power.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=10