Hi Claudiu,
Thank you for the review.
On Thu, Jun 20, 2024 at 8:40 AM claudiu beznea <claudiu.beznea@xxxxxxxxx> wrote:
>
> Hi, Prabhakar,
>
> On 19.06.2024 01:24, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add Watchdog Timer driver for RZ/V2H(P) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > drivers/watchdog/Kconfig | 8 ++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/rzv2h_wdt.c | 248 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/watchdog/rzv2h_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 85eea38dbdf4..3f7bcc10ccc2 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -938,6 +938,14 @@ 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_RENESAS || 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 2d1117564f5b..295909a1b3b9 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..08f97b4bab7f
> > --- /dev/null
> > +++ b/drivers/watchdog/rzv2h_wdt.c
> > @@ -0,0 +1,248 @@
> > +// 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 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 watchdog_device wdev;
> > + struct reset_control *rstc;
>
> You can keep the pointers first to save some padding, if any.
>
OK.
> > + unsigned long oscclk_rate;
> > +};
> > +
> > +static u32 rzv2h_wdt_get_cycle_usec(struct rzv2h_wdt_priv *priv,
> > + unsigned long cycle,
> > + u16 wdttime)
> > +{
> > + int clock_division_ratio;
> > + u64 timer_cycle_us;
> > +
> > + clock_division_ratio = CLOCK_DIV_BY_256;
> > +
> > + timer_cycle_us = clock_division_ratio * (wdttime + 1) * MICRO;
> > +
> > + return div64_ul(timer_cycle_us, cycle);
> > +}
> > +
> > +static int rzv2h_wdt_ping(struct watchdog_device *wdev)
> > +{
> > + struct rzv2h_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + 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
> > + */
> > + delay = 4 * rzv2h_wdt_get_cycle_usec(priv, priv->oscclk_rate, 0);
> > + 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);
> > +
> > + /* LSI needs RSTIRQS to be cleared */
> > + writeb(readb(priv->base + WDTRCR) & ~WDTRCR_RSTIRQS, priv->base + WDTRCR);
> > +}
> > +
> > +static int rzv2h_wdt_start(struct watchdog_device *wdev)
> > +{
> > + pm_runtime_get_sync(wdev->parent);
>
> You may consider using pm_runtime_resume_and_get() which takes care of
> failures from __pm_runtime_resume(), if any.
>
OK.
> > +
> > + /*
> > + * 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);
> > +
> > + pm_runtime_put(wdev->parent);
> > + reset_control_reset(priv->rstc);
> > +
> > + return 0;
> > +}
> > +
> > +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)
> > +{
> > + rzv2h_wdt_stop(wdev);
>
> Calling pm_runtime_put() though this function may lead to unbalanced
> runtime PM counter if the device is not used at this moment. I may be wrong
> though, I'm just reading the code, anyway (see below).
>
Agreed, I have added a check now to call stop only if WDT is active.
> > +
> > + pm_runtime_get_sync(wdev->parent);
>
> If compiled with LOCKDEP this should trigger an invalid wait context
> (see commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") and maybe [2] for a possible fix (if it's considered ok).
>
I finally managed to replicate the issue and now replaced it with the
clk_enable() api to turn ON the clocks.
Cheers,
Prabhakar
