[PATCH RFC] watchdog: add a new driver for VIA chipsets

[PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Add a new driver for the hardware watchdog timer on VIA chipsets.
Tested on a Artigo A1100, VX855 chipset.

Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
---

 Kconfig   |   13 ++++
 Makefile  |    1 
 via_wdt.c |  176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 190 insertions(+)

--- linux-3.2-rc2/drivers/watchdog/Makefile.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Makefile	2011-11-18 18:13:36.594534635 +0100
@@ -100,6 +100,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
+obj-$(CONFIG_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o
--- linux-3.2-rc2/drivers/watchdog/via_wdt.c.orig	2011-11-18 18:14:45.358174874 +0100
+++ linux-3.2-rc2/drivers/watchdog/via_wdt.c	2011-11-22 11:49:33.169408728 +0100
@@ -0,0 +1,176 @@
+/*
+ * VIA Chipset Watchdog Driver
+ *
+ * Copyright (C) 2011 Sigfox
+ * License terms: GNU General Public License (GPL) version 2
+ * Author: Marc Vertes <marc.vertes@sigfox.com>
+ * Based on a preliminary version from Harald Welte <HaraldWelte@viatech.com>
+ *
+ * The only way to activate the watchdog timer or to set its period is
+ * through BIOS setup.
+ */
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define VIA_WDT_MB_OFFSET	0xe8	/* memory base offset */
+#define VIA_WDT_TRIGGER		0x80	/* start a new countdown */
+#define VIA_WDT_FIRED		0x02	/* set if last restart was caused
+					   by expired watchdog timer */
+
+static void *wdt_mem;			/* watchdog memory region address */
+static unsigned long open_lock;
+
+static unsigned int wdt_ping(void)
+{
+	unsigned int res = readl(wdt_mem);	/* get status bits */
+	writel(res | VIA_WDT_TRIGGER, wdt_mem);
+	return res;
+}
+
+static ssize_t wdt_write(struct file *file, const char __user *buf,
+			 size_t count, loff_t *ppos)
+{
+	if (count <= 0)
+		return 0;
+	wdt_ping();
+	return 1;
+}
+
+static long wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret;
+	static struct watchdog_info ident = {
+		.options	= WDIOF_CARDRESET,
+		.identity	= "VIA Southbridge WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		ret = copy_to_user((struct watchdog_info *)arg, &ident,
+				   sizeof(ident)) ? -EFAULT : 0;
+		break;
+	case WDIOC_GETSTATUS:
+		ret = put_user(0, (int __user *)arg);
+		break;
+	case WDIOC_GETBOOTSTATUS:
+		if (wdt_ping() & VIA_WDT_FIRED)
+			ret = put_user(WDIOF_CARDRESET, (int __user *)arg);
+		else
+			ret = put_user(0, (int __user *)arg);
+		break;
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTTY;
+	}
+	return ret;
+}
+
+static int wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &open_lock))
+		return -EBUSY;
+	return nonseekable_open(inode, file);
+}
+
+static int wdt_release(struct inode *inode, struct file *file)
+{
+	clear_bit(0, &open_lock);
+	return 0;
+}
+
+static const struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= wdt_open,
+	.release	= wdt_release,
+	.unlocked_ioctl	= wdt_ioctl,
+	.write		= wdt_write,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor	= WATCHDOG_MINOR,
+	.name	= "watchdog",
+	.fops	= &wdt_fops,
+};
+
+static int __devinit wdt_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	unsigned int mmio = 0;
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		return -ENODEV;
+	}
+	pci_read_config_dword(pdev, VIA_WDT_MB_OFFSET, &mmio);
+	dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
+	if (mmio == 0) {
+		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
+		return -ENODEV;
+	}
+	wdt_mem = ioremap(mmio, 8);
+	if (wdt_mem == NULL) {
+		dev_err(&pdev->dev, "cannot remap VIA wdt mmio registers\n");
+		return -ENODEV;
+	}
+	if (misc_register(&wdt_miscdev)) {
+		dev_err(&pdev->dev, "cannot register misc device\n");
+		iounmap(wdt_mem);
+		return -EIO;
+	}
+	if (wdt_ping() & VIA_WDT_FIRED)
+		dev_info(&pdev->dev "restarted by expired watchdog timer\n");
+	return 0;
+}
+
+static void __devexit wdt_remove(struct pci_dev *dev)
+{
+	misc_deregister(&wdt_miscdev);
+	iounmap(wdt_mem);
+}
+
+/*
+ * The driver has not been tested yet on CX700 and VX800.
+ */
+DEFINE_PCI_DEVICE_TABLE(wdt_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_CX700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX800) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX855) },
+	{ 0 }
+};
+
+static struct pci_driver wdt_driver = {
+	.name		= "via_wdt",
+	.id_table	= wdt_pci_table,
+	.probe		= wdt_probe,
+	.remove		= __devexit_p(wdt_remove),
+};
+
+static int __init wdt_init(void)
+{
+	if (pci_register_driver(&wdt_driver) || wdt_mem == NULL)
+		return -ENODEV;
+	return 0;
+}
+
+static void __exit wdt_exit(void)
+{
+	pci_unregister_driver(&wdt_driver);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_AUTHOR("Marc Vertes");
+MODULE_DESCRIPTION("Driver for watchdog timer on VIA chipset");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
+MODULE_LICENSE("GPL");
--- linux-3.2-rc2/drivers/watchdog/Kconfig.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Kconfig	2011-11-18 18:13:36.595534599 +0100
@@ -779,6 +779,19 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config VIA_WDT
+	tristate "VIA Watchdog Timer"
+	depends on X86
+	---help---
+	This is the driver for the hardware watchdog timer on VIA
+	southbridge chipset CX700, VX800, VX855. Watchdog setup
+	in BIOS is required.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config W83627HF_WDT
 	tristate "W83627HF/W83627DHG Watchdog Timer"
 	depends on X86

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

On Tue, Nov 22, 2011 at 12:17:13PM +0100, Marc Vertes wrote:
> Add a new driver for the hardware watchdog timer on VIA chipsets.
> Tested on a Artigo A1100, VX855 chipset.
> 
> Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>

New watchdog drivers should use the framework. Have a look at
Documentation/watchdog/convert_drivers_to_kernel_api.txt for a guide. It
is mainly removing code, though.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Rahul Bedarkar]

On Tue, Nov 22, 2011 at 4:52 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Tue, Nov 22, 2011 at 12:17:13PM +0100, Marc Vertes wrote:
>> Add a new driver for the hardware watchdog timer on VIA chipsets.
>> Tested on a Artigo A1100, VX855 chipset.
>>
>> Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
>
> New watchdog drivers should use the framework. Have a look at
> Documentation/watchdog/convert_drivers_to_kernel_api.txt for a guide. It
> is mainly removing code, though.
I will take care of this.

Thanks,
Rahul Bedarkar
>
> Thanks,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk7LheQACgkQD27XaX1/VRutYACdEU7AXPGKVZ9fV6SaT2B010iK
> LhAAoMBk4btczZxmA/0iXTLQQwqq0Jqt
> =bziw
> -----END PGP SIGNATURE-----
>
>

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wolfram Sang <w.sang@pengutronix.de> wrote:

> On Tue, Nov 22, 2011 at 12:17:13PM +0100, Marc Vertes wrote:
> > Add a new driver for the hardware watchdog timer on VIA chipsets.
> > Tested on a Artigo A1100, VX855 chipset.
> > 
> > Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
>
> New watchdog drivers should use the framework. Have a look at
> Documentation/watchdog/convert_drivers_to_kernel_api.txt for a guide. It
> is mainly removing code, though.
>
Here it is:
---

Add a new driver for the hardware watchdog timer on VIA chipsets.
This driver uses the new watchdog framework.
Tested on a Artigo A1100, VX855 chipset.

Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
---

 Kconfig   |   14 ++++++
 Makefile  |    1 
 via_wdt.c |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)

--- linux-3.2-rc2/drivers/watchdog/Makefile.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Makefile	2011-11-18 18:13:36.594534635 +0100
@@ -100,6 +100,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
+obj-$(CONFIG_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o
--- linux-3.2-rc2/drivers/watchdog/via_wdt.c.orig	2011-11-18 18:14:45.358174874 +0100
+++ linux-3.2-rc2/drivers/watchdog/via_wdt.c	2011-11-22 17:20:05.531757353 +0100
@@ -0,0 +1,134 @@
+/*
+ * VIA Chipset Watchdog Driver
+ *
+ * Copyright (C) 2011 Sigfox
+ * License terms: GNU General Public License (GPL) version 2
+ * Author: Marc Vertes <marc.vertes@sigfox.com>
+ * Based on a preliminary version from Harald Welte <HaraldWelte@viatech.com>
+ *
+ * The only way to activate the watchdog timer or to set its period is
+ * through BIOS setup.
+ */
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/watchdog.h>
+
+#define VIA_WDT_MB_OFFSET	0xe8	/* memory base offset */
+#define VIA_WDT_TRIGGER		0x80	/* start a new countdown */
+#define VIA_WDT_FIRED		0x02	/* set if last restart was caused
+					   by expired watchdog timer */
+
+static int wdt_start(struct watchdog_device *wdev)
+{
+	/* Nothing to do. The watchdog can only be started by the BIOS. */
+	return 0;
+}
+
+static int wdt_stop(struct watchdog_device *wdev)
+{
+	/* Nothing to do. The watchdog can not be stopped. */
+	return 0;
+}
+
+static int wdt_ping(struct watchdog_device *wdev)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(wdev);
+	unsigned int res = readl(wdt_mem);	/* get status bits */
+
+	writel(res | VIA_WDT_TRIGGER, wdt_mem);
+	return 0;
+}
+
+static struct watchdog_info wdt_info = {
+	.identity = "VIA watchdog",
+	.options  =  WDIOF_CARDRESET,
+};
+
+static struct watchdog_ops wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = wdt_start,
+	.stop  = wdt_stop,
+	.ping  = wdt_ping,
+};
+
+static struct watchdog_device wdt_dev = {
+	.info = &wdt_info,
+	.ops  = &wdt_ops,
+};
+
+static int __devinit wdt_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	unsigned int mmio = 0;
+	void __iomem *wdt_mem;
+	int ret;
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		return -ENODEV;
+	}
+	pci_read_config_dword(pdev, VIA_WDT_MB_OFFSET, &mmio);
+	dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
+	if (mmio == 0) {
+		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
+		return -ENODEV;
+	}
+	wdt_mem = ioremap(mmio, 8);
+	if (wdt_mem == NULL) {
+		dev_err(&pdev->dev, "cannot remap VIA wdt mmio registers\n");
+		return -ENODEV;
+	}
+	ret = watchdog_register_device(&wdt_dev);
+	if (ret)
+		return ret;
+	watchdog_set_drvdata(&wdt_dev, wdt_mem);
+	if (readl(wdt_mem) & VIA_WDT_FIRED) {
+		wdt_dev.bootstatus |= WDIOF_CARDRESET;
+		dev_notice(&pdev->dev, "restarted by expired watchdog\n");
+	}
+	return 0;
+}
+
+static void __devexit wdt_remove(struct pci_dev *dev)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(&wdt_dev);
+
+	watchdog_unregister_device(&wdt_dev);
+	iounmap(wdt_mem);
+}
+
+/*
+ * The driver has not been tested yet on CX700 and VX800.
+ */
+DEFINE_PCI_DEVICE_TABLE(wdt_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_CX700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX800) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX855) },
+	{ 0 }
+};
+
+static struct pci_driver wdt_driver = {
+	.name		= "via_wdt",
+	.id_table	= wdt_pci_table,
+	.probe		= wdt_probe,
+	.remove		= __devexit_p(wdt_remove),
+};
+
+static int __init wdt_init(void)
+{
+	return pci_register_driver(&wdt_driver);
+}
+
+static void __exit wdt_exit(void)
+{
+	pci_unregister_driver(&wdt_driver);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_AUTHOR("Marc Vertes");
+MODULE_DESCRIPTION("Driver for watchdog timer on VIA chipset");
+MODULE_LICENSE("GPL");
--- linux-3.2-rc2/drivers/watchdog/Kconfig.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Kconfig	2011-11-22 16:39:38.175325620 +0100
@@ -779,6 +779,20 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config VIA_WDT
+	tristate "VIA Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	---help---
+	This is the driver for the hardware watchdog timer on VIA
+	southbridge chipset CX700, VX800, VX855. Watchdog setup
+	in BIOS is required.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config W83627HF_WDT
 	tristate "W83627HF/W83627DHG Watchdog Timer"
 	depends on X86

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

On Tue, Nov 22, 2011 at 06:05:48PM +0100, Marc Vertes wrote:
> Wolfram Sang <w.sang@pengutronix.de> wrote:
> 
> > On Tue, Nov 22, 2011 at 12:17:13PM +0100, Marc Vertes wrote:
> > > Add a new driver for the hardware watchdog timer on VIA chipsets.
> > > Tested on a Artigo A1100, VX855 chipset.
> > > 
> > > Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
> >
> > New watchdog drivers should use the framework. Have a look at
> > Documentation/watchdog/convert_drivers_to_kernel_api.txt for a guide. It
> > is mainly removing code, though.
> >
> Here it is:

Great, thanks.

> +static int wdt_start(struct watchdog_device *wdev)
> +{
> +	/* Nothing to do. The watchdog can only be started by the BIOS. */
> +	return 0;
> +}
> +
> +static int wdt_stop(struct watchdog_device *wdev)
> +{
> +	/* Nothing to do. The watchdog can not be stopped. */
> +	return 0;
> +}

Hmm, I'll leave this to Wim if it can stay like this (or if he wants a timer to
serve the non-stoppable watchdog or so).

> +static int __devinit wdt_probe(struct pci_dev *pdev,
> +			       const struct pci_device_id *ent)
> +{
> +	unsigned int mmio = 0;
> +	void __iomem *wdt_mem;
> +	int ret;
> +
> +	if (pci_enable_device(pdev)) {
> +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> +		return -ENODEV;
> +	}
> +	pci_read_config_dword(pdev, VIA_WDT_MB_OFFSET, &mmio);
> +	dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
> +	if (mmio == 0) {
> +		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
> +		return -ENODEV;
> +	}

What about

	if (mmio != 0) {
		dev_info("VIA Chipset...")
	} else {
		dev_err()
		return -ENODEV;
	}

to only have the needed printouts.

> +	wdt_mem = ioremap(mmio, 8);
> +	if (wdt_mem == NULL) {
> +		dev_err(&pdev->dev, "cannot remap VIA wdt mmio registers\n");
> +		return -ENODEV;
> +	}
> +	ret = watchdog_register_device(&wdt_dev);
> +	if (ret)
> +		return ret;

You need to iounmap in the error-case.

> +	watchdog_set_drvdata(&wdt_dev, wdt_mem);
> +	if (readl(wdt_mem) & VIA_WDT_FIRED) {
> +		wdt_dev.bootstatus |= WDIOF_CARDRESET;
> +		dev_notice(&pdev->dev, "restarted by expired watchdog\n");

Skip the printout. This can be detected using CARDRESET.

> +/*
> + * The driver has not been tested yet on CX700 and VX800.
> + */

Then, I'd rather skip this comment and the IDs. Or if you are sure enough it
works, leave them in ;) Best option would be testers showing up.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Mark Brown]

On Tue, Nov 22, 2011 at 06:05:48PM +0100, Marc Vertes wrote:

> +static int wdt_start(struct watchdog_device *wdev)
> +{
> +	/* Nothing to do. The watchdog can only be started by the BIOS. */
> +	return 0;
> +}

Shouldn't we just update the framework to cope with missing functions?

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wolfram Sang <w.sang@pengutronix.de> wrote:

> > +	dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
> > +	if (mmio == 0) {
> > +		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
> > +		return -ENODEV;
> > +	}
>
> What about
>
> 	if (mmio != 0) {
> 		dev_info("VIA Chipset...")
> 	} else {
> 		dev_err()
> 		return -ENODEV;
> 	}
>
> to only have the needed printouts.
>
Ok.

> > +	ret = watchdog_register_device(&wdt_dev);
> > +	if (ret)
> > +		return ret;
>
> You need to iounmap in the error-case.
Yes. Good catch.

>
> > +	watchdog_set_drvdata(&wdt_dev, wdt_mem);
> > +	if (readl(wdt_mem) & VIA_WDT_FIRED) {
> > +		wdt_dev.bootstatus |= WDIOF_CARDRESET;
> > +		dev_notice(&pdev->dev, "restarted by expired watchdog\n");
>
> Skip the printout. This can be detected using CARDRESET.
>
Ok.

> > +/*
> > + * The driver has not been tested yet on CX700 and VX800.
> > + */
>
> Then, I'd rather skip this comment and the IDs. Or if you are sure enough it
> works, leave them in ;) Best option would be testers showing up.
>
Then I take the risk and remove the comments ;). This is stable stuff inherited
from old models...

> Regards,
>
>    Wolfram
>
Thanks for your comments, I'm preparing an update.
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Tue, Nov 22, 2011 at 05:32:28PM +0000, Mark Brown wrote:
> On Tue, Nov 22, 2011 at 06:05:48PM +0100, Marc Vertes wrote:
> 
> > +static int wdt_start(struct watchdog_device *wdev)
> > +{
> > +	/* Nothing to do. The watchdog can only be started by the BIOS. */
> > +	return 0;
> > +}
> 
> Shouldn't we just update the framework to cope with missing functions?

For start(), I dunno. I don't have enough experience with watchdog drivers to
judge if this is acceptable, although I'd think so. For stop(), a few drivers
already activate a timer to keep the watchdog happy; such a mechanism should
definately go into the core. It's in my todo-list, but I won't be angry if
someone is faster ;)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Marc Vertes <marc.vertes@sigfox.com> wrote:

> Thanks for your comments, I'm preparing an update.

Here:
---
Add a new driver for the hardware watchdog timer on VIA chipsets.
This driver uses the new watchdog framework.
Tested on a Artigo A1100, VX855 chipset.

Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
---

 Kconfig   |   14 ++++++
 Makefile  |    1 
 via_wdt.c |  132 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+)

--- linux-3.2-rc2/drivers/watchdog/Makefile.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Makefile	2011-11-18 18:13:36.594534635 +0100
@@ -100,6 +100,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
+obj-$(CONFIG_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o
--- linux-3.2-rc2/drivers/watchdog/via_wdt.c.orig	2011-11-18 18:14:45.358174874 +0100
+++ linux-3.2-rc2/drivers/watchdog/via_wdt.c	2011-11-22 19:42:06.219529719 +0100
@@ -0,0 +1,132 @@
+/*
+ * VIA Chipset Watchdog Driver
+ *
+ * Copyright (C) 2011 Sigfox
+ * License terms: GNU General Public License (GPL) version 2
+ * Author: Marc Vertes <marc.vertes@sigfox.com>
+ * Based on a preliminary version from Harald Welte <HaraldWelte@viatech.com>
+ *
+ * The only way to activate the watchdog timer or to set its period is
+ * through BIOS setup.
+ */
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/watchdog.h>
+
+#define VIA_WDT_MB_OFFSET	0xe8	/* memory base offset */
+#define VIA_WDT_TRIGGER		0x80	/* start a new countdown */
+#define VIA_WDT_FIRED		0x02	/* set if last restart was caused
+					   by expired watchdog timer */
+
+static int wdt_start(struct watchdog_device *wdev)
+{
+	/* Nothing to do. The watchdog can only be started by the BIOS. */
+	return 0;
+}
+
+static int wdt_stop(struct watchdog_device *wdev)
+{
+	/* Nothing to do. The watchdog can not be stopped. */
+	return 0;
+}
+
+static int wdt_ping(struct watchdog_device *wdev)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(wdev);
+	unsigned int res = readl(wdt_mem);	/* get status bits */
+
+	writel(res | VIA_WDT_TRIGGER, wdt_mem);
+	return 0;
+}
+
+static struct watchdog_info wdt_info = {
+	.identity = "VIA watchdog",
+	.options  =  WDIOF_CARDRESET,
+};
+
+static struct watchdog_ops wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = wdt_start,
+	.stop  = wdt_stop,
+	.ping  = wdt_ping,
+};
+
+static struct watchdog_device wdt_dev = {
+	.info = &wdt_info,
+	.ops  = &wdt_ops,
+};
+
+static int __devinit wdt_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	unsigned int mmio = 0;
+	void __iomem *wdt_mem;
+	int ret;
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		return -ENODEV;
+	}
+	pci_read_config_dword(pdev, VIA_WDT_MB_OFFSET, &mmio);
+	if (mmio) {
+		dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
+	} else {
+		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
+		return -ENODEV;
+	}
+	wdt_mem = ioremap(mmio, 8);
+	if (wdt_mem == NULL) {
+		dev_err(&pdev->dev, "cannot remap VIA wdt MMIO registers\n");
+		return -ENODEV;
+	}
+	ret = watchdog_register_device(&wdt_dev);
+	if (ret) {
+		iounmap(wdt_mem);
+		return ret;
+	}
+	watchdog_set_drvdata(&wdt_dev, wdt_mem);
+	if (readl(wdt_mem) & VIA_WDT_FIRED)
+		wdt_dev.bootstatus |= WDIOF_CARDRESET;
+	return 0;
+}
+
+static void __devexit wdt_remove(struct pci_dev *dev)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(&wdt_dev);
+
+	watchdog_unregister_device(&wdt_dev);
+	iounmap(wdt_mem);
+}
+
+DEFINE_PCI_DEVICE_TABLE(wdt_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_CX700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX800) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX855) },
+	{ 0 }
+};
+
+static struct pci_driver wdt_driver = {
+	.name		= "via_wdt",
+	.id_table	= wdt_pci_table,
+	.probe		= wdt_probe,
+	.remove		= __devexit_p(wdt_remove),
+};
+
+static int __init wdt_init(void)
+{
+	return pci_register_driver(&wdt_driver);
+}
+
+static void __exit wdt_exit(void)
+{
+	pci_unregister_driver(&wdt_driver);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_AUTHOR("Marc Vertes");
+MODULE_DESCRIPTION("Driver for watchdog timer on VIA chipset");
+MODULE_LICENSE("GPL");
--- linux-3.2-rc2/drivers/watchdog/Kconfig.orig	2011-11-15 18:02:59.000000000 +0100
+++ linux-3.2-rc2/drivers/watchdog/Kconfig	2011-11-22 16:39:38.175325620 +0100
@@ -779,6 +779,20 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config VIA_WDT
+	tristate "VIA Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	---help---
+	This is the driver for the hardware watchdog timer on VIA
+	southbridge chipset CX700, VX800, VX855. Watchdog setup
+	in BIOS is required.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config W83627HF_WDT
 	tristate "W83627HF/W83627DHG Watchdog Timer"
 	depends on X86

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wolfram Sang <w.sang@pengutronix.de> wrote:

> On Tue, Nov 22, 2011 at 05:32:28PM +0000, Mark Brown wrote:
> > On Tue, Nov 22, 2011 at 06:05:48PM +0100, Marc Vertes wrote:
> > 
> > > +static int wdt_start(struct watchdog_device *wdev)
> > > +{
> > > +	/* Nothing to do. The watchdog can only be started by the BIOS. */
> > > +	return 0;
> > > +}
> > 
> > Shouldn't we just update the framework to cope with missing functions?
>
> For start(), I dunno. I don't have enough experience with watchdog drivers to
> judge if this is acceptable, although I'd think so. For stop(), a few drivers
> already activate a timer to keep the watchdog happy; such a mechanism should
> definately go into the core. It's in my todo-list, but I won't be angry if
> someone is faster ;)

A question regarding "nowayout", in the case of this VIA chipset where
a watchdog can not be stopped once started from BIOS. I think I should
set unconditionally WDOG_NO_WAY_OUT in status, shouldn't I ?

I also do not think it is correct to implement a timer and ping
proactively in case of magicclose because 1) it complicates a bit the
code, 2) there is no way to know the timeout value, and it set very low
in the BIOS (1 s).

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

(BTW I removed a few duplicates from the CC list)

> A question regarding "nowayout", in the case of this VIA chipset where
> a watchdog can not be stopped once started from BIOS. I think I should
> set unconditionally WDOG_NO_WAY_OUT in status, shouldn't I ?
> 
> I also do not think it is correct to implement a timer and ping
> proactively in case of magicclose because 1) it complicates a bit the
> code, 2) there is no way to know the timeout value, and it set very low
> in the BIOS (1 s).

Yup, I'd accept the reason that you can't know the timeout value and
then setting NO_WAY_OUT unconditionally looks like the thing to do here.

@Wim: Would you accept drivers which can only ping a watchdog but
neither start/stop it?

@Marc: If he does, would you be interested in updating the watchdog-core
so that no start()/stop() will also be accepted if there is a ping()?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wolfram Sang <w.sang@pengutronix.de> wrote:

> (BTW I removed a few duplicates from the CC list)
Yes, thanks.
>
> @Wim: Would you accept drivers which can only ping a watchdog but
> neither start/stop it?
>
> @Marc: If he does, would you be interested in updating the watchdog-core
> so that no start()/stop() will also be accepted if there is a ping()?
>
Yes, it looks reasonable to me. Watchdogs settable by BIOS only and not
cancelable (as VIA) are simple hard watchdogs for which ping() is the
only necessary operation. I'm preparing a new update.

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Dmitry Artamonow]

On 19:55 Tue 22 Nov     , Marc Vertes wrote:
> Add a new driver for the hardware watchdog timer on VIA chipsets.
> This driver uses the new watchdog framework.
> Tested on a Artigo A1100, VX855 chipset.
> 
> Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>
> ---

[ .... ]

> +obj-$(CONFIG_VIA_WDT) += via_wdt.o

"via_wdt" seems to be a too generic name. What if someone will want to
write driver for some incompatible watchdog also made by VIA?
Maybe it's worth to use something more specific - say, "via_vx8xx_wdt"?


> + *
> + * The only way to activate the watchdog timer or to set its period is
> + * through BIOS setup.
> + */

Why is to rely on BIOS setup? Is there really no information on what
registers to write to start watchdog? Or you was just lazy to find it?
It took me less than a minute to google up a datasheet[1] on VX800/VX820
and it seems to have all needed information to implement proper start()
and stop() functions. I suppose info on watchdog registers should be
applicable to VX855 as well - but it's better to check on your hardware
anyway.

[1] http://linux.via.com.tw/support/beginDownload.action?eleid=161&fid=241

-- 
Best regards,
Dmitry "MAD" Artamonow

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi All,

