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.

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();
 
-- 
2.48.1

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

On 24/10/2025 14:43, Neil Armstrong wrote:
> 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.


If it is ==0, the check you removed would not be hit and enable would
work. I don't quite get the analysis.

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

This should not be removed.

> -	pll->pll_enable_cnt = 1;

So you basically remoevd pll_enable_cnt everywhere and reverted entirely
my commit. How is this patch different than revert?

Best regards,
Krzysztof

On 10/24/25 16:34, Krzysztof Kozlowski wrote:
> On 24/10/2025 14:43, Neil Armstrong wrote:
>> 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.
> 
> 
> If it is ==0, the check you removed would not be hit and enable would
> work. I don't quite get the analysis.
> 
>>
>> 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);
> 
> This should not be removed.
> 
>> -	pll->pll_enable_cnt = 1;
> 
> So you basically remoevd pll_enable_cnt everywhere and reverted entirely
> my commit. How is this patch different than revert?

No I did not, I kept the dsi_pll_disable_pll_bias() refcounting and call from
all the clock ops, which is basically needed here to never access PLL without
PLL_SHUTDOWNB and DIGTOP_PWRDN_B being asserted.

I only removed the pll_enable_cnt from dsi_7nm_phy_enable/disable because the
PHY code is designed to allow setting the PLL rate while the PHY is disabled.
And the bonded DSI hits this use case by setting the DSI0 PHY PLL rate while
configuring the PLL1 PHY.

So I wonder why it was added in the beginning because since you call dsi_pll_disable_pll_bias()
in each clk op, the Hardware Programming Guide for DSI PHY is satisfied ?

The commit message doesn't say anything related to dsi_7nm_phy_enable/disable.

Neil

> 
> Best regards,
> Krzysztof

On 24/10/2025 16:48, Neil Armstrong wrote:
> On 10/24/25 16:34, Krzysztof Kozlowski wrote:
>> On 24/10/2025 14:43, Neil Armstrong wrote:
>>> 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.
>>
>>
>> If it is ==0, the check you removed would not be hit and enable would
>> work. I don't quite get the analysis.
>>
>>>
>>> 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);
>>
>> This should not be removed.
>>
>>> -	pll->pll_enable_cnt = 1;
>>
>> So you basically remoevd pll_enable_cnt everywhere and reverted entirely
>> my commit. How is this patch different than revert?
> 
> No I did not, I kept the dsi_pll_disable_pll_bias() refcounting and call from

Indeed, I see now.

> all the clock ops, which is basically needed here to never access PLL without
> PLL_SHUTDOWNB and DIGTOP_PWRDN_B being asserted.
> 
> I only removed the pll_enable_cnt from dsi_7nm_phy_enable/disable because the
> PHY code is designed to allow setting the PLL rate while the PHY is disabled.
> And the bonded DSI hits this use case by setting the DSI0 PHY PLL rate while
> configuring the PLL1 PHY.

OK, I understand now.

> 
> So I wonder why it was added in the beginning because since you call dsi_pll_disable_pll_bias()
> in each clk op, the Hardware Programming Guide for DSI PHY is satisfied ?

Don't remember anymore.

> 
> The commit message doesn't say anything related to dsi_7nm_phy_enable/disable.
> 
With your patch it works fine and does not produce clock warnings, so
seems fine. If you send proper fix, then:


Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

On Mon, Sep 08, 2025 at 11:49:51AM +0200, 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.
> 
> 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(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry