CLK: OMAP conversion to DT

clk_get_sys / clk_get can now find clocks from device-tree. If a DT clock is found, an entry is added to the clk_lookup list also for subsequent searches.

Signed-off-by Tero Kristo <[email protected]>

Cc Russell King <[email protected]>

---
 drivers/clk/clkdev.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

drivers/clk/clkdev.c +15 -0

index 442a313..cbeb252 100644
@@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node
 *np, const char *name) EXPORT_SYMBOL(of_clk_get_by_name);
 #endif
 
+/**
+ * clkdev_add_nolock - add lookup entry for a clock
+ * @cl: pointer to new clock lookup entry
+ *
+ * Non-locking version, used internally by clk_find() to add DT based
+ * clock lookup entries.
+ */
+static void clkdev_add_nolock(struct clk_lookup *cl)
+{
+	list_add_tail(&cl->node, &clocks);
+}
+
 /*
  * Find the correct struct clk for the device and connection ID.
  * We do slightly fuzzy matching here:

@@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char
 *dev_id, const char *con_id) {
 	struct clk_lookup *p, *cl = NULL;
 	int match, best_found = 0, best_possible = 0;
+	struct device_node *node;
+	struct clk *clk;
+	struct of_phandle_args clkspec;
 
 	if (dev_id)
 		best_possible += 2;

drivers/clk/clkdev.c +11 -0

+	clkspec.np = node;
+
+	clk = of_clk_get_from_provider(&clkspec);
+
+	if (!IS_ERR(clk)) {
+		/* We found a clock, add node to clkdev */
+		cl = clkdev_alloc(clk, con_id, dev_id);
+		if (cl)
+			clkdev_add_nolock(cl);
+	}
+
 	return cl;
 }

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world?

We already have a good way of binding things together in DT, which is using phandles.

Not even saying that this (or something this patch relies on) breaks the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node.

Quoted 9 lines · Tomasz Figa

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world?

Quoted 8 lines

We already have a good way of binding things together in DT, which is using phandles.

Not even saying that this (or something this patch relies on) breaks the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node.

Tell me this: many devices name their clock inputs. You can find these input names in data sheets and the like. These are the names defined by the hardware. What is wrong about using those names in DT?

Remember that the second argument to clk_get() is the _clock_ _input_ _name_ on the _device_ passed in the first argument. It is not *ever* some random name of the clock producer unless someone's fscked up their use of this API (in which case they're the ones with the problem.)

Hi Russell,

Quoted 19 lines · On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:

Quoted 17 lines · Tomasz Figa

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world?

We already have a good way of binding things together in DT, which is using phandles.

Not even saying that this (or something this patch relies on) breaks the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node.

Quoted 3 lines

Tell me this: many devices name their clock inputs. You can find these input names in data sheets and the like. These are the names defined by the hardware. What is wrong about using those names in DT?

Yes, clock inputs. But this is about lookup by clock name, not clock input name, as far as I can tell from the patch, based on looking for a node with given name over the whole DT.

Quoted 4 lines

Remember that the second argument to clk_get() is the _clock_ _input_ _name_ on the _device_ passed in the first argument. It is not *ever* some random name of the clock producer unless someone's fscked up their use of this API (in which case they're the ones with the problem.)

This is perfectly fine and this is how the generic common clock framework DT bindings work - clock-names property is a list of clock input names and clocks property contain specifiers of respective clocks.

Quoted 30 lines · Tomasz Figa

Hi Russell,

Quoted 26 lines · On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:

Quoted 13 lines · Tomasz Figa

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world?

We already have a good way of binding things together in DT, which is
using phandles.

Quoted 6 lines

Not even saying that this (or something this patch relies on) breaks
the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node.

Tell me this: many devices name their clock inputs. You can find these
input names in data sheets and the like. These are the names defined
by the hardware. What is wrong about using those names in DT?

Quoted 3 lines

Yes, clock inputs. But this is about lookup by clock name, not clock input name, as far as I can tell from the patch, based on looking for a node with given name over the whole DT.

"con_id" is supposed to be the clock input name when the clock API is used correctly.

Quoted 8 lines

Quoted 7 lines

