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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ usleep_range(2000, 4000);
+
+ ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_MPLL, val,
+ val & DPTX_ANA_MPLL_LOCKED,
+ 2000, 10 * 1000);
[ ... ]
quoted hunk ↗ jump to hunk
+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));
[ ... ]
quoted hunk ↗ jump to hunk
+ 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/20260809-k3-display-v2-0-327d7910bf71@gmail.com?part=5
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy