Re: [PATCH 2/3] phy: cix: add Sky1 USB/DP combo PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] phy: cix: udphy_dp_phy_set_rate() checks DPCD codes instead of Mb/s link_rate
- [High] phy: cix: udphy_dp_phy_set_voltages() applies lane 0 settings to all lanes
- [High] phy: cix: mode tracking in cix_usbdp_phy_power_on() leaks clk and skips init
- [High] phy: cix: udphy_dp_phy_configure() accesses MMIO when DP PHY is unpowered
- [High] phy: cix: udphy_dp_phy_verify_config() mishandles dp->lanes and dp->set_lanes
- [High] phy: cix: cix_u3phy_ops omits .init/.power_on and misuses .set_mode
- [Medium] phy: cix: cix_udphy_probe() resets PHY despite bootloader handover claim
--

commit d4e0ce4573b0a846fb24af3d6c745c7798d8f7a9
Author: Gary Yang <gary.yang@cixtech.com>

phy: cix: add Sky1 USB/DP combo PHY driver

This commit adds a driver for the CIX Sky1 USB/DP combo PHY, exposing
separate child PHYs for the SuperSpeedPlus USB lane and DisplayPort lane
while registering a Type-C orientation switch and mode mux.

> diff --git a/drivers/phy/cix/phy-cix-usbdp.c b/drivers/phy/cix/phy-cix-usbdp.c
> new file mode 100644
> index 0000000000000..d596e89c74f76
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usbdp.c

[ ... ]

