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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing driver-level lock between concurrent clock and PHY operations creates a race condition on PREPLL state.
- [Medium] Failure to handle `XMIT_EN` regmap write error leaks the `PREPLL` power state during `k3_inno_dp_phy_power_on`.
- [Medium] `k3_inno_dp_pxclk_set_rate` unconditionally powers up the PREPLL, undermining PHY power management.
--

--- Patch [5]: [PATCH v2 05/17] phy: spacemit: add Innosilicon DP TX PHY driver ---
commit 29f359dc5de12bf07a1c791d1b6d5ab3f658ed91
Author: Cody Kang <[email protected]>

phy: spacemit: add Innosilicon DP TX PHY driver

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 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..42ebfc347b84b
> --- /dev/null
> +++ b/drivers/phy/spacemit/phy-k3-inno-dp.c

[ ... ]

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

[Severity: High]
Is a driver-level lock needed to prevent races between the PHY and clock
frameworks?

The clock framework serializes clk_set_rate() (which calls
k3_inno_dp_program_prepll() below), and the PHY framework serializes
phy_power_on(), but they don't synchronize with each other.

A clock rate change can race with this PHY power-on, allowing the PHY to
clear the PD bit here while the clock framework is midway through
reprogramming the PLL dividers.

> +	if (ret)
> +		return ret;
> +	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;

[Severity: Medium]
Does this skip the required DPTX_ANA_PREPLL_PD rollback?

If this regmap write fails, returning directly bypasses the err_power_down
cleanup label. This would leave the pixel PLL powered up indefinitely.

> +	usleep_range(2000, 4000);
> +
> +	ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_MPLL, val,
> +				       val & DPTX_ANA_MPLL_LOCKED,
> +				       2000, 10 * 1000);

[ ... ]

> +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_PREDIV,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PREDIV, cfg->prediv));

[ ... ]

> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL_CTRL,
> +		DPTX_ANA_PREPLL_PCLKDIV5_EN,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PCLKDIV5_EN, cfg->div5_en));
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL_CTRL,
> +		DPTX_ANA_PREPLL_PCLK_DIVAUX,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PCLK_DIVAUX, cfg->divp));
> +
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL,
> +		DPTX_ANA_PREPLL_PCLK_NORMAL,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PCLK_NORMAL, 1));
> +	usleep_range(2000, 4000);
> +
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PD, 0));

[Severity: Medium]
Does this unconditionally power up the PREPLL, even if the PHY is currently
turned off?

The clock framework can call set_rate() when the display pipeline is
disabled (for example, via assigned-clock-rates during boot). Since the
clock provider doesn't implement a .disable callback, this appears to force
the PLL to turn on and stay powered up until a subsequent phy_power_off()
cycle occurs.

> +	usleep_range(2000, 4000);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy