[PATCH RFC 0/3] clk: dt: bindings for mux & divider clocks

[PATCH RFC 0/3] clk: dt: bindings for mux & divider clocks

[Mike Turquette]

This series introduces binding definitions for common register-mapped
clock multiplexor and divider IP blocks, and the corresponding setup
functions once they are matched.  The bindings are close the struct
definitions but please don't hold that against the binding: the struct
definitions closely model the hardware.

The only missing basic clock type is the gate clock.  A binding for that
was posted some time back and is similar in spirit to these[1].  I guess
we'll need to decide whether register-level programming details belong
in DT.  I believe they do since those details describe the hardware.

Note that there is still no generic clock driver that matches these
basic types, but it would be trivial to write one.  Thoughts on that?
Is it better for each of the basic clock types to be a driver that
matches, or should there be one drivers/clk/clk-basic.c which matches
all of the basic clock building blocks?  I like the latter for aesthetic
purposes.

I am using this code while converting the OMAP4 clock data over to DT
and some common boilerplate code can be factored out of several clock
drivers if this is merged.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137878.html

Mike Turquette (3):
  clk: of: helper for determining number of parent clocks
  clk: dt: binding for basic multiplexor clock
  clk: dt: binding for basic divider clock

 .../devicetree/bindings/clock/divider-clock.txt    | 80 ++++++++++++++++++++
 .../devicetree/bindings/clock/mux-clock.txt        | 75 ++++++++++++++++++
 drivers/clk/clk-divider.c                          | 88 +++++++++++++++++++++-
 drivers/clk/clk-mux.c                              | 65 +++++++++++++++-
 drivers/clk/clk.c                                  |  6 ++
 include/linux/clk-provider.h                       |  8 +-
 6 files changed, 319 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/divider-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt

-- 
1.8.1.2

[PATCH RFC 1/3] clk: of: helper for determining number of parent clocks

[Mike Turquette]

Walks the "clocks" array of parent clock phandles and returns the
number.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
DT gurus, please let me know if this is stupid.  I am unfamiliar with
parsing DT blobs and associated APIs.

 drivers/clk/clk.c            | 6 ++++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af0dbcc..73e0358 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2082,6 +2082,12 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 	return clk;
 }
 
+int of_clk_get_parent_count(struct device_node *np)
+{
+	return of_count_phandle_with_args(np, "clocks", "#clock-cells");
+}
+EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
+
 const char *of_clk_get_parent_name(struct device_node *np, int index)
 {
 	struct of_phandle_args clkspec;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 265f384..f8d38f2 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -442,6 +442,7 @@ void of_clk_del_provider(struct device_node *np);
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				  void *data);
 struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data);
+int of_clk_get_parent_count(struct device_node *np);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);
-- 
1.8.1.2

[PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Mike Turquette]

Device Tree binding for the basic clock multiplexor, plus the setup
function to register the clock.  Based on the existing fixed-clock
binding.

Also relocate declaration of of_fixed_factor_clk_setup to keep things
tidy.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 .../devicetree/bindings/clock/mux-clock.txt        | 75 ++++++++++++++++++++++
 drivers/clk/clk-mux.c                              | 65 ++++++++++++++++++-
 include/linux/clk-provider.h                       |  5 +-
 3 files changed, 143 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
