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

[Krzysztof Kozlowski]

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