[RFC 0/4] Add WDT driver for RZ/G2L

[RFC 0/4] Add WDT driver for RZ/G2L

[Biju Das]

This patch series aims to add WDT driver support for RZ/G2L SoC's.

WDT has 3 channels
1) CH0 to check the operation of Cortex-A55-CPU Core0
2) CH1 to check the operation of Cortex-A55-CPU Core1
3) CH2 to check the operation of Cortex-M33 CPU

WDT IP supports 
1) Normal Watchdog Timer Function
2) Reset Request Function due to CPU Parity Error

Once the software activates the watchdog timer, the watchdog timer does
not stop until it is reset.

The WDT Overflow System Reset Register (CPG_WDTOVF_RST) and 
WDT Reset Selector Register (CPG_WDTRST_SEL) are in CPG IP
block.

Current driver support is basic normal Watchdog Timer Function.

Later will extend support to identify the WDT channel that generated
the reset request using CPG_WDTOVF_RST. Need to figure out how to expose
this to WDT driver from CPG driver?

and also Reset Request Function due to CPU Parity Error.

Tested WDT driver with selftests tool and reboot command

All 3 channels tested with below command.

cat /dev/watchdog  & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800808; done
cat /dev/watchdog1  & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800c08; done
cat /dev/watchdog2 & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800408; done

Please share your valuable comments.

Biju Das (4):
  clk: renesas: rzg2l: Add support for watchdog reset selection
  dt-bindings: watchdog: renesas,wdt: Add support for RZ/G2L
  clk: renesas: r9a07g044: Add WDT clock and reset entries
  watchdog: Add Watchdog Timer driver for RZ/G2L

 .../bindings/watchdog/renesas,wdt.yaml        |  72 +++--
 drivers/clk/renesas/r9a07g044-cpg.c           |  37 +++
 drivers/clk/renesas/rzg2l-cpg.c               |   6 +
 drivers/clk/renesas/rzg2l-cpg.h               |  14 +
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/rzg2l_wdt.c                  | 281 ++++++++++++++++++
 7 files changed, 401 insertions(+), 18 deletions(-)
 create mode 100644 drivers/watchdog/rzg2l_wdt.c

-- 
2.17.1

[RFC 1/4] clk: renesas: rzg2l: Add support for watchdog reset selection

[Biju Das]

This patch adds support for watchdog reset selection.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 22 ++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.c     |  6 ++++++
 drivers/clk/renesas/rzg2l-cpg.h     | 14 ++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 47c16265fca9..8618b0f19d7a 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -8,6 +8,7 @@
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 
 #include <dt-bindings/clock/r9a07g044-cpg.h>
@@ -271,7 +272,28 @@ static const unsigned int r9a07g044_crit_mod_clks[] __initconst = {
 	MOD_CLK_BASE + R9A07G044_DMAC_ACLK,
 };
 
