Messages in this thread | | | Subject | Re: [PATCH] clk: walk orphan list on clock provider registration | From | Stephen Boyd <> | Date | Mon, 02 Dec 2019 10:19:22 -0800 |
| |
Quoting Jerome Brunet (2019-11-29 08:16:58) > So far, we walked the orphan list every time a new clock was registered > in CCF. This was fine since the clocks were only referenced by name. > > Now that the clock can be referenced through DT, it is not enough: > * Controller A register first a reference clocks from controller B > through DT. > * Controller B register all its clocks then register the provider. > > Each time controller B registers a new clock, the orphan list is walked > but it can't match since the provider is registered yet. When the > provider is finally registered, the orphan list is not walked unless > another clock is registered afterward. > > This can lead to situation where some clocks remain orphaned even if > the parent is available. > > Walking the orphan list on provider registration solves the problem. > > Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > ---
Sounds right. Thanks for making the fix!
I suspect there should be a reported-by tag though?
> drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 22 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index ef4416721777..917ba37c3b9d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core) > } > #endif > > +static void __clk_core_reparent_orphan(void)
Maybe drop the double underscore. clk_core prefix already means "private to this file".
> +{ > + struct clk_core *orphan; > + struct hlist_node *tmp2; > + > + /* > + * walk the list of orphan clocks and reparent any that newly finds a > + * parent. > + */ > + hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) { > + struct clk_core *parent = __clk_init_parent(orphan); > + > + /* > + * We need to use __clk_set_parent_before() and _after() to > + * to properly migrate any prepare/enable count of the orphan > + * clock. This is important for CLK_IS_CRITICAL clocks, which > + * are enabled during init but might not have a parent yet. > + */ > + if (parent) { > + /* update the clk tree topology */ > + __clk_set_parent_before(orphan, parent); > + __clk_set_parent_after(orphan, parent, NULL); > + __clk_recalc_accuracies(orphan); > + __clk_recalc_rates(orphan, 0); > + } > + } > +} > + > /** > * __clk_core_init - initialize the data structures in a struct clk_core > * @core: clk_core being initialized > @@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core) > static int __clk_core_init(struct clk_core *core) > { > int ret; > - struct clk_core *orphan; > - struct hlist_node *tmp2; > unsigned long rate; > > if (!core) > @@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core) > clk_enable_unlock(flags); > } > > - /* > - * walk the list of orphan clocks and reparent any that newly finds a > - * parent. > - */ > - hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) { > - struct clk_core *parent = __clk_init_parent(orphan); > + __clk_core_reparent_orphan(); > > - /* > - * We need to use __clk_set_parent_before() and _after() to > - * to properly migrate any prepare/enable count of the orphan > - * clock. This is important for CLK_IS_CRITICAL clocks, which > - * are enabled during init but might not have a parent yet. > - */ > - if (parent) { > - /* update the clk tree topology */ > - __clk_set_parent_before(orphan, parent); > - __clk_set_parent_after(orphan, parent, NULL); > - __clk_recalc_accuracies(orphan); > - __clk_recalc_rates(orphan, 0); > - } > - } > > kref_init(&core->ref); > out: > @@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np, > mutex_unlock(&of_clk_mutex); > pr_debug("Added clock from %pOF\n", np); > > + clk_prepare_lock(); > + __clk_core_reparent_orphan(); > + clk_prepare_unlock(); > +
Maybe make a locked version of this function and an unlocked version?
> ret = of_clk_set_defaults(np, true); > if (ret < 0) > of_clk_del_provider(np); > @@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np, > mutex_unlock(&of_clk_mutex); > pr_debug("Added clk_hw provider from %pOF\n", np); > > + clk_prepare_lock(); > + __clk_core_reparent_orphan(); > + clk_prepare_unlock(); > +
So we don't duplicate this twice.
> ret = of_clk_set_defaults(np, true); > if (ret < 0)
| |