[PATCH v5 00/05] Renesas R-Car Gen3 CPG support V5

[PATCH v5 00/05] Renesas R-Car Gen3 CPG support V5

[Magnus Damm]

Renesas R-Car Gen3 CPG support V5

[PATCH v5 01/05] clk: shmobile: Rework CONFIG_ARCH_SHMOBILE_MULTI
[PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support
[PATCH v5 03/05] clk: shmobile: rcar-gen3: Add CPG/MSTP Clock Domain support
[PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings
[PATCH v5 05/05] clk: shmobile: Make MSTP clock-output-names optional

This series is my latest attempt to provide R-Car Gen3 clock support
through a single "easy to use" patch series for drivers/clk and DT
documentation bits.

There are no special runtime or build time dependencies. SoC / board
specific integration code (DTS) is provided separately from this series.

This version includes several minor updates for all the patches.
For details please see each individual patch.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Written to be picked up independently from other R-Car Gen3 patches.

 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt      |    1 
 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   58 ++
 drivers/clk/Makefile                                                     |    1 
 drivers/clk/shmobile/Makefile                                            |   23 
 drivers/clk/shmobile/clk-mstp.c                                          |   28 -
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |  247 ++++++++++
 6 files changed, 335 insertions(+), 23 deletions(-)

[PATCH v5 00/05] Renesas R-Car Gen3 CPG support V5

[Magnus Damm]

Renesas R-Car Gen3 CPG support V5

[PATCH v5 01/05] clk: shmobile: Rework CONFIG_ARCH_SHMOBILE_MULTI
[PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support
[PATCH v5 03/05] clk: shmobile: rcar-gen3: Add CPG/MSTP Clock Domain support
[PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings
[PATCH v5 05/05] clk: shmobile: Make MSTP clock-output-names optional

This series is my latest attempt to provide R-Car Gen3 clock support
through a single "easy to use" patch series for drivers/clk and DT
documentation bits.

There are no special runtime or build time dependencies. SoC / board
specific integration code (DTS) is provided separately from this series.

This version includes several minor updates for all the patches.
For details please see each individual patch.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Written to be picked up independently from other R-Car Gen3 patches.

 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt      |    1 
 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   58 ++
 drivers/clk/Makefile                                                     |    1 
 drivers/clk/shmobile/Makefile                                            |   23 
 drivers/clk/shmobile/clk-mstp.c                                          |   28 -
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |  247 ++++++++++
 6 files changed, 335 insertions(+), 23 deletions(-)

[PATCH v5 01/05] clk: shmobile: Rework CONFIG_ARCH_SHMOBILE_MULTI

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Shmobile is all multiplatform these days, so in get rid of the
reference to CONFIG_ARCH_SHMOBILE_MULTI in drivers/clk/shmobile/.

Also instead of always enabling DIV6 and MSTP adjust the Makefile
to enable DIV6 and MSTP depending on if they are included in the
SoC or not.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

 Earlier known as "clk: shmobile: Get rid of CONFIG_ARCH_SHMOBILE_MULTI"

 Changes since V4:
 - None

 Changes since V3:
 - Got rid of hunks that modified drivers/clk/Makefile
 - This to prevent build issue with non-CCF enabled SH arch
 - A nice side effect is that this patch now can be merged any time

 Changes since V2:
 - Fixed patch subject typo CONFIG_SHMOBILE_MULTI -> CONFIG_ARCH_SHMOBILE_MULTI

 drivers/clk/shmobile/Makefile |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

--- 0001/drivers/clk/shmobile/Makefile
+++ work/drivers/clk/shmobile/Makefile	2015-08-29 16:14:57.522366518 +0900
@@ -1,13 +1,11 @@
 obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
-obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o
-obj-$(CONFIG_ARCH_R8A73A4)		+= clk-r8a73a4.o
-obj-$(CONFIG_ARCH_R8A7740)		+= clk-r8a7740.o
-obj-$(CONFIG_ARCH_R8A7778)		+= clk-r8a7778.o
-obj-$(CONFIG_ARCH_R8A7779)		+= clk-r8a7779.o
-obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o
-obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
-obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
+obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A73A4)		+= clk-r8a73a4.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7740)		+= clk-r8a7740.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7778)		+= clk-r8a7778.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A7779)		+= clk-r8a7779.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o

[PATCH v5 01/05] clk: shmobile: Rework CONFIG_ARCH_SHMOBILE_MULTI

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Shmobile is all multiplatform these days, so in get rid of the
reference to CONFIG_ARCH_SHMOBILE_MULTI in drivers/clk/shmobile/.

Also instead of always enabling DIV6 and MSTP adjust the Makefile
to enable DIV6 and MSTP depending on if they are included in the
SoC or not.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

 Earlier known as "clk: shmobile: Get rid of CONFIG_ARCH_SHMOBILE_MULTI"

 Changes since V4:
 - None

 Changes since V3:
 - Got rid of hunks that modified drivers/clk/Makefile
 - This to prevent build issue with non-CCF enabled SH arch
 - A nice side effect is that this patch now can be merged any time

 Changes since V2:
 - Fixed patch subject typo CONFIG_SHMOBILE_MULTI -> CONFIG_ARCH_SHMOBILE_MULTI

 drivers/clk/shmobile/Makefile |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

--- 0001/drivers/clk/shmobile/Makefile
+++ work/drivers/clk/shmobile/Makefile	2015-08-29 16:14:57.522366518 +0900
@@ -1,13 +1,11 @@
 obj-$(CONFIG_ARCH_EMEV2)		+= clk-emev2.o
-obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o
-obj-$(CONFIG_ARCH_R8A73A4)		+= clk-r8a73a4.o
-obj-$(CONFIG_ARCH_R8A7740)		+= clk-r8a7740.o
-obj-$(CONFIG_ARCH_R8A7778)		+= clk-r8a7778.o
-obj-$(CONFIG_ARCH_R8A7779)		+= clk-r8a7779.o
-obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o
-obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o
-obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-div6.o
-obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= clk-mstp.o
+obj-$(CONFIG_ARCH_R7S72100)		+= clk-rz.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A73A4)		+= clk-r8a73a4.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7740)		+= clk-r8a7740.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7778)		+= clk-r8a7778.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A7779)		+= clk-r8a7779.o clk-mstp.o
+obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o

[PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

From: Gaku Inami <gaku.inami.xw@bp.renesas.com>

This V5 patch adds initial CPG support for R-Car Generation 3 and in
particular the R8A7795 SoC.

The R-Car Gen3 clock hardware has a register write protection feature that
needs to be shared between the CPG function needs to be shared between
the CPG and MSTP hardware somehow. So far this feature is simply ignored.

Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
 - Simplified clks array handling - thanks Geert!
 - Updated th DT binding documentation to reflect latest state
 - of_property_count_strings() -> of_property_count_u32_elems() fix

 Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
 - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
 - Major things like syscon and driver model require more discussion.
 - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.

 Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
 - Reworked driver to rely on clock index instead of strings.
 - Dropped use of "clock-output-names".

 Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>

 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   32 +
 drivers/clk/Makefile                                                     |    1 
 drivers/clk/shmobile/Makefile                                            |    1 
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |  245 ++++++++++
 4 files changed, 279 insertions(+)

--- /dev/null
+++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt	2015-08-31 15:49:10.000000000 +0900
@@ -0,0 +1,32 @@
+* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
+
+The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three PLLs
+and several fixed ratio dividers.
+
+Required Properties:
+
+  - compatible: Must be one of
+    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
+    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
+
+  - reg: Base address and length of the memory resource used by the CPG
+
+  - clocks: References to the parent clocks: first to the EXTAL clock
+  - #clock-cells: Must be 1
+  - clock-indices: Indices of the exported clocks
+
+Example
+-------
+
+	cpg_clocks: cpg_clocks@e6150000 {
+		compatible = "renesas,r8a7795-cpg-clocks",
+			     "renesas,rcar-gen3-cpg-clocks";
+		reg = <0 0xe6150000 0 0x1000>;
+		clocks = <&extal_clk>;
+		#clock-cells = <1>;
+                clock-indices = <
+                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
+                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
+                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
+                >;
+	};
--- 0001/drivers/clk/Makefile
+++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -67,6 +67,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
+obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
 obj-$(CONFIG_ARCH_SIRF)			+= sirf/
 obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
 obj-$(CONFIG_PLAT_SPEAR)		+= spear/
--- 0006/drivers/clk/shmobile/Makefile
+++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -8,4 +8,5 @@ obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
 obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
--- /dev/null
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 21:10:01.102366518 +0900
@@ -0,0 +1,245 @@
+/*
+ * rcar_gen3 Core CPG Clocks
+ *
+ * Copyright (C) 2015 Renesas Electronics Corp.
+ *
+ * Based on rcar_gen2 Core CPG Clocks driver.
+ *
+ * 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/clk/shmobile.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define RCAR_GEN3_CLK_MAIN	0
+#define RCAR_GEN3_CLK_PLL0	1
+#define RCAR_GEN3_CLK_PLL1	2
+#define RCAR_GEN3_CLK_PLL2	3
+#define RCAR_GEN3_CLK_PLL3	4
+#define RCAR_GEN3_CLK_PLL4	5
+#define RCAR_GEN3_CLK_NR	6
+
+static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
+	[RCAR_GEN3_CLK_MAIN] = "main",
+	[RCAR_GEN3_CLK_PLL0] = "pll0",
+	[RCAR_GEN3_CLK_PLL1] = "pll1",
+	[RCAR_GEN3_CLK_PLL2] = "pll2",
+	[RCAR_GEN3_CLK_PLL3] = "pll3",
+	[RCAR_GEN3_CLK_PLL4] = "pll4",
+};
+
+struct rcar_gen3_cpg {
+	struct clk_onecell_data data;
+	spinlock_t lock;
+	void __iomem *reg;
+	struct clk *clks[RCAR_GEN3_CLK_NR];
+};
+
+#define CPG_PLL0CR	0x00d8
+#define CPG_PLL2CR	0x002c
+
+/*
+ * common function
+ */
+#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
+
+/*
+ * Reset register definitions.
+ */
+#define MODEMR 0xe6160060
+
+static u32 rcar_gen3_read_mode_pins(void)
+{
+	static u32 mode;
+	static bool mode_valid;
+
+	if (!mode_valid) {
+		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
+
+		BUG_ON(!modemr);
+		mode = ioread32(modemr);
+		iounmap(modemr);
+		mode_valid = true;
+	}
+
+	return mode;
+}
+
+/* -----------------------------------------------------------------------------
+ * CPG Clock Data
+ */
+
+/*
+ *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
+ * 14 13 19 17	(MHz)		*1	*1	*1
+ *-------------------------------------------------------------------
+ * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
+ * 0  0  1  0	Prohibited setting
+ * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
+ * 0  1  1  0	Prohibited setting
+ * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
+ * 1  0  1  0	Prohibited setting
+ * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
+ * 1  1  1  0	Prohibited setting
+ * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ *
+ * *1 : datasheet indicates VCO output (PLLx = VCO/2)
+ *
+ */
+#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
+					 (((md) & BIT(13)) >> 11) | \
+					 (((md) & BIT(19)) >> 18) | \
+					 (((md) & BIT(17)) >> 17))
+struct cpg_pll_config {
+	unsigned int extal_div;
+	unsigned int pll1_mult;
+	unsigned int pll3_mult;
+	unsigned int pll4_mult;
+};
+
+static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
+/* EXTAL div	PLL1	PLL3	PLL4 */
+	{ 1,	192,	192,	144, },
+	{ 1,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	192,	192,	144, },
+	{ 1,	156,	156,	120, },
+	{ 1,	156,	106,	120, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	156,	156,	120, },
+	{ 1,	128,	128,	96,  },
+	{ 1,	128,	84,	96,  },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	128,	128,	96,  },
+	{ 2,	192,	192,	144, },
+	{ 2,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 2,	192,	192,	144, },
+};
+
+/* -----------------------------------------------------------------------------
+ * Initialization
+ */
+
+static struct clk * __init
+rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg *cpg,
+			     const struct cpg_pll_config *config,
+			     unsigned int gen3_clk)
+{
+	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
+	unsigned int mult = 1;
+	unsigned int div = 1;
+	u32 value;
+
+	switch (gen3_clk) {
+	case RCAR_GEN3_CLK_MAIN:
+		parent_name = of_clk_get_parent_name(np, 0);
+		div = config->extal_div;
+		break;
+	case RCAR_GEN3_CLK_PLL0:
+		/* PLL0 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL0CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL1:
+		mult = config->pll1_mult / 2;
+		break;
+	case RCAR_GEN3_CLK_PLL2:
+		/* PLL2 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL2CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL3:
+		mult = config->pll3_mult;
+		break;
+	case RCAR_GEN3_CLK_PLL4:
+		mult = config->pll4_mult;
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
+					 parent_name, 0, mult, div);
+}
+
+static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
+{
+	const struct cpg_pll_config *config;
+	struct rcar_gen3_cpg *cpg;
+	u32 cpg_mode;
+	unsigned int i;
+	int num_clks;
+
+	cpg_mode = rcar_gen3_read_mode_pins();
+
+	num_clks = of_property_count_u32_elems(np, "clock-indices");
+	if (num_clks < 0) {
+		pr_err("%s: failed to count clocks\n", __func__);
+		return;
+	}
+
+	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
+	if (!cpg)
+		return;
+
+	spin_lock_init(&cpg->lock);
+
+	cpg->data.clks = cpg->clks;
+	cpg->data.clk_num = num_clks;
+
+	cpg->reg = of_iomap(np, 0);
+	if (WARN_ON(cpg->reg = NULL))
+		return;
+
+	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
+	if (!config->extal_div) {
+		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
+		       __func__, cpg_mode);
+		return;
+	}
+
+	for (i = 0; i < num_clks; ++i) {
+		struct clk *clk;
+		u32 idx;
+		int ret;
+
+		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
+		if (ret < 0)
+			break;
+
+		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
+		if (IS_ERR(clk))
+			pr_err("%s: failed to register %s %u clock (%ld)\n",
+			       __func__, np->name, idx, PTR_ERR(clk));
+		else
+			cpg->data.clks[idx] = clk;
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+}
+CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
+	       rcar_gen3_cpg_clocks_init);

[PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

From: Gaku Inami <gaku.inami.xw@bp.renesas.com>

This V5 patch adds initial CPG support for R-Car Generation 3 and in
particular the R8A7795 SoC.

The R-Car Gen3 clock hardware has a register write protection feature that
needs to be shared between the CPG function needs to be shared between
the CPG and MSTP hardware somehow. So far this feature is simply ignored.

Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
 - Simplified clks array handling - thanks Geert!
 - Updated th DT binding documentation to reflect latest state
 - of_property_count_strings() -> of_property_count_u32_elems() fix

 Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
 - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
 - Major things like syscon and driver model require more discussion.
 - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.

 Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
 - Reworked driver to rely on clock index instead of strings.
 - Dropped use of "clock-output-names".

 Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>

 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   32 +
 drivers/clk/Makefile                                                     |    1 
 drivers/clk/shmobile/Makefile                                            |    1 
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |  245 ++++++++++
 4 files changed, 279 insertions(+)

--- /dev/null
+++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt	2015-08-31 15:49:10.000000000 +0900
@@ -0,0 +1,32 @@
+* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
+
+The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three PLLs
+and several fixed ratio dividers.
+
+Required Properties:
+
+  - compatible: Must be one of
+    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
+    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
+
+  - reg: Base address and length of the memory resource used by the CPG
+
+  - clocks: References to the parent clocks: first to the EXTAL clock
+  - #clock-cells: Must be 1
+  - clock-indices: Indices of the exported clocks
+
+Example
+-------
+
+	cpg_clocks: cpg_clocks@e6150000 {
+		compatible = "renesas,r8a7795-cpg-clocks",
+			     "renesas,rcar-gen3-cpg-clocks";
+		reg = <0 0xe6150000 0 0x1000>;
+		clocks = <&extal_clk>;
+		#clock-cells = <1>;
+                clock-indices = <
+                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
+                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
+                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
+                >;
+	};
--- 0001/drivers/clk/Makefile
+++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -67,6 +67,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
 obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
+obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
 obj-$(CONFIG_ARCH_SIRF)			+= sirf/
 obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
 obj-$(CONFIG_PLAT_SPEAR)		+= spear/
--- 0006/drivers/clk/shmobile/Makefile
+++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
@@ -8,4 +8,5 @@ obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
 obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
+obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
 obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
--- /dev/null
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 21:10:01.102366518 +0900
@@ -0,0 +1,245 @@
+/*
+ * rcar_gen3 Core CPG Clocks
+ *
+ * Copyright (C) 2015 Renesas Electronics Corp.
+ *
+ * Based on rcar_gen2 Core CPG Clocks driver.
+ *
+ * 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/clk/shmobile.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define RCAR_GEN3_CLK_MAIN	0
+#define RCAR_GEN3_CLK_PLL0	1
+#define RCAR_GEN3_CLK_PLL1	2
+#define RCAR_GEN3_CLK_PLL2	3
+#define RCAR_GEN3_CLK_PLL3	4
+#define RCAR_GEN3_CLK_PLL4	5
+#define RCAR_GEN3_CLK_NR	6
+
+static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
+	[RCAR_GEN3_CLK_MAIN] = "main",
+	[RCAR_GEN3_CLK_PLL0] = "pll0",
+	[RCAR_GEN3_CLK_PLL1] = "pll1",
+	[RCAR_GEN3_CLK_PLL2] = "pll2",
+	[RCAR_GEN3_CLK_PLL3] = "pll3",
+	[RCAR_GEN3_CLK_PLL4] = "pll4",
+};
+
+struct rcar_gen3_cpg {
+	struct clk_onecell_data data;
+	spinlock_t lock;
+	void __iomem *reg;
+	struct clk *clks[RCAR_GEN3_CLK_NR];
+};
+
+#define CPG_PLL0CR	0x00d8
+#define CPG_PLL2CR	0x002c
+
+/*
+ * common function
+ */
+#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
+
+/*
+ * Reset register definitions.
+ */
+#define MODEMR 0xe6160060
+
+static u32 rcar_gen3_read_mode_pins(void)
+{
+	static u32 mode;
+	static bool mode_valid;
+
+	if (!mode_valid) {
+		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
+
+		BUG_ON(!modemr);
+		mode = ioread32(modemr);
+		iounmap(modemr);
+		mode_valid = true;
+	}
+
+	return mode;
+}
+
+/* -----------------------------------------------------------------------------
+ * CPG Clock Data
+ */
+
+/*
+ *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
+ * 14 13 19 17	(MHz)		*1	*1	*1
+ *-------------------------------------------------------------------
+ * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
+ * 0  0  1  0	Prohibited setting
+ * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
+ * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
+ * 0  1  1  0	Prohibited setting
+ * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
+ * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
+ * 1  0  1  0	Prohibited setting
+ * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
+ * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
+ * 1  1  1  0	Prohibited setting
+ * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
+ *
+ * *1 : datasheet indicates VCO output (PLLx = VCO/2)
+ *
+ */
+#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
+					 (((md) & BIT(13)) >> 11) | \
+					 (((md) & BIT(19)) >> 18) | \
+					 (((md) & BIT(17)) >> 17))
+struct cpg_pll_config {
+	unsigned int extal_div;
+	unsigned int pll1_mult;
+	unsigned int pll3_mult;
+	unsigned int pll4_mult;
+};
+
+static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
+/* EXTAL div	PLL1	PLL3	PLL4 */
+	{ 1,	192,	192,	144, },
+	{ 1,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	192,	192,	144, },
+	{ 1,	156,	156,	120, },
+	{ 1,	156,	106,	120, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	156,	156,	120, },
+	{ 1,	128,	128,	96,  },
+	{ 1,	128,	84,	96,  },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 1,	128,	128,	96,  },
+	{ 2,	192,	192,	144, },
+	{ 2,	192,	128,	144, },
+	{ 0,	0,	0,	0,   }, /* Prohibited setting */
+	{ 2,	192,	192,	144, },
+};
+
+/* -----------------------------------------------------------------------------
+ * Initialization
+ */
+
+static struct clk * __init
+rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg *cpg,
+			     const struct cpg_pll_config *config,
+			     unsigned int gen3_clk)
+{
+	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
+	unsigned int mult = 1;
+	unsigned int div = 1;
+	u32 value;
+
+	switch (gen3_clk) {
+	case RCAR_GEN3_CLK_MAIN:
+		parent_name = of_clk_get_parent_name(np, 0);
+		div = config->extal_div;
+		break;
+	case RCAR_GEN3_CLK_PLL0:
+		/* PLL0 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL0CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL1:
+		mult = config->pll1_mult / 2;
+		break;
+	case RCAR_GEN3_CLK_PLL2:
+		/* PLL2 is a configurable multiplier clock. Register it as a
+		 * fixed factor clock for now as there's no generic multiplier
+		 * clock implementation and we currently have no need to change
+		 * the multiplier value.
+		 */
+		value = rcar_clk_readl(cpg, CPG_PLL2CR);
+		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
+		break;
+	case RCAR_GEN3_CLK_PLL3:
+		mult = config->pll3_mult;
+		break;
+	case RCAR_GEN3_CLK_PLL4:
+		mult = config->pll4_mult;
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
+					 parent_name, 0, mult, div);
+}
+
+static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
+{
+	const struct cpg_pll_config *config;
+	struct rcar_gen3_cpg *cpg;
+	u32 cpg_mode;
+	unsigned int i;
+	int num_clks;
+
+	cpg_mode = rcar_gen3_read_mode_pins();
+
+	num_clks = of_property_count_u32_elems(np, "clock-indices");
+	if (num_clks < 0) {
+		pr_err("%s: failed to count clocks\n", __func__);
+		return;
+	}
+
+	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
+	if (!cpg)
+		return;
+
+	spin_lock_init(&cpg->lock);
+
+	cpg->data.clks = cpg->clks;
+	cpg->data.clk_num = num_clks;
+
+	cpg->reg = of_iomap(np, 0);
+	if (WARN_ON(cpg->reg == NULL))
+		return;
+
+	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
+	if (!config->extal_div) {
+		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
+		       __func__, cpg_mode);
+		return;
+	}
+
+	for (i = 0; i < num_clks; ++i) {
+		struct clk *clk;
+		u32 idx;
+		int ret;
+
+		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
+		if (ret < 0)
+			break;
+
+		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
+		if (IS_ERR(clk))
+			pr_err("%s: failed to register %s %u clock (%ld)\n",
+			       __func__, np->name, idx, PTR_ERR(clk));
+		else
+			cpg->data.clks[idx] = clk;
+	}
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+}
+CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
+	       rcar_gen3_cpg_clocks_init);

[PATCH v5 03/05] clk: shmobile: rcar-gen3: Add CPG/MSTP Clock Domain support

[Magnus Damm]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Add Clock Domain support to the R-Car Gen3 Clock Pulse Generator (CPG)
driver using the generic PM Domain.  This allows to power-manage the
module clocks of SoC devices that are part of the CPG/MSTP Clock Domain
using Runtime PM, or for system suspend/resume.

SoC devices that are part of the CPG/MSTP Clock Domain and can be
power-managed through an MSTP clock should be tagged in DT with a proper
"power-domains" property.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4:
 - Rebased to fit on top of updated CPG DT binding document

 Changes since V3:
 - Fixed so correct file is actually used. =)
 - Took V2 from Geert but filtered out Kconfig.platforms bits

 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   26 +++++++++-
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |    2 
 2 files changed, 26 insertions(+), 2 deletions(-)

--- 0007/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
+++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt	2015-08-31 21:20:31.782366518 +0900
@@ -2,6 +2,8 @@
 
 The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three PLLs
 and several fixed ratio dividers.
+The CPG also provides a Clock Domain for SoC devices, in combination with the
+CPG Module Stop (MSTP) Clocks.
 
 Required Properties:
 
@@ -14,9 +16,17 @@ Required Properties:
   - clocks: References to the parent clocks: first to the EXTAL clock
   - #clock-cells: Must be 1
   - clock-indices: Indices of the exported clocks
+  - #power-domain-cells: Must be 0
 
-Example
--------
+SoC devices that are part of the CPG/MSTP Clock Domain and can be power-managed
+through an MSTP clock should refer to the CPG device node in their
+"power-domains" property, as documented by the generic PM domain bindings in
+Documentation/devicetree/bindings/power/power_domain.txt.
+
+Examples
+--------
+
+  - CPG device node:
 
 	cpg_clocks: cpg_clocks@e6150000 {
 		compatible = "renesas,r8a7795-cpg-clocks",
@@ -29,4 +39,16 @@ Example
                              R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
                              R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
                 >;
+		#power-domain-cells = <0>;
+	};
+
+  - CPG/MSTP Clock Domain member device node:
+
+	scif2: serial@e6e88000 {
+		compatible = "renesas,scif-r8a7795", "renesas,scif";
+		reg = <0 0xe6e88000 0 64>;
+		interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp3_clks RCAR_R8A7795_CLK_SCIF2>;
+		clock-names = "sci_ick";
+		power-domains = <&cpg_clocks>;
 	};
--- 0007/drivers/clk/shmobile/clk-rcar-gen3.c
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 21:17:02.242366518 +0900
@@ -240,6 +240,8 @@ static void __init rcar_gen3_cpg_clocks_
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+
+	cpg_mstp_add_clk_domain(np);
 }
 CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
 	       rcar_gen3_cpg_clocks_init);

[PATCH v5 03/05] clk: shmobile: rcar-gen3: Add CPG/MSTP Clock Domain support

[Magnus Damm]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Add Clock Domain support to the R-Car Gen3 Clock Pulse Generator (CPG)
driver using the generic PM Domain.  This allows to power-manage the
module clocks of SoC devices that are part of the CPG/MSTP Clock Domain
using Runtime PM, or for system suspend/resume.

SoC devices that are part of the CPG/MSTP Clock Domain and can be
power-managed through an MSTP clock should be tagged in DT with a proper
"power-domains" property.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4:
 - Rebased to fit on top of updated CPG DT binding document

 Changes since V3:
 - Fixed so correct file is actually used. =)
 - Took V2 from Geert but filtered out Kconfig.platforms bits

 Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |   26 +++++++++-
 drivers/clk/shmobile/clk-rcar-gen3.c                                     |    2 
 2 files changed, 26 insertions(+), 2 deletions(-)

--- 0007/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
+++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt	2015-08-31 21:20:31.782366518 +0900
@@ -2,6 +2,8 @@
 
 The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three PLLs
 and several fixed ratio dividers.
