[PATCH 0/8] clk: keystone: Add common clock drivers and PM bus

[PATCH 0/8] clk: keystone: Add common clock drivers and PM bus

[Santosh Shilimkar]

Series is an attempt to add the clock drivers for Keystone SOCs
based on common clock framework. A PLL drivers taking care of
SOC PLLs and a gate control driver taking clock management for
the IPs. The current Keystone based SOCs don' support dynamic power
management usecases like DVFS, SOC ilde etc and hence most of the
usage is limited to enabling clocks and finding the current clock
rate etc. Future SOCs will start supporting some of these dynamic
power saving features.

The series also contains the PM Bus driver which makes use of
pm_clk infrastructure of the PM core. This SOC doesn't need
the clocks to be enabled very early in the boot and hence the
clock initialsation is left to the subsystem_init() level. The
series is cooked up based on downstream work done by Murali K
but I ended up mostly rewriting it.

Based on to of Mike's dt binding series [1] and tested on Keystone2
EVM with I2C, UART and SPI driver enabled in the build.

Santosh Shilimkar (8):
  clk: keystone: add Keystone PLL clock driver
  clk: keystone: Add gate control clock driver
  clk: keystone: common clk driver initialization
  clk: keystone: Build Keystone clock drivers
  ARM: dts: keystone: Add clock tree data to devicetree
  ARM: dts: keystone: Add clock phandle to UART nodes
  ARM: keystone: Enable and initialise clock drivers
  ARM: keystone: add PM bus support for clock management

 .../devicetree/bindings/clock/keystone-gate.txt    |   30 +
 .../devicetree/bindings/clock/keystone-pll.txt     |   36 +
 arch/arm/boot/dts/keystone-clocks.dtsi             |  824 ++++++++++++++++++++
 arch/arm/boot/dts/keystone.dts                     |    4 +
 arch/arm/mach-keystone/Kconfig                     |    1 +
 drivers/bus/Makefile                               |    2 +
 drivers/bus/keystone_pm_bus.c                      |   68 ++
 drivers/clk/Kconfig                                |    7 +
 drivers/clk/Makefile                               |    1 +
 drivers/clk/keystone/Makefile                      |    1 +
 drivers/clk/keystone/clk.c                         |   34 +
 drivers/clk/keystone/gate.c                        |  306 ++++++++
 drivers/clk/keystone/pll.c                         |  197 +++++
 include/linux/clk/keystone.h                       |   20 +
 14 files changed, 1531 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
 create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
 create mode 100644 arch/arm/boot/dts/keystone-clocks.dtsi
 create mode 100644 drivers/bus/keystone_pm_bus.c
 create mode 100644 drivers/clk/keystone/Makefile
 create mode 100644 drivers/clk/keystone/clk.c
 create mode 100644 drivers/clk/keystone/gate.c
 create mode 100644 drivers/clk/keystone/pll.c
 create mode 100644 include/linux/clk/keystone.h

Regards,
Santosh
[1] https://lkml.org/lkml/2013/6/21/35
-- 
1.7.9.5

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
IP typically has a multiplier, and a divider. The addtional PLL IPs like
ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
as the Main PLL is controlled by a PLL controller and memory map registers.
This difference is handle using 'has_pll_cntrl' property.

Cc: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
 drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
 include/linux/clk/keystone.h                       |   18 ++
 3 files changed, 251 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
 create mode 100644 drivers/clk/keystone/pll.c
 create mode 100644 include/linux/clk/keystone.h

diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
new file mode 100644
index 0000000..58f6470
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
@@ -0,0 +1,36 @@
+Binding for keystone PLLs. The main PLL IP typically has a multiplier,
+a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
+and PAPLL are controlled by the memory mapped register where as the Main
+PLL is controlled by a PLL controller. This difference is handle using
+'pll_has_pllctrl' property.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "keystone,pll-clk".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : parent clock phandle
+- reg - index 0 -  PLLCTRL PLLM register address
+- 	index 1 -  MAINPLL_CTL0 register address
+- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
+- pllm_lower_mask - pllm lower bit mask
+- pllm_upper_mask - pllm upper bit mask
+- plld_mask - plld mask
+- fixed_postdiv - fixed post divider value
+
+Example:
+	clock {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclk>;
+		reg = <0x02310110 4	/* PLLCTRL PLLM */
+			0x02620328 4>;	/* MAINPLL_CTL0 */
+		pll_has_pllctrl;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7f000>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <2>;
+	};
diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
new file mode 100644
index 0000000..1453eea
--- /dev/null
+++ b/drivers/clk/keystone/pll.c
@@ -0,0 +1,197 @@
+/*
+ * Main PLL clk driver for Keystone devices
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <m-karicheri2@ti.com>
+ *	Santosh Shilimkar <santosh.shilimkar@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/clk/keystone.h>
+
+/**
+ * struct clk_pll_data - pll data structure
+ * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
+ *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)
+ * @phy_pllm: Physical address of PLLM in pll controller. Used when
+ *	has_pllctrl is non zero.
+ * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
+ *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
+ *	or PA PLL available on keystone2. These PLLs are controlled by
+ *	this register. Main PLL is controlled by a PLL controller.
+ * @pllm: PLL register map address
+ * @pll_ctl0: PLL controller map address
+ * @pllm_lower_mask: multiplier lower mask
+ * @pllm_upper_mask: multiplier upper mask
+ * @pllm_upper_shift: multiplier upper shift
+ * @plld_mask: divider mask
+ * @fixed_postdiv: Post divider
+ */
+struct clk_pll_data {
+	unsigned char has_pllctrl;
+	u32 phy_pllm;
+	u32 phy_pll_ctl0;
+	void __iomem *pllm;
+	void __iomem *pll_ctl0;
+	u32 pllm_lower_mask;
+	u32 pllm_upper_mask;
+	u32 pllm_upper_shift;
+	u32 plld_mask;
+	u32 fixed_postdiv;
+};
+
+/**
+ * struct clk_pll - Main pll clock
+ * @hw: clk_hw for the pll
+ * @pll_data: PLL driver specific data
+ */
+struct clk_pll {
+	struct clk_hw hw;
+	struct clk_pll_data *pll_data;
+};
+
+#define to_clk_pll(_hw) container_of(_hw, struct clk_pll, hw)
+
+static unsigned long clk_pllclk_recalc(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct clk_pll *pll = to_clk_pll(hw);
+	struct clk_pll_data *pll_data = pll->pll_data;
+	unsigned long rate = parent_rate;
+	u32  mult = 0, prediv, postdiv, val;
+
+	/*
+	 * get bits 0-5 of multiplier from pllctrl PLLM register
+	 * if has_pllctrl is non zero */
+	if (pll_data->has_pllctrl) {
+		val = readl(pll_data->pllm);
+		mult = (val & pll_data->pllm_lower_mask);
+	}
+
+	/* bit6-12 of PLLM is in Main PLL control register */
+	val = readl(pll_data->pll_ctl0);
+	mult |= ((val & pll_data->pllm_upper_mask)
+			>> pll_data->pllm_upper_shift);
+	prediv = (val & pll_data->plld_mask);
+	postdiv = pll_data->fixed_postdiv;
+
+	rate /= (prediv + 1);
+	rate = (rate * (mult + 1));
+	rate /= postdiv;
+
+	return rate;
+}
+
+static const struct clk_ops clk_pll_ops = {
+	.recalc_rate = clk_pllclk_recalc,
+};
+
+static struct clk *clk_register_pll(struct device *dev,
+			const char *name,
+			const char *parent_name,
+			struct clk_pll_data *pll_data)
+{
+	struct clk_init_data init;
+	struct clk_pll *pll;
+	struct clk *clk;
+
+	if (!pll_data)
+		return ERR_PTR(-ENODEV);
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_pll_ops;
+	init.flags = 0;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	pll->pll_data	= pll_data;
+	pll->hw.init = &init;
+
+	clk = clk_register(NULL, &pll->hw);
+	if (IS_ERR(clk))
+		goto out;
+
+	return clk;
+out:
+	kfree(pll);
+	return NULL;
+}
+
+/**
+ * of_keystone_pll_clk_init - PLL initialisation via DT
+ * @node: device tree node for this clock
+ */
+void __init of_keystone_pll_clk_init(struct device_node *node)
+{
+	struct clk_pll_data *pll_data;
+	const char *parent_name;
+	struct clk *clk;
+	int temp;
+
+	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
+	WARN_ON(!pll_data);
+
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	if (of_find_property(node, "pll_has_pllctrl", &temp)) {
+		/* PLL is controlled by the pllctrl */
+		pll_data->has_pllctrl = 1;
+		pll_data->pllm = of_iomap(node, 0);
+		WARN_ON(!pll_data->pllm);
+
+		pll_data->pll_ctl0 = of_iomap(node, 1);
+		WARN_ON(!pll_data->pll_ctl0);
+
+		if (of_property_read_u32(node, "pllm_lower_mask",
+				&pll_data->pllm_lower_mask))
+			goto out;
+
+	} else {
+		/* PLL is controlled by the ctrl register */
+		pll_data->has_pllctrl = 0;
+		pll_data->pll_ctl0 = of_iomap(node, 0);
+	}
+
+	if (of_property_read_u32(node, "pllm_upper_mask",
+			&pll_data->pllm_upper_mask))
+		goto out;
+
+	if (of_property_read_u32(node, "pllm_upper_shift",
+			&pll_data->pllm_upper_shift))
+		goto out;
+
+	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
+		goto out;
+
+	if (of_property_read_u32(node, "fixed_postdiv",
+					&pll_data->fixed_postdiv))
+		goto out;
+
+	clk = clk_register_pll(NULL, node->name, parent_name,
+					 pll_data);
+	if (clk) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		return;
+	}
+out:
+	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
+		 node->name);
+	kfree(pll_data);
+}
+EXPORT_SYMBOL_GPL(of_keystone_pll_clk_init);
+CLK_OF_DECLARE(keystone_pll_clk, "keystone,pll-clk", of_keystone_pll_clk_init);
diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
new file mode 100644
index 0000000..1ade95d
--- /dev/null
+++ b/include/linux/clk/keystone.h
@@ -0,0 +1,18 @@
+/*
+ * Keystone Clock driver header
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Santosh Shilimkar <santosh.shilimkar@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_CLK_KEYSTONE_H_
+#define __LINUX_CLK_KEYSTONE_H_
+
+extern void of_keystone_pll_clk_init(struct device_node *node);
+
+#endif /* __LINUX_CLK_KEYSTONE_H_ */
-- 
1.7.9.5

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Mark Rutland]

[Adding dt maintainers]

On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> as the Main PLL is controlled by a PLL controller and memory map registers.
> This difference is handle using 'has_pll_cntrl' property.
> 
> Cc: Mike Turquette <mturquette@linaro.org>
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>  include/linux/clk/keystone.h                       |   18 ++
>  3 files changed, 251 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>  create mode 100644 drivers/clk/keystone/pll.c
>  create mode 100644 include/linux/clk/keystone.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> new file mode 100644
> index 0000000..58f6470
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> @@ -0,0 +1,36 @@
> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> +and PAPLL are controlled by the memory mapped register where as the Main
> +PLL is controlled by a PLL controller. This difference is handle using
> +'pll_has_pllctrl' property.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "keystone,pll-clk".

Keystone isn't a vendor, and generally, bindings have used "clock"
rather than "clk".

Perhaps "ti,keystone-pll-clock" ?

> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : parent clock phandle
> +- reg - index 0 -  PLLCTRL PLLM register address
> +- 	index 1 -  MAINPLL_CTL0 register address

Perhaps a reg-names would be useful?

> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register

Huh? I don't understand what that description means. What does the
property tell you? Is having one of the registers optional? If so that
should be described by a reg-names property associated with the reg, and
should be noted in the binding.

> +- pllm_lower_mask - pllm lower bit mask
> +- pllm_upper_mask - pllm upper bit mask
> +- plld_mask - plld mask
> +- fixed_postdiv - fixed post divider value

Please use '-' rather than '_' in property names, it's a standard
convention for dt and going against it just makes things unnecessarily
complicated.

Why are these necessary? Are clocks sharing common registers, are there
some sets of "keystone,pll-clk"s that have the same masks, or does this
just vary wildly?

Also, what types are these (presumably a single cell/u32)?

> +
> +Example:
> +	clock {
> +		#clock-cells = <0>;
> +		compatible = "keystone,pll-clk";
> +		clocks = <&refclk>;
> +		reg = <0x02310110 4	/* PLLCTRL PLLM */
> +			0x02620328 4>;	/* MAINPLL_CTL0 */
> +		pll_has_pllctrl;
> +		pllm_lower_mask	= <0x3f>;
> +		pllm_upper_mask = <0x7f000>;
> +		pllm_upper_shift = <6>;
> +		plld_mask = <0x3f>;
> +		fixed_postdiv = <2>;
> +	};
> diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
> new file mode 100644
> index 0000000..1453eea
> --- /dev/null
> +++ b/drivers/clk/keystone/pll.c
> @@ -0,0 +1,197 @@
> +/*
> + * Main PLL clk driver for Keystone devices
> + *
> + * Copyright (C) 2013 Texas Instruments Inc.
> + *	Murali Karicheri <m-karicheri2@ti.com>
> + *	Santosh Shilimkar <santosh.shilimkar@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/clk/keystone.h>
> +
> +/**
> + * struct clk_pll_data - pll data structure
> + * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
> + *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)

The description in the binding doesn't make that clear at all. The
naming scheme is confusing -- because you've named the PLLCTRL PLLM
register pllm and the MAINPLL_CTL0 register pll_ctl0, it makes it look
like the logic around has_pllctrl is being used backwards throughout the
entire driver.

> + * @phy_pllm: Physical address of PLLM in pll controller. Used when
> + *	has_pllctrl is non zero.
> + * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
> + *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
> + *	or PA PLL available on keystone2. These PLLs are controlled by
> + *	this register. Main PLL is controlled by a PLL controller.
> + * @pllm: PLL register map address
> + * @pll_ctl0: PLL controller map address
> + * @pllm_lower_mask: multiplier lower mask
> + * @pllm_upper_mask: multiplier upper mask
> + * @pllm_upper_shift: multiplier upper shift
> + * @plld_mask: divider mask
> + * @fixed_postdiv: Post divider
> + */
> +struct clk_pll_data {
> +	unsigned char has_pllctrl;

bool?

> +	u32 phy_pllm;
> +	u32 phy_pll_ctl0;
> +	void __iomem *pllm;
> +	void __iomem *pll_ctl0;
> +	u32 pllm_lower_mask;
> +	u32 pllm_upper_mask;
> +	u32 pllm_upper_shift;
> +	u32 plld_mask;
> +	u32 fixed_postdiv;
> +};

[...]

> +/**
> + * of_keystone_pll_clk_init - PLL initialisation via DT
> + * @node: device tree node for this clock
> + */
> +void __init of_keystone_pll_clk_init(struct device_node *node)
> +{
> +	struct clk_pll_data *pll_data;
> +	const char *parent_name;
> +	struct clk *clk;
> +	int temp;
> +
> +	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
> +	WARN_ON(!pll_data);

Why don't you return here, before dereferencing NULL below?

> +
> +	parent_name = of_clk_get_parent_name(node, 0);
> +
> +	if (of_find_property(node, "pll_has_pllctrl", &temp)) {

of_property_read_bool?

> +		/* PLL is controlled by the pllctrl */
> +		pll_data->has_pllctrl = 1;
> +		pll_data->pllm = of_iomap(node, 0);
> +		WARN_ON(!pll_data->pllm);
> +
> +		pll_data->pll_ctl0 = of_iomap(node, 1);
> +		WARN_ON(!pll_data->pll_ctl0);

Why do you not bail out if you couldn't map the registers you need?

> +
> +		if (of_property_read_u32(node, "pllm_lower_mask",
> +				&pll_data->pllm_lower_mask))
> +			goto out;
> +
> +	} else {
> +		/* PLL is controlled by the ctrl register */
> +		pll_data->has_pllctrl = 0;
> +		pll_data->pll_ctl0 = of_iomap(node, 0);

Huh? That's in a different order to the above (where pll_ctl0 was index
1 in the reg). I think you need to use reg-names for this, and come up
with a more scheme internal to the driver to minimise confusion.

> +	}
> +
> +	if (of_property_read_u32(node, "pllm_upper_mask",
> +			&pll_data->pllm_upper_mask))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "pllm_upper_shift",
> +			&pll_data->pllm_upper_shift))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "fixed_postdiv",
> +					&pll_data->fixed_postdiv))
> +		goto out;
> +
> +	clk = clk_register_pll(NULL, node->name, parent_name,
> +					 pll_data);
> +	if (clk) {
> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +		return;
> +	}
> +out:
> +	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
> +		 node->name);
> +	kfree(pll_data);

You haven't unmapped either of the registers you may have mapped on your
way out, and you've thrown away your only reference to them.

Thanks,
Mark.

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> [Adding dt maintainers]
> 
> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>> as the Main PLL is controlled by a PLL controller and memory map registers.
>> This difference is handle using 'has_pll_cntrl' property.
>>
>> Cc: Mike Turquette <mturquette@linaro.org>
>>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---

Thanks for review Mark.

>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>  include/linux/clk/keystone.h                       |   18 ++
>>  3 files changed, 251 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>  create mode 100644 drivers/clk/keystone/pll.c
>>  create mode 100644 include/linux/clk/keystone.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>> new file mode 100644
>> index 0000000..58f6470
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>> @@ -0,0 +1,36 @@
>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>> +and PAPLL are controlled by the memory mapped register where as the Main
>> +PLL is controlled by a PLL controller. This difference is handle using
>> +'pll_has_pllctrl' property.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible : shall be "keystone,pll-clk".
> 
> Keystone isn't a vendor, and generally, bindings have used "clock"
> rather than "clk".
> 
> Perhaps "ti,keystone-pll-clock" ?
> 
Agree.

>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : parent clock phandle
>> +- reg - index 0 -  PLLCTRL PLLM register address
>> +- 	index 1 -  MAINPLL_CTL0 register address
> 
> Perhaps a reg-names would be useful?
> 
>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> 
> Huh? I don't understand what that description means. What does the
> property tell you? Is having one of the registers optional? If so that
> should be described by a reg-names property associated with the reg, and
> should be noted in the binding.
> 
After re-reading it, yes I agree it not clear. The point is there
are two different types of IPs and pogramming model changes quite
a bit. Its not just 1 register optional.

>> +- pllm_lower_mask - pllm lower bit mask
>> +- pllm_upper_mask - pllm upper bit mask
>> +- plld_mask - plld mask
>> +- fixed_postdiv - fixed post divider value
> 
> Please use '-' rather than '_' in property names, it's a standard
> convention for dt and going against it just makes things unnecessarily
> complicated.
> 
> Why are these necessary? Are clocks sharing common registers, are there
> some sets of "keystone,pll-clk"s that have the same masks, or does this
> just vary wildly?
> 
This is mainly to take care of the programming model which varies between
IPs. Will try to make that bit more clear.

