Re: [RFC PATCH RESEND 1/2] clk: add clk accuracy retrieval support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux