Re: [PATCH v2 2/4] drm/msm/dsi/phy: Protect PHY_CMN_CLK_CFG1 against clock driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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