[PATCH 0/4] CCF support for Renesas r7s72100

[PATCH 0/4] CCF support for Renesas r7s72100

[Wolfram Sang]

This series moves the r7s72100 platfrom from the legacy clock implementation to
the common clock framework. Only the essential clocks are added in this series,
and other clocks will be added incrementally once the basic stuff is accepted.

This series has been tested with the renesas-devel-v3.14-rc4-20140227 tag. It
has a build dependency [1] because it uses the clkdev workaround. If you want
to boot into a console, you also have a runtime dependency [2]. A git branch
can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/r7s-ccf

[1] http://www.spinics.net/lists/arm-kernel/msg310279.html (ARM: shmobile: Introduce shmobile_clk_workaround())
[2] http://lkml.org/lkml/2014/2/20/205 ([PATCH V7 0/3] clocksource: Consolidate SH and ARM mach-shmobile Kconfig bits)


Wolfram Sang (4):
  ARM: r7s72100: add clock nodes to dtsi
  ARM: r7s72100: genmai: populate extal clock node
  clk: shmobile: add CPG driver for rz-platforms
  ARM: shmobile: r7s72100: use workaround for non DT-clocks

 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   4 +
 arch/arm/boot/dts/r7s72100.dtsi                 |  78 ++++++++++++++++-
 arch/arm/mach-shmobile/board-genmai-reference.c |  15 +++-
 drivers/clk/shmobile/Makefile                   |   1 +
 drivers/clk/shmobile/clk-rz.c                   | 112 ++++++++++++++++++++++++
 include/dt-bindings/clock/r7s72100-clock.h      |  25 ++++++
 6 files changed, 232 insertions(+), 3 deletions(-)
 create mode 100644 drivers/clk/shmobile/clk-rz.c
 create mode 100644 include/dt-bindings/clock/r7s72100-clock.h

-- 
1.8.5.1

[PATCH 1/4] ARM: r7s72100: add clock nodes to dtsi

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

Only essential clocks are added for now. Other clocks will be added when
needed.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---
 arch/arm/boot/dts/r7s72100.dtsi            | 78 +++++++++++++++++++++++++++++-
 include/dt-bindings/clock/r7s72100-clock.h | 25 ++++++++++
 2 files changed, 102 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/clock/r7s72100-clock.h

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ee70071..f6d4b5d 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -1,13 +1,15 @@
 /*
  * Device Tree Source for the r7s72100 SoC
  *
- * Copyright (C) 2013 Renesas Solutions Corp.
+ * Copyright (C) 2013-14 Renesas Solutions Corp.
+ * Copyright (C) 2014 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
  *
  * This file is licensed under the terms of the GNU General Public License
  * version 2.  This program is licensed "as is" without any warranty of any
  * kind, whether express or implied.
  */
 
+#include <dt-bindings/clock/r7s72100-clock.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 
 / {
@@ -28,6 +30,80 @@
 		spi4 = &spi4;
 	};
 
