Re: [PATCH 05/17] phy: spacemit: add Innosilicon DP TX PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Hardware state (PREPLL) is accessed concurrently without protection across PHY and CCF frameworks.
- [Medium] The driver fails to power off the core DP link PLL (MPLL) when the PHY is disabled, causing a power leak.
- [Medium] Missing cleanup in the `k3_inno_dp_phy_power_on` error paths leaves PHY components actively consuming power.
- [Medium] Dynamically reducing the lane count via `phy_configure()` does not disable the unused transmit lane drivers.
- [Medium] Unconditional PLL power-up during rate configuration overrides the PHY power state.
- [Medium] Missing bounds check on `opts->lanes` causes out-of-bounds array reads and undefined shift behavior in `k3_inno_dp_phy_set_voltages`.
- [Medium] The pixel clock (`pxclk`) is registered with the CCF without `.prepare`/`.unprepare` or `.enable`/`.disable` hooks, yet its power state is manipulated implicitly.
--

--- Patch [5]: [PATCH 05/17] phy: spacemit: add Innosilicon DP TX PHY driver ---
commit 46b764d91a977bdc796e404d7eaabb0362e753b0
Author: Cody Kang <codykang.hk@gmail.com>
Subject: phy: spacemit: add Innosilicon DP TX PHY driver

This commit adds support for the Innosilicon DisplayPort transmit PHY
integrated in the SpacemiT K3 SoC. The PHY shares its MMIO window with its
parent DP/eDP controller and exposes lane, rate, and voltage control. It
also registers its pixel PLL as a clock provider.

> diff --git a/drivers/phy/spacemit/phy-k3-inno-dp.c b/drivers/phy/spacemit/phy-k3-inno-dp.c
> new file mode 100644
> index 0000000000000..9dd62edb7a5af
> --- /dev/null
> +++ b/drivers/phy/spacemit/phy-k3-inno-dp.c

[ ... ]

> @@ -103,45 +103,45 @@ static int k3_inno_dp_phy_power_on(struct phy *phy)
> +	struct k3_inno_dp_phy *p = phy_get_drvdata(phy);
> +	u32 lane_en;
> +	u32 val;
> +	int ret;
> +
> +	switch (p->lanes) {
> +	case 1:
> +		lane_en = 0x1;
> +		break;
> +	case 2:
> +		lane_en = 0x3;
> +		break;
> +	case 4:
> +	default:
> +		lane_en = 0xf;
> +		break;
> +	}
> +
> +	ret = regmap_write_bits(p->regmap, DPTX_ANA_MPLL, DPTX_ANA_MPLL_PD, 0);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write_bits(p->regmap, DPTX_ANA_PREPLL,
> +				DPTX_ANA_PREPLL_PD, 0);
> +	if (ret)
> +		return ret;

[Severity: High]
Are there potential concurrency issues between the CCF and PHY frameworks when 
accessing the PREPLL?

The CCF calls k3_inno_dp_pxclk_set_rate() holding the prepare_lock, which 
powers down the PREPLL, sleeps, and programs registers. Meanwhile, the PHY 
framework calls k3_inno_dp_phy_power_on() holding phy->mutex. 

Could a concurrent phy_power_on() clear the PREPLL_PD bit during the sleep 
window, causing the clock framework to write configuration values to a 
running PLL?

> +	usleep_range(2000, 4000);
> +
> +	ret = regmap_write_bits(p->regmap, DPTX_PHY_CTRL,
> +				DPTX_PHY_CTRL_XMIT_EN,
> +				FIELD_PREP(DPTX_PHY_CTRL_XMIT_EN, lane_en));
> +	if (ret)
> +		return ret;
> +	usleep_range(2000, 4000);
> +
> +	ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_MPLL, val,
> +				       val & DPTX_ANA_MPLL_LOCKED,
> +				       2000, 10 * 1000);
> +	if (ret) {
> +		dev_err(p->dev, "DP PHY core PLL lock timed out\n");
> +		return ret;
> +	}

[Severity: Medium]
If regmap_read_poll_timeout() times out here or on the PREPLL lock, we return 
an error without reverting the powered-up hardware state.

Since the PHY core framework doesn't invoke phy_power_off() if phy_power_on() 
fails, does this leave the MPLL, PREPLL, and transmit lanes indefinitely 
powered on in an error state?

> +
> +	ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_PREPLL, val,
> +				       val & DPTX_ANA_PREPLL_LOCKED,
> +				       2000, 10 * 1000);
> +	if (ret)
> +		dev_err(p->dev, "DP PHY pixel PLL lock timed out\n");
> +
> +	return ret;
> +}
> +
> +static int k3_inno_dp_phy_power_off(struct phy *phy)
> +{
> +	struct k3_inno_dp_phy *p = phy_get_drvdata(phy);
> +
> +	regmap_write_bits(p->regmap, DPTX_PHY_CTRL, DPTX_PHY_CTRL_XMIT_EN, 0);
> +	usleep_range(2000, 4000);
> +
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +			  DPTX_ANA_PREPLL_PD);
> +	usleep_range(2000, 4000);
> +
> +	return 0;
> +}

