[PATCH v2 0/2] clk: add clk accuracy support

[PATCH v2 0/2] clk: add clk accuracy support

[Boris BREZILLON]

Hello,

This patch series adds support for clock accuracy retrieval in the common clk
framework.

Best Regards,

Boris

Changes since v1:
 - remove HAVE_CLK_GET_ACCURACY option and enable clk accuracy support only
   when using the CCF
 - export __clk_get_accuracy (might be used by clk-providers)
 - fix documentation (s/recalc_rate/recalc_accuracy/)
 - move fixed_accuracy field addition (struct clk_fixed_rate) from the 1st
   patch to the 2nd patch of this series

Boris BREZILLON (2):
  clk: add clk accuracy retrieval support
  clk: add accuracy support for fixed clock

 Documentation/clk.txt                              |    4 +
 .../devicetree/bindings/clock/fixed-clock.txt      |    3 +
 drivers/clk/clk-fixed-rate.c                       |   43 ++++++--
 drivers/clk/clk.c                                  |  109 +++++++++++++++++++-
 include/linux/clk-private.h                        |    1 +
 include/linux/clk-provider.h                       |   15 +++
 include/linux/clk.h                                |   17 +++
 7 files changed, 182 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] clk: add clk accuracy retrieval support

[Boris BREZILLON]

The clock accuracy is expressed in ppb (parts per billion) and represents
the possible clock drift.
Say you have a clock (e.g. an oscillator) which provides a fixed clock of
20MHz with an accuracy of +- 20Hz. This accuracy expressed in ppb is
20Hz/20MHz = 1000 ppb (or 1 ppm).

Clock users may need the clock accuracy information in order to choose
the best clock (the one with the best accuracy) across several available
clocks.

This patch adds clk accuracy retrieval support for common clk framework by
means of a new function called clk_get_accuracy.
This function returns the given clock accuracy expressed in ppb.

In order to get the clock accuracy, this implementation adds one callback
called recalc_accuracy to the clk_ops structure.
This callback is given the parent clock accuracy (if the clock is not a
root clock) and should recalculate the given clock accuracy.

This callback is optional and may be implemented if the clock is not
a perfect clock (accuracy != 0 ppb).

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 Documentation/clk.txt        |    4 ++
 drivers/clk/clk.c            |  109 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-private.h  |    1 +
 include/linux/clk-provider.h |   11 +++++
 include/linux/clk.h          |   17 +++++++
 5 files changed, 138 insertions(+), 4 deletions(-)

diff --git a/Documentation/clk.txt b/Documentation/clk.txt
index 3aeb5c4..eb20198 100644
--- a/Documentation/clk.txt
+++ b/Documentation/clk.txt
@@ -77,6 +77,8 @@ the operations defined in clk.h:
 		int		(*set_parent)(struct clk_hw *hw, u8 index);
 		u8		(*get_parent)(struct clk_hw *hw);
 		int		(*set_rate)(struct clk_hw *hw, unsigned long);
+		unsigned long	(*recalc_accuracy)(struct clk_hw *hw,
+						   unsigned long parent_accuracy);
 		void		(*init)(struct clk_hw *hw);
 	};
 
@@ -202,6 +204,8 @@ optional or must be evaluated on a case-by-case basis.
 .set_parent     |      |             | n             | y           | n    |
 .get_parent     |      |             | n             | y           | n    |
                 |      |             |               |             |      |
+.recalc_accuracy|      |             |               |             |      |
+                |      |             |               |             |      |
 .init           |      |             |               |             |      |
                 -----------------------------------------------------------
 [1] either one of round_rate or determine_rate is required.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2cf2ea6..4b0c1bc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -104,10 +104,11 @@ static void clk_summary_show_one(struct seq_file *s, struct clk *c, int level)
 	if (!c)
 		return;
 
-	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu",
+	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu %-11lu",
 		   level * 3 + 1, "",
 		   30 - level * 3, c->name,
-		   c->enable_count, c->prepare_count, clk_get_rate(c));
+		   c->enable_count, c->prepare_count, clk_get_rate(c),
+		   clk_get_accuracy(c));
 	seq_printf(s, "\n");
 }
 
@@ -129,8 +130,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
 {
 	struct clk *c;
 
-	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate\n");
-	seq_printf(s, "---------------------------------------------------------------------\n");
+	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate        accuracy\n");
+	seq_printf(s, "---------------------------------------------------------------------------------\n");
 
 	clk_prepare_lock();
 
@@ -167,6 +168,7 @@ static void clk_dump_one(struct seq_file *s, struct clk *c, int level)
 	seq_printf(s, "\"enable_count\": %d,", c->enable_count);
 	seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
 	seq_printf(s, "\"rate\": %lu", clk_get_rate(c));
+	seq_printf(s, "\"accuracy\": %lu", clk_get_accuracy(c));
 }
 
 static void clk_dump_subtree(struct seq_file *s, struct clk *c, int level)
@@ -248,6 +250,11 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
 	if (!d)
 		goto err_out;
 
+	d = debugfs_create_u32("clk_accuracy", S_IRUGO, clk->dentry,
+			(u32 *)&clk->accuracy);
+	if (!d)
+		goto err_out;
+
 	d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
 			(u32 *)&clk->flags);
 	if (!d)
@@ -602,6 +609,28 @@ out:
 	return ret;
 }
 
+unsigned long __clk_get_accuracy(struct clk *clk)
+{
+	unsigned long ret;
+
+	if (!clk) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = clk->accuracy;
+
+	if (clk->flags & CLK_IS_ROOT)
+		goto out;
+
+	if (!clk->parent)
+		ret = 0;
+
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__clk_get_accuracy);
+
 unsigned long __clk_get_flags(struct clk *clk)
 {
 	return !clk ? 0 : clk->flags;
@@ -1016,6 +1045,59 @@ static int __clk_notify(struct clk *clk, unsigned long msg,
 }
 
 /**
+ * __clk_recalc_accuracies
+ * @clk: first clk in the subtree
+ * @msg: notification type (see include/linux/clk.h)
+ *
+ * Walks the subtree of clks starting with clk and recalculates accuracies as
+ * it goes.  Note that if a clk does not implement the .recalc_rate callback
+ * then it is assumed that the clock will take on the rate of it's parent.
+ *
+ * Caller must hold prepare_lock.
+ */
+static void __clk_recalc_accuracies(struct clk *clk)
+{
+	unsigned long parent_accuracy = 0;
+	struct clk *child;
+
+	if (clk->parent)
+		parent_accuracy = clk->parent->accuracy;
+
+	if (clk->ops->recalc_accuracy)
+		clk->accuracy = clk->ops->recalc_accuracy(clk->hw,
+							  parent_accuracy);
+	else
+		clk->accuracy = parent_accuracy;
+
+	hlist_for_each_entry(child, &clk->children, child_node)
+		__clk_recalc_accuracies(child);
+}
+
+/**
+ * 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.
+ */
+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);
+
+	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)
@@ -1551,6 +1633,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);
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
 
@@ -1621,6 +1704,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);
+
 	/* propagate rate recalculation accordingly */
 	if (ret)
 		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);
@@ -1730,6 +1816,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 7e59253..16d182c 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);
 };
 
