| Message ID | 20250203-drm-msm-phy-pll-cfg-reg-v2-1-862b136c5d22@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dsi/phy: Improvements around concurrent PHY_CMN_CLK_CFG[01] | expand |
On Mon, Feb 03, 2025 at 06:29:18PM +0100, Krzysztof Kozlowski wrote: > PHY_CMN_CLK_CFG0 register is updated by the PHY driver and by two > divider clocks from Common Clock Framework: > devm_clk_hw_register_divider_parent_hw(). Concurrent access by the > clocks side is protected with spinlock, however driver's side in > restoring state is not. Restoring state is called from > msm_dsi_phy_enable(), so there could be a path leading to concurrent and > conflicting updates with clock framework. > > Add missing lock usage on the PHY driver side, encapsulated in its own > function so the code will be still readable. > > Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > 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 031446c87daec0af3f81df324158311f5a80014e..c164f845653816291ad96c863257f75462ef58e7 100644 > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > @@ -372,6 +372,15 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll) > ndelay(250); > } > > +static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&pll->postdiv_lock, flags); > + writel(val, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); > + spin_unlock_irqrestore(&pll->postdiv_lock, flags); > +} > + > static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll) > { > u32 data; > @@ -574,8 +583,8 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy) > val |= cached->pll_out_div; > writel(val, pll_7nm->phy->pll_base + REG_DSI_7nm_PHY_PLL_PLL_OUTDIV_RATE); > > - writel(cached->bit_clk_div | (cached->pix_clk_div << 4), > - phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); > + dsi_pll_cmn_clk_cfg0_write(pll_7nm, > + cached->bit_clk_div | (cached->pix_clk_div << 4)); Ideally this would be FIELD_PREP or a special function generated for you in the header. > > val = readl(phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > val &= ~0x3; > > -- > 2.43.0 >
On 03/02/2025 18:42, Dmitry Baryshkov wrote: > On Mon, Feb 03, 2025 at 06:29:18PM +0100, Krzysztof Kozlowski wrote: >> PHY_CMN_CLK_CFG0 register is updated by the PHY driver and by two >> divider clocks from Common Clock Framework: >> devm_clk_hw_register_divider_parent_hw(). Concurrent access by the >> clocks side is protected with spinlock, however driver's side in >> restoring state is not. Restoring state is called from >> msm_dsi_phy_enable(), so there could be a path leading to concurrent and >> conflicting updates with clock framework. >> >> Add missing lock usage on the PHY driver side, encapsulated in its own >> function so the code will be still readable. >> >> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL") >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> 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 031446c87daec0af3f81df324158311f5a80014e..c164f845653816291ad96c863257f75462ef58e7 100644 >> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c >> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c >> @@ -372,6 +372,15 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll) >> ndelay(250); >> } >> >> +static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&pll->postdiv_lock, flags); >> + writel(val, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); >> + spin_unlock_irqrestore(&pll->postdiv_lock, flags); >> +} >> + >> static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll) >> { >> u32 data; >> @@ -574,8 +583,8 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy) >> val |= cached->pll_out_div; >> writel(val, pll_7nm->phy->pll_base + REG_DSI_7nm_PHY_PLL_PLL_OUTDIV_RATE); >> >> - writel(cached->bit_clk_div | (cached->pix_clk_div << 4), >> - phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); >> + dsi_pll_cmn_clk_cfg0_write(pll_7nm, >> + cached->bit_clk_div | (cached->pix_clk_div << 4)); > > Ideally this would be FIELD_PREP or a special function generated for you > in the header. There is no header. That's patch #1 and I do not see how changing this to FIELDPREP is anyhow related to the actual problem being solved here. Best regards, Krzysztof
On Tue, Feb 04, 2025 at 10:20:51AM +0100, Krzysztof Kozlowski wrote: > On 03/02/2025 18:42, Dmitry Baryshkov wrote: > > On Mon, Feb 03, 2025 at 06:29:18PM +0100, Krzysztof Kozlowski wrote: > >> PHY_CMN_CLK_CFG0 register is updated by the PHY driver and by two > >> divider clocks from Common Clock Framework: > >> devm_clk_hw_register_divider_parent_hw(). Concurrent access by the > >> clocks side is protected with spinlock, however driver's side in > >> restoring state is not. Restoring state is called from > >> msm_dsi_phy_enable(), so there could be a path leading to concurrent and > >> conflicting updates with clock framework. > >> > >> Add missing lock usage on the PHY driver side, encapsulated in its own > >> function so the code will be still readable. > >> > >> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL") > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> --- > >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> 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 031446c87daec0af3f81df324158311f5a80014e..c164f845653816291ad96c863257f75462ef58e7 100644 > >> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > >> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > >> @@ -372,6 +372,15 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll) > >> ndelay(250); > >> } > >> > >> +static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val) > >> +{ > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&pll->postdiv_lock, flags); > >> + writel(val, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); > >> + spin_unlock_irqrestore(&pll->postdiv_lock, flags); > >> +} > >> + > >> static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll) > >> { > >> u32 data; > >> @@ -574,8 +583,8 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy) > >> val |= cached->pll_out_div; > >> writel(val, pll_7nm->phy->pll_base + REG_DSI_7nm_PHY_PLL_PLL_OUTDIV_RATE); > >> > >> - writel(cached->bit_clk_div | (cached->pix_clk_div << 4), > >> - phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); > >> + dsi_pll_cmn_clk_cfg0_write(pll_7nm, > >> + cached->bit_clk_div | (cached->pix_clk_div << 4)); > > > > Ideally this would be FIELD_PREP or a special function generated for you > > in the header. > > There is no header. That's patch #1 and I do not see how changing this > to FIELDPREP is anyhow related to the actual problem being solved here. Ack, this just moves the code. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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 031446c87daec0af3f81df324158311f5a80014e..c164f845653816291ad96c863257f75462ef58e7 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c @@ -372,6 +372,15 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm *pll) ndelay(250); } +static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val) +{ + unsigned long flags; + + spin_lock_irqsave(&pll->postdiv_lock, flags); + writel(val, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); + spin_unlock_irqrestore(&pll->postdiv_lock, flags); +} + static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll) { u32 data; @@ -574,8 +583,8 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy) val |= cached->pll_out_div; writel(val, pll_7nm->phy->pll_base + REG_DSI_7nm_PHY_PLL_PLL_OUTDIV_RATE); - writel(cached->bit_clk_div | (cached->pix_clk_div << 4), - phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); + dsi_pll_cmn_clk_cfg0_write(pll_7nm, + cached->bit_clk_div | (cached->pix_clk_div << 4)); val = readl(phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); val &= ~0x3;
PHY_CMN_CLK_CFG0 register is updated by the PHY driver and by two divider clocks from Common Clock Framework: devm_clk_hw_register_divider_parent_hw(). Concurrent access by the clocks side is protected with spinlock, however driver's side in restoring state is not. Restoring state is called from msm_dsi_phy_enable(), so there could be a path leading to concurrent and conflicting updates with clock framework. Add missing lock usage on the PHY driver side, encapsulated in its own function so the code will be still readable. Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL") Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)