+#define CPG_WDTRST_SEL			0xb14
+#define CPG_WDTRST_SEL_WDTRSTSEL(n)	BIT(n)
+
+#define CPG_WDTRST_SEL_WDTRST	(CPG_WDTRST_SEL_WDTRSTSEL(0) | \
+				 CPG_WDTRST_SEL_WDTRSTSEL(1) | \
+				 CPG_WDTRST_SEL_WDTRSTSEL(2))
+
+int r9a07g044_wdt_rst_setect(void __iomem *base)
+{
+	writel((CPG_WDTRST_SEL_WDTRST << 16) | CPG_WDTRST_SEL_WDTRST,
+	       base + CPG_WDTRST_SEL);
+
+	return 0;
+}
+
+static const struct rzg2l_cpg_soc_operations r9a07g044_cpg_ops = {
+	.wdt_rst_setect = r9a07g044_wdt_rst_setect,
+};
+
 const struct rzg2l_cpg_info r9a07g044_cpg_info = {
+	.ops = &r9a07g044_cpg_ops,
+
 	/* Core Clocks */
 	.core_clks = r9a07g044_core_clks,
 	.num_core_clks = ARRAY_SIZE(r9a07g044_core_clks),
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index a77cb47b75e7..f9dfee14a33e 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -932,6 +932,12 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
+	if (info->ops && info->ops->wdt_rst_setect) {
+		error = info->ops->wdt_rst_setect(priv->base);
+		if (error)
+			return error;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 484c7cee2629..e1b1497002ed 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -156,9 +156,20 @@ struct rzg2l_reset {
 		.bit = (_bit) \
 	}
 
+/**
+ * struct rzg2l_cpg_soc_operations - SoC-specific CPG Operations
+ *
+ * @wdt_rst_setect: WDT reset selection
+ */
+struct rzg2l_cpg_soc_operations {
+	int (*wdt_rst_setect)(void __iomem *base); /* Platform specific WDT reset selection */
+};
+
 /**
  * struct rzg2l_cpg_info - SoC-specific CPG Description
  *
+ * @ops: SoC-specific CPG Operations
+ *
  * @core_clks: Array of Core Clock definitions
  * @num_core_clks: Number of entries in core_clks[]
  * @last_dt_core_clk: ID of the last Core Clock exported to DT
@@ -176,6 +187,9 @@ struct rzg2l_reset {
  * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
  */
 struct rzg2l_cpg_info {
+	/* CPG Operations */
+	const struct rzg2l_cpg_soc_operations *ops;
+
 	/* Core Clocks */
 	const struct cpg_core_clk *core_clks;
 	unsigned int num_core_clks;
-- 
2.17.1

[RFC 2/4] dt-bindings: watchdog: renesas,wdt: Add support for RZ/G2L

[Biju Das]

Describe the WDT hardware in the RZ/G2L series.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../bindings/watchdog/renesas,wdt.yaml        | 72 ++++++++++++++-----
 1 file changed, 54 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
index ab66d3f0c476..f9f7f7207d6d 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
@@ -10,9 +10,6 @@ maintainers:
   - Wolfram Sang <wsa+renesas@sang-engineering.com>
   - Geert Uytterhoeven <geert+renesas@glider.be>
 
-allOf:
-  - $ref: "watchdog.yaml#"
-
 properties:
   compatible:
     oneOf:
@@ -22,6 +19,11 @@ properties:
               - renesas,r7s9210-wdt      # RZ/A2
           - const: renesas,rza-wdt       # RZ/A
 
+      - items:
+          - enum:
+              - renesas,r9a07g044-wdt    # RZ/G2{L,LC}
+          - const: renesas,rzg2l-wdt     # RZ/G2L
+
       - items:
           - enum:
               - renesas,r8a7742-wdt      # RZ/G1H
@@ -56,11 +58,13 @@ properties:
   reg:
     maxItems: 1
 
-  interrupts:
-    maxItems: 1
+  interrupts: true
 
-  clocks:
-    maxItems: 1
+  interrupt-names: true
+
+  clocks: true
+
+  clock-names: true
 
   power-domains:
     maxItems: 1
@@ -75,17 +79,49 @@ required:
   - reg
   - clocks
 
-if:
-  not:
-    properties:
-      compatible:
-        contains:
-          enum:
-            - renesas,rza-wdt
-then:
-  required:
-    - power-domains
-    - resets
+allOf:
+  - $ref: "watchdog.yaml#"
+
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - renesas,rza-wdt
+    then:
+      required:
+        - power-domains
+        - resets
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - renesas,rzg2l-wdt
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
+        interrupt-names:
+          items:
+            - const: wdt
+            - const: perrout
+        clocks:
+          items:
+            - description: Main clock
+            - description: Register access clock
+        clock-names:
+          items:
+            - const: oscclk
+            - const: pclk
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+        clocks:
+          maxItems: 1
 
 additionalProperties: false
 
-- 
2.17.1

Re: [RFC 2/4] dt-bindings: watchdog: renesas,wdt: Add support for RZ/G2L

[Geert Uytterhoeven]

Hi Biju,

On Thu, Nov 4, 2021 at 5:09 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Describe the WDT hardware in the RZ/G2L series.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml

> @@ -75,17 +79,49 @@ required:
>    - reg
>    - clocks
>
> -if:
> -  not:
> -    properties:
> -      compatible:
> -        contains:
> -          enum:
> -            - renesas,rza-wdt
> -then:
> -  required:
> -    - power-domains
> -    - resets
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - renesas,rza-wdt
> +    then:
> +      required:
> +        - power-domains
> +        - resets
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - renesas,rzg2l-wdt
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 2
> +        interrupt-names:
> +          items:
> +            - const: wdt
> +            - const: perrout
> +        clocks:
> +          items:
> +            - description: Main clock
> +            - description: Register access clock
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: pclk

Usually we put the internal module clock first.

Please add (at least the first one):

     required:
       - clock-names
       - interrupt-names

> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +        clocks:
> +          maxItems: 1
>
>  additionalProperties: false

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: [RFC 2/4] dt-bindings: watchdog: renesas,wdt: Add support for RZ/G2L

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [RFC 2/4] dt-bindings: watchdog: renesas,wdt: Add support for
> RZ/G2L
> 
> Hi Biju,
> 
> On Thu, Nov 4, 2021 at 5:09 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Describe the WDT hardware in the RZ/G2L series.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> 
> > @@ -75,17 +79,49 @@ required:
> >    - reg
> >    - clocks
> >
> > -if:
> > -  not:
> > -    properties:
> > -      compatible:
> > -        contains:
> > -          enum:
> > -            - renesas,rza-wdt
> > -then:
> > -  required:
> > -    - power-domains
> > -    - resets
> > +allOf:
> > +  - $ref: "watchdog.yaml#"
> > +
> > +  - if:
> > +      not:
> > +        properties:
> > +          compatible:
> > +            contains:
> > +              enum:
> > +                - renesas,rza-wdt
> > +    then:
> > +      required:
> > +        - power-domains
> > +        - resets
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - renesas,rzg2l-wdt
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 2
> > +        interrupt-names:
> > +          items:
> > +            - const: wdt
> > +            - const: perrout
> > +        clocks:
> > +          items:
> > +            - description: Main clock
> > +            - description: Register access clock
> > +        clock-names:
> > +          items:
> > +            - const: oscclk
> > +            - const: pclk
> 
> Usually we put the internal module clock first.
OK. Will put internal module clock first.
> 
> Please add (at least the first one):
> 
>      required:
>        - clock-names
>        - interrupt-names

Ok, will add the same.

Regards,
Biju

> 
> > +    else:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
> > +        clocks:
> > +          maxItems: 1
> >
> >  additionalProperties: false
> 
> 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

[RFC 3/4] clk: renesas: r9a07g044: Add WDT clock and reset entries

[Biju Das]

Add WDT{0,1,2} clock and reset entries to CPG driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 8618b0f19d7a..79bad86b6cb7 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -146,6 +146,18 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
 				0x52c, 0),
 	DEF_MOD("dmac_pclk",	R9A07G044_DMAC_PCLK, CLK_P1_DIV2,
 				0x52c, 1),
+	DEF_MOD("wdt0_pclk",	R9A07G044_WDT0_PCLK, R9A07G044_CLK_P0,
+				0x548, 0),
+	DEF_MOD("wdt0_clk",	R9A07G044_WDT0_CLK, R9A07G044_OSCCLK,
+				0x548, 1),
+	DEF_MOD("wdt1_pclk",	R9A07G044_WDT1_PCLK, R9A07G044_CLK_P0,
+				0x548, 2),
+	DEF_MOD("wdt1_clk",	R9A07G044_WDT1_CLK, R9A07G044_OSCCLK,
+				0x548, 3),
+	DEF_MOD("wdt2_pclk",	R9A07G044_WDT2_PCLK, R9A07G044_CLK_P0,
+				0x548, 4),
+	DEF_MOD("wdt2_clk",	R9A07G044_WDT2_CLK, R9A07G044_OSCCLK,
+				0x548, 5),
 	DEF_MOD("spi_clk2",	R9A07G044_SPI_CLK2, R9A07G044_CLK_SPI1,
 				0x550, 0),
 	DEF_MOD("spi_clk",	R9A07G044_SPI_CLK, R9A07G044_CLK_SPI0,
@@ -234,6 +246,9 @@ static struct rzg2l_reset r9a07g044_resets[] = {
 	DEF_RST(R9A07G044_IA55_RESETN, 0x818, 0),
 	DEF_RST(R9A07G044_DMAC_ARESETN, 0x82c, 0),
 	DEF_RST(R9A07G044_DMAC_RST_ASYNC, 0x82c, 1),
+	DEF_RST(R9A07G044_WDT0_PRESETN, 0x848, 0),
+	DEF_RST(R9A07G044_WDT1_PRESETN, 0x848, 1),
+	DEF_RST(R9A07G044_WDT2_PRESETN, 0x848, 2),
 	DEF_RST(R9A07G044_SPI_RST, 0x850, 0),
 	DEF_RST(R9A07G044_SDHI0_IXRST, 0x854, 0),
 	DEF_RST(R9A07G044_SDHI1_IXRST, 0x854, 1),
-- 
2.17.1

Re: [RFC 3/4] clk: renesas: r9a07g044: Add WDT clock and reset entries

[Geert Uytterhoeven]

On Thu, Nov 4, 2021 at 5:09 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add WDT{0,1,2} clock and reset entries to CPG driver.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v5.17.

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

[RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Biju Das]

Add Watchdog Timer driver for RZ/G2L SoC.

WDT IP block supports normal watchdog timer function and reset
request function due to CPU parity error.

This driver currently supports normal watchdog timer function
and later will add support for reset request function due to
CPU parity error.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/watchdog/Kconfig     |   8 +
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/rzg2l_wdt.c | 281 +++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+)
 create mode 100644 drivers/watchdog/rzg2l_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bf59faeb3de1..34da309a7afd 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -895,6 +895,14 @@ config RENESAS_RZAWDT
 	  This driver adds watchdog support for the integrated watchdogs in the
 	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
 
+config RENESAS_RZG2LWDT
+	tristate "Renesas RZ/G2L WDT Watchdog"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This driver adds watchdog support for the integrated watchdogs in the
+	  Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
 	tristate "Aspeed BMC watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 1bd2d6f37c53..e7e8ce546814 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
 obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
+obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
 obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
new file mode 100644
index 000000000000..a477f7792be8
--- /dev/null
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L WDT Watchdog Driver
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation
+ */
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/watchdog.h>
+
+#define WDTCNT		0x00
+#define WDTSET		0x04
+#define WDTTIM		0x08
+#define WDTINT		0x0C
+#define WDTCNT_WDTEN	BIT(0)
+#define WDTINT_INTDISP	BIT(0)
+
+#define WDT_DEFAULT_TIMEOUT		60U
+
+/* Setting period time register only 12 bit set in WDTSET[31:20] */
+#define WDTSET_COUNTER_MASK		(0xFFF00000)
+#define WDTSET_COUNTER_VAL(f)		((f) << 20)
+
+#define F2CYCLE_NSEC(f)			(1000000000 / (f))
+#define WDT_CYCLE_MSEC(f, wdttime)	((1024 * 1024 * (((u64)wdttime) + 1)) / \
+					 ((f) / 1000000))
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct rzg2l_wdt_priv {
+	void __iomem *base;
+	struct watchdog_device wdev;
+	struct reset_control *rstc;
+	unsigned long osc_clk_rate;
+	unsigned long pclk_rate;
+	unsigned long delay;
+};
+
+static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
+{
+	/* delay timer when change the setting register */
+	ndelay(priv->delay);
+}
+
+static int rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val, unsigned int reg)
+{
+	int i;
+
+	if (reg == WDTSET)
+		val &= WDTSET_COUNTER_MASK;
+
+	writel_relaxed(val, priv->base + reg);
+
+	for (i = 0; i < 1000; i++) {
+		if (readl_relaxed(priv->base + reg) == val)
+			return 0;
+		rzg2l_wdt_wait_delay(priv);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u32 time_out;
+
+	/* Clear Lapsed Time Register and clear Interrupt */
+	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
+	/* Delay timer before setting watchdog counter*/
+	rzg2l_wdt_wait_delay(priv);
+	/* 2 consecutive overflow cycle needed to trigger reset */
+	time_out = (wdev->timeout / 2 * 1000000) / WDT_CYCLE_MSEC(priv->osc_clk_rate, 0);
+	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(time_out), WDTSET);
+
+	return 0;
+}
+
+static int rzg2l_wdt_start(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	reset_control_deassert(priv->rstc);
+	pm_runtime_get_sync(wdev->parent);
+
+	/* Initialize time out */
+	rzg2l_wdt_init_timeout(wdev);
+
+	rzg2l_wdt_wait_delay(priv);
+	/* Initialize watchdog counter register */
+	rzg2l_wdt_write(priv, 0, WDTTIM);
+
+	/* Enable watchdog timer*/
+	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
+
+	set_bit(WDOG_HW_RUNNING, &wdev->status);
+
+	return 0;
+}
+
+static int rzg2l_wdt_stop(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	pm_runtime_put(wdev->parent);
+	reset_control_assert(priv->rstc);
+
+	return 0;
+}
+
+static int rzg2l_wdt_restart(struct watchdog_device *wdev,
+			     unsigned long action, void *data)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	/* Reset the module before we modify any register */
+	reset_control_reset(priv->rstc);
+	pm_runtime_get_sync(wdev->parent);
+
+	/* smallest counter value to reboot soon */
+	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
+
+	rzg2l_wdt_wait_delay(priv);
+	/* Enable watchdog timer*/
+	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
+
+	return 0;
+}
+
+static const struct watchdog_info rzg2l_wdt_ident = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity = "Renesas RZ/G2L WDT Watchdog",
+};
+
+static int rzg2l_wdt_ping(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
+
+	return 0;
+}
+
+static const struct watchdog_ops rzg2l_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = rzg2l_wdt_start,
+	.stop = rzg2l_wdt_stop,
+	.ping = rzg2l_wdt_ping,
+	.restart = rzg2l_wdt_restart,
+};
+
+static void rzg2l_wdt_reset_assert_clock_disable(void *data)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(data);
+
+	reset_control_assert(priv->rstc);
+}
+
+static int rzg2l_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzg2l_wdt_priv *priv;
+	struct clk *wdt_clk;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	/* Get watchdog main clock */
+	wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
+	if (IS_ERR(wdt_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
+
+	priv->osc_clk_rate = clk_get_rate(wdt_clk);
+	if (!priv->osc_clk_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
+
+	/* Get Peripheral clock */
+	wdt_clk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(wdt_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
+
+	priv->pclk_rate = clk_get_rate(wdt_clk);
+	if (!priv->pclk_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
+
+	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 + F2CYCLE_NSEC(priv->pclk_rate) * 9;
+
+	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
+			"failed to get cpg reset");
+
+	reset_control_deassert(priv->rstc);
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzg2l_wdt_reset_assert_clock_disable,
+				       &priv->wdev);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "failed to get reset");
+
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_resume_and_get failed");
+		goto out_pm_get;
+	}
+
+	priv->wdev.info = &rzg2l_wdt_ident;
+	priv->wdev.ops = &rzg2l_wdt_ops;
+	priv->wdev.parent = dev;
+	priv->wdev.min_timeout = 1;
+	priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
+	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
+
+	platform_set_drvdata(pdev, priv);
+	watchdog_set_drvdata(&priv->wdev, priv);
+	watchdog_set_nowayout(&priv->wdev, nowayout);
+	watchdog_set_restart_priority(&priv->wdev, 0);
+	watchdog_stop_on_unregister(&priv->wdev);
+
+	ret = watchdog_init_timeout(&priv->wdev, 0, dev);
+	if (ret)
+		dev_warn(dev, "Specified timeout invalid, using default");
+
+	ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
+	if (ret < 0)
+		goto out_pm_disable;
+
+	return 0;
+
+out_pm_disable:
+	pm_runtime_put(dev);
+out_pm_get:
+	pm_runtime_disable(dev);
+
+	return ret;
+}
+
+static int rzg2l_wdt_remove(struct platform_device *pdev)
+{
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id rzg2l_wdt_ids[] = {
+	{ .compatible = "renesas,rzg2l-wdt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
+
+static struct platform_driver rzg2l_wdt_driver = {
+	.driver = {
+		.name = "rzg2l_wdt",
+		.of_match_table = rzg2l_wdt_ids,
+	},
+	.probe = rzg2l_wdt_probe,
+	.remove = rzg2l_wdt_remove,
+};
+module_platform_driver(rzg2l_wdt_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/G2L WDT Watchdog Driver");
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Guenter Roeck]

On 11/4/21 9:08 AM, Biju Das wrote:
> Add Watchdog Timer driver for RZ/G2L SoC.
> 
> WDT IP block supports normal watchdog timer function and reset
> request function due to CPU parity error.
> 
> This driver currently supports normal watchdog timer function
> and later will add support for reset request function due to
> CPU parity error.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>   drivers/watchdog/Kconfig     |   8 +
>   drivers/watchdog/Makefile    |   1 +
>   drivers/watchdog/rzg2l_wdt.c | 281 +++++++++++++++++++++++++++++++++++
>   3 files changed, 290 insertions(+)
>   create mode 100644 drivers/watchdog/rzg2l_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bf59faeb3de1..34da309a7afd 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -895,6 +895,14 @@ config RENESAS_RZAWDT
>   	  This driver adds watchdog support for the integrated watchdogs in the
>   	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
>   
> +config RENESAS_RZG2LWDT
> +	tristate "Renesas RZ/G2L WDT Watchdog"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  This driver adds watchdog support for the integrated watchdogs in the
> +	  Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.
> +
>   config ASPEED_WATCHDOG
>   	tristate "Aspeed BMC watchdog support"
>   	depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 1bd2d6f37c53..e7e8ce546814 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>   obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>   obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
>   obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
> +obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
>   obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>   obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>   obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> new file mode 100644
> index 000000000000..a477f7792be8
> --- /dev/null
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L WDT Watchdog Driver
> + *
> + * Copyright (C) 2021 Renesas Electronics Corporation
> + */
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/watchdog.h>
> +
> +#define WDTCNT		0x00
> +#define WDTSET		0x04
> +#define WDTTIM		0x08
> +#define WDTINT		0x0C
> +#define WDTCNT_WDTEN	BIT(0)
> +#define WDTINT_INTDISP	BIT(0)
> +
> +#define WDT_DEFAULT_TIMEOUT		60U
> +
> +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> +#define WDTSET_COUNTER_MASK		(0xFFF00000)
> +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
> +
> +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
> +#define WDT_CYCLE_MSEC(f, wdttime)	((1024 * 1024 * (((u64)wdttime) + 1)) / \
> +					 ((f) / 1000000))
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct rzg2l_wdt_priv {
> +	void __iomem *base;
> +	struct watchdog_device wdev;
> +	struct reset_control *rstc;
> +	unsigned long osc_clk_rate;
> +	unsigned long pclk_rate;

pclk_rate is only used in the probe function and thus not needed here.

> +	unsigned long delay;
> +};
> +
> +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
> +{
> +	/* delay timer when change the setting register */
> +	ndelay(priv->delay);
> +}
> +
> +static int rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val, unsigned int reg)
> +{
> +	int i;
> +
> +	if (reg == WDTSET)
> +		val &= WDTSET_COUNTER_MASK;
> +
> +	writel_relaxed(val, priv->base + reg);
> +
> +	for (i = 0; i < 1000; i++) {
> +		if (readl_relaxed(priv->base + reg) == val)
> +			return 0;
> +		rzg2l_wdt_wait_delay(priv);
> +	}

This needs some explanation. Under which circumstances does the chip refuse
to update the register ?

> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	u32 time_out;
> +
> +	/* Clear Lapsed Time Register and clear Interrupt */
> +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> +	/* Delay timer before setting watchdog counter*/
> +	rzg2l_wdt_wait_delay(priv);
> +	/* 2 consecutive overflow cycle needed to trigger reset */
> +	time_out = (wdev->timeout / 2 * 1000000) / WDT_CYCLE_MSEC(priv->osc_clk_rate, 0);
> +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(time_out), WDTSET);
> +
> +	return 0;

rzg2l_wdt_write() returns an error which is ignored. This functions always
returns 0, ignoring any errors, but then the calling code ignores the return
value anyway. This doesn't make sense, or at least it is very inconsistent.
Either check for errors and handle them, or if errors are ignored anyway
there is no point of checking.

> +}
> +
> +static int rzg2l_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	reset_control_deassert(priv->rstc);
> +	pm_runtime_get_sync(wdev->parent);
> +
> +	/* Initialize time out */
> +	rzg2l_wdt_init_timeout(wdev);
> +
> +	rzg2l_wdt_wait_delay(priv);
> +	/* Initialize watchdog counter register */
> +	rzg2l_wdt_write(priv, 0, WDTTIM);
> +
> +	/* Enable watchdog timer*/
> +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> +
> +	set_bit(WDOG_HW_RUNNING, &wdev->status);