@@ -433,6 +443,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_flags(struct clk *clk);
 bool __clk_is_prepared(struct clk *clk);
 bool __clk_is_enabled(struct clk *clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..0dd9114 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -82,6 +82,23 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb);
 
 int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
 
+/**
+ * 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.
+ */
+long clk_get_accuracy(struct clk *clk);
+
+#else
+
+static inline long clk_get_accuracy(struct clk *clk)
+{
+	return -ENOTSUPP;
+}
+
 #endif
 
 /**
-- 
1.7.9.5

[PATCH v2 2/2] clk: add accuracy support for fixed clock

[Boris BREZILLON]

This patch adds support for accuracy retrieval on fixed clocks.
It also adds a new dt property called 'clock-accuracy' to define the clock
accuracy.

This can be usefull for oscillator (RC, crystal, ...) definitions which are
always given an accuracy characteristic.

Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
 .../devicetree/bindings/clock/fixed-clock.txt      |    3 ++
 drivers/clk/clk-fixed-rate.c                       |   43 +++++++++++++++++---
 include/linux/clk-provider.h                       |    4 ++
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
index 0b1fe78..48ea0ad 100644
--- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
+++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
@@ -10,6 +10,8 @@ Required properties:
 - clock-frequency : frequency of clock in Hz. Should be a single cell.
 
 Optional properties:
+- clock-accuracy : accuracy of clock in ppb (parts per billion).
+		   Should be a single cell.
 - gpios : From common gpio binding; gpio connection to clock enable pin.
 - clock-output-names : From common clock binding.
 
@@ -18,4 +20,5 @@ Example:
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
 		clock-frequency = <1000000000>;
+		clock-accuracy = <100>;
 	};
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 1ed591a..0fc56ab 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -34,22 +34,31 @@ static unsigned long clk_fixed_rate_recalc_rate(struct clk_hw *hw,
 	return to_clk_fixed_rate(hw)->fixed_rate;
 }
 
+static unsigned long clk_fixed_rate_recalc_accuracy(struct clk_hw *hw,
+		unsigned long parent_accuracy)
+{
+	return to_clk_fixed_rate(hw)->fixed_accuracy;
+}
+
 const struct clk_ops clk_fixed_rate_ops = {
 	.recalc_rate = clk_fixed_rate_recalc_rate,
+	.recalc_accuracy = clk_fixed_rate_recalc_accuracy,
 };
 EXPORT_SYMBOL_GPL(clk_fixed_rate_ops);
 
 /**
- * clk_register_fixed_rate - register fixed-rate clock with the clock framework
+ * clk_register_fixed_rate_with_accuracy - register fixed-rate clock with the
+ *					   clock framework
  * @dev: device that is registering this clock
  * @name: name of this clock
  * @parent_name: name of clock's parent
  * @flags: framework-specific flags
  * @fixed_rate: non-adjustable clock rate
+ * @fixed_accuracy: non-adjustable clock rate
  */
-struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
-		const char *parent_name, unsigned long flags,
-		unsigned long fixed_rate)
+struct clk *clk_register_fixed_rate_with_accuracy(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		unsigned long fixed_rate, unsigned long fixed_accuracy)
 {
 	struct clk_fixed_rate *fixed;
 	struct clk *clk;
@@ -70,16 +79,33 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 
 	/* struct clk_fixed_rate assignments */
 	fixed->fixed_rate = fixed_rate;
+	fixed->fixed_accuracy = fixed_accuracy;
 	fixed->hw.init = &init;
 
 	/* register the clock */
 	clk = clk_register(dev, &fixed->hw);
-
 	if (IS_ERR(clk))
 		kfree(fixed);
 
 	return clk;
 }
+EXPORT_SYMBOL_GPL(clk_register_fixed_rate_with_accuracy);
+
+/**
+ * clk_register_fixed_rate - register fixed-rate clock with the clock framework
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @parent_name: name of clock's parent
+ * @flags: framework-specific flags
+ * @fixed_rate: non-adjustable clock rate
+ */
+struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
+		const char *parent_name, unsigned long flags,
+		unsigned long fixed_rate)
+{
+	return clk_register_fixed_rate_with_accuracy(dev, name, parent_name,
+						     flags, fixed_rate, 0);
+}
 EXPORT_SYMBOL_GPL(clk_register_fixed_rate);
 
 #ifdef CONFIG_OF
@@ -91,13 +117,18 @@ void of_fixed_clk_setup(struct device_node *node)
 	struct clk *clk;
 	const char *clk_name = node->name;
 	u32 rate;
+	u32 accuracy = 0;
 
 	if (of_property_read_u32(node, "clock-frequency", &rate))
 		return;
 
+	of_property_read_u32(node, "clock-accuracy", &accuracy);
+
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-	clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate);
+	clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
+						    CLK_IS_ROOT, rate,
+						    accuracy);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 16d182c..5429f5d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -204,6 +204,7 @@ struct clk_hw {
 struct clk_fixed_rate {
 	struct		clk_hw hw;
 	unsigned long	fixed_rate;
+	unsigned long	fixed_accuracy;
 	u8		flags;
 };
 
@@ -211,6 +212,9 @@ extern const struct clk_ops clk_fixed_rate_ops;
 struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate);
+struct clk *clk_register_fixed_rate_with_accuracy(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		unsigned long fixed_rate, unsigned long fixed_accuracy);
 
 void of_fixed_clk_setup(struct device_node *np);
 
-- 
1.7.9.5

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[Jason Cooper]

Boris,

Thanks for posting this series.  Bear with me as I'm attempting to give
MikeT a hand.  Don't be afraid to tell me a question is stupid :-)

On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
> This patch adds support for accuracy retrieval on fixed clocks.
> It also adds a new dt property called 'clock-accuracy' to define the clock
> accuracy.
> 
> This can be usefull for oscillator (RC, crystal, ...) definitions which are
> always given an accuracy characteristic.

I think we need to be more explicit in the binding and the API,
especially when providing a method to recalculate the accuracy.  I
presume this recalculated value would be relative against the root
clock?

There really needs to be two attributes here:  the rated accuracy from
the manufacturer, and the calculated accuracy wrt another clock in the
system.  We only need a binding for the manufacturer rating since the
calculated accuracy is determined at runtime.

I would also prefer to see an unknown accuracy be -1.  There are already
clocks on the market (PPS reference clocks) with accuracies of
0.1ppb/day [1].  Obviously, these aren't system clocks.  So the limit on
accuracy may be a non-issue.  However, it may be worth changing the
binding property to express the units.

> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  .../devicetree/bindings/clock/fixed-clock.txt      |    3 ++
>  drivers/clk/clk-fixed-rate.c                       |   43 +++++++++++++++++---
>  include/linux/clk-provider.h                       |    4 ++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> index 0b1fe78..48ea0ad 100644
> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> @@ -10,6 +10,8 @@ Required properties:
>  - clock-frequency : frequency of clock in Hz. Should be a single cell.
>  
>  Optional properties:
> +- clock-accuracy : accuracy of clock in ppb (parts per billion).
> +		   Should be a single cell.

I would prefer to call this property 'clock-rated-ppb'.

>  - gpios : From common gpio binding; gpio connection to clock enable pin.
>  - clock-output-names : From common clock binding.
>  
> @@ -18,4 +20,5 @@ Example:
>  		compatible = "fixed-clock";
>  		#clock-cells = <0>;
>  		clock-frequency = <1000000000>;
> +		clock-accuracy = <100>;
>  	};

thx,

Jason.

[1] http://www.vectron.com/products/modules/md-010.htm

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[boris brezillon]

Hi Jason,

On 27/11/2013 15:56, Jason Cooper wrote:
> Boris,
>
> Thanks for posting this series.  Bear with me as I'm attempting to give
> MikeT a hand.
Nice to hear.
Mike already took a look at this series, but I'm happy to get more 
feedbacks.

> Don't be afraid to tell me a question is stupid :-)
Your questions are far from stupid ;-).

>
> On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
>> This patch adds support for accuracy retrieval on fixed clocks.
>> It also adds a new dt property called 'clock-accuracy' to define the clock
>> accuracy.
>>
>> This can be usefull for oscillator (RC, crystal, ...) definitions which are
>> always given an accuracy characteristic.
> I think we need to be more explicit in the binding and the API,
> especially when providing a method to recalculate the accuracy.  I
> presume this recalculated value would be relative against the root
> clock?

Yes, indirectly.
Actually the clk accuracy depends on the whole clock chain, and is 
calculated
either by comparing the real clk rate to the theorical clk rate
(accuracy = absolute_value((theorical_clk_rate - real_clk_rate)) / 
theorical_clk_rate),
or by adding all the accuracies (expressed in ppm, ppb or ppt) of the 
clk chain
(accuracy = current_clk_accuracy + parent_clk_accuracy).

Say you have a root clk with a +-10000 ppb accuracy, then a pll multiplying
this root clk by 40 and introducing a possible drift of +- 1000 ppb and
eventually a system clk based on this pll with a perfect div by 2 prescaler
(accuracy = 0 ppb).

If I understand correctly how accuracy propagates accross the clk tree,
you'll end up with a system clk with a +- 11000 ppb accuracy.

e.g.:
  root clk = 12MHz +- 10000 ppb => 12 MHz * (1 - (10000 / 10^9)) < root 
clk < 12 MHz * (1 + (10000 / 10^9))
                                                     => 11,99988 MHz < 
root clk < 12,00012 MHz
  pll clk = ((root clk) * 40) +- 1000 ppb =>  (11,99988 MHz * 40) * (1 - 
(1000 / 10^9)) < pll clk < (12,00012 MHz * 40) * (1 + (1000 / 10^9))
                                                            => 
479,994720005 MHz < pll clk < 480,005280005 MHz

  system clk = ((pll clk) / 2) +- XXX ppb => 479,994720005 MHz / 2 < 
system clk < 480,005280005 MHz / 2
                                                               => 
239,997360002 MHz < system clk < 240,002640002 MHz
                                                               => system 
clk accuracy = 0,002640002 / 240 = 11000 ppb

Please tell me if my assumptions are false.
>
> There really needs to be two attributes here:  the rated accuracy from
> the manufacturer, and the calculated accuracy wrt another clock in the
> system.  We only need a binding for the manufacturer rating since the
> calculated accuracy is determined at runtime.

Actually when I proposed this new functionnality I only had the theorical
(or manufacturer rated) accuracy in mind.
But providing an estimated accuracy (based on another clk) sounds
interresting if your reference clk is an extremly accurate one.

>
> I would also prefer to see an unknown accuracy be -1.
I decided to keep 0 as a default value for unimplemented recalc_accuracy
(or unspecified fixed accuracy) to keep existing implementation coherent.

0 means the clk is perfect, and I thought it would be easier to handle a
perfect clk (even if this is not really the case) than handling an error 
case.

Another aspect is the propagation of the clk accuracy accross the clk tree.
Returning -1 in the middle of the clk chain will drop the previous clk 
accuracy
calculation.

Anyway, I can change this if you think this is more appropriate.

> There are already
> clocks on the market (PPS reference clocks) with accuracies of
> 0.1ppb/day [1].  Obviously, these aren't system clocks.  So the limit on
> accuracy may be a non-issue.  However, it may be worth changing the
> binding property to express the units.
Wow, 0.1 ppb, this is impressive :-).


This needs more than changing the dt bindings: I currently store the
accuracy value in an unsigned long field, and expressing this in ppt
(parts per trillion) may implies storing this in an u64 field (or store a
unit field).


>
>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>> ---
>>   .../devicetree/bindings/clock/fixed-clock.txt      |    3 ++
>>   drivers/clk/clk-fixed-rate.c                       |   43 +++++++++++++++++---
>>   include/linux/clk-provider.h                       |    4 ++
>>   3 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>> index 0b1fe78..48ea0ad 100644
>> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
>> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>> @@ -10,6 +10,8 @@ Required properties:
>>   - clock-frequency : frequency of clock in Hz. Should be a single cell.
>>   
>>   Optional properties:
>> +- clock-accuracy : accuracy of clock in ppb (parts per billion).
>> +		   Should be a single cell.
> I would prefer to call this property 'clock-rated-ppb'.

Depending on what we choose to do with the accuracy field, this might be 
an option.

>
>>   - gpios : From common gpio binding; gpio connection to clock enable pin.
>>   - clock-output-names : From common clock binding.
>>   
>> @@ -18,4 +20,5 @@ Example:
>>   		compatible = "fixed-clock";
>>   		#clock-cells = <0>;
>>   		clock-frequency = <1000000000>;
>> +		clock-accuracy = <100>;
>>   	};
> thx,
>
> Jason.
>
> [1] http://www.vectron.com/products/modules/md-010.htm

Thanks for your review, and don't hesitate to ask more questions, or to 
point out
incoherencies in my approach (I'm not an expert in clk and clk accuracy 
calculation,
and I might be wrong ;-)).

Best Regards,

Boris

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[Mike Turquette]

