[PATCH 0/6] ARM: r9a06g032: add support for the watchdogs

[PATCH 0/6] ARM: r9a06g032: add support for the watchdogs

[Jean-Jacques Hiblot]

Hi all,

This series adds support for the watchdog timers of the RZ/N1.
The watchdog driver (rzn1-wdt.c) is derived from the driver available at
https://github.com/renesas-rz/rzn1_linux.git with a few modifications
(devm watchdog API usage and WDIOF_MAGICCLOSE option)

In order to be able to reset the board when a watchdog timer expires,
the RSTEN register must be configured. This is done in the clock
driver of the r9a06g032. The rationnal is that this driver is the only one
that handles the sysctrl for this platform and there are a couple of other
clock drivers that also handle resets/reboot. I intend to later post
another patch to add software-triggered reboot capability that will
leverage this change.


Jean-Jacques Hiblot (5):
  clk: renesas: r9a06g032: Enable the watchdog reset sources
  dt-bindings: clock: r9a06g032: Add the definition of the watchdog
    clock
  dt-bindings: watchdog: renesas,wdt: Add support for RZ/N1
  ARM: dts: r9a06g032: Add the watchdog nodes
  ARM: dts: r9a06g032-rzn1d400-db: Enable watchdog0 with a 10s timeout

Phil Edworthy (1):
  watchdog: Add Renesas RZ/N1 Watchdog driver

 .../bindings/watchdog/renesas,wdt.yaml        |   4 +
 arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts   |   5 +
 arch/arm/boot/dts/r9a06g032.dtsi              |  16 ++
 drivers/clk/renesas/r9a06g032-clocks.c        |  33 +++
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/rzn1_wdt.c                   | 197 ++++++++++++++++++
 include/dt-bindings/clock/r9a06g032-sysctrl.h |   1 +
 8 files changed, 265 insertions(+)
 create mode 100644 drivers/watchdog/rzn1_wdt.c

-- 
2.25.1

[PATCH 1/6] clk: renesas: r9a06g032: Enable the watchdog reset sources

[Jean-Jacques Hiblot]

The watchdog reset sources are not enabled by default.
Enabling them here to make sure that the system resets when the watchdog
timers expire.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/clk/renesas/r9a06g032-clocks.c | 33 ++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
index c99942f0e4d4..57fcad1c8ba2 100644
--- a/drivers/clk/renesas/r9a06g032-clocks.c
+++ b/drivers/clk/renesas/r9a06g032-clocks.c
@@ -129,6 +129,11 @@ enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, K_DUALGATE };
 
 #define R9A06G032_CLOCK_COUNT		(R9A06G032_UART_GROUP_34567 + 1)
 
+#define R9A06G032_SYSCTRL_REG_RSTEN		0x120
+#define WDA7RST1	BIT(2)
+#define WDA7RST0	BIT(1)
+#define MRESET		BIT(0)
+
 static const struct r9a06g032_clkdesc r9a06g032_clocks[] = {
 	D_ROOT(CLKOUT, "clkout", 25, 1),
 	D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10),
@@ -893,6 +898,19 @@ static void r9a06g032_clocks_del_clk_provider(void *data)
 	of_clk_del_provider(data);
 }
 
+static void r9a06g032_reset_sources(struct r9a06g032_priv *clocks,
+			uint32_t mask, uint32_t value)
+{
+	uint32_t rsten;
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks->lock, flags);
+	rsten = readl(clocks->reg);
+	rsten = (rsten & ~mask) | (value & mask);
+	writel(rsten, clocks->reg + R9A06G032_SYSCTRL_REG_RSTEN);
+	spin_unlock_irqrestore(&clocks->lock, flags);
+}
+
 static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -910,6 +928,8 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 	if (!clocks || !clks)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, clocks);
+
 	spin_lock_init(&clocks->lock);
 
 	clocks->data.clks = clks;
@@ -963,9 +983,21 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
+
+	/* Enable the global system reset and watchdog reset sources */
+	r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1 | MRESET, MRESET | WDA7RST0 | WDA7RST1);
+
 	return r9a06g032_add_clk_domain(dev);
 }
 
+static void r9a06g032_clocks_shutdown(struct platform_device *pdev)
+{
+	struct r9a06g032_priv *clocks = platform_get_drvdata(pdev);
+
+	/* Disable the watchdog reset sources */
+	r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1, 0);
+}
+
 static const struct of_device_id r9a06g032_match[] = {
 	{ .compatible = "renesas,r9a06g032-sysctrl" },
 	{ }
@@ -976,6 +1008,7 @@ static struct platform_driver r9a06g032_clock_driver = {
 		.name	= "renesas,r9a06g032-sysctrl",
 		.of_match_table = r9a06g032_match,
 	},
+	.shutdown = r9a06g032_clocks_shutdown,
 };
 
 static int __init r9a06g032_clocks_init(void)
-- 
2.25.1

Re: [PATCH 1/6] clk: renesas: r9a06g032: Enable the watchdog reset sources

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> The watchdog reset sources are not enabled by default.
> Enabling them here to make sure that the system resets when the watchdog
> timers expire.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

R-Car Gen3 and RZ/G2 SoCs have a similar mechanism.
On these SoCs, the boot loader takes care of the configuration, as this
is a system policy that goes beyond the Linux realm.
Perhaps the RZ/N1 boot loader can do the same?

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 1/6] clk: renesas: r9a06g032: Enable the watchdog reset sources

[Jean-Jacques Hiblot]

On 07/02/2022 16:34, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
>
> On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> The watchdog reset sources are not enabled by default.
>> Enabling them here to make sure that the system resets when the watchdog
>> timers expire.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Thanks for your patch!
>
> R-Car Gen3 and RZ/G2 SoCs have a similar mechanism.
> On these SoCs, the boot loader takes care of the configuration, as this
> is a system policy that goes beyond the Linux realm.
> Perhaps the RZ/N1 boot loader can do the same?
>
> Gr{oetje,eeting}s,

Thanks for you reviews and comments.

I'm not conformable with the idea that the safety induced by the

watchdog is removed because the bootloader didn't set the register.

I'd rather that the kernel is able to enable the watchdog reset source.

