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: wrong link_rate units and early PLL disable in udphy_dp_phy_set_rate()
- [High] phy: cix: lane 0 voltage and pre-emphasis applied to all lanes in udphy_dp_phy_set_voltages()
- [High] phy: cix: missing .init callback in cix_u3phy_ops alongside .exit
- [High] phy: cix: clock enable leak and broken mode/orientation state in cix_usbdp_phy_power_on()
- [High] phy: cix: unclocked MMIO access and missing lane bound in udphy_dp_phy_configure()
- [Medium] phy: cix: missing SVID check for DisplayPort states in usbdp_typec_mux_set()
- [Medium] phy: cix: probe resets PHY despite bootloader handover claim and dead macros
--

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

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

This commit adds a Generic PHY driver for the CIX Sky1 USB3/DisplayPort combo
PHY, exposing separate USB and DP child PHYs and 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
> @@ -0,0 +1,1477 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * phy driver for cdn_sd0804_t7g_typec
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>
> +
> +#include "phy-cix-usbdp.h"
> +
> +enum {
> +	UDPHY_MODE_NONE		= 0,
> +	UDPHY_MODE_USB		= BIT(0),
> +	UDPHY_MODE_DP		= BIT(1),
> +	UDPHY_MODE_DP_USB	= BIT(1) | BIT(0),
> +};
> +
> +#define PHY_LANE_MUX_USB			0
> +#define PHY_LANE_MUX_DP				1

[ ... ]

> +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 usbdp_typec_mux_set(struct typec_mux_dev *mux,
> +			       struct typec_mux_state *state)
> +{
> +	struct cix_udphy *udphy = typec_mux_get_drvdata(mux);
> +
> +	mutex_lock(&udphy->mutex);
> +
> +	switch (state->mode) {
> +	case TYPEC_DP_STATE_C:
> +		fallthrough;
> +	case TYPEC_DP_STATE_E:
> +		udphy->next_mode = UDPHY_MODE_DP;
> +		break;
> +	case TYPEC_DP_STATE_D:
> +		udphy->next_mode = UDPHY_MODE_DP_USB;
> +		break;
> +	case TYPEC_STATE_SAFE:
> +		udphy->next_mode = UDPHY_MODE_USB;
> +		break;
> +	default:
> +		/* usb only handle as usb+dp mode */
> +		udphy->next_mode = UDPHY_MODE_USB;
> +		break;
> +	}
> +
> +	/* TODO: check whether the HPD state needs software */
> +	mutex_unlock(&udphy->mutex);
> +
> +	return 0;
> +}

[Severity: Medium]
In drivers/phy/cix/phy-cix-usbdp.c, usbdp_typec_mux_set() switches directly
on state->mode without checking state->alt.

In include/linux/usb/typec_altmode.h and include/linux/usb/typec_dp.h, both
connector modal states (TYPEC_MODE_USB2 = 2, TYPEC_MODE_USB3 = 3,
TYPEC_MODE_USB4 = 4, TYPEC_MODE_AUDIO = 5, TYPEC_MODE_DEBUG = 6) and
DisplayPort pin assignment states (TYPEC_DP_STATE_A = 2 through
TYPEC_DP_STATE_F = 7) start at TYPEC_STATE_MODAL.

If the Type-C mux is called with a non-DisplayPort modal state such as
TYPEC_MODE_USB4, TYPEC_MODE_AUDIO, TYPEC_MODE_DEBUG, or another alternate
mode SVID, won't usbdp_typec_mux_set() match TYPEC_DP_STATE_C,
TYPEC_DP_STATE_D, or TYPEC_DP_STATE_E and misconfigure udphy->next_mode for
DisplayPort unless it first verifies state->alt && state->alt->svid ==
USB_TYPEC_DP_SID?

[ ... ]

> +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]
How does udphy->cur_mode track both the active sub-PHYs and the programmed
hardware configuration in drivers/phy/cix/phy-cix-usbdp.c?

First, sky1_udphy_init() unconditionally calls
sky1_udphy_clk_enable(udphy->apb_clk), which calls clk_prepare_enable(), while
sky1_udphy_exit() calls clk_disable_unprepare(udphy->apb_clk) only once when
udphy->cur_mode reaches UDPHY_MODE_NONE. If one child PHY is already active
(for example, cix_u3phy_init() set udphy->cur_mode = UDPHY_MODE_USB) and the
mux switches to udphy->next_mode = UDPHY_MODE_DP before
udphy_dp_phy_power_on() runs, cix_usbdp_phy_power_on() calls sky1_udphy_init()
a second time and leaks an enable reference on udphy->apb_clk. In that same
path, udphy_dp_phy_power_on() then assigns udphy->cur_mode = UDPHY_MODE_DP,
clearing UDPHY_MODE_USB so that a later cix_u3phy_exit() becomes a no-op.

Second, if udphy_dp_phy_power_on() initializes the PHY with udphy->next_mode
== UDPHY_MODE_DP, sky1_udphy_init() programs sky1_udphy_dp_conf (4-lane
DP-only mode) and sets udphy->cur_mode = UDPHY_MODE_DP. If the mux later
switches udphy->next_mode to UDPHY_MODE_DP_USB and cix_u3phy_init() calls
cix_usbdp_phy_power_on(), the check:

	if ((udphy->cur_mode == UDPHY_MODE_USB || udphy->cur_mode == UDPHY_MODE_DP) &&
	    udphy->next_mode == UDPHY_MODE_DP_USB)
		return 0;

returns early without calling sky1_udphy_init(), leaving the hardware in
4-lane DP-only mode with the USB3 lanes unconfigured.

