diff mbox series

[v4,2/2] watchdog: Add Watchdog Timer driver for RZ/G2L

Message ID 20211130114316.16622-3-biju.das.jz@bp.renesas.com
State Superseded
Headers show
Series Add WDT driver for RZ/G2L | expand

Commit Message

Biju Das Nov. 30, 2021, 11:43 a.m. UTC
Add Watchdog Timer driver for RZ/G2L SoC.

WDT IP block supports normal watchdog timer function and reset
request function due to CPU parity error.

This driver currently supports normal watchdog timer function
and later will add support for reset request function due to
CPU parity error.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
V3->V4:
 * Fixed the build issue reported by kernel test robot by Replacing the
   macro WDT_CYCLE_MSEC with div64_ul for 64-bit division to fix 32-bit
   kernels.
V2->V3:
 * Added Rb tag from Guenter Roeck
V1->V2:
 * started using clk_get/put instead of devm_clk_get/put
 * Moved devm_add_action_or_reset after set_drvdata() and 
 * removed redundant action on devm_add_action_or_reset() failure.
RFC->V1
 * Removed pclk_rate from priv.
 * rzg2l_wdt_write() returns void and Removed tiemout related to register update 
 * rzg2l_wdt_init_timeout() returns void and removed delays.
 * removed set_bit(WDOG_HW_RUNNING,..) as we can stop watchdog
 * renamed reset_assert_clock_disable->reset_assert_pm_disable_put
 * started using devm_reset_control_get_exclusive()
 * removed platform_set_drvdata(pdev, priv) as there is no user
 * removed watchdog_set_restart_priority(&priv->wdev, 0) as 0 is the default.
 * removed remove callback as it is empty.
---
 drivers/watchdog/Kconfig     |   8 ++
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/rzg2l_wdt.c | 260 +++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/watchdog/rzg2l_wdt.c

Comments

Biju Das Nov. 30, 2021, 5:48 p.m. UTC | #1
Hi Guenter,

Thanks for the feedback.

> Subject: Re: [PATCH v4 2/2] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> On 11/30/21 3:43 AM, Biju Das wrote:
> > Add Watchdog Timer driver for RZ/G2L SoC.
> >
> > WDT IP block supports normal watchdog timer function and reset request
> > function due to CPU parity error.
> >
> > This driver currently supports normal watchdog timer function and
> > later will add support for reset request function due to CPU parity
> > error.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > V3->V4:
> >   * Fixed the build issue reported by kernel test robot by Replacing the
> >     macro WDT_CYCLE_MSEC with div64_ul for 64-bit division to fix 32-bit
> >     kernels.
> > V2->V3:
> >   * Added Rb tag from Guenter Roeck
> > V1->V2:
> >   * started using clk_get/put instead of devm_clk_get/put
> >   * Moved devm_add_action_or_reset after set_drvdata() and
> >   * removed redundant action on devm_add_action_or_reset() failure.
> > RFC->V1
> >   * Removed pclk_rate from priv.
> >   * rzg2l_wdt_write() returns void and Removed tiemout related to
> register update
> >   * rzg2l_wdt_init_timeout() returns void and removed delays.
> >   * removed set_bit(WDOG_HW_RUNNING,..) as we can stop watchdog
> >   * renamed reset_assert_clock_disable->reset_assert_pm_disable_put
> >   * started using devm_reset_control_get_exclusive()
> >   * removed platform_set_drvdata(pdev, priv) as there is no user
> >   * removed watchdog_set_restart_priority(&priv->wdev, 0) as 0 is the
> default.
> >   * removed remove callback as it is empty.
> > ---
> >   drivers/watchdog/Kconfig     |   8 ++
> >   drivers/watchdog/Makefile    |   1 +
> >   drivers/watchdog/rzg2l_wdt.c | 260 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 269 insertions(+)
> >   create mode 100644 drivers/watchdog/rzg2l_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > 9d222ba17ec6..4760ee981263 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -881,6 +881,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_RZG2LWDT
> > +	tristate "Renesas RZ/G2L WDT Watchdog"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	select WATCHDOG_CORE
> > +	help
> > +	  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 ASPEED_WATCHDOG
> >   	tristate "Aspeed BMC watchdog support"
> >   	depends on ARCH_ASPEED || COMPILE_TEST diff --git
> > a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
> > 2ee97064145b..9a3dc0bd271b 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_RZG2LWDT) += rzg2l_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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c new file
> > mode 100644 index 000000000000..69530b92fff9
> > --- /dev/null
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -0,0 +1,260 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L WDT Watchdog Driver
> > + *
> > + * Copyright (C) 2021 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/watchdog.h>
> > +
> > +#define WDTCNT		0x00
> > +#define WDTSET		0x04
> > +#define WDTTIM		0x08
> > +#define WDTINT		0x0C
> > +#define WDTCNT_WDTEN	BIT(0)
> > +#define WDTINT_INTDISP	BIT(0)
> > +
> > +#define WDT_DEFAULT_TIMEOUT		60U
> > +
> > +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> > +#define WDTSET_COUNTER_MASK		(0xFFF00000)
> > +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
> > +
> > +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
> > +
> > +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 rzg2l_wdt_priv {
> > +	void __iomem *base;
> > +	struct watchdog_device wdev;
> > +	struct reset_control *rstc;
> > +	unsigned long osc_clk_rate;
> > +	unsigned long delay;
> > +};
> > +
> > +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) {
> > +	/* delay timer when change the setting register */
> > +	ndelay(priv->delay);
> > +}
> > +
> > +static u32 rzg2l_wdt_get_cycle_msec(unsigned long cycle, u32 wdttime)
> > +{
> > +	u64 timer_cycle_ms = 1024 * 1024 * 1000000ULL * (wdttime + 1);
> > +
> > +	return div64_ul(timer_cycle_ms, cycle); }
> 
> A description might be warranted here. The return value appears to be a
> timeout in seconds, based on
> 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-
> >osc_clk_rate, 0xfff); but that is not what the function name suggests.
> 
> Also, the maximum timeouts seem to be excessive. Feeding the above
> function into test code, I get
> 

As per HW manual,
24MHz is our SoC input clock,

Equation is 1024 *1024 * (t +1)  /(24 * 100000) seconds.