If it is acceptable, we could use a new DTS entry to force the policy.
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

Re: [PATCH 1/6] clk: renesas: r9a06g032: Enable the watchdog reset sources

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Tue, Feb 8, 2022 at 11:25 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 07/02/2022 16:34, Geert Uytterhoeven wrote:
> > On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:
> >> The watchdog reset sources are not enabled by default.
> >> Enabling them here to make sure that the system resets when the watchdog
> >> timers expire.
> >>
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> > Thanks for your patch!
> >
> > R-Car Gen3 and RZ/G2 SoCs have a similar mechanism.
> > On these SoCs, the boot loader takes care of the configuration, as this
> > is a system policy that goes beyond the Linux realm.
> > Perhaps the RZ/N1 boot loader can do the same?
> >
> > Gr{oetje,eeting}s,
>
> Thanks for you reviews and comments.
>
> I'm not conformable with the idea that the safety induced by the
> watchdog is removed because the bootloader didn't set the register.

What if the CM33 is the master, and the CM33 just wants to receive an
interrupt when one of the CA7 watchdog timers times out?

> I'd rather that the kernel is able to enable the watchdog reset source.
> If it is acceptable, we could use a new DTS entry to force the policy.

DT describes hardware. not software policy.
Although I agree e.g. the watchdog timeout value is software policy.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 1/6] clk: renesas: r9a06g032: Enable the watchdog reset sources

[Jean-Jacques Hiblot]

Hi Geert,

On 08/02/2022 11:35, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
>
> On Tue, Feb 8, 2022 at 11:25 AM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> On 07/02/2022 16:34, Geert Uytterhoeven wrote:
>>> On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
>>> <jjhiblot@traphandler.com> wrote:
>>>> The watchdog reset sources are not enabled by default.
>>>> Enabling them here to make sure that the system resets when the watchdog
>>>> timers expire.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>> Thanks for your patch!
>>>
>>> R-Car Gen3 and RZ/G2 SoCs have a similar mechanism.
>>> On these SoCs, the boot loader takes care of the configuration, as this
>>> is a system policy that goes beyond the Linux realm.
>>> Perhaps the RZ/N1 boot loader can do the same?
>>>
>>> Gr{oetje,eeting}s,
>> Thanks for you reviews and comments.
>>
>> I'm not conformable with the idea that the safety induced by the
>> watchdog is removed because the bootloader didn't set the register.
> What if the CM33 is the master, and the CM33 just wants to receive an
> interrupt when one of the CA7 watchdog timers times out?

In the next version of the patch I removed the part that enables the 
reset source.

However I kept the part that disables the reset source when the system 
is halted

because the system would otherwise reboot when a watchdog expires. Do you

see a scenario where this could be a problem ?

JJ

>
>> I'd rather that the kernel is able to enable the watchdog reset source.
>> If it is acceptable, we could use a new DTS entry to force the policy.
> DT describes hardware. not software policy.
> Although I agree e.g. the watchdog timeout value is software policy.

>
> 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

[PATCH 2/6] dt-bindings: clock: r9a06g032: Add the definition of the watchdog clock

[Jean-Jacques Hiblot]

This clock is actually the REF_SYNC_D8 clock.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 include/dt-bindings/clock/r9a06g032-sysctrl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/r9a06g032-sysctrl.h b/include/dt-bindings/clock/r9a06g032-sysctrl.h
index 90c0f3dc1ba1..d9d7b8b4f426 100644
--- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
+++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
@@ -74,6 +74,7 @@
 #define R9A06G032_CLK_DDRPHY_PCLK	81	/* AKA CLK_REF_SYNC_D4 */
 #define R9A06G032_CLK_FW		81	/* AKA CLK_REF_SYNC_D4 */
 #define R9A06G032_CLK_CRYPTO		81	/* AKA CLK_REF_SYNC_D4 */
+#define R9A06G032_CLK_WATCHDOG		82	/* AKA CLK_REF_SYNC_D8 */
 #define R9A06G032_CLK_A7MP		84	/* AKA DIV_CA7 */
 #define R9A06G032_HCLK_CAN0		85
 #define R9A06G032_HCLK_CAN1		86
-- 
2.25.1

Re: [PATCH 2/6] dt-bindings: clock: r9a06g032: Add the definition of the watchdog clock

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> This clock is actually the REF_SYNC_D8 clock.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

> --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> @@ -74,6 +74,7 @@
>  #define R9A06G032_CLK_DDRPHY_PCLK      81      /* AKA CLK_REF_SYNC_D4 */
>  #define R9A06G032_CLK_FW               81      /* AKA CLK_REF_SYNC_D4 */
>  #define R9A06G032_CLK_CRYPTO           81      /* AKA CLK_REF_SYNC_D4 */
> +#define R9A06G032_CLK_WATCHDOG         82      /* AKA CLK_REF_SYNC_D8 */
>  #define R9A06G032_CLK_A7MP             84      /* AKA DIV_CA7 */
>  #define R9A06G032_HCLK_CAN0            85
>  #define R9A06G032_HCLK_CAN1            86

I couldn't find this in the documentation, so I have to trust you on this.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH 2/6] dt-bindings: clock: r9a06g032: Add the definition of the watchdog clock

[Rob Herring]

On Fri, 04 Feb 2022 17:18:00 +0100, Jean-Jacques Hiblot wrote:
> This clock is actually the REF_SYNC_D8 clock.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>  include/dt-bindings/clock/r9a06g032-sysctrl.h | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 3/6] dt-bindings: watchdog: renesas,wdt: Add support for RZ/N1

[Jean-Jacques Hiblot]

Describe the WDT hardware in the RZ/N1 series.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
index 91a98ccd4226..11e1c9f101a7 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
@@ -19,6 +19,9 @@ properties:
               - renesas,r7s9210-wdt      # RZ/A2
           - const: renesas,rza-wdt       # RZ/A
 
+      - items:
+          - const: renesas,rzn1-wdt # RZ/N1
+
       - items:
           - enum:
               - renesas,r9a07g044-wdt    # RZ/G2{L,LC}
@@ -89,6 +92,7 @@ allOf:
             contains:
               enum:
                 - renesas,rza-wdt