Quoting boris brezillon (2013-11-27 09:19:08)
> Hi Jason,
> 
> On 27/11/2013 15:56, Jason Cooper wrote:
> > Boris,
> >
> > Thanks for posting this series.  Bear with me as I'm attempting to give
> > MikeT a hand.
> Nice to hear.
> Mike already took a look at this series, but I'm happy to get more 
> feedbacks.
> 
> > Don't be afraid to tell me a question is stupid :-)
> Your questions are far from stupid ;-).
> 
> >
> > On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
> >> This patch adds support for accuracy retrieval on fixed clocks.
> >> It also adds a new dt property called 'clock-accuracy' to define the clock
> >> accuracy.
> >>
> >> This can be usefull for oscillator (RC, crystal, ...) definitions which are
> >> always given an accuracy characteristic.
> > I think we need to be more explicit in the binding and the API,
> > especially when providing a method to recalculate the accuracy.  I
> > presume this recalculated value would be relative against the root
> > clock?
> 
> Yes, indirectly.
> Actually the clk accuracy depends on the whole clock chain, and is 
> calculated
> either by comparing the real clk rate to the theorical clk rate
> (accuracy = absolute_value((theorical_clk_rate - real_clk_rate)) / 
> theorical_clk_rate),
> or by adding all the accuracies (expressed in ppm, ppb or ppt) of the 
> clk chain
> (accuracy = current_clk_accuracy + parent_clk_accuracy).
> 
> Say you have a root clk with a +-10000 ppb accuracy, then a pll multiplying
> this root clk by 40 and introducing a possible drift of +- 1000 ppb and
> eventually a system clk based on this pll with a perfect div by 2 prescaler
> (accuracy = 0 ppb).
> 
> If I understand correctly how accuracy propagates accross the clk tree,
> you'll end up with a system clk with a +- 11000 ppb accuracy.
> 
> e.g.:
>   root clk = 12MHz +- 10000 ppb => 12 MHz * (1 - (10000 / 10^9)) < root 
> clk < 12 MHz * (1 + (10000 / 10^9))
>                                                      => 11,99988 MHz < 
> root clk < 12,00012 MHz
>   pll clk = ((root clk) * 40) +- 1000 ppb =>  (11,99988 MHz * 40) * (1 - 
> (1000 / 10^9)) < pll clk < (12,00012 MHz * 40) * (1 + (1000 / 10^9))
>                                                             => 
> 479,994720005 MHz < pll clk < 480,005280005 MHz
> 
>   system clk = ((pll clk) / 2) +- XXX ppb => 479,994720005 MHz / 2 < 
> system clk < 480,005280005 MHz / 2
>                                                                => 
> 239,997360002 MHz < system clk < 240,002640002 MHz
>                                                                => system 
> clk accuracy = 0,002640002 / 240 = 11000 ppb
> 
> Please tell me if my assumptions are false.
> >
> > There really needs to be two attributes here:  the rated accuracy from
> > the manufacturer, and the calculated accuracy wrt another clock in the
> > system.  We only need a binding for the manufacturer rating since the
> > calculated accuracy is determined at runtime.
> 
> Actually when I proposed this new functionnality I only had the theorical
> (or manufacturer rated) accuracy in mind.
> But providing an estimated accuracy (based on another clk) sounds
> interresting if your reference clk is an extremly accurate one.

Is there a need to model clock accuracy across the clock chain? I'm OK
modeling it in DT, and the code to do it in the clk framework isn't very
much ... but I also wonder if we're just adding complexity for no
reason.

> 
> >
> > I would also prefer to see an unknown accuracy be -1.
> I decided to keep 0 as a default value for unimplemented recalc_accuracy
> (or unspecified fixed accuracy) to keep existing implementation coherent.
> 
> 0 means the clk is perfect, and I thought it would be easier to handle a
> perfect clk (even if this is not really the case) than handling an error 
> case.
> 
> Another aspect is the propagation of the clk accuracy accross the clk tree.
> Returning -1 in the middle of the clk chain will drop the previous clk 
> accuracy
> calculation.
> 
> Anyway, I can change this if you think this is more appropriate.

What about the absence of the property? Instead of requiring a -1 value
can we simply detect that the property does not exist? This is nicer for
backwards compatibility with existing DTS.

Regards,
Mike

> 
> > There are already
> > clocks on the market (PPS reference clocks) with accuracies of
> > 0.1ppb/day [1].  Obviously, these aren't system clocks.  So the limit on
> > accuracy may be a non-issue.  However, it may be worth changing the
> > binding property to express the units.
> Wow, 0.1 ppb, this is impressive :-).
> 
> 
> This needs more than changing the dt bindings: I currently store the
> accuracy value in an unsigned long field, and expressing this in ppt
> (parts per trillion) may implies storing this in an u64 field (or store a
> unit field).
> 
> 
> >
> >> Signed-off-by: Boris BREZILLON <b.brezillon-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>   .../devicetree/bindings/clock/fixed-clock.txt      |    3 ++
> >>   drivers/clk/clk-fixed-rate.c                       |   43 +++++++++++++++++---
> >>   include/linux/clk-provider.h                       |    4 ++
> >>   3 files changed, 44 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> index 0b1fe78..48ea0ad 100644
> >> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> @@ -10,6 +10,8 @@ Required properties:
> >>   - clock-frequency : frequency of clock in Hz. Should be a single cell.
> >>   
> >>   Optional properties:
> >> +- clock-accuracy : accuracy of clock in ppb (parts per billion).
> >> +               Should be a single cell.
> > I would prefer to call this property 'clock-rated-ppb'.
> 
> Depending on what we choose to do with the accuracy field, this might be 
> an option.
> 
> >
> >>   - gpios : From common gpio binding; gpio connection to clock enable pin.
> >>   - clock-output-names : From common clock binding.
> >>   
> >> @@ -18,4 +20,5 @@ Example:
> >>              compatible = "fixed-clock";
> >>              #clock-cells = <0>;
> >>              clock-frequency = <1000000000>;
> >> +            clock-accuracy = <100>;
> >>      };
> > thx,
> >
> > Jason.
> >
> > [1] http://www.vectron.com/products/modules/md-010.htm
> 
> Thanks for your review, and don't hesitate to ask more questions, or to 
> point out
> incoherencies in my approach (I'm not an expert in clk and clk accuracy 
> calculation,
> and I might be wrong ;-)).
> 
> Best Regards,
> 
> Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[boris brezillon]

On 27/11/2013 19:10, Mike Turquette wrote:
> Quoting boris brezillon (2013-11-27 09:19:08)
>> Hi Jason,
>>
>> On 27/11/2013 15:56, Jason Cooper wrote:
>>> Boris,
>>>
>>> Thanks for posting this series.  Bear with me as I'm attempting to give
>>> MikeT a hand.
>> Nice to hear.
>> Mike already took a look at this series, but I'm happy to get more
>> feedbacks.
>>
>>> Don't be afraid to tell me a question is stupid :-)
>> Your questions are far from stupid ;-).
>>
>>> On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
>>>> This patch adds support for accuracy retrieval on fixed clocks.
>>>> It also adds a new dt property called 'clock-accuracy' to define the clock
>>>> accuracy.
>>>>
>>>> This can be usefull for oscillator (RC, crystal, ...) definitions which are
>>>> always given an accuracy characteristic.
>>> I think we need to be more explicit in the binding and the API,
>>> especially when providing a method to recalculate the accuracy.  I
>>> presume this recalculated value would be relative against the root
>>> clock?
>> Yes, indirectly.
>> Actually the clk accuracy depends on the whole clock chain, and is
>> calculated
>> either by comparing the real clk rate to the theorical clk rate
>> (accuracy = absolute_value((theorical_clk_rate - real_clk_rate)) /
>> theorical_clk_rate),
>> or by adding all the accuracies (expressed in ppm, ppb or ppt) of the
>> clk chain
>> (accuracy = current_clk_accuracy + parent_clk_accuracy).
>>
>> Say you have a root clk with a +-10000 ppb accuracy, then a pll multiplying
>> this root clk by 40 and introducing a possible drift of +- 1000 ppb and
>> eventually a system clk based on this pll with a perfect div by 2 prescaler
>> (accuracy = 0 ppb).
>>
>> If I understand correctly how accuracy propagates accross the clk tree,
>> you'll end up with a system clk with a +- 11000 ppb accuracy.
>>
>> e.g.:
>>    root clk = 12MHz +- 10000 ppb => 12 MHz * (1 - (10000 / 10^9)) < root
>> clk < 12 MHz * (1 + (10000 / 10^9))
>>                                                       => 11,99988 MHz <
>> root clk < 12,00012 MHz
>>    pll clk = ((root clk) * 40) +- 1000 ppb =>  (11,99988 MHz * 40) * (1 -
>> (1000 / 10^9)) < pll clk < (12,00012 MHz * 40) * (1 + (1000 / 10^9))
>>                                                              =>
>> 479,994720005 MHz < pll clk < 480,005280005 MHz
>>
>>    system clk = ((pll clk) / 2) +- XXX ppb => 479,994720005 MHz / 2 <
>> system clk < 480,005280005 MHz / 2
>>                                                                 =>
>> 239,997360002 MHz < system clk < 240,002640002 MHz
>>                                                                 => system
>> clk accuracy = 0,002640002 / 240 = 11000 ppb
>>
>> Please tell me if my assumptions are false.
>>> There really needs to be two attributes here:  the rated accuracy from
>>> the manufacturer, and the calculated accuracy wrt another clock in the
>>> system.  We only need a binding for the manufacturer rating since the
>>> calculated accuracy is determined at runtime.
>> Actually when I proposed this new functionnality I only had the theorical
>> (or manufacturer rated) accuracy in mind.
>> But providing an estimated accuracy (based on another clk) sounds
>> interresting if your reference clk is an extremly accurate one.
> Is there a need to model clock accuracy across the clock chain?
> I'm OK
> modeling it in DT, and the code to do it in the clk framework isn't very
> much ... but I also wonder if we're just adding complexity for no
> reason.