new file mode 100644
index 0000000..eda5ba3
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
@@ -0,0 +1,75 @@
+Binding for simple mux clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped multiplexor with multiple input clock signals or
+parents, one of which can be selected as output.  This clock does not
+gate or adjust the parent rate via a divider or multiplier.
+
+By default the "clocks" property lists the parents in the same order
+as they are programmed into the regster.  E.g:
+
+	clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
+
+results in programming the register as follows:
+
+register value		selected parent clock
+0			foo_clock
+1			bar_clock
+2			baz_clock
+
+Some clock controller IPs do not allow a value of zero to be programmed
+into the register, instead indexing begins at 1.  The optional property
+"index_one" modified the scheme as follows:
+
+register value		selected clock parent
+1			foo_clock
+2			bar_clock
+3			baz_clock
+
+Additionally an optional table of bit and parent pairs may be supplied
+like so:
+
+	table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
+
+where the first value in the pair is the parent clock and the second
+value is the bitfield to be programmed into the register.
+
+The binding must provide the register to control the mux 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 "mux-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks
+- reg : base address for register controlling adjustable mux
+- mask : arbitrary bitmask for programming the adjustable mux
+
+Optional properties:
+- clock-output-names : From common clock binding.
+- table : array of integer pairs defining parents & bitfield values
+- shift : number of bits to shift the mask, defaults to 0 if not present
+- index_one : valid input select programming starts at 1, not zero
+
+Examples:
+	clock: clock@4a008100 {
+		compatible = "mux-clock";
+		#clock-cells = <0>;
+		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
+		reg = <0x4a008100 0x4>
+		mask = <0x3>;
+		index_one;
+	};
+
+	clock: clock@4a008100 {
+		#clock-cells = <0>;
+		compatible = "mux-clock";
+		clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
+		reg = <0x4a008100 0x4>;
+		mask = <0x3>;
+		shift = <0>;
+		table = <&clock_foo 1>, <&clock_bar 2>, <&clock_baz 3>;
+	};
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 25b1734..8bcbc7c 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.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
@@ -16,6 +16,8 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 /*
  * DOC: basic adjustable multiplexer clock that cannot gate
@@ -153,3 +155,64 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 				      flags, reg, shift, mask, clk_mux_flags,
 				      NULL, lock);
 }
+
+#ifdef CONFIG_OF
+/**
+ * of_mux_clk_setup() - Setup function for simple mux rate clock
+ */
+void of_mux_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	void __iomem *reg;
+	int num_parents;
+	const char **parent_names;
+	int i;
+	u8 clk_mux_flags = 0;
+	u32 mask = 0;
+	u32 shift = 0;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+	pr_err("%s: clk_name is %s\n", __func__, clk_name);
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents < 1) {
+		pr_err("%s: mux-clock %s must have parent(s)\n",
+				__func__, node->name);
+		return;
+	}
+	pr_err("%s: num_parents is %d\n", __func__, num_parents);
+
+	parent_names = kzalloc((sizeof(char*) * num_parents),
+			GFP_KERNEL);
+
+	for (i = 0; i < num_parents; i++) {
+		parent_names[i] = of_clk_get_parent_name(node, i);
+		pr_err("%s: parent_names[%d] is %s\n", __func__, i, parent_names[i]);
+	}
+
+	reg = of_iomap(node, 0);
+	pr_err("%s: reg is 0x%p\n", __func__, reg);
+
+	if (of_property_read_u32(node, "mask", &mask)) {
+		pr_err("%s: missing mask property for %s\n", __func__, node->name);
+		return;
+	}
+
+	if (of_property_read_u32(node, "shift", &shift))
+		pr_debug("%s: missing shift property defaults to zero for %s\n",
+				__func__, node->name);
+
+	if (of_property_read_bool(node, "index_one"))
+		clk_mux_flags |= CLK_MUX_INDEX_ONE;
+
+	clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
+			0, reg, 0, mask, clk_mux_flags,
+			NULL, NULL);
+
+	if (!IS_ERR(clk))
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+EXPORT_SYMBOL_GPL(of_mux_clk_setup);
+CLK_OF_DECLARE(mux_clk, "mux-clock", of_mux_clk_setup);
+#endif
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f8d38f2..9c404c2 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -325,7 +325,7 @@ struct clk *clk_register_mux_table(struct device *dev, const char *name,
 		void __iomem *reg, u8 shift, u32 mask,
 		u8 clk_mux_flags, u32 *table, spinlock_t *lock);
 
