RE: [RFC 24/37] watchdog: renesas_wdt_gen2: Add Gen2 specific driver

[Fabrizio Castro <fabrizio.castro-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>]

Hello Guenter,

thank you so much for your feedback.
This driver is similar to the R-Car Gen3 watchdog driver (drivers/watchdog/renesas_wdt.c), at this point in time there is a discussion in progress about merging the functionality specific to R-Car Gen2 into the R-Car Gen3 driver, but this decision is stalled until we agree on something else that has in turn an impact on the driver implementation.
I'll keep your comments in mind whatever we decide to do, but I am going to get back to you only after the decision has been made.

Thanks,
Fabrizio

> Subject: Re: [RFC 24/37] watchdog: renesas_wdt_gen2: Add Gen2 specific driver
>
> On Thu, Jan 25, 2018 at 06:02:58PM +0000, Fabrizio Castro wrote:
> > R-Car Gen2 (and RZ/G1) platforms need some tweaking for SMP and watchdog
> > to coexist nicely.
> > This new driver is based on top of Wolfram Sang's driver
> > (drivers/watchdog/renesas_wdt.c), and it contains the quirks necessary
> > for R-Car Gen2 and RZ/G1 to work properly and in harmony with the rest of
> > the system. In particular, the driver:
> > * expects the device clock to be ON all the time,
> > * "pauses" the watchdog operation during suspend, and
> > * "reassures" the SMP bringup function about the availability of the
> >   watchdog registers.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram-+3GIUL1IS+U@public.gmane.orgsas.com>
> > ---
> >  drivers/watchdog/Kconfig            |  15 +-
> >  drivers/watchdog/Makefile           |   1 +
> >  drivers/watchdog/renesas_wdt_gen2.c | 270 ++++++++++++++++++++++++++++++++++++
> >  drivers/watchdog/renesas_wdt_gen2.h |  22 +++
> >  4 files changed, 306 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/watchdog/renesas_wdt_gen2.c
> >  create mode 100644 drivers/watchdog/renesas_wdt_gen2.h
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index ca200d1..e580c72 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -725,12 +725,23 @@ config ATLAS7_WATCHDOG
> >    module will be called atlas7_wdt.
> >
> >  config RENESAS_WDT
> > -tristate "Renesas WDT Watchdog"
> > +tristate "Renesas R-Car Gen3 WDT Watchdog"
> >  depends on ARCH_RENESAS || COMPILE_TEST
> >  select WATCHDOG_CORE
> >  help
> >    This driver adds watchdog support for the integrated watchdogs in the
> > -  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
> > +  Renesas R-Car Gen3 devices.
> > +
> > +config RENESAS_WDT_GEN2
> > +tristate "Renesas R-Car Gen2 and RZ/G1 WDT Watchdog"
> > +depends on ARCH_RENESAS || COMPILE_TEST
> > +select WATCHDOG_CORE
> > +help
> > +  This driver adds watchdog support for the integrated watchdogs in the
> > +  Renesas R-Car Gen2 and RZ/G1 devices.
> > +
> > +  To compile this driver as a module, choose M here: the
> > +  module will be called renesas_wdt_gen2.
> >
> >  config RENESAS_RZAWDT
> >  tristate "Renesas RZ/A WDT Watchdog"
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 715a210..57ab810 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -83,6 +83,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
> >  obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
> >  obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
> >  obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> > +obj-$(CONFIG_RENESAS_WDT_GEN2) += renesas_wdt_gen2.o
> >  obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
> >  obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> >  obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> > diff --git a/drivers/watchdog/renesas_wdt_gen2.c b/drivers/watchdog/renesas_wdt_gen2.c
> > new file mode 100644
> > index 0000000..c841636
> > --- /dev/null
> > +++ b/drivers/watchdog/renesas_wdt_gen2.c
> > @@ -0,0 +1,270 @@
> > +/*
> > + * Watchdog driver for Renesas WDT watchdog
> > + *
> > + * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
> > + * Copyright (C) 2018    Renesas Electronics Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
>
> Please use the SPDX identifier.
>
> > + */
> > +#include <linux/bitops.h>
> > +#include <linux/clk.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/watchdog.h>
> > +#include "renesas_wdt_gen2.h"
> > +
> > +#define RWTCNT0
> > +#define RWTCSRA4
> > +#define RWTCSRA_WOVFBIT(4)
> > +#define RWTCSRA_WRFLGBIT(5)
> > +#define RWTCSRA_TMEBIT(7)
> > +#define RWTCSRB8
> > +
> > +#define RWDT_DEFAULT_TIMEOUT60U
> > +
> > +/*
> > + * In probe, clk_rate is checked to be not more than 16 bit * biggest clock
> > + * divider (12 bits). d is only a factor to fully utilize the WDT counter and
> > + * will not exceed its 16 bits. Thus, no overflow, we stay below 32 bits.
> > + */
> > +#define MUL_BY_CLKS_PER_SEC(p, d) \
> > +DIV_ROUND_UP((d) * (p)->clk_rate, clk_divs[(p)->cks])
> > +
> > +/* d is 16 bit, clk_divs 12 bit -> no 32 bit overflow */
> > +#define DIV_BY_CLKS_PER_SEC(p, d) ((d) * clk_divs[(p)->cks] / (p)->clk_rate)
> > +
> > +static const unsigned int clk_divs[] = { 1, 4, 16, 32, 64, 128, 1024, 4096 };
> > +
> > +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 rwdt_priv {
> > +void __iomem *base;
> > +struct watchdog_device wdev;
> > +unsigned long clk_rate;
> > +bool enabled;
> > +u8 cks;
> > +};
> > +
> > +static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned int reg)
> > +{
> > +if (reg == RWTCNT)
> > +val |= 0x5a5a0000;
> > +else
> > +val |= 0xa5a5a500;
> > +
> > +writel_relaxed(val, priv->base + reg);
> > +}
> > +
> > +static int rwdt_init_timeout(struct watchdog_device *wdev)
> > +{
> > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +rwdt_write(priv, 65536 - MUL_BY_CLKS_PER_SEC(priv, wdev->timeout),
> > +   RWTCNT);
> > +
> > +return 0;
> > +}
> > +
> > +static int rwdt_start(struct watchdog_device *wdev)
> > +{
> > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +rwdt_write(priv, 0, RWTCSRB);
> > +rwdt_write(priv, priv->cks, RWTCSRA);
> > +rwdt_init_timeout(wdev);
> > +
> > +while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> > +cpu_relax();
> > +
> Can this get stuck ?
>
> > +rwdt_write(priv, priv->cks | RWTCSRA_TME, RWTCSRA);
> > +
> > +return 0;
> > +}
> > +
> > +static int rwdt_stop(struct watchdog_device *wdev)
> > +{
> > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +rwdt_write(priv, priv->cks, RWTCSRA);
> > +
> > +return 0;
> > +}
> > +
> > +static unsigned int rwdt_get_timeleft(struct watchdog_device *wdev)
> > +{
> > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +u16 val = readw_relaxed(priv->base + RWTCNT);
> > +
> > +return DIV_BY_CLKS_PER_SEC(priv, 65536 - val);
> > +}
> > +
> > +static int rwdt_restart(struct watchdog_device *wdev, unsigned long action,
> > +void *data)
> > +{
> > +struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +rwdt_write(priv, 0x00, RWTCSRB);
> > +rwdt_write(priv, 0x00, RWTCSRA);
> > +rwdt_write(priv, 0xffff, RWTCNT);
> > +
> > +while (readb_relaxed(priv->base + RWTCSRA) & RWTCSRA_WRFLG)
> > +cpu_relax();
> > +
> > +rwdt_write(priv, 0x80, RWTCSRA);
> > +return 0;
> > +}
> > +
> > +static const struct watchdog_info rwdt_ident = {
> > +.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> > +.identity = "Renesas WDT Watchdog",
> > +};
> > +
> > +static const struct watchdog_ops rwdt_ops = {
> > +.owner = THIS_MODULE,
> > +.start = rwdt_start,
> > +.stop = rwdt_stop,
> > +.ping = rwdt_init_timeout,
> > +.get_timeleft = rwdt_get_timeleft,
> > +.restart = rwdt_restart,
> > +};
> > +
> > +static int rwdt_probe(struct platform_device *pdev)
> > +{
> > +struct rwdt_priv *priv;
> > +struct resource *res;
> > +struct clk *clk;
> > +unsigned long clks_per_sec;
> > +int ret, i;
> > +
> > +priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +if (!priv)
> > +return -ENOMEM;
> > +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +priv->base = devm_ioremap_resource(&pdev->dev, res);
> > +if (IS_ERR(priv->base))
> > +return PTR_ERR(priv->base);
> > +
> > +clk = devm_clk_get(&pdev->dev, NULL);
> > +if (IS_ERR(clk))
> > +return PTR_ERR(clk);
> > +
> > +
> > +priv->clk_rate = clk_get_rate(clk);
> > +
> > +if (!priv->clk_rate)
> > +return -ENOENT;
> > +
> Odd line spacing. Please no double empty lines, and please no
> empty line after a function and before checking the return value.
>
> ENOENT is an odd return value. No such file or directory ?
>
> > +for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
> > +clks_per_sec = priv->clk_rate / clk_divs[i];
> > +if (clks_per_sec && clks_per_sec < 65536) {
> > +priv->cks = i;
> > +break;
> > +}
> > +}
> > +
> > +if (i < 0) {
> > +dev_err(&pdev->dev, "Can't find suitable clock divider\n");
> > +return -ERANGE;
>
> Another odd return value. Math result not presentable ?
>
> > +}
> > +
> > +priv->wdev.info = &rwdt_ident,
> > +priv->wdev.ops = &rwdt_ops,
> > +priv->wdev.parent = &pdev->dev;
> > +priv->wdev.min_timeout = 1;
> > +priv->wdev.max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
> > +priv->wdev.timeout = min(priv->wdev.max_timeout, RWDT_DEFAULT_TIMEOUT);
> > +
> > +platform_set_drvdata(pdev, priv);
> > +watchdog_set_drvdata(&priv->wdev, priv);
> > +watchdog_set_nowayout(&priv->wdev, nowayout);
> > +
> > +ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
> > +if (ret)
> > +dev_warn(&pdev->dev,
> > + "Specified timeout value invalid, using default\n");
> > +shmobile_set_wdt_clock_status(RWDT_CLOCK_ON);
> > +
> > +ret = watchdog_register_device(&priv->wdev);
>
> Please consider using devm_watchdog_register_device().
>
> > +return ret;
> > +}
> > +
> > +static int rwdt_remove(struct platform_device *pdev)
> > +{
> > +struct rwdt_priv *priv = platform_get_drvdata(pdev);
> > +
> > +watchdog_unregister_device(&priv->wdev);
> > +
> > +return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int rwdt_suspend(struct device *dev)
> > +{
> > +struct platform_device *pdev;
> > +struct rwdt_priv *priv;
> > +u8 val;
> > +
> > +pdev = to_platform_device(dev);
> > +priv = platform_get_drvdata(pdev);
> > +val = readb_relaxed(priv->base + RWTCSRA);
> > +if (val & RWTCSRA_TME) {
> > +priv->enabled = true;
> > +rwdt_write(priv, val & ~RWTCSRA_TME, RWTCSRA);
> > +} else {
> > +priv->enabled = false;
> > +}
>
> This will require an explanation why you can't use watchdog_active()
> here and in the resume function.
>
> > +return 0;
> > +}
> > +
> > +static int rwdt_resume(struct device *dev)
> > +{
> > +struct platform_device *pdev;
> > +struct rwdt_priv *priv;
> > +u8 val;
> > +
> > +pdev = to_platform_device(dev);
> > +priv = platform_get_drvdata(pdev);
> > +if (priv->enabled) {
> > +val = readb_relaxed(priv->base + RWTCSRA);
> > +rwdt_write(priv, val | RWTCSRA_TME, RWTCSRA);
> > +}
> > +return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rwdt_pm = {
> > +.suspend = rwdt_suspend,
> > +.resume = rwdt_resume,
>
> Any reason for not using helper functions such as SET_SYSTEM_SLEEP_PM_OPS()
> or SIMPLE_DEV_PM_OPS() ?
>
> > +};
> > +#endif
> > +
> > +static const struct of_device_id rwdt_ids[] = {
> > +{ .compatible = "renesas,rcar-gen2-wdt", },
> > +{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rwdt_ids);
> > +
> > +static struct platform_driver rwdt_driver = {
> > +.driver = {
> > +.name = "renesas_wdt_gen2",
> > +.of_match_table = rwdt_ids,
> > +#ifdef CONFIG_PM
> > +.pm = &rwdt_pm,
> > +#endif
> > +},
> > +.probe = rwdt_probe,
> > +.remove = rwdt_remove,
> > +};
> > +module_platform_driver(rwdt_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas R-Car Gen2 Watchdog Driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Fabrizio Castro <fabrizio.castro-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>");
> > diff --git a/drivers/watchdog/renesas_wdt_gen2.h b/drivers/watchdog/renesas_wdt_gen2.h
> > new file mode 100644
> > index 0000000..41b9e9c
> > --- /dev/null
> > +++ b/drivers/watchdog/renesas_wdt_gen2.h
> > @@ -0,0 +1,22 @@
> > +/*
> > + * drivers/watchdog/renesas_wdt_gen2.h
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Corporation
> > + *
> > + * R-Car Gen2 and RZ/G specific symbols
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#ifndef RENESAS_WDT_GEN2_H
> > +#define RENESAS_WDT_GEN2_H
> > +
> > +#define RWDT_CLOCK_ON0xdeadbeef
> > +#define RWDT_CLOCK_OFF0x00000000
> > +
> > +extern void shmobile_set_wdt_clock_status(unsigned long value);
>
> Wrong file to declare this function. It is not defined in the watchdog driver.
> Same for the defines.
>
> > +
> > +#endif /* RENESAS_WDT_GEN2_H */
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html