Linux-Watchdog Archive on lore.kernel.org
 help / Atom feed
* [PATCH] watchdog: tqmx86: Add watchdog driver for the IO controller
@ 2018-12-08 16:13 Andrew Lunn
  2018-12-08 17:07 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Lunn @ 2018-12-08 16:13 UTC (permalink / raw)
  To: wim, linux; +Cc: linux-watchdog, Andrew Lunn

Some TQ-Systems ComExpress modules have an IO controller with a
watchdog time.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/watchdog/Kconfig      |  12 +++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/tqmx86_wdt.c | 157 ++++++++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/watchdog/tqmx86_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2d64333f4782..1c99238c2f32 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1308,6 +1308,18 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config TQMX86_WDT
+	tristate "TQ-Systems TQMX86 Watchdog Timer"
+	depends on X86
+	help
+	This is the driver for the hardware watchdog timer in the TQMN86 IO
+	on controller found on some of their ComExpress Modules.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config VIA_WDT
 	tristate "VIA Watchdog Timer"
 	depends on X86 && PCI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f69cdff5ad7f..b077e35414e2 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -129,6 +129,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
+obj-$(CONFIG_TQMX86_WDT) += tqmx86_wdt.o
 obj-$(CONFIG_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
diff --git a/drivers/watchdog/tqmx86_wdt.c b/drivers/watchdog/tqmx86_wdt.c
new file mode 100644
index 000000000000..9f5d4612b97a
--- /dev/null
+++ b/drivers/watchdog/tqmx86_wdt.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Watchdog driver for TQMx86 PLD.
+ *
+ * The watchdog supports power of 2 timeouts from 1 to 4096sec.
+ * Once started, it cannot be stopped.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/timer.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+
+/* default timeout (secs) */
+#define WDT_TIMEOUT 32
+
+static unsigned int timeout = WDT_TIMEOUT;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout,
+	"Watchdog timeout in seconds. (1<=timeout<=4096, default="
+				__MODULE_STRING(WDT_TIMEOUT) ")");
+struct tqmx86_wdt {
+	struct watchdog_device wdd;
+	void __iomem *io_base;
+};
+
+#define TQMX86_WDCFG	0x00 /* Watchdog Configuration Register */
+#define TQMX86_WDCS	0x01 /* Watchdog Config/Status Register */
+
+static int tqmx86_wdt_start(struct watchdog_device *wdd)
+{
+	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
+
+	iowrite8(0x81, priv->io_base + TQMX86_WDCS);
+
+	return 0;
+}
+
+static int tqmx86_wdt_stop(struct watchdog_device *wdd)
+{
+	return -EINVAL;
+}
+
+static int tqmx86_wdt_ping(struct watchdog_device *wdd)
+{
+	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
+
+	iowrite8(0x81, priv->io_base + TQMX86_WDCS);
+
+	return 0;
+}
+
+static int tqmx86_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int new_timeout)
+{
+	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
+	u8 val;
+
+	if (new_timeout < 1 || new_timeout > 4096) {
+		dev_info(wdd->parent, "Timeout must be between 1 and 4096\n");
+		return -EINVAL;
+	}
+
+	timeout = roundup_pow_of_two(new_timeout);
+	val = ilog2(timeout) | 0x90;
+	val += 3; /* vallues 0,1,2 correspond to 0.125,0.25,0.5s timeouts */
+	iowrite8(val, priv->io_base + TQMX86_WDCFG);
+
+	wdd->timeout = timeout;
+
+	return 0;
+}
+
+static const struct watchdog_info tqmx86_wdt_info = {
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_KEEPALIVEPING,
+	.identity	= "TQMx86 Watchdog",
+};
+
+static struct watchdog_ops tqmx86_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= tqmx86_wdt_start,
+	.stop		= tqmx86_wdt_stop,
+	.ping		= tqmx86_wdt_ping,
+	.set_timeout	= tqmx86_wdt_set_timeout,
+};
+
+static int tqmx86_wdt_probe(struct platform_device *pdev)
+{
+	struct tqmx86_wdt *priv;
+	struct resource *res;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (IS_ERR(res))
+		return PTR_ERR(res);
+
+	priv->io_base = devm_ioport_map(&pdev->dev, res->start,
+					resource_size(res));
+	if (IS_ERR(priv->io_base))
+		return PTR_ERR(priv->io_base);
+
+	if (timeout < 1 || timeout > 4096) {
+		timeout = WDT_TIMEOUT;
+		dev_warn(&pdev->dev,
+			"timeout value must be 1<=x<=4096, using %d\n",
+			timeout);
+	}
+
+	platform_set_drvdata(pdev, priv);
+	watchdog_set_drvdata(&priv->wdd, priv);
+
+	tqmx86_wdt_set_timeout(&priv->wdd, timeout);
+
+	priv->wdd.parent = &pdev->dev;
+	priv->wdd.info = &tqmx86_wdt_info;
+	priv->wdd.ops = &tqmx86_wdt_ops;
+
+	watchdog_set_nowayout(&priv->wdd, 1);
+
+	err = watchdog_register_device(&priv->wdd);
+	if (err)
+		return err;
+
+	dev_info(&pdev->dev, "TQMx86 watchdog\n");
+
+	return 0;
+}
+
+static int tqmx86_wdt_remove(struct platform_device *pdev)
+{
+	struct tqmx86_wdt *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+	return 0;
+}
+
+static struct platform_driver tqmx86_wdt_driver = {
+	.driver		= {
+		.name	= "tqmx86-wdt",
+	},
+	.probe		= tqmx86_wdt_probe,
+	.remove		= tqmx86_wdt_remove,
+};
+
+module_platform_driver(tqmx86_wdt_driver);
+
+MODULE_AUTHOR("Vadim V.Vlasov <vvlasov@dev.rtsoft.ru>");
+MODULE_DESCRIPTION("TQMx86 Watchdog");
+MODULE_ALIAS("platform:tqmx86-wdt");
+MODULE_LICENSE("GPL");
-- 
2.19.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: tqmx86: Add watchdog driver for the IO controller
  2018-12-08 16:13 [PATCH] watchdog: tqmx86: Add watchdog driver for the IO controller Andrew Lunn
