Re: [PATCH v3] watchdog: nic7018_wdt: Add NIC7018 watchdog driver

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Dec 13, 2016 at 10:16:23AM +0000, Hui Chun Ong wrote:
> On Sat, 2016-12-10 at 09:22 -0800, Guenter Roeck wrote:
> > On 12/10/2016 02:25 AM, Hui Chun Ong wrote:
> > > 
> > > Add support for the watchdog timer on PXI Embedded Controller.
> > > 
> > > Signed-off-by: Hui Chun Ong <hui.chun.ong@ni.com>
> > > ---
> > > v2: Remove mutex lock and platform_device *pdev fields from struct
> > > nic7018_wdt.
> > >     Update config NIC7018_WDT description.
> > >     Update nic7018_get_config() to never return error.
> > >     Remove checking for IO resource size in nic7018_probe().
> > > 
> > > v1: Remove non-standard attributes.
> > >     Change from acpi_driver to platform_driver.
> > >     Rename driver from ni7018_wdt to nic7018_wdt.
> > > ---
> > >  Documentation/watchdog/watchdog-parameters.txt |   5 +
> > >  drivers/watchdog/Kconfig                       |  10 +
> > >  drivers/watchdog/Makefile                      |   1 +
> > >  drivers/watchdog/nic7018_wdt.c                 | 277
> > > +++++++++++++++++++++++++
> > >  4 files changed, 293 insertions(+)
> > >  create mode 100644 drivers/watchdog/nic7018_wdt.c
> > > 
> > > diff --git a/Documentation/watchdog/watchdog-parameters.txt
> > > b/Documentation/watchdog/watchdog-parameters.txt
> > > index a8d3642..bd142fa 100644
> > > --- a/Documentation/watchdog/watchdog-parameters.txt
> > > +++ b/Documentation/watchdog/watchdog-parameters.txt
> > > @@ -209,6 +209,11 @@ timeout: Initial watchdog timeout in seconds
> > > (0<timeout<516, default=60)
> > >  nowayout: Watchdog cannot be stopped once started
> > >  	(default=kernel config parameter)
> > >  -------------------------------------------------
> > > +nic7018_wdt:
> > > +timeout: Initial watchdog timeout in seconds (0<timeout<464,
> > > default=80)
> > > +nowayout: Watchdog cannot be stopped once started
> > > +	(default=kernel config parameter)
> > > +-------------------------------------------------
> > >  nuc900_wdt:
> > >  heartbeat: Watchdog heartbeats in seconds.
> > >  	(default = 15)
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index fdd3228..baa79825 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -1325,6 +1325,16 @@ config NI903X_WDT
> > >  	  To compile this driver as a module, choose M here: the
> > > module will be
> > >  	  called ni903x_wdt.
> > > 
> > > +config NIC7018_WDT
> > > +	tristate "NIC7018 Watchdog"
> > > +	depends on X86 && ACPI
> > > +	select WATCHDOG_CORE
> > > +	---help---
> > > +	  Support for National Instruments NIC7018 Watchdog.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > module will be
> > > +	  called nic7018_wdt.
> > > +
> > >  # M32R Architecture
> > > 
> > >  # M68K Architecture
> > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > > index caa9f4a..bd88e2e 100644
> > > --- a/drivers/watchdog/Makefile
> > > +++ b/drivers/watchdog/Makefile
> > > @@ -139,6 +139,7 @@ obj-$(CONFIG_INTEL_SCU_WATCHDOG) +=
> > > intel_scu_watchdog.o
> > >  obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> > >  obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
> > >  obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
> > > +obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> > > 
> > >  # M32R Architecture
> > > 
> > > diff --git a/drivers/watchdog/nic7018_wdt.c
> > > b/drivers/watchdog/nic7018_wdt.c
> > > new file mode 100644
> > > index 0000000..9b7b8d3
> > > --- /dev/null
> > > +++ b/drivers/watchdog/nic7018_wdt.c
> > > @@ -0,0 +1,277 @@
> > > +/*
> > > + * Copyright (C) 2016 National Instruments Corp.
> > > + *
> > > + * 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.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/watchdog.h>
> > > +
> > > +#define LOCK			0xA5
> > > +#define UNLOCK			0x5A
> > > +
> > > +#define WDT_CTRL_RESET_EN	BIT(7)
> > > +#define WDT_RELOAD_PORT_EN	BIT(7)
> > > +
> > > +#define WDT_CTRL		1
> > > +#define WDT_RELOAD_CTRL		2
> > > +#define WDT_PRESET_PRESCALE	4
> > > +#define WDT_REG_LOCK		5
> > > +#define WDT_COUNT		6
> > > +#define WDT_RELOAD_PORT		7
> > > +
> > > +#define WDT_MIN_TIMEOUT		1
> > > +#define WDT_MAX_TIMEOUT		464
> > > +#define WDT_DEFAULT_TIMEOUT	80
> > > +
> > > +#define WDT_MAX_COUNTER		15
> > > +
> > > +static unsigned int timeout;
> > > +module_param(timeout, uint, 0);
> > > +MODULE_PARM_DESC(timeout,
> > > +		 "Watchdog timeout in seconds. (default="
> > > +		 __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
> > > +
> > > +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 nic7018_wdt {
> > > +	u16 io_base;
> > > +	u32 period_ms;
> > > +	struct watchdog_device wdd;
> > > +};
> > > +
> > > +struct nic7018_config {
> > > +	u32 period_ms;
> > > +	u8 divider;
> > > +};
> > > +
> > > +static const struct nic7018_config nic7018_configs[] = {
> > > +	{   125, 3 },
> > The maximum timeout is (125 * 15) - 62, or 1813 ms. This will never
> > be selected,
> > since with a timeout of 2s count will be 17 below, and the entry will
> > be skipped.
> > With a timeout of 1000, the second table entry will be used as well
> > since the
> > timeout with the first entry is not exactly 1 second.
> > 
> > Given that, you might as well drop this entry.
> > 
> > 
> > > 
> > > +	{  2000, 4 },
> > > +	{ 32000, 5 },
> > > +};
> > > +
> > > +static inline u32 nic7018_timeout_ms(u32 period_ms, u8 counter)
> > > +{
> > > +	return period_ms * counter - period_ms / 2;
> > > +}
> > > +
> > > +static const struct nic7018_config *nic7018_get_config(u32
> > > timeout_ms,
> > > +						       u8
> > > *counter)
> > > +{
> > > +	u32 delta, i, best_delta = U32_MAX;
> > > +	const struct nic7018_config *config, *best_config = NULL;
> > > +	u8 count;
> > > +	*counter = WDT_MAX_COUNTER;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(nic7018_configs); i++) {
> > > +		config = &nic7018_configs[i];
> > > +
> > > +		count = DIV_ROUND_UP(timeout_ms + config-
> > > >period_ms / 2,
> > > +				     config->period_ms);
> > > +
> > > +		if (count > WDT_MAX_COUNTER)
> > > +			continue;
> > > +
> > > +		delta = nic7018_timeout_ms(config->period_ms,
> > > count) -
> > > +			timeout_ms;
> > > +
> > > +		if (delta < best_delta) {
> > > +			best_delta = delta;
> > > +			best_config = config;
> > > +			*counter = count;
> > > +		}
> > > +	}
> > > +
> > > +	return (!best_config) ? config : best_config;
> > Unecessary (), and
> > 	return best_config ? : config;
> > works without the '!'.
> > 
> > Also, given the above, it seems to me that
> > 
> > 	if (timeout < 30 && timeout != 16) {
> > 		config = &nic7018_configs[0];
> > 		count = timeout / 2 + 1;
> > 	} else {
> > 		config = &nic7018_configs[1];
> > 		count = DIV_ROUND_UP(timeout + 16, 32);
> > 		if (count > WDT_MAX_COUNTER;
> > 			count = WDT_MAX_COUNTER;
> > 	}
> > 	*counter = count;
> > 	return config;
> > 
> > would accomplish the same as your code and be much less complex and
> > easier
> > to understand. As a side effect, you could keep all timeouts in
> > seconds
> > and would not have to deal with milliseconds at all.
> > 
> 
> Is it possible to keep the miliseconds calculation in the driver even
> though its doesn't seem to be needed? The reason is because the
> watchdog timer has miliseconds timeout resolution that we plan to
> utilize for our specific application. Do you foresee the watchdog core
> adding support for miliseconds timeout?
> 

Not anytime soon. There was some argument about it, but in practice Linux
user space demons have a hard time meeting low-second timeouts. On top of that,
it really would only make sense for low-second or sub-second timeouts, but for
those I don't really see the point because it takes much longer to reboot the
kernel.

In my opinion, it is better to keep the code simple and add complexity later
if/when needed. Writing complex code just in case it may be needed at some
point in the future doesn't make much sense to me.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html