Re: [PATCH 3/3] arm64: dts: cix: add Sky1 USB/DP combo PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] phy: cix: link rate unit mismatch and PLL disable in udphy_dp_phy_set_rate()
- [High] phy: cix: unclocked MMIO and lane check in udphy_dp_phy_configure()
- [High] phy: cix: clock leak and skipped re-init in cix_usbdp_phy_power_on()
- [Medium] phy: cix: per-lane voltages ignored in udphy_dp_phy_set_voltages()
- [Medium] dt-bindings: phy: cix,sky1-usbdp-phy: missing OF graph port properties
--

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

arm64: dts: cix: add Sky1 USB/DP combo PHY

This series adds the device tree binding, driver, and device tree nodes for
the CIX Sky1 USB/DP combo PHY, supporting SuperSpeedPlus USB and DisplayPort
lane muxing and Type-C orientation switching via the S5 system controller.

> diff --git a/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> new file mode 100644
> index 0000000000000..a5b6943f84ebd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cix,sky1-usbdp-phy.yaml
> @@ -0,0 +1,150 @@

[ ... ]

> +  orientation-switch:
> +    description:
> +      The PHY handles Type-C plug orientation switching.
> +    type: boolean
> +
> +  mode-switch:
> +    description:
> +      The PHY handles multiplexing of the USB and DisplayPort lanes.
> +    type: boolean

[Severity: Medium]
When a board device tree connects a usb-c-connector node to this PHY for
Type-C orientation and mode switching, the connector links to the provider
via OF graph port or ports endpoints (matched by fwnode_graph_devcon_matches()
in typec_switch_match() and typec_mux_match()).

Because this schema specifies additionalProperties: false without defining
port or ports properties, won't board device trees that wire a Type-C
connector to a cix,sky1-usbdp-phy node fail dtbs_check validation?

[ ... ]

> 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 @@

[ ... ]

> +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 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 are udphy->apb_clk and the hardware lane configuration kept in sync when
transitioning between Type-C modes and orientations?

First, if USB powers on first (setting udphy->cur_mode = UDPHY_MODE_USB and
enabling udphy->apb_clk once in sky1_udphy_init()), and the Type-C mux then
selects 4-lane DP (udphy->next_mode = UDPHY_MODE_DP) followed by
udphy_dp_phy_power_on():

  udphy_dp_phy_power_on()
    cix_usbdp_phy_power_on()
      sky1_udphy_init()
        sky1_udphy_clk_enable(udphy->apb_clk);
    udphy->cur_mode = UDPHY_MODE_DP;

sky1_udphy_init() calls clk_prepare_enable(udphy->apb_clk) a second time, and
udphy_dp_phy_power_on() overwrites udphy->cur_mode = UDPHY_MODE_DP, dropping
UDPHY_MODE_USB. When the PHYs later power off, sky1_udphy_exit() runs at most
once, leaking a prepare/enable reference on udphy->apb_clk.

Second, if DP powers on first in 4-lane mode (udphy->cur_mode = UDPHY_MODE_DP,
programmed with sky1_udphy_dp_conf) and udphy->next_mode later changes to
UDPHY_MODE_DP_USB, cix_usbdp_phy_power_on() hits the early return:

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

Doesn't that skip reprogramming the PHY from 4-lane DP to the USB+DP combo
configuration when cix_u3phy_init() is called?

Third, if udphy_orien_sw_set() updates udphy->flip while one sub-PHY is
already active in udphy->cur_mode, cix_usbdp_phy_power_on() also returns 0
without updating udphy->usbphy_syscon or loading the flipped register table
(sky1_udphy_usbdp_conf_flip), while subsequent calls to
udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages() program the flipped
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;
> +	}

[ ... ]

> +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 include/linux/phy/phy-dp.h, struct phy_configure_opts_dp defines link_rate
in Mb/s (1620, 2700, 5400, 8100), whereas udphy_dp_phy_set_rate() matches
dp->link_rate against raw DPCD bandwidth codes (0x6, 0xa, 0x14, 0x1e).

Won't standard DisplayPort controller drivers passing link_rate in Mb/s to
phy_configure() -> udphy_dp_phy_configure() -> udphy_dp_phy_set_rate() always
hit the default case and fail with -EINVAL, while also leaving BIT(1) of
PHY_PMA_PLL_CTRL cleared?

[ ... ]

> +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]);

[Severity: Medium]
udphy_dp_phy_verify_config() validates dp->voltage[i] and dp->pre[i] for each
lane i < dp->lanes, but udphy_dp_phy_set_voltages() computes index solely from
dp->voltage[0] and dp->pre[0] and writes that configuration to all active
lanes.

Should udphy_dp_phy_set_voltages() compute the volt_cfg index per lane using
each lane's own dp->voltage[i] and dp->pre[i] values so per-lane link training
adjustments are applied?

[ ... ]

> +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;
> +		}
> +	}

[Severity: High]
What happens if udphy_dp_phy_configure() is called when UDPHY_MODE_DP is not
active in udphy->cur_mode, or with more lanes than allocated to DP?

In udphy_dp_phy_power_on(), if !(udphy->next_mode & UDPHY_MODE_DP) (for
example when Type-C is in TYPEC_STATE_SAFE or USB-only mode, or when
cix,default-conf is 0 or 1), udphy_dp_phy_power_on() returns 0 without
enabling udphy->apb_clk or deasserting udphy->preset. A subsequent call to
udphy_dp_phy_configure() will then perform MMIO reads and writes through
udphy->phy_regmap while the APB clock is disabled and APB reset is asserted.

In addition, udphy_dp_phy_verify_config() accepts dp->lanes == 4 without
checking against udphy_dplane_get(udphy), so a 4-lane DP configuration in
2-lane UDPHY_MODE_DP_USB mode will overwrite the active USB lane registers in
udphy_dp_phy_set_rate() and udphy_dp_phy_set_voltages().

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

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