> Also, what types are these (presumably a single cell/u32)?
> 
u32

>> +
>> +Example:
>> +	clock {
>> +		#clock-cells = <0>;
>> +		compatible = "keystone,pll-clk";
>> +		clocks = <&refclk>;
>> +		reg = <0x02310110 4	/* PLLCTRL PLLM */
>> +			0x02620328 4>;	/* MAINPLL_CTL0 */
>> +		pll_has_pllctrl;
>> +		pllm_lower_mask	= <0x3f>;
>> +		pllm_upper_mask = <0x7f000>;
>> +		pllm_upper_shift = <6>;
>> +		plld_mask = <0x3f>;
>> +		fixed_postdiv = <2>;
>> +	};
>> diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
>> new file mode 100644
>> index 0000000..1453eea
>> --- /dev/null
>> +++ b/drivers/clk/keystone/pll.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * Main PLL clk driver for Keystone devices
>> + *
>> + * Copyright (C) 2013 Texas Instruments Inc.
>> + *	Murali Karicheri <m-karicheri2@ti.com>
>> + *	Santosh Shilimkar <santosh.shilimkar@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/clk/keystone.h>
>> +
>> +/**
>> + * struct clk_pll_data - pll data structure
>> + * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
>> + *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)
> 
> The description in the binding doesn't make that clear at all. The
> naming scheme is confusing -- because you've named the PLLCTRL PLLM
> register pllm and the MAINPLL_CTL0 register pll_ctl0, it makes it look
> like the logic around has_pllctrl is being used backwards throughout the
> entire driver.
>
This is due to difference in the IPs. Naming scheme can be improved
though. Was bit lazy to not do that firstplace.
 
>> + * @phy_pllm: Physical address of PLLM in pll controller. Used when
>> + *	has_pllctrl is non zero.
>> + * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
>> + *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
>> + *	or PA PLL available on keystone2. These PLLs are controlled by
>> + *	this register. Main PLL is controlled by a PLL controller.
>> + * @pllm: PLL register map address
>> + * @pll_ctl0: PLL controller map address
>> + * @pllm_lower_mask: multiplier lower mask
>> + * @pllm_upper_mask: multiplier upper mask
>> + * @pllm_upper_shift: multiplier upper shift
>> + * @plld_mask: divider mask
>> + * @fixed_postdiv: Post divider
>> + */
>> +struct clk_pll_data {
>> +	unsigned char has_pllctrl;
> 
> bool?
Yes

> 
>> +	u32 phy_pllm;
>> +	u32 phy_pll_ctl0;
>> +	void __iomem *pllm;
>> +	void __iomem *pll_ctl0;
>> +	u32 pllm_lower_mask;
>> +	u32 pllm_upper_mask;
>> +	u32 pllm_upper_shift;
>> +	u32 plld_mask;
>> +	u32 fixed_postdiv;
>> +};
> 
> [...]
> 
>> +/**
>> + * of_keystone_pll_clk_init - PLL initialisation via DT
>> + * @node: device tree node for this clock
>> + */
>> +void __init of_keystone_pll_clk_init(struct device_node *node)
>> +{
>> +	struct clk_pll_data *pll_data;
>> +	const char *parent_name;
>> +	struct clk *clk;
>> +	int temp;
>> +
>> +	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
>> +	WARN_ON(!pll_data);
> 
> Why don't you return here, before dereferencing NULL below?
> 
>> +
>> +	parent_name = of_clk_get_parent_name(node, 0);
>> +
>> +	if (of_find_property(node, "pll_has_pllctrl", &temp)) {
> 
> of_property_read_bool?
> 
>> +		/* PLL is controlled by the pllctrl */
>> +		pll_data->has_pllctrl = 1;
>> +		pll_data->pllm = of_iomap(node, 0);
>> +		WARN_ON(!pll_data->pllm);
>> +
>> +		pll_data->pll_ctl0 = of_iomap(node, 1);
>> +		WARN_ON(!pll_data->pll_ctl0);
> 
> Why do you not bail out if you couldn't map the registers you need?
> 
>> +
>> +		if (of_property_read_u32(node, "pllm_lower_mask",
>> +				&pll_data->pllm_lower_mask))
>> +			goto out;
>> +
>> +	} else {
>> +		/* PLL is controlled by the ctrl register */
>> +		pll_data->has_pllctrl = 0;
>> +		pll_data->pll_ctl0 = of_iomap(node, 0);
> 
> Huh? That's in a different order to the above (where pll_ctl0 was index
> 1 in the reg). I think you need to use reg-names for this, and come up
> with a more scheme internal to the driver to minimise confusion.
> 
Agree. 

>> +	}
>> +
>> +	if (of_property_read_u32(node, "pllm_upper_mask",
>> +			&pll_data->pllm_upper_mask))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "pllm_upper_shift",
>> +			&pll_data->pllm_upper_shift))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "fixed_postdiv",
>> +					&pll_data->fixed_postdiv))
>> +		goto out;
>> +
>> +	clk = clk_register_pll(NULL, node->name, parent_name,
>> +					 pll_data);
>> +	if (clk) {
>> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +		return;
>> +	}
>> +out:
>> +	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
>> +		 node->name);
>> +	kfree(pll_data);
> 
> You haven't unmapped either of the registers you may have mapped on your
> way out, and you've thrown away your only reference to them.
> 
Yep.

Will address your comments in next version.

Regards,
Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Mark Rutland]

On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> > [Adding dt maintainers]
> > 
> > On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >> as the Main PLL is controlled by a PLL controller and memory map registers.
> >> This difference is handle using 'has_pll_cntrl' property.
> >>
> >> Cc: Mike Turquette <mturquette@linaro.org>
> >>
> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> ---
> 
> Thanks for review Mark.
> 
> >>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>  include/linux/clk/keystone.h                       |   18 ++
> >>  3 files changed, 251 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>  create mode 100644 drivers/clk/keystone/pll.c
> >>  create mode 100644 include/linux/clk/keystone.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >> new file mode 100644
> >> index 0000000..58f6470
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >> @@ -0,0 +1,36 @@
> >> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >> +and PAPLL are controlled by the memory mapped register where as the Main
> >> +PLL is controlled by a PLL controller. This difference is handle using
> >> +'pll_has_pllctrl' property.
> >> +
> >> +This binding uses the common clock binding[1].
> >> +
> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible : shall be "keystone,pll-clk".
> > 
> > Keystone isn't a vendor, and generally, bindings have used "clock"
> > rather than "clk".
> > 
> > Perhaps "ti,keystone-pll-clock" ?
> > 
> Agree.
> 
> >> +- #clock-cells : from common clock binding; shall be set to 0.
> >> +- clocks : parent clock phandle
> >> +- reg - index 0 -  PLLCTRL PLLM register address
> >> +- 	index 1 -  MAINPLL_CTL0 register address
> > 
> > Perhaps a reg-names would be useful?
> > 
> >> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> > 
> > Huh? I don't understand what that description means. What does the
> > property tell you? Is having one of the registers optional? If so that
> > should be described by a reg-names property associated with the reg, and
> > should be noted in the binding.
> > 
> After re-reading it, yes I agree it not clear. The point is there
> are two different types of IPs and pogramming model changes quite
> a bit. Its not just 1 register optional.

If that's the case, then having different compatible strings would make
sense.

> 
> >> +- pllm_lower_mask - pllm lower bit mask
> >> +- pllm_upper_mask - pllm upper bit mask
> >> +- plld_mask - plld mask
> >> +- fixed_postdiv - fixed post divider value
> > 
> > Please use '-' rather than '_' in property names, it's a standard
> > convention for dt and going against it just makes things unnecessarily
> > complicated.
> > 
> > Why are these necessary? Are clocks sharing common registers, are there
> > some sets of "keystone,pll-clk"s that have the same masks, or does this
> > just vary wildly?
> > 
> This is mainly to take care of the programming model which varies between
> IPs. Will try to make that bit more clear.

Are there more than the two IPs mentioned above? If they had separate
compatible strings, would that give enough information implicitly,
without the precise masks needing to be in the dt?

> 
> > Also, what types are these (presumably a single cell/u32)?
> > 
> u32

Ok, please could you document that in the binding?

Cheers,
Mark.

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>> [Adding dt maintainers]
>>>
>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>
>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>
>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> ---
>>
>> Thanks for review Mark.
>>
>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>  3 files changed, 251 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>> new file mode 100644
>>>> index 0000000..58f6470
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>> @@ -0,0 +1,36 @@
>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>> +'pll_has_pllctrl' property.
>>>> +
>>>> +This binding uses the common clock binding[1].
>>>> +
>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +
>>>> +Required properties:
>>>> +- compatible : shall be "keystone,pll-clk".
>>>
>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>> rather than "clk".
>>>
>>> Perhaps "ti,keystone-pll-clock" ?
>>>
>> Agree.
>>
>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>> +- clocks : parent clock phandle
>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>> +- 	index 1 -  MAINPLL_CTL0 register address
>>>
>>> Perhaps a reg-names would be useful?
>>>
>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>
>>> Huh? I don't understand what that description means. What does the
>>> property tell you? Is having one of the registers optional? If so that
>>> should be described by a reg-names property associated with the reg, and
>>> should be noted in the binding.
>>>
>> After re-reading it, yes I agree it not clear. The point is there
>> are two different types of IPs and pogramming model changes quite
>> a bit. Its not just 1 register optional.
> 
> If that's the case, then having different compatible strings would make
> sense.
> 
>>
>>>> +- pllm_lower_mask - pllm lower bit mask
>>>> +- pllm_upper_mask - pllm upper bit mask
>>>> +- plld_mask - plld mask
>>>> +- fixed_postdiv - fixed post divider value
>>>
>>> Please use '-' rather than '_' in property names, it's a standard
>>> convention for dt and going against it just makes things unnecessarily
>>> complicated.
>>>
>>> Why are these necessary? Are clocks sharing common registers, are there
>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>> just vary wildly?
>>>
>> This is mainly to take care of the programming model which varies between
>> IPs. Will try to make that bit more clear.
> 
> Are there more than the two IPs mentioned above? If they had separate
> compatible strings, would that give enough information implicitly,
> without the precise masks needing to be in the dt?
> 
I will explore the separate compatible option. Thanks for suggestion.

Regards,
Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

Mark,

On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>> [Adding dt maintainers]
>>>>
>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>
>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>
>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>> ---
>>>
>>> Thanks for review Mark.
>>>
>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>  3 files changed, 251 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>> new file mode 100644
>>>>> index 0000000..58f6470
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>> @@ -0,0 +1,36 @@
>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>> +'pll_has_pllctrl' property.
>>>>> +
>>>>> +This binding uses the common clock binding[1].
>>>>> +
>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>
>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>> rather than "clk".
>>>>
>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>
>>> Agree.
>>>
>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>> +- clocks : parent clock phandle
>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>> +- 	index 1 -  MAINPLL_CTL0 register address
>>>>
>>>> Perhaps a reg-names would be useful?
>>>>
>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>
>>>> Huh? I don't understand what that description means. What does the
>>>> property tell you? Is having one of the registers optional? If so that
>>>> should be described by a reg-names property associated with the reg, and
>>>> should be noted in the binding.
>>>>
>>> After re-reading it, yes I agree it not clear. The point is there
>>> are two different types of IPs and pogramming model changes quite
>>> a bit. Its not just 1 register optional.
>>
>> If that's the case, then having different compatible strings would make
>> sense.
>>
>>>
>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>> +- plld_mask - plld mask
>>>>> +- fixed_postdiv - fixed post divider value
>>>>
>>>> Please use '-' rather than '_' in property names, it's a standard
>>>> convention for dt and going against it just makes things unnecessarily
>>>> complicated.
>>>>
>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>> just vary wildly?
>>>>
>>> This is mainly to take care of the programming model which varies between
>>> IPs. Will try to make that bit more clear.
>>
>> Are there more than the two IPs mentioned above? If they had separate
>> compatible strings, would that give enough information implicitly,
>> without the precise masks needing to be in the dt?
>>
> I will explore the separate compatible option. Thanks for suggestion.
> 
I looked at further into separate compatible option or reg-names
to check if it can help to reduce some additional dt information.
Actually it doesn't help much. The base programming model is shared
between both types of PLL IPs. The PLLs which has PLL controller along
with MMRs, has to take into account additional bit-fields for the
multiplier and divider along with the base model multiplier and divider
registers.

Having said that, there are few parameters which are fixed like
plld_mask, pllm_upper_shift etc need not come from DT. I am going
to remove them from dt bindings and also make the reg index consistent
as it should have been in first place.

Rest of the comments are fine and will be addressed in next version.

Regards,
Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-19 10:42:19)
> Mark,
> 
> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> > On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> >> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> >>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> >>>> [Adding dt maintainers]
> >>>>
> >>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
> >>>>> This difference is handle using 'has_pll_cntrl' property.
> >>>>>
> >>>>> Cc: Mike Turquette <mturquette@linaro.org>
> >>>>>
> >>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>>> ---
> >>>
> >>> Thanks for review Mark.
> >>>
> >>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>>>>  include/linux/clk/keystone.h                       |   18 ++
> >>>>>  3 files changed, 251 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>  create mode 100644 drivers/clk/keystone/pll.c
> >>>>>  create mode 100644 include/linux/clk/keystone.h
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..58f6470
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>> @@ -0,0 +1,36 @@
> >>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >>>>> +and PAPLL are controlled by the memory mapped register where as the Main
> >>>>> +PLL is controlled by a PLL controller. This difference is handle using
> >>>>> +'pll_has_pllctrl' property.
> >>>>> +
> >>>>> +This binding uses the common clock binding[1].
> >>>>> +
> >>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> +
> >>>>> +Required properties:
> >>>>> +- compatible : shall be "keystone,pll-clk".
> >>>>
> >>>> Keystone isn't a vendor, and generally, bindings have used "clock"
> >>>> rather than "clk".
> >>>>
> >>>> Perhaps "ti,keystone-pll-clock" ?
> >>>>
> >>> Agree.
> >>>
> >>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>> +- clocks : parent clock phandle
> >>>>> +- reg - index 0 -  PLLCTRL PLLM register address
> >>>>> +-        index 1 -  MAINPLL_CTL0 register address
> >>>>
> >>>> Perhaps a reg-names would be useful?
> >>>>
> >>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> >>>>
> >>>> Huh? I don't understand what that description means. What does the
> >>>> property tell you? Is having one of the registers optional? If so that
> >>>> should be described by a reg-names property associated with the reg, and
> >>>> should be noted in the binding.
> >>>>
> >>> After re-reading it, yes I agree it not clear. The point is there
> >>> are two different types of IPs and pogramming model changes quite
> >>> a bit. Its not just 1 register optional.
> >>
> >> If that's the case, then having different compatible strings would make
> >> sense.
> >>
> >>>
> >>>>> +- pllm_lower_mask - pllm lower bit mask
> >>>>> +- pllm_upper_mask - pllm upper bit mask
> >>>>> +- plld_mask - plld mask
> >>>>> +- fixed_postdiv - fixed post divider value
> >>>>
> >>>> Please use '-' rather than '_' in property names, it's a standard
> >>>> convention for dt and going against it just makes things unnecessarily
> >>>> complicated.
> >>>>
> >>>> Why are these necessary? Are clocks sharing common registers, are there
> >>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
> >>>> just vary wildly?
> >>>>
> >>> This is mainly to take care of the programming model which varies between
> >>> IPs. Will try to make that bit more clear.
> >>
> >> Are there more than the two IPs mentioned above? If they had separate
> >> compatible strings, would that give enough information implicitly,
> >> without the precise masks needing to be in the dt?
> >>
> > I will explore the separate compatible option. Thanks for suggestion.
> > 
> I looked at further into separate compatible option or reg-names
> to check if it can help to reduce some additional dt information.
> Actually it doesn't help much. The base programming model is shared
> between both types of PLL IPs. The PLLs which has PLL controller along
> with MMRs, has to take into account additional bit-fields for the
> multiplier and divider along with the base model multiplier and divider
> registers.

It is common for the base programming model to be similar or shared
between two different compatible strings. You can use the same clk_ops
for clk_prepare, clk_enable, etc.

The point is to use the different compatible strings to encode the
differences in *data* between the two clock types. That way you have
fewer properties to list in the binding since two separate setup
functions can implicitly handle the differences in initializing the
per-clock data.

> 
> Having said that, there are few parameters which are fixed like
> plld_mask, pllm_upper_shift etc need not come from DT. I am going
> to remove them from dt bindings and also make the reg index consistent
> as it should have been in first place.

This is nice. Note that these things may change in future Keystone
versions because hardware people hate you and want to make your life
hard. So the compatible string might benefit from including the first
keystone part number that uses this binding (e.g.
ti,keystone-2420-pll-clock, which I just made up).

In the future when the register layout, offsets and masks change for
absolutely no reason (but the programming model is the same) then you
can just write a new binding that setups up your private clk_pll_data
struct without having to jam all of those details into DT (e.g.
ti,keystone-3430-pll-clock, which I also just made up).

Regards,
Mike

> 
> Rest of the comments are fine and will be addressed in next version.
> 
> Regards,
> Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-19 10:42:19)
>> Mark,
>>
>> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
>>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>>>> [Adding dt maintainers]
>>>>>>
>>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>>>
>>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>>>
>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>>> ---
>>>>>
>>>>> Thanks for review Mark.
>>>>>
>>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>>>  3 files changed, 251 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..58f6470
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>> @@ -0,0 +1,36 @@
>>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>>>> +'pll_has_pllctrl' property.
>>>>>>> +
>>>>>>> +This binding uses the common clock binding[1].
>>>>>>> +
>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>>>
>>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>>>> rather than "clk".
>>>>>>
>>>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>>>
>>>>> Agree.
>>>>>
>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>> +- clocks : parent clock phandle
>>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
>>>>>>
>>>>>> Perhaps a reg-names would be useful?
>>>>>>
>>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>>>
>>>>>> Huh? I don't understand what that description means. What does the
>>>>>> property tell you? Is having one of the registers optional? If so that
>>>>>> should be described by a reg-names property associated with the reg, and
>>>>>> should be noted in the binding.
>>>>>>
>>>>> After re-reading it, yes I agree it not clear. The point is there
>>>>> are two different types of IPs and pogramming model changes quite
>>>>> a bit. Its not just 1 register optional.
>>>>
>>>> If that's the case, then having different compatible strings would make
>>>> sense.
>>>>
>>>>>
>>>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>>>> +- plld_mask - plld mask
>>>>>>> +- fixed_postdiv - fixed post divider value
>>>>>>
>>>>>> Please use '-' rather than '_' in property names, it's a standard
>>>>>> convention for dt and going against it just makes things unnecessarily
>>>>>> complicated.
>>>>>>
>>>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>>>> just vary wildly?
>>>>>>
>>>>> This is mainly to take care of the programming model which varies between
>>>>> IPs. Will try to make that bit more clear.
>>>>
>>>> Are there more than the two IPs mentioned above? If they had separate
>>>> compatible strings, would that give enough information implicitly,
>>>> without the precise masks needing to be in the dt?
>>>>
>>> I will explore the separate compatible option. Thanks for suggestion.
>>>
>> I looked at further into separate compatible option or reg-names
>> to check if it can help to reduce some additional dt information.
>> Actually it doesn't help much. The base programming model is shared
>> between both types of PLL IPs. The PLLs which has PLL controller along
>> with MMRs, has to take into account additional bit-fields for the
>> multiplier and divider along with the base model multiplier and divider
>> registers.
> 
> It is common for the base programming model to be similar or shared
> between two different compatible strings. You can use the same clk_ops
> for clk_prepare, clk_enable, etc.
>
> The point is to use the different compatible strings to encode the
> differences in *data* between the two clock types. That way you have
> fewer properties to list in the binding since two separate setup
> functions can implicitly handle the differences in initializing the
> per-clock data.
> 
Thats the point I came back. Both PLL share the properties and
the main PLL brings in couple of properties to extend the divider
field and that was handled through the flag. As mentioned below,
some properties like plld_mask, pllm_upper_shift etc can be
dropped since they are static values.

