On Fri, Jan 31, 2025 at 04:02:51PM +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.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 34 +++++++++++++++++++------------
> 1 file changed, 21 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..6c18b9c0e1903bbd0090aceef13ae8c6f2e080ce 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,31 @@ 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);
PLease add these bits to the corresponding XML file (here and later on)
> }
>
> static void dsi_pll_enable_global_clk(struct dsi_pll_7nm *pll)
> {
> - u32 data;
> -
> writel(0x04, pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_3);
>
> - data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> - writel(data | BIT(5) | BIT(4), pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> + dsi_pll_cmn_clk_cfg1_update(pll, BIT(5) | BIT(4), BIT(5) | BIT(4));
> }
>
> static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
> @@ -574,7 +586,6 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy)
> {
> struct dsi_pll_7nm *pll_7nm = to_pll_7nm(phy->vco_hw);
> struct pll_7nm_cached_state *cached = &pll_7nm->cached_state;
> - void __iomem *phy_base = pll_7nm->phy->base;
> u32 val;
> int ret;
>
> @@ -585,11 +596,7 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy)
>
> 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;
> - val |= cached->pll_mux;
> - writel(val, phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> + dsi_pll_cmn_clk_cfg1_update(pll_7nm, 0x3, cached->pll_mux);
>
> ret = dsi_pll_7nm_vco_set_rate(phy->vco_hw,
> pll_7nm->vco_current_rate,
> @@ -742,7 +749,7 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide
> pll_by_2_bit,
> }), 2, 0, pll_7nm->phy->base +
> REG_DSI_7nm_PHY_CMN_CLK_CFG1,
> - 0, 1, 0, NULL);
> + 0, 1, 0, &pll_7nm->pclk_mux_lock);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> goto fail;
> @@ -787,6 +794,7 @@ static int dsi_pll_7nm_init(struct msm_dsi_phy *phy)
> pll_7nm_list[phy->id] = pll_7nm;
>
> spin_lock_init(&pll_7nm->postdiv_lock);
> + spin_lock_init(&pll_7nm->pclk_mux_lock);
>
> pll_7nm->phy = phy;
>
>
> --
> 2.43.0
>
--
With best wishes
Dmitry
