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

[Stephen Boyd <sboyd@kernel.org>]

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
> >         non-NULL  |    -1   |     ?
> >         non-NULL  |    >= 0 |     Y
> > 
> > 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?

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

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

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