[Severity: Medium]
This powers down the pixel PLL and disables the transmit lanes, but it
appears to skip powering down the core DP link PLL.

Does this leave the MPLL running indefinitely and leaking power when the PHY 
is meant to be disabled? Should we also be setting DPTX_ANA_MPLL_PD here?

[ ... ]

> +static int k3_inno_dp_phy_set_lanes(struct k3_inno_dp_phy *p, u8 lanes)
> +{
> +	u32 val;
> +	int ret;

[ ... ]

> +	ret = regmap_write_bits(p->regmap, DPTX_PHY_CTRL,
> +				DPTX_PHY_CTRL_NUM_LANES,
> +				FIELD_PREP(DPTX_PHY_CTRL_NUM_LANES, val));
> +	if (ret)
> +		return ret;
> +
> +	p->lanes = lanes;
> +	return 0;
> +}

[Severity: Medium]
If a caller dynamically reduces the lane count via phy_configure() (for 
instance, falling back from 4 lanes to 2 during DP link training), this 
updates the logical lane count but doesn't synchronize the 
DPTX_PHY_CTRL_XMIT_EN register bits.

Will this leave the unused physical transmit lane drivers actively driven?

[ ... ]

> +static void k3_inno_dp_phy_program_mpll(struct k3_inno_dp_phy *p,
> +					const struct k3_inno_dp_mpll_cfg *cfg)
> +{
> +	struct regmap *rm = p->regmap;

[ ... ]

> +	regmap_write_bits(rm, DPTX_ANA_PREPLL_CTRL, DPTX_ANA_PREPLL_LOCK_BYPEN,
> +		FIELD_PREP(DPTX_ANA_PREPLL_LOCK_BYPEN, 1));
> +
> +	regmap_write_bits(rm, DPTX_ANA_MPLL, DPTX_ANA_MPLL_PD,
> +		FIELD_PREP(DPTX_ANA_MPLL_PD, 0));
> +	usleep_range(2000, 4000);
> +}

[Severity: Medium]
When configuring the PLL rate, this clears the power down bit 
(DPTX_ANA_MPLL_PD) at the end of the programming sequence.

If a rate configuration occurs while the PHY is intentionally suspended, will 
this force the PLLs on and override the PHY's intended power state?

[ ... ]

> +static int k3_inno_dp_phy_set_voltages(struct k3_inno_dp_phy *p,
> +				       struct phy_configure_opts_dp *opts)
> +{
> +	u8 lane;
> +	int ret;
> +
> +	for (lane = 0; lane < opts->lanes; lane++) {
> +		unsigned int shift = lane * DPTX_PHY_LANE_BITS_PER;
> +		u32 mask = DPTX_PHY_LANE_FIELD_MASK << shift;
> +		u32 val  = (((opts->voltage[lane] & 0x3) << DPTX_PHY_LANE_VSWING_SHIFT) |
> +			    (opts->pre[lane] & 0x3)) << shift;

[Severity: Medium]
Could an unvalidated or uninitialized opts->lanes cause an out-of-bounds
array read on opts->voltage and opts->pre, as well as undefined bit shifts
here?

In k3_inno_dp_phy_configure(), k3_inno_dp_phy_set_lanes() is only called if 
opts->dp.set_lanes is true. If a caller passes opts->dp.set_lanes = 0 but 
opts->dp.set_voltages = 1, opts->dp.lanes skips validation.

[ ... ]

> +static void k3_inno_dp_program_prepll(struct k3_inno_dp_phy *p,
> +				      const struct k3_inno_dp_prepll_cfg *cfg)
> +{
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PD, 1));
> +	usleep_range(2000, 4000);
> +

[ ... ]

> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PD, 0));
> +	usleep_range(2000, 4000);
> +}

[Severity: Medium]
Similar to the MPLL configuration, this unconditionally clears the PREPLL 
power down bit at the end of programming, potentially overriding the intended 
power state of the PHY.

[ ... ]

> +static const struct clk_ops k3_inno_dp_pxclk_ops = {
> +	.recalc_rate	= k3_inno_dp_pxclk_recalc_rate,
> +	.determine_rate	= k3_inno_dp_pxclk_determine_rate,
> +	.set_rate	= k3_inno_dp_pxclk_set_rate,
> +};

[Severity: Medium]
The pixel clock is registered with the Common Clock Framework without 
.prepare/.unprepare or .enable/.disable hooks.

Since the CCF tracks the clock state purely in software while phy_power_off() 
physically powers down the PREPLL, can this cause a mismatch where downstream 
consumers lose their clock unexpectedly while the CCF thinks it is still 
running?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-k3-display-v1-0-f22a10a52867@gmail.com?part=5

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