Remember that the second argument to clk_get() is the _clock_ _input_
_name_ on the _device_ passed in the first argument. It is not *ever*
some random name of the clock producer unless someone's fscked up their
use of this API (in which case they're the ones with the problem.)

Quoted 4 lines

This is perfectly fine and this is how the generic common clock framework DT bindings work - clock-names property is a list of clock input names and clocks property contain specifiers of respective clocks.

There seems to be a some pressure to do clk_get()->of_clk_get() conversions, and also to find ways to lookup clocks.

It seems to me
that no one understands how DT handles clocks. Maybe that's where the problem is - lack of documentation about how DT clocks are handled.

Quoted 31 lines · On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote:

Quoted 30 lines · Tomasz Figa

Hi Russell,

Quoted 26 lines · On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:

Quoted 13 lines · Tomasz Figa

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world?

We already have a good way of binding things together in DT, which is
using phandles.

Quoted 6 lines

Not even saying that this (or something this patch relies on) breaks
the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node.

Tell me this: many devices name their clock inputs. You can find these
input names in data sheets and the like. These are the names defined
by the hardware. What is wrong about using those names in DT?

Quoted 6 lines

Quoted 3 lines

Yes, clock inputs. But this is about lookup by clock name, not clock input name, as far as I can tell from the patch, based on looking for a node with given name over the whole DT.

"con_id" is supposed to be the clock input name when the clock API is used correctly.

Right. Still, the code is looking for a node with the same name as the supposed input name, which seems strange to me, because clock input names are supposed to be defined in consumer's node, inside clock-names property.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Remember that the second argument to clk_get() is the _clock_ _input_
_name_ on the _device_ passed in the first argument. It is not *ever*
some random name of the clock producer unless someone's fscked up their
use of this API (in which case they're the ones with the problem.)

Quoted 7 lines

Quoted 4 lines

This is perfectly fine and this is how the generic common clock framework DT bindings work - clock-names property is a list of clock input names and clocks property contain specifiers of respective clocks.

There seems to be a some pressure to do clk_get()->of_clk_get() conversions, and also to find ways to lookup clocks.

I don't really see any need for this kind of conversion. clk_get() already handles lookup using DT correctly and from drivers perspective nothing changes.

Quoted 3 lines

It seems to me
that no one understands how DT handles clocks. Maybe that's where the problem is - lack of documentation about how DT clocks are handled.

Hmm, there is pretty much of description and examples in

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

For me it was enough to learn how DT based clock lookup works, but I'm quite used to DT, so I'm not a good test sample.

Best regards,
Tomasz

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 6 lines · Tero Kristo

drivers/clk/clkdev.c +6 -0

+
+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

This is utterly broken if you're looking up purely by a connection ID. Please, go back and read clk_get()'s documentation in linux/clk.h. It's spelt out very plainly there.

NAK.

Quoted 12 lines · Russell King - ARM Linux

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 6 lines · Tero Kristo

drivers/clk/clkdev.c +6 -0

+
+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

This is utterly broken if you're looking up purely by a connection ID. Please, go back and read clk_get()'s documentation in linux/clk.h. It's spelt out very plainly there.

Quoted 2 lines

NAK.

Hi Russell,

After looking at this a bit more, it seems difficult to get rid of the clk_get -> of_clk_get conversion, however based on this comment I am somewhat stuck. Do you think it would be acceptable if I change the implementation of this patch to work like this:

Quoted 16 lines · Tero Kristo

Quoted 12 lines · Russell King - ARM Linux

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 6 lines · Tero Kristo

drivers/clk/clkdev.c +6 -0

+
+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

This is utterly broken if you're looking up purely by a connection ID. Please, go back and read clk_get()'s documentation in linux/clk.h. It's spelt out very plainly there.

Quoted 2 lines

NAK.

Quoted 6 lines

Hi Russell,

After looking at this a bit more, it seems difficult to get rid of the clk_get -> of_clk_get conversion, however based on this comment I am somewhat stuck. Do you think it would be acceptable if I change the implementation of this patch to work like this:

Firstly, for clk_get(), you don't need to do anything - clk_get() already deals with DT, and if your DT is correct, it should already be returning the appropriate clock(s).

I guess the problem here is clk_get_sys(), because that doesn't take a struct device (and it doesn't take a struct device because it's used from code where there is no struct device to use with it.)

If you have an OF node, what's wrong with using of_clk_get_by_name()? If you don't have an OF node, how do you expect to find the clock in the device tree, remembering that clocks are specific to devices, and the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.

Quoted 24 lines · Russell King - ARM Linux

Quoted 16 lines · Tero Kristo

Quoted 12 lines · Russell King - ARM Linux

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 156

Quoted 6 lines · Tero Kristo

drivers/clk/clkdev.c +6 -0

+
+	if (cl)
+		return cl;
+
+	/* If clock was not found, attempt to look-up from DT */
+	node = of_find_node_by_name(NULL, con_id);

This is utterly broken if you're looking up purely by a connection ID. Please, go back and read clk_get()'s documentation in linux/clk.h. It's spelt out very plainly there.

Quoted 2 lines

NAK.

Quoted 6 lines

Hi Russell,

After looking at this a bit more, it seems difficult to get rid of the clk_get -> of_clk_get conversion, however based on this comment I am somewhat stuck. Do you think it would be acceptable if I change the implementation of this patch to work like this:

Quoted 7 lines

Firstly, for clk_get(), you don't need to do anything - clk_get() already deals with DT, and if your DT is correct, it should already be returning the appropriate clock(s).

I guess the problem here is clk_get_sys(), because that doesn't take a struct device (and it doesn't take a struct device because it's used from code where there is no struct device to use with it.)

Yeah, most of the OMAP clocks have been traditionally referenced in this way, they are using clk_get_sys and with dev=NULL. conn-id is mostly unique.

Quoted 5 lines

If you have an OF node, what's wrong with using of_clk_get_by_name()? If you don't have an OF node, how do you expect to find the clock in the device tree, remembering that clocks are specific to devices, and the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.

In the legacy code, pretty much all the clocks are mapped through clk_get_sys(NULL, conn), and large amount of the drivers don't even have DT conversion in place yet. I am attempting to handle the cases where we don't have OF nodes.... yet at least, without converting everything to DT, and without causing regressions (also, I don't even pretend to be capable of testing all the potential drivers for this.) Some parts of the basic infra which are using clk_get_sys are pretty silly also, like the ocp_if parts of the hwmod, I haven't figured out yet how to convert that to DT.

Personally I don't mind causing a regressions here though, if I get approval for it from Tony.

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
 .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
 arch/arm/mach-omap2/clock.h                        |  144 +-----
 arch/arm/mach-omap2/clock3xxx.h                    |    2 -
 drivers/clk/Makefile                               |    1 +
 drivers/clk/ti/Makefile                            |    3 +
 drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
 include/linux/clk/ti.h                             |  164 +++++++
 7 files changed, 727 insertions(+), 145 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt
 create mode 100644 drivers/clk/ti/Makefile
 create mode 100644 drivers/clk/ti/dpll.c
 create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +19 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+       clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
+       dd->clk_ref = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_ref)) {
+               pr_err("%s: ti,clk-ref for %s not found\n", __func__,
+                      clk_name);

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
  .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
  arch/arm/mach-omap2/clock.h                        |  144 +-----
  arch/arm/mach-omap2/clock3xxx.h                    |    2 -
  drivers/clk/Makefile                               |    1 +
  drivers/clk/ti/Makefile                            |    3 +
  drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
  include/linux/clk/ti.h                             |  164 +++++++
  7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

Documentation/devicetree/bindings/clock/ti/dpll.txt line 35

Quoted 7 lines

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

+- reg : array of register base addresses for controlling the DPLL (some
+  of these can also be left as NULL):
+       reg[0] = control register
+       reg[1] = idle status register
+       reg[2] = autoidle register
+       reg[3] = multiplier / divider set register

Are all of these always present? Using reg-names seems sensible here.

Documentation/devicetree/bindings/clock/ti/dpll.txt line 37

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +2 -0

+- ti,clk-ref : link phandle for the reference clock
+- ti,clk-bypass : link phandle for the bypass clock

You already have these in clocks, why do you need them again here?

Documentation/devicetree/bindings/clock/ti/dpll.txt line 40

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +3 -0

+
+Optional properties:
+- ti,modes : available modes for the DPLL

Huh? What type is that property? What does it mean?

Documentation/devicetree/bindings/clock/ti/dpll.txt line 43

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +3 -0

+- ti,recal-en-bit : recalibration enable bit
+- ti,recal-st-bit : recalibration status bit
+- ti,auto-recal-bit : auto recalibration enable bit

These seem rather low-level, but I see other clock bindings are doing similar things. When are these needed, and why? What type are they?

Documentation/devicetree/bindings/clock/ti/dpll.txt line 44

Quoted 2 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Documentation/devicetree/bindings/clock/ti/dpll.txt line 54

Quoted 11 lines

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

+
+Examples:
+       dpll_core_ck: dpll_core_ck at 44e00490 {
+               #clock-cells = <0>;
+               compatible = "ti,omap4-dpll-core-clock";
+               clocks = <&sys_clkin_ck>;
+               reg = <0x44e00490 0x4>, <0x44e0045c 0x4>, <0x0 0x4>,
+                               <0x44e00468 0x4>;
+               ti,clk-ref = <&sys_clkin_ck>;
+               ti,clk-bypass = <&sys_clkin_ck>;

Huh? Why aren't these both in the clocks property?

[...]

Documentation/devicetree/bindings/clock/ti/dpll.txt

Quoted 10 lines

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

+static inline void __iomem *get_dt_register(struct device_node *node,
+                                           int index)
+{
+       u32 val;
+
+       of_property_read_u32_index(node, "reg", 2 * index, &val);
+       if (val)
+               return of_iomap(node, index);
+       else
+               return NULL;

NAK. Use reg-names to handle registers being optional.

[...]

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+       clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
+       dd->clk_ref = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_ref)) {
+               pr_err("%s: ti,clk-ref for %s not found\n", __func__,
+                      clk_name);

Documentation/devicetree/bindings/clock/ti/dpll.txt

Quoted 10 lines

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

+               goto cleanup;
+       }
+
+       clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0);
+       dd->clk_bypass = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_bypass)) {
+               pr_err("%s: ti,clk-bypass for %s not found\n", __func__,
+                      clk_name);
+               goto cleanup;
+       }

You can get these from the standard clocks property. Don't do this.

Look at of_clk_get_by_name().

Thanks,
Mark.

Quoted 51 lines · Mark Rutland

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
  .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
  arch/arm/mach-omap2/clock.h                        |  144 +-----
  arch/arm/mach-omap2/clock3xxx.h                    |    2 -
  drivers/clk/Makefile                               |    1 +
  drivers/clk/ti/Makefile                            |    3 +
  drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
  include/linux/clk/ti.h                             |  164 +++++++
  7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 9 lines

Quoted 8 lines

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

+- reg : array of register base addresses for controlling the DPLL (some
+  of these can also be left as NULL):
+       reg[0] = control register
+       reg[1] = idle status register
+       reg[2] = autoidle register
+       reg[3] = multiplier / divider set register

Quoted 1 line

Are all of these always present? Using reg-names seems sensible here.

Not always, I'll change the code. I am quite new to DT and didn't actually know of the existence of reg-names prop.

Quoted 5 lines

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +3 -0

+- ti,recal-en-bit : recalibration enable bit
+- ti,recal-st-bit : recalibration status bit
+- ti,auto-recal-bit : auto recalibration enable bit

Quoted 2 lines

These seem rather low-level, but I see other clock bindings are doing similar things. When are these needed, and why? What type are they?

Bit shift values for the auto recalibration feature. HOWEVER, it seems these are actually legacy, kernel does not have support for these anymore if it ever had.... I think I can just drop these for now as they are unused by the support code even.

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 44

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

Quoted 2 lines

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

Quoted 15 lines

[...]

Documentation/devicetree/bindings/clock/ti/dpll.txt

Quoted 10 lines

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

+static inline void __iomem *get_dt_register(struct device_node *node,
+                                           int index)
+{
+       u32 val;
+
+       of_property_read_u32_index(node, "reg", 2 * index, &val);
+       if (val)
+               return of_iomap(node, index);
+       else
+               return NULL;

NAK. Use reg-names to handle registers being optional.

Yea will change this.

Quoted 20 lines

[...]

Quoted 15 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +15 -0

+       clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
+       dd->clk_ref = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_ref)) {
+               pr_err("%s: ti,clk-ref for %s not found\n", __func__,
+                      clk_name);
+               goto cleanup;
+       }
+
+       clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0);
+       dd->clk_bypass = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_bypass)) {
+               pr_err("%s: ti,clk-bypass for %s not found\n", __func__,
+                      clk_name);
+               goto cleanup;
+       }

Quoted 3 lines

You can get these from the standard clocks property. Don't do this.

Look at of_clk_get_by_name().

Or of_clk_get(node, index), which would be better?

Quoted 61 lines · Tero Kristo

Quoted 59 lines · Mark Rutland

Hi,

Quoted 43 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
   .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
   arch/arm/mach-omap2/clock.h                        |  144 +-----
   arch/arm/mach-omap2/clock3xxx.h                    |    2 -
   drivers/clk/Makefile                               |    1 +
   drivers/clk/ti/Makefile                            |    3 +
   drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
   include/linux/clk/ti.h                             |  164 +++++++
   7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +19 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 10 lines

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

+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",
+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

Quoted 3 lines

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

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

