Message-ID: <CA+V-a8u0dFGmNqJWuXXH3mVVTT6dWBhSr+SM7nFyu3DAeACjNA@mail.gmail.com>
Date: Tue, 6 Aug 2024 14:47:42 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Geert Uytterhoeven <geert+renesas@...der.be>, 
	Magnus Damm <magnus.damm@...il.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 2/2] watchdog: Add Watchdog Timer driver for RZ/V2H(P)

Hi Guenter,

Thank you for the review.

On Mon, Aug 5, 2024 at 9:34 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 8/5/24 13:04, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Add Watchdog Timer driver for RZ/V2H(P) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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

Powered by blists - more mailing lists