+The CPG also provides a Clock Domain for SoC devices, in combination with the
+CPG Module Stop (MSTP) Clocks.
 
 Required Properties:
 
@@ -14,9 +16,17 @@ Required Properties:
   - clocks: References to the parent clocks: first to the EXTAL clock
   - #clock-cells: Must be 1
   - clock-indices: Indices of the exported clocks
+  - #power-domain-cells: Must be 0
 
-Example
--------
+SoC devices that are part of the CPG/MSTP Clock Domain and can be power-managed
+through an MSTP clock should refer to the CPG device node in their
+"power-domains" property, as documented by the generic PM domain bindings in
+Documentation/devicetree/bindings/power/power_domain.txt.
+
+Examples
+--------
+
+  - CPG device node:
 
 	cpg_clocks: cpg_clocks@e6150000 {
 		compatible = "renesas,r8a7795-cpg-clocks",
@@ -29,4 +39,16 @@ Example
                              R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
                              R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
                 >;
+		#power-domain-cells = <0>;
+	};
+
+  - CPG/MSTP Clock Domain member device node:
+
+	scif2: serial@e6e88000 {
+		compatible = "renesas,scif-r8a7795", "renesas,scif";
+		reg = <0 0xe6e88000 0 64>;
+		interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp3_clks RCAR_R8A7795_CLK_SCIF2>;
+		clock-names = "sci_ick";
+		power-domains = <&cpg_clocks>;
 	};
--- 0007/drivers/clk/shmobile/clk-rcar-gen3.c
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 21:17:02.242366518 +0900
@@ -240,6 +240,8 @@ static void __init rcar_gen3_cpg_clocks_
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
+
+	cpg_mstp_add_clk_domain(np);
 }
 CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
 	       rcar_gen3_cpg_clocks_init);

[PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings

[Magnus Damm]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Add r8a7795 to the MSTP DT binding documentation.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
 - Added Acked-by from Geert

 Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
 - Picked up patch floating around on ML, added change log

 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt |    1 +
 1 file changed, 1 insertion(+)

--- 0001/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
+++ work/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt	2015-08-29 16:18:30.672366518 +0900
@@ -19,6 +19,7 @@ Required Properties:
     - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2-W) MSTP gate clocks
     - "renesas,r8a7793-mstp-clocks" for R8A7793 (R-Car M2-N) MSTP gate clocks
     - "renesas,r8a7794-mstp-clocks" for R8A7794 (R-Car E2) MSTP gate clocks
+    - "renesas,r8a7795-mstp-clocks" for R8A7795 (R-Car H3) MSTP gate clocks
     - "renesas,sh73a0-mstp-clocks" for SH73A0 (SH-MobileAG5) MSTP gate clocks
     and "renesas,cpg-mstp-clocks" as a fallback.
   - reg: Base address and length of the I/O mapped registers used by the MSTP

[PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings

[Magnus Damm]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Add r8a7795 to the MSTP DT binding documentation.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
 - Added Acked-by from Geert

 Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
 - Picked up patch floating around on ML, added change log

 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt |    1 +
 1 file changed, 1 insertion(+)

--- 0001/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
+++ work/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt	2015-08-29 16:18:30.672366518 +0900
@@ -19,6 +19,7 @@ Required Properties:
     - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2-W) MSTP gate clocks
     - "renesas,r8a7793-mstp-clocks" for R8A7793 (R-Car M2-N) MSTP gate clocks
     - "renesas,r8a7794-mstp-clocks" for R8A7794 (R-Car E2) MSTP gate clocks
+    - "renesas,r8a7795-mstp-clocks" for R8A7795 (R-Car H3) MSTP gate clocks
     - "renesas,sh73a0-mstp-clocks" for SH73A0 (SH-MobileAG5) MSTP gate clocks
     and "renesas,cpg-mstp-clocks" as a fallback.
   - reg: Base address and length of the I/O mapped registers used by the MSTP

[PATCH v5 05/05] clk: shmobile: Make MSTP clock-output-names optional

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

This patch updates the MSTP driver to make the "clock-output-names"
DT property optional instead of mandatory. In case the DT property
is omitted then the function clk_register_clkdev() is skipped.

The default for new SoCs is to not use "clock-output-names".

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4:
 - None

 Changes since V3:
 - Added support for multiple MSTP bits. =)

 drivers/clk/shmobile/clk-mstp.c |   28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

--- 0001/drivers/clk/shmobile/clk-mstp.c
+++ work/drivers/clk/shmobile/clk-mstp.c	2015-08-29 18:01:00.662366518 +0900
@@ -199,26 +199,29 @@ static void __init cpg_mstp_clocks_init(
 	for (i = 0; i < MSTP_MAX_CLOCKS; ++i) {
 		const char *parent_name;
 		const char *name;
+		bool allocated_name;
 		u32 clkidx;
 		int ret;
 
-		/* Skip clocks with no name. */
-		ret = of_property_read_string_index(np, "clock-output-names",
-						    i, &name);
-		if (ret < 0 || strlen(name) = 0)
-			continue;
-
 		parent_name = of_clk_get_parent_name(np, i);
 		ret = of_property_read_u32_index(np, idxname, i, &clkidx);
 		if (parent_name = NULL || ret < 0)
 			break;
 
 		if (clkidx >= MSTP_MAX_CLOCKS) {
-			pr_err("%s: invalid clock %s %s index %u)\n",
-			       __func__, np->name, name, clkidx);
+			pr_err("%s: invalid clock %s index %u)\n",
+			       __func__, np->name, clkidx);
 			continue;
 		}
 
+		if (of_property_read_string_index(np, "clock-output-names",
+						  i, &name) = 0) {
+			allocated_name = false;
+		} else {
+			name = kasprintf(GFP_KERNEL, "%s.%u", np->name, clkidx);
+			allocated_name = true;
+		}
+
 		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
 						       clkidx, group);
 		if (!IS_ERR(clks[clkidx])) {
@@ -231,12 +234,19 @@ static void __init cpg_mstp_clocks_init(
 			 *
 			 * FIXME: Remove this when all devices that require a
 			 * clock will be instantiated from DT.
+			 *
+			 * We currently register clkdev only when the DT
+			 * property clock-output-names is present.
 			 */
-			clk_register_clkdev(clks[clkidx], name, NULL);
+			if (!allocated_name)
+				clk_register_clkdev(clks[clkidx], name, NULL);
 		} else {
 			pr_err("%s: failed to register %s %s clock (%ld)\n",
 			       __func__, np->name, name, PTR_ERR(clks[clkidx]));
 		}
+
+		if (allocated_name)
+			kfree(name);
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &group->data);

[PATCH v5 05/05] clk: shmobile: Make MSTP clock-output-names optional

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

This patch updates the MSTP driver to make the "clock-output-names"
DT property optional instead of mandatory. In case the DT property
is omitted then the function clk_register_clkdev() is skipped.

The default for new SoCs is to not use "clock-output-names".

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V4:
 - None

 Changes since V3:
 - Added support for multiple MSTP bits. =)

 drivers/clk/shmobile/clk-mstp.c |   28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