>>
>> Having said that, there are few parameters which are fixed like
>> plld_mask, pllm_upper_shift etc need not come from DT. I am going
>> to remove them from dt bindings and also make the reg index consistent
>> as it should have been in first place.
> 
> This is nice. Note that these things may change in future Keystone
> versions because hardware people hate you and want to make your life
> hard. So the compatible string might benefit from including the first
> keystone part number that uses this binding (e.g.
> ti,keystone-2420-pll-clock, which I just made up).
> 
> In the future when the register layout, offsets and masks change for
> absolutely no reason (but the programming model is the same) then you
> can just write a new binding that setups up your private clk_pll_data
> struct without having to jam all of those details into DT (e.g.
> ti,keystone-3430-pll-clock, which I also just made up).
> 
So thats the good part so far with the keystone where the compatibility
is maintained pretty well. I know background of your comments ;-)
I will keep the bindings without any direct device association for
now considering I don't foresee any thing changing in that aspect.

Regards,
Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-20 06:41:21)
> On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
> > Quoting Santosh Shilimkar (2013-08-19 10:42:19)
> >> Mark,
> >>
> >> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> >>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> >>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> >>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> >>>>>> [Adding dt maintainers]
> >>>>>>
> >>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
> >>>>>>> This difference is handle using 'has_pll_cntrl' property.
> >>>>>>>
> >>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
> >>>>>>>
> >>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>>>>> ---
> >>>>>
> >>>>> Thanks for review Mark.
> >>>>>
> >>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>>>>>>  include/linux/clk/keystone.h                       |   18 ++
> >>>>>>>  3 files changed, 251 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
> >>>>>>>  create mode 100644 include/linux/clk/keystone.h
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..58f6470
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>> @@ -0,0 +1,36 @@
> >>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
> >>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
> >>>>>>> +'pll_has_pllctrl' property.
> >>>>>>> +
> >>>>>>> +This binding uses the common clock binding[1].
> >>>>>>> +
> >>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>>>> +
> >>>>>>> +Required properties:
> >>>>>>> +- compatible : shall be "keystone,pll-clk".
> >>>>>>
> >>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
> >>>>>> rather than "clk".
> >>>>>>
> >>>>>> Perhaps "ti,keystone-pll-clock" ?
> >>>>>>
> >>>>> Agree.
> >>>>>
> >>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>>>> +- clocks : parent clock phandle
> >>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
> >>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
> >>>>>>
> >>>>>> Perhaps a reg-names would be useful?
> >>>>>>
> >>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> >>>>>>
> >>>>>> Huh? I don't understand what that description means. What does the
> >>>>>> property tell you? Is having one of the registers optional? If so that
> >>>>>> should be described by a reg-names property associated with the reg, and
> >>>>>> should be noted in the binding.
> >>>>>>
> >>>>> After re-reading it, yes I agree it not clear. The point is there
> >>>>> are two different types of IPs and pogramming model changes quite
> >>>>> a bit. Its not just 1 register optional.
> >>>>
> >>>> If that's the case, then having different compatible strings would make
> >>>> sense.
> >>>>
> >>>>>
> >>>>>>> +- pllm_lower_mask - pllm lower bit mask
> >>>>>>> +- pllm_upper_mask - pllm upper bit mask
> >>>>>>> +- plld_mask - plld mask
> >>>>>>> +- fixed_postdiv - fixed post divider value
> >>>>>>
> >>>>>> Please use '-' rather than '_' in property names, it's a standard
> >>>>>> convention for dt and going against it just makes things unnecessarily
> >>>>>> complicated.
> >>>>>>
> >>>>>> Why are these necessary? Are clocks sharing common registers, are there
> >>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
> >>>>>> just vary wildly?
> >>>>>>
> >>>>> This is mainly to take care of the programming model which varies between
> >>>>> IPs. Will try to make that bit more clear.
> >>>>
> >>>> Are there more than the two IPs mentioned above? If they had separate
> >>>> compatible strings, would that give enough information implicitly,
> >>>> without the precise masks needing to be in the dt?
> >>>>
> >>> I will explore the separate compatible option. Thanks for suggestion.
> >>>
> >> I looked at further into separate compatible option or reg-names
> >> to check if it can help to reduce some additional dt information.
> >> Actually it doesn't help much. The base programming model is shared
> >> between both types of PLL IPs. The PLLs which has PLL controller along
> >> with MMRs, has to take into account additional bit-fields for the
> >> multiplier and divider along with the base model multiplier and divider
> >> registers.
> > 
> > It is common for the base programming model to be similar or shared
> > between two different compatible strings. You can use the same clk_ops
> > for clk_prepare, clk_enable, etc.
> >
> > The point is to use the different compatible strings to encode the
> > differences in *data* between the two clock types. That way you have
> > fewer properties to list in the binding since two separate setup
> > functions can implicitly handle the differences in initializing the
> > per-clock data.
> > 
> Thats the point I came back. Both PLL share the properties and
> the main PLL brings in couple of properties to extend the divider
> field and that was handled through the flag. As mentioned below,
> some properties like plld_mask, pllm_upper_shift etc can be
> dropped since they are static values.
> 
> >>
> >> Having said that, there are few parameters which are fixed like
> >> plld_mask, pllm_upper_shift etc need not come from DT. I am going
> >> to remove them from dt bindings and also make the reg index consistent
> >> as it should have been in first place.
> > 
> > This is nice. Note that these things may change in future Keystone
> > versions because hardware people hate you and want to make your life
> > hard. So the compatible string might benefit from including the first
> > keystone part number that uses this binding (e.g.
> > ti,keystone-2420-pll-clock, which I just made up).
> > 
> > In the future when the register layout, offsets and masks change for
> > absolutely no reason (but the programming model is the same) then you
> > can just write a new binding that setups up your private clk_pll_data
> > struct without having to jam all of those details into DT (e.g.
> > ti,keystone-3430-pll-clock, which I also just made up).
> > 
> So thats the good part so far with the keystone where the compatibility
> is maintained pretty well. I know background of your comments ;-)
> I will keep the bindings without any direct device association for
> now considering I don't foresee any thing changing in that aspect.

Sure. It's just a suggestion. Once the ABI is set it isn't supposed to
change so I'm just trying to think ahead.

Also there has been discussion on marking bindings as "unstable". I
don't know if you are interested in that but maybe putting a line at the
top of the binding stating something like:

Status: Unstable - ABI compatibility may be broken in the future

Regards,
Mike

> 
> Regards,
> Santosh

[PATCH 1/8] clk: keystone: add Keystone PLL clock driver

[Santosh Shilimkar]

On Tuesday 20 August 2013 05:23 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 06:41:21)
>> On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
>>> Quoting Santosh Shilimkar (2013-08-19 10:42:19)
>>>> Mark,
>>>>
>>>> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>>>>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>>>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>>>>>> [Adding dt maintainers]
>>>>>>>>
>>>>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>>>>>
>>>>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>>>>> ---
>>>>>>>
>>>>>>> Thanks for review Mark.
>>>>>>>
>>>>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>>>>>  3 files changed, 251 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..58f6470
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>>>>>> +'pll_has_pllctrl' property.
>>>>>>>>> +
>>>>>>>>> +This binding uses the common clock binding[1].
>>>>>>>>> +
>>>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>>>>>
>>>>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>>>>>> rather than "clk".
>>>>>>>>
>>>>>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>>>>>
>>>>>>> Agree.
>>>>>>>
>>>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>>>> +- clocks : parent clock phandle
>>>>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
>>>>>>>>
>>>>>>>> Perhaps a reg-names would be useful?
>>>>>>>>
>>>>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>>>>>
>>>>>>>> Huh? I don't understand what that description means. What does the
>>>>>>>> property tell you? Is having one of the registers optional? If so that
>>>>>>>> should be described by a reg-names property associated with the reg, and
>>>>>>>> should be noted in the binding.
>>>>>>>>
>>>>>>> After re-reading it, yes I agree it not clear. The point is there
>>>>>>> are two different types of IPs and pogramming model changes quite
>>>>>>> a bit. Its not just 1 register optional.
>>>>>>
>>>>>> If that's the case, then having different compatible strings would make
>>>>>> sense.
>>>>>>
>>>>>>>
>>>>>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>>>>>> +- plld_mask - plld mask
>>>>>>>>> +- fixed_postdiv - fixed post divider value
>>>>>>>>
>>>>>>>> Please use '-' rather than '_' in property names, it's a standard
>>>>>>>> convention for dt and going against it just makes things unnecessarily
>>>>>>>> complicated.
>>>>>>>>
>>>>>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>>>>>> just vary wildly?
>>>>>>>>
>>>>>>> This is mainly to take care of the programming model which varies between
>>>>>>> IPs. Will try to make that bit more clear.
>>>>>>
>>>>>> Are there more than the two IPs mentioned above? If they had separate
>>>>>> compatible strings, would that give enough information implicitly,
>>>>>> without the precise masks needing to be in the dt?
>>>>>>
>>>>> I will explore the separate compatible option. Thanks for suggestion.
>>>>>
>>>> I looked at further into separate compatible option or reg-names
>>>> to check if it can help to reduce some additional dt information.
>>>> Actually it doesn't help much. The base programming model is shared
>>>> between both types of PLL IPs. The PLLs which has PLL controller along
>>>> with MMRs, has to take into account additional bit-fields for the
>>>> multiplier and divider along with the base model multiplier and divider
>>>> registers.
>>>
>>> It is common for the base programming model to be similar or shared
>>> between two different compatible strings. You can use the same clk_ops
>>> for clk_prepare, clk_enable, etc.
>>>
>>> The point is to use the different compatible strings to encode the
>>> differences in *data* between the two clock types. That way you have
>>> fewer properties to list in the binding since two separate setup
>>> functions can implicitly handle the differences in initializing the
>>> per-clock data.
>>>
>> Thats the point I came back. Both PLL share the properties and
>> the main PLL brings in couple of properties to extend the divider
>> field and that was handled through the flag. As mentioned below,
>> some properties like plld_mask, pllm_upper_shift etc can be
>> dropped since they are static values.
>>
>>>>
>>>> Having said that, there are few parameters which are fixed like
>>>> plld_mask, pllm_upper_shift etc need not come from DT. I am going
>>>> to remove them from dt bindings and also make the reg index consistent
>>>> as it should have been in first place.
>>>
>>> This is nice. Note that these things may change in future Keystone
>>> versions because hardware people hate you and want to make your life
>>> hard. So the compatible string might benefit from including the first
>>> keystone part number that uses this binding (e.g.
>>> ti,keystone-2420-pll-clock, which I just made up).
>>>
>>> In the future when the register layout, offsets and masks change for
>>> absolutely no reason (but the programming model is the same) then you
>>> can just write a new binding that setups up your private clk_pll_data
>>> struct without having to jam all of those details into DT (e.g.
>>> ti,keystone-3430-pll-clock, which I also just made up).
>>>
>> So thats the good part so far with the keystone where the compatibility
>> is maintained pretty well. I know background of your comments ;-)
>> I will keep the bindings without any direct device association for
>> now considering I don't foresee any thing changing in that aspect.
> 
> Sure. It's just a suggestion. Once the ABI is set it isn't supposed to
> change so I'm just trying to think ahead.
> 
I will also spend some more time on binding so that we don't miss anything.

> Also there has been discussion on marking bindings as "unstable". I
> don't know if you are interested in that but maybe putting a line at the
> top of the binding stating something like:
> 
> Status: Unstable - ABI compatibility may be broken in the future
> 
OK. Will do that.

regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

Add the driver for the clock gate control which uses PSC (Power Sleep
Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
disabling of the clocks for different IPs present in the SoC.

Cc: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
 drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
 include/linux/clk/keystone.h                       |    1 +
 3 files changed, 337 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
 create mode 100644 drivers/clk/keystone/gate.c

diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
new file mode 100644
index 0000000..40afef5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
@@ -0,0 +1,30 @@
+Binding for Keystone gate control driver which uses PSC controller IP.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "keystone,psc-clk".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : parent clock phandle
+- reg :	psc address address space
+
+Optional properties:
+- clock-output-names : From common clock binding to override the
+			default output clock name
+- status : "enabled" if clock is always enabled
+- lpsc : lpsc module id, if not set defaults to zero
+- pd : power domain number, if not set defaults to zero (always ON)
+- gpsc : gpsc number, if not set defaults to zero
+
+Example:
+	clock {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk3>;
+		clock-output-names = "debugss_trc";
+		reg = <0x02350000 4096>;
+		lpsc = <5>;
+		pd = <1>;
+	};
diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
new file mode 100644
index 0000000..72ac478
--- /dev/null
+++ b/drivers/clk/keystone/gate.c
@@ -0,0 +1,306 @@
+/*
+ * Clock driver for Keystone 2 based devices
+ *
+ * Copyright (C) 2013 Texas Instruments.
+ *	Murali Karicheri <m-karicheri2@ti.com>
+ *	Santosh Shilimkar <santosh.shilimkar@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/module.h>
+
+/* PSC register offsets */
+#define PTCMD			0x120
+#define PTSTAT			0x128
+#define PDSTAT			0x200
+#define PDCTL			0x300
+#define MDSTAT			0x800
+#define MDCTL			0xa00
+
+/* PSC module states */
+#define PSC_STATE_SWRSTDISABLE	0
+#define PSC_STATE_SYNCRST	1
+#define PSC_STATE_DISABLE	2
+#define PSC_STATE_ENABLE	3
+
+#define MDSTAT_STATE_MASK	0x3f
+#define MDSTAT_MCKOUT		BIT(12)
+#define PDSTAT_STATE_MASK	0x1f
+#define MDCTL_FORCE		BIT(31)
+#define MDCTL_LRESET		BIT(8)
+#define PDCTL_NEXT		BIT(0)
+
+/* PSC flags. Disable state is SwRstDisable */
+#define PSC_SWRSTDISABLE	BIT(0)
+/* Force module state transtition */
+#define PSC_FORCE		BIT(1)
+/* Keep module in local reset */
+#define PSC_LRESET		BIT(2)
+#define NUM_GPSC		2
+#define REG_OFFSET		4
+
+/**
+ * struct clk_psc_data - PSC data
+ * @base: Base address for a PSC
+ * @flags: framework flags
+ * @lpsc: Is it local PSC
+ * @gpsc: Is it global PSC
+ * @domain: PSC domain
+ *
+ */
+struct clk_psc_data {
+	void __iomem *base;
+	u32	flags;
+	u32	psc_flags;
+	u8	lpsc;
+	u8	gpsc;
+	u8	domain;
+};
+
+/**
+ * struct clk_psc - PSC clock structure
+ * @hw: clk_hw for the psc
+ * @psc_data: PSC driver specific data
+ * @lock: Spinlock used by the driver
+ */
+struct clk_psc {
+	struct clk_hw hw;
+	struct clk_psc_data *psc_data;
+	spinlock_t *lock;
+};
+
+struct reg_psc {
+	u32 phy_base;
+	u32 size;
+	void __iomem *io_base;
+};
+
+static struct reg_psc psc_addr[NUM_GPSC];
+static DEFINE_SPINLOCK(psc_lock);
+
+#define to_clk_psc(_hw) container_of(_hw, struct clk_psc, hw)
+
+static void psc_config(void __iomem *psc_base, unsigned int domain,
+		unsigned int id, bool enable, u32 flags)
+{
+	u32 ptcmd, pdstat, pdctl, mdstat, mdctl, next_state;
+
+	if (enable)
+		next_state = PSC_STATE_ENABLE;
+	else if (flags & PSC_SWRSTDISABLE)
+		next_state = PSC_STATE_SWRSTDISABLE;
+	else
+		next_state = PSC_STATE_DISABLE;
+
+	mdctl = readl(psc_base + MDCTL + REG_OFFSET * id);
+	mdctl &= ~MDSTAT_STATE_MASK;
+	mdctl |= next_state;
+	if (flags & PSC_FORCE)
+		mdctl |= MDCTL_FORCE;
+	if (flags & PSC_LRESET)
+		mdctl &= ~MDCTL_LRESET;
+	writel(mdctl, psc_base + MDCTL + REG_OFFSET * id);
+
+	pdstat = readl(psc_base + PDSTAT + REG_OFFSET * domain);
+	if (!(pdstat & PDSTAT_STATE_MASK)) {
+		pdctl = readl(psc_base + PDCTL + REG_OFFSET * domain);
+		pdctl |= PDCTL_NEXT;
+		writel(pdctl, psc_base + PDCTL + REG_OFFSET * domain);
+	}
+
+	ptcmd = 1 << domain;
+	writel(ptcmd, psc_base + PTCMD);
+	while ((readl(psc_base + PTSTAT) >> domain) & 1)
+		;
+
+	do {
+		mdstat = readl(psc_base + MDSTAT + REG_OFFSET * id);
+	} while (!((mdstat & MDSTAT_STATE_MASK) == next_state));
+}
+
+static int keystone_clk_is_enabled(struct clk_hw *hw)
+{
+	struct clk_psc *psc = to_clk_psc(hw);
+	struct clk_psc_data *data = psc->psc_data;
+	u32 mdstat = readl(data->base + MDSTAT + REG_OFFSET * data->lpsc);
+
+	return (mdstat & MDSTAT_MCKOUT) ? 1 : 0;
+}
+
+static int keystone_clk_enable(struct clk_hw *hw)
+{
+	struct clk_psc *psc = to_clk_psc(hw);
+	struct clk_psc_data *data = psc->psc_data;
+	unsigned long flags = 0;
+
+	if (psc->lock)
+		spin_lock_irqsave(psc->lock, flags);
+
+	psc_config(data->base, data->domain, data->lpsc, 1, data->psc_flags);
+
+	if (psc->lock)
+		spin_unlock_irqrestore(psc->lock, flags);
+
+	return 0;
+}
+
+static void keystone_clk_disable(struct clk_hw *hw)
+{
+	struct clk_psc *psc = to_clk_psc(hw);
+	struct clk_psc_data *data = psc->psc_data;
+	unsigned long flags = 0;
+
+	if (psc->lock)
+		spin_lock_irqsave(psc->lock, flags);
+
+	psc_config(data->base, data->domain, data->lpsc, 0, data->psc_flags);
+
+	if (psc->lock)
+		spin_unlock_irqrestore(psc->lock, flags);
+}
+
+static const struct clk_ops clk_psc_ops = {
+	.enable = keystone_clk_enable,
+	.disable = keystone_clk_disable,
+	.is_enabled = keystone_clk_is_enabled,
+};
+
+/**
+ * clk_register_psc - register psc clock
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @parent_name: name of clock's parent
+ * @psc_data: platform data to configure this clock
+ * @lock: spinlock used by this clock
+ */
+static struct clk *clk_register_psc(struct device *dev,
+			const char *name,
+			const char *parent_name,
+			struct clk_psc_data *psc_data,
+			spinlock_t *lock)
+{
+	struct clk_init_data init;
+	struct clk_psc *psc;
+	struct clk *clk;
+
+	psc = kzalloc(sizeof(*psc), GFP_KERNEL);
+	if (!psc)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_psc_ops;
+	init.flags = psc_data->flags;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	psc->psc_data = psc_data;
+	psc->lock = lock;
+	psc->hw.init = &init;
+
+	clk = clk_register(NULL, &psc->hw);
+	if (IS_ERR(clk))
+		kfree(psc);
+
+	return clk;
+}
+
+/**
+ * of_psc_clk_init - initialize psc clock through DT
+ * @node: device tree node for this clock
+ * @lock: spinlock used by this clock
+ */
+static void __init of_psc_clk_init(struct device_node *node, spinlock_t *lock)
+{
+	const char *parent_name, *status = NULL, *base_flags = NULL;
+	struct clk_psc_data *data;
+	const char *clk_name = node->name;
+	u32 gpsc = 0, lpsc = 0, pd = 0;
+	struct resource res;
+	struct clk *clk;
+	int rc;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	WARN_ON(!data);
+
+	if (of_address_to_resource(node, 0, &res)) {
+		pr_err("psc_clk_init - no reg property defined\n");
+		goto out;
+	}
+
+	of_property_read_u32(node, "gpsc", &gpsc);
+	of_property_read_u32(node, "lpsc", &lpsc);
+	of_property_read_u32(node, "pd", &pd);
+
+	if (gpsc >= NUM_GPSC) {
+		pr_err("psc_clk_init - no reg property defined\n");
+		goto out;
+	}
+
+	of_property_read_string(node,
+			"clock-output-names", &clk_name);
+	parent_name = of_clk_get_parent_name(node, 0);
+	WARN_ON(!parent_name);
+
+	/* Expected that same phy_base is used for all psc clocks of
+	 * a give gpsc. So ioremap is done only once.
+	 */
+	if (psc_addr[gpsc].phy_base) {
+		if (psc_addr[gpsc].phy_base != res.start) {
+			pr_err("Different psc base for same GPSC\n");
+			goto out;
+		}
+	} else {
+		psc_addr[gpsc].phy_base = res.start;
+		psc_addr[gpsc].io_base =
+			ioremap(res.start, resource_size(&res));
+	}
+
+	WARN_ON(!psc_addr[gpsc].io_base);
+	data->base = psc_addr[gpsc].io_base;
+	data->lpsc = lpsc;
+	data->gpsc = gpsc;
+	data->domain = pd;
+
+	if (of_property_read_bool(node, "ti,psc-lreset"))
+		data->psc_flags |= PSC_LRESET;
+	if (of_property_read_bool(node, "ti,psc-force"))
+		data->psc_flags |= PSC_FORCE;
+
+	clk = clk_register_psc(NULL, clk_name, parent_name,
+				data, lock);
+
+	if (clk) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+		rc = of_property_read_string(node, "status", &status);
+		if (status && !strcmp(status, "enabled"))
+			clk_prepare_enable(clk);
+		return;
+	}
+	pr_err("psc_clk_init - error registering psc clk %s\n", node->name);
+out:
+	kfree(data);
+	return;
+}
+
+/**
+ * of_keystone_psc_clk_init - initialize psc clock through DT
+ * @node: device tree node for this clock
+ */
+void __init of_keystone_psc_clk_init(struct device_node *node)
+{
+	of_psc_clk_init(node, &psc_lock);
+}
+EXPORT_SYMBOL_GPL(of_keystone_psc_clk_init);
+CLK_OF_DECLARE(keystone_gate_clk, "keystone,psc-clk", of_keystone_psc_clk_init);
diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
index 1ade95d..7b3e154 100644
--- a/include/linux/clk/keystone.h
+++ b/include/linux/clk/keystone.h
@@ -14,5 +14,6 @@
 #define __LINUX_CLK_KEYSTONE_H_
 
 extern void of_keystone_pll_clk_init(struct device_node *node);
