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

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

In order to be able to reset the board when a watchdog timer expires,
the RSTEN register must be configured. it is the responsability of the
bootloader to set those bits (or not, depending on the chosen policy).

If the watchdog reset source is not enabled, an interrupt is triggered
when the watchdog expires. Currently this interrupt doesn't much apart
from printing a message.

Changes v1 -> v2:
* Modified the clock driver to not enable the watchdog reset sources.
  On other renesas platforms, those bits are by the bootloader. The
  watchdog reset sources are still disabled when the platform is halted
  to prevent a watchdog reset.
* Added a SOC-specific compatible "renesas,r9a06g032-wdt"
* reordered the dts/i entries
* default timeout is 60 seconds
* reworked the probe function of the wdt driver to better error cases
* removed the set_timeout() and use a fixed period computed in probe().
  This removes the confusion and makes it clear that the period defined
  by the user space in indeed handled by the watchdog core

Jean-Jacques Hiblot (5):
  dt-bindings: clock: r9a06g032: Add the definition of the 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 60s timeout
  clk: renesas: r9a06g032: Disable the watchdog reset sources when
    halting

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

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

-- 
2.25.1

[PATCH v2 1/6] dt-bindings: clock: r9a06g032: Add the definition of the 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

[PATCH v2 2/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 | 6 ++++++
 1 file changed, 6 insertions(+)

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

[PATCH v2 3/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..c5659db1581b 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -66,6 +66,22 @@ soc {
 		interrupt-parent = <&gic>;
 		ranges;
 
+		wdt0: watchdog@40008000 {
+			compatible = "renesas,r9a06g032-wdt";
+			reg = <0x40008000 0x1000>;
+			interrupts = <GIC_SPI 73 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
+			status = "disabled";
+		};
+
+		wdt1: watchdog@40009000 {
+			compatible = "renesas,r9a06g032-wdt";
+			reg = <0x40009000 0x1000>;
+			interrupts = <GIC_SPI 74 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
+			status = "disabled";
+		};
+
 		sysctrl: system-controller@4000c000 {
 			compatible = "renesas,r9a06g032-sysctrl";
 			reg = <0x4000c000 0x1000>;
-- 
2.25.1

[PATCH v2 4/6] ARM: dts: r9a06g032-rzn1d400-db: Enable watchdog0 with a 60s timeout

[Jean-Jacques Hiblot]

60s is a sensible default value.

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..3f8f3ce87e12 100644
--- a/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
+++ b/arch/arm/boot/dts/r9a06g032-rzn1d400-db.dts
@@ -26,3 +26,8 @@ aliases {
 &uart0 {
 	status = "okay";
 };
+
+&wdt0 {
+	timeout-sec = <60>;
+	status = "okay";
+};
-- 
2.25.1

[PATCH v2 5/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 | 208 ++++++++++++++++++++++++++++++++++++
 3 files changed, 217 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..bf548b9eca26
--- /dev/null
+++ b/drivers/watchdog/rzn1_wdt.c
@@ -0,0 +1,208 @@
+// 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 DEFAULT_TIMEOUT		60
+
+#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			16384
+#define RZN1_WDT_MAX				4095
+
+struct rzn1_watchdog {
+	struct watchdog_device		wdt;
+	void __iomem			*base;
+	unsigned long			clk_rate;
+};
+
+#define to_rzn1_watchdog(_ptr) \
+	container_of(_ptr, struct rzn1_watchdog, wdt)
+
+static inline uint32_t get_max_heart_beat(uint32_t clk_rate)
+{
+	return (RZN1_WDT_MAX * RZN1_WDT_PRESCALER) / (clk_rate / 1000);
+}
+static inline uint32_t compute_reload_value(uint32_t tick_ms, uint32_t clk)
+{
+	return (tick_ms * (clk / 1000)) / RZN1_WDT_PRESCALER;
+}
+
+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 |= compute_reload_value(w->max_hw_heartbeat_ms, wdt->clk_rate);
+	writel(val, wdt->base + RZN1_WDT_RETRIGGER);
+
+	return 0;
+}
+
+static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt)
+{
+	struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt;
+
+	dev_info(wdt->wdt.parent, "%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,
+};
+
+static const struct watchdog_device rzn1_wdt = {
+	.info = &rzn1_wdt_info,
+	.ops = &rzn1_wdt_ops,
+	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
+};
+
+static void rzn1_wdt_clk_disable_unprepare(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int rzn1_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzn1_watchdog *wdt;
+	struct device_node *np = dev->of_node;
+	struct clk *clk;
+	int ret;
+	int irq;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->wdt = rzn1_wdt;
+	wdt->wdt.parent = dev;
+
+	wdt->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(wdt->base))
+		return PTR_ERR(wdt->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "failed to get the irq\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(dev, irq, rzn1_wdt_irq, 0,
+			       np->name, wdt);
+	if (ret) {
+		dev_err(dev, "failed to request irq %d\n", irq);
+		return ret;
+	}
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "failed to get the clock\n");
+		return PTR_ERR(clk);
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "failed to prepare/enable the clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, rzn1_wdt_clk_disable_unprepare,
+				       clk);
+	if (ret) {
+		dev_err(dev, "failed to register clock unprepare callback\n");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	wdt->clk_rate = clk_get_rate(clk);
+	if (!wdt->clk_rate) {
+		dev_err(dev, "failed to get the clock rate\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * The period of the watchdog cannot be changed once set
+	 * and is limited to a very short period.
+	 * Configure it for a 1s period once and for all, and
+	 * rely on the heart-beat provided by the watchdog core
+	 * to make this usable by the user-space.
+	 */
+	wdt->wdt.max_hw_heartbeat_ms = get_max_heart_beat(wdt->clk_rate);
+	if (wdt->wdt.max_hw_heartbeat_ms > 1000)
+		wdt->wdt.max_hw_heartbeat_ms = 1000;
+
+	wdt->wdt.timeout = DEFAULT_TIMEOUT;
+	ret = watchdog_init_timeout(&wdt->wdt, 0, dev);
+
+	return devm_watchdog_register_device(dev, &wdt->wdt);
+}
+
+
+static const struct of_device_id rzn1_wdt_match[] = {
+	{ .compatible = "renesas,r9a06g032-wdt" },
+	{ .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

[PATCH v2 6/6] clk: renesas: r9a06g032: Disable the watchdog reset sources when halting

[Jean-Jacques Hiblot]

The watchdog reset sources must be disabled when the system is halted.
Otherwise the watchdogs will trigger a reset if they have been armed.

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

diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
index c99942f0e4d4..d96211927a1d 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,18 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
+
 	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 +1005,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 v2 5/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Tue, Feb 8, 2022 at 7:35 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,208 @@
> +// 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 DEFAULT_TIMEOUT                60
> +
> +#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                     16384
> +#define RZN1_WDT_MAX                           4095
> +
> +struct rzn1_watchdog {
> +       struct watchdog_device          wdt;
> +       void __iomem                    *base;
> +       unsigned long                   clk_rate;
> +};
> +
> +#define to_rzn1_watchdog(_ptr) \
> +       container_of(_ptr, struct rzn1_watchdog, wdt)
> +
> +static inline uint32_t get_max_heart_beat(uint32_t clk_rate)

unsigned long clk_rate

> +{
> +       return (RZN1_WDT_MAX * RZN1_WDT_PRESCALER) / (clk_rate / 1000);

Is clk_rate always a multiple of 1000? If not, you want to reorder
this to avoid losing precision.

> +}
> +static inline uint32_t compute_reload_value(uint32_t tick_ms, uint32_t clk)

unsigned long clk_rate

> +{
> +       return (tick_ms * (clk / 1000)) / RZN1_WDT_PRESCALER;

Likewise.

> +}

> +static int rzn1_wdt_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rzn1_watchdog *wdt;
> +       struct device_node *np = dev->of_node;
> +       struct clk *clk;
> +       int ret;
> +       int irq;
> +
> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +       if (!wdt)
> +               return -ENOMEM;
> +
> +       wdt->wdt = rzn1_wdt;
> +       wdt->wdt.parent = dev;
> +
> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(wdt->base))
> +               return PTR_ERR(wdt->base);
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(dev, "failed to get the irq\n");

No need to print a message, platform_get_irq() already does that.

> +               return irq;
> +       }
> +
> +       ret = devm_request_irq(dev, irq, rzn1_wdt_irq, 0,
> +                              np->name, wdt);
> +       if (ret) {
> +               dev_err(dev, "failed to request irq %d\n", irq);
> +               return ret;
> +       }
> +
> +       clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(clk)) {
> +               dev_err(dev, "failed to get the clock\n");
> +               return PTR_ERR(clk);
> +       }
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret) {
> +               dev_err(dev, "failed to prepare/enable the clock\n");
> +               return ret;
> +       }
> +
> +       ret = devm_add_action_or_reset(dev, rzn1_wdt_clk_disable_unprepare,
> +                                      clk);
> +       if (ret) {
> +               dev_err(dev, "failed to register clock unprepare callback\n");
> +               clk_disable_unprepare(clk);

Please drop this, as devm_add_action_or_reset() calls the
action on failure.

> +               return ret;
> +       }
> +
> +       wdt->clk_rate = clk_get_rate(clk);
> +       if (!wdt->clk_rate) {
> +               dev_err(dev, "failed to get the clock rate\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * The period of the watchdog cannot be changed once set
> +        * and is limited to a very short period.
> +        * Configure it for a 1s period once and for all, and
> +        * rely on the heart-beat provided by the watchdog core
> +        * to make this usable by the user-space.
> +        */
> +       wdt->wdt.max_hw_heartbeat_ms = get_max_heart_beat(wdt->clk_rate);
> +       if (wdt->wdt.max_hw_heartbeat_ms > 1000)
> +               wdt->wdt.max_hw_heartbeat_ms = 1000;
> +
> +       wdt->wdt.timeout = DEFAULT_TIMEOUT;
> +       ret = watchdog_init_timeout(&wdt->wdt, 0, dev);
> +
> +       return devm_watchdog_register_device(dev, &wdt->wdt);
> +}
> +
> +
> +static const struct of_device_id rzn1_wdt_match[] = {
> +       { .compatible = "renesas,r9a06g032-wdt" },

No need to match on the soc-specific compatible value, as the
family-specific value should be present in the DTB, too.

> +       { .compatible = "renesas,rzn1-wdt" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);

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 v2 2/6] dt-bindings: watchdog: renesas,wdt: Add support for RZ/N1

[Geert Uytterhoeven]

On Tue, Feb 8, 2022 at 7:35 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v2 3/6] ARM: dts: r9a06g032: Add the watchdog nodes

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Tue, Feb 8, 2022 at 7:35 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
> @@ -66,6 +66,22 @@ soc {
>                 interrupt-parent = <&gic>;
>                 ranges;
>
> +               wdt0: watchdog@40008000 {
> +                       compatible = "renesas,r9a06g032-wdt";

compatible = "renesas,r9a06g032-wdt", "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,r9a06g032-wdt";

compatible = "renesas,r9a06g032-wdt", "renesas,rzn1-wdt";

> +                       reg = <0x40009000 0x1000>;
> +                       interrupts = <GIC_SPI 74 IRQ_TYPE_EDGE_RISING>;
> +                       clocks = <&sysctrl R9A06G032_CLK_WATCHDOG>;
> +                       status = "disabled";
> +               };
> +
>                 sysctrl: system-controller@4000c000 {
>                         compatible = "renesas,r9a06g032-sysctrl";
>                         reg = <0x4000c000 0x1000>;
> --
> 2.25.1
>


-- 
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 v2 4/6] ARM: dts: r9a06g032-rzn1d400-db: Enable watchdog0 with a 60s timeout

[Geert Uytterhoeven]

On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> 60s is a sensible default value.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v2 5/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Jean-Jacques Hiblot]

Hi Geert,

On 09/02/2022 09:28, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
>
> On Tue, Feb 8, 2022 at 7:35 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,208 @@
>> +// 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 DEFAULT_TIMEOUT                60
>> +
>> +#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                     16384
>> +#define RZN1_WDT_MAX                           4095
>> +
>> +struct rzn1_watchdog {
>> +       struct watchdog_device          wdt;
>> +       void __iomem                    *base;
>> +       unsigned long                   clk_rate;
>> +};
>> +
>> +#define to_rzn1_watchdog(_ptr) \
>> +       container_of(_ptr, struct rzn1_watchdog, wdt)
>> +
>> +static inline uint32_t get_max_heart_beat(uint32_t clk_rate)
> unsigned long clk_rate
>
>> +{
>> +       return (RZN1_WDT_MAX * RZN1_WDT_PRESCALER) / (clk_rate / 1000);
> Is clk_rate always a multiple of 1000? If not, you want to reorder
> this to avoid losing precision.

The clock is 62.5 MHz, so dividing by 1000 doesn't cause a big precision 
loss.

I could use the 64bit division but it seemed less readable and the 
watchdog is

not used as a precise timer anyway.

>
>> +}
>> +static inline uint32_t compute_reload_value(uint32_t tick_ms, uint32_t clk)
> unsigned long clk_rate
>
>> +{
>> +       return (tick_ms * (clk / 1000)) / RZN1_WDT_PRESCALER;
> Likewise.
>
>> +}
>> +static int rzn1_wdt_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct rzn1_watchdog *wdt;
>> +       struct device_node *np = dev->of_node;
>> +       struct clk *clk;
>> +       int ret;
>> +       int irq;
>> +
>> +       wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> +       if (!wdt)
>> +               return -ENOMEM;
>> +
>> +       wdt->wdt = rzn1_wdt;
>> +       wdt->wdt.parent = dev;
>> +
>> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(wdt->base))
>> +               return PTR_ERR(wdt->base);
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(dev, "failed to get the irq\n");
> No need to print a message, platform_get_irq() already does that.
>
>> +               return irq;
>> +       }
>> +
>> +       ret = devm_request_irq(dev, irq, rzn1_wdt_irq, 0,
>> +                              np->name, wdt);
>> +       if (ret) {
>> +               dev_err(dev, "failed to request irq %d\n", irq);
>> +               return ret;
>> +       }
>> +
>> +       clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(clk)) {
>> +               dev_err(dev, "failed to get the clock\n");
>> +               return PTR_ERR(clk);
>> +       }
>> +
>> +       ret = clk_prepare_enable(clk);
>> +       if (ret) {
>> +               dev_err(dev, "failed to prepare/enable the clock\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = devm_add_action_or_reset(dev, rzn1_wdt_clk_disable_unprepare,
>> +                                      clk);
>> +       if (ret) {
>> +               dev_err(dev, "failed to register clock unprepare callback\n");
>> +               clk_disable_unprepare(clk);
> Please drop this, as devm_add_action_or_reset() calls the
> action on failure.
>
>> +               return ret;
>> +       }
>> +
>> +       wdt->clk_rate = clk_get_rate(clk);
>> +       if (!wdt->clk_rate) {
>> +               dev_err(dev, "failed to get the clock rate\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /*
>> +        * The period of the watchdog cannot be changed once set
>> +        * and is limited to a very short period.
>> +        * Configure it for a 1s period once and for all, and
>> +        * rely on the heart-beat provided by the watchdog core
>> +        * to make this usable by the user-space.
>> +        */
>> +       wdt->wdt.max_hw_heartbeat_ms = get_max_heart_beat(wdt->clk_rate);
>> +       if (wdt->wdt.max_hw_heartbeat_ms > 1000)
>> +               wdt->wdt.max_hw_heartbeat_ms = 1000;
>> +
>> +       wdt->wdt.timeout = DEFAULT_TIMEOUT;
>> +       ret = watchdog_init_timeout(&wdt->wdt, 0, dev);
>> +
>> +       return devm_watchdog_register_device(dev, &wdt->wdt);
>> +}
>> +
>> +
>> +static const struct of_device_id rzn1_wdt_match[] = {
>> +       { .compatible = "renesas,r9a06g032-wdt" },
> No need to match on the soc-specific compatible value, as the
> family-specific value should be present in the DTB, too.
>
>> +       { .compatible = "renesas,rzn1-wdt" },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match);
> 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 v2 1/6] dt-bindings: clock: r9a06g032: Add the definition of the the watchdog clock

[Rob Herring]

On Tue, 08 Feb 2022 19:35:05 +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>

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

[Rob Herring]

On Tue, 08 Feb 2022 19:35:06 +0100, Jean-Jacques Hiblot wrote:
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

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

Re: [PATCH v2 6/6] clk: renesas: r9a06g032: Disable the watchdog reset sources when halting

[Geert Uytterhoeven]

Hi Jean-Jacques,

CC watchdog people, who only received some patches.

On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> The watchdog reset sources must be disabled when the system is halted.
> Otherwise the watchdogs will trigger a reset if they have been armed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

Thanks for your patch!

[inserting changelog]

| Changes v1 -> v2:
| * Modified the clock driver to not enable the watchdog reset sources.
|   On other renesas platforms, those bits are by the bootloader. The
|   watchdog reset sources are still disabled when the platform is halted
|   to prevent a watchdog reset.

I still have my doubts about this part. So on halt, you override the
policy configured by the boot loader, which means the watchdog is no
longer triggered on halt.

From a system perspective, the system can be in five states:
  1. Running,
  2. Crashed/lock-ed up,
  3. Halt,
  4. Reboot,
  5. Poweroff.

Now, from a policy perspective, what is the difference between a
system that crashes or locks up, and a system that halts?
I.e. should the system reboot on halt, or not?

I think halting a system where the watchdog has been activated makes
no sense, and the user either wants to explicitly reboot the system, or
power it off, but never halt it.  So I think this patch is not needed.

Watchdog people: what is your opinion?
Thanks!

> --- 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,18 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>         if (error)
>                 return error;
>
> +
>         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 +1005,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)

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 v2 6/6] clk: renesas: r9a06g032: Disable the watchdog reset sources when halting

