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@xxxxxx>
> > > ---
> > > 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-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
