soc: renesas: Add R-Car RST driver for obtaining mode pin state

Add DT bindings for the Renesas R-Car Reset Controller (R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST).

As the features provided by the hardware module differ a lot across the various SoC families and members, only SoC-specific compatible values are defined.

For now we use the RST only for providing access to the state of the mode pins.

Signed-off-by Geert Uytterhoeven <[email protected]>

Acked-by Magnus Damm <[email protected]>

---
v3:
- Clarify current usage,
- Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
- Drop "syscon" compatible value,
- Add R-Car M3-W,
- Add R-Car Gen1,

v2:
- Add Acked-by.

---
 .../devicetree/bindings/reset/renesas,rst.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +0 -0

new file mode 100644
index 0000000000000000..488c72e1ee849cd3

Add DT bindings for the Renesas R-Car Reset Controller (R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST).

As the features provided by the hardware module differ a lot across the various SoC families and members, only SoC-specific compatible values are defined.

For now we use the RST only for providing access to the state of the mode pins.

Signed-off-by Geert Uytterhoeven <[email protected]>

Acked-by Magnus Damm <[email protected]>

---
v3:
- Clarify current usage,
- Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
- Drop "syscon" compatible value,
- Add R-Car M3-W,
- Add R-Car Gen1,

v2:
- Add Acked-by.

---
 .../devicetree/bindings/reset/renesas,rst.txt      | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/reset/renesas,rst.txt new file mode 100644
index 0000000000000000..488c72e1ee849cd3

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +3 -0

@@ -0,0 +1,35 @@
+DT bindings for the Renesas R-Car Reset Controller
+
+The R-Car Reset Controller provides reset control, and implements the

following

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +6 -0

+functions:
+  - Latching of the levels on mode pins when PRESET# is negated,
+  - Mode monitoring register,
+  - Reset control of peripheral devices (on R-Car Gen1),
+  - Watchdog timer (on R-Car Gen1).
+  - Register-based reset control and boot address registers for the various

CPU

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +7 -0

+    cores (on R-Car Gen2/Gen3),
+
+
+Required properties:
+  - compatible: Should be
+		  - "renesas,<soctype>-reset-wdt" for R-Car Gen1,
+		  - "renesas,<soctype>-rst" for R-Car Gen2/Gen3.

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +9 -0

+  - reg: Address start and address range for the device.
+
+
+Example:
+
+	rst: reset-controller@e6160000 {
+		compatible = "renesas,r8a7795-rst";
+		reg = <0 0xe6160000 0 0x0200>;
+	};

Quoted 26 lines

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +3 -0

@@ -0,0 +1,35 @@
+DT bindings for the Renesas R-Car Reset Controller
+
+The R-Car Reset Controller provides reset control, and implements the

following

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +6 -0

+functions:
+  - Latching of the levels on mode pins when PRESET# is negated,
+  - Mode monitoring register,
+  - Reset control of peripheral devices (on R-Car Gen1),
+  - Watchdog timer (on R-Car Gen1).
+  - Register-based reset control and boot address registers for the various

CPU

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +12 -0

+    cores (on R-Car Gen2/Gen3),
+
+
+Required properties:
+  - compatible: Should be
+               - "renesas,<soctype>-reset-wdt" for R-Car Gen1,
+               - "renesas,<soctype>-rst" for R-Car Gen2/Gen3.
+             Examples with soctypes are:
+               - "renesas,r8a7778-reset-wdt" (R-Car M1A)
+               - "renesas,r8a7779-reset-wdt" (R-Car H1)
+               - "renesas,r8a7790-rst" (R-Car H2)
+               - "renesas,r8a7791-rst" (R-Car M2-W)

Documentation/devicetree/bindings/reset/renesas,rst.txt line 26

Quoted 5 lines

Documentation/devicetree/bindings/reset/renesas,rst.txt new file +5 -0