> +static int sky1_udphy_exit(struct cix_udphy *udphy)
> +{
> +	reset_control_assert(udphy->reset);
> +	reset_control_assert(udphy->preset);
> +	clk_disable_unprepare(udphy->apb_clk);
> +	return 0;
> +}
> +
> +static int sky1_udphy_init(struct cix_udphy *udphy)
> +{
> +	int ret;
> +	u32 value;
> +	const struct cix_udphy_con_dir *con_dir = &udphy->cfg->con_dir[udphy->id];
> +
> +	regmap_update_bits(udphy->usbphy_syscon, con_dir->offset,
> +			   GENMASK(con_dir->bit, con_dir->bit),
> +			   udphy->flip << con_dir->bit);
> +
> +	usleep_range(10, 20);
> +
> +	regmap_read(udphy->usbphy_syscon, con_dir->offset, &value);
> +
> +	/* usb rcsu reset is default deassert */
> +	reset_control_assert(udphy->reset);
> +	reset_control_assert(udphy->preset);
> +
> +	ret = sky1_udphy_clk_enable(udphy->apb_clk);
> +	if (ret) {
> +		dev_err(udphy->dev, "Failed to prepare_enable udphy apb clock\n");
> +		return ret;
> +	}

[ ... ]

> +static int udphy_orien_sw_set(struct typec_switch_dev *sw,
> +			      enum typec_orientation orien)
> +{
> +	struct cix_udphy *udphy = typec_switch_get_drvdata(sw);
> +
> +	mutex_lock(&udphy->mutex);
> +
> +	if (orien == TYPEC_ORIENTATION_NONE)
> +		goto unlock_ret;
> +
> +	udphy->flip = (orien == TYPEC_ORIENTATION_REVERSE) ? true : false;
> +
> +unlock_ret:
> +	mutex_unlock(&udphy->mutex);
> +	return 0;
> +}

[ ... ]

> +static int cix_usbdp_phy_power_on(struct cix_udphy *udphy)
> +{
> +	const struct cix_udphy_cfg *phy_cfgs = udphy->cfg;
> +	int ret;
> +
> +	if (udphy->cur_mode == udphy->next_mode)
> +		return 0;
> +
> +	if ((udphy->cur_mode == UDPHY_MODE_USB || udphy->cur_mode == UDPHY_MODE_DP) &&
> +	    udphy->next_mode == UDPHY_MODE_DP_USB)
> +		return 0;
> +
> +	if (phy_cfgs->udphy_init) {
> +		ret = phy_cfgs->udphy_init(udphy);
> +		if (ret) {
> +			dev_err(udphy->dev, "failed to init udphy\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int cix_usbdp_phy_power_off(struct cix_udphy *udphy)
> +{
> +	const struct cix_udphy_cfg *phy_cfgs = udphy->cfg;
> +	int ret;
> +
> +	if (phy_cfgs->udphy_exit) {
> +		ret = phy_cfgs->udphy_exit(udphy);
> +		if (ret) {
> +			dev_err(udphy->dev, "failed to exit udphy\n");
> +			return ret;
> +		}
> +	}
> +
> +	udphy->cur_mode = UDPHY_MODE_NONE;
> +	return 0;
> +}
> +
> +static int udphy_dp_phy_power_on(struct phy *phy)
> +{
> +	struct cix_udphy *udphy = phy_get_drvdata(phy);
> +	int ret = 0;
> +
> +	mutex_lock(&udphy->mutex);
> +	if (!(udphy->next_mode & UDPHY_MODE_DP))
> +		goto unlock;
> +
> +	ret = cix_usbdp_phy_power_on(udphy);
> +	if (ret)
> +		goto unlock;
> +
> +	phy_set_bus_width(phy, udphy_dplane_get(udphy));
> +	if (udphy->next_mode == UDPHY_MODE_DP)
> +		udphy->cur_mode = UDPHY_MODE_DP;
> +	else
> +		udphy->cur_mode |= UDPHY_MODE_DP;
> +
> +unlock:
> +	mutex_unlock(&udphy->mutex);
> +	return ret;
> +}

[Severity: High]
Can the mode tracking in cix_usbdp_phy_power_on() and udphy_dp_phy_power_on()
leak theudphy->apb_clk enable count or skip needed hardware re-initialization?

First, sky1_udphy_init() unconditionally calls
sky1_udphy_clk_enable(udphy->apb_clk), whereas sky1_udphy_exit() only calls
clk_disable_unprepare(udphy->apb_clk) once when udphy->cur_mode reaches
UDPHY_MODE_NONE in cix_usbdp_phy_power_off(). If one child PHY is already
active (udphy->cur_mode != UDPHY_MODE_NONE) and udphy->next_mode changes such
that cix_usbdp_phy_power_on() invokes sky1_udphy_init() a second time,
udphy->apb_clk is prepared and enabled again without a matching disable.

In addition, when udphy->next_mode == UDPHY_MODE_DP, udphy_dp_phy_power_on()
overwrites udphy->cur_mode = UDPHY_MODE_DP instead of setting the bit with
|= UDPHY_MODE_DP. If UDPHY_MODE_USB was already set in udphy->cur_mode, that
bit is lost and a subsequent cix_u3phy_exit() returns early without powering
off the PHY or disabling udphy->apb_clk.

Second, cix_usbdp_phy_power_on() returns 0 without calling sky1_udphy_init()
when udphy->cur_mode == UDPHY_MODE_DP and udphy->next_mode ==
UDPHY_MODE_DP_USB. If the PHY was initialized in 4-lane DP-only mode using
sky1_udphy_dp_conf (where PHY_PMA_LANE_MAP is 0x51D9 and no USB3 lane
registers are programmed) and then transitions to UDPHY_MODE_DP_USB, calling
cix_u3phy_init() skips sky1_udphy_init() and leaves the USB3 lanes
unconfigured.

Similarly, udphy_orien_sw_set() updates udphy->flip without updating
udphy->usbphy_syscon or forcing re-initialization, so if udphy->flip changes
while cix_usbdp_phy_power_on() takes one of the early return paths, the
syscon orientation bit and PMA lane configuration remain in the previous
orientation.

[ ... ]

> +static int udphy_dp_phy_verify_config(struct cix_udphy *udphy,
> +				      struct phy_configure_opts_dp *dp)
> +{
> +	int i;
> +
> +	switch (dp->lanes) {
> +	case 1:
> +	case 2:
> +	case 4:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

[Severity: High]
What happens in udphy_dp_phy_verify_config() when a DisplayPort consumer
calls phy_configure() with dp->lanes == 4 while the PHY is in 2-lane combo
mode (UDPHY_MODE_DP_USB), or calls phy_configure() with dp->set_lanes == 0?

Because udphy_dp_phy_verify_config() does not check that dp->lanes is within
udphy_dplane_get(udphy), passing dp->lanes == 4 while in UDPHY_MODE_DP_USB
is accepted and causes udphy_dp_phy_set_rate() and
udphy_dp_phy_set_voltages() to overwrite the PMA registers for all 4 lanes,
clobbering the 2 lanes assigned to USB3.

Also, struct phy_configure_opts_dp gates lane reconfiguration on
dp->set_lanes, so callers updating only link rate or voltage levels with
dp->set_lanes == 0 may leave dp->lanes unset and fail this check with
-EINVAL.

> +
> +	/*
> +	 * If changing voltages is required, check swing and pre-emphasis
> +	 * levels, per-lane.
> +	 */
> +	if (dp->set_voltages) {
> +		for (i = 0; i < dp->lanes; i++) {
> +			if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> +				return -EINVAL;
> +
> +			/*
> +			 * Sum of voltage swing and pre-emphasis levels cannot
> +			 * exceed 3.
> +			 */
> +			if (dp->voltage[i] + dp->pre[i] > 3)
> +				return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int udphy_dp_phy_set_rate(struct cix_udphy *udphy,
> +				 struct phy_configure_opts_dp *dp)
> +{
> +	int ret;
> +	const struct phy_link_cfg *conf = NULL;
> +
> +	dev_dbg(udphy->dev, "lane %d, rate %x\n", dp->lanes, dp->link_rate);
> +
> +	regmap_update_bits(udphy->phy_regmap, PHY_PMA_PLL_CTRL, BIT(1), 0x0);
> +	usleep_range(10, 20);
> +
> +	switch (dp->link_rate) {
> +	case 0x6:
> +		conf = &link_cfg[0];
> +		ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> +					     ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> +		break;
> +	case 0xa:
> +		conf = &link_cfg[1];
> +		ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> +					     ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> +		break;
> +	case 0x14:
> +		conf = &link_cfg[2];
> +		ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_partial_conf,
> +					     ARRAY_SIZE(sky1_udphy_dp_partial_conf));
> +		break;
> +	case 0x1e:
> +		conf = &link_cfg[3];
> +		ret = regmap_multi_reg_write(udphy->phy_regmap, sky1_udphy_dp_8p1_conf,
> +					     ARRAY_SIZE(sky1_udphy_dp_8p1_conf));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

[Severity: High]
Should udphy_dp_phy_set_rate() match dp->link_rate against link rates in Mb/s
(1620, 2700, 5400, 8100) rather than raw DPCD link bandwidth codes (0x6,
0xa, 0x14, 0x1e)?

In include/linux/phy/phy-dp.h, struct phy_configure_opts_dp specifies
link_rate in Mb/s. Standard DisplayPort controller drivers passing 1620,
2700, 5400, or 8100 Mb/s will hit the default case and fail with -EINVAL.

Additionally, because udphy_dp_phy_verify_config() does not validate
dp->link_rate before udphy_dp_phy_set_rate() clears BIT(1) in
PHY_PMA_PLL_CTRL, returning -EINVAL here (or on a regmap_multi_reg_write()
failure) leaves PLL1 disabled in hardware.

[ ... ]

> +static int udphy_dp_phy_set_voltages(struct cix_udphy *udphy,
> +				     struct phy_configure_opts_dp *dp)
> +{
> +	u32 index = dp->voltage[0] * 4 + dp->pre[0];
> +	const struct phy_voltage_conf *cfg = volt_cfg;

[Severity: High]
Why does udphy_dp_phy_set_voltages() compute index only from dp->voltage[0]
and dp->pre[0] and apply it to all lanes?

In udphy_dp_phy_verify_config(), dp->voltage[i] and dp->pre[i] are validated
for each lane i up to dp->lanes, but udphy_dp_phy_set_voltages() ignores
dp->voltage[1..3] and dp->pre[1..3] and programs lane 0's swing and
pre-emphasis settings into every active lane in the 2-lane and 4-lane cases.

[ ... ]

> +static int udphy_dp_phy_configure(struct phy *phy,
> +				  union phy_configure_opts *opts)
> +{
> +	struct cix_udphy *udphy = phy_get_drvdata(phy);
> +	int ret;
> +
> +	mutex_lock(&udphy->mutex);
> +	ret = udphy_dp_phy_verify_config(udphy, &opts->dp);
> +	if (ret)
> +		goto unlock;
> +
> +	if (opts->dp.set_rate) {
> +		ret = udphy_dp_phy_set_rate(udphy, &opts->dp);
> +		if (ret) {
> +			dev_err(udphy->dev, "failed to set DP link rate\n");
> +			goto unlock;
> +		}
> +	}
> +
> +	if (opts->dp.set_voltages) {
> +		ret = udphy_dp_phy_set_voltages(udphy, &opts->dp);
> +		if (ret) {
> +			dev_err(udphy->dev, "dp_phy_set_voltages failed\n");
> +			goto unlock;
> +		}
> +	}
> +	mutex_unlock(&udphy->mutex);
> +	return 0;
> +
> +unlock:
> +	mutex_unlock(&udphy->mutex);
> +	return ret;
> +}

[Severity: High]
Can udphy_dp_phy_configure() access PHY MMIO registers while udphy->apb_clk
is disabled and udphy->preset is asserted?

In cix_udphy_probe() and sky1_udphy_exit(), udphy->reset and udphy->preset
are asserted and udphy->apb_clk is disabled. If udphy_dp_phy_power_on() is
called when UDPHY_MODE_DP is not set in udphy->next_mode (for example, when
usbdp_typec_mux_set() sets udphy->next_mode = UDPHY_MODE_USB for
TYPEC_STATE_SAFE or USB-only mode), udphy_dp_phy_power_on() returns 0 without
enabling udphy->apb_clk or deasserting udphy->preset.

If the DisplayPort controller then calls phy_configure() (or calls
phy_configure() before phy_power_on()), udphy_dp_phy_configure() does not
check whether udphy->cur_mode & UDPHY_MODE_DP is active before accessing
udphy->phy_regmap in udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages().

[ ... ]

> +static int cix_u3phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	if (mode == PHY_MODE_INVALID)
> +		return cix_u3phy_exit(phy);
> +
> +	return cix_u3phy_init(phy);
> +}
> +
> +static const struct phy_ops cix_u3phy_ops = {
> +	.set_mode       = cix_u3phy_set_mode,
> +	.exit		= cix_u3phy_exit,
> +	.owner		= THIS_MODULE,
> +};

[Severity: High]
Is it intentional that cix_u3phy_ops defines .exit = cix_u3phy_exit without
.init, .power_on, or .power_off, and instead wires cix_u3phy_init() and
cix_u3phy_exit() into .set_mode?

Standard USB controller drivers (such as cdns3, dwc3, and xhci-plat) manage
the PHY lifecycle through phy_init(), phy_power_on(), phy_power_off(), and
phy_exit(). During system resume (for example in cdns3_controller_resume()),
calling phy_exit() followed by phy_init() and phy_power_on() without calling
phy_set_mode() will run cix_u3phy_exit() and leave the USB3 PHY powered off
because .init and .power_on are NULL.

[ ... ]

> +static void udphy_set_default_mode(struct cix_udphy *udphy)
> +{
> +	udphy->cur_mode = UDPHY_MODE_NONE;
> +}
> +
> +static int cix_udphy_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = device_property_read_u8(dev, "cix,default-conf", &udphy->next_mode);
> +	if (ret)
> +		udphy->next_mode = UDPHY_MODE_DP_USB;
> +
> +	reset_control_assert(udphy->reset);
> +	reset_control_assert(udphy->preset);

[ ... ]

> +	udphy_set_default_mode(udphy);

[Severity: Medium]
The commit message states:

  The bootloader may hand over the port already initialized; in that
  case the first power_on calls skip the re-initialization.

However, cix_udphy_probe() unconditionally asserts both udphy->reset and
udphy->preset, and udphy_set_default_mode() sets udphy->cur_mode =
UDPHY_MODE_NONE, so the first call to cix_usbdp_phy_power_on() will always
re-initialize the PHY. Does the probe path or commit message need to be
updated to match the intended bootloader handover behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102082112.2306326-1-gary.yang@cixtech.com?part=2