| Message ID | 20190226223429.193873-7-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Rewrite clk parent handling | expand |
On Tue, 2019-02-26 at 14:34 -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. Nitpick: I think you forgot to update the commit message when renaming the struct members > > 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. > Being able to specify parents from DT is great addition !!! Thx ! Overall, it looks good but with such big patch on framework is not easy get a clear idea. I'll try to give it a spin next week. > 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> > Cc: Jeffrey Hugo <jhugo@codeaurora.org> > Cc: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > include/linux/clk-provider.h | 19 +++ > 2 files changed, 217 insertions(+), 62 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 937b8d092d17..3d01e8c56400 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); > > /*** private data structures ***/ > > +struct clk_parent_map { > + const struct clk_hw *hw; > + struct clk_core *core; > + const char *fw_name; > + const char *name; > +}; > + > struct clk_core { > const char *name; > const struct clk_ops *ops; > @@ -46,8 +53,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; > @@ -316,17 +322,92 @@ 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>; > + * }; > + * > + * Returns: -ENOENT when the provider can't be found or the clk doesn't > + * exist in the provider. -EINVAL when the name can't be found. NULL when the > + * provider knows about the clk but it isn't provided on this system. > + * A valid clk_core pointer when the clk can be found in the provider. > + */ > +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 ERR_PTR(-ENOENT); > + > + /* TODO: Support clkdev clk_lookups */ > + hw = of_clk_get_hw(dev->of_node, -1, name); > + if (IS_ERR_OR_NULL(hw)) > + return ERR_CAST(hw); > + > + 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 = ERR_PTR(-ENOENT); > + > + if (entry->hw) { > + parent = entry->hw->core; > + /* > + * We have a direct reference but it isn't registered yet? Orphan it > + * and let clk_reparent() update the orphan status when the parent > + * is registered. > + */ > + if (!parent) > + parent = ERR_PTR(-EPROBE_DEFER); > + } else { > + if (entry->fw_name) > + parent = clk_core_get(core, entry->fw_name); > + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) > + parent = clk_core_lookup(entry->name); > + } > + > + /* Only cache it if it's not an error */ > + if (!IS_ERR(parent)) > + 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 * > @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core, > return -EINVAL; > > for (i = 0; i < core->num_parents; i++) { > - if (core->parents[i] == parent) > + if (core->parents[i].core == parent) > return i; > > - if (core->parents[i]) > + if (core->parents[i].core) > continue; > > /* Fallback to comparing globally unique names */ > - if (!strcmp(parent->name, core->parent_names[i])) { > - core->parents[i] = parent; > + if (!strcmp(parent->name, core->parents[i].name)) { > + core->parents[i].core = parent; > return i; > } > } > @@ -2290,6 +2371,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) > @@ -2302,8 +2384,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].name, parent_core->name)) > + return true; > + > + return false; > } > EXPORT_SYMBOL_GPL(clk_has_parent); > > @@ -2886,9 +2971,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].name); > > - seq_printf(s, "%s\n", core->parent_names[i]); > + seq_printf(s, "%s\n", core->parents[i].name); > > return 0; > } > @@ -3022,7 +3107,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; > @@ -3076,12 +3161,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); > - > core->parent = __clk_init_parent(core); > > /* > @@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > return clk; > } > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > +{ > + if (!src) { > + if (must_exist) > + return -EINVAL; > + return 0; > + } > + > + dst = kstrdup_const(src, GFP_KERNEL); > + if (!dst) > + return -ENOMEM; > + > + return 0; > +} > + > +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; > + const 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); > + ret = clk_cpy_name(parent->name, parent_names[i], > + true); > + } else if (parent_data) { > + parent->hw = parent_data[i].hw; > + ret = clk_cpy_name(parent->fw_name, > + parent_data[i].fw_name, false); > + if (!ret) > + ret = clk_cpy_name(parent->name, > + parent_data[i].name, > + false); > + } else if (parent_hws) { > + parent->hw = parent_hws[i]; > + } else { > + ret = -EINVAL; > + WARN(1, "Must specify parents if num_parents > 0\n"); > + } > + > + if (ret) { > + do { > + kfree_const(parents[i].name); > + kfree_const(parents[i].fw_name); > + } while (--i >= 0); > + 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].name); > + kfree_const(core->parents[i].fw_name); > + } > + > + kfree(core->parents); > +} > + > /** > * clk_register - allocate a new clock, register it and return an opaque cookie > * @dev: device that is registering this clock > @@ -3323,7 +3492,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); > @@ -3356,33 +3525,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); > > @@ -3393,7 +3538,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); > @@ -3409,13 +3554,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: > @@ -3445,15 +3586,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 e443fa9fa859..f4de52c6764e 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -250,6 +250,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) > + * @fw_name: parent name local to provider registering clk > + * @name: globally unique parent name (used as a fallback) > + */ > +struct clk_parent_data { > + const struct clk_hw *hw; > + const char *fw_name; > + const char *name; > +}; > + > /** > * 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. > @@ -257,13 +269,20 @@ struct clk_ops { > * @name: clock name > * @ops: operations this clock supports > * @parent_names: array of string names for all possible parents > + * @parent_data: array of parent data for all possible parents (when some > + * parents are external to the clk controller) > + * @parent_hws: array of pointers to all possible parents (when all parents > + * are internal to the clk controller) > * @num_parents: number of possible parents > * @flags: framework-level hints and quirks > */ > struct clk_init_data { > const char *name; > const struct clk_ops *ops; > + /* Only one of the following three should be assigned */ > const char * const *parent_names; > + const struct clk_parent_data *parent_data; > + const struct clk_hw **parent_hws; > u8 num_parents; > unsigned long flags; > };
On 2/26/2019 3:34 PM, 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. > I don't know exactly what regressed from V1, but this change breaks all clock drivers as far as I can tell. All clocks from old and new (ie 8998 mmcc rebased onto this) drivers end up as orphans. Is there some data I can provide to help you figure out the issue? > 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> > Cc: Jeffrey Hugo <jhugo@codeaurora.org> > Cc: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > include/linux/clk-provider.h | 19 +++ > 2 files changed, 217 insertions(+), 62 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 937b8d092d17..3d01e8c56400 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); > > /*** private data structures ***/ > > +struct clk_parent_map { > + const struct clk_hw *hw; > + struct clk_core *core; > + const char *fw_name; > + const char *name; > +}; > + > struct clk_core { > const char *name; > const struct clk_ops *ops; > @@ -46,8 +53,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; > @@ -316,17 +322,92 @@ 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>; > + * }; > + * > + * Returns: -ENOENT when the provider can't be found or the clk doesn't > + * exist in the provider. -EINVAL when the name can't be found. NULL when the > + * provider knows about the clk but it isn't provided on this system. > + * A valid clk_core pointer when the clk can be found in the provider. > + */ > +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 ERR_PTR(-ENOENT); > + > + /* TODO: Support clkdev clk_lookups */ > + hw = of_clk_get_hw(dev->of_node, -1, name); > + if (IS_ERR_OR_NULL(hw)) > + return ERR_CAST(hw); > + > + 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 = ERR_PTR(-ENOENT); > + > + if (entry->hw) { > + parent = entry->hw->core; > + /* > + * We have a direct reference but it isn't registered yet? Orphan it > + * and let clk_reparent() update the orphan status when the parent > + * is registered. > + */ > + if (!parent) > + parent = ERR_PTR(-EPROBE_DEFER); > + } else { > + if (entry->fw_name) > + parent = clk_core_get(core, entry->fw_name); > + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) > + parent = clk_core_lookup(entry->name); > + } > + > + /* Only cache it if it's not an error */ > + if (!IS_ERR(parent)) > + 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 * > @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core, > return -EINVAL; > > for (i = 0; i < core->num_parents; i++) { > - if (core->parents[i] == parent) > + if (core->parents[i].core == parent) > return i; > > - if (core->parents[i]) > + if (core->parents[i].core) > continue; > > /* Fallback to comparing globally unique names */ > - if (!strcmp(parent->name, core->parent_names[i])) { > - core->parents[i] = parent; > + if (!strcmp(parent->name, core->parents[i].name)) { > + core->parents[i].core = parent; > return i; > } > } > @@ -2290,6 +2371,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) > @@ -2302,8 +2384,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].name, parent_core->name)) > + return true; > + > + return false; > } > EXPORT_SYMBOL_GPL(clk_has_parent); > > @@ -2886,9 +2971,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].name); > > - seq_printf(s, "%s\n", core->parent_names[i]); > + seq_printf(s, "%s\n", core->parents[i].name); > > return 0; > } > @@ -3022,7 +3107,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; > @@ -3076,12 +3161,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); > - > core->parent = __clk_init_parent(core); > > /* > @@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > return clk; > } > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > +{ > + if (!src) { > + if (must_exist) > + return -EINVAL; > + return 0; > + } > + > + dst = kstrdup_const(src, GFP_KERNEL); > + if (!dst) > + return -ENOMEM; > + > + return 0; > +} > + > +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; > + const 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); > + ret = clk_cpy_name(parent->name, parent_names[i], > + true); > + } else if (parent_data) { > + parent->hw = parent_data[i].hw; > + ret = clk_cpy_name(parent->fw_name, > + parent_data[i].fw_name, false); > + if (!ret) > + ret = clk_cpy_name(parent->name, > + parent_data[i].name, > + false); > + } else if (parent_hws) { > + parent->hw = parent_hws[i]; > + } else { > + ret = -EINVAL; > + WARN(1, "Must specify parents if num_parents > 0\n"); > + } > + > + if (ret) { > + do { > + kfree_const(parents[i].name); > + kfree_const(parents[i].fw_name); > + } while (--i >= 0); > + 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].name); > + kfree_const(core->parents[i].fw_name); > + } > + > + kfree(core->parents); > +} > + > /** > * clk_register - allocate a new clock, register it and return an opaque cookie > * @dev: device that is registering this clock > @@ -3323,7 +3492,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); > @@ -3356,33 +3525,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); > > @@ -3393,7 +3538,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); > @@ -3409,13 +3554,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: > @@ -3445,15 +3586,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 e443fa9fa859..f4de52c6764e 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -250,6 +250,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) > + * @fw_name: parent name local to provider registering clk > + * @name: globally unique parent name (used as a fallback) > + */ > +struct clk_parent_data { > + const struct clk_hw *hw; > + const char *fw_name; > + const char *name; > +}; > + > /** > * 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. > @@ -257,13 +269,20 @@ struct clk_ops { > * @name: clock name > * @ops: operations this clock supports > * @parent_names: array of string names for all possible parents > + * @parent_data: array of parent data for all possible parents (when some > + * parents are external to the clk controller) > + * @parent_hws: array of pointers to all possible parents (when all parents > + * are internal to the clk controller) > * @num_parents: number of possible parents > * @flags: framework-level hints and quirks > */ > struct clk_init_data { > const char *name; > const struct clk_ops *ops; > + /* Only one of the following three should be assigned */ > const char * const *parent_names; > + const struct clk_parent_data *parent_data; > + const struct clk_hw **parent_hws; > u8 num_parents; > unsigned long flags; > }; >
Quoting Jeffrey Hugo (2019-03-02 13:25:06) > On 2/26/2019 3:34 PM, 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. > > > > I don't know exactly what regressed from V1, but this change breaks all > clock drivers as far as I can tell. All clocks from old and new (ie > 8998 mmcc rebased onto this) drivers end up as orphans. > > Is there some data I can provide to help you figure out the issue? > Can you try this patch? It fixes a pointer blunder that I'm sad about. ----8<----- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 37aea7884166..d12afd256dc5 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3297,15 +3297,17 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } -static int clk_cpy_name(const char *dst, const char *src, bool must_exist) +static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) { + const char *dst; + if (!src) { if (must_exist) return -EINVAL; return 0; } - dst = kstrdup_const(src, GFP_KERNEL); + *dst_p = dst = kstrdup_const(src, GFP_KERNEL); if (!dst) return -ENOMEM; @@ -3341,14 +3343,14 @@ static int clk_core_populate_parent_map(struct clk_core *core) WARN(!parent_names[i], "%s: invalid NULL in %s's .parent_names\n", __func__, core->name); - ret = clk_cpy_name(parent->name, parent_names[i], + ret = clk_cpy_name(&parent->name, parent_names[i], true); } else if (parent_data) { parent->hw = parent_data[i].hw; - ret = clk_cpy_name(parent->fw_name, + ret = clk_cpy_name(&parent->fw_name, parent_data[i].fw_name, false); if (!ret) - ret = clk_cpy_name(parent->name, + ret = clk_cpy_name(&parent->name, parent_data[i].name, false); } else if (parent_hws) {
Quoting Jerome Brunet (2019-03-02 10:40:42) > On Tue, 2019-02-26 at 14:34 -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. > > Nitpick: I think you forgot to update the commit message when renaming the > struct members Fixed. Thanks! > > > > > 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. > > > > Being able to specify parents from DT is great addition !!! Thx ! Woohooo! > > Overall, it looks good but with such big patch on framework is not easy get a > clear idea. I'll try to give it a spin next week. Ok. Please be aware that I found one bug already but there are probably more lurking. I've also pushed the branch out to clk.git on kernel.org if you're interested in looking at the mass conversions going on. https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite
On 3/6/2019 10:48 AM, Stephen Boyd wrote: > Quoting Jeffrey Hugo (2019-03-02 13:25:06) >> On 2/26/2019 3:34 PM, 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. >>> >> >> I don't know exactly what regressed from V1, but this change breaks all >> clock drivers as far as I can tell. All clocks from old and new (ie >> 8998 mmcc rebased onto this) drivers end up as orphans. >> >> Is there some data I can provide to help you figure out the issue? >> > > Can you try this patch? It fixes a pointer blunder that I'm sad about. That did the trick. Everything seems to work again. I haven't identified any additional issues. > > ----8<----- > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 37aea7884166..d12afd256dc5 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3297,15 +3297,17 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > return clk; > } > > -static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > +static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) > { > + const char *dst; > + > if (!src) { > if (must_exist) > return -EINVAL; > return 0; > } > > - dst = kstrdup_const(src, GFP_KERNEL); > + *dst_p = dst = kstrdup_const(src, GFP_KERNEL); > if (!dst) > return -ENOMEM; > > @@ -3341,14 +3343,14 @@ static int clk_core_populate_parent_map(struct clk_core *core) > WARN(!parent_names[i], > "%s: invalid NULL in %s's .parent_names\n", > __func__, core->name); > - ret = clk_cpy_name(parent->name, parent_names[i], > + ret = clk_cpy_name(&parent->name, parent_names[i], > true); > } else if (parent_data) { > parent->hw = parent_data[i].hw; > - ret = clk_cpy_name(parent->fw_name, > + ret = clk_cpy_name(&parent->fw_name, > parent_data[i].fw_name, false); > if (!ret) > - ret = clk_cpy_name(parent->name, > + ret = clk_cpy_name(&parent->name, > parent_data[i].name, > false); > } else if (parent_hws) { >
On Tue, 2019-02-26 at 14:34 -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. > > 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> > Cc: Jeffrey Hugo <jhugo@codeaurora.org> > Cc: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > include/linux/clk-provider.h | 19 +++ > 2 files changed, 217 insertions(+), 62 deletions(-) Sorry for the delay. With the fix you sent to Jeffrey Tested by porting the aoclk controller of Amlogic g12a SoC. This allowed to test * hws only table * parent_data with a mix of hw pointers and fw_name (with different input controllers and also an input that is optional and never provided) Tested-by: Jerome Brunet <jbrunet@baylibre.com> With the small comment below Reviewed-by Jerome Brunet <jbrunet@baylibre.com> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 937b8d092d17..3d01e8c56400 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); > [...] > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > +{ > + if (!src) { > + if (must_exist) > + return -EINVAL; > + return 0; > + } > + > + dst = kstrdup_const(src, GFP_KERNEL); > + if (!dst) > + return -ENOMEM; > + > + return 0; > +} > + > +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; > + const 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); > + ret = clk_cpy_name(parent->name, parent_names[i], > + true); > + } else if (parent_data) { While testing, I mistakenly left both parent_names and parent_data. I was surprised that parent_data did not take precedence of parent_names. Maybe it should ? (... but I understand we are not supposed to provide both) > + parent->hw = parent_data[i].hw; > + ret = clk_cpy_name(parent->fw_name, > + parent_data[i].fw_name, false); > + if (!ret) > + ret = clk_cpy_name(parent->name, > + parent_data[i].name, > + false); > + } else if (parent_hws) { > + parent->hw = parent_hws[i]; > + } else { Maybe there should also some kinda of check to verify if more than one option (among hws, parent_data and parent_names) was provided and throw a warn ? Could be useful with drivers move away from parent_names ? > + ret = -EINVAL; > + WARN(1, "Must specify parents if num_parents > 0\n"); > + } > + > + if (ret) { > + do { > + kfree_const(parents[i].name); > + kfree_const(parents[i].fw_name); > + } while (--i >= 0); > + kfree(parents); > + > + return ret; > + } > + } > + > + return 0; > +} > + >
Quoting Jerome Brunet (2019-03-15 03:01:53) > On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote: > > --- > > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > > include/linux/clk-provider.h | 19 +++ > > 2 files changed, 217 insertions(+), 62 deletions(-) > > Sorry for the delay. > > With the fix you sent to Jeffrey > Tested by porting the aoclk controller of Amlogic g12a SoC. > This allowed to test > * hws only table > * parent_data with a mix of hw pointers and fw_name (with different input > controllers and also an input that is optional and never provided) > > Tested-by: Jerome Brunet <jbrunet@baylibre.com> > > With the small comment below > > Reviewed-by Jerome Brunet <jbrunet@baylibre.com> > Awesome. Thanks! I need to Cc Rob H to hopefully get an ack on the concept of relying on DT so I'll resend this series again next week. It would also be nice if I can throw in a couple more patches to let drivers specify a DT node when registering a clk if they don't have a struct device on hand and let drivers lookup with clk_lookups somehow. > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 937b8d092d17..3d01e8c56400 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); > > > > [...] > > > > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > > +{ > > + if (!src) { > > + if (must_exist) > > + return -EINVAL; > > + return 0; > > + } > > + > > + dst = kstrdup_const(src, GFP_KERNEL); > > + if (!dst) > > + return -ENOMEM; > > + > > + return 0; > > +} > > + > > +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; > > + const 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); > > + ret = clk_cpy_name(parent->name, parent_names[i], > > + true); > > + } else if (parent_data) { > > While testing, I mistakenly left both parent_names and parent_data. I was > surprised that parent_data did not take precedence of parent_names. > > Maybe it should ? (... but I understand we are not supposed to provide both) I don't think we can. We have a problem where drivers don't initialize the init structure properly, opting to just throw it on the stack and leave junk in there that they overwrite. We'd have to go through all the init structures and initialize them. I suppose we could make a macro for that: DECLARE_CLK_INIT_DATA(init); or something like that that does this. We could bury a magic number in there under some debug option too so that we can make sure drivers are doing this properly. Otherwise we're left to doing these weird tricks like I've done here. Regardless. I'll have to add a comment to this fact in the code. Thanks. > > > + parent->hw = parent_data[i].hw; > > + ret = clk_cpy_name(parent->fw_name, > > + parent_data[i].fw_name, false); > > + if (!ret) > > + ret = clk_cpy_name(parent->name, > > + parent_data[i].name, > > + false); > > + } else if (parent_hws) { > > + parent->hw = parent_hws[i]; > > + } else { > > Maybe there should also some kinda of check to verify if more than one option > (among hws, parent_data and parent_names) was provided and throw a warn ? > > Could be useful with drivers move away from parent_names ? Same thing. It would be nice but we're sort of unable to do so unless we do what I suggest above. Should we do it?
On Fri, 2019-03-15 at 10:16 -0700, Stephen Boyd wrote: > Quoting Jerome Brunet (2019-03-15 03:01:53) > > On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote: > > > --- > > > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > > > include/linux/clk-provider.h | 19 +++ > > > 2 files changed, 217 insertions(+), 62 deletions(-) > > > > Sorry for the delay. > > > > With the fix you sent to Jeffrey > > Tested by porting the aoclk controller of Amlogic g12a SoC. > > This allowed to test > > * hws only table > > * parent_data with a mix of hw pointers and fw_name (with different input > > controllers and also an input that is optional and never provided) > > > > Tested-by: Jerome Brunet <jbrunet@baylibre.com> > > > > With the small comment below > > > > Reviewed-by Jerome Brunet <jbrunet@baylibre.com> > > > > Awesome. Thanks! I need to Cc Rob H to hopefully get an ack on the > concept of relying on DT so I'll resend this series again next week. It > would also be nice if I can throw in a couple more patches to let > drivers specify a DT node when registering a clk if they don't have a > struct device on hand and let drivers lookup with clk_lookups somehow. > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 937b8d092d17..3d01e8c56400 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); > > > > > > > [...] > > > > > > > > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) > > > +{ > > > + if (!src) { > > > + if (must_exist) > > > + return -EINVAL; > > > + return 0; > > > + } > > > + > > > + dst = kstrdup_const(src, GFP_KERNEL); > > > + if (!dst) > > > + return -ENOMEM; > > > + > > > + return 0; > > > +} > > > + > > > +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; > > > + const 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); > > > + ret = clk_cpy_name(parent->name, parent_names[i], > > > + true); > > > + } else if (parent_data) { > > > > While testing, I mistakenly left both parent_names and parent_data. I was > > surprised that parent_data did not take precedence of parent_names. > > > > Maybe it should ? (... but I understand we are not supposed to provide both) > > I don't think we can. We have a problem where drivers don't initialize > the init structure properly, opting to just throw it on the stack and > leave junk in there that they overwrite. We'd have to go through all the > init structures and initialize them. I suppose we could make a macro for > that: > > DECLARE_CLK_INIT_DATA(init); > > or something like that that does this. We could bury a magic number in > there under some debug option too so that we can make sure drivers are > doing this properly. Otherwise we're left to doing these weird tricks > like I've done here. > > Regardless. I'll have to add a comment to this fact in the code. Thanks. > > > > + parent->hw = parent_data[i].hw; > > > + ret = clk_cpy_name(parent->fw_name, > > > + parent_data[i].fw_name, false); > > > + if (!ret) > > > + ret = clk_cpy_name(parent->name, > > > + parent_data[i].name, > > > + false); > > > + } else if (parent_hws) { > > > + parent->hw = parent_hws[i]; > > > + } else { > > > > Maybe there should also some kinda of check to verify if more than one option > > (among hws, parent_data and parent_names) was provided and throw a warn ? > > > > Could be useful with drivers move away from parent_names ? > > Same thing. It would be nice but we're sort of unable to do so unless we > do what I suggest above. Should we do it? > I was not thinking about anything complicated: * Among the 3 pointers, just throw a warn if more than one is not NULL * In the if/elseif/else, I would have put parent_data before parent_names Nothing critical about that comment though
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 937b8d092d17..3d01e8c56400 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list); /*** private data structures ***/ +struct clk_parent_map { + const struct clk_hw *hw; + struct clk_core *core; + const char *fw_name; + const char *name; +}; + struct clk_core { const char *name; const struct clk_ops *ops; @@ -46,8 +53,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; @@ -316,17 +322,92 @@ 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>; + * }; + * + * Returns: -ENOENT when the provider can't be found or the clk doesn't + * exist in the provider. -EINVAL when the name can't be found. NULL when the + * provider knows about the clk but it isn't provided on this system. + * A valid clk_core pointer when the clk can be found in the provider. + */ +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 ERR_PTR(-ENOENT); + + /* TODO: Support clkdev clk_lookups */ + hw = of_clk_get_hw(dev->of_node, -1, name); + if (IS_ERR_OR_NULL(hw)) + return ERR_CAST(hw); + + 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 = ERR_PTR(-ENOENT); + + if (entry->hw) { + parent = entry->hw->core; + /* + * We have a direct reference but it isn't registered yet? Orphan it + * and let clk_reparent() update the orphan status when the parent + * is registered. + */ + if (!parent) + parent = ERR_PTR(-EPROBE_DEFER); + } else { + if (entry->fw_name) + parent = clk_core_get(core, entry->fw_name); + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) + parent = clk_core_lookup(entry->name); + } + + /* Only cache it if it's not an error */ + if (!IS_ERR(parent)) + 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 * @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core, return -EINVAL; for (i = 0; i < core->num_parents; i++) { - if (core->parents[i] == parent) + if (core->parents[i].core == parent) return i; - if (core->parents[i]) + if (core->parents[i].core) continue; /* Fallback to comparing globally unique names */ - if (!strcmp(parent->name, core->parent_names[i])) { - core->parents[i] = parent; + if (!strcmp(parent->name, core->parents[i].name)) { + core->parents[i].core = parent; return i; } } @@ -2290,6 +2371,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) @@ -2302,8 +2384,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].name, parent_core->name)) + return true; + + return false; } EXPORT_SYMBOL_GPL(clk_has_parent); @@ -2886,9 +2971,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].name); - seq_printf(s, "%s\n", core->parent_names[i]); + seq_printf(s, "%s\n", core->parents[i].name); return 0; } @@ -3022,7 +3107,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; @@ -3076,12 +3161,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); - core->parent = __clk_init_parent(core); /* @@ -3310,6 +3389,96 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } +static int clk_cpy_name(const char *dst, const char *src, bool must_exist) +{ + if (!src) { + if (must_exist) + return -EINVAL; + return 0; + } + + dst = kstrdup_const(src, GFP_KERNEL); + if (!dst) + return -ENOMEM; + + return 0; +} + +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; + const 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); + ret = clk_cpy_name(parent->name, parent_names[i], + true); + } else if (parent_data) { + parent->hw = parent_data[i].hw; + ret = clk_cpy_name(parent->fw_name, + parent_data[i].fw_name, false); + if (!ret) + ret = clk_cpy_name(parent->name, + parent_data[i].name, + false); + } else if (parent_hws) { + parent->hw = parent_hws[i]; + } else { + ret = -EINVAL; + WARN(1, "Must specify parents if num_parents > 0\n"); + } + + if (ret) { + do { + kfree_const(parents[i].name); + kfree_const(parents[i].fw_name); + } while (--i >= 0); + 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].name); + kfree_const(core->parents[i].fw_name); + } + + kfree(core->parents); +} + /** * clk_register - allocate a new clock, register it and return an opaque cookie * @dev: device that is registering this clock @@ -3323,7 +3492,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); @@ -3356,33 +3525,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); @@ -3393,7 +3538,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); @@ -3409,13 +3554,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: @@ -3445,15 +3586,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 e443fa9fa859..f4de52c6764e 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -250,6 +250,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) + * @fw_name: parent name local to provider registering clk + * @name: globally unique parent name (used as a fallback) + */ +struct clk_parent_data { + const struct clk_hw *hw; + const char *fw_name; + const char *name; +}; + /** * 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. @@ -257,13 +269,20 @@ struct clk_ops { * @name: clock name * @ops: operations this clock supports * @parent_names: array of string names for all possible parents + * @parent_data: array of parent data for all possible parents (when some + * parents are external to the clk controller) + * @parent_hws: array of pointers to all possible parents (when all parents + * are internal to the clk controller) * @num_parents: number of possible parents * @flags: framework-level hints and quirks */ struct clk_init_data { const char *name; const struct clk_ops *ops; + /* Only one of the following three should be assigned */ const char * const *parent_names; + const struct clk_parent_data *parent_data; + const 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> Cc: Jeffrey Hugo <jhugo@codeaurora.org> Cc: Chen-Yu Tsai <wens@csie.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- include/linux/clk-provider.h | 19 +++ 2 files changed, 217 insertions(+), 62 deletions(-)