[RFC PATCH RESEND 0/2] clk: add clk accuracy support

[RFC PATCH RESEND 0/2] clk: add clk accuracy support

[Boris BREZILLON]

Hello,

Sorry for the noise, but I didn't get any feedback on this patch series.

This patch series is a proposal to add clock accuracy retrieval support to the
common clk framework.

The support of accuracy retrieval may benefit to the at91 platform (I explain
why in the following paragraphs).

I don't know if other platforms may benefit from this accuracy support. This
series is here to get feedbacks from other developpers/maintainers, and see if
this can be integrated in the clk framework.

Here is why at91 platform may need the clk accuracy informations:

AT91 SoCs provide a slow clock (32 KHz clock) which can be used as a clock
source for some peripherals (USART, TC blocks, ...).

This slow clock can be generated from 2 sources:
- a 32KHz internal RC with a poor accuracy (50000 ppm)
- a 32KHz external crystal oscillator

Most of the supported at91 boards (if not all) use the external 32KHz crystal
source except for the kizbox (the board I'm working with :)).
It probably comes from a bad hardware design (hardware team should have
connected a crystal oscillator to the SoC instead of relying on the unaccurate
internal RC).
Anyway, I can't change the hardware as it is already widely deployed.

What I'm proposing is to give clock users the ability to retrieve clocks
accuracies in order to choose the most accurate source (or at least discard
clocks with poor accuracy).

Could you tell me if this approach is right, and if other platforms/boards have
similar issues and would be interrested by this series ?

Best Regards,

Boris

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/Kconfig                                |    4 +
 drivers/clk/clk-fixed-rate.c                       |   43 +++++++--
 drivers/clk/clk.c                                  |   92 +++++++++++++++++++-
 include/linux/clk-private.h                        |    1 +
 include/linux/clk-provider.h                       |   14 +++
 include/linux/clk.h                                |   17 ++++
 8 files changed, 168 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[RFC PATCH RESEND 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/Kconfig          |    4 ++
 drivers/clk/clk.c            |   92 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-private.h  |    1 +
 include/linux/clk-provider.h |   11 +++++
 include/linux/clk.h          |   17 ++++++++
 6 files changed, 125 insertions(+), 4 deletions(-)

diff --git a/Documentation/clk.txt b/Documentation/clk.txt
index 3aeb5c4..dc52da1 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_rate    |      |             |               |             |      |
+                |      |             |               |             |      |
 .init           |      |             |               |             |      |
                 -----------------------------------------------------------
 [1] either one of round_rate or determine_rate is required.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 279407a..4d12ae7 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -6,12 +6,16 @@ config CLKDEV_LOOKUP
 config HAVE_CLK_PREPARE
 	bool
 
+config HAVE_CLK_GET_ACCURACY
+	bool
+
 config HAVE_MACH_CLKDEV
 	bool
 
 config COMMON_CLK
 	bool
 	select HAVE_CLK_PREPARE
+	select HAVE_CLK_GET_ACCURACY
 	select CLKDEV_LOOKUP
 	---help---
 	  The common clock framework is a single definition of struct
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a004769..6a8f3ef 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,11 @@ out:
 	return ret;
 }
 
+unsigned long __clk_get_accuracy(struct clk *clk)
+{
+	return !clk ? 0 : clk->accuracy;
+}
+
 unsigned long __clk_get_flags(struct clk *clk)
 {
 	return !clk ? 0 : clk->flags;
@@ -1016,6 +1028,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.
+ */
+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);
+
+	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);
 	__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);
+
 	/* 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

[RFC PATCH RESEND 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                       |    3 ++
 3 files changed, 43 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 942811d..67878a4 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -212,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: [RFC PATCH RESEND 1/2] clk: add clk accuracy retrieval support

[Mike Turquette]

Quoting Boris BREZILLON (2013-10-13 10:17:10)
> 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/Kconfig          |    4 ++
>  drivers/clk/clk.c            |   92 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/clk-private.h  |    1 +
>  include/linux/clk-provider.h |   11 +++++
>  include/linux/clk.h          |   17 ++++++++
>  6 files changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
> index 3aeb5c4..dc52da1 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_rate    |      |             |               |             |      |

s/recalc_rate/recalc_accuracy/

> +                |      |             |               |             |      |
>  .init           |      |             |               |             |      |
>                  -----------------------------------------------------------
>  [1] either one of round_rate or determine_rate is required.
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 279407a..4d12ae7 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -6,12 +6,16 @@ config CLKDEV_LOOKUP
>  config HAVE_CLK_PREPARE
>         bool
>  
> +config HAVE_CLK_GET_ACCURACY
> +       bool
> +

This sort of thing gets messy. For platforms converted to common struct
clk we select HAVE_CLK_PREPARE and we let legacy platforms select it on
a case-by-case basis.

For something like HAVE_CLK_GET_ACCURACY I am inclined to only add it
for platforms converted to the common struct clk and not even expose it
to legacy clock framework implementations. In those cases the call to
clk_get_accuracy would return -EINVAL or -EPERM or something.

Russell, any thoughts on that approach?

>  config HAVE_MACH_CLKDEV
>         bool
>  
>  config COMMON_CLK
>         bool
>         select HAVE_CLK_PREPARE
> +       select HAVE_CLK_GET_ACCURACY
>         select CLKDEV_LOOKUP
>         ---help---
>           The common clock framework is a single definition of struct
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a004769..6a8f3ef 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,11 @@ out:
>         return ret;
>  }
>  
> +unsigned long __clk_get_accuracy(struct clk *clk)
> +{
> +       return !clk ? 0 : clk->accuracy;
> +}
> +
>  unsigned long __clk_get_flags(struct clk *clk)
>  {
>         return !clk ? 0 : clk->flags;
> @@ -1016,6 +1028,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.
> + */
> +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.