> @Wim: Would you accept drivers which can only ping a watchdog but
> neither start/stop it?

Will look the driver in more detail later this week.

To answer this question though: Userspace needs to be able to stop
and start watchdog devices. That's why we have the solution with the
timer. I will look up the example that I created in the past (as part
of the documentation) and add it to linux-next (so that it is available
as an example for everyone) and either sent it to Marc or sent Marc an
updated version of his code so that he can test it.

Bottom line: Yes, there is hardware available that can only be pinged
and not be stopped, but No, Userspace needs the start/stop and ping
functionality so that a watchdog daemon can work correctly (which is
important because some watchdog daemons also do application testing.

I prefer to add this driver first with the timer build into the driver.
We should indeed think about putting the timer in the framework, but
we need to think about how we can do this when we have multiple devices
(where some need it and others don't). The roadmap should be first
multiple devices and then the timer so that we don't create an extra
hurdle for the future.

Marc: I hope to sent you something still today.

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Mark Brown]

On Wed, Nov 23, 2011 at 01:13:29PM +0100, Wim Van Sebroeck wrote:

> Bottom line: Yes, there is hardware available that can only be pinged
> and not be stopped, but No, Userspace needs the start/stop and ping
> functionality so that a watchdog daemon can work correctly (which is
> important because some watchdog daemons also do application testing.

Right, but if the hardware doesn't support this functionality it seems
better to have the core deal with this rather than requiring the driver
to implement empty operations.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> > Bottom line: Yes, there is hardware available that can only be pinged
> > and not be stopped, but No, Userspace needs the start/stop and ping
> > functionality so that a watchdog daemon can work correctly (which is
> > important because some watchdog daemons also do application testing.
> 
> Right, but if the hardware doesn't support this functionality it seems
> better to have the core deal with this rather than requiring the driver
> to implement empty operations.

As said, should indeed become part of the core, but the core needs to be
able to handle multiple devices and then this becomes more complex (because
some devices need and and other's don't).

Coming back to now: if the driver incorporates the timer (like all other
devices that can be started but not stopped once started do), then the start
and stop functions are not empty.
The timer (with the example that I prepared as part of the generic code) is
not so difficult and will be a good solution for the time being. (Unless
Dmitry Artamonow's comment about the fact that the watchdog could perhaps
be started and stopped is correct... This should be investigated first imho).

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Dmitry Artamonow <mad_soft@inbox.ru> wrote:

> "via_wdt" seems to be a too generic name. What if someone will want to
> write driver for some incompatible watchdog also made by VIA?
> Maybe it's worth to use something more specific - say, "via_vx8xx_wdt"?
>
According to Harald, it is the same watchdog on all via chipsets and
back to Athlon chipsets. But why not.

> Why is to rely on BIOS setup? Is there really no information on what
> registers to write to start watchdog? Or you was just lazy to find it?
> It took me less than a minute to google up a datasheet[1] on VX800/VX820
> and it seems to have all needed information to implement proper start()
> and stop() functions. I suppose info on watchdog registers should be
> applicable to VX855 as well - but it's better to check on your hardware
> anyway.
>
> [1] http://linux.via.com.tw/support/beginDownload.action?eleid=161&fid=241
>
I rely on BIOS setup because it is the only working method on the
hardware I could test on. This watchdog stuff exists on VIA cpus for
years (back to Athlon chipsets) and still no working open
source driver. Wonder why?

Of course I read the datasheet of the VX855 southbridge prior to
write this driver. And the VX800 one, the CX700 one. Unfortunately,
the instructions to start, stop, get and set counters simply
do not work. The doc is false and/or incomplete. Harald
already implemented a full version of this driver back in 2009
(http://git.gnumonks.org/cgi-bin/gitweb.cgi?p=linux-2.6-via.git;a=shortlog;     h=refs/heads/via-wdt),
but not working. Questions to VIA remain unanswered.

So I decided to keep in the source code only what really works, and not
the well documented but non functional stuff. If you can test or find
hardware where those instructions work as advertised, I will be happy
to expose them again in an updated driver.

The current result, in addition of actually working, is safe,
simple, and compatible with potential "correct" configurations.

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Mark Brown]

On Wed, Nov 23, 2011 at 03:12:15PM +0100, Marc Vertes wrote:

> Of course I read the datasheet of the VX855 southbridge prior to
> write this driver. And the VX800 one, the CX700 one. Unfortunately,
> the instructions to start, stop, get and set counters simply
> do not work. The doc is false and/or incomplete. Harald
> already implemented a full version of this driver back in 2009
> (http://git.gnumonks.org/cgi-bin/gitweb.cgi?p=linux-2.6-via.git;a=shortlog;     h=refs/heads/via-wdt),
> but not working. Questions to VIA remain unanswered.

The documentation may actually be accurate - for security/robustness
reasons it's not uncommon for watchdog hardware to have restrictions on
when it can be programmed.  It may be that there's something like that
going on which locks out reprogramming of the block by the time the OS
is up and running, or there may be additional steps required to go into
a secure mode that VIA don't want to disclose.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wim Van Sebroeck <wim@iguana.be> wrote:

> Hi Marc,
>
> Coming back to now: if the driver incorporates the timer (like all other
> devices that can be started but not stopped once started do), then the start
> and stop functions are not empty.
> The timer (with the example that I prepared as part of the generic code) is
> not so difficult and will be a good solution for the time being. (Unless
> Dmitry Artamonow's comment about the fact that the watchdog could perhaps
> be started and stopped is correct... This should be investigated first imho).
>
The watchdog can not be started and stopped from the driver. This is
what I investigated first and found impossible (and explains why this
driver is still missing).

I do not fully understand yet what has to be done with the timer.

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Harald Welte]

<disclaimer>
I used to do some work for VIA some years ago but am not anymore, the
email alias mostly exists because I don't want to leave the community
completely in the rain if I can still help with some VIA related issues.
</disclaimer>

On Wed, Nov 23, 2011 at 04:10:51PM +0400, Dmitry Artamonow wrote:

> "via_wdt" seems to be a too generic name. What if someone will want to
> write driver for some incompatible watchdog also made by VIA?
> Maybe it's worth to use something more specific - say, "via_vx8xx_wdt"?

In fact, to the best of my knowledge, all previous VIA x86 chipsets use
the exact same watchdog, i.e. you can go back to Athlon or even earlier
chipsets and will likely find the exact same interface.

> > + * The only way to activate the watchdog timer or to set its period is
> > + * through BIOS setup.
> 
> Why is to rely on BIOS setup? Is there really no information on what
> registers to write to start watchdog? Or you was just lazy to find it?
> It took me less than a minute to google up a datasheet[1] on VX800/VX820
> and it seems to have all needed information to implement proper start()
> and stop() functions. I suppose info on watchdog registers should be
> applicable to VX855 as well - but it's better to check on your hardware
> anyway.

Unfortunately it simply doesn't work.  When I originally implemented a
via_wdt driver some years ago, I couldn't start the watchdog no matter
how hard I tried.  My inquiries into VIA at that point didn't provide
any response either.  In fact, I even feared that nobody might ever have
used that watchdog hardware before ;)

I can ask again, but I'm not sure if it will be any different at that
time..

Regards,
	Harald
-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Dmitry Artamonow]

On 15:12 Wed 23 Nov     , Marc Vertes wrote:
> Dmitry Artamonow <mad_soft@inbox.ru> wrote:
> 
> > "via_wdt" seems to be a too generic name. What if someone will want to
> > write driver for some incompatible watchdog also made by VIA?
> > Maybe it's worth to use something more specific - say, "via_vx8xx_wdt"?
> >
> According to Harald, it is the same watchdog on all via chipsets and
> back to Athlon chipsets. But why not.

In that case I think it's ok to leave via_wdt as is.

> > Why is to rely on BIOS setup? Is there really no information on what
> > registers to write to start watchdog? Or you was just lazy to find it?
> > It took me less than a minute to google up a datasheet[1] on VX800/VX820
> > and it seems to have all needed information to implement proper start()
> > and stop() functions. I suppose info on watchdog registers should be
> > applicable to VX855 as well - but it's better to check on your hardware
> > anyway.
> >
> > [1] http://linux.via.com.tw/support/beginDownload.action?eleid=161&fid=241
> >
> I rely on BIOS setup because it is the only working method on the
> hardware I could test on. This watchdog stuff exists on VIA cpus for
> years (back to Athlon chipsets) and still no working open
> source driver. Wonder why?
> 
> Of course I read the datasheet of the VX855 southbridge prior to
> write this driver. And the VX800 one, the CX700 one. Unfortunately,
> the instructions to start, stop, get and set counters simply
> do not work. The doc is false and/or incomplete. Harald
> already implemented a full version of this driver back in 2009
> (http://git.gnumonks.org/cgi-bin/gitweb.cgi?p=linux-2.6-via.git;a=shortlog;     h=refs/heads/via-wdt),
> but not working. Questions to VIA remain unanswered.
> 
> So I decided to keep in the source code only what really works, and not
> the well documented but non functional stuff. If you can test or find
> hardware where those instructions work as advertised, I will be happy
> to expose them again in an updated driver.
> 
> The current result, in addition of actually working, is safe,
> simple, and compatible with potential "correct" configurations.

Ok. Thanks to you and Harald for explaining! That's sad HW doesn't
behave according to datasheets. Maybe it's worth to add a note about this
in the driver to warn future hackers? :)

-- 
Best regards,
Dmitry "MAD" Artamonow

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Harald,

> Unfortunately it simply doesn't work.  When I originally implemented a
> via_wdt driver some years ago, I couldn't start the watchdog no matter
> how hard I tried.  My inquiries into VIA at that point didn't provide
> any response either.  In fact, I even feared that nobody might ever have
> used that watchdog hardware before ;)
> 
> I can ask again, but I'm not sure if it will be any different at that
> time..