AFAIK the most important node in the clock chain (regarding accuracy)
is the root node.
But some nodes (like PLLs) might introduce more innacuracy.
This series propose a simple way (or at least tries to keep it simple 
:-)) to
express accuracy over the whole clk chain by means of the recalc_accuracy.

I'm not sure keeping the accuracy calculation (or retrieval) in the root 
clk node
only will simplify the calculation (or retrieval) of a leaf clk node 
accuracy (you'd
still have to walk over the clock chain to get the root clk accuracy).

My primary goal with this series is to provide a simple way (for a clock 
user) to
choose the most accurate clock among several available clocks.
This is a real need on AT91 platforms which provides internal RC oscillators
with a really poor accuracy (+- 5% <=> +- 50000 ppm).

>
>>> I would also prefer to see an unknown accuracy be -1.
>> I decided to keep 0 as a default value for unimplemented recalc_accuracy
>> (or unspecified fixed accuracy) to keep existing implementation coherent.
>>
>> 0 means the clk is perfect, and I thought it would be easier to handle a
>> perfect clk (even if this is not really the case) than handling an error
>> case.
>>
>> Another aspect is the propagation of the clk accuracy accross the clk tree.
>> Returning -1 in the middle of the clk chain will drop the previous clk
>> accuracy
>> calculation.
>>
>> Anyway, I can change this if you think this is more appropriate.
> What about the absence of the property?
> Instead of requiring a -1 value
> can we simply detect that the property does not exist? This is nicer for
> backwards compatibility with existing DTS.

I already handle the absence of the clock-accuracy property.
In this case the clock is considered perfect (accuracy = 0 ppb).

Mike, do you want me to return an error in the recalc_accuracy callback
to notifiy the absence of the clock-accuracy property ?

>
> Regards,
> Mike
>
>>> There are already
>>> clocks on the market (PPS reference clocks) with accuracies of
>>> 0.1ppb/day [1].  Obviously, these aren't system clocks.  So the limit on
>>> accuracy may be a non-issue.  However, it may be worth changing the
>>> binding property to express the units.
>> Wow, 0.1 ppb, this is impressive :-).
>>
>>
>> This needs more than changing the dt bindings: I currently store the
>> accuracy value in an unsigned long field, and expressing this in ppt
>> (parts per trillion) may implies storing this in an u64 field (or store a
>> unit field).
>>
>>
>>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>>> ---
>>>>    .../devicetree/bindings/clock/fixed-clock.txt      |    3 ++
>>>>    drivers/clk/clk-fixed-rate.c                       |   43 +++++++++++++++++---
>>>>    include/linux/clk-provider.h                       |    4 ++
>>>>    3 files changed, 44 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>>>> index 0b1fe78..48ea0ad 100644
>>>> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>>>> @@ -10,6 +10,8 @@ Required properties:
>>>>    - clock-frequency : frequency of clock in Hz. Should be a single cell.
>>>>    
>>>>    Optional properties:
>>>> +- clock-accuracy : accuracy of clock in ppb (parts per billion).
>>>> +               Should be a single cell.
>>> I would prefer to call this property 'clock-rated-ppb'.
>> Depending on what we choose to do with the accuracy field, this might be
>> an option.
>>
>>>>    - gpios : From common gpio binding; gpio connection to clock enable pin.
>>>>    - clock-output-names : From common clock binding.
>>>>    
>>>> @@ -18,4 +20,5 @@ Example:
>>>>               compatible = "fixed-clock";
>>>>               #clock-cells = <0>;
>>>>               clock-frequency = <1000000000>;
>>>> +            clock-accuracy = <100>;
>>>>       };
>>> thx,
>>>
>>> Jason.
>>>
>>> [1] http://www.vectron.com/products/modules/md-010.htm
>> Thanks for your review, and don't hesitate to ask more questions, or to
>> point out
>> incoherencies in my approach (I'm not an expert in clk and clk accuracy
>> calculation,
>> and I might be wrong ;-)).
>>
>> Best Regards,
>>
>> Boris

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[Jason Cooper]

Boris,

Sorry for the delay.

On Wed, Nov 27, 2013 at 06:19:08PM +0100, boris brezillon wrote:
> On 27/11/2013 15:56, Jason Cooper wrote:
> >On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
> >>This patch adds support for accuracy retrieval on fixed clocks.
> >>It also adds a new dt property called 'clock-accuracy' to define the clock
> >>accuracy.
> >>
> >>This can be usefull for oscillator (RC, crystal, ...) definitions which are
> >>always given an accuracy characteristic.
> >
> >I think we need to be more explicit in the binding and the API,
> >especially when providing a method to recalculate the accuracy.  I
> >presume this recalculated value would be relative against the root
> >clock?
> 
> Yes, indirectly.
> Actually the clk accuracy depends on the whole clock chain, and is
> calculated either by comparing the real clk rate to the theorical clk
> rate
> (accuracy = absolute_value((theorical_clk_rate - real_clk_rate)) /
> theorical_clk_rate),
> or by adding all the accuracies (expressed in ppm, ppb or ppt) of
> the clk chain
> (accuracy = current_clk_accuracy + parent_clk_accuracy).
> 
> Say you have a root clk with a +-10000 ppb accuracy, then a pll multiplying
> this root clk by 40 and introducing a possible drift of +- 1000 ppb and
> eventually a system clk based on this pll with a perfect div by 2 prescaler
> (accuracy = 0 ppb).
> 
> If I understand correctly how accuracy propagates accross the clk tree,
> you'll end up with a system clk with a +- 11000 ppb accuracy.
> 
> e.g.:
>  root clk = 12MHz +- 10000 ppb => 12 MHz * (1 - (10000 / 10^9)) <
> root clk < 12 MHz * (1 + (10000 / 10^9))
>                                                     => 11,99988 MHz
> < root clk < 12,00012 MHz
>  pll clk = ((root clk) * 40) +- 1000 ppb =>  (11,99988 MHz * 40) *
> (1 - (1000 / 10^9)) < pll clk < (12,00012 MHz * 40) * (1 + (1000 /
> 10^9))
>                                                            =>
> 479,994720005 MHz < pll clk < 480,005280005 MHz
> 
>  system clk = ((pll clk) / 2) +- XXX ppb => 479,994720005 MHz / 2 <
> system clk < 480,005280005 MHz / 2
>                                                               =>
> 239,997360002 MHz < system clk < 240,002640002 MHz
>                                                               =>
> system clk accuracy = 0,002640002 / 240 = 11000 ppb
> 
> Please tell me if my assumptions are false.