+extern void of_keystone_psc_clk_init(struct device_node *node);
 
 #endif /* __LINUX_CLK_KEYSTONE_H_ */
-- 
1.7.9.5

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mark Rutland]

[adding dt maintainers]

On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> Add the driver for the clock gate control which uses PSC (Power Sleep
> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> disabling of the clocks for different IPs present in the SoC.
> 
> Cc: Mike Turquette <mturquette@linaro.org>
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
>  include/linux/clk/keystone.h                       |    1 +
>  3 files changed, 337 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
>  create mode 100644 drivers/clk/keystone/gate.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> new file mode 100644
> index 0000000..40afef5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> @@ -0,0 +1,30 @@
> +Binding for Keystone gate control driver which uses PSC controller IP.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "keystone,psc-clk".

Similarly to my comments on patch 1, this should probably be something
more like "ti,keystone-psc-clock"

> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : parent clock phandle
> +- reg :        psc address address space
> +
> +Optional properties:
> +- clock-output-names : From common clock binding to override the
> +                       default output clock name
> +- status : "enabled" if clock is always enabled

Huh? This is a standard property, for which ePAPR defines "okay",
"disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
seem to follow the standard meaning judging by the code.

It looks like this could be a boolean property
("clock-always-enabled"?), but that assumes it's necessary.

Why do you need this?

> +- lpsc : lpsc module id, if not set defaults to zero

What's that needed for? Are there multiple clocks sharing a common
register bank?

> +- pd : power domain number, if not set defaults to zero (always ON)

Please don't use abbreviations unnecessarily. "power-domain-id" or
similar would be far better. What exactly does this represent? Does the
clock IP itself handle power domains, or is there some external unit
that controls power domains?

> +- gpsc : gpsc number, if not set defaults to zero

How does that interact with the lpsc property? When are these necessary?

> +
> +Example:
> +       clock {
> +               #clock-cells = <0>;
> +               compatible = "keystone,psc-clk";
> +               clocks = <&chipclk3>;
> +               clock-output-names = "debugss_trc";
> +               reg = <0x02350000 4096>;
> +               lpsc = <5>;
> +               pd = <1>;
> +       };
> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
> new file mode 100644
> index 0000000..72ac478
> --- /dev/null
> +++ b/drivers/clk/keystone/gate.c
> @@ -0,0 +1,306 @@
> +/*
> + * Clock driver for Keystone 2 based devices
> + *
> + * Copyright (C) 2013 Texas Instruments.
> + *     Murali Karicheri <m-karicheri2@ti.com>
> + *     Santosh Shilimkar <santosh.shilimkar@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +
> +/* PSC register offsets */
> +#define PTCMD                  0x120
> +#define PTSTAT                 0x128
> +#define PDSTAT                 0x200
> +#define PDCTL                  0x300
> +#define MDSTAT                 0x800
> +#define MDCTL                  0xa00
> +
> +/* PSC module states */
> +#define PSC_STATE_SWRSTDISABLE 0
> +#define PSC_STATE_SYNCRST      1
> +#define PSC_STATE_DISABLE      2
> +#define PSC_STATE_ENABLE       3
> +
> +#define MDSTAT_STATE_MASK      0x3f
> +#define MDSTAT_MCKOUT          BIT(12)
> +#define PDSTAT_STATE_MASK      0x1f
> +#define MDCTL_FORCE            BIT(31)
> +#define MDCTL_LRESET           BIT(8)
> +#define PDCTL_NEXT             BIT(0)
> +
> +/* PSC flags. Disable state is SwRstDisable */
> +#define PSC_SWRSTDISABLE       BIT(0)
> +/* Force module state transtition */
> +#define PSC_FORCE              BIT(1)
> +/* Keep module in local reset */
> +#define PSC_LRESET             BIT(2)
> +#define NUM_GPSC               2
> +#define REG_OFFSET             4
> +
> +/**
> + * struct clk_psc_data - PSC data
> + * @base: Base address for a PSC
> + * @flags: framework flags
> + * @lpsc: Is it local PSC
> + * @gpsc: Is it global PSC
> + * @domain: PSC domain
> + *
> + */
> +struct clk_psc_data {
> +       void __iomem *base;
> +       u32     flags;
> +       u32     psc_flags;
> +       u8      lpsc;
> +       u8      gpsc;
> +       u8      domain;
> +};
> +
> +/**
> + * struct clk_psc - PSC clock structure
> + * @hw: clk_hw for the psc
> + * @psc_data: PSC driver specific data
> + * @lock: Spinlock used by the driver
> + */
> +struct clk_psc {
> +       struct clk_hw hw;
> +       struct clk_psc_data *psc_data;
> +       spinlock_t *lock;
> +};
> +
> +struct reg_psc {
> +       u32 phy_base;
> +       u32 size;
> +       void __iomem *io_base;
> +};
> +
> +static struct reg_psc psc_addr[NUM_GPSC];
> +static DEFINE_SPINLOCK(psc_lock);
> +
> +#define to_clk_psc(_hw) container_of(_hw, struct clk_psc, hw)
> +
> +static void psc_config(void __iomem *psc_base, unsigned int domain,
> +               unsigned int id, bool enable, u32 flags)
> +{
> +       u32 ptcmd, pdstat, pdctl, mdstat, mdctl, next_state;
> +
> +       if (enable)
> +               next_state = PSC_STATE_ENABLE;
> +       else if (flags & PSC_SWRSTDISABLE)
> +               next_state = PSC_STATE_SWRSTDISABLE;
> +       else
> +               next_state = PSC_STATE_DISABLE;
> +
> +       mdctl = readl(psc_base + MDCTL + REG_OFFSET * id);
> +       mdctl &= ~MDSTAT_STATE_MASK;
> +       mdctl |= next_state;
> +       if (flags & PSC_FORCE)
> +               mdctl |= MDCTL_FORCE;
> +       if (flags & PSC_LRESET)
> +               mdctl &= ~MDCTL_LRESET;
> +       writel(mdctl, psc_base + MDCTL + REG_OFFSET * id);
> +
> +       pdstat = readl(psc_base + PDSTAT + REG_OFFSET * domain);
> +       if (!(pdstat & PDSTAT_STATE_MASK)) {
> +               pdctl = readl(psc_base + PDCTL + REG_OFFSET * domain);
> +               pdctl |= PDCTL_NEXT;
> +               writel(pdctl, psc_base + PDCTL + REG_OFFSET * domain);
> +       }
> +
> +       ptcmd = 1 << domain;
> +       writel(ptcmd, psc_base + PTCMD);
> +       while ((readl(psc_base + PTSTAT) >> domain) & 1)
> +               ;
> +
> +       do {
> +               mdstat = readl(psc_base + MDSTAT + REG_OFFSET * id);
> +       } while (!((mdstat & MDSTAT_STATE_MASK) == next_state));
> +}
> +
> +static int keystone_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_psc *psc = to_clk_psc(hw);
> +       struct clk_psc_data *data = psc->psc_data;
> +       u32 mdstat = readl(data->base + MDSTAT + REG_OFFSET * data->lpsc);
> +
> +       return (mdstat & MDSTAT_MCKOUT) ? 1 : 0;
> +}
> +
> +static int keystone_clk_enable(struct clk_hw *hw)
> +{
> +       struct clk_psc *psc = to_clk_psc(hw);
> +       struct clk_psc_data *data = psc->psc_data;
> +       unsigned long flags = 0;
> +
> +       if (psc->lock)
> +               spin_lock_irqsave(psc->lock, flags);
> +
> +       psc_config(data->base, data->domain, data->lpsc, 1, data->psc_flags);
> +
> +       if (psc->lock)
> +               spin_unlock_irqrestore(psc->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void keystone_clk_disable(struct clk_hw *hw)
> +{
> +       struct clk_psc *psc = to_clk_psc(hw);
> +       struct clk_psc_data *data = psc->psc_data;
> +       unsigned long flags = 0;
> +
> +       if (psc->lock)
> +               spin_lock_irqsave(psc->lock, flags);
> +
> +       psc_config(data->base, data->domain, data->lpsc, 0, data->psc_flags);
> +
> +       if (psc->lock)
> +               spin_unlock_irqrestore(psc->lock, flags);
> +}
> +
> +static const struct clk_ops clk_psc_ops = {
> +       .enable = keystone_clk_enable,
> +       .disable = keystone_clk_disable,
> +       .is_enabled = keystone_clk_is_enabled,
> +};
> +
> +/**
> + * clk_register_psc - register psc clock
> + * @dev: device that is registering this clock
> + * @name: name of this clock
> + * @parent_name: name of clock's parent
> + * @psc_data: platform data to configure this clock
> + * @lock: spinlock used by this clock
> + */
> +static struct clk *clk_register_psc(struct device *dev,
> +                       const char *name,
> +                       const char *parent_name,
> +                       struct clk_psc_data *psc_data,
> +                       spinlock_t *lock)
> +{
> +       struct clk_init_data init;
> +       struct clk_psc *psc;
> +       struct clk *clk;
> +
> +       psc = kzalloc(sizeof(*psc), GFP_KERNEL);
> +       if (!psc)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       init.ops = &clk_psc_ops;
> +       init.flags = psc_data->flags;
> +       init.parent_names = (parent_name ? &parent_name : NULL);

Is &parent_name not a pointer to a pointer on the stack? Is this only
used once?

> +       init.num_parents = (parent_name ? 1 : 0);
> +
> +       psc->psc_data = psc_data;
> +       psc->lock = lock;
> +       psc->hw.init = &init;

That's definitely a pointer to some data on the stack, and that seems to
be kept around. Is this only used for one-time initialisation, or might
it be used later?

> +
> +       clk = clk_register(NULL, &psc->hw);
> +       if (IS_ERR(clk))
> +               kfree(psc);
> +
> +       return clk;
> +}
> +
> +/**
> + * of_psc_clk_init - initialize psc clock through DT
> + * @node: device tree node for this clock
> + * @lock: spinlock used by this clock
> + */
> +static void __init of_psc_clk_init(struct device_node *node, spinlock_t *lock)
> +{
> +       const char *parent_name, *status = NULL, *base_flags = NULL;
> +       struct clk_psc_data *data;
> +       const char *clk_name = node->name;
> +       u32 gpsc = 0, lpsc = 0, pd = 0;
> +       struct resource res;
> +       struct clk *clk;
> +       int rc;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       WARN_ON(!data);

Does that not mean things will blow up later? return now?

> +
> +       if (of_address_to_resource(node, 0, &res)) {
> +               pr_err("psc_clk_init - no reg property defined\n");
> +               goto out;
> +       }
> +
> +       of_property_read_u32(node, "gpsc", &gpsc);
> +       of_property_read_u32(node, "lpsc", &lpsc);
> +       of_property_read_u32(node, "pd", &pd);
> +
> +       if (gpsc >= NUM_GPSC) {
> +               pr_err("psc_clk_init - no reg property defined\n");

Wrong error message.

> +               goto out;
> +       }
> +
> +       of_property_read_string(node,
> +                       "clock-output-names", &clk_name);
> +       parent_name = of_clk_get_parent_name(node, 0);
> +       WARN_ON(!parent_name);

Do you require the parent name? If so you need to fail gracefully, if
not you don't need the WARN_ON (perhaps a pr_warn would be better?).

> +
> +       /* Expected that same phy_base is used for all psc clocks of
> +        * a give gpsc. So ioremap is done only once.
> +        */

So these clocks are all components of a larger IP block? It might make
more sense to describe the containing block, with the actual clocks as
sub-nodes (or if the set of clocks for the containing block is known,
just the containing block).

> +       if (psc_addr[gpsc].phy_base) {
> +               if (psc_addr[gpsc].phy_base != res.start) {
> +                       pr_err("Different psc base for same GPSC\n");
> +                       goto out;
> +               }
> +       } else {
> +               psc_addr[gpsc].phy_base = res.start;
> +               psc_addr[gpsc].io_base =
> +                       ioremap(res.start, resource_size(&res));
> +       }
> +
> +       WARN_ON(!psc_addr[gpsc].io_base);

Surely things will blow up later if that's the case? return here instead?

> +       data->base = psc_addr[gpsc].io_base;
> +       data->lpsc = lpsc;
> +       data->gpsc = gpsc;
> +       data->domain = pd;
> +
> +       if (of_property_read_bool(node, "ti,psc-lreset"))
> +               data->psc_flags |= PSC_LRESET;
> +       if (of_property_read_bool(node, "ti,psc-force"))
> +               data->psc_flags |= PSC_FORCE;

Neither of these were defined in the binding, and they don't appear to
have been inherited form another binding. What are they for? Why are
they needed?

> +
> +       clk = clk_register_psc(NULL, clk_name, parent_name,
> +                               data, lock);
> +
> +       if (clk) {
> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> +               rc = of_property_read_string(node, "status", &status);
> +               if (status && !strcmp(status, "enabled"))
> +                       clk_prepare_enable(clk);
> +               return;
> +       }

As mentioned above, this abuses the standard status property, and it's
not clear why this is necessary.

Thanks,
Mark.

> +       pr_err("psc_clk_init - error registering psc clk %s\n", node->name);
> +out:
> +       kfree(data);
> +       return;
> +}
> +
> +/**
> + * of_keystone_psc_clk_init - initialize psc clock through DT
> + * @node: device tree node for this clock
> + */
> +void __init of_keystone_psc_clk_init(struct device_node *node)
> +{
> +       of_psc_clk_init(node, &psc_lock);
> +}
> +EXPORT_SYMBOL_GPL(of_keystone_psc_clk_init);
> +CLK_OF_DECLARE(keystone_gate_clk, "keystone,psc-clk", of_keystone_psc_clk_init);
> diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
> index 1ade95d..7b3e154 100644
> --- a/include/linux/clk/keystone.h
> +++ b/include/linux/clk/keystone.h
> @@ -14,5 +14,6 @@
>  #define __LINUX_CLK_KEYSTONE_H_
> 
>  extern void of_keystone_pll_clk_init(struct device_node *node);
> +extern void of_keystone_psc_clk_init(struct device_node *node);
> 
>  #endif /* __LINUX_CLK_KEYSTONE_H_ */
> --
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> [adding dt maintainers]
> 
> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
>> Add the driver for the clock gate control which uses PSC (Power Sleep
>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
>> disabling of the clocks for different IPs present in the SoC.
>>
>> Cc: Mike Turquette <mturquette@linaro.org>
>>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
>>  include/linux/clk/keystone.h                       |    1 +
>>  3 files changed, 337 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
>>  create mode 100644 drivers/clk/keystone/gate.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>> new file mode 100644
>> index 0000000..40afef5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>> @@ -0,0 +1,30 @@
>> +Binding for Keystone gate control driver which uses PSC controller IP.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible : shall be "keystone,psc-clk".
> 
> Similarly to my comments on patch 1, this should probably be something
> more like "ti,keystone-psc-clock"
> 
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : parent clock phandle
>> +- reg :        psc address address space
>> +
>> +Optional properties:
>> +- clock-output-names : From common clock binding to override the
>> +                       default output clock name
>> +- status : "enabled" if clock is always enabled
> 
> Huh? This is a standard property, for which ePAPR defines "okay",
> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> seem to follow the standard meaning judging by the code.
> 
> It looks like this could be a boolean property
> ("clock-always-enabled"?), but that assumes it's necessary.
> 
> Why do you need this?
> 
I dropped this already. Forgot to update the documentation.

>> +- lpsc : lpsc module id, if not set defaults to zero
> 
> What's that needed for? Are there multiple clocks sharing a common
> register bank?
> 
>> +- pd : power domain number, if not set defaults to zero (always ON)
> 
> Please don't use abbreviations unnecessarily. "power-domain-id" or
> similar would be far better. What exactly does this represent? Does the
> clock IP itself handle power domains, or is there some external unit
> that controls power domains?
> 
pd is commonly used but I can expand it. This represent the power
domain number.

>> +- gpsc : gpsc number, if not set defaults to zero
> 
> How does that interact with the lpsc property? When are these necessary?
> 
lpsc is local clock/module control where as gpsc sits on top of it. 

>> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
>> new file mode 100644
>> index 0000000..72ac478
>> --- /dev/null
>> +++ b/drivers/clk/keystone/gate.c

[..]

>> +/**
>> + * clk_register_psc - register psc clock
>> + * @dev: device that is registering this clock
>> + * @name: name of this clock
>> + * @parent_name: name of clock's parent
>> + * @psc_data: platform data to configure this clock
>> + * @lock: spinlock used by this clock
>> + */
>> +static struct clk *clk_register_psc(struct device *dev,
>> +                       const char *name,
>> +                       const char *parent_name,
>> +                       struct clk_psc_data *psc_data,
>> +                       spinlock_t *lock)
>> +{
>> +       struct clk_init_data init;
>> +       struct clk_psc *psc;
>> +       struct clk *clk;
>> +
>> +       psc = kzalloc(sizeof(*psc), GFP_KERNEL);
>> +       if (!psc)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       init.name = name;
>> +       init.ops = &clk_psc_ops;
>> +       init.flags = psc_data->flags;
>> +       init.parent_names = (parent_name ? &parent_name : NULL);
> 
> Is &parent_name not a pointer to a pointer on the stack? Is this only
> used once?
> 
>> +       init.num_parents = (parent_name ? 1 : 0);
>> +
>> +       psc->psc_data = psc_data;
>> +       psc->lock = lock;
>> +       psc->hw.init = &init;
> 
> That's definitely a pointer to some data on the stack, and that seems to
> be kept around. Is this only used for one-time initialisation, or might
> it be used later?
> 
Its init only.

>> +       data->base = psc_addr[gpsc].io_base;
>> +       data->lpsc = lpsc;
>> +       data->gpsc = gpsc;
>> +       data->domain = pd;
>> +
>> +       if (of_property_read_bool(node, "ti,psc-lreset"))
>> +               data->psc_flags |= PSC_LRESET;
>> +       if (of_property_read_bool(node, "ti,psc-force"))
>> +               data->psc_flags |= PSC_FORCE;
> 
> Neither of these were defined in the binding, and they don't appear to
> have been inherited form another binding. What are they for? Why are
> they needed?
> 
They represent the hardware support transition method needed to have
proper clock enable/disable sequence to work. Will update the binding
along with other comments which I agree.

>> +
>> +       clk = clk_register_psc(NULL, clk_name, parent_name,
>> +                               data, lock);
>> +
>> +       if (clk) {
>> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +
>> +               rc = of_property_read_string(node, "status", &status);
>> +               if (status && !strcmp(status, "enabled"))
>> +                       clk_prepare_enable(clk);
>> +               return;
>> +       }
> 
> As mentioned above, this abuses the standard status property, and it's
> not clear why this is necessary.
> 
Actually I have removed this one from dt nodes. Looks like missed
to pick the right patch while posting. Have dropped this change
already.

Regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mark Rutland]

On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> > [adding dt maintainers]
> > 
> > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> >> Add the driver for the clock gate control which uses PSC (Power Sleep
> >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> >> disabling of the clocks for different IPs present in the SoC.
> >>
> >> Cc: Mike Turquette <mturquette@linaro.org>
> >>
> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> ---
> >>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
> >>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
> >>  include/linux/clk/keystone.h                       |    1 +
> >>  3 files changed, 337 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>  create mode 100644 drivers/clk/keystone/gate.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >> new file mode 100644
> >> index 0000000..40afef5
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >> @@ -0,0 +1,30 @@
> >> +Binding for Keystone gate control driver which uses PSC controller IP.
> >> +
> >> +This binding uses the common clock binding[1].
> >> +
> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible : shall be "keystone,psc-clk".
> > 
> > Similarly to my comments on patch 1, this should probably be something
> > more like "ti,keystone-psc-clock"
> > 
> >> +- #clock-cells : from common clock binding; shall be set to 0.
> >> +- clocks : parent clock phandle
> >> +- reg :        psc address address space
> >> +
> >> +Optional properties:
> >> +- clock-output-names : From common clock binding to override the
> >> +                       default output clock name
> >> +- status : "enabled" if clock is always enabled
> > 
> > Huh? This is a standard property, for which ePAPR defines "okay",
> > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> > seem to follow the standard meaning judging by the code.
> > 
> > It looks like this could be a boolean property
> > ("clock-always-enabled"?), but that assumes it's necessary.
> > 
> > Why do you need this?
> > 
> I dropped this already. Forgot to update the documentation.

Ok.

> 
> >> +- lpsc : lpsc module id, if not set defaults to zero
> > 
> > What's that needed for? Are there multiple clocks sharing a common
> > register bank?
> > 
> >> +- pd : power domain number, if not set defaults to zero (always ON)
> > 
> > Please don't use abbreviations unnecessarily. "power-domain-id" or
> > similar would be far better. What exactly does this represent? Does the
> > clock IP itself handle power domains, or is there some external unit
> > that controls power domains?
> > 
> pd is commonly used but I can expand it. This represent the power
> domain number.

Does the the clock IP have some internal logic for handling power
domains only covering its clocks, or is there some external power
controller involved (i.e. do we possibly need to describe an external
unit and a linkage to it?).

> 
> >> +- gpsc : gpsc number, if not set defaults to zero
> > 
> > How does that interact with the lpsc property? When are these necessary?
> > 
> lpsc is local clock/module control where as gpsc sits on top of it. 

Ok. I'm not sure haivng these default to zero makes much sense, as that
could easily hide errors in dts. It might make more sense to make them
required and error out if they aren't present.

Unless they're zero far more often?

> 
> >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
> >> new file mode 100644
> >> index 0000000..72ac478
> >> --- /dev/null
> >> +++ b/drivers/clk/keystone/gate.c
> 
> [..]
> 
> >> +/**
> >> + * clk_register_psc - register psc clock
> >> + * @dev: device that is registering this clock
> >> + * @name: name of this clock
> >> + * @parent_name: name of clock's parent
> >> + * @psc_data: platform data to configure this clock
> >> + * @lock: spinlock used by this clock
> >> + */
> >> +static struct clk *clk_register_psc(struct device *dev,
> >> +                       const char *name,
> >> +                       const char *parent_name,
> >> +                       struct clk_psc_data *psc_data,
> >> +                       spinlock_t *lock)
> >> +{
> >> +       struct clk_init_data init;
> >> +       struct clk_psc *psc;
> >> +       struct clk *clk;
> >> +
> >> +       psc = kzalloc(sizeof(*psc), GFP_KERNEL);
> >> +       if (!psc)
> >> +               return ERR_PTR(-ENOMEM);
> >> +
> >> +       init.name = name;
> >> +       init.ops = &clk_psc_ops;
> >> +       init.flags = psc_data->flags;
> >> +       init.parent_names = (parent_name ? &parent_name : NULL);
> > 
> > Is &parent_name not a pointer to a pointer on the stack? Is this only
> > used once?
> > 
> >> +       init.num_parents = (parent_name ? 1 : 0);
> >> +
> >> +       psc->psc_data = psc_data;
> >> +       psc->lock = lock;
> >> +       psc->hw.init = &init;
> > 
> > That's definitely a pointer to some data on the stack, and that seems to
> > be kept around. Is this only used for one-time initialisation, or might
> > it be used later?
> > 
> Its init only.

Ok.

> 
> >> +       data->base = psc_addr[gpsc].io_base;
> >> +       data->lpsc = lpsc;
> >> +       data->gpsc = gpsc;
> >> +       data->domain = pd;
> >> +
> >> +       if (of_property_read_bool(node, "ti,psc-lreset"))
> >> +               data->psc_flags |= PSC_LRESET;
> >> +       if (of_property_read_bool(node, "ti,psc-force"))
> >> +               data->psc_flags |= PSC_FORCE;
> > 
> > Neither of these were defined in the binding, and they don't appear to
> > have been inherited form another binding. What are they for? Why are
> > they needed?
> > 
> They represent the hardware support transition method needed to have
> proper clock enable/disable sequence to work. Will update the binding
> along with other comments which I agree.

Ok.

> 
> >> +
> >> +       clk = clk_register_psc(NULL, clk_name, parent_name,
> >> +                               data, lock);
> >> +
> >> +       if (clk) {
> >> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> >> +
> >> +               rc = of_property_read_string(node, "status", &status);
> >> +               if (status && !strcmp(status, "enabled"))
> >> +                       clk_prepare_enable(clk);
> >> +               return;
> >> +       }
> > 
> > As mentioned above, this abuses the standard status property, and it's
> > not clear why this is necessary.
> > 
> Actually I have removed this one from dt nodes. Looks like missed
> to pick the right patch while posting. Have dropped this change
> already.

Great!

Thanks,
Mark.

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mike Turquette]

Quoting Mark Rutland (2013-08-13 09:53:44)
> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
> > On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> > > [adding dt maintainers]
> > > 
> > > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> > >> Add the driver for the clock gate control which uses PSC (Power Sleep
> > >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> > >> disabling of the clocks for different IPs present in the SoC.
> > >>
> > >> Cc: Mike Turquette <mturquette@linaro.org>
> > >>
> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > >> ---
> > >>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
> > >>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
> > >>  include/linux/clk/keystone.h                       |    1 +
> > >>  3 files changed, 337 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >>  create mode 100644 drivers/clk/keystone/gate.c
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >> new file mode 100644
> > >> index 0000000..40afef5
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> > >> @@ -0,0 +1,30 @@
> > >> +Binding for Keystone gate control driver which uses PSC controller IP.
> > >> +
> > >> +This binding uses the common clock binding[1].
> > >> +
> > >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> +
> > >> +Required properties:
> > >> +- compatible : shall be "keystone,psc-clk".
> > > 
> > > Similarly to my comments on patch 1, this should probably be something
> > > more like "ti,keystone-psc-clock"
> > > 
> > >> +- #clock-cells : from common clock binding; shall be set to 0.
> > >> +- clocks : parent clock phandle
> > >> +- reg :        psc address address space
> > >> +
> > >> +Optional properties:
> > >> +- clock-output-names : From common clock binding to override the
> > >> +                       default output clock name
> > >> +- status : "enabled" if clock is always enabled
> > > 
> > > Huh? This is a standard property, for which ePAPR defines "okay",
> > > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> > > seem to follow the standard meaning judging by the code.
> > > 
> > > It looks like this could be a boolean property
> > > ("clock-always-enabled"?), but that assumes it's necessary.
> > > 
> > > Why do you need this?
> > > 
> > I dropped this already. Forgot to update the documentation.
> 
> Ok.
> 
> > 
> > >> +- lpsc : lpsc module id, if not set defaults to zero
> > > 
> > > What's that needed for? Are there multiple clocks sharing a common
> > > register bank?
> > > 
> > >> +- pd : power domain number, if not set defaults to zero (always ON)
> > > 
> > > Please don't use abbreviations unnecessarily. "power-domain-id" or
> > > similar would be far better. What exactly does this represent? Does the
> > > clock IP itself handle power domains, or is there some external unit
> > > that controls power domains?
> > > 
> > pd is commonly used but I can expand it. This represent the power
> > domain number.
> 
> Does the the clock IP have some internal logic for handling power
> domains only covering its clocks, or is there some external power
> controller involved (i.e. do we possibly need to describe an external
> unit and a linkage to it?).

Hmm, does the clock own the power domain or does the power domain own
the clock? As you well know on OMAP the clock resides within the power
domain. So it seems to me that the better way to do this would be for
the power domain to have it's own binding and node, and then reference
the clock:

power-domain {
	...
	clocks = <&chipclk3>, <&chipclk4>;
	clock-names = "perclk", "uartclk";
	...
};

Now maybe things are different on Keystone, but if not then I don't see
why the clock binding should have anything to do with the power domain.

> 
> > 
> > >> +- gpsc : gpsc number, if not set defaults to zero
> > > 
> > > How does that interact with the lpsc property? When are these necessary?
> > > 
> > lpsc is local clock/module control where as gpsc sits on top of it. 

Similar to the above, should the gpsc have it's own binding and node
that contains the lpsc which also have their own bindings/nodes, which
finally contain the clocks? Maybe that is over engineered but I want to
consider this before the binding gets set in stone.

Regards,
Mike

> 
> Ok. I'm not sure haivng these default to zero makes much sense, as that
> could easily hide errors in dts. It might make more sense to make them
> required and error out if they aren't present.
> 
> Unless they're zero far more often?
> 
> > 
> > >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c
> > >> new file mode 100644
> > >> index 0000000..72ac478
> > >> --- /dev/null
> > >> +++ b/drivers/clk/keystone/gate.c
> > 
> > [..]
> > 
> > >> +/**
> > >> + * clk_register_psc - register psc clock
> > >> + * @dev: device that is registering this clock
> > >> + * @name: name of this clock
> > >> + * @parent_name: name of clock's parent
> > >> + * @psc_data: platform data to configure this clock
> > >> + * @lock: spinlock used by this clock
> > >> + */
> > >> +static struct clk *clk_register_psc(struct device *dev,
> > >> +                       const char *name,
> > >> +                       const char *parent_name,
> > >> +                       struct clk_psc_data *psc_data,
> > >> +                       spinlock_t *lock)
> > >> +{
> > >> +       struct clk_init_data init;
> > >> +       struct clk_psc *psc;
> > >> +       struct clk *clk;
> > >> +
> > >> +       psc = kzalloc(sizeof(*psc), GFP_KERNEL);
> > >> +       if (!psc)
> > >> +               return ERR_PTR(-ENOMEM);
> > >> +
> > >> +       init.name = name;
> > >> +       init.ops = &clk_psc_ops;
> > >> +       init.flags = psc_data->flags;
> > >> +       init.parent_names = (parent_name ? &parent_name : NULL);
> > > 
> > > Is &parent_name not a pointer to a pointer on the stack? Is this only
> > > used once?
> > > 
> > >> +       init.num_parents = (parent_name ? 1 : 0);
> > >> +
> > >> +       psc->psc_data = psc_data;
> > >> +       psc->lock = lock;
> > >> +       psc->hw.init = &init;
> > > 
> > > That's definitely a pointer to some data on the stack, and that seems to
> > > be kept around. Is this only used for one-time initialisation, or might
> > > it be used later?
> > > 
> > Its init only.
> 
> Ok.
> 
> > 
> > >> +       data->base = psc_addr[gpsc].io_base;
> > >> +       data->lpsc = lpsc;
> > >> +       data->gpsc = gpsc;
> > >> +       data->domain = pd;
> > >> +
> > >> +       if (of_property_read_bool(node, "ti,psc-lreset"))
> > >> +               data->psc_flags |= PSC_LRESET;
> > >> +       if (of_property_read_bool(node, "ti,psc-force"))
> > >> +               data->psc_flags |= PSC_FORCE;
> > > 
> > > Neither of these were defined in the binding, and they don't appear to
> > > have been inherited form another binding. What are they for? Why are
> > > they needed?
> > > 
> > They represent the hardware support transition method needed to have
> > proper clock enable/disable sequence to work. Will update the binding
> > along with other comments which I agree.
> 
> Ok.
> 
> > 
> > >> +
> > >> +       clk = clk_register_psc(NULL, clk_name, parent_name,
> > >> +                               data, lock);
> > >> +
> > >> +       if (clk) {
> > >> +               of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > >> +
> > >> +               rc = of_property_read_string(node, "status", &status);
> > >> +               if (status && !strcmp(status, "enabled"))
> > >> +                       clk_prepare_enable(clk);
> > >> +               return;
> > >> +       }
> > > 
> > > As mentioned above, this abuses the standard status property, and it's
> > > not clear why this is necessary.
> > > 
> > Actually I have removed this one from dt nodes. Looks like missed
> > to pick the right patch while posting. Have dropped this change
> > already.
> 
> Great!
> 
> Thanks,
> Mark.

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Monday 19 August 2013 04:43 PM, Mike Turquette wrote:
> Quoting Mark Rutland (2013-08-13 09:53:44)
>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
>>>> [adding dt maintainers]
>>>>
>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep
>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
>>>>> disabling of the clocks for different IPs present in the SoC.
>>>>>
>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>
>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>> ---
>>>>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
>>>>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
>>>>>  include/linux/clk/keystone.h                       |    1 +
>>>>>  3 files changed, 337 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>  create mode 100644 drivers/clk/keystone/gate.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>> new file mode 100644
>>>>> index 0000000..40afef5
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>> @@ -0,0 +1,30 @@
>>>>> +Binding for Keystone gate control driver which uses PSC controller IP.
>>>>> +
>>>>> +This binding uses the common clock binding[1].
>>>>> +
>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : shall be "keystone,psc-clk".
>>>>
>>>> Similarly to my comments on patch 1, this should probably be something
>>>> more like "ti,keystone-psc-clock"
>>>>
>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>> +- clocks : parent clock phandle
>>>>> +- reg :        psc address address space
>>>>> +
>>>>> +Optional properties:
>>>>> +- clock-output-names : From common clock binding to override the
>>>>> +                       default output clock name
>>>>> +- status : "enabled" if clock is always enabled
>>>>
>>>> Huh? This is a standard property, for which ePAPR defines "okay",
>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
>>>> seem to follow the standard meaning judging by the code.
>>>>
>>>> It looks like this could be a boolean property
>>>> ("clock-always-enabled"?), but that assumes it's necessary.
>>>>
>>>> Why do you need this?
>>>>
>>> I dropped this already. Forgot to update the documentation.
>>
>> Ok.
>>
>>>
>>>>> +- lpsc : lpsc module id, if not set defaults to zero
>>>>
>>>> What's that needed for? Are there multiple clocks sharing a common
>>>> register bank?
>>>>
>>>>> +- pd : power domain number, if not set defaults to zero (always ON)
>>>>
>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or
>>>> similar would be far better. What exactly does this represent? Does the
>>>> clock IP itself handle power domains, or is there some external unit
>>>> that controls power domains?
>>>>
>>> pd is commonly used but I can expand it. This represent the power
>>> domain number.
>>
>> Does the the clock IP have some internal logic for handling power
>> domains only covering its clocks, or is there some external power
>> controller involved (i.e. do we possibly need to describe an external
>> unit and a linkage to it?).
> 
> Hmm, does the clock own the power domain or does the power domain own
> the clock? As you well know on OMAP the clock resides within the power
> domain. So it seems to me that the better way to do this would be for
> the power domain to have it's own binding and node, and then reference
> the clock:
> 
> power-domain {
> 	...
> 	clocks = <&chipclk3>, <&chipclk4>;
> 	clock-names = "perclk", "uartclk";
> 	...
> };
> 
> Now maybe things are different on Keystone, but if not then I don't see
> why the clock binding should have anything to do with the power domain.
>
They are bit different w.r.t OMAP. LPSC itself is the clock control of the
IP. The LPSC number in the bindings is actually the specific number which
is used to reach to the address space of the clock control. One can view
that one as clock control register index.

The power domain as such are above the lpsc but the clock enable/disable needs
to follow a recommended sequence which needs the access to those registers.
As such there is no major validation done on dynamically switching off PDs
in current generation devices.

As I mentioned you to on IRC, the PD was the only odd part I have to keep
around to address the sequencing need which is kind of interlinked.
 
>>
>>>
>>>>> +- gpsc : gpsc number, if not set defaults to zero
>>>>
>>>> How does that interact with the lpsc property? When are these necessary?
>>>>
>>> lpsc is local clock/module control where as gpsc sits on top of it. 
> 
> Similar to the above, should the gpsc have it's own binding and node
> that contains the lpsc which also have their own bindings/nodes, which
> finally contain the clocks? Maybe that is over engineered but I want to
> consider this before the binding gets set in stone.
> 
Actually GPSC need not be even mentioned since its one per SOC. Let me 
see if I can drop it to avoid any confusion.

Regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-20 06:55:54)
> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote:
> > Quoting Mark Rutland (2013-08-13 09:53:44)
> >> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
> >>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> >>>> [adding dt maintainers]
> >>>>
> >>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> >>>>> Add the driver for the clock gate control which uses PSC (Power Sleep
> >>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> >>>>> disabling of the clocks for different IPs present in the SoC.
> >>>>>
> >>>>> Cc: Mike Turquette <mturquette@linaro.org>
> >>>>>
> >>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>>> ---
> >>>>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
> >>>>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
> >>>>>  include/linux/clk/keystone.h                       |    1 +
> >>>>>  3 files changed, 337 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>>  create mode 100644 drivers/clk/keystone/gate.c
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..40afef5
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>> @@ -0,0 +1,30 @@
> >>>>> +Binding for Keystone gate control driver which uses PSC controller IP.
> >>>>> +
> >>>>> +This binding uses the common clock binding[1].
> >>>>> +
> >>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> +
> >>>>> +Required properties:
> >>>>> +- compatible : shall be "keystone,psc-clk".
> >>>>
> >>>> Similarly to my comments on patch 1, this should probably be something
> >>>> more like "ti,keystone-psc-clock"
> >>>>
> >>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>> +- clocks : parent clock phandle
> >>>>> +- reg :        psc address address space
> >>>>> +
> >>>>> +Optional properties:
> >>>>> +- clock-output-names : From common clock binding to override the
> >>>>> +                       default output clock name
> >>>>> +- status : "enabled" if clock is always enabled
> >>>>
> >>>> Huh? This is a standard property, for which ePAPR defines "okay",
> >>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> >>>> seem to follow the standard meaning judging by the code.
> >>>>
> >>>> It looks like this could be a boolean property
> >>>> ("clock-always-enabled"?), but that assumes it's necessary.
> >>>>
> >>>> Why do you need this?
> >>>>
> >>> I dropped this already. Forgot to update the documentation.
> >>
> >> Ok.
> >>
> >>>
> >>>>> +- lpsc : lpsc module id, if not set defaults to zero
> >>>>
> >>>> What's that needed for? Are there multiple clocks sharing a common
> >>>> register bank?
> >>>>
> >>>>> +- pd : power domain number, if not set defaults to zero (always ON)
> >>>>
> >>>> Please don't use abbreviations unnecessarily. "power-domain-id" or
> >>>> similar would be far better. What exactly does this represent? Does the
> >>>> clock IP itself handle power domains, or is there some external unit
> >>>> that controls power domains?
> >>>>
> >>> pd is commonly used but I can expand it. This represent the power
> >>> domain number.
> >>
> >> Does the the clock IP have some internal logic for handling power
> >> domains only covering its clocks, or is there some external power
> >> controller involved (i.e. do we possibly need to describe an external
> >> unit and a linkage to it?).
> > 
> > Hmm, does the clock own the power domain or does the power domain own
> > the clock? As you well know on OMAP the clock resides within the power
> > domain. So it seems to me that the better way to do this would be for
> > the power domain to have it's own binding and node, and then reference
> > the clock:
> > 
> > power-domain {
> >       ...
> >       clocks = <&chipclk3>, <&chipclk4>;
> >       clock-names = "perclk", "uartclk";
> >       ...
> > };
> > 
> > Now maybe things are different on Keystone, but if not then I don't see
> > why the clock binding should have anything to do with the power domain.
> >
> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
> IP. The LPSC number in the bindings is actually the specific number which
> is used to reach to the address space of the clock control. One can view
> that one as clock control register index.

Thanks for the information. I have a further question about then: are
the LPSC clocks really module clocks that belong to the IP that they are
gating?

If so then they could be defined within the node defining their parent
IP. That might be enough to get rid of the LPSC index value. Again I
might be over-engineering it. Just trying to get an understanding.

> 
> The power domain as such are above the lpsc but the clock enable/disable needs
> to follow a recommended sequence which needs the access to those registers.
> As such there is no major validation done on dynamically switching off PDs
> in current generation devices.
> 
> As I mentioned you to on IRC, the PD was the only odd part I have to keep
> around to address the sequencing need which is kind of interlinked.

Right. Well maybe some day that part can go away but I understand the
need for it now.

Regards,
Mike

>  
> >>
> >>>
> >>>>> +- gpsc : gpsc number, if not set defaults to zero
> >>>>
> >>>> How does that interact with the lpsc property? When are these necessary?
> >>>>
> >>> lpsc is local clock/module control where as gpsc sits on top of it. 
> > 
> > Similar to the above, should the gpsc have it's own binding and node
> > that contains the lpsc which also have their own bindings/nodes, which
> > finally contain the clocks? Maybe that is over engineered but I want to
> > consider this before the binding gets set in stone.
> > 
> Actually GPSC need not be even mentioned since its one per SOC. Let me 
> see if I can drop it to avoid any confusion.
> 
> Regards,
> Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 06:55:54)
>> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote:
>>> Quoting Mark Rutland (2013-08-13 09:53:44)
>>>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
>>>>>> [adding dt maintainers]
>>>>>>
>>>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
>>>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep
>>>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
>>>>>>> disabling of the clocks for different IPs present in the SoC.
>>>>>>>
>>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>>>
>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
>>>>>>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
>>>>>>>  include/linux/clk/keystone.h                       |    1 +
>>>>>>>  3 files changed, 337 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>>  create mode 100644 drivers/clk/keystone/gate.c
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..40afef5
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>> @@ -0,0 +1,30 @@
>>>>>>> +Binding for Keystone gate control driver which uses PSC controller IP.
>>>>>>> +
>>>>>>> +This binding uses the common clock binding[1].
>>>>>>> +
>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : shall be "keystone,psc-clk".
>>>>>>
>>>>>> Similarly to my comments on patch 1, this should probably be something
>>>>>> more like "ti,keystone-psc-clock"
>>>>>>
>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>> +- clocks : parent clock phandle
>>>>>>> +- reg :        psc address address space
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- clock-output-names : From common clock binding to override the
>>>>>>> +                       default output clock name
>>>>>>> +- status : "enabled" if clock is always enabled
>>>>>>
>>>>>> Huh? This is a standard property, for which ePAPR defines "okay",
>>>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
>>>>>> seem to follow the standard meaning judging by the code.
>>>>>>
>>>>>> It looks like this could be a boolean property
>>>>>> ("clock-always-enabled"?), but that assumes it's necessary.
>>>>>>
>>>>>> Why do you need this?
>>>>>>
>>>>> I dropped this already. Forgot to update the documentation.
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>>>> +- lpsc : lpsc module id, if not set defaults to zero
>>>>>>
>>>>>> What's that needed for? Are there multiple clocks sharing a common
>>>>>> register bank?
>>>>>>
>>>>>>> +- pd : power domain number, if not set defaults to zero (always ON)
>>>>>>
>>>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or
>>>>>> similar would be far better. What exactly does this represent? Does the
>>>>>> clock IP itself handle power domains, or is there some external unit
>>>>>> that controls power domains?
>>>>>>
>>>>> pd is commonly used but I can expand it. This represent the power
>>>>> domain number.
>>>>
>>>> Does the the clock IP have some internal logic for handling power
>>>> domains only covering its clocks, or is there some external power
>>>> controller involved (i.e. do we possibly need to describe an external
>>>> unit and a linkage to it?).
>>>
>>> Hmm, does the clock own the power domain or does the power domain own
>>> the clock? As you well know on OMAP the clock resides within the power
>>> domain. So it seems to me that the better way to do this would be for
>>> the power domain to have it's own binding and node, and then reference
>>> the clock:
>>>
>>> power-domain {
>>>       ...
>>>       clocks = <&chipclk3>, <&chipclk4>;
>>>       clock-names = "perclk", "uartclk";
>>>       ...
>>> };
>>>
>>> Now maybe things are different on Keystone, but if not then I don't see
>>> why the clock binding should have anything to do with the power domain.
>>>
>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
>> IP. The LPSC number in the bindings is actually the specific number which
>> is used to reach to the address space of the clock control. One can view
>> that one as clock control register index.
> 
> Thanks for the information. I have a further question about then: are
> the LPSC clocks really module clocks that belong to the IP that they are
> gating?
> 
LPSC controls the clock enable/disable to the IP/module so answer is yes.
In certain cases LPSC controls clock to more than one IP as well.

> If so then they could be defined within the node defining their parent
> IP. That might be enough to get rid of the LPSC index value. Again I
> might be over-engineering it. Just trying to get an understanding.
> 
Am not sure I follow you here on not having the LPSC index. Sorry. 

>>
>> The power domain as such are above the lpsc but the clock enable/disable needs
>> to follow a recommended sequence which needs the access to those registers.
>> As such there is no major validation done on dynamically switching off PDs
>> in current generation devices.
>>
>> As I mentioned you to on IRC, the PD was the only odd part I have to keep
>> around to address the sequencing need which is kind of interlinked.
> 
> Right. Well maybe some day that part can go away but I understand the
> need for it now.
> 
right. Thanks

regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-20 14:55:56)
> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
> > Quoting Santosh Shilimkar (2013-08-20 06:55:54)
> >> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote:
> >>> Quoting Mark Rutland (2013-08-13 09:53:44)
> >>>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
> >>>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
> >>>>>> [adding dt maintainers]
> >>>>>>
> >>>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
> >>>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep
> >>>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
> >>>>>>> disabling of the clocks for different IPs present in the SoC.
> >>>>>>>
> >>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
> >>>>>>>
> >>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
> >>>>>>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
> >>>>>>>  include/linux/clk/keystone.h                       |    1 +
> >>>>>>>  3 files changed, 337 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>>>>  create mode 100644 drivers/clk/keystone/gate.c
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..40afef5
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
> >>>>>>> @@ -0,0 +1,30 @@
> >>>>>>> +Binding for Keystone gate control driver which uses PSC controller IP.
> >>>>>>> +
> >>>>>>> +This binding uses the common clock binding[1].
> >>>>>>> +
> >>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>>>> +
> >>>>>>> +Required properties:
> >>>>>>> +- compatible : shall be "keystone,psc-clk".
> >>>>>>
> >>>>>> Similarly to my comments on patch 1, this should probably be something
> >>>>>> more like "ti,keystone-psc-clock"
> >>>>>>
> >>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>>>> +- clocks : parent clock phandle
> >>>>>>> +- reg :        psc address address space
> >>>>>>> +
> >>>>>>> +Optional properties:
> >>>>>>> +- clock-output-names : From common clock binding to override the
> >>>>>>> +                       default output clock name
> >>>>>>> +- status : "enabled" if clock is always enabled
> >>>>>>
> >>>>>> Huh? This is a standard property, for which ePAPR defines "okay",
> >>>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
> >>>>>> seem to follow the standard meaning judging by the code.
> >>>>>>
> >>>>>> It looks like this could be a boolean property
> >>>>>> ("clock-always-enabled"?), but that assumes it's necessary.
> >>>>>>
> >>>>>> Why do you need this?
> >>>>>>
> >>>>> I dropped this already. Forgot to update the documentation.
> >>>>
> >>>> Ok.
> >>>>
> >>>>>
> >>>>>>> +- lpsc : lpsc module id, if not set defaults to zero
> >>>>>>
> >>>>>> What's that needed for? Are there multiple clocks sharing a common
> >>>>>> register bank?
> >>>>>>
> >>>>>>> +- pd : power domain number, if not set defaults to zero (always ON)
> >>>>>>
> >>>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or
> >>>>>> similar would be far better. What exactly does this represent? Does the
> >>>>>> clock IP itself handle power domains, or is there some external unit
> >>>>>> that controls power domains?
> >>>>>>
> >>>>> pd is commonly used but I can expand it. This represent the power
> >>>>> domain number.
> >>>>
> >>>> Does the the clock IP have some internal logic for handling power
> >>>> domains only covering its clocks, or is there some external power
> >>>> controller involved (i.e. do we possibly need to describe an external
> >>>> unit and a linkage to it?).
> >>>
> >>> Hmm, does the clock own the power domain or does the power domain own
> >>> the clock? As you well know on OMAP the clock resides within the power
> >>> domain. So it seems to me that the better way to do this would be for
> >>> the power domain to have it's own binding and node, and then reference
> >>> the clock:
> >>>
> >>> power-domain {
> >>>       ...
> >>>       clocks = <&chipclk3>, <&chipclk4>;
> >>>       clock-names = "perclk", "uartclk";
> >>>       ...
> >>> };
> >>>
> >>> Now maybe things are different on Keystone, but if not then I don't see
> >>> why the clock binding should have anything to do with the power domain.
> >>>
> >> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
> >> IP. The LPSC number in the bindings is actually the specific number which
> >> is used to reach to the address space of the clock control. One can view
> >> that one as clock control register index.
> > 
> > Thanks for the information. I have a further question about then: are
> > the LPSC clocks really module clocks that belong to the IP that they are
> > gating?
> > 
> LPSC controls the clock enable/disable to the IP/module so answer is yes.
> In certain cases LPSC controls clock to more than one IP as well.
> 
> > If so then they could be defined within the node defining their parent
> > IP. That might be enough to get rid of the LPSC index value. Again I
> > might be over-engineering it. Just trying to get an understanding.
> > 
> Am not sure I follow you here on not having the LPSC index. Sorry. 

How are the 'reg' property and the 'lpsc' property related? Does the
lpsc property modify the register address used to access the clock
control bits?

Thanks,
Mike

> 
> >>
> >> The power domain as such are above the lpsc but the clock enable/disable needs
> >> to follow a recommended sequence which needs the access to those registers.
> >> As such there is no major validation done on dynamically switching off PDs
> >> in current generation devices.
> >>
> >> As I mentioned you to on IRC, the PD was the only odd part I have to keep
> >> around to address the sequencing need which is kind of interlinked.
> > 
> > Right. Well maybe some day that part can go away but I understand the
> > need for it now.
> > 
> right. Thanks
> 
> regards,
> Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 14:55:56)
>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:

[...]

>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
>>>> IP. The LPSC number in the bindings is actually the specific number which
>>>> is used to reach to the address space of the clock control. One can view
>>>> that one as clock control register index.
>>>
>>> Thanks for the information. I have a further question about then: are
>>> the LPSC clocks really module clocks that belong to the IP that they are
>>> gating?
>>>
>> LPSC controls the clock enable/disable to the IP/module so answer is yes.
>> In certain cases LPSC controls clock to more than one IP as well.
>>
>>> If so then they could be defined within the node defining their parent
>>> IP. That might be enough to get rid of the LPSC index value. Again I
>>> might be over-engineering it. Just trying to get an understanding.
>>>
>> Am not sure I follow you here on not having the LPSC index. Sorry. 
> 
> How are the 'reg' property and the 'lpsc' property related? Does the
> lpsc property modify the register address used to access the clock
> control bits?
> 
Yes it does. Currently all nodes use fix address and then lpsc is
used as an index. But I think we can do better by just using the
right(offset) address in the reg property. Will have a look at it
and see what I can do here.

Thanks for asking this questions Mike 

regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-20 15:54:15)
> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote:
> > Quoting Santosh Shilimkar (2013-08-20 14:55:56)
> >> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
> 
> [...]
> 
> >>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
> >>>> IP. The LPSC number in the bindings is actually the specific number which
> >>>> is used to reach to the address space of the clock control. One can view
> >>>> that one as clock control register index.
> >>>
> >>> Thanks for the information. I have a further question about then: are
> >>> the LPSC clocks really module clocks that belong to the IP that they are
> >>> gating?
> >>>
> >> LPSC controls the clock enable/disable to the IP/module so answer is yes.
> >> In certain cases LPSC controls clock to more than one IP as well.
> >>
> >>> If so then they could be defined within the node defining their parent
> >>> IP. That might be enough to get rid of the LPSC index value. Again I
> >>> might be over-engineering it. Just trying to get an understanding.
> >>>
> >> Am not sure I follow you here on not having the LPSC index. Sorry. 
> > 
> > How are the 'reg' property and the 'lpsc' property related? Does the
> > lpsc property modify the register address used to access the clock
> > control bits?
> > 
> Yes it does. Currently all nodes use fix address and then lpsc is
> used as an index.

Ok cool. Well the reason I brought that up was because I even had the
idea to define these module clocks within the module nodes that own them
in DT. I am way outside of my DT knowledge at this point but I wonder
if the following type of binding is possible:

module: module at 4a308200 {
	#address-cells = <1>;
	#size-cells = <1>;
	reg = <0x4a308200 0x1000>;

	clock {
		#clock-cells = <0>;
		compatible = "keystone,psc-clk";
		clocks = <&chipclk3>;
		clock-output-names = "debugss_trc";
		reg = <0x0256>;
		pd = <1>;
	};
};

Again, my DT knowledge is pretty limited, but could the reg property of
the clock be directly affected by the parent node? That seems like it
could nicely model what the hardware is really doing.

> But I think we can do better by just using the
> right(offset) address in the reg property. Will have a look at it
> and see what I can do here.

This also solves the problem nicely.  Thanks for putting up with my
silly questions ;-)

Regards,
Mike

> 
> Thanks for asking this questions Mike 
> 
> regards,
> Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 15:54:15)
>> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote:
>>> Quoting Santosh Shilimkar (2013-08-20 14:55:56)
>>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
>>
>> [...]
>>
>>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
>>>>>> IP. The LPSC number in the bindings is actually the specific number which
>>>>>> is used to reach to the address space of the clock control. One can view
>>>>>> that one as clock control register index.
>>>>>
>>>>> Thanks for the information. I have a further question about then: are
>>>>> the LPSC clocks really module clocks that belong to the IP that they are
>>>>> gating?
>>>>>
>>>> LPSC controls the clock enable/disable to the IP/module so answer is yes.
>>>> In certain cases LPSC controls clock to more than one IP as well.
>>>>
>>>>> If so then they could be defined within the node defining their parent
>>>>> IP. That might be enough to get rid of the LPSC index value. Again I
>>>>> might be over-engineering it. Just trying to get an understanding.
>>>>>
>>>> Am not sure I follow you here on not having the LPSC index. Sorry. 
>>>
>>> How are the 'reg' property and the 'lpsc' property related? Does the
>>> lpsc property modify the register address used to access the clock
>>> control bits?
>>>
>> Yes it does. Currently all nodes use fix address and then lpsc is
>> used as an index.
> 
> Ok cool. Well the reason I brought that up was because I even had the
> idea to define these module clocks within the module nodes that own them
> in DT. I am way outside of my DT knowledge at this point but I wonder
> if the following type of binding is possible:
> 
> module: module at 4a308200 {
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	reg = <0x4a308200 0x1000>;
> 
> 	clock {
> 		#clock-cells = <0>;
> 		compatible = "keystone,psc-clk";
> 		clocks = <&chipclk3>;
> 		clock-output-names = "debugss_trc";
> 		reg = <0x0256>;
> 		pd = <1>;
> 	};
> };
> 
> Again, my DT knowledge is pretty limited, but could the reg property of
> the clock be directly affected by the parent node? That seems like it
> could nicely model what the hardware is really doing.
> 
The module(I assume you mean IP here) reg address space is separate than
that used for clock control so that doesn't fit as such. Traditionally
clock controls even though targeted for specific modules sits in different
control as at least seen on OMAP and Keystone. OCP wrappers on OMAP
were bit of exceptions but they were little bit of glue logic without
much control within the address space.