> +
> +       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.

>         __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.

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

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

[boris brezillon]

Hello Mike,

On 08/11/2013 01:51, Mike Turquette wrote:
> Quoting Boris BREZILLON (2013-10-13 10:17:10)
>> 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/Kconfig          |    4 ++
>>   drivers/clk/clk.c            |   92 ++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/clk-private.h  |    1 +
>>   include/linux/clk-provider.h |   11 +++++
>>   include/linux/clk.h          |   17 ++++++++
>>   6 files changed, 125 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
>> index 3aeb5c4..dc52da1 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_rate    |      |             |               |             |      |
> s/recalc_rate/recalc_accuracy/

Oops. I'll fix it :).
>
>> +                |      |             |               |             |      |
>>   .init           |      |             |               |             |      |
>>                   -----------------------------------------------------------
>>   [1] either one of round_rate or determine_rate is required.
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 279407a..4d12ae7 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -6,12 +6,16 @@ config CLKDEV_LOOKUP
>>   config HAVE_CLK_PREPARE
>>          bool
>>   
>> +config HAVE_CLK_GET_ACCURACY
>> +       bool
>> +
> This sort of thing gets messy. For platforms converted to common struct
> clk we select HAVE_CLK_PREPARE and we let legacy platforms select it on
> a case-by-case basis.
>
> For something like HAVE_CLK_GET_ACCURACY I am inclined to only add it
> for platforms converted to the common struct clk and not even expose it
> to legacy clock framework implementations. In those cases the call to
> clk_get_accuracy would return -EINVAL or -EPERM or something.
Okay.
If Russell agrees, I'll define a static inline function returning an
error (-ENOTSUPP ?) in case CONFIG_COMMON_CLK is not defined.

>
> Russell, any thoughts on that approach?
>
>>   config HAVE_MACH_CLKDEV
>>          bool
>>   
>>   config COMMON_CLK
>>          bool
>>          select HAVE_CLK_PREPARE
>> +       select HAVE_CLK_GET_ACCURACY
>>          select CLKDEV_LOOKUP
>>          ---help---
>>            The common clock framework is a single definition of struct
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index a004769..6a8f3ef 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,11 @@ out:
>>          return ret;
>>   }
>>   
>> +unsigned long __clk_get_accuracy(struct clk *clk)
>> +{
>> +       return !clk ? 0 : clk->accuracy;
>> +}
>> +
>>   unsigned long __clk_get_flags(struct clk *clk)
>>   {
>>          return !clk ? 0 : clk->flags;
>> @@ -1016,6 +1028,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.
>> + */
>> +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.

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 ?

>
>> +
>> +       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

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

[Mike Turquette]

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.

> 
> 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.

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.

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

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

[Mike Turquette]

Quoting Boris BREZILLON (2013-10-13 10:17:10)
> 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
...
> @@ -194,6 +204,7 @@ struct clk_hw {
>  struct clk_fixed_rate {
>         struct          clk_hw hw;
>         unsigned long   fixed_rate;
> +       unsigned long   fixed_accuracy;

This change belongs in patch #2. Please also update the kerneldoc above
this struct definition.

Regards,
Mike

>         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

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

[boris brezillon]

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

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

[boris brezillon]

On 17/11/2013 16:33, boris brezillon wrote:
> 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.
>

Oops: s/clk_recalc_rate/clk_recalc_accuracy/

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

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

[boris brezillon]

Hi Mike,

Nothing related to this thread, but if you have some time, could you 
take a look at
the last version (v5) of the at91 CCF port ?

I know Nicolas was planning to add this for 3.14, and we'd like to have 
your feedback
on the new dt bindings.

Best Regards,

Boris

On 16/11/2013 02:59, Mike Turquette wrote:
> Quoting Boris BREZILLON (2013-10-13 10:17:10)
>> 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
> ...
>> @@ -194,6 +204,7 @@ struct clk_hw {
>>   struct clk_fixed_rate {
>>          struct          clk_hw hw;
>>          unsigned long   fixed_rate;
>> +       unsigned long   fixed_accuracy;
> This change belongs in patch #2. Please also update the kerneldoc above
> this struct definition.
>
> Regards,
> Mike
>
>>          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