Min value of 0 corresponds to .4369sec, if you convert it into microseconds it becomes 0.43690x1000000 = 436906 microseconds

Max value of 0xfff corresponds to 1789956.97 msec, if you convert it into microseconds it becomes 1789956970 = 0x6AB0936A , I agree it is a big value, but it is with in 32bit limits.

> clk: 10000000 max timeout: 429496729
> clk: 20000000 max timeout: 214748364
> clk: 40000000 max timeout: 107374182
> clk: 80000000 max timeout: 53687091
> clk: 160000000 max timeout: 26843545
> clk: 320000000 max timeout: 13421772
> clk: 640000000 max timeout: 6710886
> clk: 1280000000 max timeout: 3355443
> 
> That really doesn't look correct. Even in milli-seconds, a maximum timeout
> of 429496729 ms or 429496.729 seconds at 10 MHz clock rate seems high.
> 
> > +
> > +static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val,
> > +unsigned int reg) {
> > +	if (reg == WDTSET)
> > +		val &= WDTSET_COUNTER_MASK;
> > +
> > +	writel_relaxed(val, priv->base + reg);
> > +	/* Registers other than the WDTINT is always synchronized with
> WDT_CLK */
> > +	if (reg != WDTINT)
> > +		rzg2l_wdt_wait_delay(priv);
> > +}
> > +
> > +static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u32 time_out;
> > +
> > +	/* Clear Lapsed Time Register and clear Interrupt */
> > +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> > +	/* 2 consecutive overflow cycle needed to trigger reset */
> > +	time_out = (wdev->timeout / 2 * 1000000) /
> > +rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0);
> 
> This code effectively reduces timer granularity to 2 seconds. Is that on
> purpose ?

Yes, it needs 2 consecutive overflow cycle for triggering watchdog reset

As per the above calculation,

60 secs Default timeout, the counter value = 30000000 microsec/436906 microsec = 686

And it triggers watchdog around 60 sec with the command
cat /dev/watchdog  & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800808; done

> Why not something like
> 	time_out = (wdev->timeout * (1000000 / 2)) /
> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0); instead ?


Ok.

> 
> Also, feeding the maximum timeout as calculated by
> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0xfff) into this expression
> yields really large numbers. Making things worse, those long timeouts
> cause
> 	wdev->timeout / 2 * 1000000
> to overflow easily. This calculation alone suggests that the maximum
> timeout value can not be larger than ~8589 to avoid that overflow.
> 
> More test code gives me:
> 
> clk: 10000000 max timeout: 429496729
>    timeout: 1s reg: 0x0
>    timeout: 2s reg: 0x9
>    timeout: 859s reg: 0xffb
>      Overflow: t=214748364 treg: 0x3d0916df
>    timeout: 214748364s reg: 0xffffffff
>      Overflow: t=429496729 treg: 0x7a122dbf
>    timeout: 429496729s reg: 0xffffffff
> 
> clk: 320000000 max timeout: 13421772
>    timeout: 1s reg: 0x0
>    timeout: 2s reg: 0x131
>    timeout: 27s reg: 0xf80
>      Overflow: t=6710886 treg: 0x3d0cd090
>    timeout: 6710886s reg: 0xffffffff
>      Overflow: t=13421772 treg: 0x7a19a120
>    timeout: 13421772s reg: 0xffffffff
> 
> and similar for other clock rates. This shows both the impact of the
> artificial 2s granularity and the value overflows.
> 
> Something in the calculation of max_timeout or in the calculation of the
> register value or both is wrong. Whatever it is, it needs to get fixed.

I believe microsecond calculation leads to the confusion.

Regards,
Biju


