[PATCH 06/13] clk: versatile-icst: convert to use regmap

[PATCH 06/13] clk: versatile-icst: convert to use regmap

[Linus Walleij]

Instead of passing around register bases, pass around a regmap
in this driver. This refactoring make things so much easier when
we later want to manage an ICST that is part of a syscon.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm looking for an ACK from the CLK maintainers to take this
through the ARM SoC tree once the series stabilize.
---
 drivers/clk/versatile/Kconfig    |  1 +
 drivers/clk/versatile/clk-icst.c | 88 +++++++++++++++++++++++++++-------------
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/versatile/Kconfig b/drivers/clk/versatile/Kconfig
index 1530c9352a76..e091bcec116c 100644
--- a/drivers/clk/versatile/Kconfig
+++ b/drivers/clk/versatile/Kconfig
@@ -1,6 +1,7 @@
 config COMMON_CLK_VERSATILE
 	bool "Clock driver for ARM Reference designs"
 	depends on ARCH_INTEGRATOR || ARCH_REALVIEW || ARCH_VEXPRESS || ARM64
+	select REGMAP_MMIO
 	---help---
           Supports clocking on ARM Reference designs:
 	  - Integrator/AP and Integrator/CP
diff --git a/drivers/clk/versatile/clk-icst.c b/drivers/clk/versatile/clk-icst.c
index a3893ea2199d..da5a3ccbbb96 100644
--- a/drivers/clk/versatile/clk-icst.c
+++ b/drivers/clk/versatile/clk-icst.c
@@ -19,9 +19,13 @@
 #include <linux/err.h>
 #include <linux/clk-provider.h>
 #include <linux/io.h>
+#include <linux/regmap.h>
 
 #include "clk-icst.h"
 
+/* Magic unlocking token used on all Versatile boards */
+#define VERSATILE_LOCK_VAL	0xA05F
+
 /**
  * struct clk_icst - ICST VCO clock wrapper
  * @hw: corresponding clock hardware entry
@@ -32,8 +36,9 @@
  */
 struct clk_icst {
 	struct clk_hw hw;
-	void __iomem *vcoreg;
-	void __iomem *lockreg;
+	struct regmap *map;
+	u32 vcoreg_off;
+	u32 lockreg_off;
 	struct icst_params *params;
 	unsigned long rate;
 };