[Guenter Roeck]

On 2/14/22 02:45, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
> 
> CC watchdog people, who only received some patches.
> 
> On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> The watchdog reset sources must be disabled when the system is halted.
>> Otherwise the watchdogs will trigger a reset if they have been armed.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> 
> Thanks for your patch!
> 
> [inserting changelog]
> 
> | Changes v1 -> v2:
> | * Modified the clock driver to not enable the watchdog reset sources.
> |   On other renesas platforms, those bits are by the bootloader. The
> |   watchdog reset sources are still disabled when the platform is halted
> |   to prevent a watchdog reset.
> 
> I still have my doubts about this part. So on halt, you override the
> policy configured by the boot loader, which means the watchdog is no
> longer triggered on halt.
> 
>>From a system perspective, the system can be in five states:
>    1. Running,
>    2. Crashed/lock-ed up,
>    3. Halt,
>    4. Reboot,
>    5. Poweroff.
> 
> Now, from a policy perspective, what is the difference between a
> system that crashes or locks up, and a system that halts?
> I.e. should the system reboot on halt, or not?
> 
> I think halting a system where the watchdog has been activated makes
> no sense, and the user either wants to explicitly reboot the system, or
> power it off, but never halt it.  So I think this patch is not needed.
> 
> Watchdog people: what is your opinion?

