ARM: TI SoC clock DT conversion

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

include/linux/clk-provider.h +8 -3

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
 {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
 }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 4 lines

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

[...]

Quoted 23 lines

include/linux/clk-provider.h +13 -4

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
  {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
  }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
  {
-	writel(val, reg);
+	if (regmap)
+		regmap_write(regmap, (u32)reg, val);
+	else
+		writel(val, reg);
  }

Quoted 2 lines

  #endif /* CONFIG_COMMON_CLK */

Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap)
and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont.

regmap can also return error value that could also be handled as a result.

Quoted 31 lines · Nishanth Menon

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 4 lines

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

[...]

Quoted 23 lines

include/linux/clk-provider.h +13 -4

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
  {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
  }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
  {
-	writel(val, reg);
+	if (regmap)
+		regmap_write(regmap, (u32)reg, val);
+	else
+		writel(val, reg);
  }

Quoted 8 lines

Quoted 2 lines

  #endif /* CONFIG_COMMON_CLK */

Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap)
and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont.

Well, doing that will introduce a lot of redundant code, as the checks for the presence of regmap must be copied all over the place. With this patch, all the generic clock drivers support internally both regmap or non-regmap register accesses.

Quoted 1 line

regmap can also return error value that could also be handled as a result.

True, however if the regmap fails for the clock code, not sure what we can do but panic. If this code is expanded, it is probably better to not inline it anymore.

Quoted 41 lines · Tero Kristo

Quoted 31 lines · Nishanth Menon

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 4 lines

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

[...]

Quoted 23 lines

include/linux/clk-provider.h +13 -4

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
   {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
   }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
   {
-	writel(val, reg);
+	if (regmap)
+		regmap_write(regmap, (u32)reg, val);
+	else
+		writel(val, reg);
   }

Quoted 8 lines

Quoted 2 lines

   #endif /* CONFIG_COMMON_CLK */

Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap)
and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont.

Quoted 5 lines

Well, doing that will introduce a lot of redundant code, as the checks for the presence of regmap must be copied all over the place. With this patch, all the generic clock drivers support internally both regmap or non-regmap register accesses.

using function pointers might be an appropriate solution. in general a low level reg access api that uses two different approaches sounds a little.. umm.. fishy..

Quoted 7 lines

Quoted 2 lines

regmap can also return error value that could also be handled as a result.

True, however if the regmap fails for the clock code, not sure what we can do but panic. If this code is expanded, it is probably better to not inline it anymore.

let the compiler deal with that decision. regmap operation fail should be percollated back to initiator of the request. in some cases that will be ir-recoverable, but on other cases panic might be the right answer - at the low level we are in no position to make that distinction.

Quoted 31 lines · Nishanth Menon

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 4 lines

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

[...]

Quoted 23 lines

include/linux/clk-provider.h +13 -4

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
 {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
 }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
 {
-	writel(val, reg);
+	if (regmap)
+		regmap_write(regmap, (u32)reg, val);
+	else
+		writel(val, reg);
 }

Quoted 8 lines

Quoted 2 lines

#endif /* CONFIG_COMMON_CLK */

Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap)
and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont.

Well, doing that will introduce a lot of redundant code, as the checks for the presence of regmap must be copied all over the place. With this patch, all the generic clock drivers support internally both regmap or non-regmap register accesses.

Quoted 42 lines · Tero Kristo

Quoted 31 lines · Nishanth Menon

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 4 lines

include/linux/clk-provider.h +0 -0

index 7e59253..63ff78c 100644

[...]

Quoted 23 lines

include/linux/clk-provider.h +13 -4

-static inline u32 clk_readl(u32 __iomem *reg)
+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
  {
-	return readl(reg);
+	u32 val;
+
+	if (regmap)
+		regmap_read(regmap, (u32)reg, &val);
+	else
+		val = readl(reg);
+	return val;
  }
 
-static inline void clk_writel(u32 val, u32 __iomem *reg)
+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
  {
-	writel(val, reg);
+	if (regmap)
+		regmap_write(regmap, (u32)reg, val);
+	else
+		writel(val, reg);
  }

Quoted 8 lines

Quoted 2 lines

  #endif /* CONFIG_COMMON_CLK */

Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap)
and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont.

Quoted 4 lines

Well, doing that will introduce a lot of redundant code, as the checks for the presence of regmap must be copied all over the place. With this patch, all the generic clock drivers support internally both regmap or non-regmap register accesses.

Please keep in mind that the "identity" of clk_readl() and readl() only applies in the current source code (ARM only use of common CCF primitives), while patches are pending (currently under review and receiving further improvement) which introduce several alternative implementations of clk_readl() depending on the platform. Making all of them duplicate the "regmap vs direct register access" branch would be as bad. Keeping one set of clk_readl() and clk_writel() routines and adding #ifdefs around the direct register access would be rather ugly, and I understand that preprocessor ifdef counts should get reduced instead of introduced.

virtually yours
Gerhard Sittig

Previously, only direct register read/write was supported. Now a per-clock regmap can be provided for same purpose, which allows the clock drivers to access clock registers behind e.g. I2C bus.

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

---
 drivers/clk/clk-divider.c    |    6 +++---
 drivers/clk/clk-gate.c       |    6 +++---
 drivers/clk/clk-mux.c        |    6 +++---
 include/linux/clk-provider.h |   23 +++++++++++++++++++----
 4 files changed, 28 insertions(+), 13 deletions(-)

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

new file mode 100644
index 0000000..7b87721
@@ -0,0 +1,81 @@
+Binding for Texas Instruments DPLL clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+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,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-x2-clock",
+		"ti,omap4-dpll-core-clock",
+		"ti,omap4-dpll-m4xen-clock",
+		"ti,omap4-dpll-j-type-clock",
+		"ti,am3-dpll-no-gate-clock",
+		"ti,am3-dpll-j-type-clock",
+		"ti,am3-dpll-no-gate-j-type-clock",
+		"ti,am3-dpll-clock",
+		"ti,am3-dpll-core-clock",
+		"ti,am3-dpll-x2-clock",

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

+  ti,am3-* dpll types list the registers in the same order, except "autoidle"
+  register is left out as this hardware does not have it, e.g.:
+	reg = <0x40>, <0x50>, <0x60>;
+  results in following register map:
+	base + 0x40 - control
+	base + 0x50 - idlest
+	base + 0x60 - mult-div1
+
+Optional properties:
+- DPLL mode setting - defining any one or more of the following overrides
+  default setting.
+	- ti,low-power-stop : DPLL supports low power stop mode, gating output
+	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
+	- ti,lock : DPLL locks in programmed rate

arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = ((1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED) |
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),

arch/arm/mach-omap2/dpll3xxx.c: _omap3_dpll_write_clken(clk, DPLL_LOCKED);

drivers/clk/ti/dpll.c new file +0 -0

new file mode 100644
index 0000000..89733c9

drivers/clk/ti/dpll.c new file +14 -0

+/**
+ * ti_clk_register_dpll() - Registers the DPLL clock
+ * @name:	Name of the clock node
+ * @parent_names: list of parent names
+ * @num_parents: num of parents in parent_names
+ * @flags:	init flags
+ * @dpll_data:	DPLL data
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll(const char *name,
+					const char **parent_names,
+					int num_parents, unsigned long flags,
+					struct dpll_data *dpll_data,
+					const struct clk_ops *ops,

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

+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->dpll_data = dpll_data;
+	clk_hw->ops = &clkhwops_omap3_dpll;
+	clk_hw->hw.init = &init;
+	clk_hw->regmap = regmap;
+
+	init.name = name;
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed clk_register for %s (%ld)\n", __func__, name,
+		       PTR_ERR(clk));
+		kfree(clk_hw);
+	} else {
+		omap2_init_clk_hw_omap_clocks(clk);
+	}
+
+	return clk;
+}
+
+#if defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_SOC_OMAP5) || \
+	defined(CONFIG_SOC_DRA7XX) || defined(CONFIG_SOC_AM33XX)
+/**
+ * ti_clk_register_dpll_x2() -  Registers the DPLLx2 clock
+ * @dev:	device pointer (if any)
+ * @name:	Name of the clock node
+ * @parent_name: parent name (only 1 parent)
+ * @reg:	register address for DPLL
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll_x2(struct device_node *node,
+					   const struct clk_ops *ops,
+					   const struct clk_hw_omap_ops *hw_ops,
+					   struct regmap *regmap)
+{
+	struct clk *clk;

drivers/clk/ti/dpll.c new file +24 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->ops = hw_ops;
+	of_property_read_u32(node, "reg", (u32 *)&clk_hw->clksel_reg);
+	clk_hw->regmap = regmap;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk))
+		kfree(clk_hw);
+	else
+		omap2_init_clk_hw_omap_clocks(clk);
+
+	return clk;
+}
+#endif

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 43 lines

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

new file mode 100644
index 0000000..7b87721
@@ -0,0 +1,81 @@
+Binding for Texas Instruments DPLL clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+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,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-x2-clock",
+		"ti,omap4-dpll-core-clock",
+		"ti,omap4-dpll-m4xen-clock",
+		"ti,omap4-dpll-j-type-clock",
+		"ti,am3-dpll-no-gate-clock",
+		"ti,am3-dpll-j-type-clock",
+		"ti,am3-dpll-no-gate-j-type-clock",
+		"ti,am3-dpll-clock",
+		"ti,am3-dpll-core-clock",
+		"ti,am3-dpll-x2-clock",
+
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks, first entry lists reference clock
+  and second entry bypass clock
+- reg : offsets for the register set for controlling the DPLL.

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

Quoted 5 lines

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

+  Registers are listed in following order:
+	"control" - contains the control register base address
+	"idlest" - contains the idle status register base address
+	"autoidle" - contains the autoidle register base address
+	"mult-div1" - contains the multiplier / divider register base address

If we move mult-div1 above autoidle, indices for control, idlest, mult-div1 will be constant with DPLLs that dont have autoidle. a little easier to debug.

Quoted 16 lines

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

+  ti,am3-* dpll types list the registers in the same order, except "autoidle"
+  register is left out as this hardware does not have it, e.g.:
+	reg = <0x40>, <0x50>, <0x60>;
+  results in following register map:
+	base + 0x40 - control
+	base + 0x50 - idlest
+	base + 0x60 - mult-div1
+
+Optional properties:
+- DPLL mode setting - defining any one or more of the following overrides
+  default setting.
+	- ti,low-power-stop : DPLL supports low power stop mode, gating output
+	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
+	- ti,lock : DPLL locks in programmed rate

Quoted 6 lines

arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = ((1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED) |
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),

arch/arm/mach-omap2/dpll3xxx.c: _omap3_dpll_write_clken(clk, DPLL_LOCKED);

There is no if checks for DPLL_LOCKED which is set with ti,lock, should we just drop it?

[...]

Quoted 5 lines

drivers/clk/ti/dpll.c new file +0 -0

new file mode 100644
index 0000000..89733c9

[...]

Quoted 19 lines

drivers/clk/ti/dpll.c new file +19 -0

+/**
+ * ti_clk_register_dpll() - Registers the DPLL clock
+ * @name:	Name of the clock node
+ * @parent_names: list of parent names
+ * @num_parents: num of parents in parent_names
+ * @flags:	init flags
+ * @dpll_data:	DPLL data
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll(const char *name,
+					const char **parent_names,
+					int num_parents, unsigned long flags,
+					struct dpll_data *dpll_data,
+					const struct clk_ops *ops,
+					struct regmap *regmap)
+{
+	struct clk *clk;
+	struct clk_init_data init = { NULL };
+	struct clk_hw_omap *clk_hw;

drivers/clk/ti/dpll.c line 115

Quoted 5 lines

drivers/clk/ti/dpll.c new file +5 -0

+
+	/* allocate the divider */
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);

here and in every other driver - please dont add pr_err for kzalloc not able to allocate, kzalloc will provide the warning anyways.

Quoted 49 lines

drivers/clk/ti/dpll.c new file +48 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->dpll_data = dpll_data;
+	clk_hw->ops = &clkhwops_omap3_dpll;
+	clk_hw->hw.init = &init;
+	clk_hw->regmap = regmap;
+
+	init.name = name;
+	init.ops = ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk)) {
+		pr_err("%s: failed clk_register for %s (%ld)\n", __func__, name,
+		       PTR_ERR(clk));
+		kfree(clk_hw);
+	} else {
+		omap2_init_clk_hw_omap_clocks(clk);
+	}
+
+	return clk;
+}
+
+#if defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_SOC_OMAP5) || \
+	defined(CONFIG_SOC_DRA7XX) || defined(CONFIG_SOC_AM33XX)
+/**
+ * ti_clk_register_dpll_x2() -  Registers the DPLLx2 clock
+ * @dev:	device pointer (if any)
+ * @name:	Name of the clock node
+ * @parent_name: parent name (only 1 parent)
+ * @reg:	register address for DPLL
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll_x2(struct device_node *node,
+					   const struct clk_ops *ops,
+					   const struct clk_hw_omap_ops *hw_ops,
+					   struct regmap *regmap)
+{
+	struct clk *clk;
+	struct clk_init_data init = { NULL };
+	struct clk_hw_omap *clk_hw;
+	const char *name = node->name;
+	const char *parent_name;

drivers/clk/ti/dpll.c line 169

Quoted 6 lines

drivers/clk/ti/dpll.c new file +6 -0

+
+	of_property_read_string(node, "clock-output-names", &name);
+
+	parent_name = of_clk_get_parent_name(node, 0);
+	if (!parent_name) {
+		pr_err("%s: dpll_x2 must have parent\n", __func__);

print node->name as well?

drivers/clk/ti/dpll.c line 175

Quoted 7 lines

drivers/clk/ti/dpll.c new file +6 -0

+		return ERR_PTR(-EINVAL);
+	}
+
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);

^^ here again.

Quoted 24 lines

drivers/clk/ti/dpll.c new file +24 -0

+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->ops = hw_ops;
+	of_property_read_u32(node, "reg", (u32 *)&clk_hw->clksel_reg);
+	clk_hw->regmap = regmap;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk))
+		kfree(clk_hw);
+	else
+		omap2_init_clk_hw_omap_clocks(clk);
+
+	return clk;
+}
+#endif

[...]

drivers/clk/ti/dpll.c

Quoted 2 lines

drivers/clk/ti/dpll.c new file +2 -0

+
+#ifdef CONFIG_ARCH_OMAP3

just a mention - I understand we are still in transition and we need these #ifdefs, but eventually, we should be able to make the dpll.c independent of SoC variant #ifdefs.

Quoted 8 lines · Nishanth Menon

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

Quoted 5 lines

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

new file mode 100644
index 0000000..7b87721

Quoted 23 lines

Quoted 22 lines

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

+  ti,am3-* dpll types list the registers in the same order, except "autoidle"
+  register is left out as this hardware does not have it, e.g.:
+	reg = <0x40>, <0x50>, <0x60>;
+  results in following register map:
+	base + 0x40 - control
+	base + 0x50 - idlest
+	base + 0x60 - mult-div1
+
+Optional properties:
+- DPLL mode setting - defining any one or more of the following overrides
+  default setting.
+	- ti,low-power-stop : DPLL supports low power stop mode, gating output
+	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
+	- ti,lock : DPLL locks in programmed rate

arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = ((1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED) |
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),

arch/arm/mach-omap2/dpll3xxx.c: _omap3_dpll_write_clken(clk, DPLL_LOCKED);

Quoted 2 lines

There is no if checks for DPLL_LOCKED which is set with ti,lock, should we just drop it?

Probably better to keep it in, as there might be some code in future which needs this. Kind of better for readability also, as the field is specified to list the valid modes for the DPLL. One could argue that current kernel code is wrong for _omap3_dpll_write_clken(clk, DPLL_LOCKED) as it does not check if the mode is valid for the DPLL or not.

Quoted 30 lines

[...]

Quoted 5 lines

drivers/clk/ti/dpll.c new file +0 -0

new file mode 100644
index 0000000..89733c9

[...]

Quoted 19 lines

drivers/clk/ti/dpll.c new file +19 -0

+/**
+ * ti_clk_register_dpll() - Registers the DPLL clock
+ * @name:	Name of the clock node
+ * @parent_names: list of parent names
+ * @num_parents: num of parents in parent_names
+ * @flags:	init flags
+ * @dpll_data:	DPLL data
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll(const char *name,
+					const char **parent_names,
+					int num_parents, unsigned long flags,
+					struct dpll_data *dpll_data,
+					const struct clk_ops *ops,
+					struct regmap *regmap)
+{
+	struct clk *clk;
+	struct clk_init_data init = { NULL };
+	struct clk_hw_omap *clk_hw;

Quoted 8 lines

drivers/clk/ti/dpll.c line 115

Quoted 5 lines

drivers/clk/ti/dpll.c new file +5 -0

+
+	/* allocate the divider */
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);

