On 8/5/24 13:04, Prabhakar wrote:
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
Add Watchdog Timer driver for RZ/V2H(P) SoC.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
---
v2->v3
- Fixed dependency, ARCH_R9A09G011->ARCH_R9A09G057
- Added dependency for PM
- Added delay after de-assert operation as clks are halted temporarily
after de-assert operation
- clearing WDTSR register
v1->v2
- Stopped using PM runtime calls in restart handler
- Dropped rstc deassert from probe
---
drivers/watchdog/Kconfig | 9 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/rzv2h_wdt.c | 259 +++++++++++++++++++++++++++++++++++
3 files changed, 269 insertions(+)
create mode 100644 drivers/watchdog/rzv2h_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bae1d97cce89..684b9fe84fff 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -953,6 +953,15 @@ config RENESAS_RZG2LWDT
This driver adds watchdog support for the integrated watchdogs in the
Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.
+config RENESAS_RZV2HWDT
+ tristate "Renesas RZ/V2H(P) WDT Watchdog"
+ depends on ARCH_R9A09G057 || COMPILE_TEST
+ depends on PM || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/V2H(P) SoCs. These watchdogs can be used to reset a system.
+
config ASPEED_WATCHDOG
tristate "Aspeed BMC watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b51030f035a6..ab6f2b41e38e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -86,6 +86,7 @@ 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_RENESAS_RZV2HWDT) += rzv2h_wdt.o
obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
new file mode 100644
index 000000000000..b6183940b221
--- /dev/null
+++ b/drivers/watchdog/rzv2h_wdt.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H(P) WDT Watchdog Driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corporation.
+ */
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/units.h>
+#include <linux/watchdog.h>
+
+#define WDTRR 0x00 /* RW, 8 */
+#define WDTCR 0x02 /* RW, 16 */
+#define WDTSR 0x04 /* RW, 16 */
+#define WDTRCR 0x06 /* RW, 8 */
+
+#define WDTCR_TOPS_1024 0x00
+#define WDTCR_TOPS_16384 0x03
+
+#define WDTCR_CKS_CLK_1 0x00
+#define WDTCR_CKS_CLK_256 0x50
+
+#define WDTCR_RPES_0 0x300
+#define WDTCR_RPES_75 0x000
+
+#define WDTCR_RPSS_25 0x00
+#define WDTCR_RPSS_100 0x3000
+
+#define WDTRCR_RSTIRQS BIT(7)
+
+#define CLOCK_DIV_BY_256 256
+
+#define WDT_DEFAULT_TIMEOUT 60U
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct rzv2h_wdt_priv {
+ void __iomem *base;
+ struct clk *pclk;
+ struct clk *oscclk;
+ struct reset_control *rstc;
+ struct watchdog_device wdev;
+ unsigned long oscclk_rate;
+};
+
+static int rzv2h_wdt_ping(struct watchdog_device *wdev)
+{
+ struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ static unsigned long delay;
+
+ writeb(0x0, priv->base + WDTRR);
+ writeb(0xFF, priv->base + WDTRR);
+
+ /*
+ * Refreshing the down-counter requires up to 4 cycles
+ * of the signal for counting
+ */
That doesn't explain why the delay (after pinging the watchdog) is necessary.
+ if (!delay)
+ delay = 4 * div64_ul(CLOCK_DIV_BY_256 * MICRO, priv->oscclk_rate);
"MICRO" is 1000000UL, and CLOCK_DIV_BY_256 is 256, making this 256000000 which fits
into 32 bit. oscclk_rate is unsigned long. Please explain the need for using div64_ul(),
because I don't see it.
Besides that, please explain the benefit of storing "delay" in a static variable
instead of calculating it with oscclk_rate and storing it in struct rzv2h_wdt_priv.
This should better be a good explanation because it is very unlikely that I'll accept
the code as written.
+ udelay(delay);
+
+ return 0;
+}
+
+static void rzv2h_wdt_setup(struct watchdog_device *wdev, u16 wdtcr)
+{
+ struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writew(wdtcr, priv->base + WDTCR);
+
+ writeb(0, priv->base + WDTRCR);
+
+ writew(0, priv->base + WDTSR);
+}
+
+static int rzv2h_wdt_start(struct watchdog_device *wdev)
+{
+ struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ int ret;
+
+ ret = reset_control_deassert(priv->rstc);
+ if (ret)
+ return ret;
+
+ udelay(5);
+
+ ret = pm_runtime_resume_and_get(wdev->parent);
+ if (ret) {
+ reset_control_assert(priv->rstc);
+ return ret;
+ }
+
+ /*
+ * WDTCR
+ * - CKS[7:4] - Clock Division Ratio Select - 0101b: oscclk/256
+ * - RPSS[13:12] - Window Start Position Select - 11b: 100%
+ * - RPES[9:8] - Window End Position Select - 11b: 0%
+ * - TOPS[1:0] - Timeout Period Select - 11b: 16384 cycles (3FFFh)
+ */
+ rzv2h_wdt_setup(wdev, WDTCR_CKS_CLK_256 | WDTCR_RPSS_100 |
+ WDTCR_RPES_0 | WDTCR_TOPS_16384);
+
+ rzv2h_wdt_ping(wdev);
+
The need to ping the watchdog immediately after enabling it is unusual.
Please explain.
+ return 0;
+}
+
+static int rzv2h_wdt_stop(struct watchdog_device *wdev)
+{
+ struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ int ret;
+
+ ret = pm_runtime_put(wdev->parent);
+ if (ret < 0)
+ return ret;
Shouldn't this be called _after_ stopping the watchdog ?
+
+ return reset_control_assert(priv->rstc);
+}
+
+static const struct watchdog_info rzv2h_wdt_ident = {
+ .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+ .identity = "Renesas RZ/V2H WDT Watchdog",
+};
+
+static int rzv2h_wdt_restart(struct watchdog_device *wdev,
+ unsigned long action, void *data)
+{
+ struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ int ret;
+
+ ret = reset_control_deassert(priv->rstc);
+ if (ret)
+ return ret;
+
+ /* delay to handle clock halt after de-assert operation */
+ udelay(5);
+
+ ret = clk_enable(priv->pclk);
+ if (ret) {
+ reset_control_assert(priv->rstc);
+ return ret;
+ }
+
+ ret = clk_enable(priv->oscclk);
+ if (ret) {
+ clk_disable(priv->pclk);
+ reset_control_assert(priv->rstc);
+ return ret;
+ }
+
Please explain the need for enabling the clocks here.
+ /*
+ * WDTCR
+ * - CKS[7:4] - Clock Division Ratio Select - 0000b: oscclk/1
+ * - RPSS[13:12] - Window Start Position Select - 00b: 25%
+ * - RPES[9:8] - Window End Position Select - 00b: 75%
+ * - TOPS[1:0] - Timeout Period Select - 00b: 1024 cycles (03FFh)
+ */
+ rzv2h_wdt_setup(wdev, WDTCR_CKS_CLK_1 | WDTCR_RPSS_25 |
+ WDTCR_RPES_75 | WDTCR_TOPS_1024);
+ rzv2h_wdt_ping(wdev);
+
Why is the ping here necessary ?
+ /* wait for underflow to trigger... */
+ mdelay(500);
Does that really take 500 ms ?
+
+ return 0;
+}
+
+static const struct watchdog_ops rzv2h_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = rzv2h_wdt_start,
+ .stop = rzv2h_wdt_stop,
+ .ping = rzv2h_wdt_ping,
+ .restart = rzv2h_wdt_restart,
+};
+
+static int rzv2h_wdt_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rzv2h_wdt_priv *priv;
+ unsigned long rate;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->pclk = devm_clk_get_prepared(&pdev->dev, "pclk");
+ if (IS_ERR(priv->pclk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->pclk), "no pclk");
+
+ priv->oscclk = devm_clk_get_prepared(&pdev->dev, "oscclk");
+ if (IS_ERR(priv->oscclk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->oscclk), "no oscclk");
+
+ priv->oscclk_rate = clk_get_rate(priv->oscclk);
+ if (!priv->oscclk_rate)
+ return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
+
+ priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, 16383NULL);
+ if (IS_ERR(priv->rstc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
+ "failed to get cpg reset");
+
+ rate = priv->oscclk_rate / 256;
+ priv->wdev.max_hw_heartbeat_ms = (1000 * 16383) / rate;
Not accounting to rounding, this translates to
= (1000 * 16383) / (clk_rate / 256)
= (1000 * 16383 * 256) / clk_rate
Why the added complexity ?
Also, what is the value of storing oscclk_rate instead of the calculated delay
in the private data structure ?
+ dev_dbg(dev, "max hw timeout of %dms\n",
+ priv->wdev.max_hw_heartbeat_ms);
+
+ ret = devm_pm_runtime_enable(&pdev->dev);
+ if (ret)
+ return ret;
+
+ priv->wdev.min_timeout = 1;
+ priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
+ priv->wdev.info = &rzv2h_wdt_ident;
+ priv->wdev.ops = &rzv2h_wdt_ops;
+ priv->wdev.parent = dev;
+ watchdog_set_drvdata(&priv->wdev, priv);
+ watchdog_set_nowayout(&priv->wdev, nowayout);
+ watchdog_stop_on_unregister(&priv->wdev);
+
+ ret = watchdog_init_timeout(&priv->wdev, 0, dev);
+ if (ret)
+ dev_warn(dev, "Specified timeout invalid, using default");
+
+ return devm_watchdog_register_device(&pdev->dev, &priv->wdev);
+}
+
+static const struct of_device_id rzv2h_wdt_ids[] = {
+ { .compatible = "renesas,r9a09g057-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2h_wdt_ids);
+
+static struct platform_driver rzv2h_wdt_driver = {
+ .driver = {
+ .name = "rzv2h_wdt",
+ .of_match_table = rzv2h_wdt_ids,
+ },
+ .probe = rzv2h_wdt_probe,
+};
+module_platform_driver(rzv2h_wdt_driver);
+MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("Renesas RZ/V2H(P) WDT Watchdog Driver");