+- reg : array of register base addresses for controlling the DPLL (some
+  of these can also be left as NULL):
+       reg[0] = control register
+       reg[1] = idle status register
+       reg[2] = autoidle register
+       reg[3] = multiplier / divider set register

Quoted 4 lines

Quoted 1 line

Are all of these always present? Using reg-names seems sensible here.

Not always, I'll change the code. I am quite new to DT and didn't actually know of the existence of reg-names prop.

Ok. :)

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +3 -0

+- ti,recal-en-bit : recalibration enable bit
+- ti,recal-st-bit : recalibration status bit
+- ti,auto-recal-bit : auto recalibration enable bit

Quoted 7 lines

Quoted 2 lines

These seem rather low-level, but I see other clock bindings are doing similar things. When are these needed, and why? What type are they?

Bit shift values for the auto recalibration feature. HOWEVER, it seems these are actually legacy, kernel does not have support for these anymore if it ever had.... I think I can just drop these for now as they are unused by the support code even.

Ok.

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 44

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

Quoted 7 lines

Quoted 2 lines

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Quoted 17 lines

Quoted 4 lines

[...]

Quoted 12 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt

Quoted 10 lines

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

+static inline void __iomem *get_dt_register(struct device_node *node,
+                                           int index)
+{
+       u32 val;
+
+       of_property_read_u32_index(node, "reg", 2 * index, &val);
+       if (val)
+               return of_iomap(node, index);
+       else
+               return NULL;

NAK. Use reg-names to handle registers being optional.

Quoted 1 line

Yea will change this.

Cheers.

Quoted 20 lines

Quoted 20 lines

[...]

Quoted 15 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +15 -0

+       clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
+       dd->clk_ref = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_ref)) {
+               pr_err("%s: ti,clk-ref for %s not found\n", __func__,
+                      clk_name);
+               goto cleanup;
+       }
+
+       clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0);
+       dd->clk_bypass = of_clk_get_from_provider(&clkspec);
+       if (IS_ERR(dd->clk_bypass)) {
+               pr_err("%s: ti,clk-bypass for %s not found\n", __func__,
+                      clk_name);
+               goto cleanup;
+       }

Quoted 5 lines

Quoted 3 lines

You can get these from the standard clocks property. Don't do this.

Look at of_clk_get_by_name().

Or of_clk_get(node, index), which would be better?

If you know you always have both clocks, then yes.

Cheers,
Mark.

Quoted 66 lines · Mark Rutland

Quoted 64 lines · Tero Kristo

Quoted 51 lines · Mark Rutland

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
   .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
   arch/arm/mach-omap2/clock.h                        |  144 +-----
   arch/arm/mach-omap2/clock3xxx.h                    |    2 -
   drivers/clk/Makefile                               |    1 +
   drivers/clk/ti/Makefile                            |    3 +
   drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
   include/linux/clk/ti.h                             |  164 +++++++
   7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 3 lines

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified.

Quoted 16 lines

Quoted 11 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 44

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

Quoted 3 lines

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names.

Quoted 67 lines · Tero Kristo

Quoted 66 lines · Mark Rutland

Quoted 64 lines · Tero Kristo

Quoted 51 lines · Mark Rutland

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
    .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
    arch/arm/mach-omap2/clock.h                        |  144 +-----
    arch/arm/mach-omap2/clock3xxx.h                    |    2 -
    drivers/clk/Makefile                               |    1 +
    drivers/clk/ti/Makefile                            |    3 +
    drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
    include/linux/clk/ti.h                             |  164 +++++++
    7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 6 lines

Quoted 3 lines

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified.

Ok. It's always possible to add clock-names later if a platform doesn't wire an input. We lose the possibility of future compatibility, but backwards compatibility is easy enough to maintain.

Quoted 16 lines

Quoted 16 lines

Quoted 11 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 44

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

Quoted 8 lines

Quoted 3 lines

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names.

I unfortunately don't have a good answer here, because I'm not that familiar with how we handle clockdomains for power management purposes.

As I understand it, each clock domain is essentially a clock gate controlling multiple clock signals, so it's possible to describe that with the common clock bindings, with a domain's clocks all coming from a "domain-gate-clock" node (or something like that). That would make the wiring of clocks to a domain explicit and in line with the rest of the common clock bindings. But perhaps I've simplified things a bit too far.

I'm not sure how easy it would be to use that information for power management. I don't know what the kernel logic for clock domain power management needs to know about consumers of the clocks and how it would need to poke those consumers.

Cheers,
Mark.

Quoted 95 lines · Tero Kristo

Quoted 67 lines · Mark Rutland

Quoted 66 lines · Tero Kristo

Quoted 64 lines · Mark Rutland

Quoted 51 lines

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
    .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
    arch/arm/mach-omap2/clock.h                        |  144 +-----
    arch/arm/mach-omap2/clock3xxx.h                    |    2 -
    drivers/clk/Makefile                               |    1 +
    drivers/clk/ti/Makefile                            |    3 +
    drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
    include/linux/clk/ti.h                             |  164 +++++++
    7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 6 lines

Quoted 3 lines

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified.

Ok. It's always possible to add clock-names later if a platform doesn't wire an input. We lose the possibility of future compatibility, but backwards compatibility is easy enough to maintain.

Quoted 16 lines

Quoted 16 lines

Quoted 11 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 30

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names.

Quoted 5 lines

Quoted 1 line

I unfortunately don't have a good answer here, because I'm not that familiar with how we handle clockdomains for power management purposes.

Quoted 7 lines

As I understand it, each clock domain is essentially a clock gate controlling multiple clock signals, so it's possible to describe that with the common clock bindings, with a domain's clocks all coming from a "domain-gate-clock" node (or something like that). That would make the wiring of clocks to a domain explicit and in line with the rest of the common clock bindings. But perhaps I've simplified things a bit too far.

You got it basically right, but maybe oversimplified it a bit. The root/parent clocks can still cross clockdomain boundaries, so mapping everything under a simple clockdomain gate would not work. Basically each clock has a mapping on the SoC for both its parent clock signal and the clockdomain association, kind of having two parents at the same time. In OMAP case, most of the clockdomains support hardware autoidle type functionality, which puts the domain to idle once all the clocks on it are disabled.

-Tero

Quoted 9 lines

I'm not sure how easy it would be to use that information for power management. I don't know what the kernel logic for clock domain power management needs to know about consumers of the clocks and how it would need to poke those consumers.

Cheers,
Mark.

Quoting Tero Kristo (2013-08-19 10:06:39)

Quoted 100 lines · Mark Rutland

Quoted 99 lines · Tero Kristo

Quoted 67 lines · Mark Rutland

Quoted 66 lines · Tero Kristo

Quoted 64 lines · Mark Rutland

Quoted 51 lines

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
     .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
     arch/arm/mach-omap2/clock.h                        |  144 +-----
     arch/arm/mach-omap2/clock3xxx.h                    |    2 -
     drivers/clk/Makefile                               |    1 +
     drivers/clk/ti/Makefile                            |    3 +
     drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
     include/linux/clk/ti.h                             |  164 +++++++
     7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 6 lines

Quoted 3 lines

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified.

Ok. It's always possible to add clock-names later if a platform doesn't wire an input. We lose the possibility of future compatibility, but backwards compatibility is easy enough to maintain.

Quoted 20 lines

Quoted 19 lines

Quoted 11 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 30

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names.

I'm not sure that this binding should know about the clock domain at all. Maybe the clock domain binding should list the clocks that are within the domain?

Quoted 14 lines

Quoted 6 lines

Quoted 2 lines

I unfortunately don't have a good answer here, because I'm not that familiar with how we handle clockdomains for power management purposes.

Quoted 7 lines

As I understand it, each clock domain is essentially a clock gate controlling multiple clock signals, so it's possible to describe that with the common clock bindings, with a domain's clocks all coming from a "domain-gate-clock" node (or something like that). That would make the wiring of clocks to a domain explicit and in line with the rest of the common clock bindings. But perhaps I've simplified things a bit too far.

Quoted 8 lines

You got it basically right, but maybe oversimplified it a bit. The root/parent clocks can still cross clockdomain boundaries, so mapping everything under a simple clockdomain gate would not work. Basically each clock has a mapping on the SoC for both its parent clock signal and the clockdomain association, kind of having two parents at the same time. In OMAP case, most of the clockdomains support hardware autoidle type functionality, which puts the domain to idle once all the clocks on it are disabled.

I always thought that OMAP clock domains were poorly named. Seems to me that they had more to do with the IP/module programming than clocks per se. Again, I'm not sure that putting clkdm data into this binding is correct.

Is it because you want a call to clk_enable to program the clock domain in the .enable callback? I always thought that this was better left to a pm_runtime_get callback...

Regards,
Mike

Quoted 12 lines

-Tero

Quoted 9 lines

