Re: [PATCH v2 1/2] drm/msm/dsi: Use "ref" fw clock instead of global name for VCO parent

[Stephen Boyd]

Quoting Marijn Suijten (2021-09-01 01:49:10)
> Hi Stephen,
> 
> On 2021-08-31 22:35:12, Stephen Boyd wrote:
> > Quoting Marijn Suijten (2021-08-30 16:10:26)
> > > 
> > > I'm 95% sure this shouldn't cause any problems given current DTs and
> > > their history, but that's probably not enough.  This might also impact
> > > DTs that have not yet been upstreamed, but afaik the general stance is
> > > to not care and actually serve as a fair hint/warning before new DTs
> > > make it to the list.
> > > 
> > > If there is a protocol in place to deprecate, warn, and eventually
> > > remove this reliance on global clock names I'm more than happy to add
> > > .name as a temporary fallback, even if likely unneeded.  Otherwise we
> > > might never get rid of it.
> > 
> > I'm not aware of any protocol to deprecate, warn, and remove the
> > fallback name. It's a fallback because it can't ever really be removed.
> 
> That is unfortunate, I was hoping for a breaking "kernel release" at
> some point where we could say "no more, update your DTs first".  But
> that may not be possible in every scenario?
> 
> > Anyway, if you're not willing to add the .name then that's fine.
> 
> I feel like .name has caused more problems for us than it solves, but in
> a fallback position it might be fine.  My main gripe is that I don't
> want DT to rely on the clock to also be discoverable through the clock
> tree, which we've seen on many occasions (not sure if the former was
> done upstream, but: "xo" being renamed to "xo_board", and DSI PLL clocks
> loosing +1 causing a naming mismatch with what mmcc expects, to name
> some examples).  Omitting .name is the only way to enforce that.

The simple approach to take is anything new should use fw_name. The name
member is only there for the backup mode when the DT isn't properly
setup to describe connections between clk controllers. If the code is
new then name can be omitted and everything is OK. Otherwise, if
parent_names was already being used, then most likely it will need to be
set as .name in the clk_parent_data struct to maintain compatibility. If
parent_names was wrong, then it was all broken to begin with and .name
can be omitted.

> 
> > We can
> > deal with the problem easily by adding a .name in the future if someone
> > complains that things aren't working. Sound like a plan? If so, it's
> > probably good to add some sort of note in the commit text so that when
> > the bisector lands on this patch they can realize that this
> > intentionally broke them.
> 
> I'm all for this but lack the industrial knowledge to sign off on the
> approach.  Bjorn and Dmitry should ack/agree before going ahead (you may
> wonder why I'm worrying about getting clock drivers and DT in sync on
> platforms I don't own...):
> 
> We have the following situations:
> - apq8064 used the wrong clock.  Bjorn acknowledged that landing the DT
>   fix in 5.15, and this patch in 5.16 should give enough time for DT to
>   be updated (this is nothing we can fix with .name anyway).
> - msm8974 doesn't have the clock at all.  Dmitry recommended to add
>   .name for this specific case, but I'm wondering if the 5.15 -> 5.16
>   window is enough to update DTs too?
> - msm8916 and sdm845 had the missing "ref" clock added three years ago.
>   Would a 5.16 kernel still work in any meaningful way with a DT that
>   old?
> 
> Should we decide on a case-by-case basis whether to add .name, ie. only
> for (some/all) of the aforementioned SoCs?
> 

I sort of glossed over this, sorry. Hopefully what I wrote above can
guide you and then we shouldn't really need to worry about anything?