This is only needed if the watchdog can not be stopped, which does not
appear to be the case here.

> +
> +	return 0;
> +}
> +
> +static int rzg2l_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	pm_runtime_put(wdev->parent);
> +	reset_control_assert(priv->rstc);
> +
> +	return 0;
> +}
> +
> +static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> +			     unsigned long action, void *data)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Reset the module before we modify any register */
> +	reset_control_reset(priv->rstc);
> +	pm_runtime_get_sync(wdev->parent);
> +
> +	/* smallest counter value to reboot soon */
> +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> +
> +	rzg2l_wdt_wait_delay(priv);
> +	/* Enable watchdog timer*/
> +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info rzg2l_wdt_ident = {
> +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +	.identity = "Renesas RZ/G2L WDT Watchdog",
> +};
> +
> +static int rzg2l_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_ops rzg2l_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = rzg2l_wdt_start,
> +	.stop = rzg2l_wdt_stop,
> +	.ping = rzg2l_wdt_ping,
> +	.restart = rzg2l_wdt_restart,
> +};
> +
> +static void rzg2l_wdt_reset_assert_clock_disable(void *data)
> +{
> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(data);
> +
> +	reset_control_assert(priv->rstc);
> +}
> +
> +static int rzg2l_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rzg2l_wdt_priv *priv;
> +	struct clk *wdt_clk;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	/* Get watchdog main clock */
> +	wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> +	if (IS_ERR(wdt_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
> +
> +	priv->osc_clk_rate = clk_get_rate(wdt_clk);
> +	if (!priv->osc_clk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> +
> +	/* Get Peripheral clock */
> +	wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(wdt_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> +
> +	priv->pclk_rate = clk_get_rate(wdt_clk);
> +	if (!priv->pclk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> +
> +	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 + F2CYCLE_NSEC(priv->pclk_rate) * 9;
> +
> +	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->rstc))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> +			"failed to get cpg reset");
> +
> +	reset_control_deassert(priv->rstc);
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rzg2l_wdt_reset_assert_clock_disable,
> +				       &priv->wdev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "failed to get reset");
> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed");
> +		goto out_pm_get;
> +	}
> +
> +	priv->wdev.info = &rzg2l_wdt_ident;
> +	priv->wdev.ops = &rzg2l_wdt_ops;
> +	priv->wdev.parent = dev;
> +	priv->wdev.min_timeout = 1;
> +	priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
> +	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	platform_set_drvdata(pdev, priv);