>> But I think we can do better by just using the
>> right(offset) address in the reg property. Will have a look at it
>> and see what I can do here.
> 
> This also solves the problem nicely.  Thanks for putting up with my
> silly questions ;-)
> 
You asked right and good questions.

Regards,
Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Mike Turquette]

Quoting Santosh Shilimkar (2013-08-21 06:16:33)
> On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote:
> > Quoting Santosh Shilimkar (2013-08-20 15:54:15)
> >> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote:
> >>> Quoting Santosh Shilimkar (2013-08-20 14:55:56)
> >>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
> >>
> >> [...]
> >>
> >>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
> >>>>>> IP. The LPSC number in the bindings is actually the specific number which
> >>>>>> is used to reach to the address space of the clock control. One can view
> >>>>>> that one as clock control register index.
> >>>>>
> >>>>> Thanks for the information. I have a further question about then: are
> >>>>> the LPSC clocks really module clocks that belong to the IP that they are
> >>>>> gating?
> >>>>>
> >>>> LPSC controls the clock enable/disable to the IP/module so answer is yes.
> >>>> In certain cases LPSC controls clock to more than one IP as well.
> >>>>
> >>>>> If so then they could be defined within the node defining their parent
> >>>>> IP. That might be enough to get rid of the LPSC index value. Again I
> >>>>> might be over-engineering it. Just trying to get an understanding.
> >>>>>
> >>>> Am not sure I follow you here on not having the LPSC index. Sorry. 
> >>>
> >>> How are the 'reg' property and the 'lpsc' property related? Does the
> >>> lpsc property modify the register address used to access the clock
> >>> control bits?
> >>>
> >> Yes it does. Currently all nodes use fix address and then lpsc is
> >> used as an index.
> > 
> > Ok cool. Well the reason I brought that up was because I even had the
> > idea to define these module clocks within the module nodes that own them
> > in DT. I am way outside of my DT knowledge at this point but I wonder
> > if the following type of binding is possible:
> > 
> > module: module at 4a308200 {
> >       #address-cells = <1>;
> >       #size-cells = <1>;
> >       reg = <0x4a308200 0x1000>;
> > 
> >       clock {
> >               #clock-cells = <0>;
> >               compatible = "keystone,psc-clk";
> >               clocks = <&chipclk3>;
> >               clock-output-names = "debugss_trc";
> >               reg = <0x0256>;
> >               pd = <1>;
> >       };
> > };
> > 
> > Again, my DT knowledge is pretty limited, but could the reg property of
> > the clock be directly affected by the parent node? That seems like it
> > could nicely model what the hardware is really doing.
> > 
> The module(I assume you mean IP here) reg address space is separate than
> that used for clock control so that doesn't fit as such. Traditionally
> clock controls even though targeted for specific modules sits in different
> control as at least seen on OMAP and Keystone. OCP wrappers on OMAP
> were bit of exceptions but they were little bit of glue logic without
> much control within the address space.

