Messages in this thread | | | Subject | Re: [PATCH] clk: fix clock global name usage. | From | Stephen Boyd <> | Date | Fri, 24 May 2019 10:44:53 -0700 |
| |
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.
| |