Highbank clock support using DT

From: Rob Herring <[email protected]>

__clk_reparent can remove orphans, so the list iterator needs to be the safe from removal version.

Signed-off-by Rob Herring <[email protected]>

---
?drivers/clk/clk.c | ? ?4 ++--
?1 files changed, 2 insertions(+), 2 deletions(-)

drivers/clk/clk.c +1 -1

index c7c3bc5..802eda4 100644
@@ -1172,7 +1172,7 @@ void __clk_init(struct device *dev, struct clk *clk)
 ?{
 ? ? ? ?int i;
 ? ? ? ?struct clk *orphan;
- ? ? ? struct hlist_node *tmp;
+ ? ? ? struct hlist_node *tmp, *tmp2;
 
 ? ? ? ?if (!clk)
 ? ? ? ? ? ? ? ?return;

@@ -1246,7 +1246,7 @@ void __clk_init(struct device *dev, struct clk *clk)

? ? ? ? * walk the list of orphan clocks and reparent any that are children of ? ? ? ? * this clock
? ? ? ? */
- ? ? ? hlist_for_each_entry(orphan, tmp, &clk_orphan_list, child_node)
+ ? ? ? hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) ? ? ? ? ? ? ? ?__clk_reparent(orphan, __clk_init_parent(orphan));

? ? ? ?/*
--
1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Documentation/devicetree/bindings/clock/clock-bindings.txt +0 -0

From: Grant Likely <[email protected]>

Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an of_clk_get function to allow platforms to retrieve clock data from the device tree.

Platform register a provider through of_clk_add_provider, which will be called when a device references the provider's OF node for a clock reference.

v3: - Clarified documentation

v2: - fixed errant ';' causing compile error
- Editorial fixes from Shawn Guo
- merged in adding lookup to clkdev
- changed property names to match established convention. After working with the binding a bit it really made more sense to follow the lead of 'reg', 'gpios' and 'interrupts' by making the input simply 'clocks' & 'clock-names' instead of 'clock-input-*', and to only use clock-output* for the producer nodes. (Sorry Shawn, this will mean you need to change some code, but it should be trivial)
- Add ability to inherit clocks from parent nodes by using an empty 'clock-ranges' property. Useful for busses. I could use some feedback on the new property name, 'clock-ranges' doesn't feel right to me.

Signed-off-by Grant Likely <[email protected]>

Reviewed-by Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Mike Turquette <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   |  116 ++++++++++++++
 .../devicetree/bindings/clock/fixed-clock.txt      |   21 +++
 drivers/clk/clkdev.c                               |    9 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/clock.c                                 |  165 ++++++++++++++++++++
 include/linux/of_clk.h                             |   37 +++++
 7 files changed, 355 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
 create mode 100644 Documentation/devicetree/bindings/clock/fixed-clock.txt
 create mode 100644 drivers/of/clock.c
 create mode 100644 include/linux/of_clk.h

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +9 -0

new file mode 100644
index 0000000..3cb6746
@@ -0,0 +1,116 @@
+This binding is a work-in-progress, and are based on some experimental
+work by benh[1].
+
+Sources of clock signal can be represented by any node in the device
+tree.  Those nodes are designated as clock providers.  Clock consumer
+nodes use a phandle and clock specifier pair to connect clock provider
+outputs to clock inputs.  Similar to the gpio specifiers, a clock
+specifier is an array of one more more cells identifying the clock
+output on a device.  The length of a clock specifier is defined by the

From: Grant Likely <[email protected]>

Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an of_clk_get function to allow platforms to retrieve clock data from the device tree.

Platform register a provider through of_clk_add_provider, which will be called when a device references the provider's OF node for a clock reference.

v3: - Clarified documentation

v2: - fixed errant ';' causing compile error
- Editorial fixes from Shawn Guo
- merged in adding lookup to clkdev
- changed property names to match established convention. After working with the binding a bit it really made more sense to follow the lead of 'reg', 'gpios' and 'interrupts' by making the input simply 'clocks' & 'clock-names' instead of 'clock-input-*', and to only use clock-output* for the producer nodes. (Sorry Shawn, this will mean you need to change some code, but it should be trivial)
- Add ability to inherit clocks from parent nodes by using an empty 'clock-ranges' property. Useful for busses. I could use some feedback on the new property name, 'clock-ranges' doesn't feel right to me.

Signed-off-by Grant Likely <[email protected]>

Reviewed-by Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Mike Turquette <[email protected]>

---
 .../devicetree/bindings/clock/clock-bindings.txt   |  116 ++++++++++++++
 .../devicetree/bindings/clock/fixed-clock.txt      |   21 +++
 drivers/clk/clkdev.c                               |    9 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/clock.c                                 |  165 ++++++++++++++++++++

 include/linux/of_clk.h                             |   37 +++++
 7 files changed, 355 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
 create mode 100644 Documentation/devicetree/bindings/clock/fixed-clock.txt
 create mode 100644 drivers/of/clock.c
 create mode 100644 include/linux/of_clk.h

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +116 -0

new file mode 100644
index 0000000..3cb6746
@@ -0,0 +1,116 @@
+This binding is a work-in-progress, and are based on some experimental
+work by benh[1].
+
+Sources of clock signal can be represented by any node in the device
+tree.  Those nodes are designated as clock providers.  Clock consumer
+nodes use a phandle and clock specifier pair to connect clock provider
+outputs to clock inputs.  Similar to the gpio specifiers, a clock
+specifier is an array of one more more cells identifying the clock
+output on a device.  The length of a clock specifier is defined by the
+value of a #clock-cells property in the clock provider node.
+
+[1] http://patchwork.ozlabs.org/patch/31551/
+
+==Clock providers==
+
+Required properties:
+#clock-cells:	   Number of cells in a clock specifier; typically will be
+		   set to 1
+
+Optional properties:
+clock-output-names: Recommended to be a list of strings of clock output signal
+		    names indexed by the first cell in the clock specifier.
+		    However, the meaning of clock-output-name is domain
+		    specific to the clock provider, and is only provided to
+		    encourage using the same meaning for the majority of clock
+		    providers.  This format may not work for clock providers
+		    using a complex clock specifier format.  In those cases it
+		    is recommended to omit this property and create a binding
+		    specific names property.
+
+		    Clock consumer nodes must never directly reference
+		    the provider's clock-output-name property.
+
+For example:
+
+    oscillator {
+        #clock-cells = <1>;
+        clock-output-names = "ckil", "ckih";
+    };
+
+- this node defines a device with two clock outputs, the first named
+  "ckil" and the second named "ckih".  Consumer nodes always reference
+  clocks by index. The names should reflect the clock output signal
+  names for the device.
+
+==Clock consumers==
+
+Required properties:
+clocks:		List of phandle and clock specifier pairs, one pair
+		for each clock input to the device.  Note: if the
+		clock provider specifies '0' for #clock-cells, then
+		only the phandle portion of the pair will appear.
+
+Optional properties:
+clock-names:	List of clock input name strings sorted in the same
+		order as the clocks property.  Consumers drivers
+		will use clock-names to match clock input names
+		with clocks specifiers.
+clock-ranges:	Empty property indicating that child nodes can inherit named
+		clocks from this node. Useful for bus nodes to provide a
+		clock to their children.
+
+For example:
+
+    device {
+        clocks = <&osc 1>, <&ref 0>;
+        clock-names = "baud", "register";
+    };
+
+
+This represents a device with two clock inputs, named "baud" and "register".
+The baud clock is connected to output 1 of the &osc device, and the register
+clock is connected to output 0 of the &ref.
+
+==Example==
+
+    /* external oscillator */
+    osc: oscillator {
+        compatible = "fixed-clock";
+        #clock-cells = <1>;
+        clock-frequency  = <32678>;
+        clock-output-names = "osc";
+    };
+
+    /* phase-locked-loop device, generates a higher frequency clock
+     * from the external oscillator reference */
+    pll: pll at 4c000 {
+        compatible = "vendor,some-pll-interface"
+        #clock-cells = <1>;
+        clocks = <&osc 0>;
+        clock-names = "ref";
+        reg = <0x4c000 0x1000>;
+        clock-output-names = "pll", "pll-switched";
+    };
+
+    /* UART, using the low frequency oscillator for the baud clock,
+     * and the high frequency switched PLL output for register
+     * clocking */
+    uart at a000 {
+        compatible = "fsl,imx-uart";
+        reg = <0xa000 0x1000>;
+        interrupts = <33>;
+        clocks = <&osc 0>, <&pll 1>;
+        clock-names = "baud", "register";
+    };
+
+This DT fragment defines three devices: an external oscillator to provide a
+low-frequency reference clock, a PLL device to generate a higher frequency
+clock signal, and a UART.
+
+* The oscillator is fixed-frequency, and provides one clock output, named "osc".
+* The PLL is both a clock provider and a clock consumer. It uses the clock
+  signal generated by the external oscillator, and provides two output signals
+  ("pll" and "pll-switched").
+* The UART has its baud clock connected the external oscillator and its
+  register clock connected to the PLL clock (the "pll-switched" signal)

