diff mbox series

[v3,2/2] watchdog: Add Watchdog Timer driver for RZ/V2H(P)

Message ID 20240805200400.54267-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State New
Headers show
Series Add Watchdog Timer driver for Renesas RZ/V2H(P) SoC | expand

Commit Message

Lad, Prabhakar Aug. 5, 2024, 8:04 p.m. UTC
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

Comments

Guenter Roeck Aug. 5, 2024, 8:34 p.m. UTC | #1
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");
Lad, Prabhakar Aug. 6, 2024, 1:47 p.m. UTC | #2
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
Guenter Roeck Aug. 6, 2024, 2:03 p.m. UTC | #3
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
Lad, Prabhakar Aug. 6, 2024, 3:48 p.m. UTC | #4
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 mbox series

Patch

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");