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

[Stephen Boyd]

Sorry, I'm getting back from some vacations and I'm working through the
backlog.

Quoting Jerome Brunet (2019-05-24 11:12:30)
> 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 ?

I was representing the current code and the discussion we were having.
If we apply the patch I suggested earlier then we'll make this into a
'Y'.

I thought your code fell into the first case though; NULL fw_name and
index >= 0, so we do the DT lookup and everything blows up because that
parent isn't the one expected.

> 
> > > >         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 ? 

We use the fw_name and if that isn't provided, then we use the index.
This is how the logic was written for DT parsing of clks originally, so
I'm just continuing the same logic (see of_parse_clkspec() for more
details).

> 
> > > > 
> > > > Maybe we should support the ? case, because right now it will fail to do
> > > > the DT lookup when the index is -1.

Yeah, this part. I think it's fine to do this.

> > > 
> > > 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.

Ok. So you have a mixture of some parents with only the legacy name and
then other clks with some parents in the new style? As long as we
understand each other I think we'll figure it out.

> 
> > 
> > > 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 :)

Agreed, but you've implicitly defined the .fw_name to NULL and the
.index to 0, so you're insisting that an index of 0 should be used,
albeit implicitly so.

> 
> 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

Hmmm I made the assumption that if you were going to use the new
structure that you would be using "clock-names" (.fw_name) or an index
for everything and then falling back to .name for legacy migration
reasons. I didn't really consider that a mix of .name and .fw_name
would be used for different parent indices.

> 
> > 
> > > 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 ...

We'll only try the legacy name if all else fails. We're hoping that
.fw_name or .hw is used instead because those are faster than string
comparisons on the entire tree.

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

Yes.

> 
> IMO, it would be weird and very confusing.

Let's expand the table.

    .fw_name   |  .index |  .name      | DT lookup? | fallback to legacy? 
   +-----------+---------+-------------+----+----------------------------
 1 | NULL      |    >= 0 |  NULL       |    Y       |         N
 2 | NULL      |    -1   |  NULL       |    N       |         N
 3 | non-NULL  |    -1   |  NULL       |    Y       |         N
 4 | non-NULL  |    >= 0 |  NULL       |    Y       |         N
 5 | NULL      |    >= 0 |  non-NULL   |    Y       |         Y
 6 | NULL      |    -1   |  non-NULL   |    N       |         Y
 7 | non-NULL  |    -1   |  non-NULL   |    Y       |         Y
 8 | non-NULL  |    >= 0 |  non-NULL   |    Y       |         Y

And then if .hw is set we just don't even try to use the above table.

You want case #5 to skip the DT lookup because .fw_name is NULL, but the
index is 0. I stared at this for a few minutes and I think you're
arguing that we should only do the DT lookup when index is 0 if we can't
fallback to a legacy lookup.

Initially that sounds OK, but then we'll get stuck with legacy lookups
forever if someone doesn't put a matching .fw_name into the
'clock-names' property. We don't want that to happen. We want to get off
legacy and into the new world where either .fw_name or .index is used to
specify a parent.

>From my perspective you're suffering from static initializers setting
everything to 0 and that making the index 0 and "valid" for a DT lookup
hitting up against not wanting to set anything in the structure
unnecessarily. While at the same time, it's great that .fw_name is set
to NULL by the static initializer, so it's a case of wanting the same
thing to do different things.

If we would have made it so the DT index started at 1 then this wouldn't
be a problem, but that's not possible. Or if we would have made a new
clk flag that said CLK_USE_INDEX_FOR_DT then it could have gotten over
this problem but that's basically the same as making the inverse set of
drivers use -1 for the index to indicate they don't want a DT lookup.

I still believe the large majority of clk drivers will either use .hw or
.fw_name to find parents, while falling back to the .name for legacy
reasons. The .index field won't see much use, but we'll support it for
the firmwares out there that don't use "clock-names". Similarly, we'll
have only a handful of drivers that only want to specify .name and
nothing else, and those few drivers can use .index = -1 to overcome
this.

> > > } 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.
> 
> 

We need to support DTBs that don't specify "clock-names". The
"clock-names" property isn't mandatory, just strongly recommended, and
we have various bits of code like the fixed factor clk that don't expect
to see a name, just use some index like 0 to get the parent clk. I don't
want to go retroactively force a bunch of DT to get "clock-names"
properties in them so that they can work with the new style of parents.
And I don't want them to be stuck calling of_clk_parent_fill() or
of_clk_get() to parse that index and grab the clk name out during
registration.