Re: [PATCH v7] drm/msm/dsi/phy: Fix reading zero as PLL rates when unprepared

[Neil Armstrong <neil.armstrong@linaro.org>]

Hi,

On 9/8/25 11:49, Krzysztof Kozlowski wrote:
> Hardware Programming Guide for DSI PHY says that PLL_SHUTDOWNB and
> DIGTOP_PWRDN_B have to be asserted for any PLL register access.
> Whenever dsi_pll_7nm_vco_recalc_rate() or dsi_pll_7nm_vco_set_rate()
> were called on unprepared PLL, driver read values of zero leading to all
> sort of further troubles, like failing to set pixel and byte clock
> rates.
> 
> Asserting the PLL shutdown bit is done by dsi_pll_enable_pll_bias() (and
> corresponding dsi_pll_disable_pll_bias()) which are called through the
> code, including from PLL .prepare() and .unprepare() callbacks.
> 
> The .set_rate() and .recalc_rate() can be called almost anytime from
> external users including times when PLL is or is not prepared, thus
> driver should not interfere with the prepare status.
> 
> Implement simple reference counting for the PLL bias, so
> set_rate/recalc_rate will not change the status of prepared PLL.
> 
> Issue of reading 0 in .recalc_rate() did not show up on existing
> devices, but only after re-ordering the code for SM8750.

It happens this breaks the bonded DSI use-case, mainly because both PHYs
uses the same PLL, and trying to enable the DSI0 PHY PLL from the DSI1
PHY fails because the DSI0 PHY enable_count == 0.

Reverting part the the patch makes the bonded work again:
===================><===============================
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 32f06edd21a9..24811c52d34c 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -426,11 +426,8 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll)
  	u32 data;

  	spin_lock_irqsave(&pll->pll_enable_lock, flags);
-	if (pll->pll_enable_cnt++) {
-		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
-		WARN_ON(pll->pll_enable_cnt == INT_MAX);
-		return;
-	}
+	pll->pll_enable_cnt++;
+	WARN_ON(pll->pll_enable_cnt == INT_MAX);

  	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
  	data |= DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
@@ -965,10 +962,8 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
  	u32 const delay_us = 5;
  	u32 const timeout_us = 1000;
  	struct msm_dsi_dphy_timing *timing = &phy->timing;
-	struct dsi_pll_7nm *pll = phy->pll_data;
  	void __iomem *base = phy->base;
  	bool less_than_1500_mhz;
-	unsigned long flags;
  	u32 vreg_ctrl_0, vreg_ctrl_1, lane_ctrl0;
  	u32 glbl_pemph_ctrl_0;
  	u32 glbl_str_swi_cal_sel_ctrl, glbl_hstx_str_ctrl_0;
@@ -1090,13 +1085,10 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
  		glbl_rescode_bot_ctrl = 0x3c;
  	}

-	spin_lock_irqsave(&pll->pll_enable_lock, flags);
-	pll->pll_enable_cnt = 1;
  	/* de-assert digital and pll power down */
  	data = DSI_7nm_PHY_CMN_CTRL_0_DIGTOP_PWRDN_B |
  	       DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
  	writel(data, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
-	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);

  	/* Assert PLL core reset */
  	writel(0x00, base + REG_DSI_7nm_PHY_CMN_PLL_CNTRL);