> > +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(time_out), WDTSET); }
> > +
> > +static int rzg2l_wdt_start(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	reset_control_deassert(priv->rstc);
> > +	pm_runtime_get_sync(wdev->parent);
> > +
> > +	/* Initialize time out */
> > +	rzg2l_wdt_init_timeout(wdev);
> > +
> > +	/* Initialize watchdog counter register */
> > +	rzg2l_wdt_write(priv, 0, WDTTIM);
> > +
> > +	/* Enable watchdog timer*/
> > +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_wdt_stop(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	pm_runtime_put(wdev->parent);
> > +	reset_control_assert(priv->rstc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> > +			     unsigned long action, void *data) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	/* Reset the module before we modify any register */
> > +	reset_control_reset(priv->rstc);
> > +	pm_runtime_get_sync(wdev->parent);
> > +
> > +	/* smallest counter value to reboot soon */
> > +	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> > +
> > +	/* Enable watchdog timer*/
> > +	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info rzg2l_wdt_ident = {
> > +	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> WDIOF_SETTIMEOUT,
> > +	.identity = "Renesas RZ/G2L WDT Watchdog", };
> > +
> > +static int rzg2l_wdt_ping(struct watchdog_device *wdev) {
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_ops rzg2l_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = rzg2l_wdt_start,
> > +	.stop = rzg2l_wdt_stop,
> > +	.ping = rzg2l_wdt_ping,
> > +	.restart = rzg2l_wdt_restart,
> > +};
> > +
> > +static void rzg2l_wdt_reset_assert_pm_disable_put(void *data) {
> > +	struct watchdog_device *wdev = data;
> > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	pm_runtime_put(wdev->parent);
> > +	pm_runtime_disable(wdev->parent);
> > +	reset_control_assert(priv->rstc);
> > +}
> > +
> > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct rzg2l_wdt_priv *priv;
> > +	unsigned long pclk_rate;
> > +	struct clk *wdt_clk;
> > +	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);
> > +
> > +	/* Get watchdog main clock */
> > +	wdt_clk = clk_get(&pdev->dev, "oscclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> oscclk");
> > +
> > +	priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > +	clk_put(wdt_clk);
> > +	if (!priv->osc_clk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> > +
> > +	/* Get Peripheral clock */
> > +	wdt_clk = clk_get(&pdev->dev, "pclk");
> > +	if (IS_ERR(wdt_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> > +
> > +	pclk_rate = clk_get_rate(wdt_clk);
> > +	clk_put(wdt_clk);
> > +	if (!pclk_rate)
> > +		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> > +
> > +	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 +
> > +F2CYCLE_NSEC(pclk_rate) * 9;
> > +
> > +	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");
> > +
> > +	reset_control_deassert(priv->rstc);
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe",
> ERR_PTR(ret));
> > +		goto out_pm_get;
> > +	}
> > +
> > +	priv->wdev.info = &rzg2l_wdt_ident;
> > +	priv->wdev.ops = &rzg2l_wdt_ops;
> > +	priv->wdev.parent = dev;
> > +	priv->wdev.min_timeout = 1;
> > +	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-
> >osc_clk_rate, 0xfff);
> > +	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> > +
> > +	watchdog_set_drvdata(&priv->wdev, priv);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rzg2l_wdt_reset_assert_pm_disable_put,
> > +				       &priv->wdev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	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);
> > +
> > +out_pm_get:
> > +	pm_runtime_disable(dev);
> > +	reset_control_assert(priv->rstc);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id rzg2l_wdt_ids[] = {
> > +	{ .compatible = "renesas,rzg2l-wdt", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
> > +
> > +static struct platform_driver rzg2l_wdt_driver = {
> > +	.driver = {
> > +		.name = "rzg2l_wdt",
> > +		.of_match_table = rzg2l_wdt_ids,
> > +	},
> > +	.probe = rzg2l_wdt_probe,
> > +};
> > +module_platform_driver(rzg2l_wdt_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/G2L WDT Watchdog Driver");
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_LICENSE("GPL v2");
> >
Biju Das Nov. 30, 2021, 5:56 p.m. UTC | #2
> Subject: RE: [PATCH v4 2/2] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> Hi Guenter,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v4 2/2] watchdog: Add Watchdog Timer driver for
> > RZ/G2L
> >
> > On 11/30/21 3:43 AM, Biju Das wrote:
> > > Add Watchdog Timer driver for RZ/G2L SoC.
> > >
> > > WDT IP block supports normal watchdog timer function and reset
> > > request function due to CPU parity error.
> > >
> > > This driver currently supports normal watchdog timer function and
> > > later will add support for reset request function due to CPU parity
> > > error.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > V3->V4:
> > >   * Fixed the build issue reported by kernel test robot by Replacing
> the
> > >     macro WDT_CYCLE_MSEC with div64_ul for 64-bit division to fix 32-
> bit
> > >     kernels.
> > > V2->V3:
> > >   * Added Rb tag from Guenter Roeck
> > > V1->V2:
> > >   * started using clk_get/put instead of devm_clk_get/put
> > >   * Moved devm_add_action_or_reset after set_drvdata() and
> > >   * removed redundant action on devm_add_action_or_reset() failure.
> > > RFC->V1
> > >   * Removed pclk_rate from priv.
> > >   * rzg2l_wdt_write() returns void and Removed tiemout related to
> > register update
> > >   * rzg2l_wdt_init_timeout() returns void and removed delays.
> > >   * removed set_bit(WDOG_HW_RUNNING,..) as we can stop watchdog
> > >   * renamed reset_assert_clock_disable->reset_assert_pm_disable_put
> > >   * started using devm_reset_control_get_exclusive()
> > >   * removed platform_set_drvdata(pdev, priv) as there is no user
> > >   * removed watchdog_set_restart_priority(&priv->wdev, 0) as 0 is
> > > the
> > default.
> > >   * removed remove callback as it is empty.
> > > ---
> > >   drivers/watchdog/Kconfig     |   8 ++
> > >   drivers/watchdog/Makefile    |   1 +
> > >   drivers/watchdog/rzg2l_wdt.c | 260
> +++++++++++++++++++++++++++++++++++
> > >   3 files changed, 269 insertions(+)
> > >   create mode 100644 drivers/watchdog/rzg2l_wdt.c
> > >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index
> > > 9d222ba17ec6..4760ee981263 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -881,6 +881,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_RZG2LWDT
> > > +	tristate "Renesas RZ/G2L WDT Watchdog"
> > > +	depends on ARCH_RENESAS || COMPILE_TEST
> > > +	select WATCHDOG_CORE
> > > +	help
> > > +	  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 ASPEED_WATCHDOG
> > >   	tristate "Aspeed BMC watchdog support"
> > >   	depends on ARCH_ASPEED || COMPILE_TEST diff --git
> > > a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
> > > 2ee97064145b..9a3dc0bd271b 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_RZG2LWDT) += rzg2l_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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c new
> > > file mode 100644 index 000000000000..69530b92fff9
> > > --- /dev/null
> > > +++ b/drivers/watchdog/rzg2l_wdt.c
> > > @@ -0,0 +1,260 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Renesas RZ/G2L WDT Watchdog Driver
> > > + *
> > > + * Copyright (C) 2021 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/watchdog.h>
> > > +
> > > +#define WDTCNT		0x00
> > > +#define WDTSET		0x04
> > > +#define WDTTIM		0x08
> > > +#define WDTINT		0x0C
> > > +#define WDTCNT_WDTEN	BIT(0)
> > > +#define WDTINT_INTDISP	BIT(0)
> > > +
> > > +#define WDT_DEFAULT_TIMEOUT		60U
> > > +
> > > +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> > > +#define WDTSET_COUNTER_MASK		(0xFFF00000)
> > > +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
> > > +
> > > +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
> > > +
> > > +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 rzg2l_wdt_priv {
> > > +	void __iomem *base;
> > > +	struct watchdog_device wdev;
> > > +	struct reset_control *rstc;
> > > +	unsigned long osc_clk_rate;
> > > +	unsigned long delay;
> > > +};
> > > +
> > > +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) {
> > > +	/* delay timer when change the setting register */
> > > +	ndelay(priv->delay);
> > > +}
> > > +
> > > +static u32 rzg2l_wdt_get_cycle_msec(unsigned long cycle, u32
> > > +wdttime) {
> > > +	u64 timer_cycle_ms = 1024 * 1024 * 1000000ULL * (wdttime + 1);
> > > +
> > > +	return div64_ul(timer_cycle_ms, cycle); }
> >
> > A description might be warranted here. The return value appears to be
> > a timeout in seconds, based on
> > 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-
> > >osc_clk_rate, 0xfff); but that is not what the function name suggests.
> >
> > Also, the maximum timeouts seem to be excessive. Feeding the above
> > function into test code, I get
> >
> 
> As per HW manual,
> 24MHz is our SoC input clock,
> 
> Equation is 1024 *1024 * (t +1)  /(24 * 100000) seconds.
> 
> Min value of 0 corresponds to .4369sec, if you convert it into
> microseconds it becomes 0.43690x1000000 = 436906 microseconds
> 
> Max value of 0xfff corresponds to 1789956.97 msec, if you convert it into
> microseconds it becomes 1789956970 = 0x6AB0936A , I agree it is a big
> value, but it is with in 32bit limits.
> 
> > clk: 10000000 max timeout: 429496729
> > clk: 20000000 max timeout: 214748364
> > clk: 40000000 max timeout: 107374182
> > clk: 80000000 max timeout: 53687091
> > clk: 160000000 max timeout: 26843545
> > clk: 320000000 max timeout: 13421772
> > clk: 640000000 max timeout: 6710886
> > clk: 1280000000 max timeout: 3355443
> >
> > That really doesn't look correct. Even in milli-seconds, a maximum
> > timeout of 429496729 ms or 429496.729 seconds at 10 MHz clock rate seems
> high.
> >
> > > +
> > > +static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val,
> > > +unsigned int reg) {
> > > +	if (reg == WDTSET)
> > > +		val &= WDTSET_COUNTER_MASK;
> > > +
> > > +	writel_relaxed(val, priv->base + reg);
> > > +	/* Registers other than the WDTINT is always synchronized with
> > WDT_CLK */
> > > +	if (reg != WDTINT)
> > > +		rzg2l_wdt_wait_delay(priv);
> > > +}
> > > +
> > > +static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) {
> > > +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > > +	u32 time_out;
> > > +
> > > +	/* Clear Lapsed Time Register and clear Interrupt */
> > > +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> > > +	/* 2 consecutive overflow cycle needed to trigger reset */
> > > +	time_out = (wdev->timeout / 2 * 1000000) /
> > > +rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0);
> >
> > This code effectively reduces timer granularity to 2 seconds. Is that
> > on purpose ?
> 
> Yes, it needs 2 consecutive overflow cycle for triggering watchdog reset
> 
> As per the above calculation,
> 
> 60 secs Default timeout, the counter value = 30000000 microsec/436906
> microsec = 686
> 
> And it triggers watchdog around 60 sec with the command cat /dev/watchdog
> & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800808; done
> 
> > Why not something like
> > 	time_out = (wdev->timeout * (1000000 / 2)) /
> > rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0); instead ?
> 
> 
> Ok.
> 
> >
> > Also, feeding the maximum timeout as calculated by
> > rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0xfff) into this
> > expression yields really large numbers. Making things worse, those
> > long timeouts cause
> > 	wdev->timeout / 2 * 1000000
> > to overflow easily. This calculation alone suggests that the maximum
> > timeout value can not be larger than ~8589 to avoid that overflow.
> >
> > More test code gives me:
> >
> > clk: 10000000 max timeout: 429496729
> >    timeout: 1s reg: 0x0
> >    timeout: 2s reg: 0x9
> >    timeout: 859s reg: 0xffb
> >      Overflow: t=214748364 treg: 0x3d0916df
> >    timeout: 214748364s reg: 0xffffffff
> >      Overflow: t=429496729 treg: 0x7a122dbf
> >    timeout: 429496729s reg: 0xffffffff
> >
> > clk: 320000000 max timeout: 13421772
> >    timeout: 1s reg: 0x0
> >    timeout: 2s reg: 0x131
> >    timeout: 27s reg: 0xf80
> >      Overflow: t=6710886 treg: 0x3d0cd090
> >    timeout: 6710886s reg: 0xffffffff
> >      Overflow: t=13421772 treg: 0x7a19a120
> >    timeout: 13421772s reg: 0xffffffff
> >
> > and similar for other clock rates. This shows both the impact of the
> > artificial 2s granularity and the value overflows.
> >
> > Something in the calculation of max_timeout or in the calculation of
> > the register value or both is wrong. Whatever it is, it needs to get
> fixed.