Great, you perfectly answered my questions. I think that assigning the
"final" address to the 'reg' property is the right way to go (fixed
address + LPSC index).

Regards,
Mike

> 
> >> But I think we can do better by just using the
> >> right(offset) address in the reg property. Will have a look at it
> >> and see what I can do here.
> > 
> > This also solves the problem nicely.  Thanks for putting up with my
> > silly questions ;-)
> > 
> You asked right and good questions.
> 
> Regards,
> Santosh

[PATCH 2/8] clk: keystone: Add gate control clock driver

[Santosh Shilimkar]

On Thursday 22 August 2013 04:12 AM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-21 06:16:33)
>> On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote:
>>> Quoting Santosh Shilimkar (2013-08-20 15:54:15)
>>>> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote:
>>>>> Quoting Santosh Shilimkar (2013-08-20 14:55:56)
>>>>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
>>>>
>>>> [...]
>>>>
>>>>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
>>>>>>>> IP. The LPSC number in the bindings is actually the specific number which
>>>>>>>> is used to reach to the address space of the clock control. One can view
>>>>>>>> that one as clock control register index.
>>>>>>>
>>>>>>> Thanks for the information. I have a further question about then: are
>>>>>>> the LPSC clocks really module clocks that belong to the IP that they are
>>>>>>> gating?
>>>>>>>
>>>>>> LPSC controls the clock enable/disable to the IP/module so answer is yes.
>>>>>> In certain cases LPSC controls clock to more than one IP as well.
>>>>>>
>>>>>>> If so then they could be defined within the node defining their parent
>>>>>>> IP. That might be enough to get rid of the LPSC index value. Again I
>>>>>>> might be over-engineering it. Just trying to get an understanding.
>>>>>>>
>>>>>> Am not sure I follow you here on not having the LPSC index. Sorry. 
>>>>>
>>>>> How are the 'reg' property and the 'lpsc' property related? Does the
>>>>> lpsc property modify the register address used to access the clock
>>>>> control bits?
>>>>>
>>>> Yes it does. Currently all nodes use fix address and then lpsc is
>>>> used as an index.
>>>
>>> Ok cool. Well the reason I brought that up was because I even had the
>>> idea to define these module clocks within the module nodes that own them
>>> in DT. I am way outside of my DT knowledge at this point but I wonder
>>> if the following type of binding is possible:
>>>
>>> module: module at 4a308200 {
>>>       #address-cells = <1>;
>>>       #size-cells = <1>;
>>>       reg = <0x4a308200 0x1000>;
>>>
>>>       clock {
>>>               #clock-cells = <0>;
>>>               compatible = "keystone,psc-clk";
>>>               clocks = <&chipclk3>;
>>>               clock-output-names = "debugss_trc";
>>>               reg = <0x0256>;
>>>               pd = <1>;
>>>       };
>>> };
>>>
>>> Again, my DT knowledge is pretty limited, but could the reg property of
>>> the clock be directly affected by the parent node? That seems like it
>>> could nicely model what the hardware is really doing.
>>>
>> The module(I assume you mean IP here) reg address space is separate than
>> that used for clock control so that doesn't fit as such. Traditionally
>> clock controls even though targeted for specific modules sits in different
>> control as at least seen on OMAP and Keystone. OCP wrappers on OMAP
>> were bit of exceptions but they were little bit of glue logic without
>> much control within the address space.
> 
> Great, you perfectly answered my questions. I think that assigning the
> "final" address to the 'reg' property is the right way to go (fixed
> address + LPSC index).
> 
Thanks Mike.