+               - "renesas,r8a7792-rst" (R-Car V2H
+               - "renesas,r8a7793-rst" (R-Car M2-N)
+               - "renesas,r8a7794-rst" (R-Car E2)
+               - "renesas,r8a7795-rst" (R-Car H3)
+               - "renesas,r8a7796-rst" (R-Car M3-W)

Any specific reason for such a large indentation ? (I know this is really nitpicking)

Apart from that, it's just a bit of a shame we can't have generic compatible strings, but that would require additional DT properties to describe the reset controller features, and I assume that's not a path we want to take.

Acked-by Laurent Pinchart <[email protected]>

Add a driver for the Renesas R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST module.

For now this driver just provides an API to obtain the state of the mode pins, as latched at reset time. As this is typically called from the probe function of a clock driver, which can run much earlier than any initcall, calling rcar_rst_read_mode_pins() may force an early initialization of the driver.

Despite the current simple and almost identical handling for all supported SoCs, the driver matches against SoC-specific compatible values only, as the features provided by the hardware module differ a lot across the various SoC families and members.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/soc/renesas/Makefile         |  5 ++
 drivers/soc/renesas/rcar-rst.c       | 94 ++++++++++++++++++++++++++++++++++++
 include/linux/soc/renesas/rcar-rst.h |  6 +++
 3 files changed, 105 insertions(+)
 create mode 100644 drivers/soc/renesas/rcar-rst.c
 create mode 100644 include/linux/soc/renesas/rcar-rst.h

drivers/soc/renesas/Makefile +5 -0

index f32a001c65c3ab44..0d732ff893dd8fba 100644
@@ -1,3 +1,8 @@
+obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_R8A7796)	+= rcar-rst.o
+
 obj-$(CONFIG_ARCH_R8A7779)	+= rcar-sysc.o r8a7779-sysc.o
 obj-$(CONFIG_ARCH_R8A7790)	+= rcar-sysc.o r8a7790-sysc.o
 obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o

drivers/soc/renesas/rcar-rst.c new file +72 -0

new file mode 100644
index 0000000000000000..1997c348c0853254
@@ -0,0 +1,94 @@
+/*
+ * R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST Driver
+ *
+ * Copyright (C) 2016 Glider bvba
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/soc/renesas/rcar-rst.h>
+
+struct rst_config {
+	unsigned int modemr;	/* Mode Monitoring Register Offset */
+};
+
+static const struct rst_config rcar_rst_gen1 __initconst = {
+	.modemr = 0x20,
+};
+
+static const struct rst_config rcar_rst_gen2 __initconst = {
+	.modemr = 0x60,
+};
+
+static const struct rst_config rcar_rst_gen3 __initconst = {
+	.modemr = 0x60,
+};
+
+static const struct of_device_id rcar_rst_matches[] __initconst = {
+	{ .compatible = "renesas,r8a7778-reset-wdt", .data = &rcar_rst_gen1 },
+	{ .compatible = "renesas,r8a7779-reset-wdt", .data = &rcar_rst_gen1 },
+	{ .compatible = "renesas,r8a7790-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7791-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7792-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7793-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7794-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7795-rst", .data = &rcar_rst_gen3 },
+	{ .compatible = "renesas,r8a7796-rst", .data = &rcar_rst_gen3 },
+	{ /* sentinel */ }
+};
+
+static void __iomem *rcar_rst_base __initdata;
+static u32 saved_mode __initdata;
+
+static int __init rcar_rst_init(void)
+{
+	const struct of_device_id *match;
+	const struct rst_config *cfg;
+	struct device_node *np;
+	void __iomem *base;
+	int error = 0;
+
+	if (rcar_rst_base)
+		return 0;
+
+	np = of_find_matching_node_and_match(NULL, rcar_rst_matches, &match);
+	if (!np)
+		return -ENODEV;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("%s: Cannot map regs\n", np->full_name);
+		error = -ENOMEM;
+		goto out_put;
+	}
+
+	rcar_rst_base = base;
+	cfg = match->data;
+	saved_mode = ioread32(base + cfg->modemr);

Quoted 1 line

+int __init rcar_rst_read_mode_pins(u32 *mode)

Just a style issue: Is the string 'pins' in the function name still relevant? I.e. what's about just 'rcar_rst_read_mode()'?

Hi Dirk,

Quoted 4 lines · Dirk Behme

Quoted 1 line

+int __init rcar_rst_read_mode_pins(u32 *mode)

Just a style issue: Is the string 'pins' in the function name still relevant? I.e. what's about just 'rcar_rst_read_mode()'?

I feel "mode" is a too generic word for a public API.
It's used a several contexts inside the RST module (secure mode, 64-bit addressing mode, free-running mode, step-up mode).

Hi Dirk,

Quoted 4 lines · Dirk Behme

Quoted 1 line

+int __init rcar_rst_read_mode_pins(u32 *mode)

Just a style issue: Is the string 'pins' in the function name still relevant? I.e. what's about just 'rcar_rst_read_mode()'?

