From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-x244.google.com (mail-pg0-x244.google.com [IPv6:2607:f8b0:400e:c05::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3yYXS56QZyzDrKt for ; Sat, 11 Nov 2017 07:58:13 +1100 (AEDT) Received: by mail-pg0-x244.google.com with SMTP id 207so5450425pgc.12 for ; Fri, 10 Nov 2017 12:58:13 -0800 (PST) Sender: Guenter Roeck Date: Fri, 10 Nov 2017 12:58:07 -0800 From: Guenter Roeck To: Christophe Leroy Cc: Wim Van Sebroeck , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-watchdog@vger.kernel.org Subject: Re: [PATCH v2] watchdog: mpc8xxx: use the core worker function Message-ID: <20171110205807.GA4348@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, Nov 08, 2017 at 03:39:44PM +0100, Christophe Leroy wrote: > The watchdog core includes a worker function which pings the > watchdog until user app starts pinging it and which also > pings it if the HW require more frequent pings. > Use that function instead of the dedicated timer. > In the mean time, we can allow the user to change the timeout. > > Then change the timeout module parameter to use seconds and > use the watchdog_init_timeout() core function. > > On some HW (eg: the 8xx), SWCRR contains bits unrelated to the > watchdog which have to be preserved upon write. > > This driver has nothing preventing the use of the magic close, so > enable it. > > Signed-off-by: Christophe Leroy Couple of comments, but unrelated to this patch. Reviewed-by: Guenter Roeck > --- > v2: set ddata->wdd.max_hw_heartbeat_ms and ddata->wdd.min_timeout > in probe instead of start > > drivers/watchdog/mpc8xxx_wdt.c | 84 +++++++++++++++++++----------------------- > 1 file changed, 38 insertions(+), 46 deletions(-) > > diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c > index 366e5c7e650b..aca2d6323f8a 100644 > --- a/drivers/watchdog/mpc8xxx_wdt.c > +++ b/drivers/watchdog/mpc8xxx_wdt.c > @@ -22,7 +22,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -31,10 +30,13 @@ > #include Not needed. > #include > > +#define WATCHDOG_TIMEOUT 10 > + > struct mpc8xxx_wdt { > __be32 res0; > __be32 swcrr; /* System watchdog control register */ > #define SWCRR_SWTC 0xFFFF0000 /* Software Watchdog Time Count. */ > +#define SWCRR_SWF 0x00000008 /* Software Watchdog Freeze (mpc8xx). */ > #define SWCRR_SWEN 0x00000004 /* Watchdog Enable bit. */ > #define SWCRR_SWRI 0x00000002 /* Software Watchdog Reset/Interrupt Select bit.*/ > #define SWCRR_SWPR 0x00000001 /* Software Watchdog Counter Prescale bit. */ > @@ -52,14 +54,15 @@ struct mpc8xxx_wdt_type { > struct mpc8xxx_wdt_ddata { > struct mpc8xxx_wdt __iomem *base; > struct watchdog_device wdd; > - struct timer_list timer; > spinlock_t lock; Not needed (the watchdog core handles locking). > + u16 swtc; > }; > > -static u16 timeout = 0xffff; > +static u16 timeout; > module_param(timeout, ushort, 0); > MODULE_PARM_DESC(timeout, > - "Watchdog timeout in ticks. (0 + "Watchdog timeout in seconds. (1 > static bool reset = 1; > module_param(reset, bool, 0); > @@ -80,31 +83,27 @@ static void mpc8xxx_wdt_keepalive(struct mpc8xxx_wdt_ddata *ddata) > spin_unlock(&ddata->lock); > } > > -static void mpc8xxx_wdt_timer_ping(unsigned long arg) > -{ > - struct mpc8xxx_wdt_ddata *ddata = (void *)arg; > - > - mpc8xxx_wdt_keepalive(ddata); > - /* We're pinging it twice faster than needed, just to be sure. */ > - mod_timer(&ddata->timer, jiffies + HZ * ddata->wdd.timeout / 2); > -} > - > static int mpc8xxx_wdt_start(struct watchdog_device *w) > { > struct mpc8xxx_wdt_ddata *ddata = > container_of(w, struct mpc8xxx_wdt_ddata, wdd); > - > - u32 tmp = SWCRR_SWEN | SWCRR_SWPR; > + u32 tmp = in_be32(&ddata->base->swcrr); > > /* Good, fire up the show */ > + tmp &= ~(SWCRR_SWTC | SWCRR_SWF | SWCRR_SWEN | SWCRR_SWRI | SWCRR_SWPR); > + tmp |= SWCRR_SWEN | SWCRR_SWPR | (ddata->swtc << 16); > + > if (reset) > tmp |= SWCRR_SWRI; > > - tmp |= timeout << 16; > - > out_be32(&ddata->base->swcrr, tmp); > > - del_timer_sync(&ddata->timer); > + tmp = in_be32(&ddata->base->swcrr); > + if (!(tmp & SWCRR_SWEN)) > + return -EOPNOTSUPP; > + > + ddata->swtc = tmp >> 16; > + set_bit(WDOG_HW_RUNNING, &ddata->wdd.status); > > return 0; > } > @@ -118,17 +117,8 @@ static int mpc8xxx_wdt_ping(struct watchdog_device *w) > return 0; > } > > -static int mpc8xxx_wdt_stop(struct watchdog_device *w) > -{ > - struct mpc8xxx_wdt_ddata *ddata = > - container_of(w, struct mpc8xxx_wdt_ddata, wdd); > - > - mod_timer(&ddata->timer, jiffies); > - return 0; > -} > - > static struct watchdog_info mpc8xxx_wdt_info = { > - .options = WDIOF_KEEPALIVEPING, > + .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT, > .firmware_version = 1, > .identity = "MPC8xxx", > }; > @@ -137,7 +127,6 @@ static struct watchdog_ops mpc8xxx_wdt_ops = { > .owner = THIS_MODULE, > .start = mpc8xxx_wdt_start, > .ping = mpc8xxx_wdt_ping, > - .stop = mpc8xxx_wdt_stop, > }; > > static int mpc8xxx_wdt_probe(struct platform_device *ofdev) > @@ -148,7 +137,6 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev) > struct mpc8xxx_wdt_ddata *ddata; > u32 freq = fsl_get_sys_freq(); > bool enabled; > - unsigned int timeout_sec; > > wdt_type = of_device_get_match_data(&ofdev->dev); > if (!wdt_type) > @@ -173,27 +161,17 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev) > } > > spin_lock_init(&ddata->lock); > - setup_timer(&ddata->timer, mpc8xxx_wdt_timer_ping, > - (unsigned long)ddata); > > ddata->wdd.info = &mpc8xxx_wdt_info, > ddata->wdd.ops = &mpc8xxx_wdt_ops, > > - /* Calculate the timeout in seconds */ > - timeout_sec = (timeout * wdt_type->prescaler) / freq; > - > - ddata->wdd.timeout = timeout_sec; > + ddata->wdd.timeout = WATCHDOG_TIMEOUT; > + watchdog_init_timeout(&ddata->wdd, timeout, &ofdev->dev); > > watchdog_set_nowayout(&ddata->wdd, nowayout); > > - ret = watchdog_register_device(&ddata->wdd); > - if (ret) { > - pr_err("cannot register watchdog device (err=%d)\n", ret); > - return ret; > - } > - > - pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d (%d seconds)\n", > - reset ? "reset" : "interrupt", timeout, timeout_sec); > + ddata->swtc = min(ddata->wdd.timeout * freq / wdt_type->prescaler, > + 0xffffU); > > /* > * If the watchdog was previously enabled or we're running on > @@ -201,7 +179,22 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev) > * userspace handles it. > */ > if (enabled) > - mod_timer(&ddata->timer, jiffies); > + mpc8xxx_wdt_start(&ddata->wdd); > + > + ddata->wdd.max_hw_heartbeat_ms = (ddata->swtc * wdt_type->prescaler) / > + (freq / 1000); > + ddata->wdd.min_timeout = ddata->wdd.max_hw_heartbeat_ms / 1000; > + if (ddata->wdd.timeout < ddata->wdd.min_timeout) > + ddata->wdd.timeout = ddata->wdd.min_timeout; > + > + ret = watchdog_register_device(&ddata->wdd); > + if (ret) { > + pr_err("cannot register watchdog device (err=%d)\n", ret); > + return ret; > + } > + > + pr_info("WDT driver for MPC8xxx initialized. mode:%s timeout=%d sec\n", > + reset ? "reset" : "interrupt", ddata->wdd.timeout); > > platform_set_drvdata(ofdev, ddata); > return 0; > @@ -213,7 +206,6 @@ static int mpc8xxx_wdt_remove(struct platform_device *ofdev) > > pr_crit("Watchdog removed, expect the %s soon!\n", > reset ? "reset" : "machine check exception"); > - del_timer_sync(&ddata->timer); > watchdog_unregister_device(&ddata->wdd); > > return 0; > -- > 2.13.3 >