+	clocks {
+		ranges;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		/* External root clock */
+		extal_clk: extal_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			/* This value must be overriden by the board. */
+			clock-frequency = <0>;
+			clock-output-names = "extal";
+		};
+
+		/* Special CPG clocks */
+		cpg_clocks: cpg_clocks at fcfe0000 {
+			#clock-cells = <1>;
+			compatible = "renesas,r7s72100-cpg-clocks",
+				     "renesas,rz-cpg-clocks";
+			reg = <0xfcfe0000 0x18>;
+			clocks = <&extal_clk>;
+			clock-output-names = "pll", "i", "g";
+		};
+
+		/* Fixed factor clocks */
+		b_clk: b_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-factor-clock";
+			clocks = <&cpg_clocks 0>;
+			clock-mult = <1>;
+			clock-div = <3>;
+			clock-output-names = "b";
+		};
+		p1_clk: p1_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-factor-clock";
+			clocks = <&cpg_clocks 0>;
+			clock-mult = <1>;
+			clock-div = <6>;
+			clock-output-names = "p1";
+		};
+		p0_clk: p0_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-factor-clock";
+			clocks = <&cpg_clocks 0>;
+			clock-mult = <1>;
+			clock-div = <12>;
+			clock-output-names = "p0";
+		};
+
+		/* MSTP clocks */
+		mstp3_clks: mstp3_clks at fcfe0420 {
+			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
+			reg = <0xfcfe0420 4>;
+			clocks = <&p0_clk>;
+			#clock-cells = <1>;
+			renesas,clock-indices = <R7S72100_CLK_MTU2>;
+			clock-output-names = "mtu2";
+		};
+
+		mstp4_clks: mstp4_clks at fcfe0424 {
+			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
+			reg = <0xfcfe0424 4>;
+			clocks = <&p1_clk>, <&p1_clk>, <&p1_clk>, <&p1_clk>,
+				 <&p1_clk>, <&p1_clk>, <&p1_clk>, <&p1_clk>;
+			#clock-cells = <1>;
+			renesas,clock-indices = <
+				R7S72100_CLK_SCIF0 R7S72100_CLK_SCIF1 R7S72100_CLK_SCIF2 R7S72100_CLK_SCIF3
+				R7S72100_CLK_SCIF4 R7S72100_CLK_SCIF5 R7S72100_CLK_SCIF6 R7S72100_CLK_SCIF7
+			>;
+			clock-output-names = "scif0", "scif1", "scif2", "scif3", "scif4", "scif5", "scif6", "scif7";
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h
new file mode 100644
index 0000000..eced0a8
--- /dev/null
+++ b/include/dt-bindings/clock/r7s72100-clock.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2014 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_R7S72100_H__
+#define __DT_BINDINGS_CLOCK_R7S72100_H__
+
+/* MSTP3 */
+#define R7S72100_CLK_MTU2	3
+
+/* MSTP4 */
+#define R7S72100_CLK_SCIF0	7
+#define R7S72100_CLK_SCIF1	6
+#define R7S72100_CLK_SCIF2	5
+#define R7S72100_CLK_SCIF3	4
+#define R7S72100_CLK_SCIF4	3
+#define R7S72100_CLK_SCIF5	2
+#define R7S72100_CLK_SCIF6	1
+#define R7S72100_CLK_SCIF7	0
+
+#endif /* __DT_BINDINGS_CLOCK_R7S72100_H__ */
-- 
1.8.5.1

[PATCH 2/4] ARM: r7s72100: genmai: populate extal clock node

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---
 arch/arm/boot/dts/r7s72100-genmai-reference.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
index e664611..1d045eb 100644
--- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
@@ -30,6 +30,10 @@
 	};
 };
 