+                - renesas,rzn1-wdt
     then:
       required:
         - power-domains
-- 
2.25.1

Re: [PATCH 3/6] dt-bindings: watchdog: renesas,wdt: Add support for RZ/N1

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> Describe the WDT hardware in the RZ/N1 series.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> @@ -19,6 +19,9 @@ properties:
>                - renesas,r7s9210-wdt      # RZ/A2
>            - const: renesas,rza-wdt       # RZ/A
>
> +      - items:
> +          - const: renesas,rzn1-wdt # RZ/N1

I think it would be good to have an SoC-specific compatible value
("renesas,r9a06g032-wdt") in addition to the family-specific one.

> +
>        - items:
>            - enum:
>                - renesas,r9a07g044-wdt    # RZ/G2{L,LC}

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

[PATCH 4/6] ARM: dts: r9a06g032: Add the watchdog nodes

[Jean-Jacques Hiblot]

This SOC includes 2 watchdog controllers (one per A7 core).

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index c47896e4ab58..54c91b46a5d0 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -184,6 +184,22 @@ gic: interrupt-controller@44101000 {
 			interrupts =
 				<GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
 		};
+
+		wdt0: watchdog@40008000 {
+			compatible = "renesas,rzn1-wdt";
+			reg = <0x40008000 0x1000>;
+			interrupts = <GIC_SPI 73 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
+			status = "disabled";
+		};
+
+		wdt1: watchdog@40009000 {
+			compatible = "renesas,rzn1-wdt";
+			reg = <0x40009000 0x1000>;
+			interrupts = <GIC_SPI 74 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
+			status = "disabled";
+		};
 	};
 
 	timer {
-- 
2.25.1

Re: [PATCH 4/6] ARM: dts: r9a06g032: Add the watchdog nodes

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> This SOC includes 2 watchdog controllers (one per A7 core).
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -184,6 +184,22 @@ gic: interrupt-controller@44101000 {
>                         interrupts =
>                                 <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
>                 };
> +
> +               wdt0: watchdog@40008000 {

Please insert these nodes before the system-controller@4000c000
node, to preserve sort order (by unit address).

> +                       compatible = "renesas,rzn1-wdt";

"renesas,r9a06g032-wdt", "renesas,rzn1-wdt"
as per my comments on the DT bindings patch.

> +                       reg = <0x40008000 0x1000>;
> +                       interrupts = <GIC_SPI 73 IRQ_TYPE_EDGE_RISING>;
> +                       clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
> +                       status = "disabled";
> +               };
> +
> +               wdt1: watchdog@40009000 {
> +                       compatible = "renesas,rzn1-wdt";
> +                       reg = <0x40009000 0x1000>;
> +                       interrupts = <GIC_SPI 74 IRQ_TYPE_EDGE_RISING>;
> +                       clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
> +                       status = "disabled";
> +               };
>         };
>

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

[PATCH 5/6] ARM: dts: r9a06g032-rzn1d400-db: Enable watchdog0 with a 10s timeout

[Jean-Jacques Hiblot]

10s seems a reasonable value for a watchdog.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
index 4e57ae2688fc..5c8f46b20acc 100644
--- a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
+++ b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
@@ -23,6 +23,11 @@ aliases {
 	};
 };
 
+&wdt0 {
+	timeout-sec = <10>;
+	status = "okay";
+};
+
 &uart0 {
 	status = "okay";
 };
-- 
2.25.1

Re: [PATCH 5/6] ARM: dts: r9a06g032-rzn1d400-db: Enable watchdog0 with a 10s timeout

[Geert Uytterhoeven]

Hi Jean-Jacques,

Thanks for your patch!

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> 10s seems a reasonable value for a watchdog.

All other Renesas DTS files use 60s. Would 60s be OK for you?

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

> --- a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
> +++ b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
> @@ -23,6 +23,11 @@ aliases {
>         };
>  };
>
> +&wdt0 {

Please insert below "uart0", to preserve sort order (alphabetically).

> +       timeout-sec = <10>;
> +       status = "okay";
> +};
> +
>  &uart0 {
>         status = "okay";
>  };

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

[PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Jean-Jacques Hiblot]

From: Phil Edworthy <phil.edworthy@renesas.com>

This is a driver for the standard WDT on the RZ/N1 devices. This WDT has
very limited timeout capabilities. However, it can reset the device.
To do so, the corresponding bits in the SysCtrl RSTEN register need to
be enabled. This is not done by this driver.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/watchdog/Kconfig    |   8 ++
 drivers/watchdog/Makefile   |   1 +
 drivers/watchdog/rzn1_wdt.c | 197 ++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/watchdog/rzn1_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c8fa79da23b3..ba6e4ebef404 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -883,6 +883,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_RZN1WDT
+	tristate "Renesas RZ/N1 watchdog"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This driver adds watchdog support for the integrated watchdogs in the
+	  Renesas RZ/N1 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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..38d38564f47b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -84,6 +84,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_RZN1WDT) += rzn1_wdt.o
 obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