Is this used anywhere ?

> +	watchdog_set_drvdata(&priv->wdev, priv);
> +	watchdog_set_nowayout(&priv->wdev, nowayout);
> +	watchdog_set_restart_priority(&priv->wdev, 0);

0 is the default restart priority (ie this is a reset of last resort).
It does not have to be set explicitly.

> +	watchdog_stop_on_unregister(&priv->wdev);
> +
> +	ret = watchdog_init_timeout(&priv->wdev, 0, dev);
> +	if (ret)
> +		dev_warn(dev, "Specified timeout invalid, using default");
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> +	if (ret < 0)
> +		goto out_pm_disable;
> +
> +	return 0;
> +
> +out_pm_disable:
> +	pm_runtime_put(dev);
> +out_pm_get:
> +	pm_runtime_disable(dev);
> +
> +	return ret;
> +}
> +
> +static int rzg2l_wdt_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +

This is the wrong order. The watchdog device has to be removed first,
meaning you can not use devm_watchdog_register_device(), or you would
have to use devm_add_action_or_reset() to call the runtime pm put/disable
functions.

> +	return 0;
> +}
> +
> +static const struct of_device_id rzg2l_wdt_ids[] = {
> +	{ .compatible = "renesas,rzg2l-wdt", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
> +
> +static struct platform_driver rzg2l_wdt_driver = {
> +	.driver = {
> +		.name = "rzg2l_wdt",
> +		.of_match_table = rzg2l_wdt_ids,
> +	},
> +	.probe = rzg2l_wdt_probe,
> +	.remove = rzg2l_wdt_remove,
> +};
> +module_platform_driver(rzg2l_wdt_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/G2L WDT Watchdog Driver");
> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Geert Uytterhoeven]

On Mon, Nov 8, 2021 at 7:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 11/4/21 9:08 AM, Biju Das wrote:
> > Add Watchdog Timer driver for RZ/G2L SoC.
> >
> > WDT IP block supports normal watchdog timer function and reset
> > request function due to CPU parity error.
> >
> > This driver currently supports normal watchdog timer function
> > and later will add support for reset request function due to
> > CPU parity error.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > --- /dev/null
> > +++ b/drivers/watchdog/rzg2l_wdt.c

> > +struct rzg2l_wdt_priv {
> > +     void __iomem *base;
> > +     struct watchdog_device wdev;
> > +     struct reset_control *rstc;
> > +     unsigned long osc_clk_rate;
> > +     unsigned long pclk_rate;
>
> pclk_rate is only used in the probe function and thus not needed here.

Indeed...

> > +static int rzg2l_wdt_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct rzg2l_wdt_priv *priv;
> > +     struct clk *wdt_clk;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(priv->base))
> > +             return PTR_ERR(priv->base);
> > +
> > +     /* Get watchdog main clock */
> > +     wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > +     if (IS_ERR(wdt_clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
> > +
> > +     priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > +     if (!priv->osc_clk_rate)
> > +             return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> > +
> > +     /* Get Peripheral clock */
> > +     wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > +     if (IS_ERR(wdt_clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> > +
> > +     priv->pclk_rate = clk_get_rate(wdt_clk);
> > +     if (!priv->pclk_rate)
> > +             return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");

... and this can't really happen, can it?

So you don't need to get pclk at all.  It will be controlled through
Runtime PM anyway.

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: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> On Mon, Nov 8, 2021 at 7:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > On 11/4/21 9:08 AM, Biju Das wrote:
> > > Add Watchdog Timer driver for RZ/G2L SoC.
> > >
> > > WDT IP block supports normal watchdog timer function and reset
> > > request function due to CPU parity error.
> > >
> > > This driver currently supports normal watchdog timer function and
> > > later will add support for reset request function due to CPU parity
> > > error.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > --- /dev/null
> > > +++ b/drivers/watchdog/rzg2l_wdt.c
> 
> > > +struct rzg2l_wdt_priv {
> > > +     void __iomem *base;
> > > +     struct watchdog_device wdev;
> > > +     struct reset_control *rstc;
> > > +     unsigned long osc_clk_rate;
> > > +     unsigned long pclk_rate;
> >
> > pclk_rate is only used in the probe function and thus not needed here.
> 
> Indeed...
OK.
> 
> > > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > > +     struct device *dev = &pdev->dev;
> > > +     struct rzg2l_wdt_priv *priv;
> > > +     struct clk *wdt_clk;
> > > +     int ret;
> > > +
> > > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv)
> > > +             return -ENOMEM;
> > > +
> > > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > > +     if (IS_ERR(priv->base))
> > > +             return PTR_ERR(priv->base);
> > > +
> > > +     /* Get watchdog main clock */
> > > +     wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > > +     if (IS_ERR(wdt_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > + oscclk");
> > > +
> > > +     priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > > +     if (!priv->osc_clk_rate)
> > > +             return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate
> > > + is 0");
> > > +
> > > +     /* Get Peripheral clock */
> > > +     wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > > +     if (IS_ERR(wdt_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > + pclk");
> > > +
> > > +     priv->pclk_rate = clk_get_rate(wdt_clk);
> > > +     if (!priv->pclk_rate)
> > > +             return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate
> > > + is 0");
> 
> ... and this can't really happen, can it?

But I need pclk frequency  for delay calculation. That is the reason I am doing a get. Probably after 
Getting the rate, I should do a "put". So that Run time PM will be in full control for the clocks.
Same for oscillator clk. Is it ok?

Regards,
Biju
> 
> So you don't need to get pclk at all.  It will be controlled through
> Runtime PM anyway.
> 
> 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: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Geert Uytterhoeven]

Hi Biju,

On Tue, Nov 9, 2021 at 9:22 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
> > On Mon, Nov 8, 2021 at 7:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > On 11/4/21 9:08 AM, Biju Das wrote:
> > > > Add Watchdog Timer driver for RZ/G2L SoC.
> > > >
> > > > WDT IP block supports normal watchdog timer function and reset
> > > > request function due to CPU parity error.
> > > >
> > > > This driver currently supports normal watchdog timer function and
> > > > later will add support for reset request function due to CPU parity
> > > > error.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > > > --- /dev/null
> > > > +++ b/drivers/watchdog/rzg2l_wdt.c
> >
> > > > +struct rzg2l_wdt_priv {
> > > > +     void __iomem *base;
> > > > +     struct watchdog_device wdev;
> > > > +     struct reset_control *rstc;
> > > > +     unsigned long osc_clk_rate;
> > > > +     unsigned long pclk_rate;
> > >
> > > pclk_rate is only used in the probe function and thus not needed here.
> >
> > Indeed...
> OK.
> >
> > > > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > > > +     struct device *dev = &pdev->dev;
> > > > +     struct rzg2l_wdt_priv *priv;
> > > > +     struct clk *wdt_clk;
> > > > +     int ret;
> > > > +
> > > > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > +     if (!priv)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > > > +     if (IS_ERR(priv->base))
> > > > +             return PTR_ERR(priv->base);
> > > > +
> > > > +     /* Get watchdog main clock */
> > > > +     wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > > > +     if (IS_ERR(wdt_clk))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > > + oscclk");
> > > > +
> > > > +     priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > > > +     if (!priv->osc_clk_rate)
> > > > +             return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate
> > > > + is 0");
> > > > +
> > > > +     /* Get Peripheral clock */
> > > > +     wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > > > +     if (IS_ERR(wdt_clk))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > > + pclk");
> > > > +
> > > > +     priv->pclk_rate = clk_get_rate(wdt_clk);
> > > > +     if (!priv->pclk_rate)
> > > > +             return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate
> > > > + is 0");
> >
> > ... and this can't really happen, can it?
>
> But I need pclk frequency  for delay calculation. That is the reason I am doing a get. Probably after
> Getting the rate, I should do a "put". So that Run time PM will be in full control for the clocks.
> Same for oscillator clk. Is it ok?

Oops, I missed that.  Please ignore my comment, I'll grab a coffee
soon ;-)

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: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Biju Das]

Hi Guenter Roeck,

Thanks for the feedback.

> Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> On 11/4/21 9:08 AM, Biju Das wrote:
> > Add Watchdog Timer driver for RZ/G2L SoC.
> >
> > WDT IP block supports normal watchdog timer function and reset request
> > function due to CPU parity error.
> >
> > This driver currently supports normal watchdog timer function and
> > later will add support for reset request function due to CPU parity
> > error.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >   drivers/watchdog/Kconfig     |   8 +
> >   drivers/watchdog/Makefile    |   1 +
> >   drivers/watchdog/rzg2l_wdt.c | 281 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 290 insertions(+)
> >   create mode 100644 drivers/watchdog/rzg2l_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > bf59faeb3de1..34da309a7afd 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -895,6 +895,14 @@ config RENESAS_RZAWDT
> >   	  This driver adds watchdog support for the integrated watchdogs in
> the
> >   	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
> >
> > +config RENESAS_RZG2LWDT
> > +	tristate "Renesas RZ/G2L WDT Watchdog"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	select WATCHDOG_CORE
> > +	help
> > +	  This driver adds watchdog support for the integrated watchdogs in
> the
> > +	  Renesas RZ/G2L SoCs. These watchdogs can be used to reset a
> system.
> > +
> >   config ASPEED_WATCHDOG
> >   	tristate "Aspeed BMC watchdog support"
> >   	depends on ARCH_ASPEED || COMPILE_TEST diff --git
> > a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
> > 1bd2d6f37c53..e7e8ce546814 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -85,6 +85,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
> >   obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
> >   obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> >   obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
> > +obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
> >   obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> >   obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> >   obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o diff --git
> > a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c new file
> > mode 100644 index 000000000000..a477f7792be8
> > --- /dev/null
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -0,0 +1,281 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L WDT Watchdog Driver
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corporation  */ #include
> > +<linux/bitops.h> #include <linux/clk.h> #include <linux/delay.h>
> > +#include <linux/io.h> #include <linux/kernel.h> #include
> > +<linux/module.h> #include <linux/of.h> #include
> > +<linux/platform_device.h> #include <linux/pm_runtime.h> #include
> > +<linux/reset.h> #include <linux/watchdog.h>
> > +
> > +#define WDTCNT		0x00
> > +#define WDTSET		0x04
> > +#define WDTTIM		0x08
> > +#define WDTINT		0x0C
> > +#define WDTCNT_WDTEN	BIT(0)
> > +#define WDTINT_INTDISP	BIT(0)
> > +
> > +#define WDT_DEFAULT_TIMEOUT		60U
> > +
> > +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> > +#define WDTSET_COUNTER_MASK		(0xFFF00000)
> > +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
> > +
> > +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
> > +#define WDT_CYCLE_MSEC(f, wdttime)	((1024 * 1024 * (((u64)wdttime)
> + 1)) / \
> > +					 ((f) / 1000000))
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
> > +started (default="
> > +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +struct rzg2l_wdt_priv {
> > +	void __iomem *base;
> > +	struct watchdog_device wdev;
> > +	struct reset_control *rstc;
> > +	unsigned long osc_clk_rate;
> > +	unsigned long pclk_rate;
> 
> pclk_rate is only used in the probe function and thus not needed here.

OK. It is not needed here.

 
> > +	unsigned long delay;
> > +};
> > +
> > +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) {
> > +	/* delay timer when change the setting register */
> > +	ndelay(priv->delay);
> > +}
> > +
> > +static int rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val,
> > +unsigned int reg) {
> > +	int i;
> > +
> > +	if (reg == WDTSET)
> > +		val &= WDTSET_COUNTER_MASK;
> > +
> > +	writel_relaxed(val, priv->base + reg);
> > +
> > +	for (i = 0; i < 1000; i++) {
> > +		if (readl_relaxed(priv->base + reg) == val)
> > +			return 0;
> > +		rzg2l_wdt_wait_delay(priv);
> > +	}
> 
> This needs some explanation. Under which circumstances does the chip
> refuse to update the register ?

Each register other than the WDT interrupt control register (WDTINT)
is always synchronized with WDTn_CLK (n =0, 1, 2) at regular intervals. 
The timing at which the written data is reflected in the watchdog timer
operation is 6 × P0ϕ +9 × OSCCLK).

I will make this function as void and will remove all the occurrences
of rzg2l_wdt_wait_delay.

static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val, unsigned int reg)
{
	if (reg == WDTSET)
		val &= WDTSET_COUNTER_MASK;

	writel_relaxed(val, priv->base + reg);
	/* Registers other than the WDTINT is always synchronized with WDT_CLK */
	if (reg != WDTINT)
		rzg2l_wdt_wait_delay(priv);
}

> 
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static int rzg2l_wdt_init_timeout(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u32 time_out;
> > +
> > +	/* Clear Lapsed Time Register and clear Interrupt */
> > +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> > +	/* Delay timer before setting watchdog counter*/
> > +	rzg2l_wdt_wait_delay(priv);
> > +	/* 2 consecutive overflow cycle needed to trigger reset */
> > +	time_out = (wdev->timeout / 2 * 1000000) / WDT_CYCLE_MSEC(priv-
> >osc_clk_rate, 0);
> > +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(time_out), WDTSET);
> > +
> > +	return 0;
> 
> rzg2l_wdt_write() returns an error which is ignored. This functions always
> returns 0, ignoring any errors, but then the calling code ignores the
> return value anyway. This doesn't make sense, or at least it is very
> inconsistent.
> Either check for errors and handle them, or if errors are ignored anyway
> there is no point of checking.

OK. I will make this void function as well, as rzg2l_wdt_write will be void
function as explained above.

> 
> > +}
> > +
> > +static int rzg2l_wdt_start(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	reset_control_deassert(priv->rstc);
> > +	pm_runtime_get_sync(wdev->parent);
> > +
> > +	/* Initialize time out */
> > +	rzg2l_wdt_init_timeout(wdev);
> > +
> > +	rzg2l_wdt_wait_delay(priv);
> > +	/* Initialize watchdog counter register */
> > +	rzg2l_wdt_write(priv, 0, WDTTIM);
> > +
> > +	/* Enable watchdog timer*/
> > +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +
> > +	set_bit(WDOG_HW_RUNNING, &wdev->status);
> 
> This is only needed if the watchdog can not be stopped, which does not
> appear to be the case here.

OK. I will remove this, As I can stop the watchdog by putting the module
to reset state.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_wdt_stop(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	pm_runtime_put(wdev->parent);
> > +	reset_control_assert(priv->rstc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> > +			     unsigned long action, void *data) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	/* Reset the module before we modify any register */
> > +	reset_control_reset(priv->rstc);
> > +	pm_runtime_get_sync(wdev->parent);
> > +
> > +	/* smallest counter value to reboot soon */
> > +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> > +
> > +	rzg2l_wdt_wait_delay(priv);
> > +	/* Enable watchdog timer*/
> > +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info rzg2l_wdt_ident = {
> > +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> WDIOF_SETTIMEOUT,
> > +	.identity = "Renesas RZ/G2L WDT Watchdog", };
> > +
> > +static int rzg2l_wdt_ping(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_ops rzg2l_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = rzg2l_wdt_start,
> > +	.stop = rzg2l_wdt_stop,
> > +	.ping = rzg2l_wdt_ping,
> > +	.restart = rzg2l_wdt_restart,
> > +};
> > +
> > +static void rzg2l_wdt_reset_assert_clock_disable(void *data) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(data);
> > +
> > +	reset_control_assert(priv->rstc);
> > +}
> > +
> > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct rzg2l_wdt_priv *priv;
> > +	struct clk *wdt_clk;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	/* Get watchdog main clock */
> > +	wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> oscclk");
> > +
> > +	priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > +	if (!priv->osc_clk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> > +
> > +	/* Get Peripheral clock */
> > +	wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> > +
> > +	priv->pclk_rate = clk_get_rate(wdt_clk);
> > +	if (!priv->pclk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> > +
> > +	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 +
> > +F2CYCLE_NSEC(priv->pclk_rate) * 9;
> > +
> > +	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->rstc))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> > +			"failed to get cpg reset");
> > +
> > +	reset_control_deassert(priv->rstc);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rzg2l_wdt_reset_assert_clock_disable,
> > +				       &priv->wdev);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to get reset");
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed");
> > +		goto out_pm_get;
> > +	}
> > +
> > +	priv->wdev.info = &rzg2l_wdt_ident;
> > +	priv->wdev.ops = &rzg2l_wdt_ops;
> > +	priv->wdev.parent = dev;
> > +	priv->wdev.min_timeout = 1;
> > +	priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
> > +	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> > +
> > +	platform_set_drvdata(pdev, priv);
> 
> Is this used anywhere ?