Yes, you are right, calculation of max_timeout is wrong. Currently it is microseconds
Needs to convert it into seconds.

Cheers,
Biju
Guenter Roeck Nov. 30, 2021, 6:17 p.m. UTC | #3
On 11/30/21 9:48 AM, Biju Das wrote:
> Hi Guenter,
> 
> Thanks for the feedback.
> 
>> Subject: Re: [PATCH v4 2/2] watchdog: Add Watchdog Timer driver for RZ/G2L
>>
>> On 11/30/21 3:43 AM, Biju Das wrote:
>>> Add Watchdog Timer driver for RZ/G2L SoC.
>>>
>>> WDT IP block supports normal watchdog timer function and reset request
>>> function due to CPU parity error.
>>>
>>> This driver currently supports normal watchdog timer function and
>>> later will add support for reset request function due to CPU parity
>>> error.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> V3->V4:
>>>    * Fixed the build issue reported by kernel test robot by Replacing the
>>>      macro WDT_CYCLE_MSEC with div64_ul for 64-bit division to fix 32-bit
>>>      kernels.
>>> V2->V3:
>>>    * Added Rb tag from Guenter Roeck
>>> V1->V2:
>>>    * started using clk_get/put instead of devm_clk_get/put
>>>    * Moved devm_add_action_or_reset after set_drvdata() and
>>>    * removed redundant action on devm_add_action_or_reset() failure.
>>> RFC->V1
>>>    * Removed pclk_rate from priv.
>>>    * rzg2l_wdt_write() returns void and Removed tiemout related to
>> register update
>>>    * rzg2l_wdt_init_timeout() returns void and removed delays.
>>>    * removed set_bit(WDOG_HW_RUNNING,..) as we can stop watchdog
>>>    * renamed reset_assert_clock_disable->reset_assert_pm_disable_put
>>>    * started using devm_reset_control_get_exclusive()
>>>    * removed platform_set_drvdata(pdev, priv) as there is no user
>>>    * removed watchdog_set_restart_priority(&priv->wdev, 0) as 0 is the
>> default.
>>>    * removed remove callback as it is empty.
>>> ---
>>>    drivers/watchdog/Kconfig     |   8 ++
>>>    drivers/watchdog/Makefile    |   1 +
>>>    drivers/watchdog/rzg2l_wdt.c | 260 +++++++++++++++++++++++++++++++++++
>>>    3 files changed, 269 insertions(+)
>>>    create mode 100644 drivers/watchdog/rzg2l_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> 9d222ba17ec6..4760ee981263 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -881,6 +881,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_RZG2LWDT
>>> +	tristate "Renesas RZ/G2L WDT Watchdog"
>>> +	depends on ARCH_RENESAS || COMPILE_TEST
>>> +	select WATCHDOG_CORE
>>> +	help
>>> +	  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 ASPEED_WATCHDOG
>>>    	tristate "Aspeed BMC watchdog support"
>>>    	depends on ARCH_ASPEED || COMPILE_TEST diff --git
>>> a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
>>> 2ee97064145b..9a3dc0bd271b 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_RZG2LWDT) += rzg2l_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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c new file
>>> mode 100644 index 000000000000..69530b92fff9
>>> --- /dev/null
>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>> @@ -0,0 +1,260 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Renesas RZ/G2L WDT Watchdog Driver
>>> + *
>>> + * Copyright (C) 2021 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/watchdog.h>
>>> +
>>> +#define WDTCNT		0x00
>>> +#define WDTSET		0x04
>>> +#define WDTTIM		0x08
>>> +#define WDTINT		0x0C
>>> +#define WDTCNT_WDTEN	BIT(0)
>>> +#define WDTINT_INTDISP	BIT(0)
>>> +
>>> +#define WDT_DEFAULT_TIMEOUT		60U
>>> +
>>> +/* Setting period time register only 12 bit set in WDTSET[31:20] */
>>> +#define WDTSET_COUNTER_MASK		(0xFFF00000)
>>> +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
>>> +
>>> +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
>>> +
>>> +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 rzg2l_wdt_priv {
>>> +	void __iomem *base;
>>> +	struct watchdog_device wdev;
>>> +	struct reset_control *rstc;
>>> +	unsigned long osc_clk_rate;
>>> +	unsigned long delay;
>>> +};
>>> +
>>> +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) {
>>> +	/* delay timer when change the setting register */
>>> +	ndelay(priv->delay);
>>> +}
>>> +
>>> +static u32 rzg2l_wdt_get_cycle_msec(unsigned long cycle, u32 wdttime)
>>> +{
>>> +	u64 timer_cycle_ms = 1024 * 1024 * 1000000ULL * (wdttime + 1);
>>> +
>>> +	return div64_ul(timer_cycle_ms, cycle); }
>>
>> A description might be warranted here. The return value appears to be a
>> timeout in seconds, based on
>> 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-
>>> osc_clk_rate, 0xfff); but that is not what the function name suggests.
>>
>> Also, the maximum timeouts seem to be excessive. Feeding the above
>> function into test code, I get
>>
> 
> As per HW manual,
> 24MHz is our SoC input clock,
> 
> Equation is 1024 *1024 * (t +1)  /(24 * 100000) seconds.
> 
> Min value of 0 corresponds to .4369sec, if you convert it into microseconds it becomes 0.43690x1000000 = 436906 microseconds
> 
> Max value of 0xfff corresponds to 1789956.97 msec, if you convert it into microseconds it becomes 1789956970 = 0x6AB0936A , I agree it is a big value, but it is with in 32bit limits.
> 