I feel "mode" is a too generic word for a public API.
It's used a several contexts inside the RST module (secure mode, 64-bit addressing mode, free-running mode, step-up mode).

Add a driver for the Renesas R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST module.

For now this driver just provides an API to obtain the state of the mode pins, as latched at reset time. As this is typically called from the probe function of a clock driver, which can run much earlier than any initcall, calling rcar_rst_read_mode_pins() may force an early initialization of the driver.

Despite the current simple and almost identical handling for all supported SoCs, the driver matches against SoC-specific compatible values only, as the features provided by the hardware module differ a lot across the various SoC families and members.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/soc/renesas/Makefile         |  5 ++
 drivers/soc/renesas/rcar-rst.c       | 94 +++++++++++++++++++++++++++++++++
 include/linux/soc/renesas/rcar-rst.h |  6 +++
 3 files changed, 105 insertions(+)
 create mode 100644 drivers/soc/renesas/rcar-rst.c
 create mode 100644 include/linux/soc/renesas/rcar-rst.h

drivers/soc/renesas/Makefile +5 -0

index f32a001c65c3ab44..0d732ff893dd8fba 100644
@@ -1,3 +1,8 @@
+obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
+obj-$(CONFIG_ARCH_R8A7796)	+= rcar-rst.o
+
 obj-$(CONFIG_ARCH_R8A7779)	+= rcar-sysc.o r8a7779-sysc.o
 obj-$(CONFIG_ARCH_R8A7790)	+= rcar-sysc.o r8a7790-sysc.o
 obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o

drivers/soc/renesas/rcar-rst.c new file +6 -0

new file mode 100644
index 0000000000000000..1997c348c0853254
@@ -0,0 +1,94 @@
+/*
+ * R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST Driver
+ *
+ * Copyright (C) 2016 Glider bvba
+ *
+ * This file is subject to the terms and conditions of the GNU General

Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */

drivers/soc/renesas/rcar-rst.c new file +61 -0

+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/soc/renesas/rcar-rst.h>
+
+struct rst_config {
+	unsigned int modemr;	/* Mode Monitoring Register Offset */
+};
+
+static const struct rst_config rcar_rst_gen1 __initconst = {
+	.modemr = 0x20,
+};
+
+static const struct rst_config rcar_rst_gen2 __initconst = {
+	.modemr = 0x60,
+};
+
+static const struct rst_config rcar_rst_gen3 __initconst = {
+	.modemr = 0x60,
+};
+
+static const struct of_device_id rcar_rst_matches[] __initconst = {
+	{ .compatible = "renesas,r8a7778-reset-wdt", .data = &rcar_rst_gen1 },
+	{ .compatible = "renesas,r8a7779-reset-wdt", .data = &rcar_rst_gen1 },
+	{ .compatible = "renesas,r8a7790-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7791-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7792-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7793-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7794-rst", .data = &rcar_rst_gen2 },
+	{ .compatible = "renesas,r8a7795-rst", .data = &rcar_rst_gen3 },
+	{ .compatible = "renesas,r8a7796-rst", .data = &rcar_rst_gen3 },
+	{ /* sentinel */ }
+};
+
+static void __iomem *rcar_rst_base __initdata;
+static u32 saved_mode __initdata;
+
+static int __init rcar_rst_init(void)
+{
+	const struct of_device_id *match;
+	const struct rst_config *cfg;
+	struct device_node *np;
+	void __iomem *base;
+	int error = 0;
+
+	if (rcar_rst_base)
+		return 0;
+
+	np = of_find_matching_node_and_match(NULL, rcar_rst_matches, &match);
+	if (!np)
+		return -ENODEV;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("%s: Cannot map regs\n", np->full_name);
+		error = -ENOMEM;
+		goto out_put;
+	}
+
+	rcar_rst_base = base;

drivers/soc/renesas/rcar-rst.c new file +13 -0

+int __init rcar_rst_read_mode_pins(u32 *mode)
+{
+	int error;
+
+	if (!rcar_rst_base) {
+		error = rcar_rst_init();
+		if (error)
+			return error;
+	}
+
+	*mode = saved_mode;
+	return 0;
+}

(quoted diff) +0 -0

b/include/linux/soc/renesas/rcar-rst.h new file mode 100644 index 0000000000000000..a18e0783946b66ec

include/linux/soc/renesas/rcar-rst.h new file +6 -0