here and in every other driver - please dont add pr_err for kzalloc not able to allocate, kzalloc will provide the warning anyways.

Oh it does? Never seen that happen as kzalloc never failed for me. :) However, I can remove these as I don't think they will ever happen anyway.

Quoted 22 lines

Quoted 21 lines

drivers/clk/ti/dpll.c new file +21 -0

+	clk_hw->ops = hw_ops;
+	of_property_read_u32(node, "reg", (u32 *)&clk_hw->clksel_reg);
+	clk_hw->regmap = regmap;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk))
+		kfree(clk_hw);
+	else
+		omap2_init_clk_hw_omap_clocks(clk);
+
+	return clk;
+}
+#endif

[...]

Quoted 6 lines

drivers/clk/ti/dpll.c

Quoted 2 lines

drivers/clk/ti/dpll.c new file +2 -0

+
+#ifdef CONFIG_ARCH_OMAP3

just a mention - I understand we are still in transition and we need these #ifdefs, but eventually, we should be able to make the dpll.c independent of SoC variant #ifdefs.

Eventually yes. Currently it fails to compile if you do OMAP4 / OMAP5 only build, as you don't have the OMAP3 support routines in.

Some devices require their clocks to be available with a specific dev-id con-id mapping. With DT, the clocks can be found by default only with their name, or alternatively through the device node of the consumer. With drivers, that don't support DT fully yet, add mechanism to register specific clock names.

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

Tested-by Nishanth Menon <[email protected]>

Acked-by Tony Lindgren <[email protected]>

drivers/clk/ti/clk.c new file +11 -0