+&extal_clk {
+	clock-frequency = <13330000>;
+};
+
 &i2c2 {
 	status = "okay";
 	clock-frequency = <400000>;
-- 
1.8.5.1

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---

Mike: if you are fine with this driver, it would be good if you could apply it.
Then, we can deal with the orthogonal dependencies in mach-shmobile seperately
and know that the driver is already in place when the rest gets resolved.
Thanks!

 drivers/clk/shmobile/Makefile |   1 +
 drivers/clk/shmobile/clk-rz.c | 112 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 drivers/clk/shmobile/clk-rz.c

diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
index 9ecef14..5404cb9 100644
--- a/drivers/clk/shmobile/Makefile
+++ b/drivers/clk/shmobile/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
+obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o
 obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o
 obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
diff --git a/drivers/clk/shmobile/clk-rz.c b/drivers/clk/shmobile/clk-rz.c
new file mode 100644
index 0000000..ec1c795
--- /dev/null
+++ b/drivers/clk/shmobile/clk-rz.c
@@ -0,0 +1,112 @@
+/*
+ * rz Core CPG Clocks
+ *
+ * Copyright (C) 2013 Ideas On Board SPRL
+ * Copyright (C) 2014 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+struct rz_cpg {
+	struct clk_onecell_data data;
+	void __iomem *reg;
+};
+
+#define CPG_FRQCR	0x10
+#define CPG_FRQCR2	0x14
+
+/* -----------------------------------------------------------------------------
+ * Initialization
+ */
+
+static struct clk * __init
+rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *name)
+{
+	u32 val;
+	unsigned mult, frqcr_tab[4] = { 3, 2, 0, 1 };
+
+	if (strcmp(name, "pll") == 0) {
+		/* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet */
+		unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */
+		const char *parent_name = of_clk_get_parent_name(np, 0);
+
+		mult = cpg_mode ? (32 / 4) : 30;
+		return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1);
+	}
+
+	/* FIXME:"i" and "g" are variable clocks with non-integer dividers (e.g. 2/3)
+	 * and the constraint that always g <= i. To get the rz platform started,
+	 * let them run@fixed current speed and implement the details later.
+	 */
+	if (strcmp(name, "i") == 0)
+		val = (clk_readl(cpg->reg + CPG_FRQCR) >> 8) & 3;
+	else if (strcmp(name, "g") == 0)
+		val = clk_readl(cpg->reg + CPG_FRQCR2) & 3;
+	else
+		return ERR_PTR(-EINVAL);
+
+	mult = frqcr_tab[val];
+	return clk_register_fixed_factor(NULL, name, "pll", 0, mult, 3);
+}
+
+static void __init rz_cpg_clocks_init(struct device_node *np)
+{
+	struct rz_cpg *cpg;
+	struct clk **clks;
+	unsigned i;
+	int num_clks;
+
+	num_clks = of_property_count_strings(np, "clock-output-names");
+	if (WARN(num_clks < 0, "can't count CPG clocks\n"))
+		goto out;
+
+	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
+	if (WARN(!cpg, "out of memory!\n"))
+		goto out;
+
+	clks = kzalloc(num_clks * sizeof(*clks), GFP_KERNEL);
+	if (WARN(!clks, "out of memory!\n"))
+		goto free_cpg;
+
+	cpg->data.clks = clks;
+	cpg->data.clk_num = num_clks;
+
+	cpg->reg = of_iomap(np, 0);
+	if (WARN(!cpg->reg, "can't remap CPG registers!\n"))
+		goto free_clks;
+
+	for (i = 0; i < num_clks; ++i) {
+		const char *name;
+		struct clk *clk;
+
+		of_property_read_string_index(np, "clock-output-names", i, &name);
+
+		clk = rz_cpg_register_clock(np, cpg, name);
+		if (IS_ERR(clk))
+			pr_err("%s: failed to register %s %s clock (%ld)\n",
+			       __func__, np->name, name, PTR_ERR(clk));
+		else
+			cpg->data.clks[i] = clk;
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+	return;
+
+free_clks:
+	kfree(clks);
+free_cpg:
+	kfree(cpg);
+out:
+	return;
+}
+CLK_OF_DECLARE(rz_cpg_clks, "renesas,rz-cpg-clocks",
+	       rz_cpg_clocks_init);
-- 
1.8.5.1

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 28 February 2014 22:09:27 Wolfram Sang wrote:
> From: Wolfram Sang <wsa@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> ---
> 
> Mike: if you are fine with this driver, it would be good if you could apply
> it. Then, we can deal with the orthogonal dependencies in mach-shmobile
> seperately and know that the driver is already in place when the rest gets
> resolved. Thanks!
> 
>  drivers/clk/shmobile/Makefile |   1 +
>  drivers/clk/shmobile/clk-rz.c | 112 +++++++++++++++++++++++++++++++++++++++

There's one large missing piece here: the DT bindings documentation.

>  2 files changed, 113 insertions(+)
>  create mode 100644 drivers/clk/shmobile/clk-rz.c
> 
> diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile
> index 9ecef14..5404cb9 100644
> --- a/drivers/clk/shmobile/Makefile
> +++ b/drivers/clk/shmobile/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
> +obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o
>  obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o
>  obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
> diff --git a/drivers/clk/shmobile/clk-rz.c b/drivers/clk/shmobile/clk-rz.c
> new file mode 100644
> index 0000000..ec1c795
> --- /dev/null
> +++ b/drivers/clk/shmobile/clk-rz.c
> @@ -0,0 +1,112 @@
> +/*
> + * rz Core CPG Clocks
> + *
> + * Copyright (C) 2013 Ideas On Board SPRL
> + * Copyright (C) 2014 Wolfram Sang, Sang Engineering
> <wsa@sang-engineering.com> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +struct rz_cpg {
> +	struct clk_onecell_data data;
> +	void __iomem *reg;
> +};
> +
> +#define CPG_FRQCR	0x10
> +#define CPG_FRQCR2	0x14
> +
> +/*
> ---------------------------------------------------------------------------
> -- + * Initialization
> + */
> +
> +static struct clk * __init
> +rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const
> char *name)
> +{
> +	u32 val;
> +	unsigned mult, frqcr_tab[4] = { 3, 2, 0, 1 };

I would declare the table as static const outside of this function.

> +
> +	if (strcmp(name, "pll") == 0) {
> +		/* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet 
*/
> +		unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */

It won't make a difference yet, but shouldn't this be moved to 
rz_cpg_clocks_init() ?

> +		const char *parent_name = of_clk_get_parent_name(np, 0);

You should select the parent depending on the mode (again it won't make any 
difference yet, but the code will be ready, and DT bindings should document 
two parents).

> +
> +		mult = cpg_mode ? (32 / 4) : 30;
> +		return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 
1);
> +	}
> +
> +	/* FIXME:"i" and "g" are variable clocks with non-integer dividers (e.g.
> 2/3)
> +	 * and the constraint that always g <= i. To get the rz platform started,
> +	 * let them run at fixed current speed and implement the details later.
> +	 */
> +	if (strcmp(name, "i") == 0)
> +		val = (clk_readl(cpg->reg + CPG_FRQCR) >> 8) & 3;
> +	else if (strcmp(name, "g") == 0)
> +		val = clk_readl(cpg->reg + CPG_FRQCR2) & 3;