Documentation/devicetree/bindings/clock/fixed-clock.txt new file +21 -0

new file mode 100644
index 0000000..9a75342
@@ -0,0 +1,21 @@
+Binding for simple fixed-rate clock sources.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/fixed-clock.txt
+
+Required properties:
+- compatible : shall be "fixed-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clock-frequency : frequency of clock in Hz. May be multiple cells.
+
+Optional properties:
+- gpios : From common gpio binding; gpio connection to clock enable pin.
+- clock-output-names : From common clock binding
+
+Example:
+	clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <1000000000>;
+	};

drivers/clk/clkdev.c +9 -0

index 6db161f..43628d0 100644
@@ -19,6 +19,8 @@
 #include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);

@@ -78,6 +80,13 @@ EXPORT_SYMBOL(clk_get_sys);
 struct clk *clk_get(struct device *dev, const char *con_id)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
+	struct clk *clk;
+
+	if (dev) {
+		clk = of_clk_get_by_name(dev->of_node, con_id);
+		if (clk && __clk_get(clk))
+			return clk;
+	}
 
 	return clk_get_sys(dev_id, con_id);
 }

drivers/of/Kconfig +6 -0

index 268163d..b49ab9c 100644
@@ -47,6 +47,12 @@ config OF_IRQ
 	def_bool y
 	depends on !SPARC
 
+config OF_CLOCK
+	def_bool y
+	depends on HAVE_CLK
+	help
+	  OpenFirmware clock accessors
+
 config OF_DEVICE
 	def_bool y
 

drivers/of/Makefile +1 -0

index a73f5a5..e7ad1e9 100644
@@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS)  += address.o
 obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
+obj-$(CONFIG_OF_CLOCK)	+= clock.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
 obj-$(CONFIG_OF_SPI)	+= of_spi.o

drivers/of/clock.c new file +165 -0

new file mode 100644
index 0000000..6519e96
@@ -0,0 +1,165 @@
+/*
+ * Clock infrastructure for device tree platforms
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+
+/**
+ * struct of_clk_provider - Clock provider registration structure
+ * @link: Entry in global list of clock providers
+ * @node: Pointer to device tree node of clock provider
+ * @get: Get clock callback.  Returns NULL or a struct clk for the
+ *       given clock specifier
+ * @data: context pointer to be passed into @get callback
+ */
+struct of_clk_provider {
+	struct list_head link;
+
+	struct device_node *node;
+	struct clk *(*get)(struct of_phandle_args *clkspec, void *data);
+	void *data;
+};
+
+static LIST_HEAD(of_clk_providers);
+static DEFINE_MUTEX(of_clk_lock);
+
+/**
+ * of_clk_add_provider() - Register a clock provider for a node
+ * @np: Device node pointer associated with clock provider
+ * @clk_src_get: callback for decoding clock
+ * @data: context pointer for @clk_src_get callback.
+ */
+int of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+						   void *data),
+			void *data)
+{
+	struct of_clk_provider *cp;
+
+	cp = kzalloc(sizeof(struct of_clk_provider), GFP_KERNEL);
+	if (!cp)
+		return -ENOMEM;
+
+	cp->node = of_node_get(np);
+	cp->data = data;
+	cp->get = clk_src_get;
+
+	mutex_lock(&of_clk_lock);
+	list_add(&cp->link, &of_clk_providers);
+	mutex_unlock(&of_clk_lock);
+	pr_debug("Added clock from %s\n", np->full_name);
+
+	return 0;
+}
+
+/**
+ * of_clk_del_provider() - Remove a previously registered clock provider
+ * @np: Device node pointer associated with clock provider
+ */
+void of_clk_del_provider(struct device_node *np)
+{
+	struct of_clk_provider *cp;
+
+	mutex_lock(&of_clk_lock);
+	list_for_each_entry(cp, &of_clk_providers, link) {
+		if (cp->node == np) {
+			list_del(&cp->link);
+			of_node_put(cp->node);
+			kfree(cp);
+			break;
+		}
+	}
+	mutex_unlock(&of_clk_lock);
+}
+
+static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
+{
+	struct of_clk_provider *provider;
+	struct clk *clk = NULL;
+
+	/* Check if we have such a provider in our array */
+	mutex_lock(&of_clk_lock);
+	list_for_each_entry(provider, &of_clk_providers, link) {
+		if (provider->node == clkspec->np)
+			clk = provider->get(clkspec, provider->data);
+		if (clk)
+			break;
+	}
+	mutex_unlock(&of_clk_lock);
+
+	return clk;
+}
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+	struct of_phandle_args clkspec;
+	struct clk *clk;
+	int rc;
+
+	if (index < 0)
+		return NULL;
+
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+					&clkspec);
+	if (rc)
+		return NULL;
+
+	clk = __of_clk_get_from_provider(&clkspec);
+	of_node_put(clkspec.np);
+	return clk;
+}
+
+/**
+ * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ */
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	struct clk *clk = NULL;
+
+	/* Walk up the tree of devices looking for a clock that matches */
+	while (np) {
+		int index = 0;
+
+		/*
+		 * For named clocks, first look up the name in the
+		 * "clock-names" property.  If it cannot be found, then
+		 * index will be an error code, and of_clk_get() will fail.
+		 */
+		if (name)
+			index = of_property_match_string(np, "clock-names", name);
+		clk = of_clk_get(np, index);
+		if (clk)
+			break;
+		else if (name && index >= 0) {
+			pr_err("ERROR: could not get clock %s:%s(%i)\n",
+				 np->full_name, name ? name : "", index);
+			return NULL;
+		}
+
+		/*
+		 * No matching clock found on this node.  If the parent node
+		 * has a "clock-ranges" property, then we can try one of its
+		 * clocks.
+		 */
+		np = np->parent;
+		if (np && !of_get_property(np, "clock-ranges", NULL))
+			break;
+	}
+
+	return clk;
+}