I'm not sure how easy it would be to use that information for power management. I don't know what the kernel logic for clock domain power management needs to know about consumers of the clocks and how it would need to poke those consumers.

Cheers,
Mark.

Quoted 101 lines

Quoting Tero Kristo (2013-08-19 10:06:39)

Quoted 100 lines · Mark Rutland

Quoted 99 lines · Tero Kristo

Quoted 67 lines · Mark Rutland

Quoted 66 lines · Tero Kristo

Quoted 64 lines · Mark Rutland

Quoted 51 lines

Hi,

Quoted 48 lines · Tero Kristo

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
     .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
     arch/arm/mach-omap2/clock.h                        |  144 +-----
     arch/arm/mach-omap2/clock3xxx.h                    |    2 -
     drivers/clk/Makefile                               |    1 +
     drivers/clk/ti/Makefile                            |    3 +
     drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
     include/linux/clk/ti.h                             |  164 +++++++
     7 files changed, 727 insertions(+), 145 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile
create mode 100644 drivers/clk/ti/dpll.c
create mode 100644 include/linux/clk/ti.h

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +24 -0

new file mode 100644
index 0000000..dcd6e63
@@ -0,0 +1,70 @@
+Binding for Texas Instruments DPLL clock.
+
+This binding uses the common clock binding[1].  It assumes a
+register-mapped DPLL with usually two selectable input clocks
+(reference clock and bypass clock), with digital phase locked
+loop logic for multiplying the input clock to a desired output
+clock. This clock also typically supports different operation
+modes (locked, low power stop etc.) This binding has several
+sub-types, which effectively result in slightly different setup
+for the actual DPLL clock.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of:
+               "ti,omap4-dpll-x2-clock",
+               "ti,omap3-dpll-clock",
+               "ti,omap3-dpll-core-clock",
+               "ti,omap3-dpll-per-clock",
+               "ti,omap3-dpll-per-j-type-clock",
+               "ti,omap4-dpll-clock",
+               "ti,omap4-dpll-core-clock",
+               "ti,omap4-dpll-m4xen-clock",
+               "ti,omap4-dpll-j-type-clock",

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 29

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +5 -0

+               "ti,omap4-dpll-no-gate-clock",
+               "ti,omap4-dpll-no-gate-j-type-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input).

All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always?

Quoted 6 lines

Quoted 3 lines

If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names.

Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified.

Ok. It's always possible to add clock-names later if a platform doesn't wire an input. We lose the possibility of future compatibility, but backwards compatibility is easy enough to maintain.

Quoted 20 lines

Quoted 19 lines

Quoted 11 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/dpll.txt line 30

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/dpll.txt new file +1 -0

+- ti,clkdm-name : clockdomain name for the DPLL

Could you elaborate on what this is for? What constitutes a valid name?

I'm not sure a string is the best way to define the linkage of several elements to a domain...

Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this.

I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data?

I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names.

I'm not sure that this binding should know about the clock domain at all. Maybe the clock domain binding should list the clocks that are within the domain?

Ok, I experimented with this stuff a bit to have a proper reply, and it looks like I can get this done with a clockdomain mapping, which has its own binding. Something like this:

         clockdomains {
                 usbhost_clkdm: usbhost_clkdm {
                         compatible = "ti,clockdomain";
                         clocks = <&usbhost_48m_fck>, <&usbhost_ick>;
                 };
        };

Mike, what is your approach on this? Are you okay having the implementation for this under drivers/clk? I recall you mentioned earlier that you don't want clockdomain stuff under drivers/clk, hence I am asking.

Here is the initial implementation for the binding:

void __init of_omap_clockdomain_setup(struct device_node *node)

{
         struct clk *clk;
         struct clk_hw *clk_hw;
         const char *clkdm_name = node->name;
         int i;
         int num_clks;

         num_clks = of_count_phandle_with_args(node, "clocks", 

"#clock-cells");

         for (i = 0; i < num_clks; i++) {
                 clk = of_clk_get(node, i);
                 if (__clk_get_flags(clk) & CLK_IS_BASIC) {
                         pr_warn("%s: can't setup clkdm for basic clk %s\n",
                                 __func__, __clk_get_name(clk));
                         continue;
                 }
                 clk_hw = __clk_get_hw(clk);
                 to_clk_hw_omap(clk_hw)->clkdm_name = clkdm_name;
                 omap2_init_clk_clkdm(clk_hw);
         }
}

CLK_OF_DECLARE(omap_clockdomain, "ti,clockdomain", of_omap_clockdomain_setup);

Quoted 23 lines

Quoted 14 lines

Quoted 6 lines

Quoted 2 lines

I unfortunately don't have a good answer here, because I'm not that familiar with how we handle clockdomains for power management purposes.

Quoted 7 lines

As I understand it, each clock domain is essentially a clock gate controlling multiple clock signals, so it's possible to describe that with the common clock bindings, with a domain's clocks all coming from a "domain-gate-clock" node (or something like that). That would make the wiring of clocks to a domain explicit and in line with the rest of the common clock bindings. But perhaps I've simplified things a bit too far.

Quoted 8 lines

You got it basically right, but maybe oversimplified it a bit. The root/parent clocks can still cross clockdomain boundaries, so mapping everything under a simple clockdomain gate would not work. Basically each clock has a mapping on the SoC for both its parent clock signal and the clockdomain association, kind of having two parents at the same time. In OMAP case, most of the clockdomains support hardware autoidle type functionality, which puts the domain to idle once all the clocks on it are disabled.

Quoted 8 lines

I always thought that OMAP clock domains were poorly named. Seems to me that they had more to do with the IP/module programming than clocks per se. Again, I'm not sure that putting clkdm data into this binding is correct.

Is it because you want a call to clk_enable to program the clock domain in the .enable callback? I always thought that this was better left to a pm_runtime_get callback...

My guess is that some clocks require the clockdomain itself to be forced on before they are enabled, some of the DPLLs do this for example. I am just trying not to cause any regressions with this set, thus attempting to keep most of the implementation specifics intact.

-Tero

Quoted 16 lines

Regards,
Mike

Quoted 12 lines

-Tero

Quoted 9 lines

I'm not sure how easy it would be to use that information for power management. I don't know what the kernel logic for clock domain power management needs to know about consumers of the clocks and how it would need to poke those consumers.

Cheers,
Mark.

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
 .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
 arch/arm/mach-omap2/clock.h                        |    9 --
 drivers/clk/ti/Makefile                            |    2 +-
 drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
 include/linux/clk/ti.h                             |    8 ++
 5 files changed, 156 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt
 create mode 100644 drivers/clk/ti/gate.c

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

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+

Documentation/devicetree/bindings/clock/ti/gate.txt new file +18 -0

+void __init of_omap_gate_clk_setup(struct device_node *node)
+{
+       struct clk *clk;
+       struct clk_init_data init = { 0 };
+       struct clk_hw_omap *clk_hw;
+       const char *clk_name = node->name;
+       int num_parents;
+       const char **parent_names = NULL;
+       int i;
+       u32 val;
+
+       clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+       if (!clk_hw) {
+               pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+               return;
+       }
+
+       clk_hw->hw.init = &init;

Quoted 35 lines · Tero Kristo

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
  .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
  arch/arm/mach-omap2/clock.h                        |    9 --
  drivers/clk/ti/Makefile                            |    2 +-
  drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
  include/linux/clk/ti.h                             |    8 ++
  5 files changed, 156 insertions(+), 10 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c

Documentation/devicetree/bindings/clock/ti/gate.txt new file +15 -0

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/gate-clock.txt
+
+Required properties:
+- compatible : shall be "ti,gate-clock"
+- #clock-cells : from common clock binding; shall be set to 0

Documentation/devicetree/bindings/clock/ti/gate.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+- clocks : link to phandle of parent clock
+
+Optional properties:
+- reg : base address for register controlling adjustable gate

Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?.

Documentation/devicetree/bindings/clock/ti/gate.txt line 20

Quoted 2 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,enable-bit : bit shift for programming the clock gate

Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)?

Documentation/devicetree/bindings/clock/ti/gate.txt line 22

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +2 -0

+- ti,dss-clk : use DSS hardware OPS for the clock
+- ti,am35xx-clk : use AM35xx hardware OPS for the clock

Those last two sounds like the kind of thing that should be derived from the compatible string (e.g. ti,am35xx-gate-clock).

Documentation/devicetree/bindings/clock/ti/gate.txt line 23

Quoted 2 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,clkdm-name : clockdomain to control this gate

As previously mentioned, I'm not a fan of this property. It would make more sense to describe the domain and have an explicit link to it (either nodes being children of the domain or having a phandle).

[...]

Quoted 24 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +24 -0