In my understanding the shutdown code is always executed, ie also for
restarts and poweroff. Disabling the watchdog in that situation is not
always desirable, though sometimes necessary depending on the hardware.
Disabling it through the backdoor (instead of calling watchdog_stop_on_reboot)
seems odd, though.

Guenter

> Thanks!
> 
>> --- 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,18 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>>          if (error)
>>                  return error;
>>
>> +
>>          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 +1005,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)
> 
> 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 v2 6/6] clk: renesas: r9a06g032: Disable the watchdog reset sources when halting

[Jean-Jacques Hiblot]

Hi,

On 14/02/2022 16:34, Guenter Roeck wrote:
> On 2/14/22 02:45, Geert Uytterhoeven wrote:
>> Hi Jean-Jacques,
>>
>> CC watchdog people, who only received some patches.
>>
>> On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
>> <jjhiblot@traphandler.com> wrote:
>>> The watchdog reset sources must be disabled when the system is halted.
>>> Otherwise the watchdogs will trigger a reset if they have been armed.
>>>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>
>> Thanks for your patch!
>>
>> [inserting changelog]
>>
>> | Changes v1 -> v2:
>> | * Modified the clock driver to not enable the watchdog reset sources.
>> |   On other renesas platforms, those bits are by the bootloader. The
>> |   watchdog reset sources are still disabled when the platform is 
>> halted
>> |   to prevent a watchdog reset.
>>
>> I still have my doubts about this part. So on halt, you override the
>> policy configured by the boot loader, which means the watchdog is no
>> longer triggered on halt.
>>
>>> From a system perspective, the system can be in five states:
>>    1. Running,
>>    2. Crashed/lock-ed up,
>>    3. Halt,
>>    4. Reboot,
>>    5. Poweroff.
>>
>> Now, from a policy perspective, what is the difference between a
>> system that crashes or locks up, and a system that halts?
>> I.e. should the system reboot on halt, or not?
>>
>> I think halting a system where the watchdog has been activated makes
>> no sense, and the user either wants to explicitly reboot the system, or
>> power it off, but never halt it.  So I think this patch is not needed.

I don't see halting the machine as a must-have feature either, but it 
seemed to me

that there could be other people relying on it. I'll remove this patch 
from the series.

>>
>> Watchdog people: what is your opinion?
>
> In my understanding the shutdown code is always executed, ie also for
> restarts and poweroff. Disabling the watchdog in that situation is not
> always desirable, though sometimes necessary depending on the hardware.
> Disabling it through the backdoor (instead of calling 
> watchdog_stop_on_reboot)
> seems odd, though.


Unfortunately, in this case it is not possible to stop the watchdog once 
started.

The only way to halt the machine is to disable the watchdog reset source.


JJ


>
> Guenter
>
>> Thanks!
>>
>>> --- 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,18 @@ static int __init r9a06g032_clocks_probe(struct 
>>> platform_device *pdev)
>>>          if (error)
>>>                  return error;
>>>
>>> +
>>>          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 +1005,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)
>>
>> 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
>