Nope, it looks fine by me afaict.  Thanks for the clear walk through.

> >There really needs to be two attributes here:  the rated accuracy from
> >the manufacturer, and the calculated accuracy wrt another clock in the
> >system.  We only need a binding for the manufacturer rating since the
> >calculated accuracy is determined at runtime.
> 
> Actually when I proposed this new functionnality I only had the theorical
> (or manufacturer rated) accuracy in mind.

Yes, I see we are concerned about two different things.  You need to get
the theoretical accuracy to assist with clock selection.  I was
concerned that the recalc function was attempting to measure the real
accuracy of a given clock from a tree.

Since we're only talking theoretical accuracy, that makes things a lot
simpler.  :)

> But providing an estimated accuracy (based on another clk) sounds
> interresting if your reference clk is an extremly accurate one.

Yes, I was thinking against a GPS PPS signal, but again, not relevant to
this patch series.  Also, it would be complicated by the fact that there
is no high-speed counter on ARM.

> >I would also prefer to see an unknown accuracy be -1.
> I decided to keep 0 as a default value for unimplemented recalc_accuracy
> (or unspecified fixed accuracy) to keep existing implementation coherent.
> 
> 0 means the clk is perfect, and I thought it would be easier to handle a
> perfect clk (even if this is not really the case) than handling an
> error case.
> 
> Another aspect is the propagation of the clk accuracy accross the clk tree.
> Returning -1 in the middle of the clk chain will drop the previous
> clk accuracy
> calculation.
> 
> Anyway, I can change this if you think this is more appropriate.

No, in light of this being purely theoretical accuracy, I'm fine with it
if Mike is.

> >There are already
> >clocks on the market (PPS reference clocks) with accuracies of
> >0.1ppb/day [1].  Obviously, these aren't system clocks.  So the limit on
> >accuracy may be a non-issue.  However, it may be worth changing the
> >binding property to express the units.
> Wow, 0.1 ppb, this is impressive :-).
> 
> 
> This needs more than changing the dt bindings: I currently store the
> accuracy value in an unsigned long field, and expressing this in ppt
> (parts per trillion) may implies storing this in an u64 field (or store a
> unit field).

No, let's not derail this series.  ;-)  You've addressed my concerns.
Thanks for taking the time to bring me up to speed.

thx,

Jason.

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[Jason Cooper]

On Thu, Nov 28, 2013 at 09:02:58AM +0100, boris brezillon wrote:
> On 27/11/2013 19:10, Mike Turquette wrote:
> >Quoting boris brezillon (2013-11-27 09:19:08)
> >>>On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
...
> >>>I would also prefer to see an unknown accuracy be -1.
> >>I decided to keep 0 as a default value for unimplemented recalc_accuracy
> >>(or unspecified fixed accuracy) to keep existing implementation coherent.
> >>
> >>0 means the clk is perfect, and I thought it would be easier to handle a
> >>perfect clk (even if this is not really the case) than handling an error
> >>case.
> >>
> >>Another aspect is the propagation of the clk accuracy accross the clk tree.
> >>Returning -1 in the middle of the clk chain will drop the previous clk
> >>accuracy
> >>calculation.
> >>
> >>Anyway, I can change this if you think this is more appropriate.
> >What about the absence of the property?
> >Instead of requiring a -1 value
> >can we simply detect that the property does not exist? This is nicer for
> >backwards compatibility with existing DTS.
> 
> I already handle the absence of the clock-accuracy property.
> In this case the clock is considered perfect (accuracy = 0 ppb).

Yeah, in order to calculate the theoretical accuracy of a leaf node, a
missing accuracy in the middle of the chain really derails things.
Since my previous concern (modelling the real accuracy of the clocks) is
not an issue here, I think assuming the absent accuracy is 0 is fine.
Especially since all Boris is trying to do is avoid the RC clocks.

thx,

Jason.

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

[Uwe Kleine-König]

Hello,