include/linux/of_clk.h new file +37 -0

new file mode 100644
index 0000000..dcbd27b
@@ -0,0 +1,37 @@
+/*
+ * Clock infrastructure for device tree platforms
+ */
+#ifndef __OF_CLK_H
+#define __OF_CLK_H
+
+struct device;
+struct clk;
+
+#ifdef CONFIG_OF_CLOCK
+
+struct device_node;
+
+int of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(struct of_phandle_args *args,
+						   void *data),
+			void *data);
+
+void of_clk_del_provider(struct device_node *np);
+
+struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
+
+#else
+static struct clk *of_clk_get(struct device_node *np, int index)
+{
+	return NULL;
+}
+
+static struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __OF_CLK_H */
+

Signature · 7 lines

1.7.5.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 25 lines · Rob Herring

From: Grant Likely <[email protected]>

Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an of_clk_get function to allow platforms to retrieve clock data from the device tree.

Platform register a provider through of_clk_add_provider, which will be called when a device references the provider's OF node for a clock reference.

v3: - Clarified documentation

v2: - fixed errant ';' causing compile error
- Editorial fixes from Shawn Guo
- merged in adding lookup to clkdev
- changed property names to match established convention. After working with the binding a bit it really made more sense to follow the lead of 'reg', 'gpios' and 'interrupts' by making the input simply 'clocks' & 'clock-names' instead of 'clock-input-*', and to only use clock-output* for the producer nodes. (Sorry Shawn, this will mean you need to change some code, but it should be trivial)
- Add ability to inherit clocks from parent nodes by using an empty 'clock-ranges' property. Useful for busses. I could use some feedback on the new property name, 'clock-ranges' doesn't feel right to me.

Quoted 5 lines

Signed-off-by Grant Likely <[email protected]>

Reviewed-by Shawn Guo <[email protected]>

Cc Rob Herring <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Mike Turquette <[email protected]>

Hi Rob,

Thanks for respinning this patch. Since you're actually using it, do you want to take over getting it into mainline?

Quoted 7 lines

---
 .../devicetree/bindings/clock/clock-bindings.txt   |  116 ++++++++++++++
 .../devicetree/bindings/clock/fixed-clock.txt      |   21 +++
 drivers/clk/clkdev.c                               |    9 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/clock.c                                 |  165 ++++++++++++++++++++

I would actually like to see this file moved into drivers/clk. I don't think there is any need anymore to collect OF support code into drivers/of. I plan to move the spi and gpio support code into drivers/spi and drivers/gpio respectively.

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +6 -0

+
+Optional properties:
+clock-names:	List of clock input name strings sorted in the same
+		order as the clocks property.  Consumers drivers
+		will use clock-names to match clock input names
+		with clocks specifiers.

On Tue, Mar 13, 2012 at 06:22:22PM -0500, Rob Herring wrote:
...

Documentation/devicetree/bindings/clock/clock-bindings.txt line 52

Quoted 7 lines

+==Clock consumers==

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +6 -0

+
+Required properties:
+clocks:		List of phandle and clock specifier pairs, one pair
+		for each clock input to the device.  Note: if the
+		clock provider specifies '0' for #clock-cells, then
+		only the phandle portion of the pair will appear.

Per discussion[1], the parent of the clock could be reasonably represented in C code instead of in device tree. Then how can this "clocks" property be "Required"? Or to put it another way, if this is "Required", the whole clock tree will have to be represented in device tree, no?

Quoted 6 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +6 -0

+
+Optional properties:
+clock-names:	List of clock input name strings sorted in the same
+		order as the clocks property.  Consumers drivers
+		will use clock-names to match clock input names
+		with clocks specifiers.

Documentation/devicetree/bindings/clock/clock-bindings.txt line 68

Quoted 10 lines

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +10 -0

+clock-ranges:	Empty property indicating that child nodes can inherit named
+		clocks from this node. Useful for bus nodes to provide a
+		clock to their children.
+
+For example:
+
+    device {
+        clocks = <&osc 1>, <&ref 0>;
+        clock-names = "baud", "register";
+    };

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/139414/focus=1216423

Quoted 16 lines · Shawn Guo

On Tue, Mar 13, 2012 at 06:22:22PM -0500, Rob Herring wrote:
...

Documentation/devicetree/bindings/clock/clock-bindings.txt line 52

Quoted 7 lines

+==Clock consumers==

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +6 -0

+
+Required properties:
+clocks:		List of phandle and clock specifier pairs, one pair
+		for each clock input to the device.  Note: if the
+		clock provider specifies '0' for #clock-cells, then
+		only the phandle portion of the pair will appear.

Per discussion[1], the parent of the clock could be reasonably represented in C code instead of in device tree. Then how can this "clocks" property be "Required"? Or to put it another way, if this is "Required", the whole clock tree will have to be represented in device tree, no?

I'm not sure I follow. All of this only applies if you are doing clock setup from DT. Whether you represent all clocks or only the outputs to devices in DT is up to you. So you could have a CCM node for imx with 50 clock outputs and all the intermediate clocks within the CCM are represented with C code.

