Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

[Tomeu Vizoso]

On 4 October 2014 01:15, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 10/02, Tomeu Vizoso wrote:
>> Moves clock state to struct clk_core, but takes care to change as little API as
>> possible.
>>
>> struct clk_hw still has a pointer to a struct clk, which is the
>> implementation's per-user clk instance, for backwards compatibility.
>>
>> The struct clk that clk_get_parent() returns isn't owned by the caller, but by
>> the clock implementation, so the former shouldn't call clk_put() on it.
>>
>> Because some boards in mach-omap2 still register clocks statically, their clock
>> registration had to be updated to take into account that the clock information
>> is stored in struct clk_core now.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> ---
>
>
> We should s/provider/core/ when we're dealing with clk_core
> structures in the function signature. The providers are hardware
> drivers and the core structures are for the framework, not the
> same. Furthermore, the provider drivers should only be dealing
> with clk_hw structures. The only place that clk_core should be in
> clk-provider.h is in struct clk_hw because there's no way to get
> around it.
>
> This way, provider drivers should only be including
> clk-provider.h because they only care about dealing with struct
> clk_hw. Consumers should only be including linux/clk.h where they
> only know about struct clk as an opaque pointer. Once we get OMAP
> to stop using clk-private.h we can kill off that header entirely
> (I see there are some other bogus users of that header outside of
> OMAP that we should nuke). Then the framework can include
> clk-provider.h and clk.h to map between the hw cookie and the
> consumer cookie.

Agreed.

> This is the end goal. I understand that the provider API is sort
> of a mess with us allowing drivers to use the underscore and
> non-underscore functions and the mixture of struct clk and struct
> ckl_hw throughout.
>
>  struct clk_hw <--> struct clk_core <----> struct clk
>                                        \-> struct clk
>                                        |-> struct clk

Agree this is how it should look like at some point, but for now I
need a reference to struct clk from struct clk_hw, so providers can
keep using the existing API. This reference would be removed once they
move to the new clk_hw-based API.

>  providers
>  ---------
>  struct clk_hw {
>         struct clk_core *
>         ...
>  };
>
>  consumers
>  ---------
>
>  struct clk;
>
>  hidden in core framework
>  ------------------------
>  struct clk {
>         struct clk_core *;
>         ...
>  }
>
>  struct clk_core {
>         struct clk_hw *;
>         ...
>  }
>
>
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 4eeb8de..b216b13 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -37,6 +37,13 @@ static HLIST_HEAD(clk_root_list);
>>  static HLIST_HEAD(clk_orphan_list);
>>  static LIST_HEAD(clk_notifier_list);
>>
>> +static void clk_provider_put(struct clk_core *clk);
>
> Does this need to be forward declared?

No, removed.