max_timeout is in seconds, not microseconds, and the function has _msec
in its name which doesn't reflect anything micro.

>> clk: 10000000 max timeout: 429496729
>> clk: 20000000 max timeout: 214748364
>> clk: 40000000 max timeout: 107374182
>> clk: 80000000 max timeout: 53687091
>> clk: 160000000 max timeout: 26843545
>> clk: 320000000 max timeout: 13421772
>> clk: 640000000 max timeout: 6710886
>> clk: 1280000000 max timeout: 3355443
>>
>> That really doesn't look correct. Even in milli-seconds, a maximum timeout
>> of 429496729 ms or 429496.729 seconds at 10 MHz clock rate seems high.
>>
>>> +
>>> +static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val,
>>> +unsigned int reg) {
>>> +	if (reg == WDTSET)
>>> +		val &= WDTSET_COUNTER_MASK;
>>> +
>>> +	writel_relaxed(val, priv->base + reg);
>>> +	/* Registers other than the WDTINT is always synchronized with
>> WDT_CLK */
>>> +	if (reg != WDTINT)
>>> +		rzg2l_wdt_wait_delay(priv);
>>> +}
>>> +
>>> +static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) {
>>> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +	u32 time_out;
>>> +
>>> +	/* Clear Lapsed Time Register and clear Interrupt */
>>> +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
>>> +	/* 2 consecutive overflow cycle needed to trigger reset */
>>> +	time_out = (wdev->timeout / 2 * 1000000) /
>>> +rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0);
>>
>> This code effectively reduces timer granularity to 2 seconds. Is that on
>> purpose ?
> 
> Yes, it needs 2 consecutive overflow cycle for triggering watchdog reset
> 
> As per the above calculation,
> 
> 60 secs Default timeout, the counter value = 30000000 microsec/436906 microsec = 686
> 
> And it triggers watchdog around 60 sec with the command
> cat /dev/watchdog  & for i in {1..60}; do sleep 1; echo $i; devmem2 0x12800808; done
> 
That explains the factor itself, which I did not question. I questioned
the granularity, ie why the timeout was, in practice, set to 0, 2, 4,
... seconds instead of 1, 2, 3, 4 ...

>> Why not something like
>> 	time_out = (wdev->timeout * (1000000 / 2)) /
>> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0); instead ?
> 
> 
> Ok.
> 
>>
>> Also, feeding the maximum timeout as calculated by
>> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0xfff) into this expression
>> yields really large numbers. Making things worse, those long timeouts
>> cause
>> 	wdev->timeout / 2 * 1000000
>> to overflow easily. This calculation alone suggests that the maximum
>> timeout value can not be larger than ~8589 to avoid that overflow.
>>
>> More test code gives me:
>>
>> clk: 10000000 max timeout: 429496729
>>     timeout: 1s reg: 0x0
>>     timeout: 2s reg: 0x9
>>     timeout: 859s reg: 0xffb
>>       Overflow: t=214748364 treg: 0x3d0916df
>>     timeout: 214748364s reg: 0xffffffff
>>       Overflow: t=429496729 treg: 0x7a122dbf
>>     timeout: 429496729s reg: 0xffffffff
>>
>> clk: 320000000 max timeout: 13421772
>>     timeout: 1s reg: 0x0
>>     timeout: 2s reg: 0x131
>>     timeout: 27s reg: 0xf80
>>       Overflow: t=6710886 treg: 0x3d0cd090
>>     timeout: 6710886s reg: 0xffffffff
>>       Overflow: t=13421772 treg: 0x7a19a120
>>     timeout: 13421772s reg: 0xffffffff
>>
>> and similar for other clock rates. This shows both the impact of the
>> artificial 2s granularity and the value overflows.
>>
>> Something in the calculation of max_timeout or in the calculation of the
>> register value or both is wrong. Whatever it is, it needs to get fixed.
> 
> I believe microsecond calculation leads to the confusion.
> 

