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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry