| Message ID | 20240805200400.54267-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Watchdog Timer driver for Renesas RZ/V2H(P) SoC | expand |
On 8/5/24 13:04, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Add Watchdog Timer driver for RZ/V2H(P) SoC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > 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@bp.renesas.com>"); > +MODULE_DESCRIPTION("Renesas RZ/V2H(P) WDT Watchdog Driver");
Hi Guenter, Thank you for the review. On Mon, Aug 5, 2024 at 9:34 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 8/5/24 13:04, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Add Watchdog Timer driver for RZ/V2H(P) SoC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > 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. > As per the HW manual we have, "After FFh is written to the WDT Refresh Register (WDTRR), refreshing the down-counter requires up to 4 cycles of the signal for counting. To meet this requirement, complete writing FFh to WDTRR 4 count cycles before the ms", which I misread that delay is required as 4 cycles are required to reflect, but this isnt the case we can return as soon as FFh is written and let the HW to start down-counter after 4 cycles. So Ive now updated the code to drop any delay in ping operation. > > + 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. > The down counting operation starts only after the ping operation, so after starting the wdt a ping is issued here. > > + 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 ? > Ideally this should be the reverse of start operation, hence stopping the clocks first and then the assert operation. I did this because there is a temporary halt of clock after de-assert operation. To handle this now I ve update start and stop callbacks to below code: static int rzv2h_wdt_start(struct watchdog_device *wdev) { struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev); int ret; ret = pm_runtime_resume_and_get(wdev->parent); if (ret) return ret; ret = reset_control_deassert(priv->rstc); if (ret) { pm_runtime_put(wdev->parent); return ret; } /* delay to handle clock halt after de-assert operation */ udelay(3); ..... ..... return 0; } static int rzv2h_wdt_stop(struct watchdog_device *wdev) { struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev); int ret; ret = reset_control_assert(priv->rstc); if (ret) return ret; ret = pm_runtime_put(wdev->parent); if (ret < 0) return ret; return 0; } > > + > > + 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. > When using the below from user space, while the watchdog service is not running, the below directly lands into restart callback, (please let me know if my understanding is wrong here) reboot(LINUX_REBOOT_CMD_RESTART); Ive now updated restart with below, so that we dont touch clocks if they are already ON, if (!watchdog_active(wdev)) { ret = clk_enable(priv->pclk); if (ret) return ret; ret = clk_enable(priv->oscclk); if (ret) { clk_disable(priv->pclk); return ret; } } if (!watchdog_active(wdev)) ret = reset_control_deassert(priv->rstc); else ret = reset_control_reset(priv->rstc); if (ret) { clk_disable(priv->oscclk); clk_disable(priv->pclk); return ret; } /* delay to handle clock halt after de-assert operation */ udelay(3); > > + /* > > + * 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 ? > The down counting starts after the refresh operation, hence the WDT is pinged. > > + /* wait for underflow to trigger... */ > > + mdelay(500); > > Does that really take 500 ms ? > Agreed this can be reduced as the WDT is configured to trigger asap. > > + > > + 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 ? > Agreed, I will update the above. > Also, what is the value of storing oscclk_rate instead of the calculated delay > in the private data structure ? > Agreed, as mentioned above I no longer need storing the oscclk_rate in priv structure as I have dropped adding delay in ping operation. Cheers, Prabhakar
On 8/6/24 06:47, Lad, Prabhakar wrote: > Hi Guenter, > ... >>> + /* >>> + * 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. >> > The down counting operation starts only after the ping operation, so > after starting the wdt a ping is issued here. > Please add that as comment to the code. ... > Ive now updated restart with below, so that we dont touch clocks if > they are already ON, > > if (!watchdog_active(wdev)) { > ret = clk_enable(priv->pclk); > if (ret) > return ret; > > ret = clk_enable(priv->oscclk); > if (ret) { > clk_disable(priv->pclk); > return ret; > } > } > > if (!watchdog_active(wdev)) > ret = reset_control_deassert(priv->rstc); > else > ret = reset_control_reset(priv->rstc); Please rearrange to only require a single "if (!watchdog_active())". Also, please add a comment explaining the need for calling reset_control_reset() if the watchdog is active. > if (ret) { > clk_disable(priv->oscclk); > clk_disable(priv->pclk); > return ret; > } > > /* delay to handle clock halt after de-assert operation */ > udelay(3); > > >>> + /* >>> + * 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 ? >> > The down counting starts after the refresh operation, hence the WDT is pinged. > Should be covered with the explanation in rzv2h_wdt_start(). Thanks, Guenter
Hi Guenter, On Tue, Aug 6, 2024 at 3:03 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 8/6/24 06:47, Lad, Prabhakar wrote: > > Hi Guenter, > > > ... > >>> + /* > >>> + * 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. > >> > > The down counting operation starts only after the ping operation, so > > after starting the wdt a ping is issued here. > > > > Please add that as comment to the code. > Sure, I will add the below comment: /* * Down counting starts after writing the sequence 00h -> FFh to the * WDTRR register. Hence, call the ping operation after loading the counter */ > ... > > > Ive now updated restart with below, so that we dont touch clocks if > > they are already ON, > > > > if (!watchdog_active(wdev)) { > > ret = clk_enable(priv->pclk); > > if (ret) > > return ret; > > > > ret = clk_enable(priv->oscclk); > > if (ret) { > > clk_disable(priv->pclk); > > return ret; > > } > > } > > > > if (!watchdog_active(wdev)) > > ret = reset_control_deassert(priv->rstc); > > else > > ret = reset_control_reset(priv->rstc); > > Please rearrange to only require a single "if (!watchdog_active())". > Also, please add a comment explaining the need for calling > reset_control_reset() if the watchdog is active. > Sure I will rearrange the code and add the below comment on why reset operation is required when wdt is active, /* * Writing to the WDT Control Register (WDTCR) or WDT Reset * Control Register (WDTRCR) is possible once between the * release from the reset state and the first refresh operation. * so issue a reset if watchdog is active. * Therefore, issue a reset if the watchdog is active. */ > > if (ret) { > > clk_disable(priv->oscclk); > > clk_disable(priv->pclk); > > return ret; > > } > > > > /* delay to handle clock halt after de-assert operation */ > > udelay(3); > > > > > >>> + /* > >>> + * 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 ? > >> > > The down counting starts after the refresh operation, hence the WDT is pinged. > > > > Should be covered with the explanation in rzv2h_wdt_start(). > OK Cheers, Prabhakar
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 + */ + if (!delay) + delay = 4 * div64_ul(CLOCK_DIV_BY_256 * MICRO, priv->oscclk_rate); + 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); + + 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; + + 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; + } + + /* + * 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); + + /* wait for underflow to trigger... */ + mdelay(500); + + 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, NULL); + 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; + 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@bp.renesas.com>"); +MODULE_DESCRIPTION("Renesas RZ/V2H(P) WDT Watchdog Driver");