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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