--- 0001/drivers/clk/shmobile/clk-mstp.c
+++ work/drivers/clk/shmobile/clk-mstp.c	2015-08-29 18:01:00.662366518 +0900
@@ -199,26 +199,29 @@ static void __init cpg_mstp_clocks_init(
 	for (i = 0; i < MSTP_MAX_CLOCKS; ++i) {
 		const char *parent_name;
 		const char *name;
+		bool allocated_name;
 		u32 clkidx;
 		int ret;
 
-		/* Skip clocks with no name. */
-		ret = of_property_read_string_index(np, "clock-output-names",
-						    i, &name);
-		if (ret < 0 || strlen(name) == 0)
-			continue;
-
 		parent_name = of_clk_get_parent_name(np, i);
 		ret = of_property_read_u32_index(np, idxname, i, &clkidx);
 		if (parent_name == NULL || ret < 0)
 			break;
 
 		if (clkidx >= MSTP_MAX_CLOCKS) {
-			pr_err("%s: invalid clock %s %s index %u)\n",
-			       __func__, np->name, name, clkidx);
+			pr_err("%s: invalid clock %s index %u)\n",
+			       __func__, np->name, clkidx);
 			continue;
 		}
 
+		if (of_property_read_string_index(np, "clock-output-names",
+						  i, &name) == 0) {
+			allocated_name = false;
+		} else {
+			name = kasprintf(GFP_KERNEL, "%s.%u", np->name, clkidx);
+			allocated_name = true;
+		}
+
 		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
 						       clkidx, group);
 		if (!IS_ERR(clks[clkidx])) {
@@ -231,12 +234,19 @@ static void __init cpg_mstp_clocks_init(
 			 *
 			 * FIXME: Remove this when all devices that require a
 			 * clock will be instantiated from DT.
+			 *
+			 * We currently register clkdev only when the DT
+			 * property clock-output-names is present.
 			 */
-			clk_register_clkdev(clks[clkidx], name, NULL);
+			if (!allocated_name)
+				clk_register_clkdev(clks[clkidx], name, NULL);
 		} else {
 			pr_err("%s: failed to register %s %s clock (%ld)\n",
 			       __func__, np->name, name, PTR_ERR(clks[clkidx]));
 		}
+
+		if (allocated_name)
+			kfree(name);
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &group->data);

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> 
> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> particular the R8A7795 SoC.
> 
> The R-Car Gen3 clock hardware has a register write protection feature that
> needs to be shared between the CPG function needs to be shared between
> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
> 
> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>  - Simplified clks array handling - thanks Geert!
>  - Updated th DT binding documentation to reflect latest state
>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> 
>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>  - Major things like syscon and driver model require more discussion.
>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> 
>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>  - Reworked driver to rely on clock index instead of strings.
>  - Dropped use of "clock-output-names".
> 
>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> 
>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt | 
>  32 +
>  drivers/clk/Makefile                                     |    1
>  drivers/clk/shmobile/Makefile                            |    1
>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>  4 files changed, 279 insertions(+)
> 
> --- /dev/null
> +++
> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
> xt	2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> PLLs
> +and several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be one of
> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> +
> +  - reg: Base address and length of the memory resource used by the CPG
> +
> +  - clocks: References to the parent clocks: first to the EXTAL clock
> +  - #clock-cells: Must be 1
> +  - clock-indices: Indices of the exported clocks

Do we actually need the clock-indices property, as CPG clocks are numbered 
sequentially ? It seems to me like we could drop the property, especially 
given that the number of clocks is hardcoded in the driver anyway.

> +
> +Example
> +-------
> +
> +	cpg_clocks: cpg_clocks@e6150000 {
> +		compatible = "renesas,r8a7795-cpg-clocks",
> +			     "renesas,rcar-gen3-cpg-clocks";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>;
> +		#clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;
> +	};
> --- 0001/drivers/clk/Makefile
> +++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -67,6 +67,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
> +obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
>  obj-$(CONFIG_ARCH_SIRF)			+= sirf/
>  obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
>  obj-$(CONFIG_PLAT_SPEAR)		+= spear/
> --- 0006/drivers/clk/shmobile/Makefile
> +++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -8,4 +8,5 @@ obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
>  obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
> +obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
> --- /dev/null
> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 
21:10:01.102366518
> +0900 @@ -0,0 +1,245 @@
> +/*
> + * rcar_gen3 Core CPG Clocks
> + *
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * Based on rcar_gen2 Core CPG Clocks driver.
> + *
> + * 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/clk/shmobile.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define RCAR_GEN3_CLK_MAIN	0
> +#define RCAR_GEN3_CLK_PLL0	1
> +#define RCAR_GEN3_CLK_PLL1	2
> +#define RCAR_GEN3_CLK_PLL2	3
> +#define RCAR_GEN3_CLK_PLL3	4
> +#define RCAR_GEN3_CLK_PLL4	5
> +#define RCAR_GEN3_CLK_NR	6
> +
> +static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
> +	[RCAR_GEN3_CLK_MAIN] = "main",
> +	[RCAR_GEN3_CLK_PLL0] = "pll0",
> +	[RCAR_GEN3_CLK_PLL1] = "pll1",
> +	[RCAR_GEN3_CLK_PLL2] = "pll2",
> +	[RCAR_GEN3_CLK_PLL3] = "pll3",
> +	[RCAR_GEN3_CLK_PLL4] = "pll4",
> +};
> +
> +struct rcar_gen3_cpg {
> +	struct clk_onecell_data data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +	struct clk *clks[RCAR_GEN3_CLK_NR];
> +};
> +
> +#define CPG_PLL0CR	0x00d8
> +#define CPG_PLL2CR	0x002c
> +
> +/*
> + * common function
> + */
> +#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
> +
> +/*
> + * Reset register definitions.
> + */
> +#define MODEMR 0xe6160060
> +
> +static u32 rcar_gen3_read_mode_pins(void)
> +{
> +	static u32 mode;
> +	static bool mode_valid;
> +
> +	if (!mode_valid) {
> +		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
> +
> +		BUG_ON(!modemr);
> +		mode = ioread32(modemr);
> +		iounmap(modemr);
> +		mode_valid = true;
> +	}
> +
> +	return mode;
> +}
> +
> +/* ------------------------------------------------------------------------
> + * CPG Clock Data
> + */
> +
> +/*
> + *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
> + * 14 13 19 17	(MHz)		*1	*1	*1
> + *-------------------------------------------------------------------
> + * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
> + * 0  0  1  0	Prohibited setting
> + * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
> + * 0  1  1  0	Prohibited setting
> + * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
> + * 1  0  1  0	Prohibited setting
> + * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
> + * 1  1  1  0	Prohibited setting
> + * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + *
> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)

As explained in a separate e-mail there's a few clocks on R8A7795 that derive 
directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1 
clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

> + */
> +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
> +					 (((md) & BIT(13)) >> 11) | \
> +					 (((md) & BIT(19)) >> 18) | \
> +					 (((md) & BIT(17)) >> 17))
> +struct cpg_pll_config {
> +	unsigned int extal_div;
> +	unsigned int pll1_mult;
> +	unsigned int pll3_mult;
> +	unsigned int pll4_mult;
> +};
> +
> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
> +/* EXTAL div	PLL1	PLL3	PLL4 */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	156,	156,	120, },
> +	{ 1,	156,	106,	120, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	156,	156,	120, },
> +	{ 1,	128,	128,	96,  },
> +	{ 1,	128,	84,	96,  },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	128,	128,	96,  },
> +	{ 2,	192,	192,	144, },
> +	{ 2,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 2,	192,	192,	144, },
> +};
> +
> +/* ------------------------------------------------------------------------
> + * Initialization
> + */
> +
> +static struct clk * __init
> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
> *cpg,
> +			     const struct cpg_pll_config *config,
> +			     unsigned int gen3_clk)
> +{
> +	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
> +	unsigned int mult = 1;
> +	unsigned int div = 1;
> +	u32 value;
> +
> +	switch (gen3_clk) {
> +	case RCAR_GEN3_CLK_MAIN:
> +		parent_name = of_clk_get_parent_name(np, 0);
> +		div = config->extal_div;
> +		break;
> +	case RCAR_GEN3_CLK_PLL0:
> +		/* PLL0 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL0CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;

I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same 
comment for PLL2.

> +		break;
> +	case RCAR_GEN3_CLK_PLL1:
> +		mult = config->pll1_mult / 2;
> +		break;
> +	case RCAR_GEN3_CLK_PLL2:
> +		/* PLL2 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL2CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
> +		break;
> +	case RCAR_GEN3_CLK_PLL3:
> +		mult = config->pll3_mult;
> +		break;
> +	case RCAR_GEN3_CLK_PLL4:
> +		mult = config->pll4_mult;
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
> +					 parent_name, 0, mult, div);
> +}
> +
> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
> +{
> +	const struct cpg_pll_config *config;
> +	struct rcar_gen3_cpg *cpg;
> +	u32 cpg_mode;
> +	unsigned int i;
> +	int num_clks;
> +
> +	cpg_mode = rcar_gen3_read_mode_pins();
> +
> +	num_clks = of_property_count_u32_elems(np, "clock-indices");
> +	if (num_clks < 0) {
> +		pr_err("%s: failed to count clocks\n", __func__);
> +		return;
> +	}
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	if (!cpg)
> +		return;
> +
> +	spin_lock_init(&cpg->lock);

The spin lock is never used. I'd drop it for now, and add it back later 
when/if needed.

> +	cpg->data.clks = cpg->clks;
> +	cpg->data.clk_num = num_clks;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg = NULL))
> +		return;
> +
> +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +	if (!config->extal_div) {
> +		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
> +		       __func__, cpg_mode);
> +		return;
> +	}
> +
> +	for (i = 0; i < num_clks; ++i) {
> +		struct clk *clk;
> +		u32 idx;
> +		int ret;
> +
> +		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
> +		if (ret < 0)
> +			break;
> +
> +		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %u clock (%ld)\n",
> +			       __func__, np->name, idx, PTR_ERR(clk));
> +		else
> +			cpg->data.clks[idx] = clk;
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
> +	       rcar_gen3_cpg_clocks_init);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> 
> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> particular the R8A7795 SoC.
> 
> The R-Car Gen3 clock hardware has a register write protection feature that
> needs to be shared between the CPG function needs to be shared between
> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
> 
> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>  - Simplified clks array handling - thanks Geert!
>  - Updated th DT binding documentation to reflect latest state
>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> 
>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>  - Major things like syscon and driver model require more discussion.
>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> 
>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>  - Reworked driver to rely on clock index instead of strings.
>  - Dropped use of "clock-output-names".
> 
>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> 
>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt | 
>  32 +
>  drivers/clk/Makefile                                     |    1
>  drivers/clk/shmobile/Makefile                            |    1
>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>  4 files changed, 279 insertions(+)
> 
> --- /dev/null
> +++
> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
> xt	2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> +
> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> PLLs
> +and several fixed ratio dividers.
> +
> +Required Properties:
> +
> +  - compatible: Must be one of
> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> +
> +  - reg: Base address and length of the memory resource used by the CPG
> +
> +  - clocks: References to the parent clocks: first to the EXTAL clock
> +  - #clock-cells: Must be 1
> +  - clock-indices: Indices of the exported clocks

Do we actually need the clock-indices property, as CPG clocks are numbered 
sequentially ? It seems to me like we could drop the property, especially 
given that the number of clocks is hardcoded in the driver anyway.

> +
> +Example
> +-------
> +
> +	cpg_clocks: cpg_clocks@e6150000 {
> +		compatible = "renesas,r8a7795-cpg-clocks",
> +			     "renesas,rcar-gen3-cpg-clocks";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>;
> +		#clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;
> +	};
> --- 0001/drivers/clk/Makefile
> +++ work/drivers/clk/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -67,6 +67,7 @@ obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
>  obj-$(CONFIG_ARCH_SHMOBILE_MULTI)	+= shmobile/
> +obj-$(CONFIG_ARCH_RENESAS)		+= shmobile/
>  obj-$(CONFIG_ARCH_SIRF)			+= sirf/
>  obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
>  obj-$(CONFIG_PLAT_SPEAR)		+= spear/
> --- 0006/drivers/clk/shmobile/Makefile
> +++ work/drivers/clk/shmobile/Makefile	2015-08-31 15:49:09.072366518 +0900
> @@ -8,4 +8,5 @@ obj-$(CONFIG_ARCH_R8A7790)		+= clk-rcar-
>  obj-$(CONFIG_ARCH_R8A7791)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7793)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_R8A7794)		+= clk-rcar-gen2.o clk-mstp.o clk-div6.o
> +obj-$(CONFIG_ARCH_R8A7795)		+= clk-rcar-gen3.o clk-mstp.o clk-div6.o
>  obj-$(CONFIG_ARCH_SH73A0)		+= clk-sh73a0.o clk-mstp.o clk-div6.o
> --- /dev/null
> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-08-31 
21:10:01.102366518
> +0900 @@ -0,0 +1,245 @@
> +/*
> + * rcar_gen3 Core CPG Clocks
> + *
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * Based on rcar_gen2 Core CPG Clocks driver.
> + *
> + * 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/clk/shmobile.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define RCAR_GEN3_CLK_MAIN	0
> +#define RCAR_GEN3_CLK_PLL0	1
> +#define RCAR_GEN3_CLK_PLL1	2
> +#define RCAR_GEN3_CLK_PLL2	3
> +#define RCAR_GEN3_CLK_PLL3	4
> +#define RCAR_GEN3_CLK_PLL4	5
> +#define RCAR_GEN3_CLK_NR	6
> +
> +static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
> +	[RCAR_GEN3_CLK_MAIN] = "main",
> +	[RCAR_GEN3_CLK_PLL0] = "pll0",
> +	[RCAR_GEN3_CLK_PLL1] = "pll1",
> +	[RCAR_GEN3_CLK_PLL2] = "pll2",
> +	[RCAR_GEN3_CLK_PLL3] = "pll3",
> +	[RCAR_GEN3_CLK_PLL4] = "pll4",
> +};
> +
> +struct rcar_gen3_cpg {
> +	struct clk_onecell_data data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +	struct clk *clks[RCAR_GEN3_CLK_NR];
> +};
> +
> +#define CPG_PLL0CR	0x00d8
> +#define CPG_PLL2CR	0x002c
> +
> +/*
> + * common function
> + */
> +#define rcar_clk_readl(cpg, _reg) readl(cpg->reg + _reg)
> +
> +/*
> + * Reset register definitions.
> + */
> +#define MODEMR 0xe6160060
> +
> +static u32 rcar_gen3_read_mode_pins(void)
> +{
> +	static u32 mode;
> +	static bool mode_valid;
> +
> +	if (!mode_valid) {
> +		void __iomem *modemr = ioremap_nocache(MODEMR, 4);
> +
> +		BUG_ON(!modemr);
> +		mode = ioread32(modemr);
> +		iounmap(modemr);
> +		mode_valid = true;
> +	}
> +
> +	return mode;
> +}
> +
> +/* ------------------------------------------------------------------------
> + * CPG Clock Data
> + */
> +
> +/*
> + *   MD		EXTAL		PLL0	PLL1	PLL2	PLL3	PLL4
> + * 14 13 19 17	(MHz)		*1	*1	*1
> + *-------------------------------------------------------------------
> + * 0  0  0  0	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  0  0  1	16.66 x 1	x180/2	x192/2	x144/2	x128	x144
> + * 0  0  1  0	Prohibited setting
> + * 0  0  1  1	16.66 x 1	x180/2	x192/2	x144/2	x192	x144
> + * 0  1  0  0	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 0  1  0  1	20    x 1	x150/2	x156/2	x120/2	x106	x120
> + * 0  1  1  0	Prohibited setting
> + * 0  1  1  1	20    x 1	x150/2	x156/2	x120/2	x156	x120
> + * 1  0  0  0	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  0  0  1	25    x 1	x120/2	x128/2	x96/2	x84	x96
> + * 1  0  1  0	Prohibited setting
> + * 1  0  1  1	25    x 1	x120/2	x128/2	x96/2	x128	x96
> + * 1  1  0  0	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + * 1  1  0  1	33.33 / 2	x180/2	x192/2	x144/2	x128	x144
> + * 1  1  1  0	Prohibited setting
> + * 1  1  1  1	33.33 / 2	x180/2	x192/2	x144/2	x192	x144
> + *
> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)

As explained in a separate e-mail there's a few clocks on R8A7795 that derive 
directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1 
clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

> + */
> +#define CPG_PLL_CONFIG_INDEX(md)	((((md) & BIT(14)) >> 11) | \
> +					 (((md) & BIT(13)) >> 11) | \
> +					 (((md) & BIT(19)) >> 18) | \
> +					 (((md) & BIT(17)) >> 17))
> +struct cpg_pll_config {
> +	unsigned int extal_div;
> +	unsigned int pll1_mult;
> +	unsigned int pll3_mult;
> +	unsigned int pll4_mult;
> +};
> +
> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
> +/* EXTAL div	PLL1	PLL3	PLL4 */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	192,	192,	144, },
> +	{ 1,	156,	156,	120, },
> +	{ 1,	156,	106,	120, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	156,	156,	120, },
> +	{ 1,	128,	128,	96,  },
> +	{ 1,	128,	84,	96,  },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 1,	128,	128,	96,  },
> +	{ 2,	192,	192,	144, },
> +	{ 2,	192,	128,	144, },
> +	{ 0,	0,	0,	0,   }, /* Prohibited setting */
> +	{ 2,	192,	192,	144, },
> +};
> +
> +/* ------------------------------------------------------------------------
> + * Initialization
> + */
> +
> +static struct clk * __init
> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
> *cpg,
> +			     const struct cpg_pll_config *config,
> +			     unsigned int gen3_clk)
> +{
> +	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
> +	unsigned int mult = 1;
> +	unsigned int div = 1;
> +	u32 value;
> +
> +	switch (gen3_clk) {
> +	case RCAR_GEN3_CLK_MAIN:
> +		parent_name = of_clk_get_parent_name(np, 0);
> +		div = config->extal_div;
> +		break;
> +	case RCAR_GEN3_CLK_PLL0:
> +		/* PLL0 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL0CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;

I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same 
comment for PLL2.

> +		break;
> +	case RCAR_GEN3_CLK_PLL1:
> +		mult = config->pll1_mult / 2;
> +		break;
> +	case RCAR_GEN3_CLK_PLL2:
> +		/* PLL2 is a configurable multiplier clock. Register it as a
> +		 * fixed factor clock for now as there's no generic multiplier
> +		 * clock implementation and we currently have no need to change
> +		 * the multiplier value.
> +		 */
> +		value = rcar_clk_readl(cpg, CPG_PLL2CR);
> +		mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
> +		break;
> +	case RCAR_GEN3_CLK_PLL3:
> +		mult = config->pll3_mult;
> +		break;
> +	case RCAR_GEN3_CLK_PLL4:
> +		mult = config->pll4_mult;
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
> +					 parent_name, 0, mult, div);
> +}
> +
> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
> +{
> +	const struct cpg_pll_config *config;
> +	struct rcar_gen3_cpg *cpg;
> +	u32 cpg_mode;
> +	unsigned int i;
> +	int num_clks;
> +
> +	cpg_mode = rcar_gen3_read_mode_pins();
> +
> +	num_clks = of_property_count_u32_elems(np, "clock-indices");
> +	if (num_clks < 0) {
> +		pr_err("%s: failed to count clocks\n", __func__);
> +		return;
> +	}
> +
> +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> +	if (!cpg)
> +		return;
> +
> +	spin_lock_init(&cpg->lock);

The spin lock is never used. I'd drop it for now, and add it back later 
when/if needed.

> +	cpg->data.clks = cpg->clks;
> +	cpg->data.clk_num = num_clks;
> +
> +	cpg->reg = of_iomap(np, 0);
> +	if (WARN_ON(cpg->reg == NULL))
> +		return;
> +
> +	config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +	if (!config->extal_div) {
> +		pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
> +		       __func__, cpg_mode);
> +		return;
> +	}
> +
> +	for (i = 0; i < num_clks; ++i) {
> +		struct clk *clk;
> +		u32 idx;
> +		int ret;
> +
> +		ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
> +		if (ret < 0)
> +			break;
> +
> +		clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
> +		if (IS_ERR(clk))
> +			pr_err("%s: failed to register %s %u clock (%ld)\n",
> +			       __func__, np->name, idx, PTR_ERR(clk));
> +		else
> +			cpg->data.clks[idx] = clk;
> +	}
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
> +}
> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
> +	       rcar_gen3_cpg_clocks_init);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings

[Laurent Pinchart]

Hello,

Thank you for the patch.

On Monday 31 August 2015 21:49:27 Magnus Damm wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Add r8a7795 to the MSTP DT binding documentation.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>  - Added Acked-by from Geert
> 
>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>  - Picked up patch floating around on ML, added change log
> 
>  Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt |    1
> + 1 file changed, 1 insertion(+)
> 
> --- 0001/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> +++
> work/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt	
20
> 15-08-29 16:18:30.672366518 +0900 @@ -19,6 +19,7 @@ Required Properties:
>      - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2-W) MSTP gate
> clocks - "renesas,r8a7793-mstp-clocks" for R8A7793 (R-Car M2-N) MSTP gate
> clocks - "renesas,r8a7794-mstp-clocks" for R8A7794 (R-Car E2) MSTP gate
> clocks +    - "renesas,r8a7795-mstp-clocks" for R8A7795 (R-Car H3) MSTP
> gate clocks - "renesas,sh73a0-mstp-clocks" for SH73A0 (SH-MobileAG5) MSTP
> gate clocks and "renesas,cpg-mstp-clocks" as a fallback.
>    - reg: Base address and length of the I/O mapped registers used by the
> MSTP

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 04/05] clk: shmobile: Add r8a7795 SoC to MSTP bindings

[Laurent Pinchart]

Hello,

Thank you for the patch.

On Monday 31 August 2015 21:49:27 Magnus Damm wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Add r8a7795 to the MSTP DT binding documentation.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>  - Added Acked-by from Geert
> 
>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>  - Picked up patch floating around on ML, added change log
> 
>  Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt |    1
> + 1 file changed, 1 insertion(+)
> 
> --- 0001/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> +++
> work/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt	
20
> 15-08-29 16:18:30.672366518 +0900 @@ -19,6 +19,7 @@ Required Properties:
>      - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2-W) MSTP gate
> clocks - "renesas,r8a7793-mstp-clocks" for R8A7793 (R-Car M2-N) MSTP gate
> clocks - "renesas,r8a7794-mstp-clocks" for R8A7794 (R-Car E2) MSTP gate
> clocks +    - "renesas,r8a7795-mstp-clocks" for R8A7795 (R-Car H3) MSTP
> gate clocks - "renesas,sh73a0-mstp-clocks" for SH73A0 (SH-MobileAG5) MSTP
> gate clocks and "renesas,cpg-mstp-clocks" as a fallback.
>    - reg: Base address and length of the I/O mapped registers used by the
> MSTP

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>>
>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> particular the R8A7795 SoC.
>>
>> The R-Car Gen3 clock hardware has a register write protection feature that
>> needs to be shared between the CPG function needs to be shared between
>> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
>>
>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Simplified clks array handling - thanks Geert!
>>  - Updated th DT binding documentation to reflect latest state
>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>>
>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>>  - Major things like syscon and driver model require more discussion.
>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>>
>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Reworked driver to rely on clock index instead of strings.
>>  - Dropped use of "clock-output-names".
>>
>>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>>
>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |
>>  32 +
>>  drivers/clk/Makefile                                     |    1
>>  drivers/clk/shmobile/Makefile                            |    1
>>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>>  4 files changed, 279 insertions(+)
>>
>> --- /dev/null
>> +++
>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
>> xt    2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> +
>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> PLLs
>> +and several fixed ratio dividers.
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must be one of
>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> +
>> +  - reg: Base address and length of the memory resource used by the CPG
>> +
>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> +  - #clock-cells: Must be 1
>> +  - clock-indices: Indices of the exported clocks
>
> Do we actually need the clock-indices property, as CPG clocks are numbered
> sequentially ? It seems to me like we could drop the property, especially
> given that the number of clocks is hardcoded in the driver anyway.

There are two reasons why they are there:
1) I like to be able to search DT files to see which node that is
providing what clock.
2) When adding support for additional SoCs we may add new index to the
driver. New SoC may get sparsely populated index lists and old ones
will omit the recently added index.

>> +/*
>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>> + * 14 13 19 17       (MHz)           *1      *1      *1
>> + *-------------------------------------------------------------------
>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>> + * 0  0  1  0        Prohibited setting
>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>> + * 0  1  1  0        Prohibited setting
>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>> + * 1  0  1  0        Prohibited setting
>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>> + * 1  1  1  0        Prohibited setting
>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + *
>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>
> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

Do you think that would reduce complexity or simplify the code? If so
I think we should do it. Otherwise I think it makes sense to simply
follow the data sheet.

>> + */
>> +#define CPG_PLL_CONFIG_INDEX(md)     ((((md) & BIT(14)) >> 11) | \
>> +                                      (((md) & BIT(13)) >> 11) | \
>> +                                      (((md) & BIT(19)) >> 18) | \
>> +                                      (((md) & BIT(17)) >> 17))
>> +struct cpg_pll_config {
>> +     unsigned int extal_div;
>> +     unsigned int pll1_mult;
>> +     unsigned int pll3_mult;
>> +     unsigned int pll4_mult;
>> +};
>> +
>> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
>> +/* EXTAL div PLL1    PLL3    PLL4 */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    156,    156,    120, },
>> +     { 1,    156,    106,    120, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    156,    156,    120, },
>> +     { 1,    128,    128,    96,  },
>> +     { 1,    128,    84,     96,  },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    128,    128,    96,  },
>> +     { 2,    192,    192,    144, },
>> +     { 2,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 2,    192,    192,    144, },
>> +};
>> +
>> +/* ------------------------------------------------------------------------
>> + * Initialization
>> + */
>> +
>> +static struct clk * __init
>> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
>> *cpg,
>> +                          const struct cpg_pll_config *config,
>> +                          unsigned int gen3_clk)
>> +{
>> +     const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
>> +     unsigned int mult = 1;
>> +     unsigned int div = 1;
>> +     u32 value;
>> +
>> +     switch (gen3_clk) {
>> +     case RCAR_GEN3_CLK_MAIN:
>> +             parent_name = of_clk_get_parent_name(np, 0);
>> +             div = config->extal_div;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL0:
>> +             /* PLL0 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL0CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>
> I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same
> comment for PLL2.

I agree.

>> +             break;
>> +     case RCAR_GEN3_CLK_PLL1:
>> +             mult = config->pll1_mult / 2;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL2:
>> +             /* PLL2 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL2CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL3:
>> +             mult = config->pll3_mult;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL4:
>> +             mult = config->pll4_mult;
>> +             break;
>> +     default:
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
>> +                                      parent_name, 0, mult, div);
>> +}
>> +
>> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
>> +{
>> +     const struct cpg_pll_config *config;
>> +     struct rcar_gen3_cpg *cpg;
>> +     u32 cpg_mode;
>> +     unsigned int i;
>> +     int num_clks;
>> +
>> +     cpg_mode = rcar_gen3_read_mode_pins();
>> +
>> +     num_clks = of_property_count_u32_elems(np, "clock-indices");
>> +     if (num_clks < 0) {
>> +             pr_err("%s: failed to count clocks\n", __func__);
>> +             return;
>> +     }
>> +
>> +     cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
>> +     if (!cpg)
>> +             return;
>> +
>> +     spin_lock_init(&cpg->lock);
>
> The spin lock is never used. I'd drop it for now, and add it back later
> when/if needed.

Good point!

>> +     cpg->data.clks = cpg->clks;
>> +     cpg->data.clk_num = num_clks;
>> +
>> +     cpg->reg = of_iomap(np, 0);
>> +     if (WARN_ON(cpg->reg = NULL))
>> +             return;
>> +
>> +     config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
>> +     if (!config->extal_div) {
>> +             pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
>> +                    __func__, cpg_mode);
>> +             return;
>> +     }
>> +
>> +     for (i = 0; i < num_clks; ++i) {
>> +             struct clk *clk;
>> +             u32 idx;
>> +             int ret;
>> +
>> +             ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
>> +             if (ret < 0)
>> +                     break;
>> +
>> +             clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
>> +             if (IS_ERR(clk))
>> +                     pr_err("%s: failed to register %s %u clock (%ld)\n",
>> +                            __func__, np->name, idx, PTR_ERR(clk));
>> +             else
>> +                     cpg->data.clks[idx] = clk;
>> +     }
>> +
>> +     of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
>> +}
>> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
>> +            rcar_gen3_cpg_clocks_init);

Thanks for your comments!!

/ magnus

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>>
>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> particular the R8A7795 SoC.
>>
>> The R-Car Gen3 clock hardware has a register write protection feature that
>> needs to be shared between the CPG function needs to be shared between
>> the CPG and MSTP hardware somehow. So far this feature is simply ignored.
>>
>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Simplified clks array handling - thanks Geert!
>>  - Updated th DT binding documentation to reflect latest state
>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>>
>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Reworked driver to incorporate most feedback from Stephen Boyd - thanks!!
>>  - Major things like syscon and driver model require more discussion.
>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>>
>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>>  - Reworked driver to rely on clock index instead of strings.
>>  - Dropped use of "clock-output-names".
>>
>>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>>
>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt |
>>  32 +
>>  drivers/clk/Makefile                                     |    1
>>  drivers/clk/shmobile/Makefile                            |    1
>>  drivers/clk/shmobile/clk-rcar-gen3.c                     |  245 ++++++++++
>>  4 files changed, 279 insertions(+)
>>
>> --- /dev/null
>> +++
>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.t
>> xt    2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> +
>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> PLLs
>> +and several fixed ratio dividers.
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must be one of
>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> +
>> +  - reg: Base address and length of the memory resource used by the CPG
>> +
>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> +  - #clock-cells: Must be 1
>> +  - clock-indices: Indices of the exported clocks
>
> Do we actually need the clock-indices property, as CPG clocks are numbered
> sequentially ? It seems to me like we could drop the property, especially
> given that the number of clocks is hardcoded in the driver anyway.

There are two reasons why they are there:
1) I like to be able to search DT files to see which node that is
providing what clock.
2) When adding support for additional SoCs we may add new index to the
driver. New SoC may get sparsely populated index lists and old ones
will omit the recently added index.

>> +/*
>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>> + * 14 13 19 17       (MHz)           *1      *1      *1
>> + *-------------------------------------------------------------------
>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>> + * 0  0  1  0        Prohibited setting
>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>> + * 0  1  1  0        Prohibited setting
>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>> + * 1  0  1  0        Prohibited setting
>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>> + * 1  1  1  0        Prohibited setting
>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>> + *
>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>
> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.

Do you think that would reduce complexity or simplify the code? If so
I think we should do it. Otherwise I think it makes sense to simply
follow the data sheet.

>> + */
>> +#define CPG_PLL_CONFIG_INDEX(md)     ((((md) & BIT(14)) >> 11) | \
>> +                                      (((md) & BIT(13)) >> 11) | \
>> +                                      (((md) & BIT(19)) >> 18) | \
>> +                                      (((md) & BIT(17)) >> 17))
>> +struct cpg_pll_config {
>> +     unsigned int extal_div;
>> +     unsigned int pll1_mult;
>> +     unsigned int pll3_mult;
>> +     unsigned int pll4_mult;
>> +};
>> +
>> +static const struct cpg_pll_config cpg_pll_configs[16] __initconst = {
>> +/* EXTAL div PLL1    PLL3    PLL4 */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    192,    192,    144, },
>> +     { 1,    156,    156,    120, },
>> +     { 1,    156,    106,    120, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    156,    156,    120, },
>> +     { 1,    128,    128,    96,  },
>> +     { 1,    128,    84,     96,  },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 1,    128,    128,    96,  },
>> +     { 2,    192,    192,    144, },
>> +     { 2,    192,    128,    144, },
>> +     { 0,    0,      0,      0,   }, /* Prohibited setting */
>> +     { 2,    192,    192,    144, },
>> +};
>> +
>> +/* ------------------------------------------------------------------------
>> + * Initialization
>> + */
>> +
>> +static struct clk * __init
>> +rcar_gen3_cpg_register_clock(struct device_node *np, struct rcar_gen3_cpg
>> *cpg,
>> +                          const struct cpg_pll_config *config,
>> +                          unsigned int gen3_clk)
>> +{
>> +     const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
>> +     unsigned int mult = 1;
>> +     unsigned int div = 1;
>> +     u32 value;
>> +
>> +     switch (gen3_clk) {
>> +     case RCAR_GEN3_CLK_MAIN:
>> +             parent_name = of_clk_get_parent_name(np, 0);
>> +             div = config->extal_div;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL0:
>> +             /* PLL0 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL0CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>
> I'd find 0x3f more readable than (1 << 7) - 1 but that might just be me. Same
> comment for PLL2.

I agree.

>> +             break;
>> +     case RCAR_GEN3_CLK_PLL1:
>> +             mult = config->pll1_mult / 2;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL2:
>> +             /* PLL2 is a configurable multiplier clock. Register it as a
>> +              * fixed factor clock for now as there's no generic multiplier
>> +              * clock implementation and we currently have no need to change
>> +              * the multiplier value.
>> +              */
>> +             value = rcar_clk_readl(cpg, CPG_PLL2CR);
>> +             mult = ((value >> 24) & ((1 << 7) - 1)) + 1;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL3:
>> +             mult = config->pll3_mult;
>> +             break;
>> +     case RCAR_GEN3_CLK_PLL4:
>> +             mult = config->pll4_mult;
>> +             break;
>> +     default:
>> +             return ERR_PTR(-EINVAL);
>> +     }
>> +
>> +     return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
>> +                                      parent_name, 0, mult, div);
>> +}
>> +
>> +static void __init rcar_gen3_cpg_clocks_init(struct device_node *np)
>> +{
>> +     const struct cpg_pll_config *config;
>> +     struct rcar_gen3_cpg *cpg;
>> +     u32 cpg_mode;
>> +     unsigned int i;
>> +     int num_clks;
>> +
>> +     cpg_mode = rcar_gen3_read_mode_pins();
>> +
>> +     num_clks = of_property_count_u32_elems(np, "clock-indices");
>> +     if (num_clks < 0) {
>> +             pr_err("%s: failed to count clocks\n", __func__);
>> +             return;
>> +     }
>> +
>> +     cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
>> +     if (!cpg)
>> +             return;
>> +
>> +     spin_lock_init(&cpg->lock);
>
> The spin lock is never used. I'd drop it for now, and add it back later
> when/if needed.

Good point!

>> +     cpg->data.clks = cpg->clks;
>> +     cpg->data.clk_num = num_clks;
>> +
>> +     cpg->reg = of_iomap(np, 0);
>> +     if (WARN_ON(cpg->reg == NULL))
>> +             return;
>> +
>> +     config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
>> +     if (!config->extal_div) {
>> +             pr_err("%s: Prohibited setting (cpg_mode=0x%x)\n",
>> +                    __func__, cpg_mode);
>> +             return;
>> +     }
>> +
>> +     for (i = 0; i < num_clks; ++i) {
>> +             struct clk *clk;
>> +             u32 idx;
>> +             int ret;
>> +
>> +             ret = of_property_read_u32_index(np, "clock-indices", i, &idx);
>> +             if (ret < 0)
>> +                     break;
>> +
>> +             clk = rcar_gen3_cpg_register_clock(np, cpg, config, idx);
>> +             if (IS_ERR(clk))
>> +                     pr_err("%s: failed to register %s %u clock (%ld)\n",
>> +                            __func__, np->name, idx, PTR_ERR(clk));
>> +             else
>> +                     cpg->data.clks[idx] = clk;
>> +     }
>> +
>> +     of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data);
>> +}
>> +CLK_OF_DECLARE(rcar_gen3_cpg_clks, "renesas,rcar-gen3-cpg-clocks",
>> +            rcar_gen3_cpg_clocks_init);