The registers are 16 bits wide, are 32 bits accessed valid ? I suppose they 
work as you use them.

> +	else
> +		return ERR_PTR(-EINVAL);
> +
> +	mult = frqcr_tab[val];
> +	return clk_register_fixed_factor(NULL, name, "pll", 0, mult, 3);
> +}
> +
> +static void __init rz_cpg_clocks_init(struct device_node *np)
> +{
> +	struct rz_cpg *cpg;
> +	struct clk **clks;
> +	unsigned i;

unsigned int maybe ? I'm not sure what the preferred kernel coding style is.

> +	int num_clks;
> +
> +	num_clks = of_property_count_strings(np, "clock-output-names");
> +	if (WARN(num_clks < 0, "can't count CPG clocks\n"))

Do such failures really deserve a WARN ? Isn't a pr_err() enough ?

What if num_clks == 0 ?

> +		goto out;
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	if (WARN(!cpg, "out of memory!\n"))
> +		goto out;
> +
> +	clks = kzalloc(num_clks * sizeof(*clks), GFP_KERNEL);
> +	if (WARN(!clks, "out of memory!\n"))
> +		goto free_cpg;

Does kzalloc alloc warn internally when it fails to allocate memory ? If so 
you could remove the error message. You could also perform the two allocations 
and check the result once only.

> +	cpg->data.clks = clks;
> +	cpg->data.clk_num = num_clks;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN(!cpg->reg, "can't remap CPG registers!\n"))
> +		goto free_clks;
> +
> +	for (i = 0; i < num_clks; ++i) {
> +		const char *name;
> +		struct clk *clk;
> +
> +		of_property_read_string_index(np, "clock-output-names", i, &name);
> +
> +		clk = rz_cpg_register_clock(np, cpg, name);
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %s clock (%ld)\n",
> +			       __func__, np->name, name, PTR_ERR(clk));
> +		else
> +			cpg->data.clks[i] = clk;
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +	return;
> +
> +free_clks:
> +	kfree(clks);
> +free_cpg:
> +	kfree(cpg);

I would remove that as done in the other CPG drivers, given that a small 
memory leak when the system will anyway fail to boot isn't really an issue.

> +out:
> +	return;
> +}
> +CLK_OF_DECLARE(rz_cpg_clks, "renesas,rz-cpg-clocks",
> +	       rz_cpg_clocks_init);

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> >  drivers/clk/shmobile/Makefile |   1 +
> >  drivers/clk/shmobile/clk-rz.c | 112 +++++++++++++++++++++++++++++++++++++++
> 
> There's one large missing piece here: the DT bindings documentation.

Yes, noticed that, too. Added already.

> > +	u32 val;
> > +	unsigned mult, frqcr_tab[4] = { 3, 2, 0, 1 };
> 
> I would declare the table as static const outside of this function.

Yup.

> > +	if (strcmp(name, "pll") == 0) {
> > +		/* FIXME: cpg_mode should be read from GPIO. But no GPIO support yet 
> */
> > +		unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */
> 
> It won't make a difference yet, but shouldn't this be moved to 
> rz_cpg_clocks_init() ?

This is going to be a gpio_get_value() later and I'd prefer it in the
place where the value is needed.

> 
> > +		const char *parent_name = of_clk_get_parent_name(np, 0);
> 
> You should select the parent depending on the mode (again it won't make any 
> difference yet, but the code will be ready, and DT bindings should document 
> two parents).

Two parents? Yet, CPG only needs one as input. Either XTAL or USB_X1
which is board dependent. What I should do IMO is to put the parent
property for CPG from the dtsi to the board dts. Because this is
describing hardware. And from that parent, I can simply get its name
like above.

> > +	int num_clks;
> > +
> > +	num_clks = of_property_count_strings(np, "clock-output-names");
> > +	if (WARN(num_clks < 0, "can't count CPG clocks\n"))
> 
> Do such failures really deserve a WARN ? Isn't a pr_err() enough ?

Since the system won't probably boot, I'd think so. Also, it makes the
code more concise, no?

> What if num_clks == 0 ?

Good question.

> 
> > +		goto out;
> > +
> > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > +	if (WARN(!cpg, "out of memory!\n"))
> > +		goto out;
> > +
> > +	clks = kzalloc(num_clks * sizeof(*clks), GFP_KERNEL);
> > +	if (WARN(!clks, "out of memory!\n"))
> > +		goto free_cpg;
> 
> Does kzalloc alloc warn internally when it fails to allocate memory ?

Ehrm, why don't you simply look this up instead of asking me? :)

> you could remove the error message. You could also perform the two allocations 
> and check the result once only.

What is the gain? Code savings?

> > +free_clks:
> > +	kfree(clks);
> > +free_cpg:
> > +	kfree(cpg);
> 
> I would remove that as done in the other CPG drivers, given that a small 
> memory leak when the system will anyway fail to boot isn't really an issue.

Again, now that I already coded it, what is the gain to remove it? The
drawback is that other people might get encouraged to find reasons to
allow them sloppy practices.

Thanks,

   Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/aee9a2b4/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Laurent Pinchart]

