[RFC PATCH RESEND 1/2] clk: add clk accuracy retrieval support
boris brezillon
b.brezillon at overkiz.com
Sun Nov 17 10:33:10 EST 2013
Hello Mike,
On 16/11/2013 01:50, Mike Turquette wrote:
> Quoting boris brezillon (2013-11-08 00:54:45)
>> Hello Mike,
>>
>> On 08/11/2013 01:51, Mike Turquette wrote:
>>> Quoting Boris BREZILLON (2013-10-13 10:17:10)
>>>> +/**
>>>> + * clk_get_accuracy - return the accuracy of clk
>>>> + * @clk: the clk whose accuracy is being returned
>>>> + *
>>>> + * Simply returns the cached accuracy of the clk, unless
>>>> + * CLK_GET_ACCURACY_NOCACHE flag is set, which means a recalc_rate will be
>>>> + * issued.
>>>> + * If clk is NULL then returns 0.
>>>> + */
>>>> +unsigned long clk_get_accuracy(struct clk *clk)
>>>> +{
>>>> + unsigned long accuracy;
>>>> + clk_prepare_lock();
>>>> +
>>>> + if (clk && (clk->flags & CLK_GET_ACCURACY_NOCACHE))
>>>> + __clk_recalc_accuracies(clk);
>>> I think that there is some overlap between recalculating the accuracy
>>> here and simply getting it. You only provide clk_get_accuracy and it
>>> serves both purposes. It would be better if clk_recalc_accuracy walked
>>> the subtree of children and if clk_get_accuracy simply returned a cached
>>> value.
>> I'm not sure I get your point.
>>
>> I used exactly the same model as for clk rate retrieval.
> Not exactly the same model. For rates we support public clk_recalc_rate
> and clk_get_rate functions.
I didn't find any clk_recalc_rate function in the CCF.
Could you point me out where it is defined ?
>
>> Actually there's one flag (CLK_GET_ACCURACY_NOCACHE) which is
>> checked to decide wether the accuracy should be recalculated each
>> time the get_accuracy is called or not.
>>
>> This means most of the time the clk_get_accuracy will return the cached
>> value unless the clk provider explicitely ask for accuracy recalculation
>> (e.g. a clk with dynamic accuracy according to temperature range ?).
>>
>> Are you suggesting to expose 2 functions to clk users (clk_get_accuracy
>> and clk_recalc_accuracy) ?
>> Or is clk_recalc_accuracy an internal/private function used by the CCF ?
> I was suggesting that in my previous email. However I've thought on it a
> bit and I'm not sure there is any value to having a public
> clk_recalc_accuracy right now.
>
> The only reason to do so would be if the accuracy can change in such a
> way that the clock framework is never aware of it. This would mean that
> somehow the accuracy changed "behind our back".
>
> One hypothetical example is a PLL which Linux knows about, but perhaps
> is controlled by some other co-processor (not Linux). In this case the
> co-processor may reprogram/relock the PLL in a way that affects it's
> accuracy, and a Linux driver that knows this may want to update the CCF
> book-keeping with a call to clk_recalc_accuracy.
>
> That's all hypothetical though and it can be added in later. Looking
> over your change again I think that it is sufficient for now.
This looks reasonable.
I'll propose a new patch to provide a public clk_recalc_rate function
after this
series is merged.
>
> Can you respin this patch with fixes for the two nitpicks I outlined in
> my previous mail and rebase it onto -rc1 when it drops? That should be
> any day now.
I've rebased this series (with the proposed fixes) onto linux-next.
I guess there won't be a lot of changes between the current tag (
next-20131115
<https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tag/?id=next-20131115>)
and -rc1, but I'll wait till -rc1 is released ;-).
Thanks for your review.
Best Regards,
Boris
> Thanks!
> Mike
>
>>>> +
>>>> + accuracy = __clk_get_accuracy(clk);
>>>> + clk_prepare_unlock();
>>>> +
>>>> + return accuracy;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(clk_get_accuracy);
>>>> +
>>>> +/**
>>>> * __clk_recalc_rates
>>>> * @clk: first clk in the subtree
>>>> * @msg: notification type (see include/linux/clk.h)
>>>> @@ -1545,6 +1610,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>>> {
>>>> clk_reparent(clk, new_parent);
>>>> clk_debug_reparent(clk, new_parent);
>>>> + __clk_recalc_accuracies(clk);
>>> Similar to the above statement. Why do this here? We do this for rates
>>> since calls to clk_get_rate will return the cached rate (unless the
>>> NOCACHE flag is set). But since a call to clk_get_accuracy will always
>>> recalculate it then there is no benefit to doing that here.
>> This is the same for clk_get_accuracies (it returns the cached
>> accuracy unless CLK_GET_ACCURACY_NOCACHE is defined).
>>
>> And changing parent of a clk will indirectly change the clk
>> accuracy (clk accuracies are cumulative).
>>
>>>> __clk_recalc_rates(clk, POST_RATE_CHANGE);
>>>> }
>>>>
>>>> @@ -1615,6 +1681,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>>>> /* do the re-parent */
>>>> ret = __clk_set_parent(clk, parent, p_index);
>>>>
>>>> + /* propagate accuracy recalculation */
>>>> + __clk_recalc_accuracies(clk);
>>> Ditto.
>> Ditto. :)
>>
>>
>> Please tell me if I misunderstood your requests.
>>
>> Best Regards,
>>
>> Boris
>>
>>> Regards,
>>> Mike
>>>
>>>> +
>>>> /* propagate rate recalculation accordingly */
>>>> if (ret)
>>>> __clk_recalc_rates(clk, ABORT_RATE_CHANGE);
>>>> @@ -1724,6 +1793,21 @@ int __clk_init(struct device *dev, struct clk *clk)
>>>> hlist_add_head(&clk->child_node, &clk_orphan_list);
>>>>
>>>> /*
>>>> + * Set clk's accuracy. The preferred method is to use
>>>> + * .recalc_accuracy. For simple clocks and lazy developers the default
>>>> + * fallback is to use the parent's accuracy. If a clock doesn't have a
>>>> + * parent (or is orphaned) then accuracy is set to zero (perfect
>>>> + * clock).
>>>> + */
>>>> + if (clk->ops->recalc_accuracy)
>>>> + clk->accuracy = clk->ops->recalc_accuracy(clk->hw,
>>>> + __clk_get_accuracy(clk->parent));
>>>> + else if (clk->parent)
>>>> + clk->accuracy = clk->parent->accuracy;
>>>> + else
>>>> + clk->accuracy = 0;
>>>> +
>>>> + /*
>>>> * Set clk's rate. The preferred method is to use .recalc_rate. For
>>>> * simple clocks and lazy developers the default fallback is to use the
>>>> * parent's rate. If a clock doesn't have a parent (or is orphaned)
>>>> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
>>>> index 8138c94..accb517 100644
>>>> --- a/include/linux/clk-private.h
>>>> +++ b/include/linux/clk-private.h
>>>> @@ -41,6 +41,7 @@ struct clk {
>>>> unsigned long flags;
>>>> unsigned int enable_count;
>>>> unsigned int prepare_count;
>>>> + unsigned long accuracy;
>>>> struct hlist_head children;
>>>> struct hlist_node child_node;
>>>> unsigned int notifier_count;
>>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>>> index 73bdb69..942811d 100644
>>>> --- a/include/linux/clk-provider.h
>>>> +++ b/include/linux/clk-provider.h
>>>> @@ -29,6 +29,7 @@
>>>> #define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */
>>>> #define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */
>>>> #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
>>>> +#define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
>>>>
>>>> struct clk_hw;
>>>>
>>>> @@ -108,6 +109,13 @@ struct clk_hw;
>>>> * which is likely helpful for most .set_rate implementation.
>>>> * Returns 0 on success, -EERROR otherwise.
>>>> *
>>>> + * @recalc_accuracy: Recalculate the accuracy of this clock. The clock accuracy
>>>> + * is expressed in ppb (parts per billion). The parent accuracy is
>>>> + * an input parameter.
>>>> + * Returns the calculated accuracy. Optional - if this op is not
>>>> + * set then clock accuracy will be initialized to parent accuracy
>>>> + * or 0 (perfect clock) if clock has no parent.
>>>> + *
>>>> * The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
>>>> * implementations to split any work between atomic (enable) and sleepable
>>>> * (prepare) contexts. If enabling a clock requires code that might sleep,
>>>> @@ -139,6 +147,8 @@ struct clk_ops {
>>>> u8 (*get_parent)(struct clk_hw *hw);
>>>> int (*set_rate)(struct clk_hw *hw, unsigned long,
>>>> unsigned long);
>>>> + unsigned long (*recalc_accuracy)(struct clk_hw *hw,
>>>> + unsigned long parent_accuracy);
>>>> void (*init)(struct clk_hw *hw);
>>>> };
>>>>
>>>> @@ -194,6 +204,7 @@ struct clk_hw {
>>>> struct clk_fixed_rate {
>>>> struct clk_hw hw;
>>>> unsigned long fixed_rate;
>>>> + unsigned long fixed_accuracy;
>>>> u8 flags;
>>>> };
>>>>
>>>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>>>> index 9a6d045..2fe3b54 100644
>>>> --- a/include/linux/clk.h
>>>> +++ b/include/linux/clk.h
>>>> @@ -85,6 +85,23 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
>>>> #endif
>>>>
>>>> /**
>>>> + * clk_get_accuracy - obtain the clock accuracy in ppb (parts per billion)
>>>> + * for a clock source.
>>>> + * @clk: clock source
>>>> + *
>>>> + * This gets the clock source accuracy expressed in ppb.
>>>> + * A perfect clock returns 0.
>>>> + */
>>>> +#ifdef CONFIG_HAVE_CLK_GET_ACCURACY
>>>> +unsigned long clk_get_accuracy(struct clk *clk);
>>>> +#else
>>>> +static inline unsigned long clk_get_accuracy(struct clk *clk)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +/**
>>>> * clk_prepare - prepare a clock source
>>>> * @clk: clock source
>>>> *
>>>> --
>>>> 1.7.9.5
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list