new file mode 100644
index 000000000000..0d3284a44dde
--- /dev/null
+++ b/drivers/watchdog/rzn1_wdt.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/N1 Watchdog timer.
+ * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
+ * cope with 2 seconds.
+ *
+ * Copyright 2018 Renesas Electronics Europe Ltd.
+ *
+ * Derived from Ralink RT288x watchdog timer.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define RZN1_WDT_RETRIGGER			0x0
+#define RZN1_WDT_RETRIGGER_RELOAD_VAL		0
+#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK	0xfff
+#define RZN1_WDT_RETRIGGER_PRESCALE		BIT(12)
+#define RZN1_WDT_RETRIGGER_ENABLE		BIT(13)
+#define RZN1_WDT_RETRIGGER_WDSI			(0x2 << 14)
+
+#define RZN1_WDT_PRESCALER			BIT(14)
+#define RZN1_WDT_MAX				4095
+
+struct rzn1_watchdog {
+	struct watchdog_device		wdt;
+	struct device			*dev;
+	void __iomem			*base;
+	unsigned int			irq;
+	unsigned int			reload_val;
+	unsigned long			clk_rate;
+};
+
+#define to_rzn1_watchdog(_ptr) \
+	container_of(_ptr, struct rzn1_watchdog, wdt)
+
+static int rzn1_wdt_ping(struct watchdog_device *w)
+{
+	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
+
+	/* Any value retrigggers the watchdog */
+	writel(0, wdt->base + RZN1_WDT_RETRIGGER);
+
+	return 0;
+}
+
+static int rzn1_wdt_start(struct watchdog_device *w)
+{
+	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
+	u32 val;
+
+	/*
+	 * The hardware allows you to write to this reg only once.
+	 * Since this includes the reload value, there is no way to change the
+	 * timeout once started. Also note that the WDT clock is half the bus
+	 * fabric clock rate, so if the bus fabric clock rate is changed after
+	 * the WDT is started, the WDT interval will be wrong.
+	 */
+	val = RZN1_WDT_RETRIGGER_WDSI;
+	val |= RZN1_WDT_RETRIGGER_ENABLE;
+	val |= RZN1_WDT_RETRIGGER_PRESCALE;
+	val |= wdt->reload_val;
+	writel(val, wdt->base + RZN1_WDT_RETRIGGER);
+
+	return 0;
+}
+
+static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
+{
+	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
+
+	w->timeout = t;
+
+	wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER;
+
+	return 0;
+}
+
+static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt)
+{
+	struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt;
+
+	dev_info(wdt->dev, "%s triggered\n", __func__);
+	return IRQ_HANDLED;
+}
+
+static struct watchdog_info rzn1_wdt_info = {
+	.identity = "RZ/N1 Watchdog",
+	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+};
+
+static const struct watchdog_ops rzn1_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = rzn1_wdt_start,
+	.ping = rzn1_wdt_ping,
+	.set_timeout = rzn1_wdt_set_timeout,
+};
+
+static const struct watchdog_device rzn1_wdt_dev = {
+	.info = &rzn1_wdt_info,
+	.ops = &rzn1_wdt_ops,
+	.min_timeout = 0,
+	.max_timeout = 1,
+	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
+};
+
+static int rzn1_wdt_probe(struct platform_device *pdev)
+{
+	struct rzn1_watchdog *wdt;
+	int ret;
+	struct device_node *np = pdev->dev.of_node;
+	int err;
+	struct clk *clk;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+	wdt->dev = &pdev->dev;
+	wdt->wdt = rzn1_wdt_dev;
+	platform_set_drvdata(pdev, wdt);
+
+	wdt->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(wdt->base)) {
+		dev_err(wdt->dev, "unable to get register bank\n");
+		return PTR_ERR(wdt->base);
+	}
+	wdt->irq = irq_of_parse_and_map(np, 0);
+	if (wdt->irq == NO_IRQ) {
+		dev_err(wdt->dev, "failed to get IRQ from DT\n");
+		return -EINVAL;
+	}
+
+	err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
+			       np->name, wdt);
+	if (err) {
+		dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
+		return err;
+	}
+	clk = devm_clk_get(wdt->dev, NULL);
+	if (!IS_ERR(clk) && clk_prepare_enable(clk))
+		return PTR_ERR(clk);
+
+	wdt->clk_rate = clk_get_rate(clk);
+	if (!wdt->clk_rate) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	wdt->reload_val = RZN1_WDT_MAX;
+	wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
+					(wdt->clk_rate / RZN1_WDT_PRESCALER);
+
+	ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
+	if (ret)
+		dev_warn(&pdev->dev, "Specified timeout invalid, using default");
+
+	ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);
+	if (ret)
+		goto error;
+
+	dev_info(wdt->dev, "Initialized\n");
+
+	return 0;
+
+error:
+err_clk_disable:
+	clk_disable_unprepare(clk);
+	dev_warn(wdt->dev, "Initialization failed\n");
+	return err;
+}
+
+
+static const struct of_device_id rzn1_wdt_match[] = {
+	{ .compatible = "renesas,rzn1-wdt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
+
+static struct platform_driver rzn1_wdt_driver = {
+	.probe		= rzn1_wdt_probe,
+	.driver		= {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= rzn1_wdt_match,
+	},
+};
+
+module_platform_driver(rzn1_wdt_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog");
+MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Guenter Roeck]

On 2/4/22 08:18, Jean-Jacques Hiblot wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has
> very limited timeout capabilities. However, it can reset the device.
> To do so, the corresponding bits in the SysCtrl RSTEN register need to
> be enabled. This is not done by this driver.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ---
>   drivers/watchdog/Kconfig    |   8 ++
>   drivers/watchdog/Makefile   |   1 +
>   drivers/watchdog/rzn1_wdt.c | 197 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 206 insertions(+)
>   create mode 100644 drivers/watchdog/rzn1_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c8fa79da23b3..ba6e4ebef404 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -883,6 +883,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_RZN1WDT
> +	tristate "Renesas RZ/N1 watchdog"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  This driver adds watchdog support for the integrated watchdogs in the
> +	  Renesas RZ/N1 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
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f7da867e8782..38d38564f47b 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -84,6 +84,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_RZN1WDT) += rzn1_wdt.o
>   obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
>   obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>   obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
> new file mode 100644
> index 000000000000..0d3284a44dde
> --- /dev/null
> +++ b/drivers/watchdog/rzn1_wdt.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 Watchdog timer.
> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
> + * cope with 2 seconds.
> + *
> + * Copyright 2018 Renesas Electronics Europe Ltd.
> + *
> + * Derived from Ralink RT288x watchdog timer.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define RZN1_WDT_RETRIGGER			0x0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL		0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK	0xfff
> +#define RZN1_WDT_RETRIGGER_PRESCALE		BIT(12)
> +#define RZN1_WDT_RETRIGGER_ENABLE		BIT(13)
> +#define RZN1_WDT_RETRIGGER_WDSI			(0x2 << 14)
> +
> +#define RZN1_WDT_PRESCALER			BIT(14)
> +#define RZN1_WDT_MAX				4095
> +
> +struct rzn1_watchdog {
> +	struct watchdog_device		wdt;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	unsigned int			irq;
> +	unsigned int			reload_val;
> +	unsigned long			clk_rate;
> +};
> +
> +#define to_rzn1_watchdog(_ptr) \
> +	container_of(_ptr, struct rzn1_watchdog, wdt)
> +
> +static int rzn1_wdt_ping(struct watchdog_device *w)
> +{
> +	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
> +
> +	/* Any value retrigggers the watchdog */
> +	writel(0, wdt->base + RZN1_WDT_RETRIGGER);
> +
> +	return 0;
> +}
> +
> +static int rzn1_wdt_start(struct watchdog_device *w)
> +{
> +	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
> +	u32 val;
> +
> +	/*
> +	 * The hardware allows you to write to this reg only once.
> +	 * Since this includes the reload value, there is no way to change the
> +	 * timeout once started. Also note that the WDT clock is half the bus
> +	 * fabric clock rate, so if the bus fabric clock rate is changed after
> +	 * the WDT is started, the WDT interval will be wrong.
> +	 */
> +	val = RZN1_WDT_RETRIGGER_WDSI;
> +	val |= RZN1_WDT_RETRIGGER_ENABLE;
> +	val |= RZN1_WDT_RETRIGGER_PRESCALE;
> +	val |= wdt->reload_val;
> +	writel(val, wdt->base + RZN1_WDT_RETRIGGER);
> +
> +	return 0;
> +}
> +
> +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
> +{
> +	struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
> +
> +	w->timeout = t;
> +
> +	wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER;
> +

Is that really what you want, given that it can only be set once,
when the watchdog is started for the first time ?
 From the context, I would assume that you'd want to always set reload_val
to wdt->clk_rate / RZN1_WDT_PRESCALER. That could be done in the probe
function, meaning a set_timeout function is not really needed.

> +	return 0;
> +}
> +
> +static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt)
> +{
> +	struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt;
> +
> +	dev_info(wdt->dev, "%s triggered\n", __func__);
> +	return IRQ_HANDLED;
> +}