In other words, at minimum you are just replacing the clkdev lookup with a DT lookup.

Quoted 18 lines · Rob Herring

Quoted 16 lines · Shawn Guo

On Tue, Mar 13, 2012 at 06:22:22PM -0500, Rob Herring wrote:
...

Documentation/devicetree/bindings/clock/clock-bindings.txt line 52

Quoted 7 lines

+==Clock consumers==

Documentation/devicetree/bindings/clock/clock-bindings.txt new file +6 -0

+
+Required properties:
+clocks:		List of phandle and clock specifier pairs, one pair
+		for each clock input to the device.  Note: if the
+		clock provider specifies '0' for #clock-cells, then
+		only the phandle portion of the pair will appear.

Per discussion[1], the parent of the clock could be reasonably represented in C code instead of in device tree. Then how can this "clocks" property be "Required"? Or to put it another way, if this is "Required", the whole clock tree will have to be represented in device tree, no?

Quoted 6 lines

I'm not sure I follow. All of this only applies if you are doing clock setup from DT. Whether you represent all clocks or only the outputs to devices in DT is up to you. So you could have a CCM node for imx with 50 clock outputs and all the intermediate clocks within the CCM are represented with C code.

Ok, now I have uart clock being one of the 50 clocks represented in device tree, while the parent of uart clock ipg_per is represented in C code. That said, I do not have a ipg_per clock node in device tree. My question is how I should give that required "clocks" property for the uart clock node.

	uart_serial_clk: uart-baud {
		compatible = "fsl,imx6q-clock";
		#clock-cells = <0>;
		/* clocks = <&ipg_per_clk>; */
		clocks = <???>;

	};

	uart: serial at 02020000 {
		compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
		reg = <0x02020000 0x4000>;
		interrupts = <0 26 0x04>;
		clocks = <&uart_serial_clk>
	}

Regards,
Shawn

Quoted 3 lines

In other words, at minimum you are just replacing the clkdev lookup with a DT lookup.

On 04/09/2012 09:13 AM, Shawn Guo wrote:

Quoted 6 lines

	uart_serial_clk: uart-baud {
		compatible = "fsl,imx6q-clock";
		#clock-cells = <0>;
		/* clocks = <&ipg_per_clk>; */
		clocks = <???>;

This is a provider node not a consumer, so you don't have a clocks property.

You could, but then you need the parent clock defined in the DT.

The compatible property here looks too generic. In the CCM code, how do you match it to the uart baud clock? You shouldn't be using the name.

You need to first decide if you are going to define each clock in the DT

or just the whole CCM as a node.
You can't really do something in the
middle or be evolving it over time. You could perhaps make the PLLs be separate nodes and inputs to the CCM. As they really are separate h/w blocks.

From: Grant Likely <[email protected]>

Add a helper function for finding the index of a string in a string list property. This helper is useful for bindings that use a separate *-name property for attaching names to tuples in another property such as 'reg' or 'gpios'.

Signed-off-by Grant Likely <[email protected]>

drivers/of/clock.c +8 -0

+/**
+ * of_fixed_clk_setup() - Setup function for simple fixed rate clock
+ */
+void __init of_fixed_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	u32 rate;

On Tue, Mar 13, 2012 at 06:22:24PM -0500, Rob Herring wrote:
...

Quoted 8 lines

drivers/of/clock.c +8 -0

+/**
+ * of_fixed_clk_setup() - Setup function for simple fixed rate clock
+ */
+void __init of_fixed_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	u32 rate;

drivers/of/clock.c struct clk *of_clk_get_by_name(struct device_node *np, const char *name) · line 208

Quoted 10 lines

drivers/of/clock.c +10 -0

+
+	if (of_property_read_u32(node, "clock-frequency", &rate))
+		return;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate);
+	if (clk)
+		of_clk_add_provider(node, of_fixed_clk_get, clk);
+}

It seems my comment[1] did not get addressed in this post.

Regards,
Shawn

[1] http://article.gmane.org/gmane.linux.drivers.devicetree/10589

It also only works for root fixed clocks, but the common clock allows for non-root fixed clocks. I don't think there's much use case for that at least as a DT clock node.

On Tue, Mar 13, 2012 at 06:22:24PM -0500, Rob Herring wrote:
...

Quoted 8 lines

drivers/of/clock.c +8 -0

+/**
+ * of_fixed_clk_setup() - Setup function for simple fixed rate clock
+ */
+void __init of_fixed_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	const char *clk_name = node->name;
+	u32 rate;

drivers/of/clock.c struct clk *of_clk_get_by_name(struct device_node *np, const char *name) · line 208

Quoted 10 lines

drivers/of/clock.c +10 -0

+
+	if (of_property_read_u32(node, "clock-frequency", &rate))
+		return;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate);
+	if (clk)
+		of_clk_add_provider(node, of_fixed_clk_get, clk);
+}

It seems my comment[1] did not get addressed in this post.

Regards,
Shawn

[1] http://article.gmane.org/gmane.linux.drivers.devicetree/10589

So I started implementing support for multiple outputs, but ran into a complication. If you have multiple clocks on a node, then you have to have a clk_src_get function to translate the clock cell value to a struct clk.

This would also require allocating an array of struct clk's to do the lookup.

This is all doable, but I don't see the benefit over
having a single node per fixed clock. We're not likely to have *lots* of fixed clocks. I think we should leave fixed-clock defined as a single output.

If you really see the benefit, you can add a new binding "multiple-fixed-clocks" or something platform specific.

On Sun, Apr 08, 2012 at 09:48:27AM -0500, Rob Herring wrote:
...

Quoted 4 lines

So I started implementing support for multiple outputs, but ran into a complication. If you have multiple clocks on a node, then you have to have a clk_src_get function to translate the clock cell value to a struct clk.

So this is how clk_src_get looks like.

struct clk *clk_src_get(struct of_phandle_args *a, void *data)

{
        struct clk **clks = data;
        return clks[a->args[0]];
}

Quoted 2 lines

This would also require allocating an array of struct clk's to do the lookup.

And this how allocating looks like.

        clks = kzalloc(sizeof(*clks)  * num, GFP_KERNEL);
        if (!clks)
                return -ENOMEM;

Seriously, not a big complication, right?

Quoted 5 lines

This is all doable, but I don't see the benefit over
having a single node per fixed clock. We're not likely to have *lots* of fixed clocks. I think we should leave fixed-clock defined as a single output.

The problem is there is nothing specific to fixed-clock. If you have some reason to not support blob node for fixed-clock, the reason will apply on clk_gate, clk_divider and clk_mux too. Then, which clocks will support the #clock-cells in the binding document?

So to me, you need to either have it implemented or remove it from the binding document completely.

