On Tue, Feb 04, 2025 at 10:21:25AM +0100, Krzysztof Kozlowski wrote:
> On 03/02/2025 18:41, Dmitry Baryshkov wrote:
> > On Mon, Feb 03, 2025 at 06:29:19PM +0100, Krzysztof Kozlowski wrote:
> >> PHY_CMN_CLK_CFG1 register is updated by the PHY driver and by a mux
> >> clock from Common Clock Framework:
> >> devm_clk_hw_register_mux_parent_hws(). There could be a path leading to
> >> concurrent and conflicting updates between PHY driver and clock
> >> framework, e.g. changing the mux and enabling PLL clocks.
> >>
> >> Add dedicated spinlock to be sure all PHY_CMN_CLK_CFG1 updates are
> >> synchronized.
> >>
> >> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >>
> >> ---
> >>
> >> Changes in v2:
> >> 1. Store BIT(4) and BIT(5) in local var in dsi_pll_enable_global_clk()
> >> ---
> >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 35 +++++++++++++++++++------------
> >> 1 file changed, 22 insertions(+), 13 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 c164f845653816291ad96c863257f75462ef58e7..e26f53f7cde8f0f6419a633f5d39784dc2e5bb98 100644
> >> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >> @@ -83,6 +83,9 @@ struct dsi_pll_7nm {
> >> /* protects REG_DSI_7nm_PHY_CMN_CLK_CFG0 register */
> >> spinlock_t postdiv_lock;
> >>
> >> + /* protects REG_DSI_7nm_PHY_CMN_CLK_CFG1 register */
> >> + spinlock_t pclk_mux_lock;
> >> +
> >> struct pll_7nm_cached_state cached_state;
> >>
> >> struct dsi_pll_7nm *slave;
> >> @@ -381,22 +384,32 @@ static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val)
> >> spin_unlock_irqrestore(&pll->postdiv_lock, flags);
> >> }
> >>
> >> -static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >> +static void dsi_pll_cmn_clk_cfg1_update(struct dsi_pll_7nm *pll, u32 mask,
> >> + u32 val)
> >> {
> >> + unsigned long flags;
> >> u32 data;
> >>
> >> + spin_lock_irqsave(&pll->pclk_mux_lock, flags);
> >> data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >> - writel(data & ~BIT(5), pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >> + data &= ~mask;
> >> + data |= val & mask;
> >> +
> >> + writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >> + spin_unlock_irqrestore(&pll->pclk_mux_lock, flags);
> >> +}
> >> +
> >> +static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >> +{
> >> + dsi_pll_cmn_clk_cfg1_update(pll, BIT(5), 0);
> >> }
> >>
> >> static void dsi_pll_enable_global_clk(struct dsi_pll_7nm *pll)
> >> {
> >> - u32 data;
> >> + u32 cfg_1 = BIT(5) | BIT(4);
> >
> > Please define these two bits too.
>
> Why? They were not defined before. This only moving existing code.
Previously it was just a bit magic. Currently you are adding them as
masks. I want to know if BIT(4) and BIT(5) are parts of the same
bitfield (2 bits wide) or if they define two different bits.
--
With best wishes
Dmitry