@@ -41,53 +46,67 @@ struct clk_icst {
 #define to_icst(_hw) container_of(_hw, struct clk_icst, hw)
 
 /**
- * vco_get() - get ICST VCO settings from a certain register
- * @vcoreg: register containing the VCO settings
+ * vco_get() - get ICST VCO settings from a certain ICST
+ * @icst: the ICST clock to get
+ * @vco: the VCO struct to return the value in
  */
-static struct icst_vco vco_get(void __iomem *vcoreg)
+static int vco_get(struct clk_icst *icst, struct icst_vco *vco)
 {
 	u32 val;
-	struct icst_vco vco;
-
-	val = readl(vcoreg);
-	vco.v = val & 0x1ff;
-	vco.r = (val >> 9) & 0x7f;
-	vco.s = (val >> 16) & 03;
-	return vco;
+	int ret;
+
+	ret = regmap_read(icst->map, icst->vcoreg_off, &val);
+	if (ret)
+		return ret;
+	vco->v = val & 0x1ff;
+	vco->r = (val >> 9) & 0x7f;
+	vco->s = (val >> 16) & 03;
+	return 0;
 }
 
 /**
  * vco_set() - commit changes to an ICST VCO
- * @locreg: register to poke to unlock the VCO for writing
- * @vcoreg: register containing the VCO settings
- * @vco: ICST VCO parameters to commit
+ * @icst: the ICST clock to set
+ * @vco: the VCO struct to set the changes from
  */
-static void vco_set(void __iomem *lockreg,
-			void __iomem *vcoreg,
-			struct icst_vco vco)
+static int vco_set(struct clk_icst *icst, struct icst_vco vco)
 {
 	u32 val;
+	int ret;
 
-	val = readl(vcoreg) & ~0x7ffff;
+	ret = regmap_read(icst->map, icst->vcoreg_off, &val);
+	if (ret)
+		return ret;
 	val |= vco.v | (vco.r << 9) | (vco.s << 16);
 
 	/* This magic unlocks the VCO so it can be controlled */
-	writel(0xa05f, lockreg);
-	writel(val, vcoreg);
+	ret = regmap_write(icst->map, icst->lockreg_off, VERSATILE_LOCK_VAL);
+	if (ret)
+		return ret;
+	ret = regmap_write(icst->map, icst->vcoreg_off, val);
+	if (ret)
+		return ret;
 	/* This locks the VCO again */
-	writel(0, lockreg);
+	ret = regmap_write(icst->map, icst->lockreg_off, 0);
+	if (ret)
+		return ret;
+	return 0;
 }
 
-
 static unsigned long icst_recalc_rate(struct clk_hw *hw,
 				      unsigned long parent_rate)
 {
 	struct clk_icst *icst = to_icst(hw);
 	struct icst_vco vco;
+	int ret;
 
 	if (parent_rate)
 		icst->params->ref = parent_rate;
-	vco = vco_get(icst->vcoreg);
+	ret = vco_get(icst, &vco);
+	if (ret) {
+		pr_err("ICST: could not get VCO setting\n");
+		return 0;
+	}
 	icst->rate = icst_hz(icst->params, vco);
 	return icst->rate;
 }
@@ -112,8 +131,7 @@ static int icst_set_rate(struct clk_hw *hw, unsigned long rate,
 		icst->params->ref = parent_rate;
 	vco = icst_hz_to_vco(icst->params, rate);
 	icst->rate = icst_hz(icst->params, vco);
-	vco_set(icst->lockreg, icst->vcoreg, vco);
-	return 0;
+	return vco_set(icst, vco);
 }
 
 static const struct clk_ops icst_ops = {
@@ -132,6 +150,11 @@ struct clk *icst_clk_register(struct device *dev,
 	struct clk_icst *icst;
 	struct clk_init_data init;
 	struct icst_params *pclone;
+	struct regmap_config icst_regmap_conf = {
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+	};
 
 	icst = kzalloc(sizeof(struct clk_icst), GFP_KERNEL);
 	if (!icst) {
@@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
 	init.flags = CLK_IS_ROOT;
 	init.parent_names = (parent_name ? &parent_name : NULL);
 	init.num_parents = (parent_name ? 1 : 0);
+	icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+	if (IS_ERR(icst->map)) {
+		int ret;
+
+		pr_err("could not initialize ICST regmap\n");
+		kfree(icst);
+		ret = PTR_ERR(icst->map);
+		return ERR_PTR(ret);
+	}
 	icst->hw.init = &init;
 	icst->params = pclone;
-	icst->vcoreg = base + desc->vco_offset;
-	icst->lockreg = base + desc->lock_offset;
+	icst->vcoreg_off = desc->vco_offset;
+	icst->lockreg_off = desc->lock_offset;
 
 	clk = clk_register(dev, &icst->hw);
 	if (IS_ERR(clk))
-- 
2.4.3

[PATCH 07/13] clk: versatile-icst: refactor to allocate regmap separately

[Linus Walleij]

Break out the registration function so it creates a regmap and
pass to the setup function, so the latter can be shared with
a device tree probe function that already has a regmap.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm looking for an ACK from the CLK maintainers to take this
through the ARM SoC tree once the series stabilize.
---
 drivers/clk/versatile/clk-icst.c | 44 +++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/versatile/clk-icst.c b/drivers/clk/versatile/clk-icst.c
index da5a3ccbbb96..ab5d5f73818b 100644
--- a/drivers/clk/versatile/clk-icst.c
+++ b/drivers/clk/versatile/clk-icst.c
@@ -20,6 +20,7 @@
 #include <linux/clk-provider.h>
 #include <linux/io.h>
 #include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 
 #include "clk-icst.h"
 
@@ -140,21 +141,16 @@ static const struct clk_ops icst_ops = {
 	.set_rate = icst_set_rate,
 };
 
-struct clk *icst_clk_register(struct device *dev,
+struct clk *icst_clk_setup(struct device *dev,
 			const struct clk_icst_desc *desc,
 			const char *name,
 			const char *parent_name,
-			void __iomem *base)
+			struct regmap *map)
 {
 	struct clk *clk;
 	struct clk_icst *icst;
 	struct clk_init_data init;
 	struct icst_params *pclone;
-	struct regmap_config icst_regmap_conf = {
-		.reg_bits = 32,
-		.val_bits = 32,
-		.reg_stride = 4,
-	};
 
 	icst = kzalloc(sizeof(struct clk_icst), GFP_KERNEL);
 	if (!icst) {
@@ -174,15 +170,7 @@ struct clk *icst_clk_register(struct device *dev,
 	init.flags = CLK_IS_ROOT;
 	init.parent_names = (parent_name ? &parent_name : NULL);
 	init.num_parents = (parent_name ? 1 : 0);
-	icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
-	if (IS_ERR(icst->map)) {
-		int ret;
-
-		pr_err("could not initialize ICST regmap\n");
-		kfree(icst);
-		ret = PTR_ERR(icst->map);
-		return ERR_PTR(ret);
-	}
+	icst->map = map;
 	icst->hw.init = &init;
 	icst->params = pclone;
 	icst->vcoreg_off = desc->vco_offset;
@@ -194,4 +182,28 @@ struct clk *icst_clk_register(struct device *dev,
 
 	return clk;
 }
+
+struct clk *icst_clk_register(struct device *dev,
+			const struct clk_icst_desc *desc,
+			const char *name,
+			const char *parent_name,
+			void __iomem *base)
+{
+	struct regmap_config icst_regmap_conf = {
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+	};
+	struct regmap *map;
+
+	map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+	if (IS_ERR(map)) {
+		int ret;
+
+		pr_err("could not initialize ICST regmap\n");
+		ret = PTR_ERR(map);
+		return ERR_PTR(ret);
+	}
+	return icst_clk_setup(dev, desc, name, parent_name, map);
+}
 EXPORT_SYMBOL_GPL(icst_clk_register);
-- 
2.4.3

[PATCH 08/13] clk: add ARM syscon ICST device tree bindings

[Linus Walleij]

This adds the device tree bindings for the ARM Syscon ICST
oscillators, which is a register-level interface to the
Integrated Device Technology (IDT) ICS525 and ICS307
serially programmable oscillators.

Cc: devicetree@vger.kernel.org
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm looking for an ACK from the CLK maintainers to take this
through the ARM SoC tree once the series stabilize.
---
 .../devicetree/bindings/clock/arm-syscon-icst.txt  | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/arm-syscon-icst.txt

diff --git a/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt b/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt
new file mode 100644
index 000000000000..19eb3aa765c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt
@@ -0,0 +1,40 @@
+ARM System Controller ICST clocks
+
+The ICS525 and ICS307 oscillators are produced by Integrated Devices
+Technology (IDT). ARM integrated these oscillators deeply into their
+reference designs by adding special control registers that manage such
+oscillators to their system controllers.
+
+The ARM system controller contains logic to serialized and initialize
+an ICST clock request after a write to the 32 bit register at an offset
+into the system controller. Further, to even be able to alter one of
+these frequencies, the system controller must first be unlocked by
+writing a special token to another offset in the system controller.
+
+The ICST oscillator must be provided inside a system controller node.
+
+Required properties:
+- lock-offset: the offset address into the system controller where the
+  unlocking register is located
+- vco-offset: the offset address into the system controller where the
+  ICST control register is located (even 32 bit address)
+- compatible: must be one of "arm,syscon-icst525" or "arm,syscon-icst307"
+- #clock-cells: must be <0>
+- clocks: parent clock, since the ICST needs a parent clock to derive its
+  frequency from, this attribute is compulsory.
+
+Example:
+
+syscon: syscon@10000000 {
+	compatible = "syscon";
+	reg = <0x10000000 0x1000>;
+
+	oscclk0: osc0@0c {
+		compatible = "arm,syscon-icst307";
+		#clock-cells = <0>;
+		lock-offset = <0x20>;
+		vco-offset = <0x0C>;
+		clocks = <&xtal24mhz>;
+	};
+	(...)
+};
-- 
2.4.3

[PATCH 09/13] clk: versatile-icst: add device tree support

[Linus Walleij]

This adds support for the ARM syscon ICST clocks to initialized
directly from the device tree syscon node on ARM Integrator,
Versatile and RealView reference designs.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm looking for an ACK from the CLK maintainers to take this
through the ARM SoC tree once the series stabilize.
---
 drivers/clk/versatile/clk-icst.c | 89 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/versatile/clk-icst.c b/drivers/clk/versatile/clk-icst.c
index ab5d5f73818b..b2dc06923ac1 100644
--- a/drivers/clk/versatile/clk-icst.c
+++ b/drivers/clk/versatile/clk-icst.c
@@ -3,7 +3,7 @@
  * We wrap the custom interface from <asm/hardware/icst.h> into the generic
  * clock framework.
  *
- * Copyright (C) 2012 Linus Walleij
+ * Copyright (C) 2012-2015 Linus Walleij
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -207,3 +207,90 @@ struct clk *icst_clk_register(struct device *dev,
 	return icst_clk_setup(dev, desc, name, parent_name, map);
 }
 EXPORT_SYMBOL_GPL(icst_clk_register);
+
+#ifdef CONFIG_OF
+/*
+ * In a device tree, an memory-mapped ICST clock appear as a child
+ * of a syscon node. Assume this and probe it only as a child of a
+ * syscon.
+ */
+
+static const struct icst_params icst525_params = {
+	.vco_max	= ICST525_VCO_MAX_5V,
+	.vco_min	= ICST525_VCO_MIN,
+	.vd_min 	= 8,
+	.vd_max 	= 263,
+	.rd_min 	= 3,
+	.rd_max 	= 65,
+	.s2div		= icst525_s2div,
+	.idx2s		= icst525_idx2s,
+};
+
+static const struct icst_params icst307_params = {
+	.vco_max	= ICST307_VCO_MAX,
+	.vco_min	= ICST307_VCO_MIN,
+	.vd_min		= 4 + 8,
+	.vd_max		= 511 + 8,
+	.rd_min		= 1 + 2,
+	.rd_max		= 127 + 2,
+	.s2div		= icst307_s2div,
+	.idx2s		= icst307_idx2s,
+};
+
+static void __init of_syscon_icst_setup(struct device_node *np)
+{
+	struct device_node *parent;
+	struct regmap *map;
+	struct clk_icst_desc icst_desc;
+	const char *name = np->name;
+	const char *parent_name;
+	struct clk *regclk;
+
+	/* We do not release this reference, we are using it perpetually */
+	parent = of_get_parent(np);
+	if (!parent) {
+		pr_err("no parent node for syscon ICST clock\n");
+		return;
+	}
+	map = syscon_node_to_regmap(parent);
+	if (IS_ERR(map)) {
+		pr_err("no regmap for syscon ICST clock parent\n");
+		return;
+	}
+
+	if (of_property_read_u32(np, "vco-offset", &icst_desc.vco_offset)) {
+		pr_err("no VCO register offset for ICST clock\n");
+		return;
+	}
+	if (of_property_read_u32(np, "lock-offset", &icst_desc.lock_offset)) {
+		pr_err("no lock register offset for ICST clock\n");
+		return;
+	}
+
+	if (of_device_is_compatible(np, "arm,syscon-icst525"))
+		icst_desc.params = &icst525_params;
+	else if (of_device_is_compatible(np, "arm,syscon-icst307"))
+		icst_desc.params = &icst307_params;
+	else {
+		pr_info("unknown ICST clock %s\n", name);
+		return;
+	}
+
+	/* Parent clock name is not the same as node parent */
+	parent_name = of_clk_get_parent_name(np, 0);
+
+	regclk = icst_clk_setup(NULL, &icst_desc, name, parent_name, map);
+	if (IS_ERR(regclk)) {
+		pr_err("error setting up syscon ICST clock %s\n", name);
+		return;
+	}
+	of_clk_add_provider(np, of_clk_src_simple_get, regclk);
+	pr_info("registered syscon ICST clock %s\n", name);
+}
+
+CLK_OF_DECLARE(arm_syscon_icst525_clk,
+	       "arm,syscon-icst525", of_syscon_icst_setup);
+CLK_OF_DECLARE(arm_syscon_icst307_clk,
+	       "arm,syscon-icst307", of_syscon_icst_setup);
+
+#endif
-- 
2.4.3

Re: [PATCH 06/13] clk: versatile-icst: convert to use regmap

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> @@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
>  	init.flags = CLK_IS_ROOT;
>  	init.parent_names = (parent_name ? &parent_name : NULL);
>  	init.num_parents = (parent_name ? 1 : 0);
> +	icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
> +	if (IS_ERR(icst->map)) {
> +		int ret;
> +
> +		pr_err("could not initialize ICST regmap\n");
> +		kfree(icst);
> +		ret = PTR_ERR(icst->map);

drivers/clk/versatile/clk-icst.c:183
icst_clk_register() error: dereferencing freed memory 'icst'
drivers/clk/versatile/clk-icst.c:184
icst_clk_register() warn: possible memory leak of 'pclone'


> +		return ERR_PTR(ret);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/13] clk: versatile-icst: refactor to allocate regmap separately

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> @@ -174,15 +170,7 @@ struct clk *icst_clk_register(struct device *dev,
>  	init.flags = CLK_IS_ROOT;
>  	init.parent_names = (parent_name ? &parent_name : NULL);
>  	init.num_parents = (parent_name ? 1 : 0);
> -	icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
> -	if (IS_ERR(icst->map)) {
> -		int ret;
> -
> -		pr_err("could not initialize ICST regmap\n");
> -		kfree(icst);
> -		ret = PTR_ERR(icst->map);
> -		return ERR_PTR(ret);

Oh now it gets moved.

> -	}
> +	icst->map = map;
>  	icst->hw.init = &init;
>  	icst->params = pclone;
>  	icst->vcoreg_off = desc->vco_offset;
> @@ -194,4 +182,28 @@ struct clk *icst_clk_register(struct device *dev,
>  
>  	return clk;
>  }
> +
> +struct clk *icst_clk_register(struct device *dev,
> +			const struct clk_icst_desc *desc,
> +			const char *name,
> +			const char *parent_name,
> +			void __iomem *base)
> +{
> +	struct regmap_config icst_regmap_conf = {
> +		.reg_bits = 32,
> +		.val_bits = 32,
> +		.reg_stride = 4,
> +	};
> +	struct regmap *map;
> +
> +	map = regmap_init_mmio(NULL, base, &icst_regmap_conf);

Any reason we aren't passing dev as the first argument? Same
comment applies to patch 6.

> +	if (IS_ERR(map)) {
> +		int ret;
> +
> +		pr_err("could not initialize ICST regmap\n");
> +		ret = PTR_ERR(map);
> +		return ERR_PTR(ret);

Use ERR_CAST(map) instead.

> +	}
> +	return icst_clk_setup(dev, desc, name, parent_name, map);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/13] clk: add ARM syscon ICST device tree bindings

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> diff --git a/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt b/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt
> new file mode 100644
> index 000000000000..19eb3aa765c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/arm-syscon-icst.txt
> @@ -0,0 +1,40 @@
> +ARM System Controller ICST clocks
> +
> +The ICS525 and ICS307 oscillators are produced by Integrated Devices
> +Technology (IDT). ARM integrated these oscillators deeply into their
> +reference designs by adding special control registers that manage such
> +oscillators to their system controllers.
> +
> +The ARM system controller contains logic to serialized and initialize

serialize ?

> +an ICST clock request after a write to the 32 bit register at an offset
> +into the system controller. Further, to even be able to alter one of

Furthermore?

> +these frequencies, the system controller must first be unlocked by
> +writing a special token to another offset in the system controller.

Sounds like a great design!

> +
> +The ICST oscillator must be provided inside a system controller node.
> +
> +Required properties:
> +- lock-offset: the offset address into the system controller where the
> +  unlocking register is located
> +- vco-offset: the offset address into the system controller where the
> +  ICST control register is located (even 32 bit address)

Is there any reason why we don't use a reg property for this?

> +- compatible: must be one of "arm,syscon-icst525" or "arm,syscon-icst307"
> +- #clock-cells: must be <0>
> +- clocks: parent clock, since the ICST needs a parent clock to derive its
> +  frequency from, this attribute is compulsory.
> +
> +Example:
> +
> +syscon: syscon@10000000 {
> +	compatible = "syscon";
> +	reg = <0x10000000 0x1000>;
> +
> +	oscclk0: osc0@0c {
> +		compatible = "arm,syscon-icst307";
> +		#clock-cells = <0>;
> +		lock-offset = <0x20>;
> +		vco-offset = <0x0C>;

lowercase the C?

> +		clocks = <&xtal24mhz>;
> +	};
> +	(...)
> +};

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/13] clk: versatile-icst: add device tree support

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> +
> +	if (of_device_is_compatible(np, "arm,syscon-icst525"))
> +		icst_desc.params = &icst525_params;
> +	else if (of_device_is_compatible(np, "arm,syscon-icst307"))
> +		icst_desc.params = &icst307_params;

I guess if we add anymore here we should use an of_device_id
array instead.

> +	else {
> +		pr_info("unknown ICST clock %s\n", name);

pr_warn? pr_err?

> +		return;
> +	}
> +
> +	/* Parent clock name is not the same as node parent */
> +	parent_name = of_clk_get_parent_name(np, 0);
> +
> +	regclk = icst_clk_setup(NULL, &icst_desc, name, parent_name, map);
> +	if (IS_ERR(regclk)) {
> +		pr_err("error setting up syscon ICST clock %s\n", name);
> +		return;
> +	}
> +	of_clk_add_provider(np, of_clk_src_simple_get, regclk);
> +	pr_info("registered syscon ICST clock %s\n", name);

debug print? Please remove debug noise.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/13] clk: versatile-icst: add device tree support

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> This adds support for the ARM syscon ICST clocks to initialized
> directly from the device tree syscon node on ARM Integrator,
> Versatile and RealView reference designs.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Also:

WARNING: please, no space before tabs
#47: FILE: drivers/clk/versatile/clk-icst.c:221:
+^I.vd_min ^I= 8,$

WARNING: please, no space before tabs
#48: FILE: drivers/clk/versatile/clk-icst.c:222:
+^I.vd_max ^I= 263,$

WARNING: please, no space before tabs
#49: FILE: drivers/clk/versatile/clk-icst.c:223:
+^I.rd_min ^I= 3,$

WARNING: please, no space before tabs
#50: FILE: drivers/clk/versatile/clk-icst.c:224:
+^I.rd_max ^I= 65,$

total: 0 errors, 4 warnings, 98 lines checked

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/13] clk: versatile-icst: refactor to allocate regmap separately

[Stephen Boyd]

On 10/15, Linus Walleij wrote:
> @@ -140,21 +141,16 @@ static const struct clk_ops icst_ops = {
>  	.set_rate = icst_set_rate,
>  };
>  
> -struct clk *icst_clk_register(struct device *dev,
> +struct clk *icst_clk_setup(struct device *dev,

This needs static now.

>  			const struct clk_icst_desc *desc,
>  			const char *name,
>  			const char *parent_name,
> -			void __iomem *base)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/13] clk: versatile-icst: convert to use regmap

[Linus Walleij]

On Thu, Oct 15, 2015 at 9:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/15, Linus Walleij wrote:
>> @@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
>>       init.flags = CLK_IS_ROOT;
>>       init.parent_names = (parent_name ? &parent_name : NULL);
>>       init.num_parents = (parent_name ? 1 : 0);
>> +     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
>> +     if (IS_ERR(icst->map)) {
>> +             int ret;
>> +
>> +             pr_err("could not initialize ICST regmap\n");
>> +             kfree(icst);
>> +             ret = PTR_ERR(icst->map);
>
> drivers/clk/versatile/clk-icst.c:183
> icst_clk_register() error: dereferencing freed memory 'icst'
> drivers/clk/versatile/clk-icst.c:184
> icst_clk_register() warn: possible memory leak of 'pclone'

The pclone warning is correct, nice catch. (Fixing it.)

But for the second warning, whatever static checker you're
using for this is unable to handle error pointers:

    clk = clk_register(dev, &icst->hw);
    if (IS_ERR(clk))
        kfree(icst);

    return clk;

It is quite obvious that returning clk (which may be an error code)
is OK here.

If you want, I may need to add some specific annotation to shut
up the static checker, any hints?

Yours,
Linus Walleij

Re: [PATCH 06/13] clk: versatile-icst: convert to use regmap

[Linus Walleij]

On Fri, Oct 23, 2015 at 11:27 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Oct 15, 2015 at 9:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 10/15, Linus Walleij wrote:
>>> @@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
>>>       init.flags = CLK_IS_ROOT;
>>>       init.parent_names = (parent_name ? &parent_name : NULL);
>>>       init.num_parents = (parent_name ? 1 : 0);
>>> +     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
>>> +     if (IS_ERR(icst->map)) {
>>> +             int ret;
>>> +
>>> +             pr_err("could not initialize ICST regmap\n");
>>> +             kfree(icst);
>>> +             ret = PTR_ERR(icst->map);
>>
>> drivers/clk/versatile/clk-icst.c:183
>> icst_clk_register() error: dereferencing freed memory 'icst'
>> drivers/clk/versatile/clk-icst.c:184
>> icst_clk_register() warn: possible memory leak of 'pclone'
>
> The pclone warning is correct, nice catch. (Fixing it.)

Turns out that this was around for ages, so sent a separate
fix for -stable.

Yours,
Linus Walleij

Re: [PATCH 08/13] clk: add ARM syscon ICST device tree bindings

[Linus Walleij]

On Thu, Oct 15, 2015 at 9:23 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/15, Linus Walleij wrote:

>> +Required properties:
>> +- lock-offset: the offset address into the system controller where the
>> +  unlocking register is located
>> +- vco-offset: the offset address into the system controller where the
>> +  ICST control register is located (even 32 bit address)
>
> Is there any reason why we don't use a reg property for this?

Usually reg = <> is used with two (or more) tokens:

reg = <phys_addr size>;

The exception being things like I2C addresses which
are just one token.

Since in this case, there is a "mother" reg property in the
syscon-compatible node, which we are indexing into,
it is confusing to use the same name for subnodes.

Also there is a bunch of precedents doing it like this
for sybdevices to system controllers, just
git grep offset Documentation/devicetree/bindings
will give you a bunch of them.

(Fixing the spelling comments.)

Yours,
Linus Walleij

Re: [PATCH 06/13] clk: versatile-icst: convert to use regmap

[Stephen Boyd]

On 10/23, Linus Walleij wrote:
> On Thu, Oct 15, 2015 at 9:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 10/15, Linus Walleij wrote:
> >> @@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
> >>       init.flags = CLK_IS_ROOT;
> >>       init.parent_names = (parent_name ? &parent_name : NULL);
> >>       init.num_parents = (parent_name ? 1 : 0);
> >> +     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
> >> +     if (IS_ERR(icst->map)) {
> >> +             int ret;
> >> +
> >> +             pr_err("could not initialize ICST regmap\n");
> >> +             kfree(icst);
> >> +             ret = PTR_ERR(icst->map);
> >
> > drivers/clk/versatile/clk-icst.c:183
> > icst_clk_register() error: dereferencing freed memory 'icst'
> > drivers/clk/versatile/clk-icst.c:184
> > icst_clk_register() warn: possible memory leak of 'pclone'
> 
> The pclone warning is correct, nice catch. (Fixing it.)
> 
> But for the second warning, whatever static checker you're
> using for this is unable to handle error pointers:
> 
>     clk = clk_register(dev, &icst->hw);
>     if (IS_ERR(clk))
>         kfree(icst);
> 
>     return clk;
> 
> It is quite obvious that returning clk (which may be an error code)
> is OK here.
> 
> If you want, I may need to add some specific annotation to shut
> up the static checker, any hints?
> 

Huh? I'm totally lost. I use sparse and smatch.

> >> +             kfree(icst);
> >> +             ret = PTR_ERR(icst->map);

We just freed icst, and then we dereferenced it in the next line.
What does that have to do with error pointers?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/13] clk: add ARM syscon ICST device tree bindings

[Stephen Boyd]

On 10/23, Linus Walleij wrote:
> On Thu, Oct 15, 2015 at 9:23 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 10/15, Linus Walleij wrote:
> 
> >> +Required properties:
> >> +- lock-offset: the offset address into the system controller where the
> >> +  unlocking register is located
> >> +- vco-offset: the offset address into the system controller where the
> >> +  ICST control register is located (even 32 bit address)
> >
> > Is there any reason why we don't use a reg property for this?
> 
> Usually reg = <> is used with two (or more) tokens:
> 
> reg = <phys_addr size>;
> 
> The exception being things like I2C addresses which
> are just one token.
> 
> Since in this case, there is a "mother" reg property in the
> syscon-compatible node, which we are indexing into,
> it is confusing to use the same name for subnodes.
> 
> Also there is a bunch of precedents doing it like this
> for sybdevices to system controllers, just
> git grep offset Documentation/devicetree/bindings
> will give you a bunch of them.
> 

Ok. I'm no DT expert, but it seems odd to have subnodes without a
reg property.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/13] clk: versatile-icst: add device tree support

[Linus Walleij]

On Thu, Oct 15, 2015 at 9:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/15, Linus Walleij wrote:
>> +
>> +     if (of_device_is_compatible(np, "arm,syscon-icst525"))
>> +             icst_desc.params = &icst525_params;
>> +     else if (of_device_is_compatible(np, "arm,syscon-icst307"))
>> +             icst_desc.params = &icst307_params;
>
> I guess if we add anymore here we should use an of_device_id
> array instead.

As it happens those two are gonna be it.

ARM never created any more integrated ICST devices, and
they stopped using them since. Those two are the required
ones.

(Fixing all other comments.)

Yours,
Linus Walleij

Re: [PATCH 09/13] clk: versatile-icst: add device tree support

[Russell King - ARM Linux]

On Mon, Oct 26, 2015 at 02:14:15PM +0100, Linus Walleij wrote:
> On Thu, Oct 15, 2015 at 9:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 10/15, Linus Walleij wrote:
> >> +
> >> +     if (of_device_is_compatible(np, "arm,syscon-icst525"))
> >> +             icst_desc.params = &icst525_params;
> >> +     else if (of_device_is_compatible(np, "arm,syscon-icst307"))
> >> +             icst_desc.params = &icst307_params;
> >
> > I guess if we add anymore here we should use an of_device_id
> > array instead.
> 
> As it happens those two are gonna be it.
> 
> ARM never created any more integrated ICST devices, and
> they stopped using them since. Those two are the required
> ones.

As ARM didn't create any ICST devices at all, that's hardly surprising.
These devices are created by Integrated Circuit Systems, Inc.  The 525
is a parallel-loaded clock generator, the 307 is a serial-loaded clock
generator.

However, they have no "standard" software interface - indeed, the 525
is marketed as a device that needs no processor or software to be used,
so it doesn't have a "software" interface as such.

ARM Ltd's hardware on these boards provides interfaces to these, however
the underlying ICST support I wrote was factored to separate out the
interface from the chip support - I haven't been tracking what's been
going on with these, but I hope that separation has been kept as it's
entirely logical, and describing these things in DT as an ARM Ltd device,
combining the ICST device itself with its interface would be wrong.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 06/13] clk: versatile-icst: convert to use regmap

[Linus Walleij]

On Fri, Oct 23, 2015 at 6:24 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/23, Linus Walleij wrote:
>> On Thu, Oct 15, 2015 at 9:08 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 10/15, Linus Walleij wrote:
>> >> @@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
>> >>       init.flags = CLK_IS_ROOT;
>> >>       init.parent_names = (parent_name ? &parent_name : NULL);
>> >>       init.num_parents = (parent_name ? 1 : 0);
>> >> +     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
>> >> +     if (IS_ERR(icst->map)) {
>> >> +             int ret;
>> >> +
>> >> +             pr_err("could not initialize ICST regmap\n");
>> >> +             kfree(icst);
>> >> +             ret = PTR_ERR(icst->map);
>> >
>> > drivers/clk/versatile/clk-icst.c:183
>> > icst_clk_register() error: dereferencing freed memory 'icst'
>> > drivers/clk/versatile/clk-icst.c:184
>> > icst_clk_register() warn: possible memory leak of 'pclone'
>>
>> The pclone warning is correct, nice catch. (Fixing it.)
>>
>> But for the second warning, whatever static checker you're
>> using for this is unable to handle error pointers:
>>
>>     clk = clk_register(dev, &icst->hw);
>>     if (IS_ERR(clk))
>>         kfree(icst);
>>
>>     return clk;
>>
>> It is quite obvious that returning clk (which may be an error code)
>> is OK here.
>>
>> If you want, I may need to add some specific annotation to shut
>> up the static checker, any hints?
>>
>
> Huh? I'm totally lost. I use sparse and smatch.
>
>> >> +             kfree(icst);
>> >> +             ret = PTR_ERR(icst->map);
>
> We just freed icst, and then we dereferenced it in the next line.
> What does that have to do with error pointers?

Sorry I must have confused patch versions.  :(

I see the real problem now, so will make a v3 of this.

Yours,
Linus Walleij

Re: [PATCH 09/13] clk: versatile-icst: add device tree support

[Linus Walleij]

On Mon, Oct 26, 2015 at 2:31 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> [Me]
>> ARM never created any more integrated ICST devices, and
>> they stopped using them since. Those two are the required
>> ones.
>
> As ARM didn't create any ICST devices at all, that's hardly surprising.
> These devices are created by Integrated Circuit Systems, Inc.  The 525
> is a parallel-loaded clock generator, the 307 is a serial-loaded clock
> generator.
>
> However, they have no "standard" software interface - indeed, the 525
> is marketed as a device that needs no processor or software to be used,
> so it doesn't have a "software" interface as such.

RIght, I wrote a blurb with the more elaborate and correct story
for the device tree bindings. I get sloppy sometimes.

> ARM Ltd's hardware on these boards provides interfaces to these, however
> the underlying ICST support I wrote was factored to separate out the
> interface from the chip support - I haven't been tracking what's been
> going on with these, but I hope that separation has been kept as it's
> entirely logical, and describing these things in DT as an ARM Ltd device,
> combining the ICST device itself with its interface would be wrong.

So the device tree bindings does say that, and the compatible strings
are "arm,syscon-icst525" or "arm,syscon-icst307" indicating that it is
indeed the ARM syscon register-mapped thing, and the ICST sits on
the back of that register.

The logical separation is indeed kept, if someone ever decides to
interface the ICST clocks in some other way, the code is reusable.

Yours,
Linus Walleij