Could you please ask again? It could indeed be that they lock registers.
(It could be something similar to some of the superIO chipsets that can
enable/disable (lock/unlock) access to registers).

The BIOS can at least start the hardware, so the hardware does work :-)

Note: I saw your disclaimer and I thus have no high hopes :-).

Thanks in advance,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

> Ok. Thanks to you and Harald for explaining! That's sad HW doesn't
> behave according to datasheets. Maybe it's worth to add a note about this
> in the driver to warn future hackers? :)

Yes, please.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> > Coming back to now: if the driver incorporates the timer (like all other
> > devices that can be started but not stopped once started do), then the start
> > and stop functions are not empty.
> > The timer (with the example that I prepared as part of the generic code) is
> > not so difficult and will be a good solution for the time being. (Unless
> > Dmitry Artamonow's comment about the fact that the watchdog could perhaps
> > be started and stopped is correct... This should be investigated first imho).
> >
> The watchdog can not be started and stopped from the driver. This is
> what I investigated first and found impossible (and explains why this
> driver is still missing).
> 
> I do not fully understand yet what has to be done with the timer.

Below the example driver that should get included in the Documentation.

Kind regards,
Wim.
--------------------------------------------------------------------------------
/*
 * Watchdog timer driver example with timer.
 *
 * Copyright (C) 2009-2011 Wim Van Sebroeck <wim@iguana.be>
 *
 * 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.
 */

/*
 * Some watchdog device can't stop once started. To support
 * the magic_close feature we therefor need to use an internal
 * timer to keep the watchdog being pinged when /dev/watchdog has
 * been closed correctly.
 *
 * This is an example driver for these kind of watchdog devices. 
 */

#define DRV_NAME KBUILD_MODNAME
#define pr_fmt(fmt) DRV_NAME ": " fmt

#include <linux/init.h>
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <linux/watchdog.h>
#include <linux/jiffies.h>
#include <linux/timer.h>
#include <linux/platform_device.h>

/* Hardware heartbeat in seconds */
#define WDT_HW_HEARTBEAT 2

/* Timer heartbeat (500ms) */
#define WDT_HEARTBEAT	(HZ/2)	/* should be <= ((WDT_HW_HEARTBEAT*HZ)/2) */

/* User land timeout */
#define WDT_TIMEOUT 15
static int timeout = WDT_TIMEOUT;
module_param(timeout, int, 0);
MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
	"(default = " __MODULE_STRING(WDT_TIMEOUT) ")");

static int nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, int, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started. "
	"(default = " __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

static struct watchdog_device wdt_dev;
static void wdt_timer_tick(unsigned long data);
static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
					/* The timer that pings the watchdog */
static unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
static unsigned long running;		/* is watchdog running for userspace? */

static struct platform_device *wdt_platform_device;

/*
 * Reset the watchdog timer.  (ie, pat the watchdog)
 */
static inline void wdt_reset(void)
{
	/* Reset the watchdog timer hardware here */
}

/*
 * Timer tick: the timer will make sure that the watchdog timer hardware
 * is being reset in time. The conditions to do this are:
 *  1) the watchog timer has been started and /dev/watchdog is open
 *     and there is still time left before userspace should send the
 *     next heartbeat/ping. (note: the internal heartbeat is much smaller
 *     then the external/userspace heartbeat).
 *  2) the watchdog timer has been stopped by userspace.
 */
static void wdt_timer_tick(unsigned long data)
{
	if (time_before(jiffies, next_heartbeat) ||
	   (!test_bit(WDOG_ACTIVE, &wdt_dev.status))) {
		wdt_reset();
		mod_timer(&timer, jiffies + WDT_HEARTBEAT);
	} else
		pr_crit("I will reboot your machine !\n");
}

/*
 * The watchdog operations
 */
static int wdt_ping(struct watchdog_device *wdd)
{
	/* calculate when the next userspace timeout will be */
	next_heartbeat = jiffies + timeout * HZ;
	return 0;
}

static int wdt_start(struct watchdog_device *wdd)
{
	/* calculate the next userspace timeout and modify the timer */
	wdt_ping(wdd);
	mod_timer(&timer, jiffies + WDT_HEARTBEAT);

	/* Start the watchdog timer hardware here */
	pr_info("wdt_start\n");

	running = 1;
	return 0;
}

static int wdt_stop(struct watchdog_device *wdd)
{
	/* The watchdog timer hardware can not be stopped... */
	pr_info("wdt_stop\n");

	running = 0;
	return 0;
}

static unsigned int wdt_status(struct watchdog_device *wdd)
{
	return WDIOF_FANFAULT;
}

static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
{
	if (new_timeout < 1)
		return -EINVAL;
	return 0;
}

/*
 * The watchdog kernel structures
 */
static const struct watchdog_info wdt_info = {
	.identity =	DRV_NAME,
	.options =	WDIOF_SETTIMEOUT |
			WDIOF_MAGICCLOSE |
			WDIOF_KEEPALIVEPING,
};

static const struct watchdog_ops wdt_ops = {
	.owner =	THIS_MODULE,
	.start =	wdt_start,
	.stop =		wdt_stop,
	.ping =		wdt_ping,
	.status =	wdt_status,
	.set_timeout =	wdt_set_timeout,
};

static struct watchdog_device wdt_dev = {
	.info =		&wdt_info,
	.ops =		&wdt_ops,
};

/*
 * The watchdog timer drivers init and exit routines
 */
static int __devinit wdt_probe(struct platform_device *pdev)
{
	int res;

	/* Register other stuff */

	/* Set watchdog_device parameters */
	wdt_dev.timeout = timeout;
/*	wdt_dev.dev.parent = &pdev->dev;*/
	if (nowayout)
		set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);

	/* Register the watchdog timer device */
	res = watchdog_register_device(&wdt_dev);
	if (res) {
		pr_err("watchdog_register_device returned %d\n", res);
		return res;
	}

	pr_info("enabled (timeout=%d sec)\n", timeout);

	return 0;
}

static int __devexit wdt_remove(struct platform_device *pdev)
{
	/* Unregister the watchdog timer device */
	watchdog_unregister_device(&wdt_dev);

	/* stop and delete the timer */
	pr_warn("I quit now, hardware will probably reboot!\n");
	del_timer(&timer);

	/* Unregister other stuff */
	return 0;
}

static struct platform_driver wdt_driver = {
	.probe		= wdt_probe,
	.remove		= __devexit_p(wdt_remove),
	.driver		= {
		.name	= DRV_NAME,
		.owner	= THIS_MODULE,
	},
};

static int __init wdt_init(void)
{
	int err;

	pr_info("WDT driver initialising.\n");

	err = platform_driver_register(&wdt_driver);
	if (err)
		return err;

	wdt_platform_device = platform_device_register_simple(DRV_NAME,
								-1, NULL, 0);
	if (IS_ERR(wdt_platform_device)) {
		err = PTR_ERR(wdt_platform_device);
		goto unreg_platform_driver;
	}

	return 0;

unreg_platform_driver:
	platform_driver_unregister(&wdt_driver);
	return err;
}

static void __exit wdt_exit(void)
{
	platform_device_unregister(wdt_platform_device);
	platform_driver_unregister(&wdt_driver);
	pr_info("Watchdog Module Unloaded.\n");
}

module_init(wdt_init);
module_exit(wdt_exit);

MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
MODULE_DESCRIPTION("WatchDog Timer Driver example with timer");
MODULE_LICENSE("GPL");
MODULE_ALIAS("platform:" DRV_NAME);

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> The watchdog can not be started and stopped from the driver. This is
> what I investigated first and found impossible (and explains why this
> driver is still missing).
> 
> I do not fully understand yet what has to be done with the timer.

I adapted my example on to your diff and below the result.
Can you test it? Could you also add some comments about the issues
with the datasheet as proposed earlier?

