lkml.org 
[lkml]   [2019]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] clk: fix clock global name usage.
From
Date
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.

\
 
 \ /
  Last update: 2019-05-24 19:45    [W:0.059 / U:20.976 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site