@ 2018-12-08 17:07 ` Guenter Roeck
  2018-12-08 17:16   ` Andrew Lunn
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2018-12-08 17:07 UTC (permalink / raw)
  To: Andrew Lunn, wim; +Cc: linux-watchdog

On 12/8/18 8:13 AM, Andrew Lunn wrote:
> Some TQ-Systems ComExpress modules have an IO controller with a
> watchdog time.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   drivers/watchdog/Kconfig      |  12 +++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/tqmx86_wdt.c | 157 ++++++++++++++++++++++++++++++++++
>   3 files changed, 170 insertions(+)
>   create mode 100644 drivers/watchdog/tqmx86_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 2d64333f4782..1c99238c2f32 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1308,6 +1308,18 @@ config SMSC37B787_WDT
>   
>   	  Most people will say N.
>   
> +config TQMX86_WDT
> +	tristate "TQ-Systems TQMX86 Watchdog Timer"
> +	depends on X86
> +	help
> +	This is the driver for the hardware watchdog timer in the TQMN86 IO
> +	on controller found on some of their ComExpress Modules.

"on controller" ? Should that be just "controller" ?

> +
> +	To compile this driver as a module, choose M here; the module
> +	will be called via_wdt.

via_wdt ?

> +
> +	Most people will say N.
> +
>   config VIA_WDT
>   	tristate "VIA Watchdog Timer"
>   	depends on X86 && PCI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f69cdff5ad7f..b077e35414e2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -129,6 +129,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt.o
>   obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
>   obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
>   obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
> +obj-$(CONFIG_TQMX86_WDT) += tqmx86_wdt.o
>   obj-$(CONFIG_VIA_WDT) += via_wdt.o
>   obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
>   obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
> diff --git a/drivers/watchdog/tqmx86_wdt.c b/drivers/watchdog/tqmx86_wdt.c
> new file mode 100644
> index 000000000000..9f5d4612b97a
> --- /dev/null
> +++ b/drivers/watchdog/tqmx86_wdt.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Watchdog driver for TQMx86 PLD.
> + *
> + * The watchdog supports power of 2 timeouts from 1 to 4096sec.
> + * Once started, it cannot be stopped.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/timer.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +

Alphabetic order, please.

> +/* default timeout (secs) */
> +#define WDT_TIMEOUT 32
> +
> +static unsigned int timeout = WDT_TIMEOUT;

Please set to 0 (ie don't set it) and use watchdog_init_timeout() in the probe
function to override the default.

> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +	"Watchdog timeout in seconds. (1<=timeout<=4096, default="
> +				__MODULE_STRING(WDT_TIMEOUT) ")");
> +struct tqmx86_wdt {
> +	struct watchdog_device wdd;
> +	void __iomem *io_base;
> +};
> +
> +#define TQMX86_WDCFG	0x00 /* Watchdog Configuration Register */
> +#define TQMX86_WDCS	0x01 /* Watchdog Config/Status Register */
> +
> +static int tqmx86_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
> +
> +	iowrite8(0x81, priv->io_base + TQMX86_WDCS);
> +
> +	return 0;
> +}
> +
> +static int tqmx86_wdt_stop(struct watchdog_device *wdd)
> +{
> +	return -EINVAL;
> +}

This is quite pointless. If there is no means to stop the watchdog,
just don't provide a stop function.

> +
> +static int tqmx86_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
> +
> +	iowrite8(0x81, priv->io_base + TQMX86_WDCS);
> +