Hi Wolfram,

On Monday 03 March 2014 10:19:04 Wolfram Sang wrote:
> > >  drivers/clk/shmobile/Makefile |   1 +
> > >  drivers/clk/shmobile/clk-rz.c | 112 +++++++++++++++++++++++++++++++++++
> > 
> > There's one large missing piece here: the DT bindings documentation.
> 
> Yes, noticed that, too. Added already.
> 
> > > +	u32 val;
> > > +	unsigned mult, frqcr_tab[4] = { 3, 2, 0, 1 };
> > 
> > I would declare the table as static const outside of this function.
> 
> Yup.
> 
> > > +	if (strcmp(name, "pll") == 0) {
> > > +		/* FIXME: cpg_mode should be read from GPIO. But no GPIO support
> > > yet */
> > > +		unsigned cpg_mode = 0; /* hardcoded to EXTAL for now */
> > 
> > It won't make a difference yet, but shouldn't this be moved to
> > rz_cpg_clocks_init() ?
> 
> This is going to be a gpio_get_value() later and I'd prefer it in the
> place where the value is needed.
> 
> > > +		const char *parent_name = of_clk_get_parent_name(np, 0);
> > 
> > You should select the parent depending on the mode (again it won't make
> > any difference yet, but the code will be ready, and DT bindings should
> > document two parents).
> 
> Two parents? Yet, CPG only needs one as input. Either XTAL or USB_X1
> which is board dependent. What I should do IMO is to put the parent
> property for CPG from the dtsi to the board dts. Because this is
> describing hardware. And from that parent, I can simply get its name
> like above.

While the parent is indeed selected at boot time only, and only one parent is 
thus needed, parent selection could be performed by a DIP switch connected to 
MD_CLK on the board for instance. In that case both parents should be 
available in DT, as selection will be done by the kernel at boot time, not at 
DT compile time.

> > > +	int num_clks;
> > > +
> > > +	num_clks = of_property_count_strings(np, "clock-output-names");
> > > +	if (WARN(num_clks < 0, "can't count CPG clocks\n"))
> > 
> > Do such failures really deserve a WARN ? Isn't a pr_err() enough ?
> 
> Since the system won't probably boot, I'd think so. Also, it makes the
> code more concise, no?
> 
> > What if num_clks == 0 ?
> 
> Good question.
> 
> > > +		goto out;
> > > +
> > > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > > +	if (WARN(!cpg, "out of memory!\n"))
> > > +		goto out;
> > > +
> > > +	clks = kzalloc(num_clks * sizeof(*clks), GFP_KERNEL);
> > > +	if (WARN(!clks, "out of memory!\n"))
> > > +		goto free_cpg;
> > 
> > Does kzalloc alloc warn internally when it fails to allocate memory ?
> 
> Ehrm, why don't you simply look this up instead of asking me? :)

I have and wasn't sure :-)

> > you could remove the error message. You could also perform the two
> > allocations and check the result once only.
> 
> What is the gain? Code savings?

Yes, code saving.

> > > +free_clks:
> > > +	kfree(clks);
> > > +free_cpg:
> > > +	kfree(cpg);
> > 
> > I would remove that as done in the other CPG drivers, given that a small
> > memory leak when the system will anyway fail to boot isn't really an
> > issue.
> 
> Again, now that I already coded it, what is the gain to remove it? The
> drawback is that other people might get encouraged to find reasons to
> allow them sloppy practices.

It will make the kernel binary smaller by removing code that is not needed in 
practice.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/6c45ed1e/attachment-0001.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> While the parent is indeed selected at boot time only, and only one parent is 
> thus needed, parent selection could be performed by a DIP switch connected to 
> MD_CLK on the board for instance. In that case both parents should be 
> available in DT, as selection will be done by the kernel at boot time, not at 
> DT compile time.

OK, I understand the case. I still wonder about specifying two parents,
though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
clock (or an empty one), just because USB_X1 is enumerated as second
entry?

> > Again, now that I already coded it, what is the gain to remove it? The
> > drawback is that other people might get encouraged to find reasons to
> > allow them sloppy practices.
> 
> It will make the kernel binary smaller by removing code that is not needed in 
> practice.

Sounds like a micro-optimazation to me. If you insist, we should BUG()
right away in that case, to make the code comprehensible. Returning with
a leak and saying "we will probably fail somewhere" should really be
avoided IMO.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/059fadbc/attachment-0001.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Laurent Pinchart]