Again,
	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-osc_clk_rate, 0xfff);

and max_timeout is in seconds, not milliseconds or microseconds.
Maybe it needs to be something like

	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-osc_clk_rate, 0xfff) /
		(1000000 / 2);

instead (that would limit the range of the register value to 0..0xfff), but
even then the function name (rzg2l_wdt_get_cycle_msec) doesn't match
what it returns. Maybe it should be named rzg2l_wdt_get_cycle_usec.
It might also make sense to use USEC_PER_SEC instead of 1000000 to
clarify the context.

Guenter
Biju Das Nov. 30, 2021, 6:29 p.m. UTC | #4
Hi Guenter,

Thanks for the feedback.

> >> Subject: Re: [PATCH v4 2/2] watchdog: Add Watchdog Timer driver for
> >> RZ/G2L
> >>
> >> On 11/30/21 3:43 AM, Biju Das wrote:
> >>> Add Watchdog Timer driver for RZ/G2L SoC.
> >>>
> >>> WDT IP block supports normal watchdog timer function and reset
> >>> request function due to CPU parity error.
> >>>
> >>> This driver currently supports normal watchdog timer function and
> >>> later will add support for reset request function due to CPU parity
> >>> error.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >>> ---
> >>> V3->V4:
> >>>    * Fixed the build issue reported by kernel test robot by Replacing
> the
> >>>      macro WDT_CYCLE_MSEC with div64_ul for 64-bit division to fix 32-
> bit
> >>>      kernels.
> >>> V2->V3:
> >>>    * Added Rb tag from Guenter Roeck
> >>> V1->V2:
> >>>    * started using clk_get/put instead of devm_clk_get/put
> >>>    * Moved devm_add_action_or_reset after set_drvdata() and
> >>>    * removed redundant action on devm_add_action_or_reset() failure.
> >>> RFC->V1
> >>>    * Removed pclk_rate from priv.
> >>>    * rzg2l_wdt_write() returns void and Removed tiemout related to
> >> register update
> >>>    * rzg2l_wdt_init_timeout() returns void and removed delays.
> >>>    * removed set_bit(WDOG_HW_RUNNING,..) as we can stop watchdog
> >>>    * renamed reset_assert_clock_disable->reset_assert_pm_disable_put
> >>>    * started using devm_reset_control_get_exclusive()
> >>>    * removed platform_set_drvdata(pdev, priv) as there is no user
> >>>    * removed watchdog_set_restart_priority(&priv->wdev, 0) as 0 is
> >>> the
> >> default.
> >>>    * removed remove callback as it is empty.
> >>> ---
> >>>    drivers/watchdog/Kconfig     |   8 ++
> >>>    drivers/watchdog/Makefile    |   1 +
> >>>    drivers/watchdog/rzg2l_wdt.c | 260
> +++++++++++++++++++++++++++++++++++
> >>>    3 files changed, 269 insertions(+)
> >>>    create mode 100644 drivers/watchdog/rzg2l_wdt.c
> >>>
> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >>> index
> >>> 9d222ba17ec6..4760ee981263 100644
> >>> --- a/drivers/watchdog/Kconfig
> >>> +++ b/drivers/watchdog/Kconfig
> >>> @@ -881,6 +881,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_RZG2LWDT
> >>> +	tristate "Renesas RZ/G2L WDT Watchdog"
> >>> +	depends on ARCH_RENESAS || COMPILE_TEST
> >>> +	select WATCHDOG_CORE
> >>> +	help
> >>> +	  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 ASPEED_WATCHDOG
> >>>    	tristate "Aspeed BMC watchdog support"
> >>>    	depends on ARCH_ASPEED || COMPILE_TEST diff --git
> >>> a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
> >>> 2ee97064145b..9a3dc0bd271b 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_RZG2LWDT) += rzg2l_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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c new
> >>> file mode 100644 index 000000000000..69530b92fff9
> >>> --- /dev/null
> >>> +++ b/drivers/watchdog/rzg2l_wdt.c
> >>> @@ -0,0 +1,260 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Renesas RZ/G2L WDT Watchdog Driver
> >>> + *
> >>> + * Copyright (C) 2021 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/watchdog.h>
> >>> +
> >>> +#define WDTCNT		0x00
> >>> +#define WDTSET		0x04
> >>> +#define WDTTIM		0x08
> >>> +#define WDTINT		0x0C
> >>> +#define WDTCNT_WDTEN	BIT(0)
> >>> +#define WDTINT_INTDISP	BIT(0)
> >>> +
> >>> +#define WDT_DEFAULT_TIMEOUT		60U
> >>> +
> >>> +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> >>> +#define WDTSET_COUNTER_MASK		(0xFFF00000)
> >>> +#define WDTSET_COUNTER_VAL(f)		((f) << 20)
> >>> +
> >>> +#define F2CYCLE_NSEC(f)			(1000000000 / (f))
> >>> +
> >>> +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 rzg2l_wdt_priv {
> >>> +	void __iomem *base;
> >>> +	struct watchdog_device wdev;
> >>> +	struct reset_control *rstc;
> >>> +	unsigned long osc_clk_rate;
> >>> +	unsigned long delay;
> >>> +};
> >>> +
> >>> +static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) {
> >>> +	/* delay timer when change the setting register */
> >>> +	ndelay(priv->delay);
> >>> +}
> >>> +
> >>> +static u32 rzg2l_wdt_get_cycle_msec(unsigned long cycle, u32
> >>> +wdttime) {
> >>> +	u64 timer_cycle_ms = 1024 * 1024 * 1000000ULL * (wdttime + 1);
> >>> +
> >>> +	return div64_ul(timer_cycle_ms, cycle); }
> >>
> >> A description might be warranted here. The return value appears to be
> >> a timeout in seconds, based on
> >> 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-
> >>> osc_clk_rate, 0xfff); but that is not what the function name suggests.
> >>
> >> Also, the maximum timeouts seem to be excessive. Feeding the above
> >> function into test code, I get
> >>
> >
> > As per HW manual,
> > 24MHz is our SoC input clock,
> >
> > Equation is 1024 *1024 * (t +1)  /(24 * 100000) seconds.
> >
> > Min value of 0 corresponds to .4369sec, if you convert it into
> > microseconds it becomes 0.43690x1000000 = 436906 microseconds
> >
> > Max value of 0xfff corresponds to 1789956.97 msec, if you convert it
> into microseconds it becomes 1789956970 = 0x6AB0936A , I agree it is a big
> value, but it is with in 32bit limits.
> >
> 
> max_timeout is in seconds, not microseconds, and the function has _msec in
> its name which doesn't reflect anything micro.

