Re: [PATCH v2 1/4] drm/msm/dsi/phy: Protect PHY_CMN_CLK_CFG0 updated from driver side

[Dmitry Baryshkov]

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@xxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx>


-- 
With best wishes
Dmitry