Hi Wolfram,

On Monday 03 March 2014 17:27:31 Wolfram Sang wrote:
> > While the parent is indeed selected at boot time only, and only one parent
> > is thus needed, parent selection could be performed by a DIP switch
> > connected to MD_CLK on the board for instance. In that case both parents
> > should be available in DT, as selection will be done by the kernel at
> > boot time, not at DT compile time.
> 
> OK, I understand the case. I still wonder about specifying two parents,
> though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
> clock (or an empty one), just because USB_X1 is enumerated as second
> entry?

That's a good question. Mike, would it be possible to support "holes" in the 
DT clocks lists, like the GPIO DT bindings do ?

> > > Again, now that I already coded it, what is the gain to remove it? The
> > > drawback is that other people might get encouraged to find reasons to
> > > allow them sloppy practices.
> > 
> > It will make the kernel binary smaller by removing code that is not needed
> > in practice.
> 
> Sounds like a micro-optimazation to me. If you insist, we should BUG()
> right away in that case, to make the code comprehensible. Returning with
> a leak and saying "we will probably fail somewhere" should really be
> avoided IMO.

It's a micro-optimization indeed, but a simple one, so I believe it makes 
sense. I'll leave it up to you in the end.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140304/e2ea5833/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> > > While the parent is indeed selected at boot time only, and only one parent
> > > is thus needed, parent selection could be performed by a DIP switch
> > > connected to MD_CLK on the board for instance. In that case both parents
> > > should be available in DT, as selection will be done by the kernel at
> > > boot time, not at DT compile time.
> > 
> > OK, I understand the case. I still wonder about specifying two parents,
> > though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
> > clock (or an empty one), just because USB_X1 is enumerated as second
> > entry?
> 
> That's a good question. Mike, would it be possible to support "holes" in the 
> DT clocks lists, like the GPIO DT bindings do ?

I am currently playing with the status property for clocks, so we can
"disable" clocks. Opinions?

From: Wolfram Sang <wsa@sang-engineering.com>
Subject: [PATCH] clk: allow clocks to use the 'status' property

Similar to platform devices, this allows us to set up the clock
hierarchy in dtsi files and enable the used clocks in the board file
later.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---
 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5517944..0045f55 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2542,6 +2542,9 @@ void __init of_clk_init(const struct of_device_id *matches)
 		matches = &__clk_of_table;
 
 	for_each_matching_node_and_match(np, matches, &match) {
+		if (!of_device_is_available(np))
+			continue;
+
 		of_clk_init_cb_t clk_init_cb = match->data;
 		clk_init_cb(np);
 	}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140304/8de443cc/attachment-0001.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Laurent Pinchart]

Hi Wolfram,

On Tuesday 04 March 2014 12:07:06 Wolfram Sang wrote:
> > > > While the parent is indeed selected at boot time only, and only one
> > > > parent is thus needed, parent selection could be performed by a DIP
> > > > switch connected to MD_CLK on the board for instance. In that case
> > > > both parents should be available in DT, as selection will be done by
> > > > the kernel at boot time, not at DT compile time.
> > > 
> > > OK, I understand the case. I still wonder about specifying two parents,
> > > though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
> > > clock (or an empty one), just because USB_X1 is enumerated as second
> > > entry?
> > 
> > That's a good question. Mike, would it be possible to support "holes" in
> > the DT clocks lists, like the GPIO DT bindings do ?
> 
> I am currently playing with the status property for clocks, so we can
> "disable" clocks. Opinions?

Could you please provide an example of how you would like to use that ?