On Wed, Nov 27, 2013 at 01:44:44PM +0100, Boris BREZILLON wrote:
> The clock accuracy is expressed in ppb (parts per billion) and represents
> the possible clock drift.
> Say you have a clock (e.g. an oscillator) which provides a fixed clock of
> 20MHz with an accuracy of +- 20Hz. This accuracy expressed in ppb is
> 20Hz/20MHz = 1000 ppb (or 1 ppm).
> 
> Clock users may need the clock accuracy information in order to choose
> the best clock (the one with the best accuracy) across several available
> clocks.
> 
> This patch adds clk accuracy retrieval support for common clk framework by
> means of a new function called clk_get_accuracy.
> This function returns the given clock accuracy expressed in ppb.
> 
> In order to get the clock accuracy, this implementation adds one callback
> called recalc_accuracy to the clk_ops structure.
> This callback is given the parent clock accuracy (if the clock is not a
> root clock) and should recalculate the given clock accuracy.
> 
> This callback is optional and may be implemented if the clock is not
> a perfect clock (accuracy != 0 ppb).
> 
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> ---
>  Documentation/clk.txt        |    4 ++
>  drivers/clk/clk.c            |  109 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/clk-private.h  |    1 +
>  include/linux/clk-provider.h |   11 +++++
>  include/linux/clk.h          |   17 +++++++
>  5 files changed, 138 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
> index 3aeb5c4..eb20198 100644
> --- a/Documentation/clk.txt
> +++ b/Documentation/clk.txt
> @@ -77,6 +77,8 @@ the operations defined in clk.h:
>  		int		(*set_parent)(struct clk_hw *hw, u8 index);
>  		u8		(*get_parent)(struct clk_hw *hw);
>  		int		(*set_rate)(struct clk_hw *hw, unsigned long);
> +		unsigned long	(*recalc_accuracy)(struct clk_hw *hw,
> +						   unsigned long parent_accuracy);
>  		void		(*init)(struct clk_hw *hw);
>  	};
>  
> @@ -202,6 +204,8 @@ optional or must be evaluated on a case-by-case basis.
>  .set_parent     |      |             | n             | y           | n    |
>  .get_parent     |      |             | n             | y           | n    |
>                  |      |             |               |             |      |
> +.recalc_accuracy|      |             |               |             |      |
> +                |      |             |               |             |      |
>  .init           |      |             |               |             |      |
>                  -----------------------------------------------------------
>  [1] either one of round_rate or determine_rate is required.
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2cf2ea6..4b0c1bc 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -104,10 +104,11 @@ static void clk_summary_show_one(struct seq_file *s, struct clk *c, int level)
>  	if (!c)
>  		return;
>  
> -	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu",
> +	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu %-11lu",
>  		   level * 3 + 1, "",
>  		   30 - level * 3, c->name,
> -		   c->enable_count, c->prepare_count, clk_get_rate(c));
> +		   c->enable_count, c->prepare_count, clk_get_rate(c),
> +		   clk_get_accuracy(c));
>  	seq_printf(s, "\n");
>  }
>  
> @@ -129,8 +130,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
>  {
>  	struct clk *c;
>  
> -	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate\n");
> -	seq_printf(s, "---------------------------------------------------------------------\n");
> +	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate        accuracy\n");
> +	seq_printf(s, "---------------------------------------------------------------------------------\n");
>  
>  	clk_prepare_lock();
>  
> @@ -167,6 +168,7 @@ static void clk_dump_one(struct seq_file *s, struct clk *c, int level)
>  	seq_printf(s, "\"enable_count\": %d,", c->enable_count);
>  	seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
>  	seq_printf(s, "\"rate\": %lu", clk_get_rate(c));
> +	seq_printf(s, "\"accuracy\": %lu", clk_get_accuracy(c));
>  }
>  
>  static void clk_dump_subtree(struct seq_file *s, struct clk *c, int level)
> @@ -248,6 +250,11 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
>  	if (!d)
>  		goto err_out;
>  
> +	d = debugfs_create_u32("clk_accuracy", S_IRUGO, clk->dentry,
> +			(u32 *)&clk->accuracy);
> +	if (!d)
> +		goto err_out;
> +
>  	d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
>  			(u32 *)&clk->flags);
>  	if (!d)
> @@ -602,6 +609,28 @@ out:
>  	return ret;
>  }
>  
> +unsigned long __clk_get_accuracy(struct clk *clk)
> +{
> +	unsigned long ret;
> +
> +	if (!clk) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = clk->accuracy;
> +
> +	if (clk->flags & CLK_IS_ROOT)
> +		goto out;
> +
> +	if (!clk->parent)
> +		ret = 0;
Why do you need this? Wouldn't it be enough to assert clk->accuracy
being 0 in this case?

> +out:
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(__clk_get_accuracy);
> +
>  unsigned long __clk_get_flags(struct clk *clk)
>  {
>  	return !clk ? 0 : clk->flags;
> @@ -1016,6 +1045,59 @@ static int __clk_notify(struct clk *clk, unsigned long msg,
>  }
>  
>  /**
> + * __clk_recalc_accuracies
> + * @clk: first clk in the subtree
> + * @msg: notification type (see include/linux/clk.h)
There is no msg parameter in this function.

> + * Walks the subtree of clks starting with clk and recalculates accuracies as
> + * it goes.  Note that if a clk does not implement the .recalc_rate callback
> + * then it is assumed that the clock will take on the rate of it's parent.
> + *
> + * Caller must hold prepare_lock.
> + */
> +static void __clk_recalc_accuracies(struct clk *clk)
> +{
> +	unsigned long parent_accuracy = 0;
> +	struct clk *child;
> +
> +	if (clk->parent)
> +		parent_accuracy = clk->parent->accuracy;
> +
> +	if (clk->ops->recalc_accuracy)
> +		clk->accuracy = clk->ops->recalc_accuracy(clk->hw,
> +							  parent_accuracy);
> +	else
> +		clk->accuracy = parent_accuracy;
> +
> +	hlist_for_each_entry(child, &clk->children, child_node)
> +		__clk_recalc_accuracies(child);
> +}
> +
> +/**
> + * 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.
> + */
> +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);
> +
> +	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)
> @@ -1551,6 +1633,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);
>  	__clk_recalc_rates(clk, POST_RATE_CHANGE);
>  }
>  
> @@ -1621,6 +1704,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);
> +
Would it make sense to move this call into the if (ret) below? The rate
is only recalculated there, too.

>  	/* propagate rate recalculation accordingly */
>  	if (ret)
>  		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[boris brezillon]

Hello Uwe,

Le 02/12/2013 08:50, Uwe Kleine-König a écrit :
> Hello,
>
> On Wed, Nov 27, 2013 at 01:44:44PM +0100, Boris BREZILLON wrote:
>> The clock accuracy is expressed in ppb (parts per billion) and represents
>> the possible clock drift.
>> Say you have a clock (e.g. an oscillator) which provides a fixed clock of
>> 20MHz with an accuracy of +- 20Hz. This accuracy expressed in ppb is
>> 20Hz/20MHz = 1000 ppb (or 1 ppm).
>>
>> Clock users may need the clock accuracy information in order to choose
>> the best clock (the one with the best accuracy) across several available
>> clocks.
>>
>> This patch adds clk accuracy retrieval support for common clk framework by
>> means of a new function called clk_get_accuracy.
>> This function returns the given clock accuracy expressed in ppb.
>>
>> In order to get the clock accuracy, this implementation adds one callback
>> called recalc_accuracy to the clk_ops structure.
>> This callback is given the parent clock accuracy (if the clock is not a
>> root clock) and should recalculate the given clock accuracy.
>>
>> This callback is optional and may be implemented if the clock is not
>> a perfect clock (accuracy != 0 ppb).
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>> ---
>>   Documentation/clk.txt        |    4 ++
>>   drivers/clk/clk.c            |  109 ++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/clk-private.h  |    1 +
>>   include/linux/clk-provider.h |   11 +++++
>>   include/linux/clk.h          |   17 +++++++
>>   5 files changed, 138 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
>> index 3aeb5c4..eb20198 100644
>> --- a/Documentation/clk.txt
>> +++ b/Documentation/clk.txt
>> @@ -77,6 +77,8 @@ the operations defined in clk.h:
>>   		int		(*set_parent)(struct clk_hw *hw, u8 index);
>>   		u8		(*get_parent)(struct clk_hw *hw);
>>   		int		(*set_rate)(struct clk_hw *hw, unsigned long);
>> +		unsigned long	(*recalc_accuracy)(struct clk_hw *hw,
>> +						   unsigned long parent_accuracy);
>>   		void		(*init)(struct clk_hw *hw);
>>   	};
>>   
>> @@ -202,6 +204,8 @@ optional or must be evaluated on a case-by-case basis.
>>   .set_parent     |      |             | n             | y           | n    |
>>   .get_parent     |      |             | n             | y           | n    |
>>                   |      |             |               |             |      |
>> +.recalc_accuracy|      |             |               |             |      |
>> +                |      |             |               |             |      |
>>   .init           |      |             |               |             |      |
>>                   -----------------------------------------------------------
>>   [1] either one of round_rate or determine_rate is required.
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 2cf2ea6..4b0c1bc 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -104,10 +104,11 @@ static void clk_summary_show_one(struct seq_file *s, struct clk *c, int level)
>>   	if (!c)
>>   		return;
>>   
>> -	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu",
>> +	seq_printf(s, "%*s%-*s %-11d %-12d %-10lu %-11lu",
>>   		   level * 3 + 1, "",
>>   		   30 - level * 3, c->name,
>> -		   c->enable_count, c->prepare_count, clk_get_rate(c));
>> +		   c->enable_count, c->prepare_count, clk_get_rate(c),
>> +		   clk_get_accuracy(c));
>>   	seq_printf(s, "\n");
>>   }
>>   
>> @@ -129,8 +130,8 @@ static int clk_summary_show(struct seq_file *s, void *data)
>>   {
>>   	struct clk *c;
>>   
>> -	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate\n");
>> -	seq_printf(s, "---------------------------------------------------------------------\n");
>> +	seq_printf(s, "   clock                        enable_cnt  prepare_cnt  rate        accuracy\n");
>> +	seq_printf(s, "---------------------------------------------------------------------------------\n");
>>   
>>   	clk_prepare_lock();
>>   
>> @@ -167,6 +168,7 @@ static void clk_dump_one(struct seq_file *s, struct clk *c, int level)
>>   	seq_printf(s, "\"enable_count\": %d,", c->enable_count);
>>   	seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
>>   	seq_printf(s, "\"rate\": %lu", clk_get_rate(c));
>> +	seq_printf(s, "\"accuracy\": %lu", clk_get_accuracy(c));
>>   }
>>   
>>   static void clk_dump_subtree(struct seq_file *s, struct clk *c, int level)
>> @@ -248,6 +250,11 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
>>   	if (!d)
>>   		goto err_out;
>>   
>> +	d = debugfs_create_u32("clk_accuracy", S_IRUGO, clk->dentry,
>> +			(u32 *)&clk->accuracy);
>> +	if (!d)
>> +		goto err_out;
>> +
>>   	d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
>>   			(u32 *)&clk->flags);
>>   	if (!d)
>> @@ -602,6 +609,28 @@ out:
>>   	return ret;
>>   }
>>   
>> +unsigned long __clk_get_accuracy(struct clk *clk)
>> +{
>> +	unsigned long ret;
>> +
>> +	if (!clk) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>> +	ret = clk->accuracy;
>> +
>> +	if (clk->flags & CLK_IS_ROOT)
>> +		goto out;
>> +
>> +	if (!clk->parent)
>> +		ret = 0;
> Why do you need this? Wouldn't it be enough to assert clk->accuracy
> being 0 in this case?

I'm not sure.
I did this because it was done this way for __clk_get_rate (which is
not a valid argument ;)).

