Basic divider clock

From: Markus Mayer <[email protected]>

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

From: Mike Turquette <[email protected]>

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.

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

new file mode 100644
index 0000000..ac80724
@@ -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 >;
+	};

index a428aec..a2b4c68 100644
@@ -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

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

From: Markus Mayer <[email protected]>

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/

---
 drivers/clk/clk-divider.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

index df7290c..92605bc 100644
@@ -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