Kind regards,
wim.
-----------------------------------------------------------------------
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 79fd606..f602e90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -772,6 +772,20 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config VIA_WDT
+	tristate "VIA Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	---help---
+	This is the driver for the hardware watchdog timer on VIA
+	southbridge chipset CX700, VX800, VX855. Watchdog setup
+	in BIOS is required.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config W83627HF_WDT
 	tristate "W83627HF/W83627DHG Watchdog Timer"
 	depends on X86
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index fe893e9..e8f479a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -99,6 +99,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_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o
--- /dev/null	2009-12-15 19:54:05.376338696 +0100
+++ ./drivers/watchdog/via_wdt.c	2011-11-23 22:34:24.000000000 +0100
@@ -0,0 +1,208 @@
+/*
+ * VIA Chipset Watchdog Driver
+ *
+ * Copyright (C) 2011 Sigfox
+ * License terms: GNU General Public License (GPL) version 2
+ * Author: Marc Vertes <marc.vertes@sigfox.com>
+ * Based on a preliminary version from Harald Welte <HaraldWelte@viatech.com>
+ *
+ * The only way to activate the watchdog timer or to set its period is
+ * through BIOS setup.
+ */
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/watchdog.h>
+#include <linux/jiffies.h>
+#include <linux/timer.h>
+
+#define VIA_WDT_MB_OFFSET	0xe8	/* memory base offset */
+#define VIA_WDT_TRIGGER		0x80	/* start a new countdown */
+#define VIA_WDT_FIRED		0x02	/* set if last restart was caused
+					   by expired watchdog timer */
+
+/* Hardware heartbeat in seconds */
+#define WDT_HW_HEARTBEAT 1
+
+/* Timer heartbeat (500ms) */
+#define WDT_HEARTBEAT	(HZ/2)	/* should be <= ((WDT_HW_HEARTBEAT*HZ)/2) */
+
+/* User space timeout */
+#define WDT_TIMEOUT 15
+static int timeout = WDT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
+	"(default = " __MODULE_STRING(WDT_TIMEOUT) ")");
+
+static struct watchdog_device wdt_dev;
+static void wdt_timer_tick(unsigned long data);
+static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
+					/* The timer that pings the watchdog */
+static unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
+
+static inline void wdt_reset(void)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(&wdt_dev);
+	unsigned int res = readl(wdt_mem);	/* get status bits */
+
+	writel(res | VIA_WDT_TRIGGER, wdt_mem);
+}
+
+/*
+ * Timer tick: the timer will make sure that the watchdog timer hardware
+ * is being reset in time. The conditions to do this are:
+ *  1) the watchog timer has been started and /dev/watchdog is open
+ *     and there is still time left before userspace should send the
+ *     next heartbeat/ping. (note: the internal heartbeat is much smaller
+ *     then the external/userspace heartbeat).
+ *  2) the watchdog timer has been stopped by userspace.
+ */
+static void wdt_timer_tick(unsigned long data)
+{
+	if (time_before(jiffies, next_heartbeat) ||
+	   (!test_bit(WDOG_ACTIVE, &wdt_dev.status))) {
+		wdt_reset();
+		mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	} else
+		pr_crit("I will reboot your machine !\n");
+}
+
+static int wdt_ping(struct watchdog_device *wdev)
+{
+	/* calculate when the next userspace timeout will be */
+	next_heartbeat = jiffies + timeout * HZ;
+	return 0;
+}
+
+static int wdt_start(struct watchdog_device *wdev)
+{
+	/* calculate the next userspace timeout and modify the timer */
+	wdt_ping(wdev);
+	mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	return 0;
+}
+
+static int wdt_stop(struct watchdog_device *wdev)
+{
+	/* The watchdog timer hardware can not be stopped... */
+	return 0;
+}
+
+static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
+{
+	if (new_timeout < 1)
+		return -EINVAL;
+	return 0;
+}
+
+static const struct watchdog_info wdt_info = {
+	.identity =	"VIA watchdog",
+	.options =	WDIOF_CARDRESET |
+			WDIOF_SETTIMEOUT |
+			WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+};
+
+static const struct watchdog_ops wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	wdt_start,
+	.stop =		wdt_stop,
+	.ping =		wdt_ping,
+	.set_timeout =	wdt_set_timeout,
+};
+
+static struct watchdog_device wdt_dev = {
+	.info =		&wdt_info,
+	.ops =		&wdt_ops,
+};
+
+static int __devinit wdt_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	unsigned int mmio = 0;
+	void __iomem *wdt_mem;
+	int ret = -ENODEV;
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		return -ENODEV;
+	}
+	pci_read_config_dword(pdev, VIA_WDT_MB_OFFSET, &mmio);
+	dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
+	if (mmio == 0) {
+		dev_err(&pdev->dev, "watchdog timer is not enabled in BIOS\n");
+		goto err_out_disable_device;
+	}
+	wdt_mem = ioremap(mmio, 8);
+	if (wdt_mem == NULL) {
+		dev_err(&pdev->dev, "cannot remap VIA wdt mmio registers\n");
+		goto err_out_disable_device;
+	}
+	wdt_dev.timeout = timeout;
+	set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);
+	watchdog_set_drvdata(&wdt_dev, wdt_mem);
+	if (readl(wdt_mem) & VIA_WDT_FIRED) {
+		wdt_dev.bootstatus |= WDIOF_CARDRESET;
+		dev_notice(&pdev->dev, "restarted by expired watchdog\n");
+	}
+	ret = watchdog_register_device(&wdt_dev);
+	if (ret)
+		goto err_out_iounmap;
+	/* This watchdog is allready running. Make sure our timer is also running */
+	mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	return 0;
+
+err_out_iounmap:
+	iounmap(wdt_mem);
+err_out_disable_device:
+	pci_disable_device(pdev);
+	return ret;
+}
+
+static void __devexit wdt_remove(struct pci_dev *pdev)
+{
+	void __iomem *wdt_mem = watchdog_get_drvdata(&wdt_dev);
+
+	watchdog_unregister_device(&wdt_dev);
+	dev_warn(&pdev->dev, "I quit now, hardware will probably reboot!\n");
+	del_timer(&timer);
+	iounmap(wdt_mem);
+	pci_disable_device(pdev);
+}
+
+/*
+ * The driver has not been tested yet on CX700 and VX800.
+ */
+DEFINE_PCI_DEVICE_TABLE(wdt_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_CX700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX800) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX855) },
+	{ 0 }
+};
+
+static struct pci_driver wdt_driver = {
+	.name		= "via_wdt",
+	.id_table	= wdt_pci_table,
+	.probe		= wdt_probe,
+	.remove		= __devexit_p(wdt_remove),
+};
+
+static int __init wdt_init(void)
+{
+	if (timeout < 1)
+		timeout = WDT_TIMEOUT;
+	return pci_register_driver(&wdt_driver);
+}
+
+static void __exit wdt_exit(void)
+{
+	pci_unregister_driver(&wdt_driver);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_AUTHOR("Marc Vertes");
+MODULE_DESCRIPTION("Driver for watchdog timer on VIA chipset");
+MODULE_LICENSE("GPL");

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

> > I do not fully understand yet what has to be done with the timer.
> 
> Below the example driver that should get included in the Documentation.

Shouldn't the timer stuff go to the core instead, activated by some flag?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Wolfram,

> > > I do not fully understand yet what has to be done with the timer.
> > 
> > Below the example driver that should get included in the Documentation.
> 
> Shouldn't the timer stuff go to the core instead, activated by some flag?

After we introduced support for multiple watchdoig devices.

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Hi Wim,

>
> I adapted my example on to your diff and below the result.
> Can you test it? Could you also add some comments about the issues
> with the datasheet as proposed earlier?
>
I have tested your version and it works fine. I saw that you enforced
the NO_WAY_OUT bit and implemented the KEEPALIVEPING and MAGICCLOSE
features properly. Many thanks.

Regarding the timer, the driver can not read the timeout value set in
the BIOS, so there is still a risk of reboot if the BIOS value is lower
than driver one. I assume that other drivers may be in the same case,
and that is required for user compatibility anyway.

Here is the comment block that I propose to insert regarding register
programming issues:

/*
 * Caveat: the only known way to activate the watchdog timer or to set
 * its period is through BIOS configuration, when the hardware supports it.
 * It seems that the watchdog control registers are locked and disabled if
 * the watchdog is not exposed or enabled by BIOS, and that writing them is
 * useless. No public documentation is available regarding this limitation.
 */

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> I have tested your version and it works fine. I saw that you enforced
> the NO_WAY_OUT bit and implemented the KEEPALIVEPING and MAGICCLOSE
> features properly. Many thanks.

No problem.

> Regarding the timer, the driver can not read the timeout value set in
> the BIOS, so there is still a risk of reboot if the BIOS value is lower
> than driver one. I assume that other drivers may be in the same case,
> and that is required for user compatibility anyway.

The driver assumes 1 second as heartbeat of the watchdog device.
It thus takes half of this time do actually 'tickle' the watchdog. (or 500ms).
We should set this 1 second value to the smallest time that is possible (either
the smallest value that can be set in the BIOS or the smallest value that is
listed in the datasheets).

> Here is the comment block that I propose to insert regarding register
> programming issues:
> 
> /*
>  * Caveat: the only known way to activate the watchdog timer or to set
>  * its period is through BIOS configuration, when the hardware supports it.
>  * It seems that the watchdog control registers are locked and disabled if
>  * the watchdog is not exposed or enabled by BIOS, and that writing them is
>  * useless. No public documentation is available regarding this limitation.
>  */

Looks good. I will add this. Just let me know what the smallest value is for
the timeout in the BIOS and/or datasheet and I will do the necessary for inclusion
in the watchdog tree.

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wim Van Sebroeck <wim@iguana.be> wrote:

> > Regarding the timer, the driver can not read the timeout value set in
> > the BIOS, so there is still a risk of reboot if the BIOS value is lower
> > than driver one. I assume that other drivers may be in the same case,
> > and that is required for user compatibility anyway.
>
> The driver assumes 1 second as heartbeat of the watchdog device.
> It thus takes half of this time do actually 'tickle' the watchdog. (or 500ms).
> We should set this 1 second value to the smallest time that is possible (either
> the smallest value that can be set in the BIOS or the smallest value that is
> listed in the datasheets).
>
The smallest possible value is 1 second, both in BIOS and datasheet. For
info, the maximum value is 1023 seconds, approx. 17 min.

I think it is dangerous to set the timer to 1s, both in BIOS, obviously
as the boot is still in progress when it expires, and even in the driver,
where you loose a chance to avoid mandatory reboot after only 1 second
of latency, even if the hardware timer is higher.

The value you have set, 15 seconds, is reasonable to me, and should not
be lowered. If the BIOS was well designed, it should also not allow less
than, say 1 minute.

Kind regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> The smallest possible value is 1 second, both in BIOS and datasheet. For
> info, the maximum value is 1023 seconds, approx. 17 min.

Good that means that the driver value is allready OK.

> I think it is dangerous to set the timer to 1s, both in BIOS, obviously
> as the boot is still in progress when it expires, and even in the driver,
> where you loose a chance to avoid mandatory reboot after only 1 second
> of latency, even if the hardware timer is higher.
> 
> The value you have set, 15 seconds, is reasonable to me, and should not
> be lowered. If the BIOS was well designed, it should also not allow less
> than, say 1 minute.

I have the feeling that you don't understand the function of the timer...
The timer actually sperates the userspace timeout from the watchdog's heartbeat.
The watchdog's heartbeat is what the hardware is actually using as it's timeout
value. That is the 1 second minimum in this case. if userspace is not using
the watchdog device then the timer will make sure that the watchdog is being
reset each 1/2 seconds (or 500ms). We use the smallest value because we don't
know what the actual value is and need to be safe. I do agree that you should
set this higher in the BIOS to survice the actual boot sequence, but we should
make sure that we reset regularly enough so that the system doesn't reboot in
normal operation. That's why I needed to know the minimum value.

The userspace timeout is the timeout that the watchdog daemon will use.
Meaning: in this time period the daemon needs to ping the watchdog, if not
the system should reboot. So how does the timer do this? When userspace opens
the watchdog (and thus takes control) the timer will know that we should
receive a ping between now and now+timeout. In this period the timer will
reset the watchdog each 500ms (the 1/2 seconds=half of the heartbeat time).
when we are at now+timeout and we did not receive a ping the timer will stop
resetting the watchdog, which will result in a reboot (after expiration of
watchdog's real heartbeat). If in the period between now and now+timeout
userspace did receive a ping, then the timer will now that now+timeout can
be replaced by new_now+timeout. And that's how it works. So the userspace
timeout has no real relation with the watchdog's heartbeat.

Or in short the timer does the following:
1) when /dev/watchdog is not opened by the watchdog dameon, it should reset
the watchdog hardware so that the system doesn't reboot.
2) when /dev/watchdog is opened by the watchdog dameon, it needs to reset
the watchdog hardware so that the system does not reboot unless we didn't
receive a ping in the timeout period. In that case the system should be
rebooted (and we do this by not resetting the watchdog anymore).

