| Message ID | 20250203-drm-msm-phy-pll-cfg-reg-v2-2-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: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@linaro.org> > > --- > > 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. > > 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, cfg_1, cfg_1); > } > > static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll) > @@ -574,7 +587,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 +597,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 +750,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 +795,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 >
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@linaro.org> >> >> --- >> >> 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. Best regards, Krzysztof
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@linaro.org> > >> > >> --- > >> > >> 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.
On 04/02/2025 15:26, Dmitry Baryshkov wrote: > 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@linaro.org> >>>> >>>> --- >>>> >>>> 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 No, previous code: writel(data | BIT(5) | BIT(4), pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); This is a mask and update in the same time, because: (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) is just redudant. I did not do any logical change, I did not add any mask or field. Everything was already there. > 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. While in general you are right, it does not matter for this fix. If this are separate bitfields - fix is correct. If this is one bitfield - fix is still correct. You could claim that if this was one bitfield, using 2xBIT() is not logical, but this was there already, so again my fix is only fixing and keeping entire logic or inconsistencies intact. Best regards, Krzysztof
On Tue, Feb 04, 2025 at 04:46:04PM +0100, Krzysztof Kozlowski wrote: > On 04/02/2025 15:26, Dmitry Baryshkov wrote: > > 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@linaro.org> > >>>> > >>>> --- > >>>> > >>>> 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 > > No, previous code: > > writel(data | BIT(5) | BIT(4), pll->phy->base + > REG_DSI_7nm_PHY_CMN_CLK_CFG1); > > This is a mask and update in the same time, because: > (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) > is just redudant. > > I did not do any logical change, I did not add any mask or field. > Everything was already there. Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now your code adds BIT(5) as a 'mask' parameter. Is it a correct mask for that field? That's why I'm asking you to define those - you have changed bitwrites to the masked bit writes. Masks should be defined. > > > > 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. > > While in general you are right, it does not matter for this fix. If this > are separate bitfields - fix is correct. If this is one bitfield - fix > is still correct. You could claim that if this was one bitfield, using > 2xBIT() is not logical, but this was there already, so again my fix is > only fixing and keeping entire logic or inconsistencies intact.
On 05/02/2025 03:51, Dmitry Baryshkov wrote: > On Tue, Feb 04, 2025 at 04:46:04PM +0100, Krzysztof Kozlowski wrote: >> On 04/02/2025 15:26, Dmitry Baryshkov wrote: >>> 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@linaro.org> >>>>>> >>>>>> --- >>>>>> >>>>>> 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 >> >> No, previous code: >> >> writel(data | BIT(5) | BIT(4), pll->phy->base + >> REG_DSI_7nm_PHY_CMN_CLK_CFG1); >> >> This is a mask and update in the same time, because: >> (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) >> is just redudant. >> >> I did not do any logical change, I did not add any mask or field. >> Everything was already there. > > Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now You did not address my comment. Previous code was: (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) Just for shorter syntax it was written different way: foo | BIT(5) | BIT(4) > your code adds BIT(5) as a 'mask' parameter. Is it a correct mask for No, my code does not add it. It was already there, look: foo | BIT(5) | BIT(4) ^^^^^^^ here > that field? That's why I'm asking you to define those - you have changed No, I did not change bitwrites. The code is 100% equivalent, both logically and assembly. You mistake maybe with some other part doing "writel(data & ~BIT(5)" in dsi_pll_disable_global_clk() but that's just poor diff. > bitwrites to the masked bit writes. Masks should be defined. > >> >> >>> 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. >> >> While in general you are right, it does not matter for this fix. If this >> are separate bitfields - fix is correct. If this is one bitfield - fix >> is still correct. You could claim that if this was one bitfield, using >> 2xBIT() is not logical, but this was there already, so again my fix is >> only fixing and keeping entire logic or inconsistencies intact. > Best regards, Krzysztof
Hi, On Wed, 5 Feb 2025 at 11:34, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/02/2025 03:51, Dmitry Baryshkov wrote: > > On Tue, Feb 04, 2025 at 04:46:04PM +0100, Krzysztof Kozlowski wrote: > >> On 04/02/2025 15:26, Dmitry Baryshkov wrote: > >>> 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@linaro.org> > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> 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 > >> > >> No, previous code: > >> > >> writel(data | BIT(5) | BIT(4), pll->phy->base + > >> REG_DSI_7nm_PHY_CMN_CLK_CFG1); > >> > >> This is a mask and update in the same time, because: > >> (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) > >> is just redudant. > >> > >> I did not do any logical change, I did not add any mask or field. > >> Everything was already there. > > > > Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now > > You did not address my comment. Previous code was: > > (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) > > Just for shorter syntax it was written different way: > > foo | BIT(5) | BIT(4) Previously it was a simple writel() with some bit magic. Now you call dsi_pll_cmn_clk_cfg1_update() passing the register bit field through the 'mask' argument. I'm asking to get those masks defined. Is it possible? Yes, the code is equivalent and results in the same values being written to the same registers. At the same time you have added a logical entity, a masked write. I want to be able to understand if bits 4 and 5 are a part of the same register field or they belong to two different fields and can be written separately. I really don't understand why are we spending so much time arguing about a simple #define. Okay, in case of drm/msm it is not a #define, it is <reg><bitfield/></reg>. The net result is the same. > > > your code adds BIT(5) as a 'mask' parameter. Is it a correct mask for > > No, my code does not add it. It was already there, look: > > foo | BIT(5) | BIT(4) > ^^^^^^^ here > > > > that field? That's why I'm asking you to define those - you have changed > > No, I did not change bitwrites. The code is 100% equivalent, both > logically and assembly. > > You mistake maybe with some other part doing "writel(data & ~BIT(5)" in > dsi_pll_disable_global_clk() but that's just poor diff. > > > bitwrites to the masked bit writes. Masks should be defined. > > > >> > >> > >>> 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. > >> > >> While in general you are right, it does not matter for this fix. If this > >> are separate bitfields - fix is correct. If this is one bitfield - fix > >> is still correct. You could claim that if this was one bitfield, using > >> 2xBIT() is not logical, but this was there already, so again my fix is > >> only fixing and keeping entire logic or inconsistencies intact. > > > > > Best regards, > Krzysztof
On 05/02/2025 12:23, Dmitry Baryshkov wrote: >>>>>>>> + >>>>>>>> +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 >>>> >>>> No, previous code: >>>> >>>> writel(data | BIT(5) | BIT(4), pll->phy->base + >>>> REG_DSI_7nm_PHY_CMN_CLK_CFG1); >>>> >>>> This is a mask and update in the same time, because: >>>> (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) >>>> is just redudant. >>>> >>>> I did not do any logical change, I did not add any mask or field. >>>> Everything was already there. >>> >>> Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now >> >> You did not address my comment. Previous code was: >> >> (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) >> >> Just for shorter syntax it was written different way: >> >> foo | BIT(5) | BIT(4) > > Previously it was a simple writel() with some bit magic. Now you call The mask was already there, just implied. > dsi_pll_cmn_clk_cfg1_update() passing the register bit field through > the 'mask' argument. I'm asking to get those masks defined. Is it > possible? Just like before, because implied mask is being removed due to code redundancy. I repeat it for third time already. > > Yes, the code is equivalent and results in the same values being > written to the same registers. > At the same time you have added a logical entity, a masked write. I > want to be able to understand if bits 4 and 5 are a part of the same > register field or they belong to two different fields and can be I know you want to understand it and this is achieved in separate patch, because understanding this is not related to this commit. > written separately. I really don't understand why are we spending so > much time arguing about a simple #define. Okay, in case of drm/msm it > is not a #define, it is <reg><bitfield/></reg>. The net result is the > same. I also don't get why simple fix could not be just applied and it has to become some sort of big refactoring. Best regards, Krzysztof
On Wed, Feb 05, 2025 at 02:42:03PM +0100, Krzysztof Kozlowski wrote: > On 05/02/2025 12:23, Dmitry Baryshkov wrote: > >>>>>>>> + > >>>>>>>> +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 > >>>> > >>>> No, previous code: > >>>> > >>>> writel(data | BIT(5) | BIT(4), pll->phy->base + > >>>> REG_DSI_7nm_PHY_CMN_CLK_CFG1); > >>>> > >>>> This is a mask and update in the same time, because: > >>>> (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) > >>>> is just redudant. > >>>> > >>>> I did not do any logical change, I did not add any mask or field. > >>>> Everything was already there. > >>> > >>> Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now > >> > >> You did not address my comment. Previous code was: > >> > >> (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4) > >> > >> Just for shorter syntax it was written different way: > >> > >> foo | BIT(5) | BIT(4) > > > > Previously it was a simple writel() with some bit magic. Now you call > > > The mask was already there, just implied. > > > dsi_pll_cmn_clk_cfg1_update() passing the register bit field through > > the 'mask' argument. I'm asking to get those masks defined. Is it > > possible? > > Just like before, because implied mask is being removed due to code > redundancy. > > I repeat it for third time already. > > > > > Yes, the code is equivalent and results in the same values being > > written to the same registers. > > At the same time you have added a logical entity, a masked write. I > > want to be able to understand if bits 4 and 5 are a part of the same > > register field or they belong to two different fields and can be > > I know you want to understand it and this is achieved in separate patch, > because understanding this is not related to this commit. > > > written separately. I really don't understand why are we spending so > > much time arguing about a simple #define. Okay, in case of drm/msm it > > is not a #define, it is <reg><bitfield/></reg>. The net result is the > > same. > > I also don't get why simple fix could not be just applied and it has to > become some sort of big refactoring. Well, you have refactored that in this patch. Anyway. Please post the next iteration, let's continue the dicussion there. > > Best regards, > Krzysztof
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); 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, cfg_1, cfg_1); } static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll) @@ -574,7 +587,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 +597,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 +750,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 +795,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;
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@linaro.org> --- 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(-)