I agree, it is my mistake, it should be _usec.

> 
> >> clk: 10000000 max timeout: 429496729
> >> clk: 20000000 max timeout: 214748364
> >> clk: 40000000 max timeout: 107374182
> >> clk: 80000000 max timeout: 53687091
> >> clk: 160000000 max timeout: 26843545
> >> clk: 320000000 max timeout: 13421772
> >> clk: 640000000 max timeout: 6710886
> >> clk: 1280000000 max timeout: 3355443
> >>
> >> That really doesn't look correct. Even in milli-seconds, a maximum
> >> timeout of 429496729 ms or 429496.729 seconds at 10 MHz clock rate
> seems high.
> >>
> >>> +
> >>> +static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val,
> >>> +unsigned int reg) {
> >>> +	if (reg == WDTSET)
> >>> +		val &= WDTSET_COUNTER_MASK;
> >>> +
> >>> +	writel_relaxed(val, priv->base + reg);
> >>> +	/* Registers other than the WDTINT is always synchronized with
> >> WDT_CLK */
> >>> +	if (reg != WDTINT)
> >>> +		rzg2l_wdt_wait_delay(priv);
> >>> +}
> >>> +
> >>> +static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev) {
> >>> +	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >>> +	u32 time_out;
> >>> +
> >>> +	/* Clear Lapsed Time Register and clear Interrupt */
> >>> +	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
> >>> +	/* 2 consecutive overflow cycle needed to trigger reset */
> >>> +	time_out = (wdev->timeout / 2 * 1000000) /
> >>> +rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0);
> >>
> >> This code effectively reduces timer granularity to 2 seconds. Is that
> >> on purpose ?
> >
> > Yes, it needs 2 consecutive overflow cycle for triggering watchdog
> > reset
> >
> > As per the above calculation,
> >
> > 60 secs Default timeout, the counter value = 30000000 microsec/436906
> > microsec = 686
> >
> > And it triggers watchdog around 60 sec with the command cat
> > /dev/watchdog  & for i in {1..60}; do sleep 1; echo $i; devmem2
> > 0x12800808; done
> >
> That explains the factor itself, which I did not question. I questioned
> the granularity, ie why the timeout was, in practice, set to 0, 2, 4, ...
> seconds instead of 1, 2, 3, 4 ...

OK.

> 
> >> Why not something like
> >> 	time_out = (wdev->timeout * (1000000 / 2)) /
> >> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0); instead ?
> >
> >
> > Ok.
> >
> >>
> >> Also, feeding the maximum timeout as calculated by
> >> rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0xfff) into this
> >> expression yields really large numbers. Making things worse, those
> >> long timeouts cause
> >> 	wdev->timeout / 2 * 1000000
> >> to overflow easily. This calculation alone suggests that the maximum
> >> timeout value can not be larger than ~8589 to avoid that overflow.
> >>
> >> More test code gives me:
> >>
> >> clk: 10000000 max timeout: 429496729
> >>     timeout: 1s reg: 0x0
> >>     timeout: 2s reg: 0x9
> >>     timeout: 859s reg: 0xffb
> >>       Overflow: t=214748364 treg: 0x3d0916df
> >>     timeout: 214748364s reg: 0xffffffff
> >>       Overflow: t=429496729 treg: 0x7a122dbf
> >>     timeout: 429496729s reg: 0xffffffff
> >>
> >> clk: 320000000 max timeout: 13421772
> >>     timeout: 1s reg: 0x0
> >>     timeout: 2s reg: 0x131
> >>     timeout: 27s reg: 0xf80
> >>       Overflow: t=6710886 treg: 0x3d0cd090
> >>     timeout: 6710886s reg: 0xffffffff
> >>       Overflow: t=13421772 treg: 0x7a19a120
> >>     timeout: 13421772s reg: 0xffffffff
> >>
> >> and similar for other clock rates. This shows both the impact of the
> >> artificial 2s granularity and the value overflows.
> >>
> >> Something in the calculation of max_timeout or in the calculation of
> >> the register value or both is wrong. Whatever it is, it needs to get
> fixed.
> >
> > I believe microsecond calculation leads to the confusion.
> >
> 
> Again,
> 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-osc_clk_rate,
> 0xfff);
> 
> and max_timeout is in seconds, not milliseconds or microseconds.
> Maybe it needs to be something like
> 
> 	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv-osc_clk_rate,
> 0xfff) /
> 		(1000000 / 2);
> 
> instead (that would limit the range of the register value to 0..0xfff),
> but even then the function name (rzg2l_wdt_get_cycle_msec) doesn't match
> what it returns. Maybe it should be named rzg2l_wdt_get_cycle_usec.
> It might also make sense to use USEC_PER_SEC instead of 1000000 to clarify
> the context.

Agreed. Will post v5 with these changes.

Regards,
Biju
diff mbox series

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9d222ba17ec6..4760ee981263 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -881,6 +881,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_RZG2LWDT
+	tristate "Renesas RZ/G2L WDT Watchdog"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  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 ASPEED_WATCHDOG
 	tristate "Aspeed BMC watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee97064145b..9a3dc0bd271b 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_RZG2LWDT) += rzg2l_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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