Hope this is clearer now.

Extra remark: the value in the BIOS should indeed be chosen with some
common sense: you need to survice the boot sequence, but if the heartbeat
is much higher then the timeout value, then you're system will not reboot
before the heartbeat has passed away (which means that the heartbeat of
your system will dominate anyway).

I think we should change the default timeout value to 60 seconds instead of 15.

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Hi Wim,

> Hope this is clearer now.
>
Yes. All is clear now. Thanks. I was confusing WDT_HEARTBEAT and WDT_TIMEOUT.
I fully agree with the way the timer is implemented.

> I think we should change the default timeout value to 60 seconds instead of 15.
>
Agree on that too.

Thanks for your patience.
Kind regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Wim Van Sebroeck <wim@iguana.be> wrote:

> Hi Harald,
>
> > Unfortunately it simply doesn't work.  When I originally implemented a
> > via_wdt driver some years ago, I couldn't start the watchdog no matter
> > how hard I tried.  My inquiries into VIA at that point didn't provide
> > any response either.  In fact, I even feared that nobody might ever have
> > used that watchdog hardware before ;)
> > 
> > I can ask again, but I'm not sure if it will be any different at that
> > time..
>
> Could you please ask again? It could indeed be that they lock registers.
> (It could be something similar to some of the superIO chipsets that can
> enable/disable (lock/unlock) access to registers).
>
> The BIOS can at least start the hardware, so the hardware does work :-)
>
Hey, I just discovered that enabling Plug&Play in the BIOS is the key!

It unlocks watchdog control registers and opens access to counters and
all the stuff. Sometimes lateral thinking helps :-)

So the driver will be able to normally support all features as documented
in the datasheet. I plan an update tomorrow.

Regards,
--
Marc

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Wim Van Sebroeck]

Hi Marc,

> > The BIOS can at least start the hardware, so the hardware does work :-)
> >
> Hey, I just discovered that enabling Plug&Play in the BIOS is the key!
> 
> It unlocks watchdog control registers and opens access to counters and
> all the stuff. Sometimes lateral thinking helps :-)
> 
> So the driver will be able to normally support all features as documented
> in the datasheet. I plan an update tomorrow.

Nice !!! Please add a note about the fact that you need to enable PnP in the
driver.

Kind regards,
Wim.

Re: [PATCH RFC] watchdog: add a new driver for VIA chipsets

[Marc Vertes]

Add a new driver for the hardware watchdog timer on VIA chipsets.
This driver uses the new watchdog framework.
Remember to switch on PnP in BIOS.
Tested on a Artigo A1100, VX855 chipset.

Signed-off-by: Marc Vertes <marc.vertes@sigfox.com>

---

 Kconfig   |   14 +++
 Makefile  |    1 
 via_wdt.c |  268 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+)

