Messages in this thread | | | Date | Sun, 17 Nov 2013 16:33:10 +0100 | From | boris brezillon <> | Subject | Re: [RFC PATCH RESEND 1/2] clk: add clk accuracy retrieval support |
| |
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@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
| |