Regards,
Shawn

Quoted 3 lines

If you really see the benefit, you can add a new binding "multiple-fixed-clocks" or something platform specific.

You still haven't given any benefits of supporting multiple clocks. It's slightly fewer dts lines, but not really anything else.

I don't think people are going to define clocks generically in DT at the mux, clk gate and divider level anyway. If you only have a few clocks then you may, and 1 clock output per node is probably okay. If you have hundreds of clocks then you probably won't, and will have a SOC specific binding anyway.

On Mon, Apr 09, 2012 at 09:18:27AM -0500, Rob Herring wrote:
...

Quoted 3 lines

You still haven't given any benefits of supporting multiple clocks. It's slightly fewer dts lines, but not really anything else.

What's more important than fewer dts lines is fewer nodes. Isn't it the whole point of #clock-cells? In the real imx example I gave, with #clock-cells support, I can have only one node to represent 3 fixed clocks.

...

Quoted 6 lines

I don't think people are going to define clocks generically in DT at the mux, clk gate and divider level anyway. If you only have a few clocks then you may, and 1 clock output per node is probably okay. If you have hundreds of clocks then you probably won't, and will have a SOC specific binding anyway.

Why?

Take a look at clk-imx6q.c[1], you will find except pll and pfd, all the imx6q clocks are represented as gate, divider and mux, and we should not need a SoC specific binding for them.

So you want to have generic bindings for every clock which implies a bunch of nodes if not a node per clock anyway, but then object to 3 nodes for fixed clocks.

I'm talking about the bindings and you are pointing me to an implementation with no bindings. It's 2 different things. The implementation can fully be the generic clk implementations, but the clock bindings can still be either a node per clock or a monolithic clock module node. There is no fixed rule here and there should not be. We can each chose what works best for us.

I see a couple of examples that your clocks are still SoC specific despite your claims.

Your clock gates may reuse clk_gate struct, but they still have custom ops to handle the 2-bit field. Obviously, you decided not to merge 2-bit field support into the existing clk gate code (the correct decision IMHO). I made a similar decision.

For practically every clock, you need to set the spinlock to imx_ccm_lock. How are you going to know which clocks to set this lock for with a "generic" binding? You have to distinguish those from IPU clocks for example.

From: Rob Herring <[email protected]>

For simple cases, the clock provider data can simply be the struct clk ptr.

Signed-off-by Rob Herring <[email protected]>

---
 drivers/of/clock.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

drivers/of/clock.c +6 -0

index 06fd5ad..f4a0965 100644
@@ -33,6 +33,12 @@ struct of_clk_provider {
 static LIST_HEAD(of_clk_providers);
 static DEFINE_MUTEX(of_clk_lock);
 
+static struct clk *of_clk_simple_get(struct of_phandle_args *clkspec,
+				     void *data)
+{
+	return data;
+}
+
 /**
  * of_clk_add_provider() - Register a clock provider for a node
  * @np: Device node pointer associated with clock provider

drivers/of/clock.c +1 -6

 	mutex_lock(&of_clk_lock);
 	list_add(&cp->link, &of_clk_providers);

@@ -183,11 +192,6 @@ void __init of_clk_init(const struct of_device_id *matches)
 	}
 }
 
-static struct clk *of_fixed_clk_get(struct of_phandle_args *a, void *data)
-{
-	return data;
-}
-
 /**
  * of_fixed_clk_setup() - Setup function for simple fixed rate clock
  */

@@ -204,5 +208,5 @@ void __init of_fixed_clk_setup(struct device_node *node)
 
 	clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate);
 	if (clk)
-		of_clk_add_provider(node, of_fixed_clk_get, clk);
+		of_clk_add_provider(node, NULL, clk);
 }

Signature · 1 line

1.7.5.4

From: Rob Herring <[email protected]>

This adds real clock support to Calxeda Highbank SOC using the common clock infrastructure.

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm/Kconfig                  |    1 +
 arch/arm/boot/dts/highbank.dts    |   76 +++++++++
 arch/arm/mach-highbank/Makefile   |    2 +-
 arch/arm/mach-highbank/clock.c    |   62 -------
 arch/arm/mach-highbank/highbank.c |   16 ++
 drivers/clk/Makefile              |    2 +
 drivers/clk/clk-highbank.c        |  335 +++++++++++++++++++++++++++++++++++++
 7 files changed, 431 insertions(+), 63 deletions(-)
 delete mode 100644 arch/arm/mach-highbank/clock.c
 create mode 100644 drivers/clk/clk-highbank.c

arch/arm/Kconfig +1 -0

index a48aecc..2019548 100644
@@ -346,6 +346,7 @@ config ARCH_HIGHBANK
 	select ARM_TIMER_SP804
 	select CACHE_L2X0
 	select CLKDEV_LOOKUP
+	select COMMON_CLK
 	select CPU_V7
 	select GENERIC_CLOCKEVENTS
 	select HAVE_ARM_SCU

arch/arm/boot/dts/highbank.dts +18 -0

index 305635b..06194d5 100644
@@ -24,6 +24,7 @@
 	compatible = "calxeda,highbank";
 	#address-cells = <1>;
 	#size-cells = <1>;
+	clock-ranges;
 
 	cpus {
 		#address-cells = <1>;

@@ -75,12 +76,14 @@
 			compatible = "arm,smp-twd";
 			reg = <0xfff10600 0x20>;
 			interrupts = <1 13 0xf04>;
+			clocks = <&a9periphclk>;
 		};
 
 		watchdog at fff10620 {
 			compatible = "arm,cortex-a9-wdt";
 			reg = <0xfff10620 0x20>;
 			interrupts = <1 14 0xf04>;
+			clocks = <&a9periphclk>;
 		};
 
 		intc: interrupt-controller at fff11000 {

@@ -117,12 +120,14 @@
 			compatible = "calxeda,hb-sdhci";
 			reg = <0xffe0e000 0x1000>;
 			interrupts = <0 90 4>;
+			clocks = <&eclk>;
 		};
 
 		ipc at fff20000 {
 			compatible = "arm,pl320", "arm,primecell";
 			reg = <0xfff20000 0x1000>;
 			interrupts = <0 7 4>;
+			clocks = <&pclk>;
 		};
 
 		gpioe: gpio at fff30000 {

@@ -131,6 +136,7 @@
 			gpio-controller;
 			reg = <0xfff30000 0x1000>;
 			interrupts = <0 14 4>;
+			clocks = <&pclk>;
 		};
 
 		gpiof: gpio at fff31000 {

@@ -139,6 +145,7 @@
 			gpio-controller;
 			reg = <0xfff31000 0x1000>;
 			interrupts = <0 15 4>;
+			clocks = <&pclk>;
 		};
 
 		gpiog: gpio at fff32000 {

@@ -147,6 +154,7 @@
 			gpio-controller;
 			reg = <0xfff32000 0x1000>;
 			interrupts = <0 16 4>;
+			clocks = <&pclk>;
 		};
 
 		gpioh: gpio at fff33000 {

@@ -155,24 +163,30 @@
 			gpio-controller;
 			reg = <0xfff33000 0x1000>;
 			interrupts = <0 17 4>;
+			clocks = <&pclk>;
 		};
 
 		timer {
 			compatible = "arm,sp804", "arm,primecell";
 			reg = <0xfff34000 0x1000>;
 			interrupts = <0 18 4>;
+			clocks = <&pclk>, <&pclk>;
+			clock-names = "apb_pclk", "bus";
 		};
 
 		rtc at fff35000 {
 			compatible = "arm,pl031", "arm,primecell";
 			reg = <0xfff35000 0x1000>;
 			interrupts = <0 19 4>;
+			clocks = <&pclk>;
 		};
 
 		serial at fff36000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0xfff36000 0x1000>;
 			interrupts = <0 20 4>;
