Re: [PATCH v17 6/7] phy: rockchip: usbdp: Add USB-C state without DP enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] phy: rockchip: usbdp: default mapping in
  rk_udphy_set_typec_default_mapping() still uses TYPEC_DP_STATE_D
--

commit d9ec7f64d43ec45817d1ec726a45689f86fe82b5
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Add USB-C state without DP enabled

This commit splits out lane mux setup and adds a separate USB-only state
(UDPHY_MODE_USB with 0 DP lanes) for TYPEC_STATE_SAFE and TYPEC_STATE_USB
in rk_udphy_set_typec_state().

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index b79b6db96a0e6..553b88c45108a 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -583,32 +583,14 @@ static void rk_udphy_dp_lane_enable(struct rk_udphy *udphy, int dp_lanes)
>  				   CMN_DP_CMN_RSTN, FIELD_PREP(CMN_DP_CMN_RSTN, 0x0));
>  }
>  
> -static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 hw_mode)
> +static void rk_udphy_set_lane_mux(struct rk_udphy *udphy)
>  {
> -	if (udphy->hw_mode == hw_mode)
> -		return;
> -
> -	udphy->phy_needs_reinit = true;
> -	udphy->hw_mode = hw_mode;
> -}
> -
> -static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state)
> -{
> -	u8 hw_mode;
> -
> -	switch (state) {
> -	case TYPEC_DP_STATE_C:
> -	case TYPEC_DP_STATE_E:
> +	if (udphy->dp_lanes == 4) {
>  		udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
>  		udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
>  		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
>  		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
> -		hw_mode = UDPHY_MODE_DP;
> -		udphy->dp_lanes = 4;
> -		break;
> -
> -	case TYPEC_DP_STATE_D:
> -	default:
> +	} else {
>  		if (udphy->flip) {
>  			udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
>  			udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
> @@ -620,12 +602,39 @@ static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state
>  			udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
>  			udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
>  		}
> -		hw_mode = UDPHY_MODE_DP_USB;
> -		udphy->dp_lanes = 2;
> +	}
> +}
> +
> +static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 hw_mode, u8 dp_lanes)
> +{
> +	if (udphy->hw_mode == hw_mode && udphy->dp_lanes == dp_lanes)
> +		return;
> +
> +	udphy->phy_needs_reinit = true;
> +	udphy->hw_mode = hw_mode;
> +	udphy->dp_lanes = dp_lanes;
> +}
> +
> +static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state)
> +{
> +	switch (state) {
> +	case TYPEC_DP_STATE_C:
> +	case TYPEC_DP_STATE_E:
> +		rk_udphy_mode_set(udphy, UDPHY_MODE_DP, 4);
> +		break;
> +
> +	case TYPEC_DP_STATE_D:
> +		rk_udphy_mode_set(udphy, UDPHY_MODE_DP_USB, 2);
> +		break;
> +
> +	case TYPEC_STATE_SAFE:
> +	case TYPEC_STATE_USB:
> +	default:
> +		rk_udphy_mode_set(udphy, UDPHY_MODE_USB, 0);
>  		break;
>  	}
>  
> -	rk_udphy_mode_set(udphy, hw_mode);
> +	rk_udphy_set_lane_mux(udphy);
>  }
>  
>  static void rk_udphy_set_typec_default_mapping(struct rk_udphy *udphy)

[Severity: Medium]
Should rk_udphy_set_typec_default_mapping() also pass TYPEC_STATE_USB (or
TYPEC_STATE_SAFE) instead of TYPEC_DP_STATE_D?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_set_typec_default_mapping() {
	...
	/* default to USB3 + DP as 4 lane USB is not supported */
	rk_udphy_set_typec_state(udphy, TYPEC_DP_STATE_D);
}

Because rk_udphy_orien_sw_set() calls rk_udphy_set_typec_default_mapping()
on every attached orientation update, leaving TYPEC_DP_STATE_D there flips
udphy->hw_mode to UDPHY_MODE_DP_USB (dp_lanes = 2) and sets
udphy->phy_needs_reinit = true whenever the PHY was in UDPHY_MODE_USB.

When a Type-C port driver sets orientation before mode (such as
tcpm_mux_set()), this bounces hw_mode from UDPHY_MODE_USB to
UDPHY_MODE_DP_USB and back to UDPHY_MODE_USB, forcing an unnecessary PHY
and DWC3 reset even when neither orientation nor USB-only mode changed.
When a port driver sets mode before orientation (such as
ucsi_handle_connector_change()) or only registers an orientation switch,
rk_udphy_orien_sw_set() overwrites UDPHY_MODE_USB back to
UDPHY_MODE_DP_USB and leaves the DP side enabled for USB-only connections.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-rockchip-usbdp-cleanup-part3-v17-0-1af5f566408e@collabora.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy