Re: [PATCH v6 4/5] phy: starfive: Add jh7110-inno-hdmi-phy driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] phy: starfive: hdmi: fix rate cache and BIAS/RX state across disable/enable
- [High] phy: starfive: jh7110-inno-hdmi: invalid pre_pll_cfg_table divider values
- [Medium] phy: starfive: jh7110-inno-hdmi: broken Pre-PLL power and lock sequencing in clk_ops
- [Low] phy: starfive: unused inno_read() helper and register macros in hdmi phy
--

commit de90e248a2b1a2d110bed58fd3d5de2e75a01d3d
Author: Michal Wilczynski <m.wilczynski@samsung.com>

phy: starfive: Add jh7110-inno-hdmi-phy driver

This commit adds the Innosilicon HDMI PHY driver for the StarFive JH7110 SoC.
The driver acts as both a clock provider for the variable pixel clock via the
Pre-PLL and a PHY provider configuring the Post-PLL and analog blocks.

> diff --git a/drivers/phy/starfive/phy-jh7110-inno-hdmi.c b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c
> new file mode 100644
> index 0000000000000..ebda363e50a3e
> --- /dev/null
> +++ b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c
[ ... ]
> +/* REG: 0x1aa */
> +#define STF_INNO_POST_PLL_DIV_1		0x1aa
> +#define STF_INNO_POST_PLL_POST_DIV_ENABLE	GENMASK(3, 2)
> +#define STF_INNO_POST_PLL_REFCLK_SEL_TMDS	BIT(1)
> +#define STF_INNO_POST_PLL_POWER_DOWN		BIT(0)
> +
> +/* REG: 0x1ab */
> +#define STF_INNO_POST_PLL_DIV_2		0x1ab
> +#define STF_INNO_POST_PLL_PRE_DIV(x)		FIELD_PREP(GENMASK(5, 0), x)
> +#define STF_INNO_POST_PLL_FB_DIV_8(x)		FIELD_PREP(BIT(7), (x) >> 8)
> +
> +/* REG: 0x1ac */
> +#define STF_INNO_POST_PLL_DIV_3		0x1ac
> +#define STF_INNO_POST_PLL_FB_DIV_7_0(x)	FIELD_PREP(GENMASK(7, 0), x)
> +
> +/* REG: 0x1ad */
> +#define STF_INNO_POST_PLL_DIV_4		0x1ad
> +#define STF_INNO_POST_PLL_POST_DIV_MASK	GENMASK(1, 0)
> +
> +/* REG: 0x1af */
> +#define STF_INNO_POST_PLL_LOCK_STATUS		0x1af
> +#define STF_INNO_POST_PLL_LOCK		BIT(0)
> +
> +/* REG: 0x1b0 */
> +#define STF_INNO_BIAS_CONTROL			0x1b0
> +#define STF_INNO_BIAS_ENABLE			BIT(2)
> +
> +/* REG: 0x1b2 */
> +#define STF_INNO_TMDS_CONTROL			0x1b2
> +#define STF_INNO_TMDS_CLK_DRIVER_EN		BIT(3)
> +#define STF_INNO_TMDS_D2_DRIVER_EN		BIT(2)
> +#define STF_INNO_TMDS_D1_DRIVER_EN		BIT(1)
> +#define STF_INNO_TMDS_D0_DRIVER_EN		BIT(0)
> +#define STF_INNO_TMDS_DRIVER_ENABLE		(STF_INNO_TMDS_CLK_DRIVER_EN | \
> +						 STF_INNO_TMDS_D2_DRIVER_EN | \
> +						 STF_INNO_TMDS_D1_DRIVER_EN | \
> +						 STF_INNO_TMDS_D0_DRIVER_EN)
[ ... ]
> +static const struct inno_hdmi_phy_pre_pll_config pre_pll_cfg_table[] = {
> +	{ 25175000, 25175000, 1, 100, 2, 3, 3, 12, 3, 3, 4, 0, 0xf55555 },
> +	{ 25200000, 25200000, 1, 100, 2, 3, 3, 12, 3, 3, 4, 0, 0 },
> +	{ 27000000, 27000000, 1, 90, 3, 2, 2, 10, 3, 3, 4, 0, 0 },
> +	{ 27027000, 27027000, 1, 90, 3, 2, 2, 10, 3, 3, 4, 0, 0x170a3d },
> +	{ 28320000, 28320000, 1, 28, 2, 1, 1, 3, 0, 3, 4, 0, 0x51eb85 },
> +	{ 30240000, 30240000, 1, 30, 2, 1, 1, 3, 0, 3, 4, 0, 0x3d70a3 },
> +	{ 31500000, 31500000, 1, 31, 2, 1, 1, 3, 0, 3, 4, 0, 0x7fffff },
> +	{ 33750000, 33750000, 1, 33, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },
[ ... ]
> +	{ 78750000, 78750000, 1, 78, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },
[ ... ]
> +	{ 85750000, 85750000, 1, 85, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },
> +	{ 85800000, 85800000, 1, 85, 2, 1, 1, 3, 0, 3, 4, 0, 0xcccccc },
> +	{ 88750000, 88750000, 1, 88, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },
[ ... ]
> +	{ 136750000, 136750000, 1, 136, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },
[ ... ]
> +	{ 262750000, 262750000, 1, 262, 2, 1, 1, 3, 0, 3, 4, 0, 0xcfffff },

[Severity: High]
Do these eight entries in pre_pll_cfg_table generate the wrong pixel clock
and cause starfive_hdmi_phy_power_on() to fail with -EINVAL?

With the 24 MHz reference clock:
- 25175000 has div = 1 * 12 * 4 * 2 = 96, so 24 MHz * (100 + 0xf55555 / 2^24)
  / 96 yields 25,239,583 Hz, which inno_hdmi_phy_pre_pll_recalc_rate()
  recalculates as 25240000 Hz.
- 25200000 has div = 96 and fracdiv = 0, so 24 MHz * 100 / 96 yields
  25000000 Hz.
- 33750000, 78750000, 85750000, 88750000, 136750000, and 262750000 have
  div = 1 * 3 * 4 * 2 = 24 and fracdiv = 0xcfffff (~0.8125 instead of
  0xc00000 = 0.75), which adds 812.5 kHz instead of 750 kHz and recalculates
  to 33812000, 78812000, 85812000, 88812000, 136812000, and 262812000 Hz.

When hdmi_pclk is prepared and starfive_hdmi_phy_clk_recalc_rate() runs:

    clk_change_rate()
      clk_recalc()
        starfive_hdmi_phy_clk_recalc_rate()
          inno_hdmi_phy_pre_pll_recalc_rate()

the clock framework caches the recalculated rate in core->rate. When
starfive_hdmi_phy_power_on() then calls:

    tmdsclock = clk_get_rate(inno->phyclk);
    pre_cfg = inno_hdmi_phy_pre_pll_lookup(&inno->pre_pll, tmdsclock,
                                           tmdsclock);

inno_hdmi_phy_pre_pll_lookup() requires an exact match against
cfg->pixclock in pre_pll_cfg_table, which fails with -EINVAL for all eight
modes.

[ ... ]
> +static inline void inno_write(struct starfive_hdmi_phy *inno, u32 reg, u8 val)
> +{
> +	regmap_write(inno->regmap, reg * 4, val);
> +}
> +
> +static inline u8 inno_read(struct starfive_hdmi_phy *inno, u32 reg)
> +{
> +	u32 val;
> +
> +	regmap_read(inno->regmap, reg * 4, &val);
> +	return val;
> +}

[Severity: Low]
Is inno_read() needed here? It is never called in this file, and the
register bit macros STF_INNO_POST_PLL_POST_DIV_ENABLE,
STF_INNO_POST_PLL_REFCLK_SEL_TMDS, STF_INNO_POST_PLL_FB_DIV_8,
STF_INNO_POST_PLL_FB_DIV_7_0, STF_INNO_POST_PLL_POST_DIV_MASK, and
STF_INNO_TMDS_DRIVER_ENABLE defined at the top of the file are also unused
(while starfive_hdmi_phy_power_on() open-codes 0x0e, 0x02, cfg->fbdiv & 0xff,
and 0x8f), which triggers compiler warnings under W=2 builds.

[ ... ]
> +static int starfive_hdmi_phy_clk_prepare(struct clk_hw *hw)
> +{
> +	struct starfive_hdmi_phy *inno = to_starfive_hdmi_phy(hw);
> +	int ret;
> +
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, false);
> +
> +	if (inno_hdmi_phy_pre_pll_is_locked(&inno->pre_pll))
> +		return 0;
> +
> +	ret = inno_hdmi_phy_pre_pll_wait_locked(&inno->pre_pll, 100000);
> +	if (ret) {
> +		dev_err(inno->dev, "Timeout waiting for pre-PLL lock\n");
> +		inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, true);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void starfive_hdmi_phy_clk_unprepare(struct clk_hw *hw)
> +{
> +	struct starfive_hdmi_phy *inno = to_starfive_hdmi_phy(hw);
> +
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, true);
> +	inno->pixclock = 0; /* Invalidate cached rate */
> +}
> +
> +static unsigned long starfive_hdmi_phy_clk_recalc_rate(struct clk_hw *hw,
> +						       unsigned long parent_rate)
> +{
> +	struct starfive_hdmi_phy *inno = to_starfive_hdmi_phy(hw);
> +
> +	/*
> +	 * Reading the PLL back requires the HDMI controller's system clock,
> +	 * which the sibling controller driver holds and this driver cannot
> +	 * request without creating a probe cycle with voutcrg. The clock
> +	 * framework walks every registered clock regardless of which drivers
> +	 * have bound, so fall back to the cached rate whenever this clock is
> +	 * not prepared; an unclocked access to the shared register window
> +	 * wedges the bus hard enough that the CPU stops taking NMIs.
> +	 */
> +	if (!clk_hw_is_prepared(hw))
> +		return inno->pixclock;
> +
> +	inno->pixclock = inno_hdmi_phy_pre_pll_recalc_rate(&inno->pre_pll,
> +							   parent_rate);
> +
> +	return inno->pixclock;
> +}
[ ... ]
> +static int starfive_hdmi_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +					  unsigned long parent_rate)
> +{
> +	struct starfive_hdmi_phy *inno = to_starfive_hdmi_phy(hw);
> +	const struct inno_hdmi_phy_pre_pll_config *cfg;
> +
> +	/*
> +	 * The JH7110 only drives 8bpc, so the TMDS clock always matches the
> +	 * pixel clock.
> +	 */
> +	cfg = inno_hdmi_phy_pre_pll_lookup(&inno->pre_pll, rate, rate);
> +	if (IS_ERR(cfg))
> +		return PTR_ERR(cfg);
> +
> +	dev_dbg(inno->dev, "%s rate %lu tmdsclk %lu\n",
> +		__func__, rate, cfg->tmdsclock);
> +
> +	if (inno->pixclock == rate && inno->tmdsclock == cfg->tmdsclock)
> +		return 0;
> +
> +	inno_update_bits(inno, STF_INNO_BIAS_CONTROL,
> +			 STF_INNO_BIAS_ENABLE, STF_INNO_BIAS_ENABLE);
> +	inno_write(inno, STF_INNO_RX_CONTROL, STF_INNO_RX_ENABLE);
> +
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, true);
> +	inno_hdmi_phy_pre_pll_configure(&inno->pre_pll, cfg);
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, false);
> +
> +	inno->pixclock = rate;
> +	inno->tmdsclock = cfg->tmdsclock;
> +
> +	return 0;
> +}

[Severity: Medium]
Should starfive_hdmi_phy_clk_set_rate() wait for the Pre-PLL to lock or keep
it powered down when the clock is unprepared?

In starfive_hdmi_phy_clk_set_rate(), the driver powers down the Pre-PLL,
configures it, and powers it back up via
inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, false), returning 0 without
calling inno_hdmi_phy_pre_pll_wait_locked().

If clk_set_rate() is called while hdmi_pclk is already prepared,
starfive_hdmi_phy_clk_prepare() is not called again because prepare_count is
already non-zero, so nothing waits for the Pre-PLL to lock before
starfive_hdmi_phy_power_on() configures and enables the Post-PLL (which uses
the Pre-PLL TMDS clock as its reference).

Conversely, if clk_set_rate() is called while hdmi_pclk is unprepared,
starfive_hdmi_phy_clk_set_rate() leaves the Pre-PLL, STF_INNO_BIAS_ENABLE,
and STF_INNO_RX_ENABLE powered on in hardware while prepare_count is 0.

> +
> +static const struct clk_ops starfive_hdmi_phy_clk_ops = {
> +	.prepare = starfive_hdmi_phy_clk_prepare,
> +	.unprepare = starfive_hdmi_phy_clk_unprepare,
> +	.recalc_rate = starfive_hdmi_phy_clk_recalc_rate,
> +	.determine_rate = starfive_hdmi_phy_clk_determine_rate,
> +	.set_rate = starfive_hdmi_phy_clk_set_rate,
> +};
> +
> +static void starfive_hdmi_phy_power_down(struct starfive_hdmi_phy *inno)
> +{
> +	inno_write(inno, STF_INNO_TMDS_CONTROL, 0x00);
> +	inno_write(inno, STF_INNO_SERIALIER_CONTROL, 0x00);
> +	inno_write(inno, STF_INNO_LDO_CONTROL, 0x00);
> +	inno_update_bits(inno, STF_INNO_BIAS_CONTROL,
> +			 STF_INNO_BIAS_ENABLE, 0x00);
> +	inno_write(inno, STF_INNO_RX_CONTROL, 0x00);
> +
> +	/* Power down Post-PLL */
> +	inno_update_bits(inno, STF_INNO_POST_PLL_DIV_1,
> +			 STF_INNO_POST_PLL_POWER_DOWN,
> +			 STF_INNO_POST_PLL_POWER_DOWN);
> +
> +	inno->tmdsclock = 0;
> +	inno->pixclock = 0;
> +}

[Severity: High]
What happens when HDMI output is disabled and then re-enabled at the same
pixel clock rate?

When the display is disabled:
1. starfive_hdmi_phy_power_off() -> starfive_hdmi_phy_power_down() clears
   STF_INNO_BIAS_ENABLE and STF_INNO_RX_CONTROL to 0, and sets
   inno->tmdsclock = 0 and inno->pixclock = 0.
2. clk_disable_unprepare() -> starfive_hdmi_phy_clk_unprepare() powers down
   the Pre-PLL and sets inno->pixclock = 0.

However, clk_core_unprepare() in the clock framework does not clear
core->rate, so the clock framework still caches the previous rate R.

When the display is re-enabled at the same rate R:
1. clk_set_rate(phyclk, R) sees rate == clk_core_get_rate_nolock(core) in
   clk_core_set_rate_nolock() and returns 0 early without calling
   starfive_hdmi_phy_clk_set_rate().
2. clk_prepare_enable(phyclk) calls starfive_hdmi_phy_clk_prepare() before
   phy_power_on(), powering up the Pre-PLL and polling
   inno_hdmi_phy_pre_pll_wait_locked() while STF_INNO_BIAS_ENABLE and
   STF_INNO_RX_ENABLE are still disabled at 0 (and without reprogramming the
   Pre-PLL if the shared register block was reset).

Additionally, if a clock tree walk invokes starfive_hdmi_phy_clk_recalc_rate()
while phyclk is unprepared after inno->pixclock was zeroed, it returns 0 and
overwrites core->rate with 0. If the display is then re-enabled without a new
clk_set_rate() call, clk_get_rate(inno->phyclk) in
starfive_hdmi_phy_power_on() returns 0 and fails
inno_hdmi_phy_pre_pll_lookup() with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-jh7110-inno-hdmi-phy-v6-0-f3379684d97e@samsung.com?part=4

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