What is the point of supporting an interrupt if it doesn't do anything
other than saying that it happened ?

> +
> +static struct watchdog_info rzn1_wdt_info = {
> +	.identity = "RZ/N1 Watchdog",
> +	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +};
> +
> +static const struct watchdog_ops rzn1_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = rzn1_wdt_start,
> +	.ping = rzn1_wdt_ping,
> +	.set_timeout = rzn1_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_device rzn1_wdt_dev = {
> +	.info = &rzn1_wdt_info,
> +	.ops = &rzn1_wdt_ops,
> +	.min_timeout = 0,

Really ? Did you try that ?

> +	.max_timeout = 1,

Documentation says

  * @max_hw_heartbeat_ms:
  *              Hardware limit for maximum timeout, in milli-seconds.
  *              Replaces max_timeout if specified.

and you set max_hw_heartbeat_ms below, so this is unnecessary.

> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
> +};
> +
> +static int rzn1_wdt_probe(struct platform_device *pdev)
> +{
> +	struct rzn1_watchdog *wdt;
> +	int ret;
> +	struct device_node *np = pdev->dev.of_node;
> +	int err;
> +	struct clk *clk;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > +	if (!wdt)
> +		return -ENOMEM;
> +	wdt->dev = &pdev->dev;
> +	wdt->wdt = rzn1_wdt_dev;
> +	platform_set_drvdata(pdev, wdt);
> +
> +	wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(wdt->base)) {
> +		dev_err(wdt->dev, "unable to get register bank\n");
> +		return PTR_ERR(wdt->base);
> +	}
> +	wdt->irq = irq_of_parse_and_map(np, 0);
> +	if (wdt->irq == NO_IRQ) {
> +		dev_err(wdt->dev, "failed to get IRQ from DT\n");
> +		return -EINVAL;
> +	}
> +
> +	err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
> +			       np->name, wdt);
> +	if (err) {
> +		dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
> +		return err;
> +	}
> +	clk = devm_clk_get(wdt->dev, NULL);
> +	if (!IS_ERR(clk) && clk_prepare_enable(clk))
> +		return PTR_ERR(clk);

That doesn't work. clk is not an ERR_PTR here, and the return value
is thus an PTR_ERR() on a real pointer. This needs to be split into
separate statements and checks.

> +
> +	wdt->clk_rate = clk_get_rate(clk);
> +	if (!wdt->clk_rate) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	wdt->reload_val = RZN1_WDT_MAX;
> +	wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
> +					(wdt->clk_rate / RZN1_WDT_PRESCALER);
> +
> +	ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
> +	if (ret)
> +		dev_warn(&pdev->dev, "Specified timeout invalid, using default");
> +

wdt->wdt.parent needs to be set as well.

> +	ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);

You either have to add a callback to disable and unprepare the clock
when the driver is removed (see other watchdog drivers for example), or
you need a remove function and can not use devm_watchdog_register_device().

> +	if (ret)
> +		goto error;
> +
> +	dev_info(wdt->dev, "Initialized\n");
> +
> +	return 0;
> +
> +error:
> +err_clk_disable:

To labels pointing to the same place does not add any value.

> +	clk_disable_unprepare(clk);
> +	dev_warn(wdt->dev, "Initialization failed\n");

This is not a warning.