+			clocks = <&pclk>, <&pclk>;
+			clock-names = "apb_pclk", "bus";
 		};
 
 		smic at fff3a000 {

@@ -187,12 +201,74 @@
 		sregs at fff3c000 {
 			compatible = "calxeda,hb-sregs";
 			reg = <0xfff3c000 0x1000>;
+
+			clocks {
+				#address-cells = <1>;
+				#size-cells = <0>;

arch/arm/boot/dts/highbank.dts +7 -0

+					clocks = <&osc>;
+					reg = <0x108>;
+				};
+
+				a9pll: a9pll {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-pll-clock";

arch/arm/boot/dts/highbank.dts +31 -0

+				};
+
+				a9bclk: a9bclk {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-a9bus-clock";
+					clocks = <&a9pll>;
+					reg = <0x104>;
+					clock-divider = <4>;
+				};
+
+				emmcpll: emmcpll {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-pll-clock";
+					clocks = <&osc>;
+					reg = <0x10C>;
+				};
+
+				eclk: eclk {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-emmc-clock";
+					clocks = <&emmcpll>;
+					reg = <0x114>;
+				};
+
+				pclk: pclk {
+					#clock-cells = <0>;
+					compatible = "fixed-clock";
+					clock-frequency = <150000000>;
+				};
+			};
 		};
 
 		dma at fff3d000 {
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0xfff3d000 0x1000>;
 			interrupts = <0 92 4>;
+			clocks = <&pclk>;
 		};
 
 		ethernet at fff50000 {

arch/arm/mach-highbank/Makefile +1 -1

index 986958a..8bf3868 100644
@@ -1,4 +1,4 @@
-obj-y					:= clock.o highbank.o system.o
+obj-y					:= highbank.o system.o
 obj-$(CONFIG_DEBUG_HIGHBANK_UART)	+= lluart.o
 obj-$(CONFIG_SMP)			+= platsmp.o
 obj-$(CONFIG_LOCAL_TIMERS)		+= localtimer.o

arch/arm/mach-highbank/clock.c deleted +0 -62

deleted file mode 100644
index c25a2ae..0000000
@@ -1,62 +0,0 @@
-/*
- * Copyright 2011 Calxeda, Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/errno.h>
-#include <linux/clk.h>
-#include <linux/clkdev.h>
-
-struct clk {
-	unsigned long rate;
-};
-
-int clk_enable(struct clk *clk)
-{
-	return 0;
-}
-
-void clk_disable(struct clk *clk)
-{}
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	return clk->rate;
-}
-
-long clk_round_rate(struct clk *clk, unsigned long rate)
-{
-	return clk->rate;
-}
-
-int clk_set_rate(struct clk *clk, unsigned long rate)
-{
-	return 0;
-}
-
-static struct clk eclk = { .rate = 200000000 };
-static struct clk pclk = { .rate = 150000000 };
-
-static struct clk_lookup lookups[] = {
-	{ .clk = &pclk, .con_id = "apb_pclk", },
-	{ .clk = &pclk, .dev_id = "sp804", },
-	{ .clk = &eclk, .dev_id = "ffe0e000.sdhci", },
-	{ .clk = &pclk, .dev_id = "fff36000.serial", },
-};
-
-void __init highbank_clocks_init(void)
-{
-	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
-}

arch/arm/mach-highbank/highbank.c +16 -0

index 8394d51..5c0cdaa 100644
@@ -23,6 +23,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
 #include <linux/smp.h>
+#include <linux/of_clk.h>
 
 #include <asm/cacheflush.h>
 #include <asm/smp_plat.h>

@@ -91,6 +92,16 @@ static void __init highbank_init_irq(void)
 	l2x0_of_init(0, ~0UL);
 }
 
+static struct clk_lookup lookups[] = {
+	{
+		.dev_id = "sp804",
+		.con_id = NULL,
+	}, {
+		.dev_id = "smp_twd",
+		.con_id = NULL,
+	}
+};
+
 static void __init highbank_timer_init(void)
 {
 	int irq;

@@ -108,6 +119,11 @@ static void __init highbank_timer_init(void)
 	irq = irq_of_parse_and_map(np, 0);
 
 	highbank_clocks_init();
+	lookups[0].clk = of_clk_get(np, 0);
+
+	np = of_find_compatible_node(NULL, NULL, "arm,smp-twd");
+	lookups[1].clk = of_clk_get(np, 0);
+	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
 
 	sp804_clocksource_init(timer_base + 0x20, "timer1");
 	sp804_clockevents_init(timer_base, irq, "timer0");

drivers/clk/Makefile +2 -0

index 1f736bc..70c713f 100644
@@ -2,3 +2,5 @@
 obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
 obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-fixed-rate.o clk-gate.o \
 				   clk-mux.o clk-divider.o
+
+obj-$(CONFIG_ARCH_HIGHBANK)	+= clk-highbank.o

drivers/clk/clk-highbank.c new file +202 -0

new file mode 100644
index 0000000..722fc67
@@ -0,0 +1,335 @@
+/*
+ * Copyright 2011-2012 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/errno.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+
+extern void __iomem *sregs_base;
+
+#define HB_PLL_LOCK_500		0x20000000
+#define HB_PLL_LOCK		0x10000000
+#define HB_PLL_DIVF_SHIFT	20
+#define HB_PLL_DIVF_MASK	0x0ff00000
+#define HB_PLL_DIVQ_SHIFT	16
+#define HB_PLL_DIVQ_MASK	0x00070000
+#define HB_PLL_DIVR_SHIFT	8
+#define HB_PLL_DIVR_MASK	0x00001f00
+#define HB_PLL_RANGE_SHIFT	4
+#define HB_PLL_RANGE_MASK	0x00000070
+#define HB_PLL_BYPASS		0x00000008
+#define HB_PLL_RESET		0x00000004
+#define HB_PLL_EXT_BYPASS	0x00000002
+#define HB_PLL_EXT_ENA		0x00000001
+
+#define HB_PLL_VCO_MIN_FREQ	2133000000
+#define HB_PLL_MAX_FREQ		HB_PLL_VCO_MIN_FREQ
+#define HB_PLL_MIN_FREQ		(HB_PLL_VCO_MIN_FREQ / 64)
+
+#define HB_A9_BCLK_DIV_MASK	0x00000006
+#define HB_A9_BCLK_DIV_SHIFT	1
+
+struct hb_clk {
+        struct clk_hw	hw;
+	void __iomem	*reg;
+	char *parent_name;
+};
+#define to_hb_clk(p) container_of(p, struct hb_clk, hw)
+
+static int clk_pll_prepare(struct clk_hw *clk)
+	{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 reg;
+
+	reg = readl(hbclk->reg);
+	reg &= ~HB_PLL_RESET;
+	writel(reg, hbclk->reg);
+
+	while ((readl(hbclk->reg) & HB_PLL_LOCK) == 0)
+		;
+	while ((readl(hbclk->reg) & HB_PLL_LOCK_500) == 0)
+		;
+
+	return 0;
+}
+
+static void clk_pll_unprepare(struct clk_hw *clk)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 reg;
+
+	reg = readl(hbclk->reg);
+	reg |= HB_PLL_RESET;
+	writel(reg, hbclk->reg);
+}
+
+static int clk_pll_enable(struct clk_hw *clk)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 reg;
+
+	reg = readl(hbclk->reg);
+	reg |= HB_PLL_EXT_ENA;
+	writel(reg, hbclk->reg);
+
+	return 0;
+}
+
+static void clk_pll_disable(struct clk_hw *clk)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 reg;
+
+	reg = readl(hbclk->reg);
+	reg &= ~HB_PLL_EXT_ENA;
+	writel(reg, hbclk->reg);
+}
+
+static unsigned long clk_pll_recalc_rate(struct clk_hw *clk,
+					 unsigned long parent_rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	unsigned long divf, divq, vco_freq, reg;
+
+	reg = readl(hbclk->reg);
+	if (reg & HB_PLL_EXT_BYPASS)
+		return parent_rate;
+
+	divf = (reg & HB_PLL_DIVF_MASK) >> HB_PLL_DIVF_SHIFT;
+	divq = (reg & HB_PLL_DIVQ_MASK) >> HB_PLL_DIVQ_SHIFT;
+	vco_freq = parent_rate * (divf + 1);
+
+	return vco_freq / (1 << divq);
+}
+
+static void clk_pll_calc(unsigned long rate, unsigned long ref_freq,
+			u32 *pdivq, u32 *pdivf)
+{
+	u32 divq, divf;
+	unsigned long vco_freq;
+
+	if (rate < HB_PLL_MIN_FREQ)
+		rate = HB_PLL_MIN_FREQ;
+	if (rate > HB_PLL_MAX_FREQ)
+		rate = HB_PLL_MAX_FREQ;
+
+	for (divq = 1; divq <= 6; divq++) {
+		if ((rate * (1 << divq)) >= HB_PLL_VCO_MIN_FREQ)
+			break;
+	}
+
+	vco_freq = rate * (1 << divq);
+	divf = (vco_freq + (ref_freq / 2)) / ref_freq;
+	divf--;
+
+	*pdivq = divq;
+	*pdivf = divf;
+}
+
+static long clk_pll_round_rate(struct clk_hw *clk, unsigned long rate,
+			       unsigned long *unused)
+{
+	u32 divq, divf;
+	unsigned long ref_freq = clk_get_rate(clk_get_parent(clk->clk));
+
+	clk_pll_calc(rate, ref_freq, &divq, &divf);
+
+	return (ref_freq * (divf + 1)) / (1 << divq);
+}
+
+static int clk_pll_set_rate(struct clk_hw *clk, unsigned long rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	unsigned long parent_rate;
+	u32 divq, divf;
+	u32 reg;
+
+	parent_rate = clk_get_rate(clk_get_parent(clk->clk));
+	clk_pll_calc(rate, parent_rate, &divq, &divf);
+
+	reg = readl(hbclk->reg);
+	if (divf != ((reg & HB_PLL_DIVF_MASK) >> HB_PLL_DIVF_SHIFT)) {
+		/* Need to re-lock PLL, so put it into bypass mode */
+		reg |= HB_PLL_EXT_BYPASS;
+		writel(reg | HB_PLL_EXT_BYPASS, hbclk->reg);
+
+		reg &= ~(HB_PLL_DIVF_MASK | HB_PLL_DIVQ_MASK);
+		reg |= (divf << HB_PLL_DIVF_SHIFT) | (divq << HB_PLL_DIVQ_SHIFT);
+		writel(reg | HB_PLL_RESET, hbclk->reg);
+		writel(reg, hbclk->reg);
+
+		while ((readl(hbclk->reg) & HB_PLL_LOCK) == 0)
+			;
+		while ((readl(hbclk->reg) & HB_PLL_LOCK_500) == 0)
+			;
+		reg |= HB_PLL_EXT_ENA;
+		reg &= ~HB_PLL_EXT_BYPASS;
+	} else {
+		reg &= ~HB_PLL_DIVQ_MASK;
+		reg |= divq << HB_PLL_DIVQ_SHIFT;
+	}
+	writel(reg, hbclk->reg);
+
+	return 0;
+}
+
+static const struct clk_ops clk_pll_ops = {
+	.prepare = clk_pll_prepare,
+	.unprepare = clk_pll_unprepare,
+	.enable = clk_pll_enable,
+	.disable = clk_pll_disable,
+	.recalc_rate = clk_pll_recalc_rate,
+	.round_rate = clk_pll_round_rate,
+	.set_rate = clk_pll_set_rate,
+};
+