Regards,
Santosh

[PATCH 3/8] clk: keystone: common clk driver initialization

[Santosh Shilimkar]

Initialise common clock drivers for Keystone 2 devices.

Cc: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/clk/keystone/clk.c   |   34 ++++++++++++++++++++++++++++++++++
 include/linux/clk/keystone.h |    1 +
 2 files changed, 35 insertions(+)
 create mode 100644 drivers/clk/keystone/clk.c

diff --git a/drivers/clk/keystone/clk.c b/drivers/clk/keystone/clk.c
new file mode 100644
index 0000000..9001380
--- /dev/null
+++ b/drivers/clk/keystone/clk.c
@@ -0,0 +1,34 @@
+/*
+ * Common Clock initialization code for Keystone SOCs
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <m-karicheri2@ti.com>
+ *	Santosh Shilimkar <santosh.shilimkar@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/clk/keystone.h>
+
+static const __initconst struct of_device_id clk_match[] = {
+	{ .compatible = "fixed-clock", .data = of_fixed_clk_setup, },
+	{ .compatible = "fixed-factor-clock", .data =
+					 of_fixed_factor_clk_setup, },
+	{ .compatible = "keystone,pll-clk", .data = of_keystone_pll_clk_init, },
+	{ .compatible = "mux-clk", .data = of_mux_clk_setup, },
+	{ .compatible = "divider-clock", .data = of_divider_clk_setup, },
+	{ .compatible = "keystone,psc-clk", .data = of_keystone_psc_clk_init, },
+	{}
+};
+
+void __init of_keystone_clk_init()
+{
+	/* initialize clk providers from device tree */
+	of_clk_init(clk_match);
+}
diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
index 7b3e154..282e88f 100644
--- a/include/linux/clk/keystone.h
+++ b/include/linux/clk/keystone.h
@@ -15,5 +15,6 @@
 
 extern void of_keystone_pll_clk_init(struct device_node *node);
 extern void of_keystone_psc_clk_init(struct device_node *node);
+extern void of_keystone_clk_init(void);
 
 #endif /* __LINUX_CLK_KEYSTONE_H_ */
-- 
1.7.9.5

[PATCH 3/8] clk: keystone: common clk driver initialization

[Nishanth Menon]

On 08/05/2013 11:12 AM, Santosh Shilimkar wrote:
> Initialise common clock drivers for Keystone 2 devices.
>
> Cc: Mike Turquette <mturquette@linaro.org>
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>   drivers/clk/keystone/clk.c   |   34 ++++++++++++++++++++++++++++++++++
>   include/linux/clk/keystone.h |    1 +
>   2 files changed, 35 insertions(+)
>   create mode 100644 drivers/clk/keystone/clk.c
>
> diff --git a/drivers/clk/keystone/clk.c b/drivers/clk/keystone/clk.c
> new file mode 100644
> index 0000000..9001380
> --- /dev/null
> +++ b/drivers/clk/keystone/clk.c
> @@ -0,0 +1,34 @@
> +/*
> + * Common Clock initialization code for Keystone SOCs
> + *
> + * Copyright (C) 2013 Texas Instruments Inc.
> + *	Murali Karicheri <m-karicheri2@ti.com>
> + *	Santosh Shilimkar <santosh.shilimkar@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/clk/keystone.h>
> +
> +static const __initconst struct of_device_id clk_match[] = {
> +	{ .compatible = "fixed-clock", .data = of_fixed_clk_setup, },
> +	{ .compatible = "fixed-factor-clock", .data =
> +					 of_fixed_factor_clk_setup, },
> +	{ .compatible = "keystone,pll-clk", .data = of_keystone_pll_clk_init, },
> +	{ .compatible = "mux-clk", .data = of_mux_clk_setup, },
> +	{ .compatible = "divider-clock", .data = of_divider_clk_setup, },
> +	{ .compatible = "keystone,psc-clk", .data = of_keystone_psc_clk_init, },
> +	{}
> +};
> +
> +void __init of_keystone_clk_init()
> +{
> +	/* initialize clk providers from device tree */
> +	of_clk_init(clk_match);
> +}
> diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
> index 7b3e154..282e88f 100644
> --- a/include/linux/clk/keystone.h
> +++ b/include/linux/clk/keystone.h
> @@ -15,5 +15,6 @@
>
>   extern void of_keystone_pll_clk_init(struct device_node *node);
>   extern void of_keystone_psc_clk_init(struct device_node *node);
> +extern void of_keystone_clk_init(void);
>
>   #endif /* __LINUX_CLK_KEYSTONE_H_ */
>

You can dump the entire file - with of_clk_init(NULL);

every driver which is registered with CLK_OF_DECLARE() will work, and 
further, you can even get rid of all the nasty export code, extra 
headers etc..

See how it was done in the OMAP support[1] - this should simplify much 
of the code, as well as allow you to share drivers/clk/ti.


[1] http://marc.info/?l=devicetree&m=137546079103144&w=2


-- 
Regards,
Nishanth Menon

[PATCH 3/8] clk: keystone: common clk driver initialization

[Santosh Shilimkar]

On Monday 05 August 2013 02:54 PM, Nishanth Menon wrote:
> On 08/05/2013 11:12 AM, Santosh Shilimkar wrote:
>> Initialise common clock drivers for Keystone 2 devices.
>>
>> Cc: Mike Turquette <mturquette@linaro.org>
>>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>   drivers/clk/keystone/clk.c   |   34 ++++++++++++++++++++++++++++++++++
>>   include/linux/clk/keystone.h |    1 +
>>   2 files changed, 35 insertions(+)
>>   create mode 100644 drivers/clk/keystone/clk.c
>>
>> diff --git a/drivers/clk/keystone/clk.c b/drivers/clk/keystone/clk.c
>> new file mode 100644
>> index 0000000..9001380
>> --- /dev/null
>> +++ b/drivers/clk/keystone/clk.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Common Clock initialization code for Keystone SOCs
>> + *
>> + * Copyright (C) 2013 Texas Instruments Inc.
>> + *    Murali Karicheri <m-karicheri2@ti.com>
>> + *    Santosh Shilimkar <santosh.shilimkar@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/types.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/clk/keystone.h>
>> +
>> +static const __initconst struct of_device_id clk_match[] = {
>> +    { .compatible = "fixed-clock", .data = of_fixed_clk_setup, },
>> +    { .compatible = "fixed-factor-clock", .data =
>> +                     of_fixed_factor_clk_setup, },
>> +    { .compatible = "keystone,pll-clk", .data = of_keystone_pll_clk_init, },
>> +    { .compatible = "mux-clk", .data = of_mux_clk_setup, },
>> +    { .compatible = "divider-clock", .data = of_divider_clk_setup, },
>> +    { .compatible = "keystone,psc-clk", .data = of_keystone_psc_clk_init, },
>> +    {}
>> +};
>> +
>> +void __init of_keystone_clk_init()
>> +{
>> +    /* initialize clk providers from device tree */
>> +    of_clk_init(clk_match);
>> +}
>> diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
>> index 7b3e154..282e88f 100644
>> --- a/include/linux/clk/keystone.h
>> +++ b/include/linux/clk/keystone.h
>> @@ -15,5 +15,6 @@
>>
>>   extern void of_keystone_pll_clk_init(struct device_node *node);
>>   extern void of_keystone_psc_clk_init(struct device_node *node);
>> +extern void of_keystone_clk_init(void);
>>
>>   #endif /* __LINUX_CLK_KEYSTONE_H_ */
>>
> 
> You can dump the entire file - with of_clk_init(NULL);
> 
> every driver which is registered with CLK_OF_DECLARE() will work, and further, you can even get rid of all the nasty export code, extra headers etc..
> 
of_clk_init(NULL) is indeed good idea and will have a look at it. This should
be actually applicable to many clock drivers as well which I see have the
custom init.

Regarding the nasty export code, this series doesn't rely on any exports except
the dt init functions and if of_clk_init(NULL) takes care of it, I will
drop it.

> See how it was done in the OMAP support[1] - this should simplify much of the code, as well as allow you to share drivers/clk/ti.
> 
Will have a look. Regarding the sharing code, the series actually uses all what
common clock framework support. Only PLL and gate related code is specific and
that is not common with OMAP anyways because IPs are completely different.
When we find a any commonalities, we can actually put them under ti
directory to allow sharing.

Thanks for review.

regards,
Santosh

[PATCH 4/8] clk: keystone: Build Keystone clock drivers

[Santosh Shilimkar]

Now build the keystone common clock drivers. The build is made
conditional based on COMMON_CLK_KEYSTONE

Cc: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/clk/Kconfig           |    7 +++++++
 drivers/clk/Makefile          |    1 +
 drivers/clk/keystone/Makefile |    1 +
 3 files changed, 9 insertions(+)
 create mode 100644 drivers/clk/keystone/Makefile

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 51380d6..45c01c6 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -87,6 +87,13 @@ config CLK_PPC_CORENET
 	  This adds the clock driver support for Freescale PowerPC corenet
 	  platforms using common clock framework.
 
+config COMMON_CLK_KEYSTONE
+	tristate "Clock drivers for Keystone based SOCs"
+	depends on ARCH_KEYSTONE && OF
+	---help---
+          Supports clock drivers for Keystone based SOCs. These SOCs have local
+	  a power sleep control module that gate the clock to the IPs and PLLs.
+
 endmenu
 
 source "drivers/clk/mvebu/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 4038c2b..ec9e290 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_VT8500)	+= clk-vt8500.o
 obj-$(CONFIG_ARCH_ZYNQ)		+= zynq/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-$(CONFIG_PLAT_SAMSUNG)	+= samsung/
+obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
 
 obj-$(CONFIG_X86)		+= x86/
 
diff --git a/drivers/clk/keystone/Makefile b/drivers/clk/keystone/Makefile
new file mode 100644
index 0000000..8706ca7
--- /dev/null
+++ b/drivers/clk/keystone/Makefile
@@ -0,0 +1 @@
+obj-y			+= clk.o pll.o gate.o
-- 
1.7.9.5

[PATCH 5/8] ARM: dts: keystone: Add clock tree data to devicetree