Not used. It is supposed to use in remove function, for unregistering
watchdog device(in case, if we use watchdog_register_device)

But I am planning to use devm_watchdog_register_device.
So will remove both platform_set_drvdata and remove function.

> 
> > +	watchdog_set_drvdata(&priv->wdev, priv);
> > +	watchdog_set_nowayout(&priv->wdev, nowayout);
> > +	watchdog_set_restart_priority(&priv->wdev, 0);
> 
> 0 is the default restart priority (ie this is a reset of last resort).
> It does not have to be set explicitly.

Agreed. Will remove this.

> 
> > +	watchdog_stop_on_unregister(&priv->wdev);
> > +
> > +	ret = watchdog_init_timeout(&priv->wdev, 0, dev);
> > +	if (ret)
> > +		dev_warn(dev, "Specified timeout invalid, using default");
> > +
> > +	ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> > +	if (ret < 0)
> > +		goto out_pm_disable;
> > +
> > +	return 0;
> > +
> > +out_pm_disable:
> > +	pm_runtime_put(dev);
> > +out_pm_get:
> > +	pm_runtime_disable(dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rzg2l_wdt_remove(struct platform_device *pdev) {
> > +	pm_runtime_put(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +
> 
> This is the wrong order. The watchdog device has to be removed first,
> meaning you can not use devm_watchdog_register_device(), or you would have
> to use devm_add_action_or_reset() to call the runtime pm put/disable
> functions.

OK, will use devm_watchdog_register_device and will put runtime pm put/disable
to devm_add_action_or_reset. After this remove function is empty, and will remove it.

Regards,
Biju

> 
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rzg2l_wdt_ids[] = {
> > +	{ .compatible = "renesas,rzg2l-wdt", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
> > +
> > +static struct platform_driver rzg2l_wdt_driver = {
> > +	.driver = {
> > +		.name = "rzg2l_wdt",
> > +		.of_match_table = rzg2l_wdt_ids,
> > +	},
> > +	.probe = rzg2l_wdt_probe,
> > +	.remove = rzg2l_wdt_remove,
> > +};
> > +module_platform_driver(rzg2l_wdt_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/G2L WDT Watchdog Driver");
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_LICENSE("GPL v2");
> >

Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Philipp Zabel]

On Thu, 2021-11-04 at 16:08 +0000, Biju Das wrote:
[...]
> +static int rzg2l_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rzg2l_wdt_priv *priv;
> +	struct clk *wdt_clk;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	/* Get watchdog main clock */
> +	wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> +	if (IS_ERR(wdt_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
> +
> +	priv->osc_clk_rate = clk_get_rate(wdt_clk);
> +	if (!priv->osc_clk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> +
> +	/* Get Peripheral clock */
> +	wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(wdt_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> +
> +	priv->pclk_rate = clk_get_rate(wdt_clk);
> +	if (!priv->pclk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> +
> +	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 + F2CYCLE_NSEC(priv->pclk_rate) * 9;
> +
> +	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);

Please use devm_reset_control_get_exclusive().

> +	if (IS_ERR(priv->rstc))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> +			"failed to get cpg reset");
> +
> +	reset_control_deassert(priv->rstc);
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rzg2l_wdt_reset_assert_clock_disable,

I suppose rzg2l_wdt_reset_assert_clock_disable should be renamed to
rzg2l_wdt_reset_assert given that it does not disable a clock.

> +				       &priv->wdev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "failed to get reset");

I think this should just return ret, as the only possible failure from
devm_add_action_or_reset() is -ENOMEM.

> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed");

Consider printing ret with %pe.

> +		goto out_pm_get;
> +	}
> +
> +	priv->wdev.info = &rzg2l_wdt_ident;
> +	priv->wdev.ops = &rzg2l_wdt_ops;
> +	priv->wdev.parent = dev;
> +	priv->wdev.min_timeout = 1;
> +	priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
> +	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> +
> +	platform_set_drvdata(pdev, priv);
> +	watchdog_set_drvdata(&priv->wdev, priv);
> +	watchdog_set_nowayout(&priv->wdev, nowayout);
> +	watchdog_set_restart_priority(&priv->wdev, 0);
> +	watchdog_stop_on_unregister(&priv->wdev);
> +
> +	ret = watchdog_init_timeout(&priv->wdev, 0, dev);
> +	if (ret)
> +		dev_warn(dev, "Specified timeout invalid, using default");
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> +	if (ret < 0)
> +		goto out_pm_disable;
> +
> +	return 0;
> +
> +out_pm_disable:
> +	pm_runtime_put(dev);
> +out_pm_get:
> +	pm_runtime_disable(dev);
> +
> +	return ret;
> +}

regards
Philipp

RE: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Biju Das]

Hello Philipp Zabel,

Thanks for the feedback. 

> Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> On Thu, 2021-11-04 at 16:08 +0000, Biju Das wrote:
> [...]
> > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct rzg2l_wdt_priv *priv;
> > +	struct clk *wdt_clk;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	/* Get watchdog main clock */
> > +	wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> oscclk");
> > +
> > +	priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > +	if (!priv->osc_clk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> > +
> > +	/* Get Peripheral clock */
> > +	wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> > +
> > +	priv->pclk_rate = clk_get_rate(wdt_clk);
> > +	if (!priv->pclk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> > +
> > +	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 +
> > +F2CYCLE_NSEC(priv->pclk_rate) * 9;
> > +
> > +	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> 
> Please use devm_reset_control_get_exclusive().

Agreed. Will use devm_reset_control_get_exclusive.
> 
> > +	if (IS_ERR(priv->rstc))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> > +			"failed to get cpg reset");
> > +
> > +	reset_control_deassert(priv->rstc);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rzg2l_wdt_reset_assert_clock_disable,
> 
> I suppose rzg2l_wdt_reset_assert_clock_disable should be renamed to
> rzg2l_wdt_reset_assert given that it does not disable a clock.

OK. But I am going to use devm_watchdog_register_device and planning to 
add pm disable/put in rzg2l_wdt_reset_assert_pm_disable function.

After that driver won't have remove callback as it is empty.

> 
> > +				       &priv->wdev);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to get reset");
> 
> I think this should just return ret, as the only possible failure from
> devm_add_action_or_reset() is -ENOMEM.

Agreed, will just return ret.

> 
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed");
> 
> Consider printing ret with %pe.

Ok will print ret with %pe.

Regards,
Biju

> 
> > +		goto out_pm_get;
> > +	}
> > +
> > +	priv->wdev.info = &rzg2l_wdt_ident;
> > +	priv->wdev.ops = &rzg2l_wdt_ops;
> > +	priv->wdev.parent = dev;
> > +	priv->wdev.min_timeout = 1;
> > +	priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
> > +	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +	watchdog_set_drvdata(&priv->wdev, priv);
> > +	watchdog_set_nowayout(&priv->wdev, nowayout);
> > +	watchdog_set_restart_priority(&priv->wdev, 0);
> > +	watchdog_stop_on_unregister(&priv->wdev);
> > +
> > +	ret = watchdog_init_timeout(&priv->wdev, 0, dev);
> > +	if (ret)
> > +		dev_warn(dev, "Specified timeout invalid, using default");
> > +
> > +	ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> > +	if (ret < 0)
> > +		goto out_pm_disable;
> > +
> > +	return 0;
> > +
> > +out_pm_disable:
> > +	pm_runtime_put(dev);
> > +out_pm_get:
> > +	pm_runtime_disable(dev);
> > +
> > +	return ret;
> > +}
> 
> regards
> Philipp