Re: [PATCH] clk: fix clock global name usage.

[Jerome Brunet]

On Fri, 2019-05-24 at 10:44 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2019-05-24 08:00:08)
> > On Fri, 2019-05-24 at 07:33 -0700, Stephen Boyd wrote:
> > > Do you set the index to 0 in this clk's parent_data? We purposefully set
> > > the index to -1 in clk_core_populate_parent_map() so that the fw_name
> > > can be NULL but the index can be something >= 0 and then we'll use that
> > > to lookup the clk from DT. We need to support that combination.
> > > 
> > >         fw_name   |   index |  DT lookup?
> > >         ----------+---------+------------
> > >         NULL      |    >= 0 |     Y
> > >         NULL      |    -1   |     N

These two I understand

> > >         non-NULL  |    -1   |     ?

If fw_name is provided, you have everything you need to get the clock, why the ?

> > >         non-NULL  |    >= 0 |     Y

If both fw_name and index are provided, how do you perform the look up ? using
the index ? or the fw_name ? 

> > > 
> > > Maybe we should support the ? case, because right now it will fail to do
> > > the DT lookup when the index is -1.
> > 
> > Hi Stephen,
> > 
> > We are trying to migrate all meson clocks to the new parent structure.
> > There is a little quirk which forces us to continue to use legacy names
> > for a couple of clocks.
> > 
> > We heavily use static data which init everything to 0.
> > Here is an example:
> > 
> > static struct clk_regmap g12a_aoclk_cts_rtc_oscin = {
> > [...]
> >         .hw.init = &(struct clk_init_data){
> >                 .name = "g12a_ao_cts_rtc_oscin",
> >                 .ops = &clk_regmap_mux_ops,
> > -               .parent_names = (const char *[]){ "g12a_ao_32k_by_oscin",
> > -                                                 IN_PREFIX "ext_32k-0" },
> > +               .parent_data = (const struct clk_parent_data []) {
> > +                       { .name = "g12a_ao_32k_by_oscin" },
> > +                       { .fw_name = "ext-32k-0", },
> > +               },
> >                 .num_parents = 2,
> >                 .flags = CLK_SET_RATE_PARENT,
> >         },
> > };
> > 
> > With this, instead of taking name = "g12a_ao_32k_by_oscin" for entry #0
> > it takes DT names at index 0 which is not what we intended.
> > 
> > If I understand correctly we should put
> > +                       { .name = "g12a_ao_32k_by_oscin", index = -1, },
> > 
> > And would be alright ?
> 
> I don't understand why this wouldn't have a .fw_name or an .index >= 0,
> or both. Is there some reason why that isn't happening?

And now its me not following :)

In the case I presenting, I only defined the (legacy) name because that we want
to use. In another thread, I'll explain the particular problem that make us use
this legacy name, I just to dont want to over complicate this topic now.

> 
> > While I understand it, it is not very obvious or nice to look at.
> > Plus it is a bit weird that this -1 is required for .name and not .hw.
> 
> Sure. It can be better documented. Sorry it's not super obvious. I added
> this later in the series. We could have:
> 
> 	#define CLK_SKIP_FW_LOOKUP .index = -1
> 
> and then this would read as:
> 
>         { .name = "g12a_ao_32k_by_oscin", CLK_SKIP_FW_LOOKUP },

Sure but it is still a bit ugly and un-intuitive. If I only defined the legacy
name, it's pretty obvious that what I want to use ... I should not have to
insist :)

And again the fact that (legacy) .name is silently discarded if index is not
defined, but .hw or .fw_name are taken into account no matter what is not
consistent

> 
> > Do you think we could come up with a priority order which makes the first
> > example work ?
> 
> Maybe? I'm open to suggestions.
> 
> > Something like:
> > 
> > if (hw) {
> >         /* use pointer */
> > } else if (name) {
> >         /* use legacy global names */
> 
> I don't imagine we can get rid of legacy name for a long time, so this
> can't be in this order. Otherwise we'll try to lookup the legacy name
> before trying the DT lookup and suffer performance hits doing a big
> global search while also skipping the DT lookup that we want drivers to
> use if they're more modern.

You'll try to look up the legacy name only if it is defined, in which case you
know you this is what you want to use, so I don't see the penalty.  Unless ...

Are trying to support case where multiple fields among hw, name, fw_name, index
would be defined simultaneously ??

IMO, it would be weird and very confusing.

> 
> > } else if (fw_name) {
> >         /* use DT names */
> > } else if (index >= 0) 
> >         /* use DT index */
> > } else {
> >         return -EINVAL;
> > }
> > 
> > The last 2 clause could be removed if we make index an unsigned.
> > 
> 
> So just assign -1 to .index? I still think my patch may be needed if
> somehow the index is assigned to something less than 0 and the .fw_name
> is specified. I guess that's possible if the struct is on the stack, so
> we'll probably have to allow this combination.

Maybe it just solve the problem, I don't fully understand its implication. I
might need to try it, and see

> 

digressing a bit ...

I don't remember seeing the index field in the initial review of your series ?
what made you add this ?

Isn't it simpler to mandate the use of the clock-names property ? Referring to
the clock property by index is really weak and should discouraged IMO.