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