Thanks for your comments!!

/ magnus

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> +/*
>>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>>> + * 14 13 19 17       (MHz)           *1      *1      *1
>>> + *-------------------------------------------------------------------
>>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>>> + * 0  0  1  0        Prohibited setting
>>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>>> + * 0  1  1  0        Prohibited setting
>>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>>> + * 1  0  1  0        Prohibited setting
>>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>>> + * 1  1  1  0        Prohibited setting
>>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + *
>>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>>
>> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
>> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
>> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

It would avoid having to apply a multiplier of two to the RPC clock.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> +/*
>>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3    PLL4
>>> + * 14 13 19 17       (MHz)           *1      *1      *1
>>> + *-------------------------------------------------------------------
>>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128    x144
>>> + * 0  0  1  0        Prohibited setting
>>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192    x144
>>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106    x120
>>> + * 0  1  1  0        Prohibited setting
>>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156    x120
>>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
>>> + * 1  0  1  0        Prohibited setting
>>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
>>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128    x144
>>> + * 1  1  1  0        Prohibited setting
>>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192    x144
>>> + *
>>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
>>
>> As explained in a separate e-mail there's a few clocks on R8A7795 that derive
>> directly from PLL1 VCO. I thus wonder whether we shouldn't expose the PLL1
>> clock as the VCO output and create VCO/2 using a fixed factor clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

It would avoid having to apply a multiplier of two to the RPC clock.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

On Tuesday 01 September 2015 13:36:28 Geert Uytterhoeven wrote:
> On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> >>> +/*
> >>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >>> PLL4
> >>> + * 14 13 19 17       (MHz)           *1      *1      *1
> >>> + *-------------------------------------------------------------------
> >>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 0  0  1  0        Prohibited setting
> >>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >>> x120
> >>> + * 0  1  1  0        Prohibited setting
> >>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84    
> >>> x96
> >>> + * 1  0  1  0        Prohibited setting
> >>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 1  1  1  0        Prohibited setting
> >>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + *
> >>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> >> 
> >> As explained in a separate e-mail there's a few clocks on R8A7795 that
> >> derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> >> the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> >> clock in DT.
> >
> > Do you think that would reduce complexity or simplify the code? If so
> > I think we should do it. Otherwise I think it makes sense to simply
> > follow the data sheet.
> 
> It would avoid having to apply a multiplier of two to the RPC clock.

And it would match the hardware clock tree topology. Applying a multiplier to 
the RPC clock is a hack I can live with though, so I'll let you decide which 
option is best, but in general matching the hardware seems a good idea to me.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

On Tuesday 01 September 2015 13:36:28 Geert Uytterhoeven wrote:
> On Tue, Sep 1, 2015 at 12:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> >>> +/*
> >>> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >>> PLL4
> >>> + * 14 13 19 17       (MHz)           *1      *1      *1
> >>> + *-------------------------------------------------------------------
> >>> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 0  0  1  0        Prohibited setting
> >>> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >>> x120
> >>> + * 0  1  1  0        Prohibited setting
> >>> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >>> x120
> >>> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84    
> >>> x96
> >>> + * 1  0  1  0        Prohibited setting
> >>> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128   
> >>> x96
> >>> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >>> x144
> >>> + * 1  1  1  0        Prohibited setting
> >>> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >>> x144
> >>> + *
> >>> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> >> 
> >> As explained in a separate e-mail there's a few clocks on R8A7795 that
> >> derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> >> the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> >> clock in DT.
> >
> > Do you think that would reduce complexity or simplify the code? If so
> > I think we should do it. Otherwise I think it makes sense to simply
> > follow the data sheet.
> 
> It would avoid having to apply a multiplier of two to the RPC clock.

And it would match the hardware clock tree topology. Applying a multiplier to 
the RPC clock is a hack I can live with though, so I'll let you decide which 
option is best, but in general matching the hardware seems a good idea to me.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >> 
> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >> particular the R8A7795 SoC.
> >> 
> >> The R-Car Gen3 clock hardware has a register write protection feature
> >> that needs to be shared between the CPG function needs to be shared
> >> between the CPG and MSTP hardware somehow. So far this feature is simply
> >> ignored.
> >> 
> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >> ---
> >> 
> >>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Simplified clks array handling - thanks Geert!
> >>  - Updated th DT binding documentation to reflect latest state
> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>  
> >>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>  thanks!!
> >>  - Major things like syscon and driver model require more discussion.
> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>  
> >>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Reworked driver to rely on clock index instead of strings.
> >>  - Dropped use of "clock-output-names".
> >>  
> >>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>  
> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
> >>  |   32 +
> >>  drivers/clk/Makefile                                |    1
> >>  drivers/clk/shmobile/Makefile                       |    1
> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>  4 files changed, 279 insertions(+)
> >> 
> >> --- /dev/null
> >> +++
> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
> >> .txt
> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >> +
> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> >> PLLs
> >> +and several fixed ratio dividers.
> >> +
> >> +Required Properties:
> >> +
> >> +  - compatible: Must be one of
> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >> +
> >> +  - reg: Base address and length of the memory resource used by the CPG
> >> +
> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >> +  - #clock-cells: Must be 1
> >> +  - clock-indices: Indices of the exported clocks
> > 
> > Do we actually need the clock-indices property, as CPG clocks are numbered
> > sequentially ? It seems to me like we could drop the property, especially
> > given that the number of clocks is hardcoded in the driver anyway.
> 
> There are two reasons why they are there:
> 1) I like to be able to search DT files to see which node that is
> providing what clock.

The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
clock.h so that should already give a hint. Additionally I don't think you'll 
search the CPG core clocks very often in the DT sources as they're usually not 
used directly by devices :-)

> 2) When adding support for additional SoCs we may add new index to the
> driver. New SoC may get sparsely populated index lists and old ones
> will omit the recently added index.

If the CPG core differs between SoCs shouldn't that be handled by the DT 
compatibility string ?

> >> +/*
> >> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >> PLL4
> >> + * 14 13 19 17       (MHz)           *1      *1      *1
> >> + *-------------------------------------------------------------------
> >> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 0  0  1  0        Prohibited setting
> >> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >> x120
> >> + * 0  1  1  0        Prohibited setting
> >> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
> >> + * 1  0  1  0        Prohibited setting
> >> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 1  1  1  0        Prohibited setting
> >> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + *
> >> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> > 
> > As explained in a separate e-mail there's a few clocks on R8A7795 that
> > derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> > the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> > clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

I've commented on this in a reply to Geert's e-mail.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >> 
> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >> particular the R8A7795 SoC.
> >> 
> >> The R-Car Gen3 clock hardware has a register write protection feature
> >> that needs to be shared between the CPG function needs to be shared
> >> between the CPG and MSTP hardware somehow. So far this feature is simply
> >> ignored.
> >> 
> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >> ---
> >> 
> >>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Simplified clks array handling - thanks Geert!
> >>  - Updated th DT binding documentation to reflect latest state
> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>  
> >>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>  thanks!!
> >>  - Major things like syscon and driver model require more discussion.
> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>  
> >>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
> >>  - Reworked driver to rely on clock index instead of strings.
> >>  - Dropped use of "clock-output-names".
> >>  
> >>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>  
> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
> >>  |   32 +
> >>  drivers/clk/Makefile                                |    1
> >>  drivers/clk/shmobile/Makefile                       |    1
> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>  4 files changed, 279 insertions(+)
> >> 
> >> --- /dev/null
> >> +++
> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
> >> .txt
> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >> +
> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
> >> PLLs
> >> +and several fixed ratio dividers.
> >> +
> >> +Required Properties:
> >> +
> >> +  - compatible: Must be one of
> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >> +
> >> +  - reg: Base address and length of the memory resource used by the CPG
> >> +
> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >> +  - #clock-cells: Must be 1
> >> +  - clock-indices: Indices of the exported clocks
> > 
> > Do we actually need the clock-indices property, as CPG clocks are numbered
> > sequentially ? It seems to me like we could drop the property, especially
> > given that the number of clocks is hardcoded in the driver anyway.
> 
> There are two reasons why they are there:
> 1) I like to be able to search DT files to see which node that is
> providing what clock.

The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
clock.h so that should already give a hint. Additionally I don't think you'll 
search the CPG core clocks very often in the DT sources as they're usually not 
used directly by devices :-)

> 2) When adding support for additional SoCs we may add new index to the
> driver. New SoC may get sparsely populated index lists and old ones
> will omit the recently added index.

If the CPG core differs between SoCs shouldn't that be handled by the DT 
compatibility string ?

> >> +/*
> >> + *   MD              EXTAL           PLL0    PLL1    PLL2    PLL3   
> >> PLL4
> >> + * 14 13 19 17       (MHz)           *1      *1      *1
> >> + *-------------------------------------------------------------------
> >> + * 0  0  0  0        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  0  0  1        16.66 x 1       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 0  0  1  0        Prohibited setting
> >> + * 0  0  1  1        16.66 x 1       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 0  1  0  0        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 0  1  0  1        20    x 1       x150/2  x156/2  x120/2  x106   
> >> x120
> >> + * 0  1  1  0        Prohibited setting
> >> + * 0  1  1  1        20    x 1       x150/2  x156/2  x120/2  x156   
> >> x120
> >> + * 1  0  0  0        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  0  0  1        25    x 1       x120/2  x128/2  x96/2   x84     x96
> >> + * 1  0  1  0        Prohibited setting
> >> + * 1  0  1  1        25    x 1       x120/2  x128/2  x96/2   x128    x96
> >> + * 1  1  0  0        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + * 1  1  0  1        33.33 / 2       x180/2  x192/2  x144/2  x128   
> >> x144
> >> + * 1  1  1  0        Prohibited setting
> >> + * 1  1  1  1        33.33 / 2       x180/2  x192/2  x144/2  x192   
> >> x144
> >> + *
> >> + * *1 : datasheet indicates VCO output (PLLx = VCO/2)
> > 
> > As explained in a separate e-mail there's a few clocks on R8A7795 that
> > derive directly from PLL1 VCO. I thus wonder whether we shouldn't expose
> > the PLL1 clock as the VCO output and create VCO/2 using a fixed factor
> > clock in DT.
>
> Do you think that would reduce complexity or simplify the code? If so
> I think we should do it. Otherwise I think it makes sense to simply
> follow the data sheet.

I've commented on this in a reply to Geert's e-mail.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Geert Uytterhoeven]

Hi Magnus,

On Mon, Aug 31, 2015 at 2:49 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> --- /dev/null
> +++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt       2015-08-31 15:49:10.000000000 +0900
> @@ -0,0 +1,32 @@

> +       cpg_clocks: cpg_clocks@e6150000 {
> +               compatible = "renesas,r8a7795-cpg-clocks",
> +                            "renesas,rcar-gen3-cpg-clocks";
> +               reg = <0 0xe6150000 0 0x1000>;
> +               clocks = <&extal_clk>;
> +               #clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;

The new "clock-indices" hunk uses spaces insteads of TABs for indentation.

> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Geert Uytterhoeven]

Hi Magnus,

On Mon, Aug 31, 2015 at 2:49 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> --- /dev/null
> +++ work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt       2015-08-31 15:49:10.000000000 +0900
> @@ -0,0 +1,32 @@

> +       cpg_clocks: cpg_clocks@e6150000 {
> +               compatible = "renesas,r8a7795-cpg-clocks",
> +                            "renesas,rcar-gen3-cpg-clocks";
> +               reg = <0 0xe6150000 0 0x1000>;
> +               clocks = <&extal_clk>;
> +               #clock-cells = <1>;
> +                clock-indices = <
> +                             R8A7795_CLK_MAIN R8A7795_CLK_PLL0
> +                             R8A7795_CLK_PLL1 R8A7795_CLK_PLL2
> +                             R8A7795_CLK_PLL3 R8A7795_CLK_PLL4
> +                >;

The new "clock-indices" hunk uses spaces insteads of TABs for indentation.

> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>
>> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >> particular the R8A7795 SoC.
>> >>
>> >> The R-Car Gen3 clock hardware has a register write protection feature
>> >> that needs to be shared between the CPG function needs to be shared
>> >> between the CPG and MSTP hardware somehow. So far this feature is simply
>> >> ignored.
>> >>
>> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> >> ---
>> >>
>> >>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Simplified clks array handling - thanks Geert!
>> >>  - Updated th DT binding documentation to reflect latest state
>> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>
>> >>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>  thanks!!
>> >>  - Major things like syscon and driver model require more discussion.
>> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>
>> >>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Reworked driver to rely on clock index instead of strings.
>> >>  - Dropped use of "clock-output-names".
>> >>
>> >>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>
>> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
>> >>  |   32 +
>> >>  drivers/clk/Makefile                                |    1
>> >>  drivers/clk/shmobile/Makefile                       |    1
>> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>  4 files changed, 279 insertions(+)
>> >>
>> >> --- /dev/null
>> >> +++
>> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
>> >> .txt
>> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >> +
>> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> >> PLLs
>> >> +and several fixed ratio dividers.
>> >> +
>> >> +Required Properties:
>> >> +
>> >> +  - compatible: Must be one of
>> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >> +
>> >> +  - reg: Base address and length of the memory resource used by the CPG
>> >> +
>> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >> +  - #clock-cells: Must be 1
>> >> +  - clock-indices: Indices of the exported clocks
>> >
>> > Do we actually need the clock-indices property, as CPG clocks are numbered
>> > sequentially ? It seems to me like we could drop the property, especially
>> > given that the number of clocks is hardcoded in the driver anyway.
>>
>> There are two reasons why they are there:
>> 1) I like to be able to search DT files to see which node that is
>> providing what clock.
>
> The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
> clock.h so that should already give a hint. Additionally I don't think you'll
> search the CPG core clocks very often in the DT sources as they're usually not
> used directly by devices :-)

You are right that they may not be searched very frequently. But at
least the CPG is consistent with MSTP with this implementation. =)

>> 2) When adding support for additional SoCs we may add new index to the
>> driver. New SoC may get sparsely populated index lists and old ones
>> will omit the recently added index.
>
> If the CPG core differs between SoCs shouldn't that be handled by the DT
> compatibility string ?

You know that I personally always prefered an incremental approach
with little speculation in DT. =)

So using SoC specific compat strings over generic ones would in
general be my preferred choice. I suspect the initial developer of
this patch simply followed the same style as Gen2 with a generic
compat string.

Does this mean that you for the r8a7795 CPG prefer SoC specific compat
string to begin with over a more generic one? If we go for SoC
specific compat string then perhaps we also should change file names?

Thanks,

/ magnus

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> > On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>
>> >> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >> particular the R8A7795 SoC.
>> >>
>> >> The R-Car Gen3 clock hardware has a register write protection feature
>> >> that needs to be shared between the CPG function needs to be shared
>> >> between the CPG and MSTP hardware somehow. So far this feature is simply
>> >> ignored.
>> >>
>> >> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> >> ---
>> >>
>> >>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Simplified clks array handling - thanks Geert!
>> >>  - Updated th DT binding documentation to reflect latest state
>> >>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>
>> >>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>  thanks!!
>> >>  - Major things like syscon and driver model require more discussion.
>> >>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>
>> >>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>> >>  - Reworked driver to rely on clock index instead of strings.
>> >>  - Dropped use of "clock-output-names".
>> >>
>> >>  Earlier versions: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>
>> >>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.txt
>> >>  |   32 +
>> >>  drivers/clk/Makefile                                |    1
>> >>  drivers/clk/shmobile/Makefile                       |    1
>> >>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>  4 files changed, 279 insertions(+)
>> >>
>> >> --- /dev/null
>> >> +++
>> >> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks
>> >> .txt
>> >> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >> +
>> >> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes three
>> >> PLLs
>> >> +and several fixed ratio dividers.
>> >> +
>> >> +Required Properties:
>> >> +
>> >> +  - compatible: Must be one of
>> >> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >> +
>> >> +  - reg: Base address and length of the memory resource used by the CPG
>> >> +
>> >> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >> +  - #clock-cells: Must be 1
>> >> +  - clock-indices: Indices of the exported clocks
>> >
>> > Do we actually need the clock-indices property, as CPG clocks are numbered
>> > sequentially ? It seems to me like we could drop the property, especially
>> > given that the number of clocks is hardcoded in the driver anyway.
>>
>> There are two reasons why they are there:
>> 1) I like to be able to search DT files to see which node that is
>> providing what clock.
>
> The clocks are listed in the CPG section of include/dt-bindings/clock/r8a7795-
> clock.h so that should already give a hint. Additionally I don't think you'll
> search the CPG core clocks very often in the DT sources as they're usually not
> used directly by devices :-)

You are right that they may not be searched very frequently. But at
least the CPG is consistent with MSTP with this implementation. =)

>> 2) When adding support for additional SoCs we may add new index to the
>> driver. New SoC may get sparsely populated index lists and old ones
>> will omit the recently added index.
>
> If the CPG core differs between SoCs shouldn't that be handled by the DT
> compatibility string ?

You know that I personally always prefered an incremental approach
with little speculation in DT. =)

So using SoC specific compat strings over generic ones would in
general be my preferred choice. I suspect the initial developer of
this patch simply followed the same style as Gen2 with a generic
compat string.

Does this mean that you for the r8a7795 CPG prefer SoC specific compat
string to begin with over a more generic one? If we go for SoC
specific compat string then perhaps we also should change file names?

Thanks,

/ magnus

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >>>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>> 
> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >>>> particular the R8A7795 SoC.
> >>>> 
> >>>> The R-Car Gen3 clock hardware has a register write protection feature
> >>>> that needs to be shared between the CPG function needs to be shared
> >>>> between the CPG and MSTP hardware somehow. So far this feature is
> >>>> simply ignored.
> >>>> 
> >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >>>> ---
> >>>> 
> >>>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Simplified clks array handling - thanks Geert!
> >>>>  - Updated th DT binding documentation to reflect latest state
> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>>>  
> >>>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>>>  thanks!!
> >>>>  - Major things like syscon and driver model require more discussion.
> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>>>  
> >>>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Reworked driver to rely on clock index instead of strings.
> >>>>  - Dropped use of "clock-output-names".
> >>>>  
> >>>>  Earlier versions: Kuninori Morimoto
> >>>>  <kuninori.morimoto.gx@renesas.com>
> >>>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>>  
> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
> >>>>  txt  |   32 +
> >>>>  drivers/clk/Makefile                                |    1
> >>>>  drivers/clk/shmobile/Makefile                       |    1
> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>>>  4 files changed, 279 insertions(+)
> >>>> 
> >>>> --- /dev/null
> >>>> +++
> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
> >>>> cks.txt
> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >>>> +
> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
> >>>> three PLLs
> >>>> +and several fixed ratio dividers.
> >>>> +
> >>>> +Required Properties:
> >>>> +
> >>>> +  - compatible: Must be one of
> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >>>> +
> >>>> +  - reg: Base address and length of the memory resource used by the
> >>>> CPG
> >>>> +
> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >>>> +  - #clock-cells: Must be 1
> >>>> +  - clock-indices: Indices of the exported clocks
> >>> 
> >>> Do we actually need the clock-indices property, as CPG clocks are
> >>> numbered sequentially ? It seems to me like we could drop the property,
> >>> especially given that the number of clocks is hardcoded in the driver
> >>> anyway.
> >> 
> >> There are two reasons why they are there:
> >> 1) I like to be able to search DT files to see which node that is
> >> providing what clock.
> > 
> > The clocks are listed in the CPG section of
> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
> > hint. Additionally I don't think you'll search the CPG core clocks very
> > often in the DT sources as they're usually not used directly by devices
> > :-)
> 
> You are right that they may not be searched very frequently. But at
> least the CPG is consistent with MSTP with this implementation. =)
> 
> >> 2) When adding support for additional SoCs we may add new index to the
> >> driver. New SoC may get sparsely populated index lists and old ones
> >> will omit the recently added index.
> > 
> > If the CPG core differs between SoCs shouldn't that be handled by the DT
> > compatibility string ?
> 
> You know that I personally always prefered an incremental approach
> with little speculation in DT. =)
> 
> So using SoC specific compat strings over generic ones would in
> general be my preferred choice. I suspect the initial developer of
> this patch simply followed the same style as Gen2 with a generic
> compat string.
> 
> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
> string to begin with over a more generic one? If we go for SoC
> specific compat string then perhaps we also should change file names?

We use an SoC-specific compat string already so we won't be stuck in the 
future. I would keep matching on the generic compatible string for now until 
the unlikely future event that a Gen3 SoC would have a different CPG. I don't 
think there's too much point in making DT bindings (and the corresponding 
code) more complex than necessary to plan for an unlikely future hardware 
change that we know nothing about at this point anyway.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Laurent Pinchart]

Hi Magnus,

On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
> >>>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>> 
> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
> >>>> particular the R8A7795 SoC.
> >>>> 
> >>>> The R-Car Gen3 clock hardware has a register write protection feature
> >>>> that needs to be shared between the CPG function needs to be shared
> >>>> between the CPG and MSTP hardware somehow. So far this feature is
> >>>> simply ignored.
> >>>> 
> >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >>>> ---
> >>>> 
> >>>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Simplified clks array handling - thanks Geert!
> >>>>  - Updated th DT binding documentation to reflect latest state
> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
> >>>>  
> >>>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
> >>>>  thanks!!
> >>>>  - Major things like syscon and driver model require more discussion.
> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
> >>>>  
> >>>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
> >>>>  - Reworked driver to rely on clock index instead of strings.
> >>>>  - Dropped use of "clock-output-names".
> >>>>  
> >>>>  Earlier versions: Kuninori Morimoto
> >>>>  <kuninori.morimoto.gx@renesas.com>
> >>>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
> >>>>  
> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
> >>>>  txt  |   32 +
> >>>>  drivers/clk/Makefile                                |    1
> >>>>  drivers/clk/shmobile/Makefile                       |    1
> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
> >>>>  4 files changed, 279 insertions(+)
> >>>> 
> >>>> --- /dev/null
> >>>> +++
> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
> >>>> cks.txt
> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
> >>>> +
> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
> >>>> three PLLs
> >>>> +and several fixed ratio dividers.
> >>>> +
> >>>> +Required Properties:
> >>>> +
> >>>> +  - compatible: Must be one of
> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
> >>>> +
> >>>> +  - reg: Base address and length of the memory resource used by the
> >>>> CPG
> >>>> +
> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
> >>>> +  - #clock-cells: Must be 1
> >>>> +  - clock-indices: Indices of the exported clocks
> >>> 
> >>> Do we actually need the clock-indices property, as CPG clocks are
> >>> numbered sequentially ? It seems to me like we could drop the property,
> >>> especially given that the number of clocks is hardcoded in the driver
> >>> anyway.
> >> 
> >> There are two reasons why they are there:
> >> 1) I like to be able to search DT files to see which node that is
> >> providing what clock.
> > 
> > The clocks are listed in the CPG section of
> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
> > hint. Additionally I don't think you'll search the CPG core clocks very
> > often in the DT sources as they're usually not used directly by devices
> > :-)
> 
> You are right that they may not be searched very frequently. But at
> least the CPG is consistent with MSTP with this implementation. =)
> 
> >> 2) When adding support for additional SoCs we may add new index to the
> >> driver. New SoC may get sparsely populated index lists and old ones
> >> will omit the recently added index.
> > 
> > If the CPG core differs between SoCs shouldn't that be handled by the DT
> > compatibility string ?
> 
> You know that I personally always prefered an incremental approach
> with little speculation in DT. =)
> 
> So using SoC specific compat strings over generic ones would in
> general be my preferred choice. I suspect the initial developer of
> this patch simply followed the same style as Gen2 with a generic
> compat string.
> 
> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
> string to begin with over a more generic one? If we go for SoC
> specific compat string then perhaps we also should change file names?

We use an SoC-specific compat string already so we won't be stuck in the 
future. I would keep matching on the generic compatible string for now until 
the unlikely future event that a Gen3 SoC would have a different CPG. I don't 
think there's too much point in making DT bindings (and the corresponding 
code) more complex than necessary to plan for an unlikely future hardware 
change that we know nothing about at this point anyway.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Wed, Sep 2, 2015 at 2:21 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
>> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >>>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>>
>> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >>>> particular the R8A7795 SoC.
>> >>>>
>> >>>> The R-Car Gen3 clock hardware has a register write protection feature
>> >>>> that needs to be shared between the CPG function needs to be shared
>> >>>> between the CPG and MSTP hardware somehow. So far this feature is
>> >>>> simply ignored.
>> >>>>
>> >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> >>>> ---
>> >>>>
>> >>>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Simplified clks array handling - thanks Geert!
>> >>>>  - Updated th DT binding documentation to reflect latest state
>> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>>>
>> >>>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>>>  thanks!!
>> >>>>  - Major things like syscon and driver model require more discussion.
>> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>>>
>> >>>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Reworked driver to rely on clock index instead of strings.
>> >>>>  - Dropped use of "clock-output-names".
>> >>>>
>> >>>>  Earlier versions: Kuninori Morimoto
>> >>>>  <kuninori.morimoto.gx@renesas.com>
>> >>>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>>
>> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
>> >>>>  txt  |   32 +
>> >>>>  drivers/clk/Makefile                                |    1
>> >>>>  drivers/clk/shmobile/Makefile                       |    1
>> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>>>  4 files changed, 279 insertions(+)
>> >>>>
>> >>>> --- /dev/null
>> >>>> +++
>> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
>> >>>> cks.txt
>> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >>>> +
>> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
>> >>>> three PLLs
>> >>>> +and several fixed ratio dividers.
>> >>>> +
>> >>>> +Required Properties:
>> >>>> +
>> >>>> +  - compatible: Must be one of
>> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >>>> +
>> >>>> +  - reg: Base address and length of the memory resource used by the
>> >>>> CPG
>> >>>> +
>> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >>>> +  - #clock-cells: Must be 1
>> >>>> +  - clock-indices: Indices of the exported clocks
>> >>>
>> >>> Do we actually need the clock-indices property, as CPG clocks are
>> >>> numbered sequentially ? It seems to me like we could drop the property,
>> >>> especially given that the number of clocks is hardcoded in the driver
>> >>> anyway.
>> >>
>> >> There are two reasons why they are there:
>> >> 1) I like to be able to search DT files to see which node that is
>> >> providing what clock.
>> >
>> > The clocks are listed in the CPG section of
>> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
>> > hint. Additionally I don't think you'll search the CPG core clocks very
>> > often in the DT sources as they're usually not used directly by devices
>> > :-)
>>
>> You are right that they may not be searched very frequently. But at
>> least the CPG is consistent with MSTP with this implementation. =)
>>
>> >> 2) When adding support for additional SoCs we may add new index to the
>> >> driver. New SoC may get sparsely populated index lists and old ones
>> >> will omit the recently added index.
>> >
>> > If the CPG core differs between SoCs shouldn't that be handled by the DT
>> > compatibility string ?
>>
>> You know that I personally always prefered an incremental approach
>> with little speculation in DT. =)
>>
>> So using SoC specific compat strings over generic ones would in
>> general be my preferred choice. I suspect the initial developer of
>> this patch simply followed the same style as Gen2 with a generic
>> compat string.
>>
>> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
>> string to begin with over a more generic one? If we go for SoC
>> specific compat string then perhaps we also should change file names?
>
> We use an SoC-specific compat string already so we won't be stuck in the
> future. I would keep matching on the generic compatible string for now until
> the unlikely future event that a Gen3 SoC would have a different CPG. I don't
> think there's too much point in making DT bindings (and the corresponding
> code) more complex than necessary to plan for an unlikely future hardware
> change that we know nothing about at this point anyway.

Totally agree!

So we discussed this over chat a bit more and the following
observations/agreements were made:

- Magnus clarified that the clock-indices used in current Gen3 CPG
version were selected over clock-output-names based on feedback from
CCF maintainers that clock-output-names were best to be dropped. Both
Laurent and Magnus agreed that getting rid of the clock-output-names
in DT must be a good thing - especially if the CCF maintainers prefer
that.

- Removing clock-output-names from DT does however not work as-is,
some CCF core changes may be needed. We need to work together with the
CCF maintainers to resolve this.

- We did a quick study of CPG on Gen2 and noticed that r8a7790 and
r8a7791 core clocks are not identical.

- On Gen2 using a generic DT compat string with possibility to add
clocks seemed to work well when adding r8a7791 support as second SoC
after r8a7790.

- Using clock-indices on Gen3 to allow adding new clocks over time
with generic DT compat string match in the driver to begin with seems
like a good way forward.

- The current style of using SoC-specific and generic DT compat
strings in DT documentation and DTS together with generic DT compat
string in driver (with opt-in SoC specific matcing if needed) works
well for most on-chip devices/drivers and CPG is no exception.

- If a future SoC has some incompatibility when it comes to core
clocks then we can chose to deal with this either via a DT compat
string check or extending the number of clocks using a clock-index.
How to handle this will most likely be a case-by-case desicion.

- Sharing clock index #defines for DT ABI through a header file is
desired but it is OK to handle it as an incremental feature patch.

The above basically means that the current driver is in pretty OK
shape, but we still need to figure out how to adjust the CCF core code
to allow operation without clock-output-names in DT.

Hope this sums up the current situation!

Cheers,

/ magnus

Re: [PATCH v5 02/05] clk: shmobile: Add Renesas R-Car Gen3 CPG support

[Magnus Damm]

Hi Laurent,

On Wed, Sep 2, 2015 at 2:21 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 02 September 2015 11:27:27 Magnus Damm wrote:
>> On Tue, Sep 1, 2015 at 9:30 PM, Laurent Pinchart wrote:
>> > On Tuesday 01 September 2015 19:59:01 Magnus Damm wrote:
>> >> On Tue, Sep 1, 2015 at 3:00 PM, Laurent Pinchart wrote:
>> >>> On Monday 31 August 2015 21:49:04 Magnus Damm wrote:
>> >>>> From: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>>
>> >>>> This V5 patch adds initial CPG support for R-Car Generation 3 and in
>> >>>> particular the R8A7795 SoC.
>> >>>>
>> >>>> The R-Car Gen3 clock hardware has a register write protection feature
>> >>>> that needs to be shared between the CPG function needs to be shared
>> >>>> between the CPG and MSTP hardware somehow. So far this feature is
>> >>>> simply ignored.
>> >>>>
>> >>>> Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> >>>> ---
>> >>>>
>> >>>>  Changes since V4: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Simplified clks array handling - thanks Geert!
>> >>>>  - Updated th DT binding documentation to reflect latest state
>> >>>>  - of_property_count_strings() -> of_property_count_u32_elems() fix
>> >>>>
>> >>>>  Changes since V3: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Reworked driver to incorporate most feedback from Stephen Boyd -
>> >>>>  thanks!!
>> >>>>  - Major things like syscon and driver model require more discussion.
>> >>>>  - Added hunk to build drivers/clk/shmobile if ARCH_RENESAS is set.
>> >>>>
>> >>>>  Changes since V2: (Magnus Damm <damm+renesas@opensource.se>)
>> >>>>  - Reworked driver to rely on clock index instead of strings.
>> >>>>  - Dropped use of "clock-output-names".
>> >>>>
>> >>>>  Earlier versions: Kuninori Morimoto
>> >>>>  <kuninori.morimoto.gx@renesas.com>
>> >>>>  Initial version: Gaku Inami <gaku.inami.xw@bp.renesas.com>
>> >>>>
>> >>>>  Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clocks.
>> >>>>  txt  |   32 +
>> >>>>  drivers/clk/Makefile                                |    1
>> >>>>  drivers/clk/shmobile/Makefile                       |    1
>> >>>>  drivers/clk/shmobile/clk-rcar-gen3.c                |  245 ++++++++++
>> >>>>  4 files changed, 279 insertions(+)
>> >>>>
>> >>>> --- /dev/null
>> >>>> +++
>> >>>> work/Documentation/devicetree/bindings/clock/renesas,rcar-gen3-cpg-clo
>> >>>> cks.txt
>> >>>> 2015-08-31 15:49:10.000000000 +0900 @@ -0,0 +1,32 @@
>> >>>> +* Renesas R-Car Gen3 Clock Pulse Generator (CPG)
>> >>>> +
>> >>>> +The CPG generates core clocks for the R-Car Gen3 SoCs. It includes
>> >>>> three PLLs
>> >>>> +and several fixed ratio dividers.
>> >>>> +
>> >>>> +Required Properties:
>> >>>> +
>> >>>> +  - compatible: Must be one of
>> >>>> +    - "renesas,r8a7795-cpg-clocks" for the r8a7795 CPG
>> >>>> +    - "renesas,rcar-gen3-cpg-clocks" for the generic R-Car Gen3 CPG
>> >>>> +
>> >>>> +  - reg: Base address and length of the memory resource used by the
>> >>>> CPG
>> >>>> +
>> >>>> +  - clocks: References to the parent clocks: first to the EXTAL clock
>> >>>> +  - #clock-cells: Must be 1
>> >>>> +  - clock-indices: Indices of the exported clocks
>> >>>
>> >>> Do we actually need the clock-indices property, as CPG clocks are
>> >>> numbered sequentially ? It seems to me like we could drop the property,
>> >>> especially given that the number of clocks is hardcoded in the driver
>> >>> anyway.
>> >>
>> >> There are two reasons why they are there:
>> >> 1) I like to be able to search DT files to see which node that is
>> >> providing what clock.
>> >
>> > The clocks are listed in the CPG section of
>> > include/dt-bindings/clock/r8a7795- clock.h so that should already give a
>> > hint. Additionally I don't think you'll search the CPG core clocks very
>> > often in the DT sources as they're usually not used directly by devices
>> > :-)
>>
>> You are right that they may not be searched very frequently. But at
>> least the CPG is consistent with MSTP with this implementation. =)
>>
>> >> 2) When adding support for additional SoCs we may add new index to the
>> >> driver. New SoC may get sparsely populated index lists and old ones
>> >> will omit the recently added index.
>> >
>> > If the CPG core differs between SoCs shouldn't that be handled by the DT
>> > compatibility string ?
>>
>> You know that I personally always prefered an incremental approach
>> with little speculation in DT. =)
>>
>> So using SoC specific compat strings over generic ones would in
>> general be my preferred choice. I suspect the initial developer of
>> this patch simply followed the same style as Gen2 with a generic
>> compat string.
>>
>> Does this mean that you for the r8a7795 CPG prefer SoC specific compat
>> string to begin with over a more generic one? If we go for SoC
>> specific compat string then perhaps we also should change file names?
>
> We use an SoC-specific compat string already so we won't be stuck in the
> future. I would keep matching on the generic compatible string for now until
> the unlikely future event that a Gen3 SoC would have a different CPG. I don't
> think there's too much point in making DT bindings (and the corresponding
> code) more complex than necessary to plan for an unlikely future hardware
> change that we know nothing about at this point anyway.

Totally agree!

So we discussed this over chat a bit more and the following
observations/agreements were made:

- Magnus clarified that the clock-indices used in current Gen3 CPG
version were selected over clock-output-names based on feedback from
CCF maintainers that clock-output-names were best to be dropped. Both
Laurent and Magnus agreed that getting rid of the clock-output-names
in DT must be a good thing - especially if the CCF maintainers prefer
that.

- Removing clock-output-names from DT does however not work as-is,
some CCF core changes may be needed. We need to work together with the
CCF maintainers to resolve this.

- We did a quick study of CPG on Gen2 and noticed that r8a7790 and
r8a7791 core clocks are not identical.

- On Gen2 using a generic DT compat string with possibility to add
clocks seemed to work well when adding r8a7791 support as second SoC
after r8a7790.

- Using clock-indices on Gen3 to allow adding new clocks over time
with generic DT compat string match in the driver to begin with seems
like a good way forward.

- The current style of using SoC-specific and generic DT compat
strings in DT documentation and DTS together with generic DT compat
string in driver (with opt-in SoC specific matcing if needed) works
well for most on-chip devices/drivers and CPG is no exception.

- If a future SoC has some incompatibility when it comes to core
clocks then we can chose to deal with this either via a DT compat
string check or extending the number of clocks using a clock-index.
How to handle this will most likely be a case-by-case desicion.

- Sharing clock index #defines for DT ABI through a header file is
desired but it is OK to handle it as an incremental feature patch.

The above basically means that the current driver is in pretty OK
shape, but we still need to figure out how to adjust the CCF core code
to allow operation without clock-output-names in DT.

Hope this sums up the current situation!

Cheers,

/ magnus