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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux