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