new file mode 100644
index 000000000000..69530b92fff9
--- /dev/null
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -0,0 +1,260 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L WDT Watchdog Driver
+ *
+ * Copyright (C) 2021 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/watchdog.h>
+
+#define WDTCNT		0x00
+#define WDTSET		0x04
+#define WDTTIM		0x08
+#define WDTINT		0x0C
+#define WDTCNT_WDTEN	BIT(0)
+#define WDTINT_INTDISP	BIT(0)
+
+#define WDT_DEFAULT_TIMEOUT		60U
+
+/* Setting period time register only 12 bit set in WDTSET[31:20] */
+#define WDTSET_COUNTER_MASK		(0xFFF00000)
+#define WDTSET_COUNTER_VAL(f)		((f) << 20)
+
+#define F2CYCLE_NSEC(f)			(1000000000 / (f))
+
+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 rzg2l_wdt_priv {
+	void __iomem *base;
+	struct watchdog_device wdev;
+	struct reset_control *rstc;
+	unsigned long osc_clk_rate;
+	unsigned long delay;
+};
+
+static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
+{
+	/* delay timer when change the setting register */
+	ndelay(priv->delay);
+}
+
+static u32 rzg2l_wdt_get_cycle_msec(unsigned long cycle, u32 wdttime)
+{
+	u64 timer_cycle_ms = 1024 * 1024 * 1000000ULL * (wdttime + 1);
+
+	return div64_ul(timer_cycle_ms, cycle);
+}
+
+static void rzg2l_wdt_write(struct rzg2l_wdt_priv *priv, u32 val, unsigned int reg)
+{
+	if (reg == WDTSET)
+		val &= WDTSET_COUNTER_MASK;
+
+	writel_relaxed(val, priv->base + reg);
+	/* Registers other than the WDTINT is always synchronized with WDT_CLK */
+	if (reg != WDTINT)
+		rzg2l_wdt_wait_delay(priv);
+}
+
+static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	u32 time_out;
+
+	/* Clear Lapsed Time Register and clear Interrupt */
+	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
+	/* 2 consecutive overflow cycle needed to trigger reset */
+	time_out = (wdev->timeout / 2 * 1000000) / rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0);
+	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(time_out), WDTSET);
+}
+
+static int rzg2l_wdt_start(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	reset_control_deassert(priv->rstc);
+	pm_runtime_get_sync(wdev->parent);
+
+	/* Initialize time out */
+	rzg2l_wdt_init_timeout(wdev);
+
+	/* Initialize watchdog counter register */
+	rzg2l_wdt_write(priv, 0, WDTTIM);
+
+	/* Enable watchdog timer*/
+	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
+
+	return 0;
+}
+
+static int rzg2l_wdt_stop(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	pm_runtime_put(wdev->parent);
+	reset_control_assert(priv->rstc);
+
+	return 0;
+}
+
+static int rzg2l_wdt_restart(struct watchdog_device *wdev,
+			     unsigned long action, void *data)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	/* Reset the module before we modify any register */
+	reset_control_reset(priv->rstc);
+	pm_runtime_get_sync(wdev->parent);
+
+	/* smallest counter value to reboot soon */
+	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
+
+	/* Enable watchdog timer*/
+	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
+
+	return 0;
+}
+
+static const struct watchdog_info rzg2l_wdt_ident = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity = "Renesas RZ/G2L WDT Watchdog",
+};
+
+static int rzg2l_wdt_ping(struct watchdog_device *wdev)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	rzg2l_wdt_write(priv, WDTINT_INTDISP, WDTINT);
+
+	return 0;
+}
+
+static const struct watchdog_ops rzg2l_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = rzg2l_wdt_start,
+	.stop = rzg2l_wdt_stop,
+	.ping = rzg2l_wdt_ping,
+	.restart = rzg2l_wdt_restart,
+};
+
+static void rzg2l_wdt_reset_assert_pm_disable_put(void *data)
+{
+	struct watchdog_device *wdev = data;
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	pm_runtime_put(wdev->parent);
+	pm_runtime_disable(wdev->parent);
+	reset_control_assert(priv->rstc);
+}
+
+static int rzg2l_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzg2l_wdt_priv *priv;
+	unsigned long pclk_rate;
+	struct clk *wdt_clk;
+	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);
+
+	/* Get watchdog main clock */
+	wdt_clk = clk_get(&pdev->dev, "oscclk");
+	if (IS_ERR(wdt_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
+
+	priv->osc_clk_rate = clk_get_rate(wdt_clk);
+	clk_put(wdt_clk);
+	if (!priv->osc_clk_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
+
+	/* Get Peripheral clock */
+	wdt_clk = clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(wdt_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
+
+	pclk_rate = clk_get_rate(wdt_clk);
+	clk_put(wdt_clk);
+	if (!pclk_rate)
+		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
+
+	priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 + F2CYCLE_NSEC(pclk_rate) * 9;
+
+	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");
+
+	reset_control_deassert(priv->rstc);
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", ERR_PTR(ret));
+		goto out_pm_get;
+	}
+
+	priv->wdev.info = &rzg2l_wdt_ident;
+	priv->wdev.ops = &rzg2l_wdt_ops;
+	priv->wdev.parent = dev;
+	priv->wdev.min_timeout = 1;
+	priv->wdev.max_timeout = rzg2l_wdt_get_cycle_msec(priv->osc_clk_rate, 0xfff);
+	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
+
+	watchdog_set_drvdata(&priv->wdev, priv);
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzg2l_wdt_reset_assert_pm_disable_put,
+				       &priv->wdev);
+	if (ret < 0)
+		return ret;
+
+	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);
+
+out_pm_get:
+	pm_runtime_disable(dev);
+	reset_control_assert(priv->rstc);
+
+	return ret;
+}
+
+static const struct of_device_id rzg2l_wdt_ids[] = {
+	{ .compatible = "renesas,rzg2l-wdt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
+
+static struct platform_driver rzg2l_wdt_driver = {
+	.driver = {
+		.name = "rzg2l_wdt",
+		.of_match_table = rzg2l_wdt_ids,
+	},
+	.probe = rzg2l_wdt_probe,
+};
+module_platform_driver(rzg2l_wdt_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/G2L WDT Watchdog Driver");
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_LICENSE("GPL v2");