> +	return err;
> +}
> +
> +
> +static const struct of_device_id rzn1_wdt_match[] = {
> +	{ .compatible = "renesas,rzn1-wdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
> +
> +static struct platform_driver rzn1_wdt_driver = {
> +	.probe		= rzn1_wdt_probe,
> +	.driver		= {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= rzn1_wdt_match,
> +	},
> +};
> +
> +module_platform_driver(rzn1_wdt_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog");
> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Jean-Jacques Hiblot]

On 04/02/2022 18:27, Guenter Roeck wrote:
> On 2/4/22 08:18, Jean-Jacques Hiblot wrote:
>> From: Phil Edworthy <phil.edworthy@renesas.com>
>>
>> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has
>> very limited timeout capabilities. However, it can reset the device.
>> To do so, the corresponding bits in the SysCtrl RSTEN register need to
>> be enabled. This is not done by this driver.
>>
>> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>> ---
>>   drivers/watchdog/Kconfig    |   8 ++
>>   drivers/watchdog/Makefile   |   1 +
>>   drivers/watchdog/rzn1_wdt.c | 197 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 206 insertions(+)
>>   create mode 100644 drivers/watchdog/rzn1_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index c8fa79da23b3..ba6e4ebef404 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -883,6 +883,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_RZN1WDT
>> +    tristate "Renesas RZ/N1 watchdog"
>> +    depends on ARCH_RENESAS || COMPILE_TEST
>> +    select WATCHDOG_CORE
>> +    help
>> +      This driver adds watchdog support for the integrated watchdogs 
>> in the
>> +      Renesas RZ/N1 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
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index f7da867e8782..38d38564f47b 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -84,6 +84,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_RZN1WDT) += rzn1_wdt.o
>>   obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o
>>   obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>>   obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c
>> new file mode 100644
>> index 000000000000..0d3284a44dde
>> --- /dev/null
>> +++ b/drivers/watchdog/rzn1_wdt.c
>> @@ -0,0 +1,197 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Renesas RZ/N1 Watchdog timer.
>> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It 
>> can't even
>> + * cope with 2 seconds.
>> + *
>> + * Copyright 2018 Renesas Electronics Europe Ltd.
>> + *
>> + * Derived from Ralink RT288x watchdog timer.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define RZN1_WDT_RETRIGGER            0x0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL        0
>> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK    0xfff
>> +#define RZN1_WDT_RETRIGGER_PRESCALE        BIT(12)
>> +#define RZN1_WDT_RETRIGGER_ENABLE        BIT(13)
>> +#define RZN1_WDT_RETRIGGER_WDSI            (0x2 << 14)
>> +
>> +#define RZN1_WDT_PRESCALER            BIT(14)
>> +#define RZN1_WDT_MAX                4095
>> +
>> +struct rzn1_watchdog {
>> +    struct watchdog_device        wdt;
>> +    struct device            *dev;
>> +    void __iomem            *base;
>> +    unsigned int            irq;
>> +    unsigned int            reload_val;
>> +    unsigned long            clk_rate;
>> +};
>> +
>> +#define to_rzn1_watchdog(_ptr) \
>> +    container_of(_ptr, struct rzn1_watchdog, wdt)
>> +
>> +static int rzn1_wdt_ping(struct watchdog_device *w)
>> +{
>> +    struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
>> +
>> +    /* Any value retrigggers the watchdog */
>> +    writel(0, wdt->base + RZN1_WDT_RETRIGGER);
>> +
>> +    return 0;
>> +}
>> +
>> +static int rzn1_wdt_start(struct watchdog_device *w)
>> +{
>> +    struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
>> +    u32 val;
>> +
>> +    /*
>> +     * The hardware allows you to write to this reg only once.
>> +     * Since this includes the reload value, there is no way to 
>> change the
>> +     * timeout once started. Also note that the WDT clock is half 
>> the bus
>> +     * fabric clock rate, so if the bus fabric clock rate is changed 
>> after
>> +     * the WDT is started, the WDT interval will be wrong.
>> +     */
>> +    val = RZN1_WDT_RETRIGGER_WDSI;
>> +    val |= RZN1_WDT_RETRIGGER_ENABLE;
>> +    val |= RZN1_WDT_RETRIGGER_PRESCALE;
>> +    val |= wdt->reload_val;
>> +    writel(val, wdt->base + RZN1_WDT_RETRIGGER);
>> +
>> +    return 0;
>> +}
>> +
>> +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned 
>> int t)
>> +{
>> +    struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
>> +
>> +    w->timeout = t;
>> +
>> +    wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER;
>> +
>
> Is that really what you want, given that it can only be set once,
> when the watchdog is started for the first time ?
> From the context, I would assume that you'd want to always set reload_val
> to wdt->clk_rate / RZN1_WDT_PRESCALER. That could be done in the probe
> function, meaning a set_timeout function is not really needed.
>
Thanks for the review and advice. I didn't want to modify too heavily 
the out-of-tree

driver at first. I'll update the driver and take your comments into 
consideration

>> +    return 0;
>> +}
>> +
>> +static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt)
>> +{
>> +    struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt;
>> +
>> +    dev_info(wdt->dev, "%s triggered\n", __func__);
>> +    return IRQ_HANDLED;
>> +}
>
> What is the point of supporting an interrupt if it doesn't do anything
> other than saying that it happened ?
It is possible to trigger an interrupt or reset the SOC when the watchdog

expires. It depends on the value of the RSTEN register in the sysctrl.

This interrupt handler has no functional purpose, but keeping it has 2 
benefits:

- it let the user know that a watchdog interrupt occurred.

- it serves as a reference for other developers to build custom handlers.