@@ -0,0 +1,6 @@
+#ifndef __LINUX_SOC_RENESAS_RCAR_RST_H__
+#define __LINUX_SOC_RENESAS_RCAR_RST_H__
+
+int rcar_rst_read_mode_pins(u32 *mode);
+
+#endif /* __LINUX_SOC_RENESAS_RCAR_RST_H__ */

Quoted 5 lines

drivers/soc/renesas/rcar-rst.c new file +2 -0

 
+static int __init rcar_rst_init(void)
+{

drivers/soc/renesas/rcar-rst.c

Quoted 2 lines

drivers/soc/renesas/rcar-rst.c new file +2 -0

+}
+arch_initcall(rcar_rst_init);

Given that rcar_rst_init() is only used as a support function for rcar_rst_read_mode_pins(), how about removing the initcall ?

All R-Car Gen3 clock drivers now obtain the values of the mode pins from the R-Car RST driver.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/clk/renesas/rcar-gen3-cpg.c | 17 -----------------
 drivers/clk/renesas/rcar-gen3-cpg.h |  1 -
 2 files changed, 18 deletions(-)

(quoted diff) +0 -0

b/drivers/clk/renesas/rcar-gen3-cpg.c index
bb4f2f9a8c2f5ba8..9d76076da4948fc4 100644

drivers/clk/renesas/rcar-gen3-cpg.c +0 -17

@@ -333,23 +333,6 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct
 device *dev, __clk_get_name(parent), 0, mult, div);
 }
 
