Re: [PATCH v2] clk: respect the clock dependencies in of_clk_init

[Mike Turquette]

Quoting Tomasz Figa (2014-02-23 10:46:35)
> Hi Gregory,
> 
> On 10.02.2014 18:42, 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.
> 
> In general this is really great. It's a first step towards sorting out 
> dependencies between clock providers correctly. I have some comments 
> inline, though.

Just to add in here, I think the approach is good but agree with Tomasz'
review comments.

Regards,
Mike

> 
> >
> > The strict dependency check (all parents of a given clk must be
> > initialized) was added by Boris BREZILLON
> 
> Shouldn't this be reflected by a tag of this patch? If you squash a 
> patch signed off by someone then I believe their sign-off tag should be 
> added to the base patch. Correct me if I'm wrong, though.
> 
> >
> > Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Since the v1, I have merged the  strict dependency check from Boris.
> > And of course tested on my Armada 370 and Armada XP based board
> >
> >   drivers/clk/clk.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 106 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 5517944495d8..684976993297 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2526,24 +2526,127 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> >   }
> >   EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
> >
> > +struct clock_provider {
> 
> The name is a bit too generic and slightly misleading. IMHO struct 
> deferred_clk_provider (and deferred_clk_providers for the list) would be 
> better.
> 
> > +     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)
> > +{
> > +     struct of_phandle_args clkspec;
> > +     struct of_clk_provider *provider;
> > +     int num_parents;
> > +     bool found;
> > +     int i;
> > +
> > +     /*
> > +      * If there is no clock parent, no need to wait for them, then
> > +      * we can consider their absence as being ready
> > +      */
> > +     num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> > +     if (num_parents <= 0)
> > +             return 1;
> 
> of_clk_get_parent_count() can be used here...
> 
> > +
> > +     for (i = 0; i < num_parents; i++) {
> > +             if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> > +                                            &clkspec))
> > +                     return 1;
> > +
> > +             /* Check if we have such a provider in our array */
> > +             found = false;
> > +             list_for_each_entry(provider, &of_clk_providers, link) {
> > +                     if (provider->node == clkspec.np) {
> > +                             found = true;
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             if (!found)
> > +                     return 0;
> > +     }
> 
> ...or even better, __of_clk_get_from_provider() could modified to return 
> -EPROBE_DEFER if requested provider is not registered and you could 
> simply call of_clk_get(np, i) and handle its return value appropriately:
> 
> - on !IS_ERR(clk) call clk_put() and continue with iterations,
> - on IS_ERR(clk) && PTR_ERR(clk) == -EPROBE_DEFER return 0 immediately,
> - in any other case end the loop (end of clock specifiers).
> 
> This would make CCF even closer to proper handling of provider ordering, 
> with a nice side effect of handling deferred probe for platform devices.
> 
> > +
> > +     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;
> >
> >       if (!matches)
> >               matches = &__clk_of_table;
> >
> >       for_each_matching_node_and_match(np, matches, &match) {
> >               of_clk_init_cb_t clk_init_cb = match->data;
> > -             clk_init_cb(np);
> > +
> > +
> > +             if (parent_ready(np)) {
> > +                     /*
> > +                      * The parent clock is ready or there is no
> > +                      * clock parent at all, in this case the
> > +                      * provider can be initialize immediately.
> > +                      */
> > +                     clk_init_cb(np);
> > +             } else {
> > +                     /*
> > +                      * The parent clock is not ready, this
> > +                      * provider is moved to a list to be
> > +                      * initialized later
> > +                      */
> > +                     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);
> > +             }
> > +     }
> 
> I wonder if this couldn't be replaced with simply adding all the 
> providers to the list first and then proceeding with the loop below to 
> handle the registrations.
> 
> > +
> > +     while (!list_empty(&clk_provider_list)) {
> > +             is_init_done = false;
> > +             list_for_each_entry_safe(clk_provider, next,
> > +                                     &clk_provider_list, node) {
> > +                     if (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;
> > +                     }
> > +             }
> > +
> > +             if (!is_init_done) {
> > +                     /*
> > +                      * 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
> > +                      */
> > +                     list_for_each_entry_safe(clk_provider, next,
> > +                                             &clk_provider_list, node) {
> > +                             clk_provider->clk_init_cb(clk_provider->np);
> > +                             list_del(&clk_provider->node);
> > +                             kfree(clk_provider);
> 
> Hmm, this is basically the code above repeated without the if. What 
> about something like the code snippet below?
> 
> bool force = false;
> 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;
>                 }
>         }
> 
>         if (!is_init_done)
>                 force = true;
> }
> 
> Best regards,
> Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/