| Message ID | 20190129061021.94775-8-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Rewrite clk parent handling | expand |
On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote: > The common clk framework is lacking in ability to describe the clk > topology without specifying strings for every possible parent-child > link. There are a few drawbacks to the current approach: > > 1) String comparisons are used for everything, including describing > topologies that are 'local' to a single clock controller. > > 2) clk providers (e.g. i2c clk drivers) need to create globally unique > clk names to avoid collisions in the clk namespace, leading to awkward > name generation code in various clk drivers. > > 3) DT bindings may not fully describe the clk topology and linkages > between clk controllers because drivers can easily rely on globally unique > strings to describe connections between clks. > > This leads to confusing DT bindings, complicated clk name generation > code, and inefficient string comparisons during clk registration just so > that the clk framework can detect the topology of the clk tree. > Furthermore, some drivers call clk_get() and then __clk_get_name() to > extract the globally unique clk name just so they can specify the parent > of the clk they're registering. We have of_clk_parent_fill() but that > mostly only works for single clks registered from a DT node, which isn't > the norm. Let's simplify this all by introducing two new ways of > specifying clk parents. > > The first method is an array of pointers to clk_hw structures > corresponding to the parents at that index. This works for clks that are > registered when we have access to all the clk_hw pointers for the > parents. > > The second method is a mix of clk_hw pointers and strings of local and > global parent clk names. If the .name member of the map is set we'll > look for that clk by performing a DT based lookup of the device the clk > is registered with and the .name specified in the map. If that fails, > we'll fallback to the .fallback member and perform a global clk name > lookup like we've always done before. > > Using either one of these new methods is entirely optional. Existing > drivers will continue to work, and they can migrate to this new approach > as they see fit. Eventually, we'll want to get rid of the 'parent_names' > array in struct clk_init_data and use one of these new methods instead. This may indeed allow to remove a lot of annoying code. However, does this remove the globally unique name string constraints ? Are we now allowed to have 2 instances of a driver registering a clock named "foo" ? If this is the case, I wonder: * How will it work with debugfs: clock names are used to create the directories in there, plus clk_summary will quickly get messy. * How will it behave if 2 clock registers with "foo" and one clock register with the fallback parent "foo" ? > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Michael Turquette <mturquette@baylibre.com> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 215 +++++++++++++++++++++++++---------- > include/linux/clk-provider.h | 17 ++- > 2 files changed, 173 insertions(+), 59 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 202902e64799..0cd90a2339ad 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list); > > /*** private data structures ***/ > > +struct clk_parent_map { > + struct clk_hw *hw; > + struct clk_core *core; > + const char *name; > + const char *fallback; > +}; > + > struct clk_core { > const char *name; > const struct clk_ops *ops; > @@ -49,8 +56,7 @@ struct clk_core { > struct module *owner; > struct device *dev; > struct clk_core *parent; > - const char **parent_names; > - struct clk_core **parents; > + struct clk_parent_map *parents; > u8 num_parents; > u8 new_parent_index; > unsigned long rate; > @@ -319,17 +325,77 @@ static struct clk_core *clk_core_lookup(const char > *name) > return NULL; > } > > +/** > + * clk_core_get - Find the parent of a clk using a clock specifier in DT > + * @core: clk to find parent of > + * @name: name to search for in 'clock-names' of device providing clk > + * > + * This is the preferred method for clk providers to find the parent of a > + * clk when that parent is external to the clk controller. The parent_names > + * array is indexed and treated as a local name matching a string in the > device > + * node's 'clock-names' property. This allows clk providers to use their > own > + * namespace instead of looking for a globally unique parent string. > + * > + * For example the following DT snippet would allow a clock registered by > the > + * clock-controller@c001 that has a clk_init_data::parent_data array > + * with 'xtal' in the 'name' member to find the clock provided by the > + * clock-controller@f00abcd without needing to get the globally unique name > of > + * the xtal clk. > + * > + * parent: clock-controller@f00abcd { > + * reg = <0xf00abcd 0xabcd>; > + * #clock-cells = <0>; > + * }; > + * > + * clock-controller@c001 { > + * reg = <0xc001 0xf00d>; > + * clocks = <&parent>; > + * clock-names = "xtal"; > + * #clock-cells = <1>; > + * }; > + */ > +static struct clk_core *clk_core_get(struct clk_core *core, const char > *name) > +{ > + struct clk_hw *hw; > + struct device *dev = core->dev; > + > + if (!dev) > + return NULL; > + > + /* TODO: Support clkdev clk_lookups */ > + hw = of_clk_get_hw(dev->of_node, -1, name); > + if (IS_ERR_OR_NULL(hw)) > + return NULL; > + > + return hw->core; > +} > + > +static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > +{ > + struct clk_parent_map *entry = &core->parents[index]; > + struct clk_core *parent = NULL; > + > + if (entry->hw) > + parent = entry->hw->core; > + else if (entry->name) > + parent = clk_core_get(core, entry->name); > + > + if (!parent) > + parent = clk_core_lookup(entry->fallback); > + > + entry->core = parent; > +} > + > static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core, > u8 index) > { > - if (!core || index >= core->num_parents) > + if (!core || index >= core->num_parents || !core->parents) > return NULL; > > - if (!core->parents[index]) > - core->parents[index] = > - clk_core_lookup(core->parent_names[index]); > + if (!core->parents[index].core) > + clk_core_fill_parent_index(core, index); > > - return core->parents[index]; > + return core->parents[index].core; > } > > struct clk_hw * > @@ -2353,6 +2419,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw > *new_parent) > bool clk_has_parent(struct clk *clk, struct clk *parent) > { > struct clk_core *core, *parent_core; > + int i; > > /* NULL clocks should be nops, so return success if either is NULL. */ > if (!clk || !parent) > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk > *parent) > if (core->parent == parent_core) > return true; > > - return match_string(core->parent_names, core->num_parents, > - parent_core->name) >= 0; > + for (i = 0; i < core->num_parents; i++) > + if (!strcmp(core->parents[i].fallback, parent_core->name)) > + return true; > + > + return false; > } > EXPORT_SYMBOL_GPL(clk_has_parent); > > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s, > void *data) > int i; > > for (i = 0; i < core->num_parents - 1; i++) > - seq_printf(s, "%s ", core->parent_names[i]); > + seq_printf(s, "%s ", core->parents[i].fallback); > > - seq_printf(s, "%s\n", core->parent_names[i]); > + seq_printf(s, "%s\n", core->parents[i].fallback); Wouldn't this show nothing if parent_data is used but fallback is not provided (like in your example when you provide the clk_hw pointer instead) or did I miss something ? > > return 0; > } > @@ -3085,7 +3155,7 @@ static inline void clk_debug_unregister(struct > clk_core *core) > */ > static int __clk_core_init(struct clk_core *core) > { > - int i, ret; > + int ret; > struct clk_core *orphan; > struct hlist_node *tmp2; > unsigned long rate; > @@ -3139,12 +3209,6 @@ static int __clk_core_init(struct clk_core *core) > goto out; > } > > - /* throw a WARN if any entries in parent_names are NULL */ > - for (i = 0; i < core->num_parents; i++) > - WARN(!core->parent_names[i], > - "%s: invalid NULL in %s's .parent_names\n", > - __func__, core->name); > - > ret = clk_init_parent(core); > if (ret) > goto out; > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev, > struct clk_hw *hw, > return clk; > } > > +static int clk_core_populate_parent_map(struct clk_core *core) > +{ > + const struct clk_init_data *init = core->hw->init; > + u8 num_parents = init->num_parents; > + const char * const *parent_names = init->parent_names; > + struct clk_hw **parent_hws = init->parent_hws; > + const struct clk_parent_data *parent_data = init->parent_data; > + int i, ret = 0; > + struct clk_parent_map *parents, *parent; > + > + if (!num_parents) > + return 0; > + > + /* > + * Avoid unnecessary string look-ups of clk_core's possible parents by > + * having a cache of names/clk_hw pointers to clk_core pointers. > + */ > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL); > + core->parents = parents; > + if (!parents) > + return -ENOMEM; > + > + /* Copy everything over because it might be __initdata */ > + for (i = 0, parent = parents; i < num_parents; i++, parent++) { > + if (parent_names) { > + /* throw a WARN if any entries are NULL */ > + WARN(!parent_names[i], > + "%s: invalid NULL in %s's .parent_names\n", > + __func__, core->name); > + parent->fallback = kstrdup_const(parent_names[i], > + GFP_KERNEL); > + if (!parent->fallback) { > + ret = -ENOMEM; > + while (--i >= 0) > + kfree_const(parents[i].fallback); > + } > + } else if (parent_data) { > + parent->hw = parent_data[i].hw; > + parent->name = parent_data[i].name; > + parent->fallback = parent_data[i].fallback; I'm a bit confused by the comment at the top of the loop. Strings in parent_data are not copied over like we used to do with parent_names. Is it allowed to have parent_data in __initdata ? It could be error prone if parent_names and parent_data behaved differently on this. > + } else if (parent_hws) { > + parent->hw = parent_hws[i]; > + } else { > + ret = -EINVAL; > + WARN(1, "Must specify parents if num_parents > 0\n"); > + } > + > + if (ret) { > + kfree(parents); > + return ret; > + } > + } > + > + return 0; > +} > + > +static void clk_core_free_parent_map(struct clk_core *core) > +{ > + int i = core->num_parents; > + > + if (!core->num_parents) > + return; > + > + while (--i >= 0) > + kfree_const(core->parents[i].fallback); > + kfree(core->parents); > +} > + > /** > * clk_register - allocate a new clock, register it and return an opaque > cookie > * @dev: device that is registering this clock > @@ -3373,7 +3505,7 @@ struct clk *clk_hw_create_clk(struct device *dev, > struct clk_hw *hw, > */ > struct clk *clk_register(struct device *dev, struct clk_hw *hw) > { > - int i, ret; > + int ret; > struct clk_core *core; > > core = kzalloc(sizeof(*core), GFP_KERNEL); > @@ -3406,33 +3538,9 @@ struct clk *clk_register(struct device *dev, struct > clk_hw *hw) > core->max_rate = ULONG_MAX; > hw->core = core; > > - /* allocate local copy in case parent_names is __initdata */ > - core->parent_names = kcalloc(core->num_parents, sizeof(char *), > - GFP_KERNEL); > - > - if (!core->parent_names) { > - ret = -ENOMEM; > - goto fail_parent_names; > - } > - > - > - /* copy each string name in case parent_names is __initdata */ > - for (i = 0; i < core->num_parents; i++) { > - core->parent_names[i] = kstrdup_const(hw->init- > >parent_names[i], > - GFP_KERNEL); > - if (!core->parent_names[i]) { > - ret = -ENOMEM; > - goto fail_parent_names_copy; > - } > - } > - > - /* avoid unnecessary string look-ups of clk_core's possible parents. > */ > - core->parents = kcalloc(core->num_parents, sizeof(*core->parents), > - GFP_KERNEL); > - if (!core->parents) { > - ret = -ENOMEM; > + ret = clk_core_populate_parent_map(core); > + if (ret) > goto fail_parents; > - }; > > INIT_HLIST_HEAD(&core->clks); > > @@ -3443,7 +3551,7 @@ struct clk *clk_register(struct device *dev, struct > clk_hw *hw) > hw->clk = alloc_clk(core, NULL, NULL); > if (IS_ERR(hw->clk)) { > ret = PTR_ERR(hw->clk); > - goto fail_parents; > + goto fail_create_clk; > } > > clk_core_link_consumer(hw->core, hw->clk); > @@ -3459,13 +3567,9 @@ struct clk *clk_register(struct device *dev, struct > clk_hw *hw) > free_clk(hw->clk); > hw->clk = NULL; > > +fail_create_clk: > + clk_core_free_parent_map(core); > fail_parents: > - kfree(core->parents); > -fail_parent_names_copy: > - while (--i >= 0) > - kfree_const(core->parent_names[i]); > - kfree(core->parent_names); > -fail_parent_names: > fail_ops: > kfree_const(core->name); > fail_name: > @@ -3495,15 +3599,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register); > static void __clk_release(struct kref *ref) > { > struct clk_core *core = container_of(ref, struct clk_core, ref); > - int i = core->num_parents; > > lockdep_assert_held(&prepare_lock); > > - kfree(core->parents); > - while (--i >= 0) > - kfree_const(core->parent_names[i]); > - > - kfree(core->parent_names); > + clk_core_free_parent_map(core); > kfree_const(core->name); > kfree(core); > } > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 8b84dee942bf..f513f4074a93 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -264,6 +264,18 @@ struct clk_ops { > void (*debug_init)(struct clk_hw *hw, struct dentry > *dentry); > }; > > +/** > + * struct clk_parent_data - clk parent information > + * @hw: parent clk_hw pointer (used for clk providers with internal clks) > + * @name: parent name local to provider registering clk > + * @fallback: globally unique parent name (used as a fallback) > + */ > +struct clk_parent_data { > + struct clk_hw *hw; > + const char *name; > + const char *fallback; If I understand correctly, .name and .fallback will be ignored if hw is provided ? Maybe this should be documented somehow ? > +}; > + > /** > * struct clk_init_data - holds init data that's common to all clocks and > is > * shared between the clock provider and the common clock framework. > @@ -277,7 +289,10 @@ struct clk_ops { > struct clk_init_data { > const char *name; > const struct clk_ops *ops; > - const char * const *parent_names; > + /* Only one of the following three should be assigned */ > + const char * const *parent_names; /* If NULL (and > num_parents != 0) look at parent_data and parent_hws */ > + const struct clk_parent_data *parent_data; > + struct clk_hw **parent_hws; Isn't parent_hws redundant with parent_data ? you may pass the clk_hw pointer with both, right ? > u8 num_parents; > unsigned long flags; > };
Quoting Jerome Brunet (2019-01-29 02:12:00) > On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote: > > > > Using either one of these new methods is entirely optional. Existing > > drivers will continue to work, and they can migrate to this new approach > > as they see fit. Eventually, we'll want to get rid of the 'parent_names' > > array in struct clk_init_data and use one of these new methods instead. > > This may indeed allow to remove a lot of annoying code. > > However, does this remove the globally unique name string constraints ? Are we > now allowed to have 2 instances of a driver registering a clock named "foo" ? Yes it would be allowed to have multiple clocks with the same name but I'm not trying to solve that exact problem right now. The framework already complains if that's happening, so drivers need to generate unique names regardless of this series. > > If this is the case, I wonder: > * How will it work with debugfs: clock names are used to create the > directories in there, plus clk_summary will quickly get messy. Agreed. We should probably prepend the device name to the front of the clk now when creating in debugfs. I'll throw a patch for that into the pile to get that problem out of the way. I'm also beginning to think we should add a way to pass in the of_node for a clk when it's registered. Probably need to do that with the initdata structure again. That way we can lookup a clk's parents with the DT node if they don't call clk_register() with a device and also to generate a better unique name for the clk for debug purposes. > * How will it behave if 2 clock registers with "foo" and one clock register > with the fallback parent "foo" ? Sorry, I'm not following. The fallback is global so we'll be unable to figure out which parent the clk is referring to. Maybe this is an argument for keeping everything globally unique in the clk name space? > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > Cc: Jerome Brunet <jbrunet@baylibre.com> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Michael Turquette <mturquette@baylibre.com> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > --- > > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk > > *parent) > > if (core->parent == parent_core) > > return true; > > > > - return match_string(core->parent_names, core->num_parents, > > - parent_core->name) >= 0; > > + for (i = 0; i < core->num_parents; i++) > > + if (!strcmp(core->parents[i].fallback, parent_core->name)) > > + return true; This is also messy and not great BTW. > > + > > + return false; > > } > > EXPORT_SYMBOL_GPL(clk_has_parent); > > > > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s, > > void *data) > > int i; > > > > for (i = 0; i < core->num_parents - 1; i++) > > - seq_printf(s, "%s ", core->parent_names[i]); > > + seq_printf(s, "%s ", core->parents[i].fallback); > > > > - seq_printf(s, "%s\n", core->parent_names[i]); > > + seq_printf(s, "%s\n", core->parents[i].fallback); > > Wouldn't this show nothing if parent_data is used but fallback is not provided > (like in your example when you provide the clk_hw pointer instead) or did I > miss something ? Yes, it will show nothing. Maybe we need to generate the debug name somehow? That code sounds quite awful because we're going in reverse to the device through a DT node pointer. Or add an indicator in the output if the parent is a global name vs. a local name or a clk debugfs name? "global_name(g)" "local_name(l)" "debug_name" Or if we can't figure anything out then perhaps just ignoring this problem for now is fine. It is debugfs after all. > > > > > return 0; > > } > > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev, > > struct clk_hw *hw, > > return clk; > > } > > > > +static int clk_core_populate_parent_map(struct clk_core *core) > > +{ > > + const struct clk_init_data *init = core->hw->init; > > + u8 num_parents = init->num_parents; > > + const char * const *parent_names = init->parent_names; > > + struct clk_hw **parent_hws = init->parent_hws; > > + const struct clk_parent_data *parent_data = init->parent_data; > > + int i, ret = 0; > > + struct clk_parent_map *parents, *parent; > > + > > + if (!num_parents) > > + return 0; > > + > > + /* > > + * Avoid unnecessary string look-ups of clk_core's possible parents by > > + * having a cache of names/clk_hw pointers to clk_core pointers. > > + */ > > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL); > > + core->parents = parents; > > + if (!parents) > > + return -ENOMEM; > > + > > + /* Copy everything over because it might be __initdata */ > > + for (i = 0, parent = parents; i < num_parents; i++, parent++) { > > + if (parent_names) { > > + /* throw a WARN if any entries are NULL */ > > + WARN(!parent_names[i], > > + "%s: invalid NULL in %s's .parent_names\n", > > + __func__, core->name); > > + parent->fallback = kstrdup_const(parent_names[i], > > + GFP_KERNEL); > > + if (!parent->fallback) { > > + ret = -ENOMEM; > > + while (--i >= 0) > > + kfree_const(parents[i].fallback); > > + } > > + } else if (parent_data) { > > + parent->hw = parent_data[i].hw; > > + parent->name = parent_data[i].name; > > + parent->fallback = parent_data[i].fallback; > > I'm a bit confused by the comment at the top of the loop. Strings in > parent_data are not copied over like we used to do with parent_names. > > Is it allowed to have parent_data in __initdata ? It could be error prone if > parent_names and parent_data behaved differently on this. Good catch, thanks. Will fix. > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > index 8b84dee942bf..f513f4074a93 100644 > > --- a/include/linux/clk-provider.h > > +++ b/include/linux/clk-provider.h > > @@ -264,6 +264,18 @@ struct clk_ops { > > void (*debug_init)(struct clk_hw *hw, struct dentry > > *dentry); > > }; > > > > +/** > > + * struct clk_parent_data - clk parent information > > + * @hw: parent clk_hw pointer (used for clk providers with internal clks) > > + * @name: parent name local to provider registering clk > > + * @fallback: globally unique parent name (used as a fallback) > > + */ > > +struct clk_parent_data { > > + struct clk_hw *hw; > > + const char *name; > > + const char *fallback; > > If I understand correctly, .name and .fallback will be ignored if hw is > provided ? Maybe this should be documented somehow ? Sure. I'll add some documentation to the long portion of the kernel-doc here describing priority order. > > > +}; > > + > > /** > > * struct clk_init_data - holds init data that's common to all clocks and > > is > > * shared between the clock provider and the common clock framework. > > @@ -277,7 +289,10 @@ struct clk_ops { > > struct clk_init_data { > > const char *name; > > const struct clk_ops *ops; > > - const char * const *parent_names; > > + /* Only one of the following three should be assigned */ > > + const char * const *parent_names; /* If NULL (and > > num_parents != 0) look at parent_data and parent_hws */ > > + const struct clk_parent_data *parent_data; > > + struct clk_hw **parent_hws; > > Isn't parent_hws redundant with parent_data ? you may pass the clk_hw pointer > with both, right ? Yeah it's redundant, but I thought that drivers may not want to waste the extra space for pointers if they had all the pointers in hand for the parents they care about, or if they had a single parent and just wanted to point to that directly. Another thought is to have a union on these three pointers and then a flag indicating which method is used: union { const char * const *parent_names; const struct clk_parent_data *parent_data; struct clk_hw **parent_hws; }; #define CLK_PARENTS_POINTERS BIT(3) #define CLK_PARENTS_DATA BIT(8)
On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote: > Quoting Jerome Brunet (2019-01-29 02:12:00) > > On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote: > > > Using either one of these new methods is entirely optional. Existing > > > drivers will continue to work, and they can migrate to this new approach > > > as they see fit. Eventually, we'll want to get rid of the 'parent_names' > > > array in struct clk_init_data and use one of these new methods instead. > > > > This may indeed allow to remove a lot of annoying code. > > > > However, does this remove the globally unique name string constraints ? > > Are we > > now allowed to have 2 instances of a driver registering a clock named > > "foo" ? > > Yes it would be allowed to have multiple clocks with the same name but > I'm not trying to solve that exact problem right now. The framework > already complains if that's happening, so drivers need to generate > unique names regardless of this series. Ok. Got it > > > If this is the case, I wonder: > > * How will it work with debugfs: clock names are used to create the > > directories in there, plus clk_summary will quickly get messy. > > Agreed. We should probably prepend the device name to the front of the > clk now when creating in debugfs. I'll throw a patch for that into the > pile to get that problem out of the way. If we are supposed to keep globally unique names, maybe we should be keep things as they are for now. > > I'm also beginning to think we should add a way to pass in the of_node > for a clk when it's registered. Probably need to do that with the > initdata structure again. That way we can lookup a clk's parents with > the DT node if they don't call clk_register() with a device and also to > generate a better unique name for the clk for debug purposes. I don't get it now, but I'm sure I'll understand better with the code later on :) > > > * How will it behave if 2 clock registers with "foo" and one clock > > register > > with the fallback parent "foo" ? > > Sorry, I'm not following. The fallback is global so we'll be unable to > figure out which parent the clk is referring to. This is what I was thinking as well. But since the global clock namespace is not the point of this series, the question is off topic. Let's leave this foranother day > Maybe this is an > argument for keeping everything globally unique in the clk name space? For now, sure. > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > > Cc: Jerome Brunet <jbrunet@baylibre.com> > > > Cc: Russell King <linux@armlinux.org.uk> > > > Cc: Michael Turquette <mturquette@baylibre.com> > > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > > --- > > > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk > > > *parent) > > > if (core->parent == parent_core) > > > return true; > > > > > > - return match_string(core->parent_names, core->num_parents, > > > - parent_core->name) >= 0; > > > + for (i = 0; i < core->num_parents; i++) > > > + if (!strcmp(core->parents[i].fallback, parent_core->name)) > > > + return true; > > This is also messy and not great BTW. > > > > + > > > + return false; > > > } > > > EXPORT_SYMBOL_GPL(clk_has_parent); > > > > > > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file > > > *s, > > > void *data) > > > int i; > > > > > > for (i = 0; i < core->num_parents - 1; i++) > > > - seq_printf(s, "%s ", core->parent_names[i]); > > > + seq_printf(s, "%s ", core->parents[i].fallback); > > > > > > - seq_printf(s, "%s\n", core->parent_names[i]); > > > + seq_printf(s, "%s\n", core->parents[i].fallback); > > > > Wouldn't this show nothing if parent_data is used but fallback is not > > provided > > (like in your example when you provide the clk_hw pointer instead) or did > > I > > miss something ? > > Yes, it will show nothing. Maybe we need to generate the debug name > somehow? That would be nice. that or dropping the possible parent entry. > That code sounds quite awful because we're going in reverse to > the device through a DT node pointer. Or add an indicator in the output > if the parent is a global name vs. a local name or a clk debugfs name? > > "global_name(g)" > "local_name(l)" > "debug_name" > > Or if we can't figure anything out then perhaps just ignoring this > problem for now is fine. It is debugfs after all. Humm, I don't know if there are any rules regarding debugfs but it bothers me if we knowingly report wrong values through it. This may lead other developers to waste their time on this If we can't report the actual parent_name, we should at very least warn about it - display [FIXME] or [MISSING] for example. > > > > > > > return 0; > > > } > > > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev, > > > struct clk_hw *hw, > > > return clk; > > > } > > > > > > +static int clk_core_populate_parent_map(struct clk_core *core) > > > +{ > > > + const struct clk_init_data *init = core->hw->init; > > > + u8 num_parents = init->num_parents; > > > + const char * const *parent_names = init->parent_names; > > > + struct clk_hw **parent_hws = init->parent_hws; > > > + const struct clk_parent_data *parent_data = init->parent_data; > > > + int i, ret = 0; > > > + struct clk_parent_map *parents, *parent; > > > + > > > + if (!num_parents) > > > + return 0; > > > + > > > + /* > > > + * Avoid unnecessary string look-ups of clk_core's possible > > > parents by > > > + * having a cache of names/clk_hw pointers to clk_core pointers. > > > + */ > > > + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL); > > > + core->parents = parents; > > > + if (!parents) > > > + return -ENOMEM; > > > + > > > + /* Copy everything over because it might be __initdata */ > > > + for (i = 0, parent = parents; i < num_parents; i++, parent++) { > > > + if (parent_names) { > > > + /* throw a WARN if any entries are NULL */ > > > + WARN(!parent_names[i], > > > + "%s: invalid NULL in %s's > > > .parent_names\n", > > > + __func__, core->name); > > > + parent->fallback = kstrdup_const(parent_names[i], > > > + GFP_KERNEL); > > > + if (!parent->fallback) { > > > + ret = -ENOMEM; > > > + while (--i >= 0) > > > + kfree_const(parents[i].fallback); > > > + } > > > + } else if (parent_data) { > > > + parent->hw = parent_data[i].hw; > > > + parent->name = parent_data[i].name; > > > + parent->fallback = parent_data[i].fallback; > > > > I'm a bit confused by the comment at the top of the loop. Strings in > > parent_data are not copied over like we used to do with parent_names. > > > > Is it allowed to have parent_data in __initdata ? It could be error prone > > if > > parent_names and parent_data behaved differently on this. > > Good catch, thanks. Will fix. > > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > > index 8b84dee942bf..f513f4074a93 100644 > > > --- a/include/linux/clk-provider.h > > > +++ b/include/linux/clk-provider.h > > > @@ -264,6 +264,18 @@ struct clk_ops { > > > void (*debug_init)(struct clk_hw *hw, struct dentry > > > *dentry); > > > }; > > > > > > +/** > > > + * struct clk_parent_data - clk parent information > > > + * @hw: parent clk_hw pointer (used for clk providers with internal > > > clks) > > > + * @name: parent name local to provider registering clk > > > + * @fallback: globally unique parent name (used as a fallback) > > > + */ > > > +struct clk_parent_data { > > > + struct clk_hw *hw; > > > + const char *name; > > > + const char *fallback; > > > > If I understand correctly, .name and .fallback will be ignored if hw is > > provided ? Maybe this should be documented somehow ? > > Sure. I'll add some documentation to the long portion of the kernel-doc > here describing priority order. > > > > +}; > > > + > > > /** > > > * struct clk_init_data - holds init data that's common to all clocks > > > and > > > is > > > * shared between the clock provider and the common clock framework. > > > @@ -277,7 +289,10 @@ struct clk_ops { > > > struct clk_init_data { > > > const char *name; > > > const struct clk_ops *ops; > > > - const char * const *parent_names; > > > + /* Only one of the following three should be assigned */ > > > + const char * const *parent_names; /* If NULL (and > > > num_parents != 0) look at parent_data and parent_hws */ > > > + const struct clk_parent_data *parent_data; > > > + struct clk_hw **parent_hws; > > > > Isn't parent_hws redundant with parent_data ? you may pass the clk_hw > > pointer > > with both, right ? > > Yeah it's redundant, but I thought that drivers may not want to waste > the extra space for pointers if they had all the pointers in hand for > the parents they care about, or if they had a single parent and just > wanted to point to that directly. Fine. Makes sense. > > Another thought is to have a union on these three pointers and then a > flag indicating which method is used: > > union { > const char * const *parent_names; > const struct clk_parent_data *parent_data; > struct clk_hw **parent_hws; > }; > > #define CLK_PARENTS_POINTERS BIT(3) > #define CLK_PARENTS_DATA BIT(8) > Whatever is simpler :) If keeping the current solution, we just need to document the priority order.
On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote: > > > +/** > > > + * struct clk_parent_data - clk parent information > > > + * @hw: parent clk_hw pointer (used for clk providers with internal > > > clks) > > > + * @name: parent name local to provider registering clk > > > + * @fallback: globally unique parent name (used as a fallback) > > > + */ > > > +struct clk_parent_data { > > > + struct clk_hw *hw; > > > + const char *name; > > > + const char *fallback; One last nitpick about this structure, because I did not figure it out at first. 'fallback' is what we known as 'name' in CCF so far. What do you think about renaming 'fallback' to 'name' and 'name' to something more obvious, like 'of_name' or 'fw_name' or something else ? > > > > If I understand correctly, .name and .fallback will be ignored if hw is > > provided ? Maybe this should be documented somehow ? > > Sure. I'll add some documentation to the long portion of the kernel-doc > here describing priority order. Anyway, with this patch, I should be able to remove a lot of (ugly) code I have been writting lately. I'll be happy to test it when you have a v2 ready.
Quoting Jerome Brunet (2019-02-13 01:32:23) > On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote: > > > > +/** > > > > + * struct clk_parent_data - clk parent information > > > > + * @hw: parent clk_hw pointer (used for clk providers with internal > > > > clks) > > > > + * @name: parent name local to provider registering clk > > > > + * @fallback: globally unique parent name (used as a fallback) > > > > + */ > > > > +struct clk_parent_data { > > > > + struct clk_hw *hw; > > > > + const char *name; > > > > + const char *fallback; > > One last nitpick about this structure, because I did not figure it out at > first. > > 'fallback' is what we known as 'name' in CCF so far. > > What do you think about renaming 'fallback' to 'name' and 'name' to something > more obvious, like 'of_name' or 'fw_name' or something else ? Ok. I'm not super fond of assuming it's the DT specific, so maybe fw_name is good, or ext_name for external name? Or con_id to match clkdev? > > > > > > > If I understand correctly, .name and .fallback will be ignored if hw is > > > provided ? Maybe this should be documented somehow ? > > > > Sure. I'll add some documentation to the long portion of the kernel-doc > > here describing priority order. > > Anyway, with this patch, I should be able to remove a lot of (ugly) code I > have been writting lately. I'll be happy to test it when you have a v2 ready. > Great!
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 202902e64799..0cd90a2339ad 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -42,6 +42,13 @@ static LIST_HEAD(clk_notifier_list); /*** private data structures ***/ +struct clk_parent_map { + struct clk_hw *hw; + struct clk_core *core; + const char *name; + const char *fallback; +}; + struct clk_core { const char *name; const struct clk_ops *ops; @@ -49,8 +56,7 @@ struct clk_core { struct module *owner; struct device *dev; struct clk_core *parent; - const char **parent_names; - struct clk_core **parents; + struct clk_parent_map *parents; u8 num_parents; u8 new_parent_index; unsigned long rate; @@ -319,17 +325,77 @@ static struct clk_core *clk_core_lookup(const char *name) return NULL; } +/** + * clk_core_get - Find the parent of a clk using a clock specifier in DT + * @core: clk to find parent of + * @name: name to search for in 'clock-names' of device providing clk + * + * This is the preferred method for clk providers to find the parent of a + * clk when that parent is external to the clk controller. The parent_names + * array is indexed and treated as a local name matching a string in the device + * node's 'clock-names' property. This allows clk providers to use their own + * namespace instead of looking for a globally unique parent string. + * + * For example the following DT snippet would allow a clock registered by the + * clock-controller@c001 that has a clk_init_data::parent_data array + * with 'xtal' in the 'name' member to find the clock provided by the + * clock-controller@f00abcd without needing to get the globally unique name of + * the xtal clk. + * + * parent: clock-controller@f00abcd { + * reg = <0xf00abcd 0xabcd>; + * #clock-cells = <0>; + * }; + * + * clock-controller@c001 { + * reg = <0xc001 0xf00d>; + * clocks = <&parent>; + * clock-names = "xtal"; + * #clock-cells = <1>; + * }; + */ +static struct clk_core *clk_core_get(struct clk_core *core, const char *name) +{ + struct clk_hw *hw; + struct device *dev = core->dev; + + if (!dev) + return NULL; + + /* TODO: Support clkdev clk_lookups */ + hw = of_clk_get_hw(dev->of_node, -1, name); + if (IS_ERR_OR_NULL(hw)) + return NULL; + + return hw->core; +} + +static void clk_core_fill_parent_index(struct clk_core *core, u8 index) +{ + struct clk_parent_map *entry = &core->parents[index]; + struct clk_core *parent = NULL; + + if (entry->hw) + parent = entry->hw->core; + else if (entry->name) + parent = clk_core_get(core, entry->name); + + if (!parent) + parent = clk_core_lookup(entry->fallback); + + entry->core = parent; +} + static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core, u8 index) { - if (!core || index >= core->num_parents) + if (!core || index >= core->num_parents || !core->parents) return NULL; - if (!core->parents[index]) - core->parents[index] = - clk_core_lookup(core->parent_names[index]); + if (!core->parents[index].core) + clk_core_fill_parent_index(core, index); - return core->parents[index]; + return core->parents[index].core; } struct clk_hw * @@ -2353,6 +2419,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent) bool clk_has_parent(struct clk *clk, struct clk *parent) { struct clk_core *core, *parent_core; + int i; /* NULL clocks should be nops, so return success if either is NULL. */ if (!clk || !parent) @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk *parent) if (core->parent == parent_core) return true; - return match_string(core->parent_names, core->num_parents, - parent_core->name) >= 0; + for (i = 0; i < core->num_parents; i++) + if (!strcmp(core->parents[i].fallback, parent_core->name)) + return true; + + return false; } EXPORT_SYMBOL_GPL(clk_has_parent); @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s, void *data) int i; for (i = 0; i < core->num_parents - 1; i++) - seq_printf(s, "%s ", core->parent_names[i]); + seq_printf(s, "%s ", core->parents[i].fallback); - seq_printf(s, "%s\n", core->parent_names[i]); + seq_printf(s, "%s\n", core->parents[i].fallback); return 0; } @@ -3085,7 +3155,7 @@ static inline void clk_debug_unregister(struct clk_core *core) */ static int __clk_core_init(struct clk_core *core) { - int i, ret; + int ret; struct clk_core *orphan; struct hlist_node *tmp2; unsigned long rate; @@ -3139,12 +3209,6 @@ static int __clk_core_init(struct clk_core *core) goto out; } - /* throw a WARN if any entries in parent_names are NULL */ - for (i = 0; i < core->num_parents; i++) - WARN(!core->parent_names[i], - "%s: invalid NULL in %s's .parent_names\n", - __func__, core->name); - ret = clk_init_parent(core); if (ret) goto out; @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } +static int clk_core_populate_parent_map(struct clk_core *core) +{ + const struct clk_init_data *init = core->hw->init; + u8 num_parents = init->num_parents; + const char * const *parent_names = init->parent_names; + struct clk_hw **parent_hws = init->parent_hws; + const struct clk_parent_data *parent_data = init->parent_data; + int i, ret = 0; + struct clk_parent_map *parents, *parent; + + if (!num_parents) + return 0; + + /* + * Avoid unnecessary string look-ups of clk_core's possible parents by + * having a cache of names/clk_hw pointers to clk_core pointers. + */ + parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL); + core->parents = parents; + if (!parents) + return -ENOMEM; + + /* Copy everything over because it might be __initdata */ + for (i = 0, parent = parents; i < num_parents; i++, parent++) { + if (parent_names) { + /* throw a WARN if any entries are NULL */ + WARN(!parent_names[i], + "%s: invalid NULL in %s's .parent_names\n", + __func__, core->name); + parent->fallback = kstrdup_const(parent_names[i], + GFP_KERNEL); + if (!parent->fallback) { + ret = -ENOMEM; + while (--i >= 0) + kfree_const(parents[i].fallback); + } + } else if (parent_data) { + parent->hw = parent_data[i].hw; + parent->name = parent_data[i].name; + parent->fallback = parent_data[i].fallback; + } else if (parent_hws) { + parent->hw = parent_hws[i]; + } else { + ret = -EINVAL; + WARN(1, "Must specify parents if num_parents > 0\n"); + } + + if (ret) { + kfree(parents); + return ret; + } + } + + return 0; +} + +static void clk_core_free_parent_map(struct clk_core *core) +{ + int i = core->num_parents; + + if (!core->num_parents) + return; + + while (--i >= 0) + kfree_const(core->parents[i].fallback); + kfree(core->parents); +} + /** * clk_register - allocate a new clock, register it and return an opaque cookie * @dev: device that is registering this clock @@ -3373,7 +3505,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, */ struct clk *clk_register(struct device *dev, struct clk_hw *hw) { - int i, ret; + int ret; struct clk_core *core; core = kzalloc(sizeof(*core), GFP_KERNEL); @@ -3406,33 +3538,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) core->max_rate = ULONG_MAX; hw->core = core; - /* allocate local copy in case parent_names is __initdata */ - core->parent_names = kcalloc(core->num_parents, sizeof(char *), - GFP_KERNEL); - - if (!core->parent_names) { - ret = -ENOMEM; - goto fail_parent_names; - } - - - /* copy each string name in case parent_names is __initdata */ - for (i = 0; i < core->num_parents; i++) { - core->parent_names[i] = kstrdup_const(hw->init->parent_names[i], - GFP_KERNEL); - if (!core->parent_names[i]) { - ret = -ENOMEM; - goto fail_parent_names_copy; - } - } - - /* avoid unnecessary string look-ups of clk_core's possible parents. */ - core->parents = kcalloc(core->num_parents, sizeof(*core->parents), - GFP_KERNEL); - if (!core->parents) { - ret = -ENOMEM; + ret = clk_core_populate_parent_map(core); + if (ret) goto fail_parents; - }; INIT_HLIST_HEAD(&core->clks); @@ -3443,7 +3551,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) hw->clk = alloc_clk(core, NULL, NULL); if (IS_ERR(hw->clk)) { ret = PTR_ERR(hw->clk); - goto fail_parents; + goto fail_create_clk; } clk_core_link_consumer(hw->core, hw->clk); @@ -3459,13 +3567,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) free_clk(hw->clk); hw->clk = NULL; +fail_create_clk: + clk_core_free_parent_map(core); fail_parents: - kfree(core->parents); -fail_parent_names_copy: - while (--i >= 0) - kfree_const(core->parent_names[i]); - kfree(core->parent_names); -fail_parent_names: fail_ops: kfree_const(core->name); fail_name: @@ -3495,15 +3599,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register); static void __clk_release(struct kref *ref) { struct clk_core *core = container_of(ref, struct clk_core, ref); - int i = core->num_parents; lockdep_assert_held(&prepare_lock); - kfree(core->parents); - while (--i >= 0) - kfree_const(core->parent_names[i]); - - kfree(core->parent_names); + clk_core_free_parent_map(core); kfree_const(core->name); kfree(core); } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 8b84dee942bf..f513f4074a93 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -264,6 +264,18 @@ struct clk_ops { void (*debug_init)(struct clk_hw *hw, struct dentry *dentry); }; +/** + * struct clk_parent_data - clk parent information + * @hw: parent clk_hw pointer (used for clk providers with internal clks) + * @name: parent name local to provider registering clk + * @fallback: globally unique parent name (used as a fallback) + */ +struct clk_parent_data { + struct clk_hw *hw; + const char *name; + const char *fallback; +}; + /** * struct clk_init_data - holds init data that's common to all clocks and is * shared between the clock provider and the common clock framework. @@ -277,7 +289,10 @@ struct clk_ops { struct clk_init_data { const char *name; const struct clk_ops *ops; - const char * const *parent_names; + /* Only one of the following three should be assigned */ + const char * const *parent_names; /* If NULL (and num_parents != 0) look at parent_data and parent_hws */ + const struct clk_parent_data *parent_data; + struct clk_hw **parent_hws; u8 num_parents; unsigned long flags; };
The common clk framework is lacking in ability to describe the clk topology without specifying strings for every possible parent-child link. There are a few drawbacks to the current approach: 1) String comparisons are used for everything, including describing topologies that are 'local' to a single clock controller. 2) clk providers (e.g. i2c clk drivers) need to create globally unique clk names to avoid collisions in the clk namespace, leading to awkward name generation code in various clk drivers. 3) DT bindings may not fully describe the clk topology and linkages between clk controllers because drivers can easily rely on globally unique strings to describe connections between clks. This leads to confusing DT bindings, complicated clk name generation code, and inefficient string comparisons during clk registration just so that the clk framework can detect the topology of the clk tree. Furthermore, some drivers call clk_get() and then __clk_get_name() to extract the globally unique clk name just so they can specify the parent of the clk they're registering. We have of_clk_parent_fill() but that mostly only works for single clks registered from a DT node, which isn't the norm. Let's simplify this all by introducing two new ways of specifying clk parents. The first method is an array of pointers to clk_hw structures corresponding to the parents at that index. This works for clks that are registered when we have access to all the clk_hw pointers for the parents. The second method is a mix of clk_hw pointers and strings of local and global parent clk names. If the .name member of the map is set we'll look for that clk by performing a DT based lookup of the device the clk is registered with and the .name specified in the map. If that fails, we'll fallback to the .fallback member and perform a global clk name lookup like we've always done before. Using either one of these new methods is entirely optional. Existing drivers will continue to work, and they can migrate to this new approach as they see fit. Eventually, we'll want to get rid of the 'parent_names' array in struct clk_init_data and use one of these new methods instead. Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Jerome Brunet <jbrunet@baylibre.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Michael Turquette <mturquette@baylibre.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk.c | 215 +++++++++++++++++++++++++---------- include/linux/clk-provider.h | 17 ++- 2 files changed, 173 insertions(+), 59 deletions(-)