--- linux-3.2-rc3/drivers/watchdog/Makefile.orig	2011-11-24 05:20:28.000000000 +0100
+++ linux-3.2-rc3/drivers/watchdog/Makefile	2011-11-25 20:41:58.098297812 +0100
@@ -99,6 +99,7 @@ obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_SMSC_SCH311X_WDT) += sch311x_wdt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
+obj-$(CONFIG_VIA_WDT) += via_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83697UG_WDT) += w83697ug_wdt.o
--- linux-3.2-rc3/drivers/watchdog/via_wdt.c.orig	2011-11-25 20:35:19.219272614 +0100
+++ linux-3.2-rc3/drivers/watchdog/via_wdt.c	2011-11-25 20:35:53.128104742 +0100
@@ -0,0 +1,268 @@
+/*
+ * VIA Chipset Watchdog Driver
+ *
+ * Copyright (C) 2011 Sigfox
+ * License terms: GNU General Public License (GPL) version 2
+ * Author: Marc Vertes <marc.vertes@sigfox.com>
+ * Based on a preliminary version from Harald Welte <HaraldWelte@viatech.com>
+ * Timer code by Wim Van Sebroeck <wim@iguana.be>
+ *
+ * Caveat: PnP must be enabled in BIOS to allow full access to watchdog
+ * control registers. If not, the watchdog must be configured in BIOS manually.
+ */
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/timer.h>
+#include <linux/watchdog.h>
+
+/* Configuration registers relative to the pci device */
+#define VIA_WDT_MMIO_BASE	0xe8	/* MMIO region base address */
+#define VIA_WDT_CONF		0xec	/* watchdog enable state */
+
+/* Relevant bits for the VIA_WDT_CONF register */
+#define VIA_WDT_CONF_ENABLE	0x01	/* 1: enable watchdog */
+#define VIA_WDT_CONF_MMIO	0x02	/* 1: enable watchdog MMIO */
+
+/*
+ * The MMIO region contains the watchog control register and the
+ * hardware timer counter.
+ */
+#define VIA_WDT_MMIO_LEN	8	/* MMIO region length in bytes */
+#define VIA_WDT_CTL		0	/* MMIO addr+0: state/control reg. */
+#define VIA_WDT_COUNT		4	/* MMIO addr+4: timer counter reg. */
+
+/* Bits for the VIA_WDT_CTL register */
+#define VIA_WDT_RUNNING		0x01	/* 0: stop, 1: running */
+#define VIA_WDT_FIRED		0x02	/* 1: restarted by expired watchdog */
+#define VIA_WDT_PWROFF		0x04	/* 0: reset, 1: poweroff */
+#define VIA_WDT_DISABLED	0x08	/* 1: timer is disabled */
+#define VIA_WDT_TRIGGER		0x80	/* 1: start a new countdown */
+
+/* Hardware heartbeat in seconds */
+#define WDT_HW_HEARTBEAT 1
+
+/* Timer heartbeat (500ms) */
+#define WDT_HEARTBEAT	(HZ/2)	/* should be <= ((WDT_HW_HEARTBEAT*HZ)/2) */
+
+/* User space timeout in seconds */
+#define WDT_TIMEOUT_MAX	1023	/* approx. 17 min. */
+#define WDT_TIMEOUT	60
+static int timeout = WDT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds, between 1 and 1023 "
+	"(default = " __MODULE_STRING(WDT_TIMEOUT) ")");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
+	"(default = " __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static struct watchdog_device wdt_dev;
+static struct resource wdt_res;
+static void __iomem *wdt_mem;
+static unsigned int mmio;
+static void wdt_timer_tick(unsigned long data);
+static DEFINE_TIMER(timer, wdt_timer_tick, 0, 0);
+					/* The timer that pings the watchdog */
+static unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
+
+static inline void wdt_reset(void)
+{
+	unsigned int ctl = readl(wdt_mem);
+
+	writel(ctl | VIA_WDT_TRIGGER, wdt_mem);
+}
+
+/*
+ * Timer tick: the timer will make sure that the watchdog timer hardware
+ * is being reset in time. The conditions to do this are:
+ *  1) the watchog timer has been started and /dev/watchdog is open
+ *     and there is still time left before userspace should send the
+ *     next heartbeat/ping. (note: the internal heartbeat is much smaller
+ *     then the external/userspace heartbeat).
+ *  2) the watchdog timer has been stopped by userspace.
+ */
+static void wdt_timer_tick(unsigned long data)
+{
+	if (time_before(jiffies, next_heartbeat) ||
+	   (!test_bit(WDOG_ACTIVE, &wdt_dev.status))) {
+		wdt_reset();
+		mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	} else
+		pr_crit("I will reboot your machine !\n");
+}
+
+static int wdt_ping(struct watchdog_device *wdd)
+{
+	/* calculate when the next userspace timeout will be */
+	next_heartbeat = jiffies + timeout * HZ;
+	return 0;
+}
+
+static int wdt_start(struct watchdog_device *wdd)
+{
+	unsigned int ctl = readl(wdt_mem);
+
+	writel(timeout, wdt_mem + VIA_WDT_COUNT);
+	writel(ctl | VIA_WDT_RUNNING | VIA_WDT_TRIGGER, wdt_mem);
+	wdt_ping(wdd);
+	mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	return 0;
+}
+
+static int wdt_stop(struct watchdog_device *wdd)
+{
+	unsigned int ctl = readl(wdt_mem);
+
+	writel(ctl & ~VIA_WDT_RUNNING, wdt_mem);
+	return 0;
+}
+
+static int wdt_set_timeout(struct watchdog_device *wdd,
+			   unsigned int new_timeout)
+{
+	if (new_timeout < 1 || new_timeout > WDT_TIMEOUT_MAX)
+		return -EINVAL;
+	writel(new_timeout, wdt_mem + VIA_WDT_COUNT);
+	timeout = new_timeout;
+	return 0;
+}
+
+static const struct watchdog_info wdt_info = {
+	.identity =	"VIA watchdog",
+	.options =	WDIOF_CARDRESET |
+			WDIOF_SETTIMEOUT |
+			WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+};
+
+static const struct watchdog_ops wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	wdt_start,
+	.stop =		wdt_stop,
+	.ping =		wdt_ping,
+	.set_timeout =	wdt_set_timeout,
+};
+
+static struct watchdog_device wdt_dev = {
+	.info =		&wdt_info,
+	.ops =		&wdt_ops,
+};
+
+static int __devinit wdt_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *ent)
+{
+	unsigned char conf;
+	int ret = -ENODEV;
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev, "cannot enable PCI device\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Allocate a MMIO region which contains watchdog control register
+	 * and counter, then configure the watchdog to use this region.
+	 * This is possible only if PnP is properly enabled in BIOS.
+	 * If not, the watchdog must be configured in BIOS manually.
+	 */
+	if (allocate_resource(&iomem_resource, &wdt_res, VIA_WDT_MMIO_LEN,
+			      0xf0000000, 0xffffff00, 0xff, NULL, NULL)) {
+		dev_err(&pdev->dev, "MMIO allocation failed\n");
+		goto err_out_disable_device;
+	}
+
+	pci_write_config_dword(pdev, VIA_WDT_MMIO_BASE, wdt_res.start);
+	pci_read_config_byte(pdev, VIA_WDT_CONF, &conf);
+	conf |= VIA_WDT_CONF_ENABLE | VIA_WDT_CONF_MMIO;
+	pci_write_config_byte(pdev, VIA_WDT_CONF, conf);
+
+	pci_read_config_dword(pdev, VIA_WDT_MMIO_BASE, &mmio);
+	if (mmio) {
+		dev_info(&pdev->dev, "VIA Chipset watchdog MMIO: %x\n", mmio);
+	} else {
+		dev_err(&pdev->dev, "MMIO setting failed. Check BIOS.\n");
+		goto err_out_resource;
+	}
+
+	if (!request_mem_region(mmio, VIA_WDT_MMIO_LEN, "via_wdt")) {
+		dev_err(&pdev->dev, "MMIO region busy\n");
+		goto err_out_resource;
+	}
+
+	wdt_mem = ioremap(mmio, VIA_WDT_MMIO_LEN);
+	if (wdt_mem == NULL) {
+		dev_err(&pdev->dev, "cannot remap VIA wdt MMIO registers\n");
+		goto err_out_release;
+	}
+
+	wdt_dev.timeout = timeout;
+	if (nowayout)
+		set_bit(WDOG_NO_WAY_OUT, &wdt_dev.status);
+
+	if (readl(wdt_mem) & VIA_WDT_FIRED)
+		wdt_dev.bootstatus |= WDIOF_CARDRESET;
+
+	ret = watchdog_register_device(&wdt_dev);
+	if (ret)
+		goto err_out_iounmap;
+
+	/* start triggering, in case of watchdog already enabled by BIOS */
+	mod_timer(&timer, jiffies + WDT_HEARTBEAT);
+	return 0;
+
+err_out_iounmap:
+	iounmap(wdt_mem);
+err_out_release:
+	release_mem_region(mmio, VIA_WDT_MMIO_LEN);
+err_out_resource:
+	release_resource(&wdt_res);
+err_out_disable_device:
+	pci_disable_device(pdev);
+	return ret;
+}
+
+static void __devexit wdt_remove(struct pci_dev *pdev)
+{
+	watchdog_unregister_device(&wdt_dev);
+	del_timer(&timer);
+	iounmap(wdt_mem);
+	release_mem_region(mmio, VIA_WDT_MMIO_LEN);
+	release_resource(&wdt_res);
+	pci_disable_device(pdev);
+}
+
+DEFINE_PCI_DEVICE_TABLE(wdt_pci_table) = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_CX700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX800) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VX855) },
+	{ 0 }
+};
+
+static struct pci_driver wdt_driver = {
+	.name		= "via_wdt",
+	.id_table	= wdt_pci_table,
+	.probe		= wdt_probe,
+	.remove		= __devexit_p(wdt_remove),
+};
+
+static int __init wdt_init(void)
+{
+	if (timeout < 1 || timeout > WDT_TIMEOUT_MAX)
+		timeout = WDT_TIMEOUT;
+	return pci_register_driver(&wdt_driver);
+}
+
+static void __exit wdt_exit(void)
+{
+	pci_unregister_driver(&wdt_driver);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_AUTHOR("Marc Vertes");
+MODULE_DESCRIPTION("Driver for watchdog timer on VIA chipset");
+MODULE_LICENSE("GPL");
--- linux-3.2-rc3/drivers/watchdog/Kconfig.orig	2011-11-24 05:20:28.000000000 +0100
+++ linux-3.2-rc3/drivers/watchdog/Kconfig	2011-11-25 20:42:46.826056558 +0100
@@ -772,6 +772,20 @@ config SMSC37B787_WDT
 
 	  Most people will say N.
 
+config VIA_WDT
+	tristate "VIA Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	---help---
+	This is the driver for the hardware watchdog timer on VIA
+	southbridge chipset CX700, VX800, VX855. Watchdog setup
+	in BIOS is required.
+
+	To compile this driver as a module, choose M here; the module
+	will be called via_wdt.
+
+	Most people will say N.
+
 config W83627HF_WDT
 	tristate "W83627HF/W83627DHG Watchdog Timer"
 	depends on X86