[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

[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

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

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

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