If the start and the ping function are the same, the ping function
can be omitted.

> +	return 0;
> +}
> +
> +static int tqmx86_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int new_timeout)
> +{
> +	struct tqmx86_wdt *priv = watchdog_get_drvdata(wdd);
> +	u8 val;
> +
> +	if (new_timeout < 1 || new_timeout > 4096) {
> +		dev_info(wdd->parent, "Timeout must be between 1 and 4096\n");
> +		return -EINVAL;
> +	}

Not necessary; checked by infrastructure.

> +
> +	timeout = roundup_pow_of_two(new_timeout);
> +	val = ilog2(timeout) | 0x90;
> +	val += 3; /* vallues 0,1,2 correspond to 0.125,0.25,0.5s timeouts */

values

> +	iowrite8(val, priv->io_base + TQMX86_WDCFG);
> +
> +	wdd->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info tqmx86_wdt_info = {
> +	.options	= WDIOF_SETTIMEOUT |
> +			  WDIOF_KEEPALIVEPING,
> +	.identity	= "TQMx86 Watchdog",
> +};
> +
> +static struct watchdog_ops tqmx86_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= tqmx86_wdt_start,
> +	.stop		= tqmx86_wdt_stop,
> +	.ping		= tqmx86_wdt_ping,
> +	.set_timeout	= tqmx86_wdt_set_timeout,
> +};
> +
> +static int tqmx86_wdt_probe(struct platform_device *pdev)
> +{
> +	struct tqmx86_wdt *priv;
> +	struct resource *res;
> +	int err;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (IS_ERR(res))
> +		return PTR_ERR(res);
> +
> +	priv->io_base = devm_ioport_map(&pdev->dev, res->start,
> +					resource_size(res));
> +	if (IS_ERR(priv->io_base))
> +		return PTR_ERR(priv->io_base);
> +
> +	if (timeout < 1 || timeout > 4096) {
> +		timeout = WDT_TIMEOUT;
> +		dev_warn(&pdev->dev,
> +			"timeout value must be 1<=x<=4096, using %d\n",
> +			timeout);
> +	}
> +
You should set the default in the wdd structure and call watchdog_init_timeout()
to update it.

> +	platform_set_drvdata(pdev, priv);
> +	watchdog_set_drvdata(&priv->wdd, priv);
> +
> +	tqmx86_wdt_set_timeout(&priv->wdd, timeout);
> +
> +	priv->wdd.parent = &pdev->dev;
> +	priv->wdd.info = &tqmx86_wdt_info;
> +	priv->wdd.ops = &tqmx86_wdt_ops;
> +
You should also specify minimum and maximum timeout
to enable boundary checking by the watchdog core.

> +	watchdog_set_nowayout(&priv->wdd, 1);

Why not use CONFIG_WATCHDOG_NOWAYOUT and WATCHDOG_NOWAYOUT ?

> +
> +	err = watchdog_register_device(&priv->wdd);
> +	if (err)
> +		return err;
> +
> +	dev_info(&pdev->dev, "TQMx86 watchdog\n");
> +
> +	return 0;
> +}
> +
> +static int tqmx86_wdt_remove(struct platform_device *pdev)
> +{
> +	struct tqmx86_wdt *priv = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&priv->wdd);
> +	return 0;

If the remove function only removes the watchdog, you can use
devm_watchdog_register_device() to register it and drop the remove function.

> +}
> +
> +static struct platform_driver tqmx86_wdt_driver = {
> +	.driver		= {
> +		.name	= "tqmx86-wdt",
> +	},
> +	.probe		= tqmx86_wdt_probe,
> +	.remove		= tqmx86_wdt_remove,
> +};
> +
> +module_platform_driver(tqmx86_wdt_driver);
> +
> +MODULE_AUTHOR("Vadim V.Vlasov <vvlasov@dev.rtsoft.ru>");
> +MODULE_DESCRIPTION("TQMx86 Watchdog");
> +MODULE_ALIAS("platform:tqmx86-wdt");
> +MODULE_LICENSE("GPL");
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: tqmx86: Add watchdog driver for the IO controller
  2018-12-08 17:07 ` Guenter Roeck
@ 2018-12-08 17:16   ` Andrew Lunn
  0 siblings, 0 replies; 3+ messages in thread
From: Andrew Lunn @ 2018-12-08 17:16 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: wim, linux-watchdog

Hi Guenter

Thanks for the fast review.

As you can probably guess, i'm not the primary author of this driver,
nor particularly experienced with the watchdog subsystem. So thanks
for the valuable comments. I will go off and rework the driver.

    Andrew

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-08 16:13 [PATCH] watchdog: tqmx86: Add watchdog driver for the IO controller Andrew Lunn
2018-12-08 17:07 ` Guenter Roeck
2018-12-08 17:16   ` Andrew Lunn

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable: git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox