clk: keystone: Add common clock drivers and PM bus

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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +15 -0

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

drivers/clk/keystone/pll.c new file +16 -0

new file mode 100644
index 0000000..1453eea
@@ -0,0 +1,197 @@
+/*
+ * Main PLL clk driver for Keystone devices
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <[email protected]>
+ *	Santosh Shilimkar <[email protected]>
+ *
+ * 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>

drivers/clk/keystone/pll.c new file +6 -0

+ * @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.

drivers/clk/keystone/pll.c new file +10 -0

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

drivers/clk/keystone/pll.c new file +17 -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;
+

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 17

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 45 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +15 -0

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

drivers/clk/keystone/pll.c new file +24 -0

new file mode 100644
index 0000000..1453eea
@@ -0,0 +1,197 @@
+/*
+ * Main PLL clk driver for Keystone devices
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <[email protected]>
+ *	Santosh Shilimkar <[email protected]>
+ *
+ * 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

drivers/clk/keystone/pll.c line 26

Quoted 2 lines

drivers/clk/keystone/pll.c new file +2 -0

+ * @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.

Quoted 10 lines

drivers/clk/keystone/pll.c new file +10 -0

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

drivers/clk/keystone/pll.c line 42

Quoted 6 lines

drivers/clk/keystone/pll.c new file +6 -0

+ * @pllm_upper_shift: multiplier upper shift
+ * @plld_mask: divider mask
+ * @fixed_postdiv: Post divider
+ */
+struct clk_pll_data {
+	unsigned char has_pllctrl;

bool?

Quoted 11 lines

drivers/clk/keystone/pll.c new file +10 -0

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

[...]

drivers/clk/keystone/pll.c line 147

Quoted 13 lines

drivers/clk/keystone/pll.c new file +13 -0

+/**
+ * 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?

drivers/clk/keystone/pll.c line 151

Quoted 4 lines

drivers/clk/keystone/pll.c new file +4 -0

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

of_property_read_bool?

drivers/clk/keystone/pll.c line 158

Quoted 7 lines

drivers/clk/keystone/pll.c new file +7 -0

+		/* 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?

drivers/clk/keystone/pll.c line 167

Quoted 9 lines

drivers/clk/keystone/pll.c new file +9 -0

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

Quoted 22 lines

drivers/clk/keystone/pll.c new file +22 -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;

drivers/clk/keystone/pll.c line 194

Quoted 5 lines

drivers/clk/keystone/pll.c new file +5 -0

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

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 5 lines

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.

Quoted 10 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 4 lines

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.

Quoted 3 lines

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

u32

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 2 lines

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?

Quoted 87 lines · Mark Rutland

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 7 lines

Quoted 2 lines

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.

Mark,

Quoted 88 lines · Santosh Shilimkar

Quoted 87 lines · Mark Rutland

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

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

Quoting Santosh Shilimkar (2013-08-19 10:42:19)

Quoted 100 lines

Mark,

Quoted 88 lines · Santosh Shilimkar

Quoted 87 lines · Mark Rutland

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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).

Quoted 113 lines

Quoting Santosh Shilimkar (2013-08-19 10:42:19)

Quoted 100 lines

Mark,

Quoted 88 lines · Santosh Shilimkar

Quoted 87 lines · Mark Rutland

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 12 lines

Quoted 5 lines

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).

Quoted 6 lines

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

Quoting Santosh Shilimkar (2013-08-20 06:41:21)

Quoted 144 lines · Mike Turquette

Quoted 113 lines

Quoting Santosh Shilimkar (2013-08-19 10:42:19)

Quoted 100 lines

Mark,

Quoted 88 lines · Santosh Shilimkar

Quoted 87 lines · Mark Rutland

Quoted 60 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

[Adding dt maintainers]

Quoted 10 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---

Thanks for review Mark.

Quoted 30 lines

Quoted 16 lines

 .../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

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +2 -0

new file mode 100644
index 0000000..58f6470
@@ -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

Documentation/devicetree/bindings/clock/keystone-pll.txt line 12

Quoted 10 lines

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

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

Quoted 2 lines

Perhaps "ti,keystone-pll-clock" ?

Agree.

Quoted 9 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 1 line

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +1 -0

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 14 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-pll.txt new file +4 -0

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

Quoted 9 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 18 lines

Quoted 5 lines

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).

Quoted 4 lines

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

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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt new file +11 -0

+
+Example:
+       clock {
+               #clock-cells = <0>;
+               compatible = "keystone,psc-clk";
+               clocks = <&chipclk3>;
+               clock-output-names = "debugss_trc";
+               reg = <0x02350000 4096>;
+               lpsc = <5>;
+               pd = <1>;
+       };

drivers/clk/keystone/gate.c new file +194 -0

new file mode 100644
index 0000000..72ac478
@@ -0,0 +1,306 @@
+/*
+ * Clock driver for Keystone 2 based devices
+ *
+ * Copyright (C) 2013 Texas Instruments.
+ *     Murali Karicheri <[email protected]>
+ *     Santosh Shilimkar <[email protected]>
+ *
+ * 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;

drivers/clk/keystone/gate.c new file +15 -0

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

drivers/clk/keystone/gate.c new file +16 -0

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

include/linux/clk/keystone.h +1 -1

index 1ade95d..7b3e154 100644
@@ -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

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 1 line

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 19

Quoted 1 line

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

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

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

Quoted 5 lines

drivers/clk/keystone/gate.c new file +0 -0

new file mode 100644
index 0000000..72ac478

Quoted 16 lines

drivers/clk/keystone/gate.c new file +16 -0

+/**
+ * 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;

drivers/clk/keystone/gate.c line 204

Quoted 10 lines

drivers/clk/keystone/gate.c new file +10 -0

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

drivers/clk/keystone/gate.c line 209

Quoted 5 lines

drivers/clk/keystone/gate.c new file +5 -0

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

drivers/clk/keystone/gate.c line 278

Quoted 9 lines

drivers/clk/keystone/gate.c new file +9 -0

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

Quoted 7 lines

drivers/clk/keystone/gate.c new file +7 -0

+
+       clk = clk_register_psc(NULL, clk_name, parent_name,
+                               data, lock);
+
+       if (clk) {
+               of_clk_add_provider(node, of_clk_src_simple_get, clk);
+

drivers/clk/keystone/gate.c line 290

Quoted 5 lines

drivers/clk/keystone/gate.c new file +5 -0

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

Quoted 49 lines · Mark Rutland

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 5 lines

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.

Quoted 8 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Quoted 1 line

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

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

Quoted 5 lines

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.

Quoted 5 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 19

Quoted 2 lines

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

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

Quoted 6 lines

Quoted 6 lines

drivers/clk/keystone/gate.c new file +0 -0

new file mode 100644
index 0000000..72ac478

[..]

Quoted 36 lines

Quoted 16 lines

drivers/clk/keystone/gate.c new file +16 -0

+/**
+ * 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;

drivers/clk/keystone/gate.c line 204

Quoted 10 lines

drivers/clk/keystone/gate.c new file +10 -0

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

Quoted 5 lines

drivers/clk/keystone/gate.c new file +5 -0

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

Quoted 4 lines

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.

Quoted 11 lines

Quoted 10 lines

drivers/clk/keystone/gate.c new file +9 -0

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

Quoted 4 lines

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.

Quoted 17 lines

drivers/clk/keystone/gate.c line 290

Quoted 13 lines

drivers/clk/keystone/gate.c new file +12 -0

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

Quoted 50 lines · Mark Rutland

Quoted 49 lines

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Quoted 2 lines

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?).

Quoted 7 lines

Quoted 6 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 19

Quoted 3 lines

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

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

Quoted 45 lines

Quoted 6 lines

Quoted 6 lines

drivers/clk/keystone/gate.c new file +0 -0

new file mode 100644
index 0000000..72ac478

[..]

Quoted 36 lines

Quoted 16 lines

drivers/clk/keystone/gate.c new file +16 -0

+/**
+ * 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;

drivers/clk/keystone/gate.c line 204

Quoted 10 lines

drivers/clk/keystone/gate.c new file +10 -0

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

Quoted 5 lines

drivers/clk/keystone/gate.c new file +5 -0

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

Quoted 5 lines

Quoted 4 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

drivers/clk/keystone/gate.c new file +9 -0

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

Quoted 7 lines

Quoted 4 lines

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.

Quoted 17 lines

Quoted 17 lines

drivers/clk/keystone/gate.c line 290

Quoted 13 lines

drivers/clk/keystone/gate.c new file +12 -0

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

Quoted 3 lines

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.

Quoting Mark Rutland (2013-08-13 09:53:44)

Quoted 73 lines · Santosh Shilimkar

Quoted 50 lines · Mark Rutland

Quoted 49 lines

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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

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

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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.

Quoted 95 lines

Quoting Mark Rutland (2013-08-13 09:53:44)

Quoted 73 lines · Santosh Shilimkar

Quoted 50 lines · Mark Rutland

Quoted 49 lines

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Quoted 7 lines

Quoted 2 lines

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";
      ...
};

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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

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

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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

Quoting Santosh Shilimkar (2013-08-20 06:55:54)

Quoted 99 lines · Mike Turquette

Quoted 98 lines

Quoting Mark Rutland (2013-08-13 09:53:44)

Quoted 73 lines · Santosh Shilimkar

Quoted 50 lines · Mark Rutland

Quoted 49 lines

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 100 lines

Quoting Santosh Shilimkar (2013-08-20 06:55:54)

Quoted 99 lines · Mike Turquette

Quoted 98 lines

Quoting Mark Rutland (2013-08-13 09:53:44)

Quoted 73 lines · Santosh Shilimkar

Quoted 50 lines · Mark Rutland

Quoted 49 lines

[adding dt maintainers]

Quoted 21 lines · 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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 .../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

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

new file mode 100644
index 0000000..40afef5

Documentation/devicetree/bindings/clock/keystone-gate.txt line 8

Quoted 8 lines

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

@@ -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"

Documentation/devicetree/bindings/clock/keystone-gate.txt line 16

Quoted 8 lines

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

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/keystone-gate.txt line 17

Quoted 2 lines

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

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

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

Documentation/devicetree/bindings/clock/keystone-gate.txt line 18

Quoted 1 line

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

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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoting Santosh Shilimkar (2013-08-20 14:55:56)

Quoted 1 line · Mike Turquette

Quoted 16 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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?

Quoted 2 lines

Quoting Santosh Shilimkar (2013-08-20 14:55:56)

Quoted 1 line · Mike Turquette

[...]

Quoted 16 lines

Quoted 16 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

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

Quoting Santosh Shilimkar (2013-08-20 15:54:15)

Quoted 22 lines · Mike Turquette

Quoted 2 lines

Quoting Santosh Shilimkar (2013-08-20 14:55:56)

Quoted 1 line · Mike Turquette

[...]

Quoted 16 lines

Quoted 16 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 3 lines

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 ;-)

Quoted 52 lines

Quoting Santosh Shilimkar (2013-08-20 15:54:15)

Quoted 22 lines · Mike Turquette

Quoted 2 lines

Quoting Santosh Shilimkar (2013-08-20 14:55:56)

Quoted 1 line · Mike Turquette

[...]

Quoted 16 lines

Quoted 16 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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>;
      };
};

Quoted 4 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoting Santosh Shilimkar (2013-08-21 06:16:33)

Quoted 57 lines · Mike Turquette

Quoted 52 lines

Quoting Santosh Shilimkar (2013-08-20 15:54:15)

Quoted 22 lines · Mike Turquette

Quoted 2 lines

Quoting Santosh Shilimkar (2013-08-20 14:55:56)

Quoted 1 line · Mike Turquette

[...]

Quoted 16 lines

Quoted 16 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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>;
      };
};

Quoted 4 lines

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.

Quoted 6 lines

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).

Initialise common clock drivers for Keystone 2 devices.

Cc Mike Turquette <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

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

drivers/clk/keystone/clk.c new file +34 -0

new file mode 100644
index 0000000..9001380
@@ -0,0 +1,34 @@
+/*
+ * Common Clock initialization code for Keystone SOCs
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <[email protected]>
+ *	Santosh Shilimkar <[email protected]>
+ *
+ * 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);
+}

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

index 7b3e154..282e88f 100644

Quoted 61 lines · Santosh Shilimkar

Initialise common clock drivers for Keystone 2 devices.

Cc Mike Turquette <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

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

drivers/clk/keystone/clk.c new file +34 -0

new file mode 100644
index 0000000..9001380
@@ -0,0 +1,34 @@
+/*
+ * Common Clock initialization code for Keystone SOCs
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *    Murali Karicheri <[email protected]>
+ *    Santosh Shilimkar <[email protected]>
+ *
+ * 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);
+}

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

index 7b3e154..282e88f 100644
@@ -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);
 

include/linux/clk/keystone.h line 20

Quoted 2 lines

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

  #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.