@@ -1209,9 +1201,7 @@ static bool dsi_7nm_set_continuous_clock(struct msm_dsi_phy *phy, bool enable)

  static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
  {
-	struct dsi_pll_7nm *pll = phy->pll_data;
  	void __iomem *base = phy->base;
-	unsigned long flags;
  	u32 data;

  	DBG("");
@@ -1238,11 +1228,8 @@ static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
  	writel(data, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
  	writel(0, base + REG_DSI_7nm_PHY_CMN_LANE_CTRL0);

-	spin_lock_irqsave(&pll->pll_enable_lock, flags);
-	pll->pll_enable_cnt = 0;
  	/* Turn off all PHY blocks */
  	writel(0x00, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
-	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);

  	/* make sure phy is turned off */
  	wmb();
===================><===============================

This removed the phy_enable/_disable from the equation because the DSI PHY
code already supports enabling the PLL when the PHY is disabled.

Could you test if it still works fine om SM8750 ?

Neil

> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Continuing changelog from "drm/msm: Add support for SM8750" where this
> was part of.
> 
> Changes in v7:
> - Rebase
> - I did not remove ndelay(250) as discussed with Dmitry, because:
>    1. Indeed the HPG does not mention any delay needed, unlike PHY 10 nm.
>    2. However downstream source code for PHY 3+4+5 nm has exactly these
>       delays. This could be copy-paste or could be intentional workaround
>       for some issue about which I have no clue. Timings are tricky and
>       I don't think I should be introducing changes without actually
>       knowing them.
> - Add Rb tags
> - Link to v6: https://lore.kernel.org/r/20250610-b4-sm8750-display-v6-0-ee633e3ddbff@linaro.org
> 
> Changes in v6:
> 1. Print error on pll bias enable/disable imbalance refcnt
> 
> Changes in v5:
> 1. New patch
> ---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.h     |  1 +
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 53 +++++++++++++++++++++++
>   2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index 3cbf08231492..e391505fdaf0 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -109,6 +109,7 @@ struct msm_dsi_phy {
>   	struct msm_dsi_dphy_timing timing;
>   	const struct msm_dsi_phy_cfg *cfg;
>   	void *tuning_cfg;
> +	void *pll_data;
>   
>   	enum msm_dsi_phy_usecase usecase;
>   	bool regulator_ldo_mode;
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 2c2bbda46c78..32f06edd21a9 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -90,6 +90,13 @@ struct dsi_pll_7nm {
>   	/* protects REG_DSI_7nm_PHY_CMN_CLK_CFG1 register */
>   	spinlock_t pclk_mux_lock;
>   
> +	/*
> +	 * protects REG_DSI_7nm_PHY_CMN_CTRL_0 register and pll_enable_cnt
> +	 * member
> +	 */
> +	spinlock_t pll_enable_lock;
> +	int pll_enable_cnt;
> +
>   	struct pll_7nm_cached_state cached_state;
>   
>   	struct dsi_pll_7nm *slave;
> @@ -103,6 +110,9 @@ struct dsi_pll_7nm {
>    */
>   static struct dsi_pll_7nm *pll_7nm_list[DSI_MAX];
>   
> +static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll);
> +static void dsi_pll_disable_pll_bias(struct dsi_pll_7nm *pll);
> +
>   static void dsi_pll_setup_config(struct dsi_pll_config *config)
>   {
>   	config->ssc_freq = 31500;
> @@ -340,6 +350,7 @@ static int dsi_pll_7nm_vco_set_rate(struct clk_hw *hw, unsigned long rate,
>   	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
>   	struct dsi_pll_config config;
>   
> +	dsi_pll_enable_pll_bias(pll_7nm);
>   	DBG("DSI PLL%d rate=%lu, parent's=%lu", pll_7nm->phy->id, rate,
>   	    parent_rate);
>   
> @@ -357,6 +368,7 @@ static int dsi_pll_7nm_vco_set_rate(struct clk_hw *hw, unsigned long rate,
>   
>   	dsi_pll_ssc_commit(pll_7nm, &config);
>   
> +	dsi_pll_disable_pll_bias(pll_7nm);
>   	/* flush, ensure all register writes are done*/
>   	wmb();
>   
> @@ -385,24 +397,47 @@ static int dsi_pll_7nm_lock_status(struct dsi_pll_7nm *pll)
>   
>   static void dsi_pll_disable_pll_bias(struct dsi_pll_7nm *pll)
>   {
> +	unsigned long flags;
>   	u32 data;
>   
> +	spin_lock_irqsave(&pll->pll_enable_lock, flags);
> +	--pll->pll_enable_cnt;
> +	if (pll->pll_enable_cnt < 0) {
> +		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
> +		DRM_DEV_ERROR_RATELIMITED(&pll->phy->pdev->dev,
> +					  "bug: imbalance in disabling PLL bias\n");
> +		return;
> +	} else if (pll->pll_enable_cnt > 0) {
> +		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
> +		return;
> +	} /* else: == 0 */
> +
>   	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>   	data &= ~DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
>   	writel(0, pll->phy->pll_base + REG_DSI_7nm_PHY_PLL_SYSTEM_MUXES);
>   	writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
> +	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>   	ndelay(250);
>   }
>   
>   static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll)
>   {
> +	unsigned long flags;
>   	u32 data;
>   
> +	spin_lock_irqsave(&pll->pll_enable_lock, flags);
> +	if (pll->pll_enable_cnt++) {
> +		spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
> +		WARN_ON(pll->pll_enable_cnt == INT_MAX);
> +		return;
> +	}
> +
>   	data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>   	data |= DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
>   	writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>   
>   	writel(0xc0, pll->phy->pll_base + REG_DSI_7nm_PHY_PLL_SYSTEM_MUXES);
> +	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>   	ndelay(250);
>   }
>   
> @@ -543,6 +578,7 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
>   	u32 dec;
>   	u64 pll_freq, tmp64;
>   
> +	dsi_pll_enable_pll_bias(pll_7nm);
>   	dec = readl(base + REG_DSI_7nm_PHY_PLL_DECIMAL_DIV_START_1);
>   	dec &= 0xff;
>   
> @@ -567,6 +603,8 @@ static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
>   	DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>   	    pll_7nm->phy->id, (unsigned long)vco_rate, dec, frac);
>   
> +	dsi_pll_disable_pll_bias(pll_7nm);
> +
>   	return (unsigned long)vco_rate;
>   }
>   
> @@ -600,6 +638,7 @@ static void dsi_7nm_pll_save_state(struct msm_dsi_phy *phy)
>   	void __iomem *phy_base = pll_7nm->phy->base;
>   	u32 cmn_clk_cfg0, cmn_clk_cfg1;
>   
> +	dsi_pll_enable_pll_bias(pll_7nm);
>   	cached->pll_out_div = readl(pll_7nm->phy->pll_base +
>   			REG_DSI_7nm_PHY_PLL_PLL_OUTDIV_RATE);
>   	cached->pll_out_div &= 0x3;
> @@ -611,6 +650,7 @@ static void dsi_7nm_pll_save_state(struct msm_dsi_phy *phy)
>   	cmn_clk_cfg1 = readl(phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
>   	cached->pll_mux = FIELD_GET(DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL__MASK, cmn_clk_cfg1);
>   
> +	dsi_pll_disable_pll_bias(pll_7nm);
>   	DBG("DSI PLL%d outdiv %x bit_clk_div %x pix_clk_div %x pll_mux %x",
>   	    pll_7nm->phy->id, cached->pll_out_div, cached->bit_clk_div,
>   	    cached->pix_clk_div, cached->pll_mux);
> @@ -833,8 +873,10 @@ static int dsi_pll_7nm_init(struct msm_dsi_phy *phy)
>   
>   	spin_lock_init(&pll_7nm->postdiv_lock);
>   	spin_lock_init(&pll_7nm->pclk_mux_lock);
> +	spin_lock_init(&pll_7nm->pll_enable_lock);
>   
>   	pll_7nm->phy = phy;
> +	phy->pll_data = pll_7nm;
>   
>   	ret = pll_7nm_register(pll_7nm, phy->provided_clocks->hws);
>   	if (ret) {
> @@ -923,8 +965,10 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
>   	u32 const delay_us = 5;
>   	u32 const timeout_us = 1000;
>   	struct msm_dsi_dphy_timing *timing = &phy->timing;
> +	struct dsi_pll_7nm *pll = phy->pll_data;
>   	void __iomem *base = phy->base;
>   	bool less_than_1500_mhz;
> +	unsigned long flags;
>   	u32 vreg_ctrl_0, vreg_ctrl_1, lane_ctrl0;
>   	u32 glbl_pemph_ctrl_0;
>   	u32 glbl_str_swi_cal_sel_ctrl, glbl_hstx_str_ctrl_0;
> @@ -1046,10 +1090,13 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
>   		glbl_rescode_bot_ctrl = 0x3c;
>   	}
>   
> +	spin_lock_irqsave(&pll->pll_enable_lock, flags);
> +	pll->pll_enable_cnt = 1;
>   	/* de-assert digital and pll power down */
>   	data = DSI_7nm_PHY_CMN_CTRL_0_DIGTOP_PWRDN_B |
>   	       DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB;
>   	writel(data, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
> +	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
>   
>   	/* Assert PLL core reset */
>   	writel(0x00, base + REG_DSI_7nm_PHY_CMN_PLL_CNTRL);
> @@ -1162,7 +1209,9 @@ static bool dsi_7nm_set_continuous_clock(struct msm_dsi_phy *phy, bool enable)
>   
>   static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
>   {
> +	struct dsi_pll_7nm *pll = phy->pll_data;
>   	void __iomem *base = phy->base;
> +	unsigned long flags;
>   	u32 data;
>   
>   	DBG("");
> @@ -1189,8 +1238,12 @@ static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
>   	writel(data, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
>   	writel(0, base + REG_DSI_7nm_PHY_CMN_LANE_CTRL0);
>   
> +	spin_lock_irqsave(&pll->pll_enable_lock, flags);
> +	pll->pll_enable_cnt = 0;
>   	/* Turn off all PHY blocks */
>   	writel(0x00, base + REG_DSI_7nm_PHY_CMN_CTRL_0);
> +	spin_unlock_irqrestore(&pll->pll_enable_lock, flags);
> +
>   	/* make sure phy is turned off */
>   	wmb();
>