[Santosh Shilimkar]

Add clock tree for Keystone 2 based SOCs.

Cc: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/boot/dts/keystone-clocks.dtsi |  824 ++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/keystone.dts         |    2 +
 2 files changed, 826 insertions(+)
 create mode 100644 arch/arm/boot/dts/keystone-clocks.dtsi

diff --git a/arch/arm/boot/dts/keystone-clocks.dtsi b/arch/arm/boot/dts/keystone-clocks.dtsi
new file mode 100644
index 0000000..b8c6b3a
--- /dev/null
+++ b/arch/arm/boot/dts/keystone-clocks.dtsi
@@ -0,0 +1,824 @@
+/*
+ * Device Tree Source for Keystone 2 clock tree
+ *
+ * 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.
+ */
+
+clocks {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	refclkmain: refclkmain {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <122880000>;
+		clock-output-names = "refclk-main";
+	};
+
+	mainpllclk: mainpllclk at 2310110 {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclkmain>;
+		reg = <0x02310110 4	/* PLLCTRL PLLM */
+			0x02620350 4>;	/* MAINPLL_CTL0 */
+		pll_has_pllctrl;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7f000>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <2>;
+	};
+
+	mainmuxclk: mainmuxclk at 2310108 {
+		#clock-cells = <0>;
+		compatible = "mux-clk";
+		clocks = <&mainpllclk>, <&refclkmain>;
+		reg = <0x02310108 4>;
+		bit-shift = <23>;
+		bit-mask = <1>;
+		clock-output-names = "mainmuxclk";
+	};
+
+	chipclk1: chipclk1 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&mainmuxclk>;
+		clock-div = <1>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1";
+	};
+
+	chipclk1rstiso: chipclk1rstiso {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&mainmuxclk>;
+		clock-div = <1>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1rstiso";
+	};
+
+	gemtraceclk: gemtraceclk at 2310120 {
+		#clock-cells = <0>;
+		compatible = "divider-clock";
+		clocks = <&mainmuxclk>;
+		reg = <0x02310120 4>;
+		bit-shift = <0>;
+		bit-mask = <8>;
+		clock-output-names = "gemtraceclk";
+	};
+
+	chipstmxptclk: chipstmxptclk {
+		#clock-cells = <0>;
+		compatible = "divider-clock";
+		clocks = <&mainmuxclk>;
+		reg = <0x02310164 4>;
+		bit-shift = <0>;
+		bit-mask = <8>;
+		clock-output-names = "chipstmxptclk";
+	};
+
+	chipclk12: chipclk12 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <2>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk12";
+	};
+
+	chipclk13: chipclk13 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <3>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk13";
+	};
+
+	chipclk14: chipclk14 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <4>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk14";
+	};
+
+	chipclk16: chipclk16 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <6>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk16";
+	};
+
+	chipclk112: chipclk112 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <12>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk112";
+	};
+
+	chipclk124: chipclk124 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1>;
+		clock-div = <24>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk114";
+	};
+
+	chipclk1rstiso13: chipclk1rstiso13 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1rstiso>;
+		clock-div = <3>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1rstiso13";
+	};
+
+	chipclk1rstiso14: chipclk1rstiso14 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1rstiso>;
+		clock-div = <4>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1rstiso14";
+	};
+
+	chipclk1rstiso16: chipclk1rstiso16 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1rstiso>;
+		clock-div = <6>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1rstiso16";
+	};
+
+	chipclk1rstiso112: chipclk1rstiso112 {
+		#clock-cells = <0>;
+		compatible = "fixed-factor-clock";
+		clocks = <&chipclk1rstiso>;
+		clock-div = <12>;
+		clock-mult = <1>;
+		clock-output-names = "chipclk1rstiso112";
+	};
+
+	clkmodrst0: clkmodrst0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk16>;
+		clock-output-names = "modrst0";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+
+	clkusb: clkusb {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk16>;
+		clock-output-names = "usb";
+		reg = <0x02350000 4096>;
+		lpsc = <2>;
+	};
+
+	clkaemifspi: clkaemifspi {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk16>;
+		clock-output-names = "aemif-spi";
+		reg = <0x02350000 4096>;
+		status = "enabled";
+		lpsc = <3>;
+	};
+
+
+	clkdebugsstrc: clkdebugsstrc {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "debugss-trc";
+		reg = <0x02350000 4096>;
+		lpsc = <5>;
+		pd = <1>;
+	};
+
+	clktetbtrc: clktetbtrc {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tetb-trc";
+		reg = <0x02350000 4096>;
+		lpsc = <6>;
+		pd = <1>;
+	};
+
+	clkpa: clkpa {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk16>;
+		clock-output-names = "pa";
+		reg = <0x02350000 4096>;
+		lpsc = <7>;
+		pd = <2>;
+	};
+
+	clkcpgmac: clkcpgmac {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkpa>;
+		clock-output-names = "cpgmac";
+		reg = <0x02350000 4096>;
+		lpsc = <8>;
+		pd = <2>;
+	};
+
+	clksa: clksa {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkpa>;
+		clock-output-names = "sa";
+		reg = <0x02350000 4096>;
+		lpsc = <9>;
+		pd = <2>;
+	};
+
+	clkpcie: clkpcie {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk12>;
+		clock-output-names = "pcie";
+		reg = <0x02350000 4096>;
+		lpsc = <10>;
+		pd = <3>;
+	};
+
+	clksrio: clksrio {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1rstiso13>;
+		clock-output-names = "srio";
+		reg = <0x02350000 4096>;
+		lpsc = <11>;
+		pd = <4>;
+	};
+
+	clkhyperlink0: clkhyperlink0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk12>;
+		clock-output-names = "hyperlink-0";
+		reg = <0x02350000 4096>;
+		lpsc = <12>;
+		pd = <5>;
+	};
+
+	clksr: clksr {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1rstiso112>;
+		clock-output-names = "sr";
+		reg = <0x02350000 4096>;
+		lpsc = <13>;
+		pd = <6>;
+	};
+
+	clkmsmcsram: clkmsmcsram {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "msmcsram";
+		reg = <0x02350000 4096>;
+		lpsc = <14>;
+		pd = <7>;
+	};
+
+	clkgem0: clkgem0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem0";
+		reg = <0x02350000 4096>;
+		lpsc = <15>;
+		pd = <8>;
+	};
+
+	clkgem1: clkgem1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem1";
+		reg = <0x02350000 4096>;
+		lpsc = <16>;
+		pd = <9>;
+	};
+
+	clkgem2: clkgem2 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem2";
+		reg = <0x02350000 4096>;
+		lpsc = <17>;
+		pd = <10>;
+	};
+
+	clkgem3: clkgem3 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem3";
+		reg = <0x02350000 4096>;
+		lpsc = <18>;
+		pd = <11>;
+	};
+
+	clkgem4: clkgem4 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem4";
+		reg = <0x02350000 4096>;
+		lpsc = <19>;
+		pd = <12>;
+	};
+
+	clkgem5: clkgem5 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem5";
+		reg = <0x02350000 4096>;
+		lpsc = <20>;
+		pd = <13>;
+	};
+
+	clkgem6: clkgem6 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem6";
+		reg = <0x02350000 4096>;
+		lpsc = <21>;
+		pd = <14>;
+	};
+
+	clkgem7: clkgem7 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk1>;
+		clock-output-names = "gem7";
+		reg = <0x02350000 4096>;
+		lpsc = <22>;
+		pd = <15>;
+	};
+
+	clkddr30: clkddr30 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk12>;
+		clock-output-names = "ddr3-0";
+		reg = <0x02350000 4096>;
+		lpsc = <23>;
+		pd = <16>;
+	};
+
+	clkddr31: clkddr31 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "ddr3-1";
+		reg = <0x02350000 4096>;
+		lpsc = <24>;
+		pd = <16>;
+	};
+
+	clktac: clktac {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tac";
+		reg = <0x02350000 4096>;
+		lpsc = <25>;
+		pd = <17>;
+	};
+
+	clkrac01: clktac01 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "rac-01";
+		reg = <0x02350000 4096>;
+		lpsc = <26>;
+		pd = <17>;
+	};
+
+	clkrac23: clktac23 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "rac-23";
+		reg = <0x02350000 4096>;
+		lpsc = <27>;
+		pd = <18>;
+	};
+
+	clkfftc0: clkfftc0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-0";
+		reg = <0x02350000 4096>;
+		lpsc = <28>;
+		pd = <19>;
+	};
+
+	clkfftc1: clkfftc1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-1";
+		reg = <0x02350000 4096>;
+		lpsc = <29>;
+		pd = <19>;
+	};
+
+	clkfftc2: clkfftc2 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-2";
+		reg = <0x02350000 4096>;
+		lpsc = <30>;
+		pd = <20>;
+	};
+
+	clkfftc3: clkfftc3 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-3";
+		reg = <0x02350000 4096>;
+		lpsc = <31>;
+		pd = <20>;
+	};
+
+	clkfftc4: clkfftc4 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-4";
+		reg = <0x02350000 4096>;
+		lpsc = <32>;
+		pd = <20>;
+	};
+
+	clkfftc5: clkfftc5 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "fftc-5";
+		reg = <0x02350000 4096>;
+		lpsc = <33>;
+		pd = <20>;
+	};
+
+	clkaif: clkaif {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "aif";
+		reg = <0x02350000 4096>;
+		lpsc = <34>;
+		pd = <21>;
+	};
+
+	clktcp3d0: clktcp3d0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tcp3d-0";
+		reg = <0x02350000 4096>;
+		lpsc = <35>;
+		pd = <22>;
+	};
+
+	clktcp3d1: clktcp3d1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tcp3d-1";
+		reg = <0x02350000 4096>;
+		lpsc = <36>;
+		pd = <22>;
+	};
+
+	clktcp3d2: clktcp3d2 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tcp3d-2";
+		reg = <0x02350000 4096>;
+		lpsc = <37>;
+		pd = <23>;
+	};
+
+	clktcp3d3: clktcp3d3 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "tcp3d-3";
+		reg = <0x02350000 4096>;
+		lpsc = <38>;
+		pd = <23>;
+	};
+
+	clkvcp0: clkvcp0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-0";
+		reg = <0x02350000 4096>;
+		lpsc = <39>;
+		pd = <24>;
+	};
+
+	clkvcp1: clkvcp1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-1";
+		reg = <0x02350000 4096>;
+		lpsc = <40>;
+		pd = <24>;
+	};
+
+	clkvcp2: clkvcp2 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-2";
+		reg = <0x02350000 4096>;
+		lpsc = <41>;
+		pd = <24>;
+	};
+
+	clkvcp3: clkvcp3 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-3";
+		reg = <0x02350000 4096>;
+		lpsc = <42>;
+		pd = <24>;
+	};
+
+	clkvcp4: clkvcp4 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-4";
+		reg = <0x02350000 4096>;
+		lpsc = <43>;
+		pd = <25>;
+	};
+
+	clkvcp5: clkvcp5 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-5";
+		reg = <0x02350000 4096>;
+		lpsc = <44>;
+		pd = <25>;
+	};
+
+	clkvcp6: clkvcp6 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-6";
+		reg = <0x02350000 4096>;
+		lpsc = <45>;
+		pd = <25>;
+	};
+
+	clkvcp7: clkvcp7 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "vcp-7";
+		reg = <0x02350000 4096>;
+		lpsc = <46>;
+		pd = <25>;
+	};
+
+	clkbcp: clkbcp {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "bcp";
+		reg = <0x02350000 4096>;
+		lpsc = <47>;
+		pd = <26>;
+	};
+
+	clkdxb: clkdxb {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "dxb";
+		reg = <0x02350000 4096>;
+		lpsc = <48>;
+		pd = <27>;
+	};
+
+	clkhyperlink1: clkhyperlink1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk12>;
+		clock-output-names = "hyperlink-1";
+		reg = <0x02350000 4096>;
+		lpsc = <49>;
+		pd = <28>;
+	};
+
+	clkxge: clkxge {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&chipclk13>;
+		clock-output-names = "xge";
+		reg = <0x02350000 4096>;
+		lpsc = <50>;
+		pd = <29>;
+	};
+
+	clkwdtimer0: clkwdtimer0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "timer0";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkwdtimer1: clkwdtimer1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "timer1";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkwdtimer2: clkwdtimer2 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "timer2";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkwdtimer3: clkwdtimer3 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "timer3";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkuart0: clkuart0 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "uart0";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkuart1: clkuart1 {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "uart1";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkaemif: clkaemif {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkaemifspi>;
+		clock-output-names = "aemif";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkusim: clkusim {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "usim";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clki2c: clki2c {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "i2c";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkspi: clkspi {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkaemifspi>;
+		clock-output-names = "spi";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkgpio: clkgpio {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "gpio";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	clkkeymgr: clkkeymgr {
+		#clock-cells = <0>;
+		compatible = "keystone,psc-clk";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "keymgr";
+		status = "enabled";
+		reg = <0x02350000 4096>;
+	};
+
+	papllclk: papllclk at 2620358 {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclkmain>;
+		clock-output-names = "pa-pll-clk";
+		reg = <0x02620358 4>;	/* PAPLL_CTL0 */
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7ffc0>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <6>;
+	};
+
+	ddr3allclk: ddr3apllclk at 2620360 {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclkmain>;
+		clock-output-names = "ddr-3a-pll-clk";
+		reg = <0x02620360 4>;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7ffc0>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <6>;
+	};
+
+	ddr3bllclk: ddr3bpllclk at 2620368 {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclkmain>;
+		clock-output-names = "ddr-3b-pll-clk";
+		reg = <0x02620368 4>;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7ffc0>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <6>;
+	};
+
+	armpllclk: armpllclk at 2620370 {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclkmain>;
+		clock-output-names = "arm-pll-clk";
+		reg = <0x02620370 4>;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7ffc0>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <6>;
+	};
+};
diff --git a/arch/arm/boot/dts/keystone.dts b/arch/arm/boot/dts/keystone.dts
index 1334b42..57a7cd9 100644
--- a/arch/arm/boot/dts/keystone.dts
+++ b/arch/arm/boot/dts/keystone.dts
@@ -93,6 +93,8 @@
 			reg = <0x023100e8 4>;	/* pll reset control reg */
 		};
 
+		/include/ "keystone-clocks.dtsi"
+
 		uart0: serial at 02530c00 {
 			compatible = "ns16550a";
 			current-speed = <115200>;
-- 
1.7.9.5

[PATCH 6/8] ARM: dts: keystone: Add clock phandle to UART nodes

[Santosh Shilimkar]

Now since the clock tree is added, update UART dt nodes with clock data.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/boot/dts/keystone.dts |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/keystone.dts b/arch/arm/boot/dts/keystone.dts
index 57a7cd9..5a2d632 100644
--- a/arch/arm/boot/dts/keystone.dts
+++ b/arch/arm/boot/dts/keystone.dts
@@ -102,6 +102,7 @@
 			reg-io-width = <4>;
 			reg = <0x02530c00 0x100>;
 			clock-frequency = <133120000>;
+			clocks	= <&clkuart0>;
 			interrupts = <0 277 0xf01>;
 		};
 
@@ -112,6 +113,7 @@
 			reg-io-width = <4>;
 			reg = <0x02531000 0x100>;
 			clock-frequency = <133120000>;
+			clocks	= <&clkuart1>;
 			interrupts = <0 280 0xf01>;
 		};
 
-- 
1.7.9.5

[PATCH 7/8] ARM: keystone: Enable and initialise clock drivers

[Santosh Shilimkar]

Enable and initialise common clock drivers on Keystone 2
based SOCs.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-keystone/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
index 51a50e9..f912776 100644
--- a/arch/arm/mach-keystone/Kconfig
+++ b/arch/arm/mach-keystone/Kconfig
@@ -10,6 +10,7 @@ config ARCH_KEYSTONE
 	select HAVE_SCHED_CLOCK
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select ARM_ERRATA_798181 if SMP
+	select COMMON_CLK_KEYSTONE
 	help
 	  Support for boards based on the Texas Instruments Keystone family of
 	  SoCs.
-- 
1.7.9.5

[PATCH 8/8] ARM: keystone: add PM bus support for clock management

[Santosh Shilimkar]

Add runtime PM core support to Keystone SOCs by using the pm_clk
infrastructure of the PM core. Patch is based on Kevin's pm_domain
work on DaVinci SOCs.

Keystone SOC doesn't have depedency to enable clocks in early
in the boot and hence the clock and PM bus initialisation is done
at subsystem_init() level at least for now.

Cc: Kevin Hilman <khilman@linaro.org>

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/bus/Makefile          |    2 ++
 drivers/bus/keystone_pm_bus.c |   68 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 drivers/bus/keystone_pm_bus.c

diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 8947bdd..cb7f304 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
 obj-$(CONFIG_OMAP_INTERCONNECT)	+= omap_l3_smx.o omap_l3_noc.o
 # CCI cache coherent interconnect for ARM platforms
 obj-$(CONFIG_ARM_CCI)		+= arm-cci.o
+# PM bus driver for Keystone SOCs
+obj-$(CONFIG_ARCH_KEYSTONE)	+= keystone_pm_bus.o
diff --git a/drivers/bus/keystone_pm_bus.c b/drivers/bus/keystone_pm_bus.c
new file mode 100644
index 0000000..cbb4000
--- /dev/null
+++ b/drivers/bus/keystone_pm_bus.c
@@ -0,0 +1,68 @@
+/*
+ * PM Bus driver for Keystone2 devices
+ *
+ * Copyright 2013 Texas Instruments, Inc.
+ *	Santosh Shilimkar <santosh.shillimkar@ti.com>
+ *
+ * Based on Kevins work on DAVINCI SOCs
+ *	Kevin Hilman <khilman@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_clock.h>
+#include <linux/platform_device.h>
+
+#ifdef CONFIG_PM_RUNTIME
+static int keystone_pm_runtime_suspend(struct device *dev)
+{
+	int ret;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	ret = pm_generic_runtime_suspend(dev);
+	if (ret)
+		return ret;
+
+	ret = pm_clk_suspend(dev);
+	if (ret) {
+		pm_generic_runtime_resume(dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int keystone_pm_runtime_resume(struct device *dev)
+{
+	dev_dbg(dev, "%s\n", __func__);
+
+	pm_clk_resume(dev);
+
+	return pm_generic_runtime_resume(dev);
+}
+#endif
+
+static struct dev_pm_domain keystone_pm_bus = {
+	.ops = {
+		SET_RUNTIME_PM_OPS(keystone_pm_runtime_suspend,
+				   keystone_pm_runtime_resume, NULL)
+		USE_PLATFORM_PM_SLEEP_OPS
+	},
+};
+
+static struct pm_clk_notifier_block platform_bus_notifier = {
+	.pm_domain = &keystone_pm_bus,
+};
+
+int __init keystone_pm_runtime_init(void)
+{
+	pm_clk_add_notifier(&platform_bus_type, &platform_bus_notifier);
+
+	return 0;
+}
+subsystem_init(keystone_pm_runtime_init);
-- 
1.7.9.5