> From: Wolfram Sang <wsa@sang-engineering.com>
> Subject: [PATCH] clk: allow clocks to use the 'status' property
> 
> Similar to platform devices, this allows us to set up the clock
> hierarchy in dtsi files and enable the used clocks in the board file
> later.
> 
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> ---
>  drivers/clk/clk.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5517944..0045f55 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2542,6 +2542,9 @@ void __init of_clk_init(const struct of_device_id
> *matches) matches = &__clk_of_table;
> 
>  	for_each_matching_node_and_match(np, matches, &match) {
> +		if (!of_device_is_available(np))
> +			continue;
> +
>  		of_clk_init_cb_t clk_init_cb = match->data;
>  		clk_init_cb(np);
>  	}

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140304/2e1916f0/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> > I am currently playing with the status property for clocks, so we can
> > "disable" clocks. Opinions?
> 
> Could you please provide an example of how you would like to use that ?

Working on it...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140304/9e3537e7/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> > > While the parent is indeed selected at boot time only, and only one parent
> > > is thus needed, parent selection could be performed by a DIP switch
> > > connected to MD_CLK on the board for instance. In that case both parents
> > > should be available in DT, as selection will be done by the kernel at
> > > boot time, not at DT compile time.
> > 
> > OK, I understand the case. I still wonder about specifying two parents,
> > though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
> > clock (or an empty one), just because USB_X1 is enumerated as second
> > entry?
> 
> That's a good question. Mike, would it be possible to support "holes" in the 
> DT clocks lists, like the GPIO DT bindings do ?

I talked to Magnus and we decided to start with hardcoded EXTAL for now,
and leave USB_X1 support for in incremental patch when this is actually
needed. Note that the devkit will most likely be the only board ever
with a selectable root clock.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140305/74e5a367/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Laurent Pinchart]

Hi Wolfram,

On Wednesday 05 March 2014 14:54:32 Wolfram Sang wrote:
> > > > While the parent is indeed selected at boot time only, and only one
> > > > parent is thus needed, parent selection could be performed by a DIP
> > > > switch connected to MD_CLK on the board for instance. In that case
> > > > both parents should be available in DT, as selection will be done by
> > > > the kernel at boot time, not at DT compile time.
> > > 
> > > OK, I understand the case. I still wonder about specifying two parents,
> > > though. If a board uses USB_X1, it then has to spefify a dummy EXTAL
> > > clock (or an empty one), just because USB_X1 is enumerated as second
> > > entry?
> > 
> > That's a good question. Mike, would it be possible to support "holes" in
> > the DT clocks lists, like the GPIO DT bindings do ?
> 
> I talked to Magnus and we decided to start with hardcoded EXTAL for now,
> and leave USB_X1 support for in incremental patch when this is actually
> needed.

That's fine with me (although given the low complexity I would probably have 
gone directly to supporting both clocks) as long as we can ensure both forward 
and backward compatibility. Should we give a name to the clock input through 
the clock-names property for that reason ?

> Note that the devkit will most likely be the only board ever with a
> selectable root clock.

Most probably, but that's no reason not to support it :-)

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140305/c3a5fc7e/attachment.sig>

[PATCH 3/4] clk: shmobile: add CPG driver for rz-platforms

[Wolfram Sang]

> That's fine with me (although given the low complexity I would probably have 
> gone directly to supporting both clocks) as long as we can ensure both forward 

We need to wait for GPIO support anyhow to get it real.

> and backward compatibility. Should we give a name to the clock input through 
> the clock-names property for that reason ?

Not needed IMO.

> > Note that the devkit will most likely be the only board ever with a
> > selectable root clock.
> 
> Most probably, but that's no reason not to support it :-)

Not in itself. Yet a low demand might be, a demand of 0 surely is ;)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140305/d7ef843e/attachment-0001.sig>

[PATCH 4/4] ARM: shmobile: r7s72100: use workaround for non DT-clocks

[Wolfram Sang]

From: Wolfram Sang <wsa@sang-engineering.com>

SCIF2 and MTU2 are not yet prepared for DT usage, so use the common
workaround via clkdev for now.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
---
 arch/arm/mach-shmobile/board-genmai-reference.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-genmai-reference.c b/arch/arm/mach-shmobile/board-genmai-reference.c
index 7630c10..91dcdd0 100644
--- a/arch/arm/mach-shmobile/board-genmai-reference.c
+++ b/arch/arm/mach-shmobile/board-genmai-reference.c
@@ -18,18 +18,29 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <linux/clk-provider.h>
 #include <linux/kernel.h>
 #include <linux/of_platform.h>
+#include <mach/clock.h>
 #include <mach/common.h>
 #include <mach/r7s72100.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 
