[PATCH] clk: fix clock global name usage.

[PATCH] clk: fix clock global name usage.

[Alexandre Mergnat]

A recent patch allows the clock framework to specify the parent
relationship with either the clk_hw pointer, the global name or through
Device Tree name.

But the global name isn't handled by the clk framework because the DT name
is considered valid even if it's NULL, so of_clk_get_hw() returns an
unexpected clock (the first clock specified in DT).

This can be fixed by calling of_clk_get_hw() only when DT name is not NULL.

Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
Cc: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index bdb077ba59b9..9624a75e5a8d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -368,7 +368,7 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct device_node *np = core->of_node;
 
-	if (np && index >= 0)
+	if (name && np && index >= 0)
 		hw = of_clk_get_hw(np, index, name);
 
 	/*
-- 
2.17.1

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

[Stephen Boyd]

Quoting Alexandre Mergnat (2019-05-24 00:27:45)
> A recent patch allows the clock framework to specify the parent
> relationship with either the clk_hw pointer, the global name or through
> Device Tree name.

You could point to the commit instead of saying "a recent patch". Would
provide more clarity.

> 
> But the global name isn't handled by the clk framework because the DT name
> is considered valid even if it's NULL, so of_clk_get_hw() returns an
> unexpected clock (the first clock specified in DT).

Yes, the DT name can be NULL and then we would use the index.

> 
> This can be fixed by calling of_clk_get_hw() only when DT name is not NULL.
> 
> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index bdb077ba59b9..9624a75e5a8d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -368,7 +368,7 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
>         const char *dev_id = dev ? dev_name(dev) : NULL;
>         struct device_node *np = core->of_node;
>  
> -       if (np && index >= 0)
> +       if (name && np && index >= 0)

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.

So this patch instead?

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b34e84bb8167..a554cb9316a5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -368,7 +368,7 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct device_node *np = core->of_node;
 
-	if (np && index >= 0)
+	if (np && (index >= 0 || name))
 		hw = of_clk_get_hw(np, index, name);
 
 	/*

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

[Jerome Brunet]

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 ?

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.

Do you think we could come up with a priority order which makes the first
example work ?

Something like:

if (hw) {
	/* use pointer */
} else if (name) {
	/* use legacy global names */
} 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.

Cheers
Jerome

> 
> So this patch instead?

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

[Stephen Boyd]

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.

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.

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.

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

[Jerome Brunet]

On Thu, 2019-06-06 at 15:54 -0700, Stephen Boyd wrote:
> > > > 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.

Then this where we have different views ont he matter. I think this makes the
logic a lot more complex.

A controller should *know* if a clock is coming from 'somewhere else' and how.
I think we should not try multiple methods hoping one will eventually work.

I has already caught us now and I'm willing to bet we won't be the last.

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

Sure ... but we are also suffering from the complexity of the logic behind this
(and the table above)

When I define .hw, I don't need to initialize the rest but when I define .name I
must define (at least) .index to a specific value ... This is at least tricky,
and IMO, inconsistent.

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

I'm sorry, I still find all this very confusing just to add a fallback
mechanism. It would be simpler to ask people to choose the method they.

The transition to DT can be done submitting DT part first and submitting the
clock driver part later on

Anyway, we'll define index to -1 since this how it is supposed to be. Thx for
your explanation

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

[wens Tsai]

On Fri, May 24, 2019 at 3:29 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>
> A recent patch allows the clock framework to specify the parent
> relationship with either the clk_hw pointer, the global name or through
> Device Tree name.
>
> But the global name isn't handled by the clk framework because the DT name
> is considered valid even if it's NULL, so of_clk_get_hw() returns an
> unexpected clock (the first clock specified in DT).
>
> This can be fixed by calling of_clk_get_hw() only when DT name is not NULL.
>
> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index bdb077ba59b9..9624a75e5a8d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -368,7 +368,7 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
>         const char *dev_id = dev ? dev_name(dev) : NULL;
>         struct device_node *np = core->of_node;
>
> -       if (np && index >= 0)
> +       if (name && np && index >= 0)

I think the opposite should be the case. If either the name or index is valid,
and there's a device node backing it, the code path should be entered.

This is implied by the description of struct clk_parent_data:

    @index: parent index local to provider registering clk (if @fw_name absent)

So the code path should be valid regardless of the value of .index.

That would make it

        if (np && (name || index >= 0)) ...

Regards
ChenYu


>                 hw = of_clk_get_hw(np, index, name);
>
>         /*
> --
> 2.17.1
>

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

[Stephen Boyd]

Quoting wens Tsai (2019-06-10 23:46:17)
> On Fri, May 24, 2019 at 3:29 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> >
> > A recent patch allows the clock framework to specify the parent
> > relationship with either the clk_hw pointer, the global name or through
> > Device Tree name.
> >
> > But the global name isn't handled by the clk framework because the DT name
> > is considered valid even if it's NULL, so of_clk_get_hw() returns an
> > unexpected clock (the first clock specified in DT).
> >
> > This can be fixed by calling of_clk_get_hw() only when DT name is not NULL.
> >
> > Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> > ---
> >  drivers/clk/clk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index bdb077ba59b9..9624a75e5a8d 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -368,7 +368,7 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
> >         const char *dev_id = dev ? dev_name(dev) : NULL;
> >         struct device_node *np = core->of_node;
> >
> > -       if (np && index >= 0)
> > +       if (name && np && index >= 0)
> 
> I think the opposite should be the case. If either the name or index is valid,
> and there's a device node backing it, the code path should be entered.
> 
> This is implied by the description of struct clk_parent_data:
> 
>     @index: parent index local to provider registering clk (if @fw_name absent)
> 
> So the code path should be valid regardless of the value of .index.
> 
> That would make it
> 
>         if (np && (name || index >= 0)) ...
> 

Sure. I'll post my fix and pick the patch into clk-fixes so that this
works.