Third, udphy_orien_sw_set() updates udphy->flip without marking the PHY as
needing re-initialization. If udphy->flip changes after one sub-PHY has
already initialized in UDPHY_MODE_DP_USB, won't cix_usbdp_phy_power_on() skip
sky1_udphy_init(), leaving the syscon orientation bit and lane configuration
tables in the old orientation while udphy_dp_phy_set_rate() and
udphy_dp_phy_set_voltages() program the flipped PMA lanes?

[ ... ]

> +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;
> +	}
> +
> +	/*
> +	 * 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]
In drivers/phy/cix/phy-cix-usbdp.c, udphy_dp_phy_set_rate() switches on
dp->link_rate using DPCD bandwidth code values (0x6, 0xa, 0x14, 0x1e).

According to include/linux/phy/phy-dp.h, struct phy_configure_opts_dp
defines link_rate in Mb/s (1620, 2700, 5400, 8100). When a standard
DisplayPort bridge or controller driver calls phy_configure() ->
udphy_dp_phy_configure() -> udphy_dp_phy_set_rate() with dp->set_rate = 1 and
dp->link_rate in Mb/s, won't it always hit the default case and fail with
-EINVAL?

Also, because udphy_dp_phy_verify_config() does not validate dp->link_rate and
udphy_dp_phy_set_rate() clears BIT(1) of PHY_PMA_PLL_CTRL before the switch
statement, returning -EINVAL on an unsupported rate leaves PLL1 disabled.

[ ... ]

> +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;
> +
> +	dev_dbg(udphy->dev, "lane %d, vs %d, pe %d\n", dp->lanes, dp->voltage[0],
> +		dp->pre[0]);
> +
> +	switch (dp->lanes) {
> +	case 1:
> +		if (!udphy->flip) { /* lane 2 */
> +			regmap_update_bits(udphy->phy_regmap, TX_DIAG_ACYA_LANE2,
> +					   BIT(0), 0x1);
> +			regmap_write(udphy->phy_regmap, TX_TXCC_CTRL_LANE2,
> +				     cfg[index].tx_txcc_ctrl);
> +			regmap_write(udphy->phy_regmap, DRV_DIAG_TX_DRV_LANE2,
> +				     cfg[index].drv_diag_tx_drv);
> +			regmap_write(udphy->phy_regmap, TX_TXCC_MGNFS_MULT_000_LANE2,
> +				     cfg[index].tx_txcc_mgnfc_mult_000);
> +			regmap_write(udphy->phy_regmap, TX_TXCC_CPOST_MULT_00_LANE2,
> +				     cfg[index].tx_txcc_cpost_mult_00);
> +			regmap_update_bits(udphy->phy_regmap, TX_DIAG_ACYA_LANE2,
> +					   BIT(0), 0x0);

[Severity: High]
In drivers/phy/cix/phy-cix-usbdp.c, udphy_dp_phy_verify_config() validates
dp->voltage[i] and dp->pre[i] for each lane up to dp->lanes, but
udphy_dp_phy_set_voltages() computes index solely from dp->voltage[0] and
dp->pre[0] and writes cfg[index] to all active lanes in the 2-lane and 4-lane
cases.

During DisplayPort link training on a 2-lane or 4-lane link, if the sink
requests different voltage swing or pre-emphasis levels across lanes, won't
lanes 1..3 be programmed with lane 0's settings instead of dp->voltage[i] and
dp->pre[i]?

Could this also use a per-lane helper instead of duplicating the 6-register
programming sequence across all branches?

[ ... ]

> +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]
What happens if udphy_dp_phy_configure() in drivers/phy/cix/phy-cix-usbdp.c is
called before udphy_dp_phy_power_on(), or after udphy_dp_phy_power_on()
returned 0 early because !(udphy->next_mode & UDPHY_MODE_DP)?

In both cases, udphy->apb_clk is disabled and udphy->preset is asserted (from
cix_udphy_probe() or sky1_udphy_exit()), yet udphy_dp_phy_configure() proceeds
to call udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages(), performing
MMIO reads and writes on udphy->base without checking (udphy->cur_mode &
UDPHY_MODE_DP). Won't accessing those registers with the APB clock gated and
reset asserted cause a bus abort or hang?

Additionally, udphy_dp_phy_verify_config() allows dp->lanes == 4 even when
udphy->next_mode is UDPHY_MODE_DP_USB (where udphy_dplane_get(udphy) is 2),
which causes udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages() to
overwrite the PMA registers of the active USB3 lanes.

[ ... ]

> +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]
In drivers/phy/cix/phy-cix-usbdp.c, cix_u3phy_ops defines .exit =
cix_u3phy_exit, but omits .init = cix_u3phy_init and only invokes
cix_u3phy_init() from cix_u3phy_set_mode().

When a USB controller driver calls phy_init() during probe before
phy_set_mode(), or calls phy_exit() followed by phy_init() across suspend and
resume without calling phy_set_mode(), won't phy_init() be a no-op that leaves
the USB3 PHY uninitialized and in reset?

[ ... ]

> +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)
> +{
> +	struct cix_udphy *udphy;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct device_node *np = dev->of_node;
> +	u32 id;
> +	int ret = 0;

[ ... ]

> +	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, in drivers/phy/cix/phy-cix-usbdp.c, cix_udphy_probe() unconditionally
calls reset_control_assert() on udphy->reset and udphy->preset, and
udphy_set_default_mode() unconditionally sets udphy->cur_mode =
UDPHY_MODE_NONE. As a result, any bootloader initialization is wiped out in
probe and the first power_on call never skips re-initialization. Should
cix_udphy_probe() and udphy_set_default_mode() be updated to match the commit
description, or should the commit message be updated to match the code?

Also, PHY_LANE_MUX_USB and PHY_LANE_MUX_DP at the top of
drivers/phy/cix/phy-cix-usbdp.c are unused and can be removed.

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

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