+void __init of_omap_gate_clk_setup(struct device_node *node)
+{
+       struct clk *clk;
+       struct clk_init_data init = { 0 };
+       struct clk_hw_omap *clk_hw;
+       const char *clk_name = node->name;
+       int num_parents;
+       const char **parent_names = NULL;
+       int i;
+       u32 val;
+
+       clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+       if (!clk_hw) {
+               pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+               return;
+       }
+
+       clk_hw->hw.init = &init;
+
+       of_property_read_string(node, "clock-output-names", &clk_name);
+       of_property_read_string(node, "ti,clkdm-name", &clk_hw->clkdm_name);
+
+       init.name = clk_name;
+       init.flags = 0;

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+
+       if (of_property_read_u32_index(node, "reg", 0, &val)) {
+               /* No register, clkdm control only */
+               init.ops = &omap_gate_clkdm_clk_ops;

If they're truly compatible, you can just see if you can of_iomap, and if not, continue. Your reg values might be wider than 32 bits, and usig of_property_read_u32 to read this feels wrong.

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +5 -0

+       } else {
+               init.ops = &omap_gate_clk_ops;
+               clk_hw->enable_reg = of_iomap(node, 0);
+               of_property_read_u32(node, "ti,enable-bit", &val);
+               clk_hw->enable_bit = val;

What if of_property_read_u32 failed to read the "ti,enable-bit" property? One might not be present, it's marked as optional in the binding description.

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 9 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +8 -0

+
+               clk_hw->ops = &clkhwops_wait;
+
+               if (of_property_read_bool(node, "ti,dss-clk"))
+                       clk_hw->ops = &clkhwops_omap3430es2_dss_usbhost_wait;
+
+               if (of_property_read_bool(node, "ti,am35xx-clk"))
+                       clk_hw->ops = &clkhwops_am35xx_ipss_module_wait;

I really don't like this. I think this should be done based on the compatible string.

Quoted 36 lines · Mark Rutland

Quoted 35 lines · Tero Kristo

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
  .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
  arch/arm/mach-omap2/clock.h                        |    9 --
  drivers/clk/ti/Makefile                            |    2 +-
  drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
  include/linux/clk/ti.h                             |    8 ++
  5 files changed, 156 insertions(+), 10 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c

Documentation/devicetree/bindings/clock/ti/gate.txt new file +15 -0

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/gate-clock.txt
+
+Required properties:
+- compatible : shall be "ti,gate-clock"
+- #clock-cells : from common clock binding; shall be set to 0

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+- clocks : link to phandle of parent clock
+
+Optional properties:
+- reg : base address for register controlling adjustable gate

Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?.

I think I implemented this in somewhat confusing manner. This could be split to:

ti,gate-clock:
requires reg and ti,enable-bit info
ti,clkdm-clock:
requires ti,clkdm-name

clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active.

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 20

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,enable-bit : bit shift for programming the clock gate

Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)?

Yea, same register is shared.

Quoted 7 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 22

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +2 -0

+- ti,dss-clk : use DSS hardware OPS for the clock
+- ti,am35xx-clk : use AM35xx hardware OPS for the clock

Those last two sounds like the kind of thing that should be derived from the compatible string (e.g. ti,am35xx-gate-clock).

Hmm yea, I think I can change this and add some sub-types.

Quoted 1 line

Quoted 5 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 23

Quoted 1 line

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,clkdm-name : clockdomain to control this gate

As previously mentioned, I'm not a fan of this property. It would make more sense to describe the domain and have an explicit link to it (either nodes being children of the domain or having a phandle).

Same comments as with patch #2.

Quoted 28 lines

[...]

Quoted 24 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +24 -0

+void __init of_omap_gate_clk_setup(struct device_node *node)
+{
+       struct clk *clk;
+       struct clk_init_data init = { 0 };
+       struct clk_hw_omap *clk_hw;
+       const char *clk_name = node->name;
+       int num_parents;
+       const char **parent_names = NULL;
+       int i;
+       u32 val;
+
+       clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+       if (!clk_hw) {
+               pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+               return;
+       }
+
+       clk_hw->hw.init = &init;
+
+       of_property_read_string(node, "clock-output-names", &clk_name);
+       of_property_read_string(node, "ti,clkdm-name", &clk_hw->clkdm_name);
+
+       init.name = clk_name;
+       init.flags = 0;

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+
+       if (of_property_read_u32_index(node, "reg", 0, &val)) {
+               /* No register, clkdm control only */
+               init.ops = &omap_gate_clkdm_clk_ops;

If they're truly compatible, you can just see if you can of_iomap, and if not, continue. Your reg values might be wider than 32 bits, and usig of_property_read_u32 to read this feels wrong.

I'll split this to two separate supported types.

Quoted 10 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +5 -0

+       } else {
+               init.ops = &omap_gate_clk_ops;
+               clk_hw->enable_reg = of_iomap(node, 0);
+               of_property_read_u32(node, "ti,enable-bit", &val);
+               clk_hw->enable_bit = val;

What if of_property_read_u32 failed to read the "ti,enable-bit" property? One might not be present, it's marked as optional in the binding description.

I'll make sure this is present in case it is needed.

Quoted 12 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 9 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +8 -0

+
+               clk_hw->ops = &clkhwops_wait;
+
+               if (of_property_read_bool(node, "ti,dss-clk"))
+                       clk_hw->ops = &clkhwops_omap3430es2_dss_usbhost_wait;
+
+               if (of_property_read_bool(node, "ti,am35xx-clk"))
+                       clk_hw->ops = &clkhwops_am35xx_ipss_module_wait;

I really don't like this. I think this should be done based on the compatible string.

Yea, will change.

Quoted 49 lines · Tero Kristo

Quoted 36 lines · Mark Rutland

Quoted 35 lines · Tero Kristo

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
   .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
   arch/arm/mach-omap2/clock.h                        |    9 --
   drivers/clk/ti/Makefile                            |    2 +-
   drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
   include/linux/clk/ti.h                             |    8 ++
   5 files changed, 156 insertions(+), 10 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c

Documentation/devicetree/bindings/clock/ti/gate.txt new file +15 -0

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/gate-clock.txt
+
+Required properties:
+- compatible : shall be "ti,gate-clock"
+- #clock-cells : from common clock binding; shall be set to 0

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+- clocks : link to phandle of parent clock
+
+Optional properties:
+- reg : base address for register controlling adjustable gate

Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?.

I think I implemented this in somewhat confusing manner. This could be split to:

Quoted 7 lines

ti,gate-clock:
requires reg and ti,enable-bit info
ti,clkdm-clock:
requires ti,clkdm-name

clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active.

Ok.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,enable-bit : bit shift for programming the clock gate

Quoted 4 lines

Quoted 2 lines

Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)?

Yea, same register is shared.

Ok. Are those gate clocks are part of a larger "gate clocks" block, with the register that controls them? or are they really independent? Does the register control other items too?

If they were part of some bigger unit, it might be possible to have a binding like the following (though maybe not):

gate_clks: gate_clks {
	compatible = "ti,gate-clocks-block";
	reg = <0x4003a000 0x1000>;

	#clock-cells = <1>;
	/*
	 * has 4 gate clocks at bit shifts 0,1,2,3,
	 * corresponding to input clocks 0,1,2,3
	 */
	clocks = <&clk1 0 23>,
		 <&clk1 0 4>,
		 <&clk3 2>,
		 <&clk3 5>;
};

device {
	compatible = "vendor,some-device";
	clocks = <&gate_clks 1>; /* gate clock with bitshift 1*/
};

Quoted 13 lines

Quoted 7 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 22

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +2 -0

+- ti,dss-clk : use DSS hardware OPS for the clock
+- ti,am35xx-clk : use AM35xx hardware OPS for the clock

Those last two sounds like the kind of thing that should be derived from the compatible string (e.g. ti,am35xx-gate-clock).

Hmm yea, I think I can change this and add some sub-types.

Quoted 3 lines

Quoted 2 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,clkdm-name : clockdomain to control this gate

Quoted 5 lines

Quoted 3 lines

As previously mentioned, I'm not a fan of this property. It would make more sense to describe the domain and have an explicit link to it (either nodes being children of the domain or having a phandle).

Same comments as with patch #2.

Same reply as to patch #2 :)

Quoted 66 lines

Quoted 29 lines

[...]

Quoted 24 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +24 -0

+void __init of_omap_gate_clk_setup(struct device_node *node)
+{
+       struct clk *clk;
+       struct clk_init_data init = { 0 };
+       struct clk_hw_omap *clk_hw;
+       const char *clk_name = node->name;
+       int num_parents;
+       const char **parent_names = NULL;
+       int i;
+       u32 val;
+
+       clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+       if (!clk_hw) {
+               pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+               return;
+       }
+
+       clk_hw->hw.init = &init;
+
+       of_property_read_string(node, "clock-output-names", &clk_name);
+       of_property_read_string(node, "ti,clkdm-name", &clk_hw->clkdm_name);
+
+       init.name = clk_name;
+       init.flags = 0;

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+
+       if (of_property_read_u32_index(node, "reg", 0, &val)) {
+               /* No register, clkdm control only */
+               init.ops = &omap_gate_clkdm_clk_ops;

If they're truly compatible, you can just see if you can of_iomap, and if not, continue. Your reg values might be wider than 32 bits, and usig of_property_read_u32 to read this feels wrong.

I'll split this to two separate supported types.

Quoted 10 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +5 -0

+       } else {
+               init.ops = &omap_gate_clk_ops;
+               clk_hw->enable_reg = of_iomap(node, 0);
+               of_property_read_u32(node, "ti,enable-bit", &val);
+               clk_hw->enable_bit = val;

What if of_property_read_u32 failed to read the "ti,enable-bit" property? One might not be present, it's marked as optional in the binding description.

I'll make sure this is present in case it is needed.

Quoted 12 lines

Documentation/devicetree/bindings/clock/ti/gate.txt

Quoted 9 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +8 -0

+
+               clk_hw->ops = &clkhwops_wait;
+
+               if (of_property_read_bool(node, "ti,dss-clk"))
+                       clk_hw->ops = &clkhwops_omap3430es2_dss_usbhost_wait;
+
+               if (of_property_read_bool(node, "ti,am35xx-clk"))
+                       clk_hw->ops = &clkhwops_am35xx_ipss_module_wait;

I really don't like this. I think this should be done based on the compatible string.

Quoted 1 line

Yea, will change.

Cheers!

Mark.

Quoted 69 lines · Mark Rutland

Quoted 49 lines · Tero Kristo

Quoted 36 lines · Mark Rutland

Quoted 35 lines · Tero Kristo

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
   .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
   arch/arm/mach-omap2/clock.h                        |    9 --
   drivers/clk/ti/Makefile                            |    2 +-
   drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
   include/linux/clk/ti.h                             |    8 ++
   5 files changed, 156 insertions(+), 10 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c

Documentation/devicetree/bindings/clock/ti/gate.txt new file +15 -0

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/gate-clock.txt
+
+Required properties:
+- compatible : shall be "ti,gate-clock"
+- #clock-cells : from common clock binding; shall be set to 0

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+- clocks : link to phandle of parent clock
+
+Optional properties:
+- reg : base address for register controlling adjustable gate

Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?.

I think I implemented this in somewhat confusing manner. This could be split to:

Quoted 7 lines

ti,gate-clock:
requires reg and ti,enable-bit info
ti,clkdm-clock:
requires ti,clkdm-name

clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active.

Ok.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 20

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,enable-bit : bit shift for programming the clock gate

Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)?

Yea, same register is shared.

Quoted 3 lines

Ok. Are those gate clocks are part of a larger "gate clocks" block, with the register that controls them? or are they really independent? Does the register control other items too?

Not really. Typically they only have the clockdomain in common, and the individual clocks are mostly controlled independently from each other. For example on omap3 we have following register:

CM_FCLKEN_PER
Physical Address 0x4800 5000
BIT31:19 RESERVED Write 0s for future compatibility. Read returns 0. BIT18 EN_UART4 UART4 functional clock control.
BIT17 EN_GPIO6 GPIO6 functional clock control.
BIT16 EN_GPIO5 GPIO5 functional clock control.
BIT15 EN_GPIO4 GPIO4 functional clock control.
BIT14 EN_GPIO3 GPIO3 functional clock control.
BIT13 EN_GPIO2 GPIO2 functional clock control.
BIT12 EN_WDT3 WDT3 functional clock control.
BIT11 EN_UART3 Type UART3 functional clock control.
BIT10 EN_GPT9 GPTIMER 9 functional clock control.
BIT9 EN_GPT8 GPTIMER 8 functional clock control.
BIT8 EN_GPT7 GPTIMER 7 functional clock control.
BIT7 EN_GPT6 GPTIMER 6 functional clock control.
BIT6 EN_GPT5 GPTIMER 5 functional clock control.
BIT5 EN_GPT4 GPTIMER 4 functional clock control.
BIT4 EN_GPT3GPTIMER 3 functional clock control.
BIT3 EN_GPT2 GPTIMER 2 functional clock control.
BIT2 EN_MCBSP4 McBSP 4 functional clock control.
BIT1 EN_MCBSP3 McBSP3 functional clock control.
BIT0 EN_MCBSP2 McBSP 2 functional clock control.

So multiple drivers will be using this register for example.

Quoted 74 lines · Tero Kristo

Quoted 72 lines · Mark Rutland

Quoted 49 lines · Tero Kristo

Quoted 36 lines · Mark Rutland

Quoted 35 lines · Tero Kristo

This node adds support for a clock node which allows control to the clockdomain enable / disable.

Signed-off-by Tero Kristo <[email protected]>

---
    .../devicetree/bindings/clock/ti/gate.txt          |   41 ++++++++
    arch/arm/mach-omap2/clock.h                        |    9 --
    drivers/clk/ti/Makefile                            |    2 +-
    drivers/clk/ti/gate.c                              |  106 ++++++++++++++++++++
    include/linux/clk/ti.h                             |    8 ++
    5 files changed, 156 insertions(+), 10 deletions(-)

create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c

Documentation/devicetree/bindings/clock/ti/gate.txt new file +15 -0

new file mode 100644
index 0000000..620a73d
@@ -0,0 +1,41 @@
+Binding for Texas Instruments gate clock.
+
+This binding uses the common clock binding[1]. This clock is
+quite much similar to the basic gate-clock [2], however,
+it supports a number of additional features. If no register
+is provided for this clock, the code assumes that a clockdomain
+will be controlled instead and the corresponding hw-ops for
+that is used.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/gate-clock.txt
+
+Required properties:
+- compatible : shall be "ti,gate-clock"
+- #clock-cells : from common clock binding; shall be set to 0

Quoted 8 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +4 -0

+- clocks : link to phandle of parent clock
+
+Optional properties:
+- reg : base address for register controlling adjustable gate

Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?.

I think I implemented this in somewhat confusing manner. This could be split to:

Quoted 7 lines

ti,gate-clock:
requires reg and ti,enable-bit info
ti,clkdm-clock:
requires ti,clkdm-name

clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active.

Ok.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/ti/gate.txt line 20

Quoted 3 lines

Documentation/devicetree/bindings/clock/ti/gate.txt new file +1 -0

+- ti,enable-bit : bit shift for programming the clock gate

Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)?

Yea, same register is shared.

Ok. Are those gate clocks are part of a larger "gate clocks" block, with the register that controls them? or are they really independent? Does the register control other items too?

Quoted 3 lines

Not really. Typically they only have the clockdomain in common, and the individual clocks are mostly controlled independently from each other. For example on omap3 we have following register:

You say they typically only have the clockdomain in common. Do you mean that they always have the same clockdomain, but not necessarily other properties, or may they not even have the clockdomain in common?

Quoted 25 lines

CM_FCLKEN_PER
Physical Address 0x4800 5000
BIT31:19 RESERVED Write 0s for future compatibility. Read returns 0. BIT18 EN_UART4 UART4 functional clock control.
BIT17 EN_GPIO6 GPIO6 functional clock control.
BIT16 EN_GPIO5 GPIO5 functional clock control.
BIT15 EN_GPIO4 GPIO4 functional clock control.
BIT14 EN_GPIO3 GPIO3 functional clock control.
BIT13 EN_GPIO2 GPIO2 functional clock control.
BIT12 EN_WDT3 WDT3 functional clock control.
BIT11 EN_UART3 Type UART3 functional clock control.
BIT10 EN_GPT9 GPTIMER 9 functional clock control.
BIT9 EN_GPT8 GPTIMER 8 functional clock control.
BIT8 EN_GPT7 GPTIMER 7 functional clock control.
BIT7 EN_GPT6 GPTIMER 6 functional clock control.
BIT6 EN_GPT5 GPTIMER 5 functional clock control.
BIT5 EN_GPT4 GPTIMER 4 functional clock control.
BIT4 EN_GPT3GPTIMER 3 functional clock control.
BIT3 EN_GPT2 GPTIMER 2 functional clock control.
BIT2 EN_MCBSP4 McBSP 4 functional clock control.
BIT1 EN_MCBSP3 McBSP3 functional clock control.
BIT0 EN_MCBSP2 McBSP 2 functional clock control.

Quoted 1 line

So multiple drivers will be using this register for example.

The point I was trying to get across is that this looks like a single logical block which controls the (independent) gating of several clocks, along the same lines as multiple swtiches bound together in a DIP switch.

It's equally valid to view that as several clock gates which happen to have their control bits in close proximity in the memory map, as you suggest.

Thanks,
Mark.

This patch creates a unique node for each clock in the OMAP4 power, reset and clock manager (PRCM). OMAP443x and OMAP446x have slightly different clock tree which is taken into account in the data.

Signed-off-by Tero Kristo <[email protected]>

---
 arch/arm/boot/dts/omap443x-clocks.dtsi |   17 +
 arch/arm/boot/dts/omap443x.dtsi        |    8 +
 arch/arm/boot/dts/omap4460.dtsi        |    8 +
 arch/arm/boot/dts/omap446x-clocks.dtsi |   27 +
 arch/arm/boot/dts/omap44xx-clocks.dtsi | 1648
++++++++++++++++++++++++++++++++ 5 files changed, 1708 insertions(+)
 create mode 100644 arch/arm/boot/dts/omap443x-clocks.dtsi
 create mode 100644 arch/arm/boot/dts/omap446x-clocks.dtsi
 create mode 100644 arch/arm/boot/dts/omap44xx-clocks.dtsi

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap443x-clocks.dtsi new file mode 100644 index 0000000..2bd82b2

arch/arm/boot/dts/omap443x-clocks.dtsi new file +7 -0

@@ -0,0 +1,17 @@
+/*
+ * Device Tree Source for OMAP443x clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap443x-clocks.dtsi new file +8 -0

+
+bandgap_fclk: bandgap_fclk at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&sys_32k_ck>;
+	bit-shift = <8>;
+	reg = <0x4a307888 0x4>;
+};

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap443x.dtsi index bcf455e..dfd648c 100644

arch/arm/boot/dts/omap443x.dtsi +8 -0

@@ -30,4 +30,12 @@
 		       0x4a00232C 0x4>;
 		compatible = "ti,omap4430-bandgap";
 	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		/include/ "omap44xx-clocks.dtsi"
+		/include/ "omap443x-clocks.dtsi"
+	};
 };

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap4460.dtsi index c2f0f39..d9d00b2 100644

arch/arm/boot/dts/omap4460.dtsi +8 -0

@@ -38,4 +38,12 @@
 		interrupts = <0 126 IRQ_TYPE_LEVEL_HIGH>; /* talert */
 		gpios = <&gpio3 22 0>; /* tshut */
 	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		/include/ "omap44xx-clocks.dtsi"
+		/include/ "omap446x-clocks.dtsi"
+	};
 };

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap446x-clocks.dtsi new file mode 100644 index 0000000..86d0805

arch/arm/boot/dts/omap446x-clocks.dtsi new file +7 -0

@@ -0,0 +1,27 @@
+/*
+ * Device Tree Source for OMAP446x clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap446x-clocks.dtsi new file +18 -0

+
+div_ts_ck: div_ts_ck at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "divider-clock";
+	clocks = <&l4_wkup_clk_mux_ck>;
+	bit-shift = <24>;
+	reg = <0x4a307888 0x4>;
+	table = < 8 0 >, < 16 1 >, < 32 2 >;
+	bit-mask = <0x3>;
+};
+
+bandgap_ts_fclk: bandgap_ts_fclk at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&div_ts_ck>;
+	bit-shift = <8>;
+	reg = <0x4a307888 0x4>;
+};

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap44xx-clocks.dtsi new file mode 100644 index 0000000..23f623c

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +7 -0

@@ -0,0 +1,1648 @@
+/*
+ * Device Tree Source for OMAP4 clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +99 -0

+
+extalt_clkin_ck: extalt_clkin_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <59000000>;
+};
+
+pad_clks_src_ck: pad_clks_src_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+pad_clks_ck: pad_clks_ck at 4a004108 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&pad_clks_src_ck>;
+	bit-shift = <8>;
+	reg = <0x4a004108 0x4>;
+};
+
+pad_slimbus_core_clks_ck: pad_slimbus_core_clks_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+secure_32k_clk_src_ck: secure_32k_clk_src_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <32768>;
+};
+
+slimbus_src_clk: slimbus_src_clk {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+slimbus_clk: slimbus_clk at 4a004108 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&slimbus_src_clk>;
+	bit-shift = <10>;
+	reg = <0x4a004108 0x4>;
+};
+
+sys_32k_ck: sys_32k_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <32768>;
+};
+
+virt_12000000_ck: virt_12000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+virt_13000000_ck: virt_13000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <13000000>;
+};
+
+virt_16800000_ck: virt_16800000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <16800000>;
+};
+
+virt_19200000_ck: virt_19200000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <19200000>;
+};
+
+virt_26000000_ck: virt_26000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <26000000>;
+};
+
+virt_27000000_ck: virt_27000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <27000000>;
+};
+
+virt_38400000_ck: virt_38400000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <38400000>;
+};
+
+sys_clkin_ck: sys_clkin_ck at 4a306110 {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&virt_12000000_ck>, <&virt_13000000_ck>,

<&virt_16800000_ck>, <&virt_19200000_ck>, <&virt_26000000_ck>,
<&virt_27000000_ck>, <&virt_38400000_ck>; + reg = <0x4a306110 0x4>;

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +56 -0

+	bit-mask = <0x7>;
+	index-starts-at-one;
+};
+
+tie_low_clock_ck: tie_low_clock_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <0>;
+};
+
+utmi_phy_clkout_ck: utmi_phy_clkout_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60mhsp1_ck: xclk60mhsp1_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60mhsp2_ck: xclk60mhsp2_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60motg_ck: xclk60motg_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+abe_dpll_bypass_clk_mux_ck: abe_dpll_bypass_clk_mux_ck at 4a306108 {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&sys_clkin_ck>, <&sys_32k_ck>;
+	bit-shift = <24>;
+	reg = <0x4a306108 0x4>;
+	bit-mask = <0x1>;
+};
+
+abe_dpll_refclk_mux_ck: abe_dpll_refclk_mux_ck at 4a30610c {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&sys_clkin_ck>, <&sys_32k_ck>;
+	reg = <0x4a30610c 0x4>;
+	bit-mask = <0x1>;
+};
+
+dpll_abe_ck: dpll_abe_ck at 4a0041e0 {
+	#clock-cells = <0>;
+	compatible = "ti,omap4-dpll-m4xen-clock";
+	clocks = <&abe_dpll_refclk_mux_ck>, <&abe_dpll_bypass_clk_mux_ck>;
+	reg = <0x4a0041e0 0x4>, <0x4a0041e4 0x4>, <0x4a0041e8 0x4>,

<0x4a0041ec 0x4>; + ti,clk-ref = <&abe_dpll_refclk_mux_ck>;

Quoted 279 lines · On Friday 02 of August 2013 19:25:25 Tero Kristo wrote:

This patch creates a unique node for each clock in the OMAP4 power, reset and clock manager (PRCM). OMAP443x and OMAP446x have slightly different clock tree which is taken into account in the data.

Signed-off-by Tero Kristo <[email protected]>

---
  arch/arm/boot/dts/omap443x-clocks.dtsi |   17 +
  arch/arm/boot/dts/omap443x.dtsi        |    8 +
  arch/arm/boot/dts/omap4460.dtsi        |    8 +
  arch/arm/boot/dts/omap446x-clocks.dtsi |   27 +
  arch/arm/boot/dts/omap44xx-clocks.dtsi | 1648

++++++++++++++++++++++++++++++++ 5 files changed, 1708 insertions(+)
create mode 100644 arch/arm/boot/dts/omap443x-clocks.dtsi create mode 100644 arch/arm/boot/dts/omap446x-clocks.dtsi create mode 100644 arch/arm/boot/dts/omap44xx-clocks.dtsi

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap443x-clocks.dtsi new file mode 100644 index 0000000..2bd82b2

arch/arm/boot/dts/omap443x-clocks.dtsi new file +7 -0

@@ -0,0 +1,17 @@
+/*
+ * Device Tree Source for OMAP443x clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap443x-clocks.dtsi new file +8 -0

+
+bandgap_fclk: bandgap_fclk at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&sys_32k_ck>;
+	bit-shift = <8>;
+	reg = <0x4a307888 0x4>;
+};

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap443x.dtsi index bcf455e..dfd648c 100644

arch/arm/boot/dts/omap443x.dtsi +8 -0

@@ -30,4 +30,12 @@
  		       0x4a00232C 0x4>;
  		compatible = "ti,omap4430-bandgap";
  	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		/include/ "omap44xx-clocks.dtsi"
+		/include/ "omap443x-clocks.dtsi"
+	};
  };

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap4460.dtsi index c2f0f39..d9d00b2 100644

arch/arm/boot/dts/omap4460.dtsi +8 -0

@@ -38,4 +38,12 @@
  		interrupts = <0 126 IRQ_TYPE_LEVEL_HIGH>; /* talert */
  		gpios = <&gpio3 22 0>; /* tshut */
  	};
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		/include/ "omap44xx-clocks.dtsi"
+		/include/ "omap446x-clocks.dtsi"
+	};
  };

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap446x-clocks.dtsi new file mode 100644 index 0000000..86d0805

arch/arm/boot/dts/omap446x-clocks.dtsi new file +7 -0

@@ -0,0 +1,27 @@
+/*
+ * Device Tree Source for OMAP446x clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap446x-clocks.dtsi new file +18 -0

+
+div_ts_ck: div_ts_ck at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "divider-clock";
+	clocks = <&l4_wkup_clk_mux_ck>;
+	bit-shift = <24>;
+	reg = <0x4a307888 0x4>;
+	table = < 8 0 >, < 16 1 >, < 32 2 >;
+	bit-mask = <0x3>;
+};
+
+bandgap_ts_fclk: bandgap_ts_fclk at 4a307888 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&div_ts_ck>;
+	bit-shift = <8>;
+	reg = <0x4a307888 0x4>;
+};

(quoted diff) +0 -0

b/arch/arm/boot/dts/omap44xx-clocks.dtsi new file mode 100644 index 0000000..23f623c

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +7 -0

@@ -0,0 +1,1648 @@
+/*
+ * Device Tree Source for OMAP4 clock data
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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 +

* published by the Free Software Foundation.
+ */

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +99 -0

+
+extalt_clkin_ck: extalt_clkin_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <59000000>;
+};
+
+pad_clks_src_ck: pad_clks_src_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+pad_clks_ck: pad_clks_ck at 4a004108 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&pad_clks_src_ck>;
+	bit-shift = <8>;
+	reg = <0x4a004108 0x4>;
+};
+
+pad_slimbus_core_clks_ck: pad_slimbus_core_clks_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+secure_32k_clk_src_ck: secure_32k_clk_src_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <32768>;
+};
+
+slimbus_src_clk: slimbus_src_clk {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+slimbus_clk: slimbus_clk at 4a004108 {
+	#clock-cells = <0>;
+	compatible = "gate-clock";
+	clocks = <&slimbus_src_clk>;
+	bit-shift = <10>;
+	reg = <0x4a004108 0x4>;
+};
+
+sys_32k_ck: sys_32k_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <32768>;
+};
+
+virt_12000000_ck: virt_12000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <12000000>;
+};
+
+virt_13000000_ck: virt_13000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <13000000>;
+};
+
+virt_16800000_ck: virt_16800000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <16800000>;
+};
+
+virt_19200000_ck: virt_19200000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <19200000>;
+};
+
+virt_26000000_ck: virt_26000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <26000000>;
+};
+
+virt_27000000_ck: virt_27000000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <27000000>;
+};
+
+virt_38400000_ck: virt_38400000_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <38400000>;
+};
+
+sys_clkin_ck: sys_clkin_ck at 4a306110 {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&virt_12000000_ck>, <&virt_13000000_ck>,

<&virt_16800000_ck>, <&virt_19200000_ck>, <&virt_26000000_ck>,
<&virt_27000000_ck>, <&virt_38400000_ck>; + reg = <0x4a306110 0x4>;

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +56 -0

+	bit-mask = <0x7>;
+	index-starts-at-one;
+};
+
+tie_low_clock_ck: tie_low_clock_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <0>;
+};
+
+utmi_phy_clkout_ck: utmi_phy_clkout_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60mhsp1_ck: xclk60mhsp1_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60mhsp2_ck: xclk60mhsp2_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+xclk60motg_ck: xclk60motg_ck {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <60000000>;
+};
+
+abe_dpll_bypass_clk_mux_ck: abe_dpll_bypass_clk_mux_ck at 4a306108 {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&sys_clkin_ck>, <&sys_32k_ck>;
+	bit-shift = <24>;
+	reg = <0x4a306108 0x4>;
+	bit-mask = <0x1>;
+};
+
+abe_dpll_refclk_mux_ck: abe_dpll_refclk_mux_ck at 4a30610c {
+	#clock-cells = <0>;
+	compatible = "mux-clock";
+	clocks = <&sys_clkin_ck>, <&sys_32k_ck>;
+	reg = <0x4a30610c 0x4>;
+	bit-mask = <0x1>;
+};
+
+dpll_abe_ck: dpll_abe_ck at 4a0041e0 {
+	#clock-cells = <0>;
+	compatible = "ti,omap4-dpll-m4xen-clock";
+	clocks = <&abe_dpll_refclk_mux_ck>, <&abe_dpll_bypass_clk_mux_ck>;
+	reg = <0x4a0041e0 0x4>, <0x4a0041e4 0x4>, <0x4a0041e8 0x4>,

<0x4a0041ec 0x4>; + ti,clk-ref = <&abe_dpll_refclk_mux_ck>;

arch/arm/boot/dts/omap44xx-clocks.dtsi line 167

Quoted 1 line

arch/arm/boot/dts/omap44xx-clocks.dtsi new file +1 -0

+	ti,clk-bypass = <&abe_dpll_bypass_clk_mux_ck>;

Hmm, why do you need to pass references to other clocks using private properties? Is there a reason you can't use the clocks and clock-names properties defined by standard clock bindings?

The same for other PLLs defined in this patch.

clk-44xx.c now contains the clock init functionality for omap4, including DT clock registration and adding of static clkdev entries.

drivers/clk/ti/clk-44xx.c new file +0 -0

drivers/clk/ti/clk-44xx.c new file +28 -0

+int __init omap4xxx_clk_init(void)
+{
+	int rc;
+	struct clk *abe_dpll_ref, *abe_dpll, *sys_32k_ck, *usb_dpll;
+
+	/* FIXME register clocks from DT first */
+	of_clk_init(NULL);
+
+	omap_dt_clocks_register(omap44xx_clks);
+
+	omap2_clk_disable_autoidle_all();
+
+	/*
+	 * On OMAP4460 the ABE DPLL fails to turn on if in idle low-power
+	 * state when turning the ABE clock domain. Workaround this by
+	 * locking the ABE DPLL on boot.
+	 * Lock the ABE DPLL in any case to avoid issues with audio.
+	 */
+	abe_dpll_ref = clk_get_sys(NULL, "abe_dpll_refclk_mux_ck");
+	sys_32k_ck = clk_get_sys(NULL, "sys_32k_ck");
+	rc = clk_set_parent(abe_dpll_ref, sys_32k_ck);
+	abe_dpll = clk_get_sys(NULL, "dpll_abe_ck");
+	if (!rc)
+		rc = clk_set_rate(abe_dpll, OMAP4_DPLL_ABE_DEFFREQ);
+	if (rc)
+		pr_err("%s: failed to configure ABE DPLL!\n", __func__);
+
+	/*

Quoted 2 lines · Tero Kristo

clk-44xx.c now contains the clock init functionality for omap4, including DT clock registration and adding of static clkdev entries.

Few comments below from "boot new hardware with old kernels" point of view that seems to be pretty close for clocks.

Quoted 2 lines

drivers/clk/ti/clk-44xx.c new file +0 -0

...

Quoted 28 lines

drivers/clk/ti/clk-44xx.c new file +28 -0

+int __init omap4xxx_clk_init(void)
+{
+	int rc;
+	struct clk *abe_dpll_ref, *abe_dpll, *sys_32k_ck, *usb_dpll;
+
+	/* FIXME register clocks from DT first */
+	of_clk_init(NULL);
+
+	omap_dt_clocks_register(omap44xx_clks);
+
+	omap2_clk_disable_autoidle_all();
+
+	/*
+	 * On OMAP4460 the ABE DPLL fails to turn on if in idle low-power
+	 * state when turning the ABE clock domain. Workaround this by
+	 * locking the ABE DPLL on boot.
+	 * Lock the ABE DPLL in any case to avoid issues with audio.
+	 */
+	abe_dpll_ref = clk_get_sys(NULL, "abe_dpll_refclk_mux_ck");
+	sys_32k_ck = clk_get_sys(NULL, "sys_32k_ck");
+	rc = clk_set_parent(abe_dpll_ref, sys_32k_ck);
+	abe_dpll = clk_get_sys(NULL, "dpll_abe_ck");
+	if (!rc)
+		rc = clk_set_rate(abe_dpll, OMAP4_DPLL_ABE_DEFFREQ);
+	if (rc)
+		pr_err("%s: failed to configure ABE DPLL!\n", __func__);
+
+	/*

drivers/clk/ti/clk-44xx.c line 118

Quoted 10 lines

drivers/clk/ti/clk-44xx.c new file +10 -0

+	 * Lock USB DPLL on OMAP4 devices so that the L3INIT power
+	 * domain can transition to retention state when not in use.
+	 */
+	usb_dpll = clk_get_sys(NULL, "dpll_usb_ck");
+	rc = clk_set_rate(usb_dpll, OMAP4_DPLL_USB_DEFFREQ);
+	if (rc)
+		pr_err("%s: failed to configure USB DPLL!\n", __func__);
+
+	return 0;
+}

Maybe try to have a generic init function, then have SoC specific quirk function pointers set up based on the DT compatible property?

Grep for varioius _of_match[] examples in the drivers.

That way you might be able to make clocks work for some new similar hardware with just .dts change and patching in the quirks later on as needed ;)

Regards,

Tony