+#ifdef CONFIG_COMMON_CLK
+/*
+ * This is a really crude hack to provide clkdev support to platform
+ * devices until they get moved to DT.
+ */
+static const struct clk_name clk_names[] = {
+	{ "mtu2", NULL, "sh_mtu2.0" },
+	{ "scif2", NULL, "sh-sci.2" },
+};
+#endif
+
 static void __init genmai_add_standard_devices(void)
 {
 #ifdef CONFIG_COMMON_CLK
-	of_clk_init(NULL);
+	shmobile_clk_workaround(clk_names, ARRAY_SIZE(clk_names), true);
 #else
 	r7s72100_clock_init();
 #endif
-- 
1.8.5.1

[PATCH 4/4] ARM: shmobile: r7s72100: use workaround for non DT-clocks

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 28 February 2014 22:09:28 Wolfram Sang wrote:
> From: Wolfram Sang <wsa@sang-engineering.com>
> 
> SCIF2 and MTU2 are not yet prepared for DT usage, so use the common
> workaround via clkdev for now.

What about adding a DT node for the serial port instead ? :-)

> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> ---
>  arch/arm/mach-shmobile/board-genmai-reference.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-genmai-reference.c
> b/arch/arm/mach-shmobile/board-genmai-reference.c index 7630c10..91dcdd0
> 100644
> --- a/arch/arm/mach-shmobile/board-genmai-reference.c
> +++ b/arch/arm/mach-shmobile/board-genmai-reference.c
> @@ -18,18 +18,29 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
> USA */
> 
> -#include <linux/clk-provider.h>
>  #include <linux/kernel.h>
>  #include <linux/of_platform.h>
> +#include <mach/clock.h>
>  #include <mach/common.h>
>  #include <mach/r7s72100.h>
>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
> 
> +#ifdef CONFIG_COMMON_CLK
> +/*
> + * This is a really crude hack to provide clkdev support to platform
> + * devices until they get moved to DT.
> + */
> +static const struct clk_name clk_names[] = {
> +	{ "mtu2", NULL, "sh_mtu2.0" },
> +	{ "scif2", NULL, "sh-sci.2" },
> +};
> +#endif
> +
>  static void __init genmai_add_standard_devices(void)
>  {
>  #ifdef CONFIG_COMMON_CLK
> -	of_clk_init(NULL);
> +	shmobile_clk_workaround(clk_names, ARRAY_SIZE(clk_names), true);
>  #else
>  	r7s72100_clock_init();
>  #endif

-- 
Regards,

Laurent Pinchart

[PATCH 4/4] ARM: shmobile: r7s72100: use workaround for non DT-clocks

[Wolfram Sang]

On Sun, Mar 02, 2014 at 11:21:40PM +0100, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> Thank you for the patch.
> 
> On Friday 28 February 2014 22:09:28 Wolfram Sang wrote:
> > From: Wolfram Sang <wsa@sang-engineering.com>
> > 
> > SCIF2 and MTU2 are not yet prepared for DT usage, so use the common
> > workaround via clkdev for now.
> 
> What about adding a DT node for the serial port instead ? :-)

Yes, done in V2 already. Yet, I'd like to keep SCIF in here because it
is the least intrusive way to get the system to boot. When the SCIF
nodes are added later incrementally, I remove the sh-sci.2 workaround,
but the whole mechanism is still needed for MTU2 anyhow.

Thanks,

   Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/2e375263/attachment.sig>

[PATCH 4/4] ARM: shmobile: r7s72100: use workaround for non DT-clocks

[Laurent Pinchart]

On Monday 03 March 2014 10:22:00 Wolfram Sang wrote:
> On Sun, Mar 02, 2014 at 11:21:40PM +0100, Laurent Pinchart wrote:
> > Hi Wolfram,
> > 
> > Thank you for the patch.
> > 
> > On Friday 28 February 2014 22:09:28 Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa@sang-engineering.com>
> > > 
> > > SCIF2 and MTU2 are not yet prepared for DT usage, so use the common
> > > workaround via clkdev for now.
> > 
> > What about adding a DT node for the serial port instead ? :-)
> 
> Yes, done in V2 already. Yet, I'd like to keep SCIF in here because it
> is the least intrusive way to get the system to boot. When the SCIF
> nodes are added later incrementally, I remove the sh-sci.2 workaround,
> but the whole mechanism is still needed for MTU2 anyhow.

True. I'm working on MTU2 DT support, but that will take a bit more time.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140303/69234a61/attachment.sig>