| Message ID | 20250422151314.173561-1-loic.poulain@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dsi: Fix 14nm DSI PHY PLL Lock issue | expand |
On Tue, Apr 22, 2025 at 05:13:14PM +0200, Loic Poulain wrote: > To configure and enable the DSI PHY PLL clocks, the MDSS AHB clock must > be active for MMIO operations. This is typically handled during the DSI > PHY enabling process. However, since these PLL clocks are registered as > proper entities within the clock framework, they can be enabled apart > from the DSI PHY, leading to enabling failures (and subsequent warnings): > > ``` > msm_dsi_phy 5e94400.phy: [drm:dsi_pll_14nm_vco_prepare] *ERROR* DSI PLL lock failed > ------------[ cut here ]------------ > dsi0pllbyte already disabled > WARNING: CPU: 3 PID: 1 at drivers/clk/clk.c:1194 clk_core_disable+0xa4/0xac > CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: > Tainted: [W]=WARN > Hardware name: Qualcomm Technologies, Inc. Robotics RB1 (DT) > pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [...] > ``` > > This issue is particularly prevalent at boot time during the disabling of > unused clock (clk_disable_unused()) which includes enabling the parent > clock(s) when CLK_OPS_PARENT_ENABLE flag is set. > > This problem is often mitigated via clk_ignore_unused kernel param... > > To fix this issue properly, we take a clk reference from the clk_prepare > callback and release it in clk_unprepare. I think this should be handled by using corresponding CCF flags rather than manually handling the clocks. I don't think that CCF is really reentrant here (and I'm surprised that you didn't hit a deadlock or a lockdep splat). Another option might be to use pm-runtime and/or pm_clk to manage AHB clock, making sure that it is enabled if the DSI PHY is used. > > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com> > --- > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > index 3a1c8ece6657..7a547ae8e749 100644 > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > @@ -543,6 +543,8 @@ static int dsi_pll_14nm_vco_prepare(struct clk_hw *hw) > if (unlikely(pll_14nm->phy->pll_on)) > return 0; > > + clk_prepare_enable(pll_14nm->phy->ahb_clk); > + > if (dsi_pll_14nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0) > dsi_pll_14nm_vco_set_rate(hw, pll_14nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE); > > @@ -554,6 +556,7 @@ static int dsi_pll_14nm_vco_prepare(struct clk_hw *hw) > > if (unlikely(!locked)) { > DRM_DEV_ERROR(&pll_14nm->phy->pdev->dev, "DSI PLL lock failed\n"); > + clk_disable_unprepare(pll_14nm->phy->ahb_clk); > return -EINVAL; > } > > @@ -576,6 +579,8 @@ static void dsi_pll_14nm_vco_unprepare(struct clk_hw *hw) > writel(0, cmn_base + REG_DSI_14nm_PHY_CMN_PLL_CNTRL); > > pll_14nm->phy->pll_on = false; > + > + clk_disable_unprepare(pll_14nm->phy->ahb_clk); > } > > static long dsi_pll_14nm_clk_round_rate(struct clk_hw *hw, > -- > 2.34.1 >
Hi Dmitry, On Wed, Apr 23, 2025 at 3:15 PM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote: > > On Tue, Apr 22, 2025 at 05:13:14PM +0200, Loic Poulain wrote: > > To configure and enable the DSI PHY PLL clocks, the MDSS AHB clock must > > be active for MMIO operations. This is typically handled during the DSI > > PHY enabling process. However, since these PLL clocks are registered as > > proper entities within the clock framework, they can be enabled apart > > from the DSI PHY, leading to enabling failures (and subsequent warnings): > > > > ``` > > msm_dsi_phy 5e94400.phy: [drm:dsi_pll_14nm_vco_prepare] *ERROR* DSI PLL lock failed > > ------------[ cut here ]------------ > > dsi0pllbyte already disabled > > WARNING: CPU: 3 PID: 1 at drivers/clk/clk.c:1194 clk_core_disable+0xa4/0xac > > CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: > > Tainted: [W]=WARN > > Hardware name: Qualcomm Technologies, Inc. Robotics RB1 (DT) > > pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > [...] > > ``` > > > > This issue is particularly prevalent at boot time during the disabling of > > unused clock (clk_disable_unused()) which includes enabling the parent > > clock(s) when CLK_OPS_PARENT_ENABLE flag is set. > > > > This problem is often mitigated via clk_ignore_unused kernel param... > > > > To fix this issue properly, we take a clk reference from the clk_prepare > > callback and release it in clk_unprepare. > > I think this should be handled by using corresponding CCF flags rather > than manually handling the clocks. I don't think that CCF is really > reentrant here (and I'm surprised that you didn't hit a deadlock or a > lockdep splat). > > Another option might be to use pm-runtime and/or pm_clk to manage AHB > clock, making sure that it is enabled if the DSI PHY is used. Yes, good point, I think that can be indeed generically fixed via regular runtime-pm, as DSI PHY is the parent of the PLL. Will submit a new version. Regards, Loic
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c index 3a1c8ece6657..7a547ae8e749 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c @@ -543,6 +543,8 @@ static int dsi_pll_14nm_vco_prepare(struct clk_hw *hw) if (unlikely(pll_14nm->phy->pll_on)) return 0; + clk_prepare_enable(pll_14nm->phy->ahb_clk); + if (dsi_pll_14nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0) dsi_pll_14nm_vco_set_rate(hw, pll_14nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE); @@ -554,6 +556,7 @@ static int dsi_pll_14nm_vco_prepare(struct clk_hw *hw) if (unlikely(!locked)) { DRM_DEV_ERROR(&pll_14nm->phy->pdev->dev, "DSI PLL lock failed\n"); + clk_disable_unprepare(pll_14nm->phy->ahb_clk); return -EINVAL; } @@ -576,6 +579,8 @@ static void dsi_pll_14nm_vco_unprepare(struct clk_hw *hw) writel(0, cmn_base + REG_DSI_14nm_PHY_CMN_PLL_CNTRL); pll_14nm->phy->pll_on = false; + + clk_disable_unprepare(pll_14nm->phy->ahb_clk); } static long dsi_pll_14nm_clk_round_rate(struct clk_hw *hw,
To configure and enable the DSI PHY PLL clocks, the MDSS AHB clock must be active for MMIO operations. This is typically handled during the DSI PHY enabling process. However, since these PLL clocks are registered as proper entities within the clock framework, they can be enabled apart from the DSI PHY, leading to enabling failures (and subsequent warnings): ``` msm_dsi_phy 5e94400.phy: [drm:dsi_pll_14nm_vco_prepare] *ERROR* DSI PLL lock failed ------------[ cut here ]------------ dsi0pllbyte already disabled WARNING: CPU: 3 PID: 1 at drivers/clk/clk.c:1194 clk_core_disable+0xa4/0xac CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: Tainted: [W]=WARN Hardware name: Qualcomm Technologies, Inc. Robotics RB1 (DT) pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [...] ``` This issue is particularly prevalent at boot time during the disabling of unused clock (clk_disable_unused()) which includes enabling the parent clock(s) when CLK_OPS_PARENT_ENABLE flag is set. This problem is often mitigated via clk_ignore_unused kernel param... To fix this issue properly, we take a clk reference from the clk_prepare callback and release it in clk_unprepare. Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com> --- drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 5 +++++ 1 file changed, 5 insertions(+)