+void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
+{
+       struct ti_dt_clk *c;
+       struct device_node *node;
+       struct clk *clk;
+       struct of_phandle_args clkspec;
+
+       for (c = oclks; c->node_name != NULL; c++) {
+               node = of_find_node_by_name(NULL, c->node_name);
+               clkspec.np = node;
+               clk = of_clk_get_from_provider(&clkspec);

Quoted 9 lines · Tero Kristo

Some devices require their clocks to be available with a specific dev-id con-id mapping. With DT, the clocks can be found by default only with their name, or alternatively through the device node of the consumer. With drivers, that don't support DT fully yet, add mechanism to register specific clock names.

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

Tested-by Nishanth Menon <[email protected]>

Acked-by Tony Lindgren <[email protected]>

You guys are wonderful, by the way ;)

Quoted 11 lines

drivers/clk/ti/clk.c new file +11 -0

+void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
+{
+       struct ti_dt_clk *c;
+       struct device_node *node;
+       struct clk *clk;
+       struct of_phandle_args clkspec;
+
+       for (c = oclks; c->node_name != NULL; c++) {
+               node = of_find_node_by_name(NULL, c->node_name);
+               clkspec.np = node;
+               clk = of_clk_get_from_provider(&clkspec);

drivers/clk/ti/clk.c line 52

Quoted 10 lines

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

+
+               if (!IS_ERR(clk)) {
+                       c->lk.clk = clk;
+                       clkdev_add(&c->lk);
+               } else {
+                       pr_warn("%s: failed to lookup clock node %s\n",
+                               __func__, c->node_name);
+               }
+       }
+}

I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms.

I would say there is a good case for this being generic and used by all platforms waiting for con->id/dev->id to be moved to DT-awareness, the structure itself is very generic within clkdev as long as the drivers conformed anyway (and if they didn't, none of this helps) and I don't think it really needs to be a TI-only thing. The next thing that's going to happen when another platform arrives that wants to do the same thing is duplication or consolidation - so it would probably be a good idea to make this generic to start.

While there the names could be a bit more descriptive - dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map, DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this is the absolute correct thing to do under all circumstances, but in fact this is purely for mapping the old way to the new, better way.. in the event a brand new platform arrives, with all new drivers (or only compliant drivers), none of this code should be implemented for it and it should be presented this way.

Quoted 6 lines · Matt Sealey

I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms.

Quoted 1 line

I would say there is a good case for this being generic

An earlier implementation of this patch (or at least a patch that tries to accomplish the same as this does, add support for legacy devices) did try to add generic solution. There was some discussion on it and I decided to drop it due to bad feedback.

See: http://www.spinics.net/lists/linux-omap/msg95147.html

Quoted 7 lines · Tero Kristo

Quoted 6 lines · Matt Sealey

I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms.

Quoted 8 lines

Quoted 1 line

I would say there is a good case for this being generic

An earlier implementation of this patch (or at least a patch that tries to accomplish the same as this does, add support for legacy devices) did try to add generic solution. There was some discussion on it and I decided to drop it due to bad feedback.

See: http://www.spinics.net/lists/linux-omap/msg95147.html

I don't see anything that says this should not be generic, just that it was implemented relatively poorly at the time...

I agree with Tomasz, find_by_name in device trees is basically evil (it's only supposed to be used if you already know the name because something else passed it to you, and just want it's phandle, but names CAN be ambiguous. The real fix is always pass phandles and not strings of the name property), but that's not to say this thing needs to be TI-specific..

Ah well. If it ends up getting used elsewhere, it can be quickly patched into a generic version anyway, so.. yay!

I'm really interested in this all going in - but I would love to see where this is just all patched into a tree already. Pulling things out against a bunch of trees gave me more merge failures and weird happenings than I care to deal with. Is there a TI clock development tree with all this in somewhere publically accessible? I'd like to get a feel for how the DT looks and what's going on, I can't "see" it from the patches. Once it's there.. I'll spend a week or 5 doing all of the i.MX chips and you'll have a second test case for any problems :)

Quoted 9 lines · Tero Kristo

Some devices require their clocks to be available with a specific dev-id con-id mapping. With DT, the clocks can be found by default only with their name, or alternatively through the device node of the consumer. With drivers, that don't support DT fully yet, add mechanism to register specific clock names.

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

Tested-by Nishanth Menon <[email protected]>

Acked-by Tony Lindgren <[email protected]>

You guys are wonderful, by the way ;)

Quoted 19 lines

drivers/clk/ti/clk.c new file +17 -0

+void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
+{
+       struct ti_dt_clk *c;
+       struct device_node *node;
+       struct clk *clk;
+       struct of_phandle_args clkspec;
+
+       for (c = oclks; c->node_name != NULL; c++) {
+               node = of_find_node_by_name(NULL, c->node_name);
+               clkspec.np = node;
+               clk = of_clk_get_from_provider(&clkspec);
+
+               if (!IS_ERR(clk)) {
+                       c->lk.clk = clk;
+                       clkdev_add(&c->lk);
+               } else {
+                       pr_warn("%s: failed to lookup clock node

%s\n",
+ __func__, c->node_name);

drivers/clk/ti/clk.c line 52

Quoted 3 lines

drivers/clk/ti/clk.c new file +3 -0

+               }
+       }
+}

I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms.

I would say there is a good case for this being generic and used by all platforms waiting for con->id/dev->id to be moved to DT-awareness, the structure itself is very generic within clkdev as long as the drivers conformed anyway (and if they didn't, none of this helps) and I don't think it really needs to be a TI-only thing. The next thing that's going to happen when another platform arrives that wants to do the same thing is duplication or consolidation - so it would probably be a good idea to make this generic to start.

While there the names could be a bit more descriptive - dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map, DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this is the absolute correct thing to do under all circumstances, but in fact this is purely for mapping the old way to the new, better way.. in the event a brand new platform arrives, with all new drivers (or only compliant drivers), none of this code should be implemented for it and it should be presented this way.

Thanks,
Matt Sealey <[email protected]>

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Matt,

Quoted 4 lines

I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms.

SoC platforms that are fully converted to Device Tree do not need any legacy clkdev namings, because they have all clkdev look-ups performed using data from DT (see generic clock bindings).

So this would be not "*all* SoC platforms", but instead "SoC platforms, which are currently undergoing conversion to DT or conversion of which is yet to be started".

Still this might be a good enough justification for having a generic solution for this problem, but I would wait with this until the real need on some of such platforms shows up. I don't see anything wrong in implementing this first as TI-specific quirk and then possibly making it generic if such need shows up.

By the way, we already have something like this in clk/samsung/clk.c . This is called aliases there and allows registering clkdev look-ups for particular clocks. However this is specific to our clock drivers that register all clocks inside a single clock privder, based on static ID assignment and all clocks statically listed in clock driver - see clk/samsung/clk-s3c64xx.c for an example.

ti_dt_clk_init_provider() can now be used to initialize the contents of a single clock IP block. This parses all the clocks under the IP block and calls the corresponding init function for them.

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

---
 drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk/ti.h |    1 +
 2 files changed, 60 insertions(+)

drivers/clk/ti/clk.c +20 -0

index ad58b01..7f030d7 100644
@@ -19,6 +19,9 @@
 #include <linux/clkdev.h>
 #include <linux/clk/ti.h>
 #include <linux/of.h>
+#include <linux/list.h>
+
+extern struct of_device_id __clk_of_table[];
 
 /**
  * ti_dt_clocks_register - register DT duplicate clocks during boot

@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
 		}
 	}
 }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+	struct regmap *regmap;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+				    struct regmap *regmap)
+{
+	const struct of_device_id *match;
+	struct device_node *np;

drivers/clk/ti/clk.c +28 -0

+		ret = clk_init_cb(np, regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s: adding to again list...\n", np->name);
+			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
+			retry->np = np;
+			retry->regmap = regmap;
+			retry->init_cb = clk_init_cb;
+			list_add(&retry->node, &retry_list);
+		} else if (ret) {
+			pr_err("%s: clock init failed for %s (%d)!\n", __func__,
+			       np->name, ret);
+		}
+	}
+
+	list_for_each_entry_safe(retry, tmp, &retry_list, node) {
+		pr_debug("%s: retry-init: %s\n", __func__, retry->np->name);
+		ret = retry->init_cb(retry->np, retry->regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s failed again?\n", retry->np->name);
+		} else {
+			if (ret)
+				pr_err("%s: clock init failed for %s (%d)!\n",
+				       __func__, retry->np->name, ret);
+			list_del(&retry->node);
+			kfree(retry);
+		}
+	}
+}

include/linux/clk/ti.h +1 -0

index 9786752..7ab02fa 100644
@@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
 			 unsigned long parent_rate);
 
 void ti_dt_clocks_register(struct ti_dt_clk *oclks);
+void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);
 
 extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
 extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;

Quoted 49 lines · Tero Kristo

ti_dt_clk_init_provider() can now be used to initialize the contents of a single clock IP block. This parses all the clocks under the IP block and calls the corresponding init function for them.

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

---
  drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/clk/ti.h |    1 +
  2 files changed, 60 insertions(+)

drivers/clk/ti/clk.c +24 -0

index ad58b01..7f030d7 100644
@@ -19,6 +19,9 @@
  #include <linux/clkdev.h>
  #include <linux/clk/ti.h>
  #include <linux/of.h>
+#include <linux/list.h>
+
+extern struct of_device_id __clk_of_table[];
 
  /**
   * ti_dt_clocks_register - register DT duplicate clocks during boot

@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
  		}
  	}
  }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+	struct regmap *regmap;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+				    struct regmap *regmap)
+{
+	const struct of_device_id *match;
+	struct device_node *np;
+	ti_of_clk_init_cb_t clk_init_cb;
+	struct clk_init_item *retry;
+	struct clk_init_item *tmp;
+	int ret;

drivers/clk/ti/clk.c void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) · line 82

Quoted 6 lines

drivers/clk/ti/clk.c +6 -0

+
+	for_each_child_of_node(parent, np) {
+		match = of_match_node(__clk_of_table, np);
+		if (!match)
+			continue;
+		clk_init_cb = match->data;

I must admit I am confused here.

a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match data. The prototype of the generic of_clk_init_cb_t is typedef void (*of_clk_init_cb_t)(struct device_node *);
b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes from __clk_of_table

I assume we need ti_dt_clk_init_provider to be always called with clock list, as a result of_clk_init(NULL); will never be invoked. This is counter productive as you have have generic non SoC clock providers as well who would have been invoked with of_clk_init(NULL);

I do strongly encourage using of_clk_init(NULL) and not having to switch the clk_init call back with SoC specific one as it forces un-intended dependency.

Further such as SoC specific callback should have been in a header.

drivers/clk/ti/clk.c void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) · line 83

Quoted 1 line

drivers/clk/ti/clk.c +1 -0

+		pr_debug("%s: initializing: %s\n", __func__, np->name);

one further comment - using pr_fmt will save us from using __func__ for every pr_* message :(

Quoted 42 lines

drivers/clk/ti/clk.c +28 -0

+		ret = clk_init_cb(np, regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s: adding to again list...\n", np->name);
+			retry = kzalloc(sizeof(*retry), GFP_KERNEL);
+			retry->np = np;
+			retry->regmap = regmap;
+			retry->init_cb = clk_init_cb;
+			list_add(&retry->node, &retry_list);
+		} else if (ret) {
+			pr_err("%s: clock init failed for %s (%d)!\n", __func__,
+			       np->name, ret);
+		}
+	}
+
+	list_for_each_entry_safe(retry, tmp, &retry_list, node) {
+		pr_debug("%s: retry-init: %s\n", __func__, retry->np->name);
+		ret = retry->init_cb(retry->np, retry->regmap);
+		if (ret == -EAGAIN) {
+			pr_debug("%s failed again?\n", retry->np->name);
+		} else {
+			if (ret)
+				pr_err("%s: clock init failed for %s (%d)!\n",
+				       __func__, retry->np->name, ret);
+			list_del(&retry->node);
+			kfree(retry);
+		}
+	}
+}

include/linux/clk/ti.h +1 -0

index 9786752..7ab02fa 100644
@@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
  			 unsigned long parent_rate);
 
  void ti_dt_clocks_register(struct ti_dt_clk *oclks);
+void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);
 
  extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
  extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;

Quoted 50 lines · Nishanth Menon

Quoted 49 lines · Tero Kristo

ti_dt_clk_init_provider() can now be used to initialize the contents of a single clock IP block. This parses all the clocks under the IP block and calls the corresponding init function for them.

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

---
  drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/clk/ti.h |    1 +
  2 files changed, 60 insertions(+)

drivers/clk/ti/clk.c +24 -0

index ad58b01..7f030d7 100644
@@ -19,6 +19,9 @@
  #include <linux/clkdev.h>
  #include <linux/clk/ti.h>
  #include <linux/of.h>
+#include <linux/list.h>
+
+extern struct of_device_id __clk_of_table[];
 
  /**
   * ti_dt_clocks_register - register DT duplicate clocks during boot

@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
  		}
  	}
  }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+	struct regmap *regmap;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+				    struct regmap *regmap)
+{
+	const struct of_device_id *match;
+	struct device_node *np;
+	ti_of_clk_init_cb_t clk_init_cb;
+	struct clk_init_item *retry;
+	struct clk_init_item *tmp;
+	int ret;

Quoted 8 lines

drivers/clk/ti/clk.c void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) · line 82

Quoted 6 lines

drivers/clk/ti/clk.c +6 -0

+
+	for_each_child_of_node(parent, np) {
+		match = of_match_node(__clk_of_table, np);
+		if (!match)
+			continue;
+		clk_init_cb = match->data;

I must admit I am confused here.

Yea this patch is something I am not quite comfortable myself yet and would like improvement ideas....

Quoted 5 lines

a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match data. The prototype of the generic of_clk_init_cb_t is typedef void (*of_clk_init_cb_t)(struct device_node *);
b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes from __clk_of_table

__clk_of_table contains the function pointers for the clock init functions, not clock nodes.

Quoted 6 lines

I assume we need ti_dt_clk_init_provider to be always called with clock list, as a result of_clk_init(NULL); will never be invoked. This is counter productive as you have have generic non SoC clock providers as well who would have been invoked with of_clk_init(NULL);

Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock nodes under respective IP blocks. Two reasons here:

a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to the clock init functions, basically the pointer to the memory map region (regmap.)
b) of_clk_init(NULL) will initialize all the clock nodes in the system, irrespective of the hierarchy considerations.

Only thing that can be done, is to make the API introduced in this patch a generic API and call it something like of_clk_init_children().

Quoted 63 lines · Tero Kristo

Quoted 50 lines · Nishanth Menon

Quoted 49 lines · Tero Kristo

ti_dt_clk_init_provider() can now be used to initialize the contents of a single clock IP block. This parses all the clocks under the IP block and calls the corresponding init function for them.

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

---
   drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
   include/linux/clk/ti.h |    1 +
   2 files changed, 60 insertions(+)

drivers/clk/ti/clk.c +24 -0

index ad58b01..7f030d7 100644
@@ -19,6 +19,9 @@
   #include <linux/clkdev.h>
   #include <linux/clk/ti.h>
   #include <linux/of.h>
+#include <linux/list.h>
+
+extern struct of_device_id __clk_of_table[];
 
   /**
    * ti_dt_clocks_register - register DT duplicate clocks during boot

@@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
   		}
   	}
   }
+
+typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
+
+struct clk_init_item {
+	struct regmap *regmap;
+	struct device_node *np;
+	ti_of_clk_init_cb_t init_cb;
+	struct list_head node;
+};
+
+static LIST_HEAD(retry_list);
+
+void __init ti_dt_clk_init_provider(struct device_node *parent,
+				    struct regmap *regmap)
+{
+	const struct of_device_id *match;
+	struct device_node *np;
+	ti_of_clk_init_cb_t clk_init_cb;
+	struct clk_init_item *retry;
+	struct clk_init_item *tmp;
+	int ret;

Quoted 8 lines

drivers/clk/ti/clk.c void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) · line 82

Quoted 6 lines

drivers/clk/ti/clk.c +6 -0

+
+	for_each_child_of_node(parent, np) {
+		match = of_match_node(__clk_of_table, np);
+		if (!match)
+			continue;
+		clk_init_cb = match->data;

I must admit I am confused here.

Yea this patch is something I am not quite comfortable myself yet and would like improvement ideas....

Quoted 8 lines

Quoted 5 lines

a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match data. The prototype of the generic of_clk_init_cb_t is typedef void (*of_clk_init_cb_t)(struct device_node *);
b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes from __clk_of_table

__clk_of_table contains the function pointers for the clock init functions, not clock nodes.

yes, apologies, should have stated the prototypes of functions in a single __clk_of_table should not be two different argument list. The reason is as follows: CLK_OF_DECLARE fills up that list. there can be generic drivers such as [1] which might be registered, OR in the case of MULTI_ARCH - multiple other SoC drivers would have registered there. There is a bunch of stuff we could mess up here.

If we do not have a choice, if we would like to maintain a different prototype based init list, we should probably do it in a different structure - example __clk_of_table_regmap or something similar..

Quoted 10 lines

Quoted 6 lines

I assume we need ti_dt_clk_init_provider to be always called with clock list, as a result of_clk_init(NULL); will never be invoked. This is counter productive as you have have generic non SoC clock providers as well who would have been invoked with of_clk_init(NULL);

Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock nodes under respective IP blocks. Two reasons here:

Quoted 8 lines

a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to the clock init functions, basically the pointer to the memory map region (regmap.)
b) of_clk_init(NULL) will initialize all the clock nodes in the system, irrespective of the hierarchy considerations.

Only thing that can be done, is to make the API introduced in this patch a generic API and call it something like of_clk_init_children().

you still can do that:

of_prcm_init->ti_dt_clk_init_provider-> instead of matching nodes from __clk_of_table, you can make it __clk_of_regmap_table -> the CLK_OF_DECLARE will not be good enough here ofcourse, but this will probably belong to a generic regmap enabled clock support framework - i dont see much that is TI specific in drivers/clk/ti/clk.c and it makes sense to be part of generic clock framework handling as generic clock framework will receive regmap support based on previous patches. that allows for other platforms to use regmap based clk drivers as well.

[1] http://marc.info/?l=linux-omap&m=138331451210184&w=2

TI clk driver now routes some of the basic clocks through own registration routine to allow autoidle support. This routine just checks a couple of device node properties and adds autoidle support if required, and just passes the registration forward to basic clocks.

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

---
 arch/arm/mach-omap2/clock.c |    6 +++
 drivers/clk/ti/Makefile     |    2 +-
 drivers/clk/ti/autoidle.c   |  109 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk/ti.h      |    9 ++++
 4 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/ti/autoidle.c

arch/arm/mach-omap2/clock.c +6 -0

index 0c38ca9..223f432b 100644
@@ -520,6 +520,9 @@ int omap2_clk_enable_autoidle_all(void)
 	list_for_each_entry(c, &clk_hw_omap_clocks, node)
 		if (c->ops && c->ops->allow_idle)
 			c->ops->allow_idle(c);
+
+	of_ti_clk_allow_autoidle_all();
+
 	return 0;
 }
 

@@ -539,6 +542,9 @@ int omap2_clk_disable_autoidle_all(void)
 	list_for_each_entry(c, &clk_hw_omap_clocks, node)
 		if (c->ops && c->ops->deny_idle)
 			c->ops->deny_idle(c);
+
+	of_ti_clk_deny_autoidle_all();
+
 	return 0;
 }
 

drivers/clk/ti/Makefile +1 -1

index 05af5d8..533efb4 100644
@@ -1,3 +1,3 @@
 ifneq ($(CONFIG_OF),)
-obj-y					+= clk.o dpll.o
+obj-y					+= clk.o dpll.o autoidle.o
 endif

drivers/clk/ti/autoidle.c new file +39 -0

new file mode 100644
index 0000000..efa2a3e
@@ -0,0 +1,109 @@
+/*
+ * TI clock autoidle support
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * Tero Kristo <[email protected]>
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk/ti.h>
+
+struct clk_ti_autoidle {
+	u32			reg;
+	struct regmap		*regmap;
+	u8			shift;
+	u8			flags;
+	const char		*name;
+	struct list_head	node;
+};
+
+#define AUTOIDLE_LOW		0x1
+
+static LIST_HEAD(autoidle_clks);
+
+static void ti_allow_autoidle(struct clk_ti_autoidle *clk)
+{

drivers/clk/ti/autoidle.c new file +4 -0

+}
+
+static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
+{

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

+}
+
+void of_ti_clk_allow_autoidle_all(void)
+{
+	struct clk_ti_autoidle *c;
+
+	list_for_each_entry(c, &autoidle_clks, node)
+		ti_allow_autoidle(c);
+}
+

drivers/clk/ti/autoidle.c new file +7 -0

+{
+	u32 shift;
+	struct clk_ti_autoidle *clk;
+
+	if (of_property_read_u32(node, "ti,autoidle-shift", &shift))
+		return 0;
+

drivers/clk/ti/autoidle.c new file +9 -0

+	clk->regmap = regmap;
+
+	if (of_property_read_bool(node, "ti,invert-autoidle-bit"))
+		clk->flags |= AUTOIDLE_LOW;
+
+	list_add(&clk->node, &autoidle_clks);
+
+	return 0;
+}

include/linux/clk/ti.h +9 -0

index 7ab02fa..e45005c 100644
@@ -178,6 +178,15 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
 
 void ti_dt_clocks_register(struct ti_dt_clk *oclks);
 void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);
+int of_ti_autoidle_setup(struct device_node *node, struct regmap *regmap);
+
+#ifdef CONFIG_OF
+void of_ti_clk_allow_autoidle_all(void);
+void of_ti_clk_deny_autoidle_all(void);
+#else
+static inline void of_ti_clk_allow_autoidle_all(void) { }
+static inline void of_ti_clk_deny_autoidle_all(void) { }
+#endif
 
 extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
 extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;

Quoted 95 lines · Tero Kristo

TI clk driver now routes some of the basic clocks through own registration routine to allow autoidle support. This routine just checks a couple of device node properties and adds autoidle support if required, and just passes the registration forward to basic clocks.

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

---
  arch/arm/mach-omap2/clock.c |    6 +++
  drivers/clk/ti/Makefile     |    2 +-
  drivers/clk/ti/autoidle.c   |  109 +++++++++++++++++++++++++++++++++++++++++++
  include/linux/clk/ti.h      |    9 ++++
  4 files changed, 125 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/ti/autoidle.c

arch/arm/mach-omap2/clock.c +6 -0

index 0c38ca9..223f432b 100644
@@ -520,6 +520,9 @@ int omap2_clk_enable_autoidle_all(void)
  	list_for_each_entry(c, &clk_hw_omap_clocks, node)
  		if (c->ops && c->ops->allow_idle)
  			c->ops->allow_idle(c);
+
+	of_ti_clk_allow_autoidle_all();
+
  	return 0;
  }
 

@@ -539,6 +542,9 @@ int omap2_clk_disable_autoidle_all(void)
  	list_for_each_entry(c, &clk_hw_omap_clocks, node)
  		if (c->ops && c->ops->deny_idle)
  			c->ops->deny_idle(c);
+
+	of_ti_clk_deny_autoidle_all();
+
  	return 0;
  }
 

drivers/clk/ti/Makefile +1 -1

index 05af5d8..533efb4 100644
@@ -1,3 +1,3 @@
  ifneq ($(CONFIG_OF),)
-obj-y					+= clk.o dpll.o
+obj-y					+= clk.o dpll.o autoidle.o
  endif

drivers/clk/ti/autoidle.c new file +42 -0

new file mode 100644
index 0000000..efa2a3e
@@ -0,0 +1,109 @@
+/*
+ * TI clock autoidle support
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * Tero Kristo <[email protected]>
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk/ti.h>
+
+struct clk_ti_autoidle {
+	u32			reg;
+	struct regmap		*regmap;
+	u8			shift;
+	u8			flags;
+	const char		*name;
+	struct list_head	node;
+};
+
+#define AUTOIDLE_LOW		0x1
+
+static LIST_HEAD(autoidle_clks);
+
+static void ti_allow_autoidle(struct clk_ti_autoidle *clk)
+{
+	u32 val;
+
+	regmap_read(clk->regmap, clk->reg, &val);

drivers/clk/ti/autoidle.c line 49

Quoted 7 lines

drivers/clk/ti/autoidle.c new file +7 -0

+
+	if (clk->flags & AUTOIDLE_LOW)
+		val &= ~(1 << clk->shift);
+	else
+		val |= (1 << clk->shift);
+
+	regmap_write(clk->regmap, clk->reg, val);

regmap_update_bits ?

Quoted 7 lines

drivers/clk/ti/autoidle.c new file +7 -0

+}
+
+static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
+{
+	u32 val;
+
+	regmap_read(clk->regmap, clk->reg, &val);

drivers/clk/ti/autoidle.c line 63

Quoted 7 lines

drivers/clk/ti/autoidle.c new file +7 -0

+
+	if (clk->flags & AUTOIDLE_LOW)
+		val |= (1 << clk->shift);
+	else
+		val &= ~(1 << clk->shift);
+
+	regmap_write(clk->regmap, clk->reg, val);

regmap_update_bits ?

and ofcourse error handling for regmap ops..

Quoted 16 lines

drivers/clk/ti/autoidle.c new file +15 -0

+}
+
+void of_ti_clk_allow_autoidle_all(void)
+{
+	struct clk_ti_autoidle *c;
+
+	list_for_each_entry(c, &autoidle_clks, node)
+		ti_allow_autoidle(c);
+}
+
+void of_ti_clk_deny_autoidle_all(void)
+{
+	struct clk_ti_autoidle *c;
+
+	list_for_each_entry(c, &autoidle_clks, node)

drivers/clk/ti/autoidle.c line 83

Quoted 5 lines

drivers/clk/ti/autoidle.c new file +5 -0

+		ti_deny_autoidle(c);
+}
+
+int __init of_ti_autoidle_setup(struct device_node *node,
+				struct regmap *regmap)

will be nice if you could kernel-doc all public functions at the very least.

Quoted 13 lines

drivers/clk/ti/autoidle.c new file +12 -0

+{
+	u32 shift;
+	struct clk_ti_autoidle *clk;
+
+	if (of_property_read_u32(node, "ti,autoidle-shift", &shift))
+		return 0;
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+
+	if (!clk) {
+		pr_err("%s: kzalloc failed\n", __func__);
+		return -ENOMEM;

drivers/clk/ti/autoidle.c line 100

Quoted 5 lines

drivers/clk/ti/autoidle.c new file +5 -0

+	}
+
+	clk->shift = shift;
+	clk->name = node->name;
+	of_property_read_u32(node, "reg", &clk->reg);

dt binding?
what happens if someone forgets this?

Quoted 31 lines

drivers/clk/ti/autoidle.c new file +9 -0

+	clk->regmap = regmap;
+
+	if (of_property_read_bool(node, "ti,invert-autoidle-bit"))
+		clk->flags |= AUTOIDLE_LOW;
+
+	list_add(&clk->node, &autoidle_clks);
+
+	return 0;
+}

include/linux/clk/ti.h +9 -0

index 7ab02fa..e45005c 100644
@@ -178,6 +178,15 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
 
  void ti_dt_clocks_register(struct ti_dt_clk *oclks);
  void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);
+int of_ti_autoidle_setup(struct device_node *node, struct regmap *regmap);
+
+#ifdef CONFIG_OF
+void of_ti_clk_allow_autoidle_all(void);
+void of_ti_clk_deny_autoidle_all(void);
+#else
+static inline void of_ti_clk_allow_autoidle_all(void) { }
+static inline void of_ti_clk_deny_autoidle_all(void) { }
+#endif
 
  extern const struct clk_hw_omap_ops clkhwops_omap3_dpll;
  extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;

Quoted 1 line · Nishanth Menon

On 10/25/2013 10:57 AM, Tero Kristo wrote:

Quoted 5 lines

Quoted 5 lines

drivers/clk/ti/autoidle.c new file +0 -0

new file mode 100644
index 0000000..efa2a3e

Quoted 7 lines

Quoted 7 lines

drivers/clk/ti/autoidle.c new file +7 -0

+}
+
+static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
+{
+	u32 val;
+
+	regmap_read(clk->regmap, clk->reg, &val);

Quoted 8 lines

drivers/clk/ti/autoidle.c line 63

Quoted 7 lines

drivers/clk/ti/autoidle.c new file +7 -0

+
+	if (clk->flags & AUTOIDLE_LOW)
+		val |= (1 << clk->shift);
+	else
+		val &= ~(1 << clk->shift);
+
+	regmap_write(clk->regmap, clk->reg, val);

regmap_update_bits ?

Same.

Quoted 2 lines

and ofcourse error handling for regmap ops..

What do you propose to do in error case? Panic, WARN() or just printk?

Quoted 2 lines · Tero Kristo

Quoted 1 line · Nishanth Menon

On 10/25/2013 10:57 AM, Tero Kristo wrote:

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/clk/ti/autoidle.c new file +0 -0

new file mode 100644
index 0000000..efa2a3e

[...]

Quoted 16 lines

Quoted 15 lines

Quoted 4 lines

drivers/clk/ti/autoidle.c new file +4 -0

+}
+
+static void ti_deny_autoidle(struct clk_ti_autoidle *clk)
+{

drivers/clk/ti/autoidle.c line 63

Quoted 10 lines

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

+	u32 val;
+
+	regmap_read(clk->regmap, clk->reg, &val);
+
+	if (clk->flags & AUTOIDLE_LOW)
+		val |= (1 << clk->shift);
+	else
+		val &= ~(1 << clk->shift);
+
+	regmap_write(clk->regmap, clk->reg, val);

regmap_update_bits ?

Quoted 6 lines

Same.

Quoted 2 lines

and ofcourse error handling for regmap ops..

What do you propose to do in error case? Panic, WARN() or just printk?

return error to caller and percolate back up the call stack.

one other thing I missed, will be nice to introduce a common bindings for autoidle which tends to be reused in other drivers..

Quoted 1 line · Nishanth Menon

On 11/01/2013 04:18 AM, Tero Kristo wrote:

Quoted 2 lines

one other thing I missed, will be nice to introduce a common bindings for autoidle which tends to be reused in other drivers..

You mean documentation or? Autoidle from this patch is only used by two clock drivers (divider + fixed-factor.) DPLLs use completely different setup. Converting the interface clocks to use the same might be rather nasty thing to do.

Quoted 2 lines · Tero Kristo

Quoted 1 line · Nishanth Menon

On 11/01/2013 04:18 AM, Tero Kristo wrote:

[...]

Quoted 3 lines

Quoted 2 lines

one other thing I missed, will be nice to introduce a common bindings for autoidle which tends to be reused in other drivers..

Quoted 4 lines

You mean documentation or? Autoidle from this patch is only used by two clock drivers (divider + fixed-factor.) DPLLs use completely different setup. Converting the interface clocks to use the same might be rather nasty thing to do.

The definitions are common - so, yes, it will be nice to have autoidle.txt which is then pointed to by divider, fixed-clock etc..

This is a multipurpose clock node, which contains support for multiple sub-clocks. Uses basic composite clock type to implement the actual functionality, and TI specific gate, mux and divider clocks.

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

---
 .../devicetree/bindings/clock/ti/composite.txt     |   54 +++++
 drivers/clk/ti/Makefile                            |    2 +-
 drivers/clk/ti/composite.c                         |  222 ++++++++++++++++++++
 include/linux/clk/ti.h                             |    8 +
 4 files changed, 285 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/composite.txt
 create mode 100644 drivers/clk/ti/composite.c

Documentation/devicetree/bindings/clock/ti/composite.txt new file +54 -0

new file mode 100644
index 0000000..5f43c47
@@ -0,0 +1,54 @@
+Binding for TI composite clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped composite clock with multiple different sub-types;
+
+a multiplexer clock with multiple input clock signals or parents, one
+of which can be selected as output, this behaves exactly as [2]
+
+an adjustable clock rate divider, this behaves exactly as [3]
+
+a gating function which can be used to enable and disable the output
+clock, this behaves exactly as [4]
+
+The binding must provide a list of the component clocks that shall be
+merged to this clock. The component clocks shall be of one of the
+"ti,*composite*-clock" types.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/ti/mux.txt
+[3] Documentation/devicetree/bindings/clock/ti/divider.txt
+[4] Documentation/devicetree/bindings/clock/ti/gate.txt
+
+Required properties:
+- compatible : shall be: "ti,composite-clock"
+- clocks : link phandles of component clocks
+- #clock-cells : from common clock binding; shall be set to 0.
+
+Examples:
+
+usb_l4_gate_ick: usb_l4_gate_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-interface-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <5>;
+	reg = <0x0a10>;
+};
+
+usb_l4_div_ick: usb_l4_div_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-divider-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <4>;
+	ti,max-div = <1>;
+	reg = <0x0a40>;
+	ti,index-starts-at-one;
+};
+
+usb_l4_ick: usb_l4_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-clock";
+	clocks = <&usb_l4_gate_ick>, <&usb_l4_div_ick>;
+};

drivers/clk/ti/Makefile +1 -1

index 533efb4..a4a7595 100644
@@ -1,3 +1,3 @@
 ifneq ($(CONFIG_OF),)
-obj-y					+= clk.o dpll.o autoidle.o
+obj-y					+= clk.o dpll.o autoidle.o composite.o
 endif

drivers/clk/ti/composite.c new file +57 -0

new file mode 100644
index 0000000..9ce7e54
@@ -0,0 +1,222 @@
+/*
+ * TI composite clock support
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * Tero Kristo <[email protected]>
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk/ti.h>
+#include <linux/list.h>
+
+#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
+
+static unsigned long ti_composite_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	return clk_divider_ops.recalc_rate(hw, parent_rate);
+}
+
+static long ti_composite_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *prate)
+{
+	return -EINVAL;
+}
+
+static int ti_composite_set_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long parent_rate)
+{
+	return -EINVAL;
+}
+
+static const struct clk_ops ti_composite_divider_ops = {
+	.recalc_rate	= &ti_composite_recalc_rate,
+	.round_rate	= &ti_composite_round_rate,
+	.set_rate	= &ti_composite_set_rate,
+};
+
+static const struct clk_ops ti_composite_gate_ops = {
+	.enable		= &omap2_dflt_clk_enable,
+	.disable	= &omap2_dflt_clk_disable,
+	.is_enabled	= &omap2_dflt_clk_is_enabled,
+};
+

drivers/clk/ti/composite.c new file +29 -0

+	"mux", "gate", "divider",
+};
+
+static LIST_HEAD(component_clks);
+
+static struct component_clk *_lookup_component(struct device_node *node, int i)
+{
+	int rc;
+	struct of_phandle_args clkspec;
+	struct component_clk *comp;
+
+	rc = of_parse_phandle_with_args(node, "clocks", "#clock-cells", i,
+					&clkspec);
+	if (rc)
+		return NULL;
+
+	list_for_each_entry(comp, &component_clks, link) {
+		if (comp->node == clkspec.np)
+			return comp;
+	}
+	return NULL;
+}
+
+static inline struct clk_hw *_get_hw(struct component_clk *clk)
+{
+	if (clk)
+		return clk->hw;
+
+	return NULL;

drivers/clk/ti/composite.c new file +4 -0

+		if (!clks[i])
+			continue;
+		if (clks[i]->num_parents) {
+			num_parents = clks[i]->num_parents;

drivers/clk/ti/composite.c new file +14 -0

+	}
+
+	clk = clk_register_composite(NULL, name, parent_names, num_parents,
+				     _get_hw(clks[CLK_COMPONENT_TYPE_MUX]),
+				     &clk_mux_ops,
+				     _get_hw(clks[CLK_COMPONENT_TYPE_DIVIDER]),
+				     &ti_composite_divider_ops,
+				     _get_hw(clks[CLK_COMPONENT_TYPE_GATE]),
+				     &ti_composite_gate_ops, 0);
+
+	if (!IS_ERR(clk)) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		goto cleanup;
+	}

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

+
+	return ret;
+}
+CLK_OF_DECLARE(ti_composite_clock, "ti,composite-clock",
+	       of_ti_composite_clk_setup);
+
+int __init ti_clk_add_component(struct device_node *node, struct clk_hw *hw,
+				int type)
+{
+	int num_parents;
+	const char **parent_names;
+	struct component_clk *clk;
+	int i;
+
+	num_parents = of_clk_get_parent_count(node);
+
+	if (num_parents < 1) {
+		pr_err("%s: component-clock %s must have parent(s)\n", __func__,
+		       node->name);
+		return -EINVAL;
+	}
+
+	parent_names = kzalloc((sizeof(char *) * num_parents), GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
+
+	for (i = 0; i < num_parents; i++)
+		parent_names[i] = of_clk_get_parent_name(node, i);
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk) {
+		kfree(parent_names);
+		return -ENOMEM;
+	}
+
+	clk->num_parents = num_parents;
+	clk->parent_names = parent_names;
+	clk->hw = hw;
+	clk->node = node;
+	clk->type = type;
+	list_add(&clk->link, &component_clks);
+
+	return 0;
+}

include/linux/clk/ti.h +0 -0

index e45005c..96c9725 100644

include/linux/clk/ti.h +3 -0

+};
+
 /**
  * struct ti_dt_clk - OMAP DT clock alias declarations
  * @lk: clock lookup definition

@@ -179,6 +186,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
 void ti_dt_clocks_register(struct ti_dt_clk *oclks);
 void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap);
 int of_ti_autoidle_setup(struct device_node *node, struct regmap *regmap);
+int ti_clk_add_component(struct device_node *node, struct clk_hw *hw, int type);
 
 #ifdef CONFIG_OF
 void of_ti_clk_allow_autoidle_all(void);

Quoted 68 lines · Tero Kristo

This is a multipurpose clock node, which contains support for multiple sub-clocks. Uses basic composite clock type to implement the actual functionality, and TI specific gate, mux and divider clocks.

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

---
 .../devicetree/bindings/clock/ti/composite.txt     |   54 +++++
 drivers/clk/ti/Makefile                            |    2 +-
 drivers/clk/ti/composite.c                         |  222 ++++++++++++++++++++
 include/linux/clk/ti.h                             |    8 +
 4 files changed, 285 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/composite.txt
 create mode 100644 drivers/clk/ti/composite.c

Documentation/devicetree/bindings/clock/ti/composite.txt new file +48 -0

new file mode 100644
index 0000000..5f43c47
@@ -0,0 +1,54 @@
+Binding for TI composite clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped composite clock with multiple different sub-types;
+
+a multiplexer clock with multiple input clock signals or parents, one
+of which can be selected as output, this behaves exactly as [2]
+
+an adjustable clock rate divider, this behaves exactly as [3]
+
+a gating function which can be used to enable and disable the output
+clock, this behaves exactly as [4]
+
+The binding must provide a list of the component clocks that shall be
+merged to this clock. The component clocks shall be of one of the
+"ti,*composite*-clock" types.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/ti/mux.txt
+[3] Documentation/devicetree/bindings/clock/ti/divider.txt
+[4] Documentation/devicetree/bindings/clock/ti/gate.txt
+
+Required properties:
+- compatible : shall be: "ti,composite-clock"
+- clocks : link phandles of component clocks
+- #clock-cells : from common clock binding; shall be set to 0.
+
+Examples:
+
+usb_l4_gate_ick: usb_l4_gate_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-interface-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <5>;
+	reg = <0x0a10>;
+};
+
+usb_l4_div_ick: usb_l4_div_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-divider-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <4>;
+	ti,max-div = <1>;
+	reg = <0x0a40>;
+	ti,index-starts-at-one;
+};

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

Quoted 6 lines

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

+
+usb_l4_ick: usb_l4_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-clock";
+	clocks = <&usb_l4_gate_ick>, <&usb_l4_div_ick>;
+};

From Topology perspective, would this be a better approach?

Quoted 67 lines · Nishanth Menon

Quoted 66 lines · Tero Kristo

This is a multipurpose clock node, which contains support for multiple sub-clocks. Uses basic composite clock type to implement the actual functionality, and TI specific gate, mux and divider clocks.

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

---
  .../devicetree/bindings/clock/ti/composite.txt     |   54 +++++
  drivers/clk/ti/Makefile                            |    2 +-
  drivers/clk/ti/composite.c                         |  222 ++++++++++++++++++++
  include/linux/clk/ti.h                             |    8 +
  4 files changed, 285 insertions(+), 1 deletion(-)

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

Documentation/devicetree/bindings/clock/ti/composite.txt new file +46 -0

new file mode 100644
index 0000000..5f43c47
@@ -0,0 +1,54 @@
+Binding for TI composite clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped composite clock with multiple different sub-types;
+
+a multiplexer clock with multiple input clock signals or parents, one
+of which can be selected as output, this behaves exactly as [2]
+
+an adjustable clock rate divider, this behaves exactly as [3]
+
+a gating function which can be used to enable and disable the output
+clock, this behaves exactly as [4]
+
+The binding must provide a list of the component clocks that shall be
+merged to this clock. The component clocks shall be of one of the
+"ti,*composite*-clock" types.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/clock/ti/mux.txt
+[3] Documentation/devicetree/bindings/clock/ti/divider.txt
+[4] Documentation/devicetree/bindings/clock/ti/gate.txt
+
+Required properties:
+- compatible : shall be: "ti,composite-clock"
+- clocks : link phandles of component clocks
+- #clock-cells : from common clock binding; shall be set to 0.
+
+Examples:
+
+usb_l4_gate_ick: usb_l4_gate_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-interface-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <5>;
+	reg = <0x0a10>;
+};
+
+usb_l4_div_ick: usb_l4_div_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-divider-clock";
+	clocks = <&l4_ick>;
+	ti,bit-shift = <4>;
+	ti,max-div = <1>;
+	reg = <0x0a40>;

Quoted 8 lines

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

Quoted 8 lines

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

+	ti,index-starts-at-one;
+};
+
+usb_l4_ick: usb_l4_ick {
+	#clock-cells = <0>;
+	compatible = "ti,composite-clock";
+	clocks = <&usb_l4_gate_ick>, <&usb_l4_div_ick>;
+};

From Topology perspective, would this be a better approach?

usb_l4_ick: usb_l4_ick {
	#clock-cells = <0>;
	compatible = "ti,composite-clock";

	usb_l4_gate_ick: usb_l4_gate_ick {
		#clock-cells = <0>;
		compatible = "ti,composite-interface-clock";
		clocks = <&l4_ick>;
		ti,bit-shift = <5>;
		reg = <0x0a10>;
	};

	usb_l4_div_ick: usb_l4_div_ick {
		#clock-cells = <0>;