-/*
- * Reset register definitions.
- */
-#define MODEMR	0xe6160060
-
-u32 __init rcar_gen3_read_mode_pins(void)
-{
-	void __iomem *modemr = ioremap_nocache(MODEMR, 4);
-	u32 mode;
-
-	BUG_ON(!modemr);
-	mode = ioread32(modemr);
-	iounmap(modemr);
-
-	return mode;
-}
-
 int __init rcar_gen3_cpg_init(const struct rcar_gen3_cpg_pll_config
 *config, unsigned int clk_extalr)
 {

(quoted diff) +0 -0

b/drivers/clk/renesas/rcar-gen3-cpg.h index
f699085147d1aece..f788f481dd42cdf6 100644

drivers/clk/renesas/rcar-gen3-cpg.h +0 -1

@@ -33,7 +33,6 @@ struct rcar_gen3_cpg_pll_config {
 
 #define CPG_RCKCR	0x240
 
-u32 rcar_gen3_read_mode_pins(void);
 struct clk *rcar_gen3_cpg_clk_register(struct device *dev,
 	const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
 	struct clk **clks, void __iomem *base);

Obtain the values of the mode pins from the R-Car RST driver, which relies on the presence in DT of a device node for the RST module.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/clk/renesas/r8a7795-cpg-mssr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/clk/renesas/r8a7795-cpg-mssr.c index
e53aff5b23ad4967..e1f2359a59feee7e 100644

drivers/clk/renesas/r8a7795-cpg-mssr.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/soc/renesas/rcar-rst.h>
 
 #include <dt-bindings/clock/r8a7795-cpg-mssr.h>
 

@@ -294,7 +295,12 @@ static const struct rcar_gen3_cpg_pll_config

cpg_pll_configs[16] __initconst = { static int __init r8a7795_cpg_mssr_init(struct device *dev)
{

drivers/clk/renesas/r8a7795-cpg-mssr.c +6 -1

 	const struct rcar_gen3_cpg_pll_config *cpg_pll_config;
-	u32 cpg_mode = rcar_gen3_read_mode_pins();
+	u32 cpg_mode;
+	int error;
+
+	error = rcar_rst_read_mode_pins(&cpg_mode);
+	if (error)
+		return error;
 
 	cpg_pll_config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
 	if (!cpg_pll_config->extal_div) {

Obtain the values of the mode pins from the R-Car RST driver, which relies on the presence in DT of a device node for the RST module.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/clk/renesas/r8a7796-cpg-mssr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/clk/renesas/r8a7796-cpg-mssr.c index
c84b549c14d2e57d..3db2d1cfc8cd788e 100644

drivers/clk/renesas/r8a7796-cpg-mssr.c +1 -0

@@ -16,6 +16,7 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/soc/renesas/rcar-rst.h>
 
 #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
 

@@ -159,7 +160,12 @@ static const struct rcar_gen3_cpg_pll_config

cpg_pll_configs[16] __initconst = { static int __init r8a7796_cpg_mssr_init(struct device *dev)
{

drivers/clk/renesas/r8a7796-cpg-mssr.c +6 -1

 	const struct rcar_gen3_cpg_pll_config *cpg_pll_config;
-	u32 cpg_mode = rcar_gen3_read_mode_pins();
+	u32 cpg_mode;
+	int error;
+
+	error = rcar_rst_read_mode_pins(&cpg_mode);
+	if (error)
+		return error;
 
 	cpg_pll_config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
 	if (!cpg_pll_config->extal_div) {

The R-Car M1A board code no longer calls r8a7778_clocks_init().

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/clk/renesas/clk-r8a7778.c | 13 -------------
 include/linux/clk/renesas.h       |  1 -
 2 files changed, 14 deletions(-)

drivers/clk/renesas/clk-r8a7778.c +0 -3

index 07ea411098a75ad1..886a8380e91247a1 100644
@@ -143,16 +143,3 @@ static void __init r8a7778_cpg_clocks_init(struct device_node *np)
 
 CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
 	       r8a7778_cpg_clocks_init);
-
-void __init r8a7778_clocks_init(u32 mode)
-{

Quoted 24 lines · Geert Uytterhoeven

The R-Car M1A board code no longer calls r8a7778_clocks_init().

Signed-off-by Geert Uytterhoeven <[email protected]>

---
v3:
- New.

---
 drivers/clk/renesas/clk-r8a7778.c | 13 -------------
 include/linux/clk/renesas.h       |  1 -
 2 files changed, 14 deletions(-)

drivers/clk/renesas/clk-r8a7778.c +0 -3

index 07ea411098a75ad1..886a8380e91247a1 100644
@@ -143,16 +143,3 @@ static void __init r8a7778_cpg_clocks_init(struct
 device_node *np)
 
 CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
               r8a7778_cpg_clocks_init);
-
-void __init r8a7778_clocks_init(u32 mode)
-{

drivers/clk/renesas/clk-r8a7778.c static void __init r8a7778_cpg_clocks_init(struct · line 158

Quoted 10 lines

drivers/clk/renesas/clk-r8a7778.c +0 -10

-       BUG_ON(!(mode & BIT(19)));
-
-       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
-                        (!!(mode & BIT(12)) << 1) |
-                        (!!(mode & BIT(11)));
-       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
-                       (!!(mode & BIT(1)));
-
-       of_clk_init(NULL);
-}

Just a question on how you structured the patches: Is there a special reason why you do the adding of new code and the removal of dead code in two patches?

Having both in one patch normally makes it more obvious which old code is replaced by which new code.

An example: In patch

[PATCH/RFC v3 11/22] clk: renesas: r8a7778: Obtain mode pin values using R-Car RST driver

I wondered where the section

(quoted diff) +7 -0

+       BUG_ON(!(mode & BIT(19)));
+
+       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
+                        (!!(mode & BIT(12)) << 1) |
+                        (!!(mode & BIT(11)));
+       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
+                       (!!(mode & BIT(1)));

comes from. This becomes obvious with this patch 20/22. But it's 9 patches later ;) So why don't make the whole replacement visible in one patch?

Hi all,

Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the state of the mode pins either by a call from the board code, or directly by using a hardcoded register access. This is a bit messy, and creates a dependency between driver and platform code.

This RFC patch series converts the various Renesas R-Car clock drivers and support code from reading the mode pin states using a hardcoded register access to using a new R-Car RST driver.

This series consists of 5 parts:
A. Patches 1 and 2 add DT bindings and driver code for the R-Car RST
driver,
B. Patches 3-10 add device nodes for the RST modules to the R-Car DTS
files,
C. Patches 11-16 convert the clock drivers to call into the new R-Car
RST driver,
D. Patches 17-19 remove passing mode pin state to the clock drivers
from the board code,
E. Patches 20-22 remove dead code from the clock drivers.

I've kept all parts together as this is an RFC, and to avoid losing the bigger picture. I will split it in series per maintainer, when we have decided how to handle the dependencies (see below).

After this, there are still a few users of rcar_gen2_read_mode_pins() left in board code, which should be converted in one way or another. However, all R-Car clock drivers will rely on the presence in DT of a device node for the RST module. Backwards compatibility with old DTBs is retained only for R-Car Gen2, which has fallback code calling into rcar_gen2_read_mode_pins().

As is usually the case with moving functionality from board code to DT, there are lots of hard dependencies:
- The DT updates in Part B can be merged as soon as the DT bindings in Part A have been approved,
- The clock driver updates in Part C depend functionally on the driver code in Part A,
- The board code cleanups in Part D depend on the clock driver updates in Part C,
- The block driver cleanups in part E depend on the board code cleanups in part D.

This series has evolved over time, cfr.
- "[PATCH/RFC 00/11] ARM: shmobile: Let CPG use syscon for MD pin

    values" (http://www.spinics.net/lists/linux-clk/msg01478.html),
  - "[PATCH 00/10] arm64: renesas: Obtain MD pin values using
    syscon/regmap".
    (http://www.spinics.net/lists/linux-sh/msg44757.html)

Changes compared to v2:
- Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
- Drop "syscon" compatible value and "renesas,modemr" property, use a real driver instead,
- Add support for R-Car M1A, H1, and M3-W.

Changes compared to v1:
- Add support for R-Car H3.

An alternative and broader solution was proposed by Simon Horman as "[PATCH/RFC 0/6] boot-mode-reg: Add core"
(http://www.spinics.net/lists/linux-sh/msg45969.html).

This patch series is against renesas-drivers.
It has been tested on r8a7778/bockw, r8a7779/marzen, r8a7791/koelsch, and r8a7795/salvator-x.

Thanks for your comments!

Geert Uytterhoeven (22):
reset: Add renesas,rst DT bindings
soc: renesas: Add R-Car RST driver
ARM: dts: r8a7778: Add device node for RESET/WDT module
ARM: dts: r8a7779: Add device node for RESET/WDT module
ARM: dts: r8a7790: Add device node for RST module
ARM: dts: r8a7791: Add device node for RST module
ARM: dts: r8a7793: Add device node for RST module
ARM: dts: r8a7794: Add device node for RST module
arm64: renesas: r8a7795 dtsi: Add device node for RST module
arm64: renesas: r8a7796 dtsi: Add device node for RST module
clk: renesas: r8a7778: Obtain mode pin values using R-Car RST driver
clk: renesas: r8a7779: Obtain mode pin values from R-Car RST driver
clk: renesas: rcar-gen2: Obtain mode pin values using RST driver
clk: renesas: r8a7795: Obtain mode pin values from R-Car RST driver
clk: renesas: r8a7796: Obtain mode pin values from R-Car RST driver
clk: renesas: rcar-gen3-cpg: Remove obsolete

    rcar_gen3_read_mode_pins()

ARM: shmobile: r8a7778: Stop passing mode pins state to clock driver
ARM: shmobile: r8a7779: Stop passing mode pins state to clock driver
ARM: shmobile: rcar-gen2: Stop passing mode pins state to clock driver
clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()
clk: renesas: r8a7779: Remove obsolete r8a7779_clocks_init()
clk: renesas: rcar-gen2: Remove obsolete rcar_gen2_clocks_init()

 .../devicetree/bindings/reset/renesas,rst.txt      | 35 ++++++++
 arch/arm/boot/dts/r8a7778.dtsi                     |  5 ++
 arch/arm/boot/dts/r8a7779.dtsi                     |  5 ++
 arch/arm/boot/dts/r8a7790.dtsi                     |  5 ++
 arch/arm/boot/dts/r8a7791.dtsi                     |  5 ++
 arch/arm/boot/dts/r8a7793.dtsi                     |  5 ++
 arch/arm/boot/dts/r8a7794.dtsi                     |  5 ++
 arch/arm/mach-shmobile/setup-r8a7778.c             | 15 ----
 arch/arm/mach-shmobile/setup-r8a7779.c             | 27 -------
 arch/arm/mach-shmobile/setup-rcar-gen2.c           |  6 +-
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  5 ++
 arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  5 ++
 drivers/clk/renesas/clk-r8a7778.c                  | 26 +++---
 drivers/clk/renesas/clk-r8a7779.c                  | 18 ++---
 drivers/clk/renesas/clk-rcar-gen2.c                | 17 ++--
 drivers/clk/renesas/r8a7795-cpg-mssr.c             |  8 +-
 drivers/clk/renesas/r8a7796-cpg-mssr.c             |  8 +-
 drivers/clk/renesas/rcar-gen3-cpg.c                | 17 ----
 drivers/clk/renesas/rcar-gen3-cpg.h                |  1 -

 drivers/soc/renesas/Makefile                       |  5 ++
 drivers/soc/renesas/rcar-rst.c                     | 94 ++++++++++++++++++++++
 include/linux/clk/renesas.h                        |  4 -
 include/linux/soc/renesas/rcar-rst.h               |  6 ++
 23 files changed, 227 insertions(+), 100 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt
 create mode 100644 drivers/soc/renesas/rcar-rst.c
 create mode 100644 include/linux/soc/renesas/rcar-rst.h

Hi all,

Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the state of the mode pins either by a call from the board code, or directly by using a hardcoded register access. This is a bit messy, and creates a dependency between driver and platform code.

This RFC patch series converts the various Renesas R-Car clock drivers and support code from reading the mode pin states using a hardcoded register access to using a new R-Car RST driver.

Quoted 5 lines · Geert Uytterhoeven

Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the state of the mode pins either by a call from the board code, or directly by using a hardcoded register access. This is a bit messy, and creates a dependency between driver and platform code.

Quoted 3 lines

This RFC patch series converts the various Renesas R-Car clock drivers and support code from reading the mode pin states using a hardcoded register access to using a new R-Car RST driver.

Dumb question, can we use the nvmem reading APIs instead of an SoC specific function to read the modes?

Hi Stephen,

Quoted 6 lines · Stephen Boyd

Quoted 5 lines · Geert Uytterhoeven

Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the state of the mode pins either by a call from the board code, or directly by using a hardcoded register access. This is a bit messy, and creates a dependency between driver and platform code.

Quoted 6 lines

Quoted 3 lines

This RFC patch series converts the various Renesas R-Car clock drivers and support code from reading the mode pin states using a hardcoded register access to using a new R-Car RST driver.

Dumb question, can we use the nvmem reading APIs instead of an SoC specific function to read the modes?

Thanks for your suggestion, the nvmem API indeed looks like a suitable API, as it does support read-only nvmems.

Unfortunately I also see a few disadvantages:
1. nvmem_init() is a subsys_initcall(), while most of our users (except for the recent renesas-cpg-mssr driver) are clock drivers using CLK_OF_DECLARE(), and are thus initialized from of_clk_init() at much earlier time_init() time.
Of course the mvmem subsystem and/or the clock drivers can be changed, if deemed useful.

2. Using the nvmem DT bindings means we have to add more DT glue from the nvmem consumer(s) to the nvmem provider. As we need to provide backwards compatibility with old DTSes, this means we need more C code or DT fixup code to handle that.

3. The nvmem subsystem may be overkill to provide access to a simple 32-bit read-only register that never changes value after boot.

Quoted 17 lines · Geert Uytterhoeven

Quoted 6 lines · Stephen Boyd

Quoted 5 lines · Geert Uytterhoeven

Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the state of the mode pins either by a call from the board code, or directly by using a hardcoded register access. This is a bit messy, and creates a dependency between driver and platform code.

Quoted 6 lines

Quoted 3 lines

This RFC patch series converts the various Renesas R-Car clock drivers and support code from reading the mode pin states using a hardcoded register access to using a new R-Car RST driver.

Dumb question, can we use the nvmem reading APIs instead of an SoC specific function to read the modes?

Thanks for your suggestion, the nvmem API indeed looks like a suitable API, as it does support read-only nvmems.

Quoted 7 lines

Unfortunately I also see a few disadvantages:
1. nvmem_init() is a subsys_initcall(), while most of our users (except for the recent renesas-cpg-mssr driver) are clock drivers using CLK_OF_DECLARE(), and are thus initialized from of_clk_init() at much earlier time_init() time.
Of course the mvmem subsystem and/or the clock drivers can be changed, if deemed useful.

Sounds like this is solvable.

Quoted 4 lines

2. Using the nvmem DT bindings means we have to add more DT glue from the nvmem consumer(s) to the nvmem provider. As we need to provide backwards compatibility with old DTSes, this means we need more C code or DT fixup code to handle that.

Ah I wasn't aware we were keeping backwards compatibility around.

Quoted 2 lines

3. The nvmem subsystem may be overkill to provide access to a simple 32-bit read-only register that never changes value after boot.

The nvmem subsystem is designed to read values from things that mostly never change. Overkill may be true, but the nice thing about using nvmem APIs is that the driver doesn't have to use some platform specific function that duplicates similar functionality. It's unfortunate that backwards incompatibility limits our ability to move to common linux frameworks when they are created after the binding is used.