[RFC 0/2] Basic divider clock

[RFC 0/2] Basic divider clock

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

This series contains a patch that was originally submitted by Mike
Turquette in August 2013, but never merged.[1] Although I didn't see
any comments rejecting it.

We have been using this code for our STB platform for quite some time
and wouldn't mind seeing it upstream. What are the current thoughts on
merging this code?

The patch is almost entirely how Mike submitted it originally. For the
purposes of this RFC, I kept my modifications separate, to make it
easier to see what is different from the original. I am expecting that
my modifications would need to be squashed into the main patch with
annotations in the commit message.

There were trivial conflicts applying the original patch to 4.11-rc1.
Those were entirely due to the patch context changing
(clk_hw_unregister_divider() didn't exist back then) and not due to any
incompatibilities with the patch itself.

The other changes I made are listed and described in patch 2/2.

[1] https://patchwork.kernel.org/patch/2848065/

Markus Mayer (1):
  clk: divider: modifications for 4.11

Mike Turquette (1):
  clk: dt: binding for basic divider clock

 .../devicetree/bindings/clock/divider-clock.txt    |  90 +++++++++++++++++++
 drivers/clk/clk-divider.c                          | 100 ++++++++++++++++++++-
 include/linux/clk-provider.h                       |   1 +
 3 files changed, 190 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/divider-clock.txt

-- 
2.7.4

[RFC 1/2] clk: dt: binding for basic divider clock

[Markus Mayer]

From: Mike Turquette <mturquette@linaro.org>

Devicetree binding for the basic clock divider, plus the setup function
to register the clock.  Based on the existing fixed-clock binding.

Tero Kristo contributed helpful bug fixes to this patch.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 .../devicetree/bindings/clock/divider-clock.txt    | 90 ++++++++++++++++++++
 drivers/clk/clk-divider.c                          | 98 +++++++++++++++++++++-
 include/linux/clk-provider.h                       |  1 +
 3 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/divider-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/divider-clock.txt b/Documentation/devicetree/bindings/clock/divider-clock.txt
new file mode 100644
index 0000000..ac80724
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/divider-clock.txt
@@ -0,0 +1,90 @@
+Binding for simple divider clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped adjustable clock rate divider that does not gate and has
+only one input clock or parent.  By default the value programmed into
+the register is one less than the actual divisor value.  E.g:
+
+register value		actual divisor value
+0			1
+1			2
+2			3
+
+This assumption may be modified by the following optional properties:
+
+index-starts-at-one - valid divisor values start at 1, not the default
+of 0.  E.g:
+register value		actual divisor value
+1			1
+2			2
+3			3
+
+index-power-of-two - valid divisor values are powers of two.  E.g:
+register value		actual divisor value
+0			1
+1			2
+2			4
+
+index-allow-zero - same as index_one, but zero is divide-by-1.  E.g:
+register value		actual divisor value
+0			1
+1			1
+2			2
+
+Additionally a table of valid dividers may be supplied like so:
+
+	table = <4 0>, <8, 1>;
+
+where the first value in the pair is the divider and the second value is
+the programmed register bitfield.
+
+The binding must also provide the register to control the divider and
+the mask for the corresponding control bits.  Optionally the number of
+bits to shift that mask, if necessary.  If the shift value is missing it
+is the same as supplying a zero shift.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "divider-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link to phandle of parent clock
+- reg : base address for register controlling adjustable divider
+- bit-mask : arbitrary bitmask for programming the adjustable divider
+
+Optional properties:
+- clock-output-names : from common clock binding.
+- table : array of integer pairs defining divisors & bitfield values
+- bit-shift : number of bits to shift the bit-mask, defaults to
+  (ffs(mask) - 1) if not present
+- minimum-divider : min divisor for dividing the input clock rate, only
+  needed if the first divisor is offset from the default value
+- maximum-divider : max divisor for dividing the input clock rate, only
+  needed if the max divisor is less than (mask + 1).
+- index-starts-at-one : valid divisor programming starts at 1, not zero
+- index-power-of-two : valid divisor programming must be a power of two
+- index-allow-zero : implies index-one, and programming zero results in
+  divide-by-one
+- hiword-mask : lower half of the register programs the divider, upper
+  half of the register indicates bits that were updated in the lower
+  half
+
+Examples:
+	clock_foo: clock_foo@4a008100 {
+		compatible = "divider-clock";
+		#clock-cells = <0>;
+		clocks = <&clock_baz>;
+		reg = <0x4a008100 0x4>
+		mask = <0x3>
+		maximum-divider = <3>
+	};
+
+	clock_bar: clock_bar@4a008108 {
+		#clock-cells = <0>;
+		compatible = "divider-clock";
+		clocks = <&clock_foo>;
+		reg = <0x4a008108 0x4>;
+		mask = <0x1>;
+		shift = <0>;
+		table = < 4 0 >, < 8 1 >;
+	};
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 96386ff..df7290c 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 2011 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
  * Copyright (C) 2011 Richard Zhao, Linaro <richard.zhao@linaro.org>
- * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
+ * Copyright (C) 2011-2013 Mike Turquette, Linaro Ltd <mturquette@linaro.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -17,6 +17,8 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/log2.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 /*
  * DOC: basic adjustable divider clock that cannot gate
@@ -612,3 +614,97 @@ void clk_hw_unregister_divider(struct clk_hw *hw)
 	kfree(div);
 }
 EXPORT_SYMBOL_GPL(clk_hw_unregister_divider);
+
+
+#ifdef CONFIG_OF
+struct clk_div_table *of_clk_get_div_table(struct device_node *node)
+{
+	int i;
+	u32 table_size;
+	struct clk_div_table *table;
+	const __be32 *tablespec;
+	u32 val;
+
+	tablespec = of_get_property(node, "table", &table_size);
+
+	if (!tablespec)
+		return NULL;
+
+	table_size /= sizeof(struct clk_div_table);
+
+	table = kzalloc(sizeof(struct clk_div_table) * table_size, GFP_KERNEL);
+	if (!table) {
+		pr_err("%s: unable to allocate memory for %s table\n", __func__, node->name);
+		return NULL;
+	}
+
+	for (i = 0; i < table_size; i++) {
+		of_property_read_u32_index(node, "table", i * 2, &val);
+		table[i].div = val;
+		of_property_read_u32_index(node, "table", i * 2 + 1, &val);
+		table[i].val = val;
+	}
+
+	return table;
+}
+
+/**
+ * of_divider_clk_setup() - Setup function for simple div rate clock
+ */
+void of_divider_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	void __iomem *reg;
+	const char *parent_name;
+	u8 clk_divider_flags = 0;
+	u32 mask = 0;
+	u32 shift = 0;
+	struct clk_div_table *table;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	reg = of_iomap(node, 0);
+	if (!reg) {
+		pr_err("%s: no memory mapped for property reg\n", __func__);
+		return;
+	}
+
+	if (of_property_read_u32(node, "bit-mask", &mask)) {
+		pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
+		return;
+	}
+
+	if (of_property_read_u32(node, "bit-shift", &shift)) {
+		shift = __ffs(mask);
+		pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
+				__func__, shift, node->name);
+	}
+
+	if (of_property_read_bool(node, "index-starts-at-one"))
+		clk_divider_flags |= CLK_DIVIDER_ONE_BASED;
+
+	if (of_property_read_bool(node, "index-power-of-two"))
+		clk_divider_flags |= CLK_DIVIDER_POWER_OF_TWO;
+
+	if (of_property_read_bool(node, "index-allow-zero"))
+		clk_divider_flags |= CLK_DIVIDER_ALLOW_ZERO;
+
+	if (of_property_read_bool(node, "hiword-mask"))
+		clk_divider_flags |= CLK_DIVIDER_HIWORD_MASK;
+
+	table = of_clk_get_div_table(node);
+	if (IS_ERR(table))
+		return;
+
+	clk = _register_divider(NULL, clk_name, parent_name, 0, reg, shift,
+			mask, clk_divider_flags, table, NULL);
+
+	if (!IS_ERR(clk))
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+EXPORT_SYMBOL_GPL(of_divider_clk_setup);
+CLK_OF_DECLARE(divider_clk, "divider-clock", of_divider_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a428aec..a2b4c68 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -439,6 +439,7 @@ struct clk_hw *clk_hw_register_divider_table(struct device *dev,
 		spinlock_t *lock);
 void clk_unregister_divider(struct clk *clk);
 void clk_hw_unregister_divider(struct clk_hw *hw);
+void of_divider_clk_setup(struct device_node *node);
 
 /**
  * struct clk_mux - multiplexer clock
-- 
2.7.4

[RFC 2/2] clk: divider: modifications for 4.11

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

These changes were applied on top of the original commit[1]. Some
changes are more stylistic in nature, others were necessary due to
changes in the clock framework.

- make of_clk_get_div_table() static
- turn "u32 table_size" into "unsigned int table_size"
- wrap two over-long pr_err() lines
- change _register_divider() call and related code, since it is
  returning a (struct clk_hw *) now instead of (struct clk *)

[1] https://patchwork.kernel.org/patch/2848065/

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/clk/clk-divider.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index df7290c..92605bc 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -617,10 +617,10 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_divider);
 
 
 #ifdef CONFIG_OF
-struct clk_div_table *of_clk_get_div_table(struct device_node *node)
+static struct clk_div_table *of_clk_get_div_table(struct device_node *node)
 {
 	int i;
-	u32 table_size;
+	unsigned int table_size;
 	struct clk_div_table *table;
 	const __be32 *tablespec;
 	u32 val;
@@ -634,7 +634,8 @@ struct clk_div_table *of_clk_get_div_table(struct device_node *node)
 
 	table = kzalloc(sizeof(struct clk_div_table) * table_size, GFP_KERNEL);
 	if (!table) {
-		pr_err("%s: unable to allocate memory for %s table\n", __func__, node->name);
+		pr_err("%s: unable to allocate memory for %s table\n", __func__,
+		       node->name);
 		return NULL;
 	}
 
@@ -653,7 +654,7 @@ struct clk_div_table *of_clk_get_div_table(struct device_node *node)
  */
 void of_divider_clk_setup(struct device_node *node)
 {
-	struct clk *clk;
+	struct clk_hw *hw;
 	const char *clk_name = node->name;
 	void __iomem *reg;
 	const char *parent_name;
@@ -673,7 +674,8 @@ void of_divider_clk_setup(struct device_node *node)
 	}
 
 	if (of_property_read_u32(node, "bit-mask", &mask)) {
-		pr_err("%s: missing bit-mask property for %s\n", __func__, node->name);
+		pr_err("%s: missing bit-mask property for %s\n", __func__,
+		       node->name);
 		return;
 	}
 
@@ -699,11 +701,11 @@ void of_divider_clk_setup(struct device_node *node)
 	if (IS_ERR(table))
 		return;
 
-	clk = _register_divider(NULL, clk_name, parent_name, 0, reg, shift,
+	hw = _register_divider(NULL, clk_name, parent_name, 0, reg, shift,
 			mask, clk_divider_flags, table, NULL);
 
-	if (!IS_ERR(clk))
-		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (!IS_ERR(hw))
+		of_clk_add_provider(node, of_clk_src_simple_get, hw->clk);
 }
 EXPORT_SYMBOL_GPL(of_divider_clk_setup);
 CLK_OF_DECLARE(divider_clk, "divider-clock", of_divider_clk_setup);
-- 
2.7.4

Re: [RFC 1/2] clk: dt: binding for basic divider clock

[Stephen Boyd]

On 03/14, Markus Mayer wrote:
> From: Mike Turquette <mturquette@linaro.org>
> 
> Devicetree binding for the basic clock divider, plus the setup function
> to register the clock.  Based on the existing fixed-clock binding.
> 
> Tero Kristo contributed helpful bug fixes to this patch.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>

These sorts of details should go into C code. Is there any
difficulty in writing a driver to do so? So far we've avoided
adding the generic divider, multiplier, mux, bindings because
those usually need some register level details that we've been
reluctant to add to DT.

That's because if something is wrong with these register level
details or we need to make some policy updates, e.g. favor this
divider over another, we can't really do that with a DT update.
So we really want to see DT nodes have some hardware specific
compatible string, and also put the logic for the driver into the
code so that it can be easily fixed and updated after the fact.

The DTS file is there to tell us "I have a divider from company X
here and it's connected to this and that" and the driver is there
to do the work of driving that piece of hardware so it works
properly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project