drivers/clk/clk-highbank.c new file +95 -0

+};
+
+static unsigned long clk_periclk_recalc_rate(struct clk_hw *clk,
+					     unsigned long parent_rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 div;
+
+	div = readl(hbclk->reg);
+	div++;
+	div *= 2;
+
+	return parent_rate / div;
+}
+
+static long clk_periclk_round_rate(struct clk_hw *clk, unsigned long rate,
+				   unsigned long *unused)
+{
+	unsigned long parent_rate = clk_get_rate(clk_get_parent(clk->clk));
+	u32 div;
+
+	div = parent_rate / rate;
+	div++;
+	div &= ~0x1;
+
+	return parent_rate / div;
+}
+
+static int clk_periclk_set_rate(struct clk_hw *clk, unsigned long rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 div;
+
+	div = clk_get_rate(clk_get_parent(clk->clk)) / rate;
+	if (div & 0x1)
+		return -EINVAL;
+
+	writel(div >> 1, hbclk->reg);
+	return 0;
+}
+
+static const struct clk_ops periclk_ops = {
+	.recalc_rate = clk_periclk_recalc_rate,
+	.round_rate = clk_periclk_round_rate,
+	.set_rate = clk_periclk_set_rate,
+};
+
+static void __init hb_clk_init(struct device_node *node, const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct hb_clk *hb_clk;
+	const char *clk_name = node->name;
+	int rc;
+
+	rc = of_property_read_u32(node, "reg", &reg);
+	if (WARN_ON(rc))
+		return;
+
+	hb_clk = kzalloc(sizeof(*hb_clk), GFP_KERNEL);
+	if (WARN_ON(!hb_clk))
+		return;
+
+	hb_clk->reg = sregs_base + reg;
+
+	of_property_read_string(node, "clock-outputs", &clk_name);
+
+	hb_clk->parent_name = of_clk_get_parent_name(node, 0);
+
+	printk("registering clk %s, parent = %s\n", clk_name, hb_clk->parent_name);
+	clk = clk_register(NULL, clk_name, ops, &hb_clk->hw,
+			   &hb_clk->parent_name, 1, 0);
+	if (WARN_ON(!clk)) {
+		kfree(hb_clk);
+		return;
+	}
+	rc = of_clk_add_provider(node, NULL, clk);
+}
+
+static void __init hb_pll_init(struct device_node *node)
+{
+	hb_clk_init(node, &clk_pll_ops);
+}
+
+static void __init hb_a9periph_init(struct device_node *node)
+{
+	hb_clk_init(node, &a9periphclk_ops);
+}
+
+static void __init hb_a9bus_init(struct device_node *node)
+{
+	hb_clk_init(node, &a9bclk_ops);
+}
+
+static void __init hb_emmc_init(struct device_node *node)

drivers/clk/clk-highbank.c new file +7 -0

+	{}
+};
+
+void __init highbank_clocks_init(void)
+{
+	of_clk_init(clk_match);
+}

Quoted 7 lines · Rob Herring

From: Rob Herring <[email protected]>

This adds real clock support to Calxeda Highbank SOC using the common clock infrastructure.

Signed-off-by Rob Herring <[email protected]>

---

Quoted 4 lines

arch/arm/boot/dts/highbank.dts +4 -0

+
+				osc: oscillator {
+					#clock-cells = <0>;
+					compatible = "fixed-clock";

arch/arm/boot/dts/highbank.dts line 217

Quoted 6 lines

arch/arm/boot/dts/highbank.dts +6 -0

+					clock-frequency = <33333000>;
+				};
+
+				ddrpll: ddrpll {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-pll-clock";

Where are all these "calxeda,*-clock' compatible documented?

Quoted 12 lines

arch/arm/boot/dts/highbank.dts +11 -0

+					clocks = <&osc>;
+					reg = <0x108>;
+				};
+
+				a9pll: a9pll {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-pll-clock";
+					clocks = <&osc>;
+					reg = <0x100>;
+				};
+

arch/arm/boot/dts/highbank.dts line 234

Quoted 6 lines

arch/arm/boot/dts/highbank.dts +6 -0

+				a9periphclk: a9periphclk {
+					#clock-cells = <0>;
+					compatible = "calxeda,hb-a9periph-clock";
+					clocks = <&a9pll>;
+					reg = <0x104>;
+					clock-divider = <4>;

Where is this "clock-divider" binding documented?

Quoted 4 lines

drivers/clk/clk-highbank.c new file +4 -0

+static unsigned long clk_cpu_periphclk_recalc_rate(struct clk_hw *clk,
+						   unsigned long parent_rate)
+{
+	return parent_rate / 2;

drivers/clk/clk-highbank.c line 212

Quoted 6 lines

drivers/clk/clk-highbank.c new file +6 -0

+}
+
+static const struct clk_ops a9periphclk_ops = {
+	.recalc_rate = clk_cpu_periphclk_recalc_rate,
+};
+

This basically is a clk-fixed-factor added by Sascha.

Quoted 6 lines

drivers/clk/clk-highbank.c new file +6 -0

+static unsigned long clk_cpu_a9bclk_recalc_rate(struct clk_hw *clk,
+						unsigned long parent_rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 div = (readl(hbclk->reg) & HB_A9_BCLK_DIV_MASK) >> HB_A9_BCLK_DIV_SHIFT;
+

drivers/clk/clk-highbank.c line 223

Quoted 5 lines

drivers/clk/clk-highbank.c new file +5 -0

+	return parent_rate / (div + 2);
+}
+
+static const struct clk_ops a9bclk_ops = {
+	.recalc_rate = clk_cpu_a9bclk_recalc_rate,

Since there is a divider for the clock, the ops should have .round_rate and .set_rate, no?

Quoted 14 lines

drivers/clk/clk-highbank.c new file +14 -0

+};
+
+static unsigned long clk_periclk_recalc_rate(struct clk_hw *clk,
+					     unsigned long parent_rate)
+{
+	struct hb_clk *hbclk = to_hb_clk(clk);
+	u32 div;
+
+	div = readl(hbclk->reg);
+	div++;
+	div *= 2;
+
+	return parent_rate / div;
+}

drivers/clk/clk-highbank.c line 328

Quoted 7 lines

drivers/clk/clk-highbank.c new file +7 -0

+
+static const __initconst struct of_device_id clk_match[] = {
+	{ .compatible = "fixed-clock", .data = of_fixed_clk_setup, },
+	{ .compatible = "calxeda,hb-pll-clock", .data = hb_pll_init, },
+	{ .compatible = "calxeda,hb-a9periph-clock", .data = hb_a9periph_init, },
+	{ .compatible = "calxeda,hb-a9bus-clock", .data = hb_a9bus_init, },
+	{ .compatible = "calxeda,hb-emmc-clock", .data = hb_emmc_init, },

So that's the difference between your clock and mine. You do not reuse any basic clk, except clk_fixed_rate, while my clocks are all about basic clk except pll. That's why you need a long list of SoC specific binding, while I do not. All I need is the binding for those basic clks.

Regards,
Shawn

Quoted 7 lines

drivers/clk/clk-highbank.c new file +7 -0

+	{}
+};
+
+void __init highbank_clocks_init(void)
+{
+	of_clk_init(clk_match);
+}