On Tue, Jun 10, 2025 at 04:05:46PM +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 <[email protected]>
>
> ---
>
> 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
> 7ea608f620fe17ae4ccc41ba9e52ba043af0c022..82baec385b3224c8b3e36742230d806c4fe68cbb
> 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
> 4df865dfe6fe111297f0d08199c515d3b5e5a0b6..22f80e99a7a7514085ef80ced1cf78876bcc6ba3
> 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -88,6 +88,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;
> @@ -101,6 +108,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;
> @@ -316,6 +326,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);
>
> @@ -333,6 +344,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();
>
> @@ -361,24 +373,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);
What is this ndelay protecting? Is is to let the hardware to wind down
correctly? I'm worried about dsi_pll_disable_pll_bias() beng followed up
by dsi_pll_enable_pll_bias() in another thread, which would mean that
corresponding writes to the REG_DSI_7nm_PHY_CMN_CTRL_0 can come up
without any delay between them.
> }
>
> 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);
> }
>
> @@ -519,6 +554,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;
>
> @@ -543,6 +579,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;
> }
>
> @@ -578,6 +616,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;
> @@ -589,6 +628,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);
> @@ -807,8 +847,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) {
> @@ -891,8 +933,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;
> @@ -1000,10 +1044,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);
> @@ -1115,7 +1162,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("");
> @@ -1141,8 +1190,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.45.2
>
--
With best wishes
Dmitry