>
>> +
>> +static struct watchdog_info rzn1_wdt_info = {
>> +    .identity = "RZ/N1 Watchdog",
>> +    .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | 
>> WDIOF_KEEPALIVEPING,
>> +};
>> +
>> +static const struct watchdog_ops rzn1_wdt_ops = {
>> +    .owner = THIS_MODULE,
>> +    .start = rzn1_wdt_start,
>> +    .ping = rzn1_wdt_ping,
>> +    .set_timeout = rzn1_wdt_set_timeout,
>> +};
>> +
>> +static const struct watchdog_device rzn1_wdt_dev = {
>> +    .info = &rzn1_wdt_info,
>> +    .ops = &rzn1_wdt_ops,
>> +    .min_timeout = 0,
>
> Really ? Did you try that ?
>> +    .max_timeout = 1,
>
> Documentation says
>
>  * @max_hw_heartbeat_ms:
>  *              Hardware limit for maximum timeout, in milli-seconds.
>  *              Replaces max_timeout if specified.
>
> and you set max_hw_heartbeat_ms below, so this is unnecessary.
>
>> +    .status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>> +};
>> +
>> +static int rzn1_wdt_probe(struct platform_device *pdev)
>> +{
>> +    struct rzn1_watchdog *wdt;
>> +    int ret;
>> +    struct device_node *np = pdev->dev.of_node;
>> +    int err;
>> +    struct clk *clk;
>> +
>> +    wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > +    
>> if (!wdt)
>> +        return -ENOMEM;
>> +    wdt->dev = &pdev->dev;
>> +    wdt->wdt = rzn1_wdt_dev;
>> +    platform_set_drvdata(pdev, wdt);
>> +
>> +    wdt->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(wdt->base)) {
>> +        dev_err(wdt->dev, "unable to get register bank\n");
>> +        return PTR_ERR(wdt->base);
>> +    }
>> +    wdt->irq = irq_of_parse_and_map(np, 0);
>> +    if (wdt->irq == NO_IRQ) {
>> +        dev_err(wdt->dev, "failed to get IRQ from DT\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
>> +                   np->name, wdt);
>> +    if (err) {
>> +        dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
>> +        return err;
>> +    }
>> +    clk = devm_clk_get(wdt->dev, NULL);
>> +    if (!IS_ERR(clk) && clk_prepare_enable(clk))
>> +        return PTR_ERR(clk);
>
> That doesn't work. clk is not an ERR_PTR here, and the return value
> is thus an PTR_ERR() on a real pointer. This needs to be split into
> separate statements and checks.
>
>> +
>> +    wdt->clk_rate = clk_get_rate(clk);
>> +    if (!wdt->clk_rate) {
>> +        err = -EINVAL;
>> +        goto err_clk_disable;
>> +    }
>> +
>> +    wdt->reload_val = RZN1_WDT_MAX;
>> +    wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
>> +                    (wdt->clk_rate / RZN1_WDT_PRESCALER);
>> +
>> +    ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
>> +    if (ret)
>> +        dev_warn(&pdev->dev, "Specified timeout invalid, using 
>> default");
>> +
>
> wdt->wdt.parent needs to be set as well.
>
>> +    ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);
>
> You either have to add a callback to disable and unprepare the clock
> when the driver is removed (see other watchdog drivers for example), or
> you need a remove function and can not use 
> devm_watchdog_register_device().
>
>> +    if (ret)
>> +        goto error;
>> +
>> +    dev_info(wdt->dev, "Initialized\n");
>> +
>> +    return 0;
>> +
>> +error:
>> +err_clk_disable:
>
> To labels pointing to the same place does not add any value.
>
>> +    clk_disable_unprepare(clk);
>> +    dev_warn(wdt->dev, "Initialization failed\n");
>
> This is not a warning.
>
>> +    return err;
>> +}
>> +
>> +
>> +static const struct of_device_id rzn1_wdt_match[] = {
>> +    { .compatible = "renesas,rzn1-wdt" },
>> +    {},
>> +};
>> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
>> +
>> +static struct platform_driver rzn1_wdt_driver = {
>> +    .probe        = rzn1_wdt_probe,
>> +    .driver        = {
>> +        .name        = KBUILD_MODNAME,
>> +        .of_match_table    = rzn1_wdt_match,
>> +    },
>> +};
>> +
>> +module_platform_driver(rzn1_wdt_driver);
>> +
>> +MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog");
>> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
>> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[kernel test robot]

Hi Jean-Jacques,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on geert-renesas-devel/next geert-renesas-drivers/renesas-clk linus/master v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220205/202202050309.wemilTv5-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/12248ab8278751ebab4bf211becde9db4956ca5a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
        git checkout 12248ab8278751ebab4bf211becde9db4956ca5a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/watchdog/rzn1_wdt.c: In function 'rzn1_wdt_probe':
>> drivers/watchdog/rzn1_wdt.c:134:25: error: 'NO_IRQ' undeclared (first use in this function); did you mean 'NR_IRQS'?
     134 |         if (wdt->irq == NO_IRQ) {
         |                         ^~~~~~
         |                         NR_IRQS
   drivers/watchdog/rzn1_wdt.c:134:25: note: each undeclared identifier is reported only once for each function it appears in


vim +134 drivers/watchdog/rzn1_wdt.c

   112	
   113	static int rzn1_wdt_probe(struct platform_device *pdev)
   114	{
   115		struct rzn1_watchdog *wdt;
   116		int ret;
   117		struct device_node *np = pdev->dev.of_node;
   118		int err;
   119		struct clk *clk;
   120	
   121		wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
   122		if (!wdt)
   123			return -ENOMEM;
   124		wdt->dev = &pdev->dev;
   125		wdt->wdt = rzn1_wdt_dev;
   126		platform_set_drvdata(pdev, wdt);
   127	
   128		wdt->base = devm_platform_ioremap_resource(pdev, 0);
   129		if (IS_ERR(wdt->base)) {
   130			dev_err(wdt->dev, "unable to get register bank\n");
   131			return PTR_ERR(wdt->base);
   132		}
   133		wdt->irq = irq_of_parse_and_map(np, 0);
 > 134		if (wdt->irq == NO_IRQ) {
   135			dev_err(wdt->dev, "failed to get IRQ from DT\n");
   136			return -EINVAL;
   137		}
   138	
   139		err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
   140				       np->name, wdt);
   141		if (err) {
   142			dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
   143			return err;
   144		}
   145		clk = devm_clk_get(wdt->dev, NULL);
   146		if (!IS_ERR(clk) && clk_prepare_enable(clk))
   147			return PTR_ERR(clk);
   148	
   149		wdt->clk_rate = clk_get_rate(clk);
   150		if (!wdt->clk_rate) {
   151			err = -EINVAL;
   152			goto err_clk_disable;
   153		}
   154	
   155		wdt->reload_val = RZN1_WDT_MAX;
   156		wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
   157						(wdt->clk_rate / RZN1_WDT_PRESCALER);
   158	
   159		ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
   160		if (ret)
   161			dev_warn(&pdev->dev, "Specified timeout invalid, using default");
   162	
   163		ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);
   164		if (ret)
   165			goto error;
   166	
   167		dev_info(wdt->dev, "Initialized\n");
   168	
   169		return 0;
   170	
   171	error:
   172	err_clk_disable:
   173		clk_disable_unprepare(clk);
   174		dev_warn(wdt->dev, "Initialization failed\n");
   175		return err;
   176	}
   177	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4364 bytes --]

Hi Jean-Jacques,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on geert-renesas-devel/next geert-renesas-drivers/renesas-clk linus/master v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220205/202202050309.wemilTv5-lkp(a)intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/12248ab8278751ebab4bf211becde9db4956ca5a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
        git checkout 12248ab8278751ebab4bf211becde9db4956ca5a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/watchdog/rzn1_wdt.c: In function 'rzn1_wdt_probe':
>> drivers/watchdog/rzn1_wdt.c:134:25: error: 'NO_IRQ' undeclared (first use in this function); did you mean 'NR_IRQS'?
     134 |         if (wdt->irq == NO_IRQ) {
         |                         ^~~~~~
         |                         NR_IRQS
   drivers/watchdog/rzn1_wdt.c:134:25: note: each undeclared identifier is reported only once for each function it appears in


vim +134 drivers/watchdog/rzn1_wdt.c

   112	
   113	static int rzn1_wdt_probe(struct platform_device *pdev)
   114	{
   115		struct rzn1_watchdog *wdt;
   116		int ret;
   117		struct device_node *np = pdev->dev.of_node;
   118		int err;
   119		struct clk *clk;
   120	
   121		wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
   122		if (!wdt)
   123			return -ENOMEM;
   124		wdt->dev = &pdev->dev;
   125		wdt->wdt = rzn1_wdt_dev;
   126		platform_set_drvdata(pdev, wdt);
   127	
   128		wdt->base = devm_platform_ioremap_resource(pdev, 0);
   129		if (IS_ERR(wdt->base)) {
   130			dev_err(wdt->dev, "unable to get register bank\n");
   131			return PTR_ERR(wdt->base);
   132		}
   133		wdt->irq = irq_of_parse_and_map(np, 0);
 > 134		if (wdt->irq == NO_IRQ) {
   135			dev_err(wdt->dev, "failed to get IRQ from DT\n");
   136			return -EINVAL;
   137		}
   138	
   139		err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
   140				       np->name, wdt);
   141		if (err) {
   142			dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
   143			return err;
   144		}
   145		clk = devm_clk_get(wdt->dev, NULL);
   146		if (!IS_ERR(clk) && clk_prepare_enable(clk))
   147			return PTR_ERR(clk);
   148	
   149		wdt->clk_rate = clk_get_rate(clk);
   150		if (!wdt->clk_rate) {
   151			err = -EINVAL;
   152			goto err_clk_disable;
   153		}
   154	
   155		wdt->reload_val = RZN1_WDT_MAX;
   156		wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
   157						(wdt->clk_rate / RZN1_WDT_PRESCALER);
   158	
   159		ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
   160		if (ret)
   161			dev_warn(&pdev->dev, "Specified timeout invalid, using default");
   162	
   163		ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);
   164		if (ret)
   165			goto error;
   166	
   167		dev_info(wdt->dev, "Initialized\n");
   168	
   169		return 0;
   170	
   171	error:
   172	err_clk_disable:
   173		clk_disable_unprepare(clk);
   174		dev_warn(wdt->dev, "Initialization failed\n");
   175		return err;
   176	}
   177	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
>
> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has
> very limited timeout capabilities. However, it can reset the device.
> To do so, the corresponding bits in the SysCtrl RSTEN register need to
> be enabled. This is not done by this driver.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/watchdog/rzn1_wdt.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 Watchdog timer.
> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
> + * cope with 2 seconds.
> + *
> + * Copyright 2018 Renesas Electronics Europe Ltd.
> + *
> + * Derived from Ralink RT288x watchdog timer.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define RZN1_WDT_RETRIGGER                     0x0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL          0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK     0xfff
> +#define RZN1_WDT_RETRIGGER_PRESCALE            BIT(12)
> +#define RZN1_WDT_RETRIGGER_ENABLE              BIT(13)
> +#define RZN1_WDT_RETRIGGER_WDSI                        (0x2 << 14)
> +
> +#define RZN1_WDT_PRESCALER                     BIT(14)

"BIT(14)" is a bit strange, as this is used as a scalar, never as
a bitmask.

> +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
> +{
> +       struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
> +
> +       w->timeout = t;
> +
> +       wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER;

I guess the multiplication can overflow in 32-bit arithmetic?

> +
> +       return 0;
> +}

> +static int rzn1_wdt_probe(struct platform_device *pdev)
> +{
> +       struct rzn1_watchdog *wdt;
> +       int ret;
> +       struct device_node *np = pdev->dev.of_node;
> +       int err;
> +       struct clk *clk;
> +
> +       wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +       if (!wdt)
> +               return -ENOMEM;
> +       wdt->dev = &pdev->dev;
> +       wdt->wdt = rzn1_wdt_dev;
> +       platform_set_drvdata(pdev, wdt);
> +
> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(wdt->base)) {
> +               dev_err(wdt->dev, "unable to get register bank\n");

No need to print an error message, __devm_ioremap_resource() takes
care of that.

> +               return PTR_ERR(wdt->base);
> +       }
> +       wdt->irq = irq_of_parse_and_map(np, 0);
> +       if (wdt->irq == NO_IRQ) {
> +               dev_err(wdt->dev, "failed to get IRQ from DT\n");
> +               return -EINVAL;
> +       }

Please use platform_get_irq() instead. Note that on error, it prints
an error message, and returns a negative value.  So you cannot assign
it to wdt->irq before checking, as the latter is unsigned:

    ret = platform_get_irq(pdev, 0);
    if (ret < 0)
            return ret;

    wdt->irq = ret;

> +       wdt->reload_val = RZN1_WDT_MAX;
> +       wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
> +                                       (wdt->clk_rate / RZN1_WDT_PRESCALER);

To avoid loss of precision, it's better to reorder operations.
As the dividend doesn't fit in 32-bit, you have to use a
64-bit-by-unsigned-long division:

    div64_ul(RZN1_WDT_MAX * 1000ULL * RZN1_WDT_PRESCALER,
             wdt->clk_rate);

> +
> +       ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
> +       if (ret)
> +               dev_warn(&pdev->dev, "Specified timeout invalid, using default");
> +
> +       ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);

"return devm_watchdog_register_device(...);"?

> +       if (ret)
> +               goto error;
> +
> +       dev_info(wdt->dev, "Initialized\n");
> +
> +       return 0;


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