-void of_fixed_factor_clk_setup(struct device_node *node);
+void of_mux_clk_setup(struct device_node *node);
 
 /**
  * struct clk_fixed_factor - fixed multiplier and divider clock
@@ -346,10 +346,13 @@ struct clk_fixed_factor {
 };
 
 extern struct clk_ops clk_fixed_factor_ops;
+
 struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 
+void of_fixed_factor_clk_setup(struct device_node *node);
+
 /***
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *
-- 
1.8.1.2

[PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Mike Turquette]

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

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 .../devicetree/bindings/clock/divider-clock.txt    | 80 ++++++++++++++++++++
 drivers/clk/clk-divider.c                          | 88 +++++++++++++++++++++-
 include/linux/clk-provider.h                       |  2 +
 3 files changed, 169 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..31e9273
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/divider-clock.txt
@@ -0,0 +1,80 @@
+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_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
+
+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
+- 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
+- shift : number of bits to shift the mask, defaults to 0 if not present
+- index_one : valid divisor programming starts at 1, not zero
+- index_power_of_two : valid divisor programming must be a power of two
+- allow_zero : implies index_one, and programming zero results in
+  divide-by-one
+
+Examples:
+	clock_foo: clock_foo@4a008100 {
+		compatible = "divider-clock";
+		#clock-cells = <0>;
+		clocks = <&clock_baz>;
+		reg = <0x4a008100 0x4>
+		mask = <0x3>
+	};
+
+	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 6d96741..3b76591 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
@@ -320,3 +322,87 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
 	return _register_divider(dev, name, parent_name, flags, reg, shift,
 			width, clk_divider_flags, table, lock);
 }
+
+#ifdef CONFIG_OF
+struct clk_div_table *of_clk_get_div_table(struct device_node *node)
+{
+	int i;
+	int table_size = 0;
+	struct clk_div_table *table;
+	u32 pair[2];
+
+	table_size = of_count_phandle_with_args(node, "table", "#clock-cells");
+
+	if (table_size < 1)
+		return NULL;
+
+	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++) {
+		if (!of_property_read_u32_array(node, "table", pair, ARRAY_SIZE(pair))) {
+			table[i].val = pair[0];
+			table[i].div = pair[1];
+		}
+	}
+
+	return table;
+}
+
+/**
+ * of_div_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;
+	u8 mask = 0;
+	u8 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 (of_property_read_u8(node, "mask", &mask)) {
+		pr_err("%s: missing mask property for %s\n", __func__, node->name);
+		return;
+	}
+
+	if (of_property_read_u8(node, "shift", &shift))
+		pr_debug("%s: missing shift property defaults to zero for %s\n",
+				__func__, node->name);
+
+	if (of_property_read_bool(node, "index_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;
+
+	table = of_clk_get_div_table(node);
+	if (IS_ERR(table))
+		return;
+
+	clk = clk_register_divider_table(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 9c404c2..63521e7 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -283,6 +283,8 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
 		u8 clk_divider_flags, const struct clk_div_table *table,
 		spinlock_t *lock);
 
+void of_divider_clk_setup(struct device_node *node);
+
 /**
  * struct clk_mux - multiplexer clock
  *
-- 
1.8.1.2

Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Heiko Stübner]

Hi Mike,

I think it's a multiplexEr clock in the patch title, and see below


Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
> Device Tree binding for the basic clock multiplexor, plus the setup
> function to register the clock.  Based on the existing fixed-clock
> binding.
> 
> Also relocate declaration of of_fixed_factor_clk_setup to keep things
> tidy.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>

[...]

> +
> +	reg = of_iomap(node, 0);
> +	pr_err("%s: reg is 0x%p\n", __func__, reg);
> +
> +	if (of_property_read_u32(node, "mask", &mask)) {
> +		pr_err("%s: missing mask property for %s\n", __func__, node->name);
> +		return;
> +	}
> +
> +	if (of_property_read_u32(node, "shift", &shift))
> +		pr_debug("%s: missing shift property defaults to zero for %s\n",
> +				__func__, node->name);
> +
> +	if (of_property_read_bool(node, "index_one"))
> +		clk_mux_flags |= CLK_MUX_INDEX_ONE;
> +
> +	clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
> +			0, reg, 0, mask, clk_mux_flags,

                      ^- should probably be shift


Otherwise looks cool and I'm currently trying it with my Rockchip code.


Heiko

Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Mike Turquette]

Quoting Heiko Stübner (2013-06-03 12:33:19)
> Hi Mike,
> 
> I think it's a multiplexEr clock in the patch title, and see below
> 

Doh, you are right.  But "xor" is so much cooler looking than "xer"...

> 
> Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
> > Device Tree binding for the basic clock multiplexor, plus the setup
> > function to register the clock.  Based on the existing fixed-clock
> > binding.
> > 
> > Also relocate declaration of of_fixed_factor_clk_setup to keep things
> > tidy.
> > 
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> 
> [...]
> 
> > +
> > +     reg = of_iomap(node, 0);
> > +     pr_err("%s: reg is 0x%p\n", __func__, reg);
> > +
> > +     if (of_property_read_u32(node, "mask", &mask)) {
> > +             pr_err("%s: missing mask property for %s\n", __func__, node->name);
> > +             return;
> > +     }
> > +
> > +     if (of_property_read_u32(node, "shift", &shift))
> > +             pr_debug("%s: missing shift property defaults to zero for %s\n",
> > +                             __func__, node->name);
> > +
> > +     if (of_property_read_bool(node, "index_one"))
> > +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
> > +
> > +     clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
> > +                     0, reg, 0, mask, clk_mux_flags,
> 
>                       ^- should probably be shift
> 
> 
> Otherwise looks cool and I'm currently trying it with my Rockchip code.
> 

Right again.  My test platform seems to not shift the mask at all so
this did not cause a visible bug for me.

Thanks for the review,
Mike

> 
> Heiko

Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Heiko Stübner]

Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
> Quoting Heiko Stübner (2013-06-03 12:33:19)
> 
> > Hi Mike,
> > 
> > I think it's a multiplexEr clock in the patch title, and see below
> 
> Doh, you are right.  But "xor" is so much cooler looking than "xer"...
> 
> > Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
> > > Device Tree binding for the basic clock multiplexor, plus the setup
> > > function to register the clock.  Based on the existing fixed-clock
> > > binding.
> > > 
> > > Also relocate declaration of of_fixed_factor_clk_setup to keep things
> > > tidy.
> > > 
> > > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > 
> > [...]
> > 
> > > +
> > > +     reg = of_iomap(node, 0);
> > > +     pr_err("%s: reg is 0x%p\n", __func__, reg);
> > > +
> > > +     if (of_property_read_u32(node, "mask", &mask)) {
> > > +             pr_err("%s: missing mask property for %s\n", __func__,
> > > node->name); +             return;
> > > +     }
> > > +
> > > +     if (of_property_read_u32(node, "shift", &shift))
> > > +             pr_debug("%s: missing shift property defaults to zero for
> > > %s\n", +                             __func__, node->name);
> > > +
> > > +     if (of_property_read_bool(node, "index_one"))
> > > +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
> > > +
> > > +     clk = clk_register_mux_table(NULL, clk_name, parent_names,
> > > num_parents, +                     0, reg, 0, mask, clk_mux_flags,
> > > 
> >                       ^- should probably be shift
> > 
> > Otherwise looks cool and I'm currently trying it with my Rockchip code.
> 
> Right again.  My test platform seems to not shift the mask at all so
> this did not cause a visible bug for me.

I'm currently converting my rockchip stuff to it, as it would solve a lot of 
problems with the numerous clock types I'm defining just to put the shift and 
mask values somewhere.

So lets see if anything more turns up ;-)

Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Heiko Stübner]

Am Montag, 3. Juni 2013, 22:15:45 schrieb Heiko Stübner:
> Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
> > Quoting Heiko Stübner (2013-06-03 12:33:19)
> > 
> > > Hi Mike,
> > > 
> > > I think it's a multiplexEr clock in the patch title, and see below
> > 
> > Doh, you are right.  But "xor" is so much cooler looking than "xer"...
> > 
> > > Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
> > > > Device Tree binding for the basic clock multiplexor, plus the setup
> > > > function to register the clock.  Based on the existing fixed-clock
> > > > binding.
> > > > 
> > > > Also relocate declaration of of_fixed_factor_clk_setup to keep things
> > > > tidy.
> > > > 
> > > > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > > 
> > > [...]
> > > 
> > > > +
> > > > +     reg = of_iomap(node, 0);
> > > > +     pr_err("%s: reg is 0x%p\n", __func__, reg);
> > > > +
> > > > +     if (of_property_read_u32(node, "mask", &mask)) {
> > > > +             pr_err("%s: missing mask property for %s\n", __func__,
> > > > node->name); +             return;
> > > > +     }
> > > > +
> > > > +     if (of_property_read_u32(node, "shift", &shift))
> > > > +             pr_debug("%s: missing shift property defaults to zero
> > > > for %s\n", +                             __func__, node->name);
> > > > +
> > > > +     if (of_property_read_bool(node, "index_one"))
> > > > +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
> > > > +
> > > > +     clk = clk_register_mux_table(NULL, clk_name, parent_names,
> > > > num_parents, +                     0, reg, 0, mask, clk_mux_flags,
> > > > 
> > >                       ^- should probably be shift
> > > 
> > > Otherwise looks cool and I'm currently trying it with my Rockchip code.
> > 
> > Right again.  My test platform seems to not shift the mask at all so
> > this did not cause a visible bug for me.
> 
> I'm currently converting my rockchip stuff to it, as it would solve a lot
> of problems with the numerous clock types I'm defining just to put the
> shift and mask values somewhere.
> 
> So lets see if anything more turns up ;-)

yep ... of_mux_clk_setup is quite talkative ... there are quite some (debug-?) 
pr_errs in it ;-)


Heiko

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Heiko Stübner]

Am Montag, 3. Juni 2013, 19:53:10 schrieb Mike Turquette:
> Devicetree binding for the basic clock divider, plus the setup function
> to register the clock.  Based on the existing fixed-clock binding.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---

[...]

> +/**
> + * of_div_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;
> +	u8 mask = 0;
> +	u8 shift = 0;

in the mux-clock these 3 are unsigned long and u32 types ... what is correct?


> +	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 (of_property_read_u8(node, "mask", &mask)) {
> +		pr_err("%s: missing mask property for %s\n", __func__, node->name);
> +		return;
> +	}
> +
> +	if (of_property_read_u8(node, "shift", &shift))
> +		pr_debug("%s: missing shift property defaults to zero for %s\n",
> +				__func__, node->name);

same here ... mux reads u32

> +	if (of_property_read_bool(node, "index_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;
> +
> +	table = of_clk_get_div_table(node);
> +	if (IS_ERR(table))
> +		return;
> +
> +	clk = clk_register_divider_table(NULL, clk_name,
> +			parent_name, 0,
> +			reg, shift, mask,
> +			clk_divider_flags, table,
> +			NULL);

this causes trouble, as the divider clock code above still requires a width 
instead of a mask. I remember talk about this going to change separately, but 
couldn't find anything of the sort in linux-next.



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

Re: [PATCH RFC 0/3] clk: dt: bindings for mux & divider clocks

[Heiko Stübner]

Am Montag, 3. Juni 2013, 19:53:07 schrieb Mike Turquette:
> This series introduces binding definitions for common register-mapped
> clock multiplexor and divider IP blocks, and the corresponding setup
> functions once they are matched.  The bindings are close the struct
> definitions but please don't hold that against the binding: the struct
> definitions closely model the hardware.
> 
> The only missing basic clock type is the gate clock.  A binding for that
> was posted some time back and is similar in spirit to these[1].  I guess
> we'll need to decide whether register-level programming details belong
> in DT.  I believe they do since those details describe the hardware.
> 
> Note that there is still no generic clock driver that matches these
> basic types, but it would be trivial to write one.  Thoughts on that?
> Is it better for each of the basic clock types to be a driver that
> matches, or should there be one drivers/clk/clk-basic.c which matches
> all of the basic clock building blocks?  I like the latter for aesthetic
> purposes.
> 
> I am using this code while converting the OMAP4 clock data over to DT
> and some common boilerplate code can be factored out of several clock
> drivers if this is merged.

apart from the stuff pointed out in the replies to the patches this works 
really well on my upcoming Rockchip platform and saves quite a lot silly clock 
definitions whose only purpose is to hold the shift and width values.


So, for this series:

Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH RFC 2/3] clk: dt: binding for basic multiplexor clock

[Mike Turquette]

On Mon, Jun 3, 2013 at 2:39 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Montag, 3. Juni 2013, 22:15:45 schrieb Heiko Stübner:
>> Am Montag, 3. Juni 2013, 22:07:22 schrieb Mike Turquette:
>> > Quoting Heiko Stübner (2013-06-03 12:33:19)
>> >
>> > > Hi Mike,
>> > >
>> > > I think it's a multiplexEr clock in the patch title, and see below
>> >
>> > Doh, you are right.  But "xor" is so much cooler looking than "xer"...
>> >
>> > > Am Montag, 3. Juni 2013, 19:53:09 schrieb Mike Turquette:
>> > > > Device Tree binding for the basic clock multiplexor, plus the setup
>> > > > function to register the clock.  Based on the existing fixed-clock
>> > > > binding.
>> > > >
>> > > > Also relocate declaration of of_fixed_factor_clk_setup to keep things
>> > > > tidy.
>> > > >
>> > > > Signed-off-by: Mike Turquette <mturquette@linaro.org>
>> > >
>> > > [...]
>> > >
>> > > > +
>> > > > +     reg = of_iomap(node, 0);
>> > > > +     pr_err("%s: reg is 0x%p\n", __func__, reg);
>> > > > +
>> > > > +     if (of_property_read_u32(node, "mask", &mask)) {
>> > > > +             pr_err("%s: missing mask property for %s\n", __func__,
>> > > > node->name); +             return;
>> > > > +     }
>> > > > +
>> > > > +     if (of_property_read_u32(node, "shift", &shift))
>> > > > +             pr_debug("%s: missing shift property defaults to zero
>> > > > for %s\n", +                             __func__, node->name);
>> > > > +
>> > > > +     if (of_property_read_bool(node, "index_one"))
>> > > > +             clk_mux_flags |= CLK_MUX_INDEX_ONE;
>> > > > +
>> > > > +     clk = clk_register_mux_table(NULL, clk_name, parent_names,
>> > > > num_parents, +                     0, reg, 0, mask, clk_mux_flags,
>> > > >
>> > >                       ^- should probably be shift
>> > >
>> > > Otherwise looks cool and I'm currently trying it with my Rockchip code.
>> >
>> > Right again.  My test platform seems to not shift the mask at all so
>> > this did not cause a visible bug for me.
>>
>> I'm currently converting my rockchip stuff to it, as it would solve a lot
>> of problems with the numerous clock types I'm defining just to put the
>> shift and mask values somewhere.
>>
>> So lets see if anything more turns up ;-)
>
> yep ... of_mux_clk_setup is quite talkative ... there are quite some (debug-?)
> pr_errs in it ;-)
>

Yes I realized that after sending.  Hopefully not too many curse words
in the trace messages.  Will remove for v2.

Regards,
Mike

>
> Heiko

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Stephen Boyd]

On 06/03/13 10:53, Mike Turquette wrote:
> +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
> +- 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
> +- shift : number of bits to shift the mask, defaults to 0 if not present
> +- index_one : valid divisor programming starts at 1, not zero
> +- index_power_of_two : valid divisor programming must be a power of two
> +- allow_zero : implies index_one, and programming zero results in
> +  divide-by-one

It's preferred that property names have dashes instead of underscores.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Matt Sealey]

On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/03/13 10:53, Mike Turquette wrote:
>> +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
>> +- 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
>> +- shift : number of bits to shift the mask, defaults to 0 if not present
>> +- index_one : valid divisor programming starts at 1, not zero
>> +- index_power_of_two : valid divisor programming must be a power of two
>> +- allow_zero : implies index_one, and programming zero results in
>> +  divide-by-one
>
> It's preferred that property names have dashes instead of underscores.

I think I have a suggestion or two here..

I mailed one to Mike earlier, I have had some mailing list problems
which I think are solved now..

If you provide a mask for programming the divider, surely the "shift"
property is therefore redundant. Unless some device has a completely
strange way of doing things and uses two seperated bitfields for
programming in the same register, the shift value is simply a function
of ffs() on the mask, isn't it? It is nice to put the information
specifically in the device tree, but where a shift is not specified
it'd probably be a good idea to go about deriving that value that way
anyway, right, and if we're doing that.. why specify it?

Also it may be much simpler to simply define the default clock setup
as being an integer divider that follows the form 2^n - I am not sure
I can think off the top of my head of any clock dividers doing the
rounds that have divide-by-non-power-of-two and if they exist out
there (please correct me) they would be the rare ones. I think
properties that modify behavior should err on the side of being rarely
used and to define unusual behavior, not confirm the status quo.

Just to be sure, though, someone would need to go visit a bunch of
current clock handlers already implemented or read a lot of docs to
see how they're expecting their dividers. Maybe some of the Samsung,
Qualcomm, TI, Freescale guys can comment on what is the most common
case inside their SoCs.

In any case, once you figure out that, rather than specifying a full
table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just
give the lower and upper limits and let the driver figure out what is
in between with two values. If there were gaps, a full table would be
necessary. The allow zero property might be better served by the very
same range property.

I would also say.. bit-mask rather than mask, divider-table rather
than table, divider-range (modifying the idea above). While we're
inside a particular namespace in the binding I think naming properties
with their intent (i.e. what table is it?) is good behavior.

--
Matt Sealey <matt@genesi-usa.com>

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Mike Turquette]

Quoting Matt Sealey (2013-06-04 10:39:53)
> On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/03/13 10:53, Mike Turquette wrote:
> >> +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
> >> +- 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
> >> +- shift : number of bits to shift the mask, defaults to 0 if not present
> >> +- index_one : valid divisor programming starts at 1, not zero
> >> +- index_power_of_two : valid divisor programming must be a power of two
> >> +- allow_zero : implies index_one, and programming zero results in
> >> +  divide-by-one
> >
> > It's preferred that property names have dashes instead of underscores.
> 
> I think I have a suggestion or two here..
> 
> I mailed one to Mike earlier, I have had some mailing list problems
> which I think are solved now..
> 
> If you provide a mask for programming the divider, surely the "shift"
> property is therefore redundant. Unless some device has a completely
> strange way of doing things and uses two seperated bitfields for
> programming in the same register, the shift value is simply a function
> of ffs() on the mask, isn't it? It is nice to put the information
> specifically in the device tree, but where a shift is not specified
> it'd probably be a good idea to go about deriving that value that way
> anyway, right, and if we're doing that.. why specify it?
> 

Shift is optional.  Also I think the integer values in a DTS are
32-bits, so if some day someone had a clock driver that needed to write
to a 64-bit register then shift might be useful there, combined with a
32-bit mask.  Or perhaps dtc will have a 64-bit value for that case?  I
don't know.

I certainly do see your point about only using the mask, and the
original clock basic types did this a while back before the shift+width
style came into vogue.

If other reviewers also want to use a pure 32-bit mask and no shift then
I'll drop it in the next round.

> Also it may be much simpler to simply define the default clock setup
> as being an integer divider that follows the form 2^n - I am not sure
> I can think off the top of my head of any clock dividers doing the
> rounds that have divide-by-non-power-of-two and if they exist out
> there (please correct me) they would be the rare ones. I think
> properties that modify behavior should err on the side of being rarely
> used and to define unusual behavior, not confirm the status quo.
> 

Please review the current use of CLK_DIVIDER_POWER_OF_TWO in the kernel.
The only platform using this flag for power-of-two dividers is OMAP2430.
Based on usage of clk_register_divider I think the common case is a
clock divider that divides by any integer value programmed into it, up
to the limit of the bitfield width.

Perhaps your platform is the corner case, in which case power-of-two is
the property you should use.

> Just to be sure, though, someone would need to go visit a bunch of
> current clock handlers already implemented or read a lot of docs to
> see how they're expecting their dividers. Maybe some of the Samsung,
> Qualcomm, TI, Freescale guys can comment on what is the most common
> case inside their SoCs.
> 

The following platforms use the basic divider without the power-of-two
flag:

imx6, loongson1B, mmp2, pxa168, pxa190, spearX, sunxi, tegra2/3/114,
omap2+.

> In any case, once you figure out that, rather than specifying a full
> table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just
> give the lower and upper limits and let the driver figure out what is
> in between with two values. If there were gaps, a full table would be
> necessary. The allow zero property might be better served by the very
> same range property.
> 

You have described how it already works.  The table is only there for
when a common formula fails to capture the possible divisor
combinations.  For the default the driver starts the index at 0 and goes
up to mask/width, for index_one the driver does the same but starts at
1, not 0, for power of two ... well you get the picture.  The table is
only of use when these common divider schemes are insufficient to
determine the divisors.

> I would also say.. bit-mask rather than mask, divider-table rather
> than table, divider-range (modifying the idea above). While we're
> inside a particular namespace in the binding I think naming properties
> with their intent (i.e. what table is it?) is good behavior.
> 

I'll make the properties more verbose in the next patch.

Thanks for the review,
Mike

> --
> Matt Sealey <matt@genesi-usa.com>

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Matt Sealey]

On Tue, Jun 4, 2013 at 2:22 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Matt Sealey (2013-06-04 10:39:53)
>> On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/03/13 10:53, Mike Turquette wrote:
>> >> +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
>> >> +- 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
>> >> +- shift : number of bits to shift the mask, defaults to 0 if not present
>> >> +- index_one : valid divisor programming starts at 1, not zero
>> >> +- index_power_of_two : valid divisor programming must be a power of two
>> >> +- allow_zero : implies index_one, and programming zero results in
>> >> +  divide-by-one
>> >
>> > It's preferred that property names have dashes instead of underscores.
>>
>> I think I have a suggestion or two here..
>>
>> I mailed one to Mike earlier, I have had some mailing list problems
>> which I think are solved now..
>>
>> If you provide a mask for programming the divider, surely the "shift"
>> property is therefore redundant. Unless some device has a completely
>> strange way of doing things and uses two seperated bitfields for
>> programming in the same register, the shift value is simply a function
>> of ffs() on the mask, isn't it? It is nice to put the information
>> specifically in the device tree, but where a shift is not specified
>> it'd probably be a good idea to go about deriving that value that way
>> anyway, right, and if we're doing that.. why specify it?
>
> Shift is optional.  Also I think the integer values in a DTS are
> 32-bits, so if some day someone had a clock driver that needed to write
> to a 64-bit register then shift might be useful there, combined with a
> 32-bit mask.  Or perhaps dtc will have a 64-bit value for that case?  I
> don't know.
>
> I certainly do see your point about only using the mask, and the
> original clock basic types did this a while back before the shift+width
> style came into vogue.
>
> If other reviewers also want to use a pure 32-bit mask and no shift then
> I'll drop it in the next round.

Hmm.. that's a very good point...

You can't have a 64-bit OF property unless you're on a 64-bit platform
or do some really weird workarounds for it (multiple cells etc.).
That's how the OF spec was laid out, it sucks.. but it's the standard
and the DT seems to stay compatible with the general concept. A
supplemental binding to support clock dividers living in 64-bit
registers may be a better way.

You'd need a shift for the mask if you could only do a 32-bit property
(32-bit platform with a 4-byte cell size) and a shift for the bits
that would go into the mask as well as a width property for the
register size to bound the shift..  Too complicated for now. I would
say stick with 32-bit until this magical 64-bit divider register
appears, and then do something slightly different :)

>> Also it may be much simpler to simply define the default clock setup
>> as being an integer divider that follows the form 2^n - I am not sure
>> I can think off the top of my head of any clock dividers doing the
>> rounds that have divide-by-non-power-of-two and if they exist out
>> there (please correct me) they would be the rare ones. I think
>> properties that modify behavior should err on the side of being rarely
>> used and to define unusual behavior, not confirm the status quo.
>
> Please review the current use of CLK_DIVIDER_POWER_OF_TWO in the kernel.

You're right, I think I got it backwards or was actually thinking of
allow_zero (as in, 0x0 divider = divide by 1)? i.MX definitely has
more 0x0=div-by-1 than any other kind..

>> In any case, once you figure out that, rather than specifying a full
>> table of <0 1>, <1 2>, <2, 4> and creating a big list, you could just
>> give the lower and upper limits and let the driver figure out what is
>> in between with two values. If there were gaps, a full table would be
>> necessary. The allow zero property might be better served by the very
>> same range property.
>
> You have described how it already works.  The table is only there for
> when a common formula fails to capture the possible divisor
> combinations.  For the default the driver starts the index at 0 and goes
> up to mask/width, for index_one the driver does the same but starts at
> 1, not 0, for power of two ... well you get the picture.  The table is
> only of use when these common divider schemes are insufficient to
> determine the divisors.

Right, but in the case of index_one and allow_zero, one property for
ranges basically covers the case where a slight modification to the
formula used would be required, and allows specifying the first valid
divider and the last valid divider which might come in handy when you
have a mask of a certain size (say, 6 bits), but the actual divider is
only 4 bits wide. You may need to ensure that you clear reserved bits
within that masked region, but not use the mask itself to calculate
the full range.

I can't think of a good case but on i.MX53 the DVFSP load tracking
register 1 has a field that subdivides the ARM core clock by 1, 2 or 3
(using 0, 1 or 2 as the divider value) in a 6-bit field, where
000011-111111 are reserved. I would think the correct value for the
mask is the full 6-bit field, but the range should define that only
1-3 are valid (saving one entry). It isn't really something that would
end up using a clock framework divider though.

If there's a divider built for future expandability or somehow
restricted at the design level to only accept a few bits, or only
'restricted' at the documentation level (.. it's been done) then it
might be best to use the mask to mask off the register, and not to
define valid bits to set, unless you resolve to specify two masks..
but a range property would be better and cover every case, while not
having to force DT writers to list every divider in a big field
(imagine if it's 3 bits that's a 16 entry table.. there are some 5 and
6-bit clock dividers floating around), I think an opportunity to
consolidate two properties into one AND give an easy out to
oddly-ranged dividers (where low and high divider settings may be
'invalid' but between those values is a contiguous range of a
significant number of values (and here I would say, more than 2).

Since we shouldn't be guessing, divider-p-o-2 is still required but as
you corrected me.. rarer.

>> I would also say.. bit-mask rather than mask, divider-table rather
>> than table, divider-range (modifying the idea above). While we're
>> inside a particular namespace in the binding I think naming properties
>> with their intent (i.e. what table is it?) is good behavior.
>>
>
> I'll make the properties more verbose in the next patch.

Lovely :)

-- 
Matt Sealey <matt@genesi-usa.com>

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Heiko Stübner]

Am Dienstag, 4. Juni 2013, 21:22:43 schrieb Mike Turquette:
> Quoting Matt Sealey (2013-06-04 10:39:53)
> 
> > On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <sboyd@codeaurora.org> 
wrote:
> > > On 06/03/13 10:53, Mike Turquette wrote:
> > >> +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
> > >> +- 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
> > >> +- shift : number of bits to shift the mask, defaults to 0 if not
> > >> present +- index_one : valid divisor programming starts at 1, not
> > >> zero +- index_power_of_two : valid divisor programming must be a
> > >> power of two +- allow_zero : implies index_one, and programming zero
> > >> results in +  divide-by-one
> > > 
> > > It's preferred that property names have dashes instead of underscores.
> > 
> > I think I have a suggestion or two here..
> > 
> > I mailed one to Mike earlier, I have had some mailing list problems
> > which I think are solved now..
> > 
> > If you provide a mask for programming the divider, surely the "shift"
> > property is therefore redundant. Unless some device has a completely
> > strange way of doing things and uses two seperated bitfields for
> > programming in the same register, the shift value is simply a function
> > of ffs() on the mask, isn't it? It is nice to put the information
> > specifically in the device tree, but where a shift is not specified
> > it'd probably be a good idea to go about deriving that value that way
> > anyway, right, and if we're doing that.. why specify it?
> 
> Shift is optional.  Also I think the integer values in a DTS are
> 32-bits, so if some day someone had a clock driver that needed to write
> to a 64-bit register then shift might be useful there, combined with a
> 32-bit mask.  Or perhaps dtc will have a 64-bit value for that case?  I
> don't know.
> 
> I certainly do see your point about only using the mask, and the
> original clock basic types did this a while back before the shift+width
> style came into vogue.
> 
> If other reviewers also want to use a pure 32-bit mask and no shift then
> I'll drop it in the next round.

I would object to dropping the shift :-)

If you look at the two clk extensions in my rockchip patches you can see that 
they use a different regiment to set values.

Using the lower 16 bits to set the value and the upper 16 bit to mark which 
values are changed, i.e. (mask << shift + 16) | (val << shift).

(I'm not sure, if the naming or solution could be improved though)


Heiko

Re: [PATCH RFC 0/3] clk: dt: bindings for mux & divider clocks

[Shawn Guo]

Mike,

On Mon, Jun 03, 2013 at 10:53:07AM -0700, Mike Turquette wrote:
> I am using this code while converting the OMAP4 clock data over to DT

I see these basic clk bindings can be useful for platforms that have
a relatively simple clock tree, but I'm a little surprised that you plan
to move OMAP4 to it.  I'm wondering how many nodes you will have to add
to OMAP4 device tree.  For imx6q example, it means ~200 nodes addition
to device tree, which is obviously a bloating of device tree.

Shawn

> and some common boilerplate code can be factored out of several clock
> drivers if this is merged.

Re: [PATCH RFC 0/3] clk: dt: bindings for mux & divider clocks

[Shawn Guo]

On Fri, Jun 07, 2013 at 10:52:54AM -0700, Mike Turquette wrote:
> Yes, there was a time when I was firmly against doing such a thing.
> However I'm not sure it is so bad now.  More and more SoCs are going to
> keep getting merged into the kernel and that just means more and more
> clock data.  Perhaps DT is best suited to handle this bloat.  I broke
> the clock data out into a separate file so that it didn't overwhelm the
> existing omap4.dtsi.

I'm actually more concerned by run-time impact.  Any of_find_node_*()
call will possibly have to scan all those hundreds of nodes to find the
desired one.  Anyway, it's an OMAP folks decision, and the impact might
not be that much on those fast CPUs.

Shawn

> 
> Either way I marked this series as RFC precisely due to your point.  I
> want feedback on how the OMAP folks feel about this.  So far no has has
> NACKed it ;-)

Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

[Mike Turquette]

Quoting Heiko Stübner (2013-06-03 15:18:32)
> Am Montag, 3. Juni 2013, 19:53:10 schrieb Mike Turquette:
> > Devicetree binding for the basic clock divider, plus the setup function
> > to register the clock.  Based on the existing fixed-clock binding.
> > 
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > ---
> 
> [...]
> 
> > +/**
> > + * of_div_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;
> > +     u8 mask = 0;
> > +     u8 shift = 0;
> 
> in the mux-clock these 3 are unsigned long and u32 types ... what is correct?
> 

Good catch.  Shifts should be u8, masks should be u32.  I've left the
flags as u8.  The binding doesn't dictate this and if we end up having
more flags (hopefully not!) then this value can be embiggened.

> 
> > +     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 (of_property_read_u8(node, "mask", &mask)) {
> > +             pr_err("%s: missing mask property for %s\n", __func__, node->name);
> > +             return;
> > +     }
> > +
> > +     if (of_property_read_u8(node, "shift", &shift))
> > +             pr_debug("%s: missing shift property defaults to zero for %s\n",
> > +                             __func__, node->name);
> 
> same here ... mux reads u32
> 
> > +     if (of_property_read_bool(node, "index_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;
> > +
> > +     table = of_clk_get_div_table(node);
> > +     if (IS_ERR(table))
> > +             return;
> > +
> > +     clk = clk_register_divider_table(NULL, clk_name,
> > +                     parent_name, 0,
> > +                     reg, shift, mask,
> > +                     clk_divider_flags, table,
> > +                     NULL);
> 
> this causes trouble, as the divider clock code above still requires a width 
> instead of a mask. I remember talk about this going to change separately, but 
> couldn't find anything of the sort in linux-next.

Right.  I viewed creation of the DT bindings a bit separately from the
CCF code, which I think is the right approach.  A mask is definitely a
more useful structure than a width value, and the DT bindings need to be
at least a little future-proof, so I chose a mask.

I'll update the divider code to use a mask and post that as part of the
v2 series.

Regards,
Mike

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