>> +static long clk_provider_get_accuracy(struct clk_core *clk);
>> +static bool clk_provider_is_prepared(struct clk_core *clk);
>> +static bool clk_provider_is_enabled(struct clk_core *clk);
>> +static long clk_provider_round_rate(struct clk_core *clk, unsigned long rate);
>> @@ -356,7 +363,7 @@ out:
>>   *
>>   * Caller must hold prepare_lock.
>>   */
>> -static void clk_debug_unregister(struct clk *clk)
>> +static void clk_debug_unregister(struct clk_core *clk)
>>  {
>>       debugfs_remove_recursive(clk->dentry);
>>  }
>> @@ -366,8 +373,8 @@ struct dentry *clk_debugfs_add_file(struct clk *clk, char *name, umode_t mode,
>
> We should pass struct clk_hw here instead of struct clk. Let's do
> it soon before we get any users.

Sounds good to me.

>>  {
>>       struct dentry *d = NULL;
>>
>> -     if (clk->dentry)
>> -             d = debugfs_create_file(name, mode, clk->dentry, data, fops);
>> +     if (clk->core->dentry)
>> +             d = debugfs_create_file(name, mode, clk->core->dentry, data, fops);
>>
>>       return d;
>>  }
>> @@ -545,53 +553,67 @@ late_initcall_sync(clk_disable_unused);
>>
>>  const char *__clk_get_name(struct clk *clk)
>>  {
>> -     return !clk ? NULL : clk->name;
>> +     return !clk ? NULL : clk->core->name;
>>  }
>>  EXPORT_SYMBOL_GPL(__clk_get_name);
>>
>>  struct clk_hw *__clk_get_hw(struct clk *clk)
>>  {
>> -     return !clk ? NULL : clk->hw;
>> +     return !clk ? NULL : clk->core->hw;
>>  }
>>  EXPORT_SYMBOL_GPL(__clk_get_hw);
>>
>>  u8 __clk_get_num_parents(struct clk *clk)
>>  {
>> -     return !clk ? 0 : clk->num_parents;
>> +     return !clk ? 0 : clk->core->num_parents;
>>  }
>>  EXPORT_SYMBOL_GPL(__clk_get_num_parents);
>>
>>  struct clk *__clk_get_parent(struct clk *clk)
>>  {
>> -     return !clk ? NULL : clk->parent;
>> +     /* TODO: Create a per-user clk and change callers to call clk_put */
>
> More like replace all callers with a function that returns their
> parent's hw pointer.

Sounds good, but I thought about it and have chosen to just remove the comment.

>         struct clk_hw *clk_provider_get_parent(struct clk_hw *hw)
>
>
>> +     return !clk ? NULL : clk->core->parent->hw->clk;
>>  }
>>  EXPORT_SYMBOL_GPL(__clk_get_parent);
>>
>> -struct clk *clk_get_parent_by_index(struct clk *clk, u8 index)
>> +static struct clk_core *clk_provider_get_parent_by_index(struct clk_core *clk,
>> +                                                      u8 index)
>>  {
>>       if (!clk || index >= clk->num_parents)
>>               return NULL;
>>       else if (!clk->parents)
>> -             return __clk_lookup(clk->parent_names[index]);
>> +             return clk_provider_lookup(clk->parent_names[index]);
>>       else if (!clk->parents[index])
>>               return clk->parents[index] =
>> -                     __clk_lookup(clk->parent_names[index]);
>> +                     clk_provider_lookup(clk->parent_names[index]);
>>       else
>>               return clk->parents[index];
>>  }
>> +
>> +struct clk *clk_get_parent_by_index(struct clk *clk, u8 index)
>> +{
>> +     struct clk_core *parent;
>> +
>> +     parent = clk_provider_get_parent_by_index(clk->core, index);
>> +     if (IS_ERR(parent))
>> +             return (void *)parent;
>
> What is this for? Does it actually happen that we have error
> pointers here?

No, have simplified it.

>> +
>> +     /* TODO: Create a per-user clk and change callers to call clk_put */
>> +     return parent->hw->clk;
>> +}
>>  EXPORT_SYMBOL_GPL(clk_get_parent_by_index);
>>
>>  unsigned int __clk_get_enable_count(struct clk *clk)
>>  {
>> -     return !clk ? 0 : clk->enable_count;
>> +     return !clk ? 0 : clk->core->enable_count;
>>  }
>>
>>  unsigned int __clk_get_prepare_count(struct clk *clk)
>>  {
>> -     return !clk ? 0 : clk->prepare_count;
>> +     return !clk ? 0 : clk->core->prepare_count;
>>  }
>
> This function isn't even used. Maybe we should remove it.

Have just gone ahead with your suggestion.

>>
>> -unsigned long __clk_get_rate(struct clk *clk)
>> +static unsigned long clk_provider_get_rate_nolock(struct clk_core *clk)
>>  {
>>       unsigned long ret;
>>
>> @@ -1612,11 +1722,11 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
>>   *
>>   * For single-parent clocks without .get_parent, first check to see if the
>>   * .parents array exists, and if so use it to avoid an expensive tree
>> - * traversal.  If .parents does not exist then walk the tree with __clk_lookup.
>> + * traversal.  If .parents does not exist then walk the tree with clk_provider_lookup.
>
> Maybe just remove the function name entirely so we don't have to
> keep updating something that's obvious from the code.

Sounds good.

>>   */
>> -static struct clk *__clk_init_parent(struct clk *clk)
>> +static struct clk_core *__clk_init_parent(struct clk_core *clk)
>>  {
>> -     struct clk *ret = NULL;
>> +     struct clk_core *ret = NULL;
>>       u8 index;
>>
>>       /* handle the trivial cases */
>> @@ -1626,7 +1736,7 @@ static struct clk *__clk_init_parent(struct clk *clk)
>>
>>       if (clk->num_parents == 1) {
>>               if (IS_ERR_OR_NULL(clk->parent))
>> -                     ret = clk->parent = __clk_lookup(clk->parent_names[0]);
>> +                     ret = clk->parent = clk_provider_lookup(clk->parent_names[0]);
>>               ret = clk->parent;
>>               goto out;
>>       }
>> @@ -1640,7 +1750,7 @@ static struct clk *__clk_init_parent(struct clk *clk)
>>
>>       /*
>>        * Do our best to cache parent clocks in clk->parents.  This prevents
>> -      * unnecessary and expensive calls to __clk_lookup.  We don't set
>> +      * unnecessary and expensive calls to clk_provider_lookup.  We don't set
>
> Ditto.
>
>>        * clk->parent here; that is done by the calling function
>>        */
>>
>> @@ -2306,7 +2438,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
>>       if (cn->clk == clk) {
>>               ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
>>
>> -             clk->notifier_count--;
>> +             clk->core->notifier_count--;
>>
>>               /* XXX the notifier code should handle this better */
>>               if (!cn->notifier_head.head) {
>> @@ -2325,6 +2457,31 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_notifier_unregister);
>>
>> +struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev_id,
>
> This should take clk_hw instead of clk_core.

Nod.

>> +                          const char *con_id)
>> +{
>> +     struct clk *clk;
>> +
>> +     /* This is to allow this function to be chained to others */
>> +     if (!clk_core || IS_ERR(clk_core))
>> +             return (struct clk *) clk_core;
>> +
>> +     clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>> +     if (!clk)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     clk->core = clk_core;
>> +     clk->dev_id = dev_id;
>> +     clk->con_id = con_id;
>> +
>> +     return clk;
>> +}
>> +
>> +
>>  #ifdef CONFIG_OF
>>  /**
>>   * struct of_clk_provider - Clock provider registration structure
>> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
>> index c798138..4a17902 100644
>> --- a/drivers/clk/clk.h
>> +++ b/drivers/clk/clk.h
>> @@ -9,9 +9,16 @@
>>   * published by the Free Software Foundation.
>>   */
>>
>> +#include <linux/clk-private.h>
>
> Ah, please no. Why do we need this?

Not needed any more.

>> +
>>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>
> These ifdefs look useless.
>
>>  struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
>>  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>>  void of_clk_lock(void);
>>  void of_clk_unlock(void);
>>  #endif
>> +
>> +#if defined(CONFIG_COMMON_CLK)
>
> So we shouldn't need this one either.

Fine.

>> +struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev_id,
>> +                          const char *con_id);
>> +#endif
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index da4bda8..fe3712f 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -168,14 +172,27 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
>>  struct clk *clk_get_sys(const char *dev_id, const char *con_id)
>>  {
>>       struct clk_lookup *cl;
>> +     struct clk *clk = NULL;
>>
>>       mutex_lock(&clocks_mutex);
>>       cl = clk_find(dev_id, con_id);
>> -     if (cl && !__clk_get(cl->clk))
>> -             cl = NULL;
>> +     if (cl) {
>> +#if defined(CONFIG_COMMON_CLK)
>> +             clk = __clk_create_clk(cl->clk->core, dev_id, con_id);
>> +             if (clk && !__clk_get(clk)) {
>> +                     kfree(clk);
>
> This looks weird. It makes me suspect we've failed to reference
> count something properly. Can we avoid this?

Can you extend on this? But I see how the behaviour doesn't match the
previous one because cl should be NULLed when __clk_get fails. I have
fixed this.

>> +                     clk = NULL;
>> +             }
>> +#else
>> +             if (!__clk_get(cl->clk))
>> +                     cl = NULL;
>> +             else
>> +                     clk = cl->clk;
>> +#endif
>> +     }
>>       mutex_unlock(&clocks_mutex);
>>
>> -     return cl ? cl->clk : ERR_PTR(-ENOENT);
>> +     return cl ? clk : ERR_PTR(-ENOENT);
>>  }
>>  EXPORT_SYMBOL(clk_get_sys);
>>
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 411dd7e..8fd6320 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -514,7 +519,7 @@ struct clk *clk_get_parent_by_index(struct clk *clk, u8 index);
>>  unsigned int __clk_get_enable_count(struct clk *clk);
>>  unsigned int __clk_get_prepare_count(struct clk *clk);
>>  unsigned long __clk_get_rate(struct clk *clk);
>> -unsigned long __clk_get_accuracy(struct clk *clk);
>> +unsigned long __clk_get_accuracy(struct clk_core *clk);
>
> Oops. Maybe we can just delete this one given that nobody uses it.

Good.

>>  unsigned long __clk_get_flags(struct clk *clk);
>>  bool __clk_is_prepared(struct clk *clk);
>>  bool __clk_is_enabled(struct clk *clk);
>> @@ -523,6 +528,22 @@ long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
>>                             unsigned long *best_parent_rate,
>>                             struct clk **best_parent_p);
>>
>> +unsigned long clk_provider_get_rate(struct clk_core *clk);
>> +struct clk_core *clk_provider_get_parent(struct clk_core *clk);
>> +
>> +/**
>> + * __clk_core_to_clk - return per-user clk
>> + * @clk: struct clk_core for which we want a per-user clk
>> + *
>> + * Returns a per-user clock that is owned by its provider. The caller shall not
>> + * call clk_get() on it.
>> + *
>> + * This function should be only needed by implementors of
>> + * clk_ops.determine_rate() and should be dropped once all have moved to a
>> + * variant that returns **clk_core instead.
>
> Can we do that before this patch? And also move them to use **clk_hw instead?

Will look at it.

Thanks for the great feedback!

Tomeu

>> + */
>> +struct clk *__clk_core_to_clk(struct clk_core *clk);
>> +
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> --
> 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/
--
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/