[PATCH v3] clk: respect the clock dependencies in of_clk_init

[mturquette@linaro.org (Mike Turquette)]

Quoting Gregory CLEMENT (2014-02-28 02:46:12)
> Hi Mike,
> 
> On 24/02/2014 19:10, Gregory CLEMENT wrote:
> > Until now the clock providers were initialized in the order found in
> > the device tree. This led to have the dependencies between the clocks
> > not respected: children clocks could be initialized before their
> > parent clocks.
> > 
> > Instead of forcing each platform to manage its own initialization order,
> > this patch adds this work inside the framework itself.
> > 
> > Using the data of the device tree the of_clk_init function now delayed
> > the initialization of a clock provider if its parent provider was not
> > ready yet.
> > 
> > The strict dependency check (all parents of a given clk must be
> > initialized) was added by Boris BREZILLON
> 
> Are you ok with this version?
> Will you take it for 3.15?
> Or maybe you expected that it will be part of a pull request?
> 
> However as it is modifying the core of the framework I thought that you
> would take it and apply yourself.

Hi Gregory,

I have taken this into clk-next. If no regressions pop up over the next
few days then it should go into 3.15.

Thanks,
Mike

> 
> Thanks,
> 
> Gregory
> 
> > 
> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> > ---
> > Mike,
> > 
> > This patch depend on the patch "clk: return probe defer when DT clock
> > not yet ready": http://article.gmane.org/gmane.linux.kernel/1643466
> > 
> > If for any reason you don't want to take it, then I will write a new
> > version closer to the v2 for the parent_ready() function.
> > 
> > I have taken into account all the remarks from Tomasz.
> > 
> > Thanks,
> > 
> > Changelog:
> > v2->v3:
> > 
> > - added the SoB flag of Boris
> > - used of_clk_get(np, i) in the parent_ready() function.
> > - simplified the loops to manage the order dependencies in of_clk_init()
> > 
> > v1 -> v2:
> > Merged the  strict dependency check from Boris.
> > 
> > 
> >  drivers/clk/clk.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 79 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 32d84e921b23..3a07f8ac2e11 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2526,24 +2526,99 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
> >  
> > +struct clock_provider {
> > +     of_clk_init_cb_t clk_init_cb;
> > +     struct device_node *np;
> > +     struct list_head node;
> > +};
> > +
> > +static LIST_HEAD(clk_provider_list);
> > +
> > +/*
> > + * This function looks for a parent clock. If there is one, then it
> > + * checks that the provider for this parent clock was initialized, in
> > + * this case the parent clock will be ready.
> > + */
> > +static int parent_ready(struct device_node *np)
> > +{
> > +     int i = 0;
> > +
> > +     while (true) {
> > +             struct clk *clk = of_clk_get(np, i);
> > +
> > +             /* this parent is ready we can check the next one */
> > +             if (!IS_ERR(clk)) {
> > +                     clk_put(clk);
> > +                     i++;
> > +                     continue;
> > +             }
> > +
> > +             /* at least one parent is not ready, we exit now */
> > +             if (PTR_ERR(clk) == -EPROBE_DEFER)
> > +                     return 0;
> > +
> > +             /*
> > +              * Here we make assumption that the device tree is
> > +              * written correctly. So an error means that there is
> > +              * no more parent. As we didn't exit yet, then the
> > +              * previous parent are ready. If there is no clock
> > +              * parent, no need to wait for them, then we can
> > +              * consider their absence as being ready
> > +              */
> > +             return 1;
> > +     }
> > +}
> > +
> >  /**
> >   * of_clk_init() - Scan and init clock providers from the DT
> >   * @matches: array of compatible values and init functions for providers.
> >   *
> > - * This function scans the device tree for matching clock providers and
> > - * calls their initialization functions
> > + * This function scans the device tree for matching clock providers
> > + * and calls their initialization functions. It also do it by trying
> > + * to follow the dependencies.
> >   */
> >  void __init of_clk_init(const struct of_device_id *matches)
> >  {
> >       const struct of_device_id *match;
> >       struct device_node *np;
> > +     struct clock_provider *clk_provider, *next;
> > +     bool is_init_done;
> > +     bool force = false;
> >  
> >       if (!matches)
> >               matches = &__clk_of_table;
> >  
> > +     /* First prepare the list of the clocks providers */
> >       for_each_matching_node_and_match(np, matches, &match) {
> > -             of_clk_init_cb_t clk_init_cb = match->data;
> > -             clk_init_cb(np);
> > +             struct clock_provider *parent =
> > +                     kzalloc(sizeof(struct clock_provider),  GFP_KERNEL);
> > +
> > +             parent->clk_init_cb = match->data;
> > +             parent->np = np;
> > +             list_add(&parent->node, &clk_provider_list);
> > +     }
> > +
> > +     while (!list_empty(&clk_provider_list)) {
> > +             is_init_done = false;
> > +             list_for_each_entry_safe(clk_provider, next,
> > +                                     &clk_provider_list, node) {
> > +                     if (force || parent_ready(clk_provider->np)) {
> > +                             clk_provider->clk_init_cb(clk_provider->np);
> > +                             list_del(&clk_provider->node);
> > +                             kfree(clk_provider);
> > +                             is_init_done = true;
> > +                     }
> > +             }
> > +
> > +             /*
> > +              * We didn't managed to initialize any of the
> > +              * remaining providers during the last loop, so now we
> > +              * initialize all the remaining ones unconditionally
> > +              * in case the clock parent was not mandatory
> > +              */
> > +             if (!is_init_done)
> > +                     force = true;
> > +
> >       }
> >  }
> >  #endif
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com