Now that I think about it I don't find any good reason for not directly
returning the clk->accuracy (even if the clk is orphan).

I mean, if we consider the parent_clk as perfect (which is the case if
the clk is orphan), then the child clk might still introduce some 
inaccuracy.
And I think it's better to return the maximum clk accuracy instead of
considering the clk is perfect.

I'll fix this in the next version.

>> +out:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(__clk_get_accuracy);
>> +
>>   unsigned long __clk_get_flags(struct clk *clk)
>>   {
>>   	return !clk ? 0 : clk->flags;
>> @@ -1016,6 +1045,59 @@ static int __clk_notify(struct clk *clk, unsigned long msg,
>>   }
>>   
>>   /**
>> + * __clk_recalc_accuracies
>> + * @clk: first clk in the subtree
>> + * @msg: notification type (see include/linux/clk.h)
> There is no msg parameter in this function.

I'll fix the comment.

>
>> + * Walks the subtree of clks starting with clk and recalculates accuracies as
>> + * it goes.  Note that if a clk does not implement the .recalc_rate callback
>> + * then it is assumed that the clock will take on the rate of it's parent.
>> + *
>> + * Caller must hold prepare_lock.
>> + */
>> +static void __clk_recalc_accuracies(struct clk *clk)
>> +{
>> +	unsigned long parent_accuracy = 0;
>> +	struct clk *child;
>> +
>> +	if (clk->parent)
>> +		parent_accuracy = clk->parent->accuracy;
>> +
>> +	if (clk->ops->recalc_accuracy)
>> +		clk->accuracy = clk->ops->recalc_accuracy(clk->hw,
>> +							  parent_accuracy);
>> +	else
>> +		clk->accuracy = parent_accuracy;
>> +
>> +	hlist_for_each_entry(child, &clk->children, child_node)
>> +		__clk_recalc_accuracies(child);
>> +}
>> +
>> +/**
>> + * 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.
>> + */
>> +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);
>> +
>> +	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)
>> @@ -1551,6 +1633,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);
>>   	__clk_recalc_rates(clk, POST_RATE_CHANGE);
>>   }
>>   
>> @@ -1621,6 +1704,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);
>> +
> Would it make sense to move this call into the if (ret) below? The rate
> is only recalculated there, too.
You mean only if __clk_set_parent returns 0.
Yes it makes sense: the accuracy should not change if the parent hasn't 
changed
(is this case, if we fail to change the parent).

BTW, the rate is recalculated anyway, the only thing that changes is the 
notification
message.

I'll fix this if no ones finds a good reason to recalculate the accuracy 
when we fail
to change the parent.

Thanks for your review

Best Regars,

Boris
>>   	/* propagate rate recalculation accordingly */
>>   	if (ret)
>>   		__clk_recalc_rates(clk, ABORT_RATE_CHANGE);
> Best regards
> Uwe
>

Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

[Mike Turquette]

Quoting Jason Cooper (2013-12-01 19:15:58)
> On Thu, Nov 28, 2013 at 09:02:58AM +0100, boris brezillon wrote:
> > On 27/11/2013 19:10, Mike Turquette wrote:
> > >Quoting boris brezillon (2013-11-27 09:19:08)
> > >>>On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
> ...
> > >>>I would also prefer to see an unknown accuracy be -1.
> > >>I decided to keep 0 as a default value for unimplemented recalc_accuracy
> > >>(or unspecified fixed accuracy) to keep existing implementation coherent.
> > >>
> > >>0 means the clk is perfect, and I thought it would be easier to handle a
> > >>perfect clk (even if this is not really the case) than handling an error
> > >>case.
> > >>
> > >>Another aspect is the propagation of the clk accuracy accross the clk tree.
> > >>Returning -1 in the middle of the clk chain will drop the previous clk
> > >>accuracy
> > >>calculation.
> > >>
> > >>Anyway, I can change this if you think this is more appropriate.
> > >What about the absence of the property?
> > >Instead of requiring a -1 value
> > >can we simply detect that the property does not exist? This is nicer for
> > >backwards compatibility with existing DTS.
> > 
> > I already handle the absence of the clock-accuracy property.
> > In this case the clock is considered perfect (accuracy = 0 ppb).
> 
> Yeah, in order to calculate the theoretical accuracy of a leaf node, a
> missing accuracy in the middle of the chain really derails things.
> Since my previous concern (modelling the real accuracy of the clocks) is
> not an issue here, I think assuming the absent accuracy is 0 is fine.
> Especially since all Boris is trying to do is avoid the RC clocks.

Agreed. If accuracy data must exist along the entire chain for the
calculation to be successful then the feature will be useless due to
lack of adoption of the accuracy data.

Regards,
Mike

> 
> thx,
> 
> Jason.