watchdog: Add support for the Avionic Design Xanthos watchdog timer.
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
This patch adds support for the watchdog timer on Avionic Design Xanthos boards.
--- drivers/watchdog/Kconfig | 7 + drivers/watchdog/Makefile | 1 + drivers/watchdog/adx_wdt.c | 380 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 388 insertions(+), 0 deletions(-)
drivers/watchdog/Kconfig +7 -0
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG To compile this driver as a module, choose M here: the module will be called stmp3xxx_wdt. +config ADX_WATCHDOG + tristate "Avionic Design Xanthos watchdog" + depends on ARCH_PXA_ADX + help + Say Y here if you want support for the watchdog timer on Avionic + Design Xanthos boards. + # AVR32 Architecture config AT32AP700X_WDT
drivers/watchdog/Makefile +1 -0
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o +obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
drivers/watchdog/adx_wdt.c new file +380 -0
380 lines not on this page · raw message
Signature · 1 line
tg: (64f1607..) adx/watchdog (depends on: adx/master)
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
drivers/watchdog/adx_wdt.c line 53
Quoted 8 lines · On Fri, 21 Aug 2009 15:10:02 +0200 Thierry Reding…
This patch adds support for the watchdog timer on Avionic Design Xanthos boards.
...
drivers/watchdog/adx_wdt.c new file +2
-0+static struct watchdog_info adx_wdt_info = { + .identity ="Avionic Design Xanthos Watchdog" ,
heh, that fits with zero bytes to spare.
drivers/watchdog/adx_wdt.c line 219
Quoted 9 lines
...
drivers/watchdog/adx_wdt.c new file +6
-0+static const struct file_operations adx_wdt_fops = { + .owner =THIS_MODULE , + .llseek = no_llseek, + .open = adx_wdt_open, + .release = adx_wdt_release, + .ioctl = adx_wdt_ioctl,
Nope. All other watchdog drivers use .unlocked_ioctl and this one should as well, please.
Quoted 2 lines
(quoted diff) +2
-0+ .write = adx_wdt_write, +};
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
In reply to Thierry RedingHi Thierry,
Quoted 1 line
+ *
linux/drivers/watchdog/adx_wdt.c
Please change this to adx_wdt.c . If the tree get's moved then we don't want to review every driver to get this "fixed".
drivers/watchdog/adx_wdt.c line 34
Quoted 3 lines
drivers/watchdog/adx_wdt.c new file +3
-0+struct adx_wdt_nb { +struct adx_wdt *wdt; +};
Is this being used?
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+static void adx_wdt_set_timeout (struct adx_wdt *wdt,unsigned long timeout) +{ +unsigned long flags; +unsigned int state; + +spin_lock_irqsave (&wdt->lock, flags);
drivers/watchdog/adx_wdt.c line 110
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+ state = wdt->state; +adx_wdt_stop_locked (wdt); +writel (timeout, wdt->base +ADX_WDT_TIMEOUT ); + +if (state ==WDT_STATE_START ) +adx_wdt_start_locked (wdt); + + wdt->timeout = timeout; +spin_unlock_irqrestore (&wdt->lock, flags); +}
You implemented the timeout towards userspace indeed as seconds. Internally you are using milliseconds. I would prefer to see the conversion from seconds to milliseconds taking place in the adx_wdt_set_timeout function. This will make conversion to the new watchdog api easier.
I just thought about having the internal timeout routines being in milliseconds.
Most watchdogs use seconds anyway, so that would mean a lot of multiply and divide by 1000 or additional code that isn't going to be a benefit. So I prefer to stick to seconds.
drivers/watchdog/adx_wdt.c line 133
Quoted 13 lines
drivers/watchdog/adx_wdt.c new file +13
-0+static int adx_wdt_open (struct inode *inode,struct file *file) +{ +struct adx_wdt *wdt =platform_get_drvdata (adx_wdt_dev); + +if (test_and_set_bit (0 , &driver_open)) +return -EBUSY ; + +adx_wdt_set_timeout (wdt,30 *1000 ); +adx_wdt_start (wdt); + file->private_data = wdt; + +return nonseekable_open (inode, file); +}
I prefer to have file->private_data = wdt; to be set before you start the watchdog.
drivers/watchdog/adx_wdt.c line 146
Quoted 2 lines
drivers/watchdog/adx_wdt.c new file +2
-0+static int adx_wdt_ioctl (struct inode *inode,struct file *file, +unsigned int cmd,unsigned long arg)
we use unlocked_ioctl. Ths should thus be:
static long adx_wdt_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
Quoted 8 lines
drivers/watchdog/adx_wdt.c new file +8
-0+{ +struct adx_wdt *wdt = file->private_data; +void __user *argp = (void __user *)arg; +unsigned long __user *p = argp; +unsigned long timeout =0 ; +unsigned int options; +int ret = -EINVAL ; +
drivers/watchdog/adx_wdt.c line 164
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+switch (cmd) { +case WDIOC_GETSUPPORT : +if (copy_to_user (argp, &adx_wdt_info,sizeof (adx_wdt_info))) +return -EFAULT ; +else +return 0 ; + +case WDIOC_GETSTATUS : +case WDIOC_GETBOOTSTATUS : +return put_user (!!(0 ), p);
Why not just "return put_user(0, p);"?
drivers/watchdog/adx_wdt.c line 219
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+static const struct file_operations adx_wdt_fops = { + .owner =THIS_MODULE , + .llseek = no_llseek, + .open = adx_wdt_open, + .release = adx_wdt_release, + .ioctl = adx_wdt_ioctl,
And then this becomes: .unlocked_ioctl = adx_wdt_ioctl,
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+ .write = adx_wdt_write, +}; + +static struct miscdevice adx_wdt_miscdev = { + .minor =WATCHDOG_MINOR , + .name ="watchdog" , + .fops = &adx_wdt_fops, +}; + +static irqreturn_t adx_wdt_interrupt (int irq,void *dev_id)
drivers/watchdog/adx_wdt.c line 239
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+{ +struct adx_wdt *wdt = dev_id; +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +writel (wdt->timeout, wdt->base +ADX_WDT_TIMEOUT ); +spin_unlock_irqrestore (&wdt->lock, flags); + +return IRQ_HANDLED ; +}
What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt. It's userspace that needs to control wether or not we are in a stable situation or not. See discussion with Wan ZongShun 2 to 3 weeks ago.
Quoted 49 lines
drivers/watchdog/adx_wdt.c new file +49
-0+ +static int __devinit adx_wdt_probe (struct platform_device *pdev) +{ +struct resource *res; +struct adx_wdt *wdt; +int ret =0 ; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) { +dev_err (&pdev->dev,"cannot allocate WDT structure\n" ); +return -ENOMEM ; + } + +spin_lock_init (&wdt->lock); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"cannot obtain I/O memory region\n" ); +return -ENXIO ; + } + + res =devm_request_mem_region (&pdev->dev, res->start, + res->end - res->start +1 , res->name); +if (!res) { +dev_err (&pdev->dev,"cannot request I/O memory region\n" ); +return -ENXIO ; + } + + wdt->base =devm_ioremap_nocache (&pdev->dev, res->start, + res->end - res->start +1 ); +if (!wdt->base) { +dev_err (&pdev->dev,"cannot remap I/O memory region\n" ); +return -ENXIO ; + } + + res =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); +if (!res) { +dev_err (&pdev->dev,"cannot obtain IRQ\n" ); +return -ENXIO ; + } + + wdt->irq = res->start; + + ret =devm_request_irq (&pdev->dev, wdt->irq, adx_wdt_interrupt, +IRQF_SHARED ,WATCHDOG_NAME , wdt); +if (ret <0 ) { +dev_err (&pdev->dev,"cannot request IRQ\n" ); +return -ENXIO ; + }
drivers/watchdog/adx_wdt.c line 298
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+ + ret =misc_register (&adx_wdt_miscdev); +if (ret) { +dev_err (&pdev->dev,"cannot register miscdev on minor %d " +"(err=%d)\n" ,WATCHDOG_MINOR , ret); +return ret; + } + +platform_set_drvdata (pdev, wdt); + adx_wdt_dev = pdev;
Please put these 2 lines in front of the misc_register.
drivers/watchdog/adx_wdt.c line 309
Quoted 11 lines
drivers/watchdog/adx_wdt.c new file +11
-0+ +return 0 ; +} + +static int __devexit adx_wdt_remove(struct platform_device *pdev) +{ +struct adx_wdt *wdt =platform_get_drvdata (pdev); + +platform_set_drvdata (pdev,NULL ); +misc_deregister (&adx_wdt_miscdev); +adx_wdt_stop (wdt);
and put the platform_set_drvdata after the adx_wdt_stop.
drivers/watchdog/adx_wdt.c line 144
Quoted 4 lines
drivers/watchdog/adx_wdt.c new file +4
-0+ +return 0 ; +} +
Thanks in advance,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 11 lines · Wim Van Sebroeck
Hi Thierry,
Quoted 1 line
+ *
linux/drivers/watchdog/adx_wdt.cPlease change this to
adx_wdt.c. If the tree get's moved then we don't want to review every driver to get this "fixed".drivers/watchdog/adx_wdt.c line 34
Quoted 3 lines
drivers/watchdog/adx_wdt.c new file +3
-0+struct adx_wdt_nb { +struct adx_wdt *wdt; +};Is this being used?
No. I missed it when replacing the reboot/shutdown notifier code.
Quoted 21 lines
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+static void adx_wdt_set_timeout (struct adx_wdt *wdt,unsigned long timeout) +{ +unsigned long flags; +unsigned int state; + +spin_lock_irqsave (&wdt->lock, flags);drivers/watchdog/adx_wdt.c line 110
Quoted 10 lines
drivers/watchdog/adx_wdt.c new file +10
-0+ state = wdt->state; +adx_wdt_stop_locked (wdt); +writel (timeout, wdt->base +ADX_WDT_TIMEOUT ); + +if (state ==WDT_STATE_START ) +adx_wdt_start_locked (wdt); + + wdt->timeout = timeout; +spin_unlock_irqrestore (&wdt->lock, flags); +}You implemented the timeout towards userspace indeed as seconds. Internally you are using milliseconds. I would prefer to see the conversion from seconds to milliseconds taking place in the
adx_wdt_set_timeoutfunction. This will make conversion to the new watchdog api easier.
Quoted 2 lines
I just thought about having the internal timeout routines being in milliseconds.
Most watchdogs use seconds anyway, so that would mean a lot of multiply and divide by 1000 or additional code that isn't going to be a benefit. So I prefer to stick to seconds.
Okay. I've moved the conversion to the adx_wdt_set_timeout() function and all other code now passes seconds to that function (I also renamed the parameter to "seconds" to clarify this).
Quoted 7 lines
Quoted 7 lines
drivers/watchdog/adx_wdt.c new file +7
-0+static int adx_wdt_open (struct inode *inode,struct file *file) +{ +struct adx_wdt *wdt =platform_get_drvdata (adx_wdt_dev); + +if (test_and_set_bit (0 , &driver_open)) +return -EBUSY ; +
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 133
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+adx_wdt_set_timeout (wdt,30 *1000 ); +adx_wdt_start (wdt); + file->private_data = wdt; + +return nonseekable_open (inode, file); +}I prefer to have
file->private_data= wdt; to be set before you start the watchdog.
Done.
Quoted 6 lines
drivers/watchdog/adx_wdt.c line 146
Quoted 2 lines
drivers/watchdog/adx_wdt.c new file +2
-0+static int adx_wdt_ioctl (struct inode *inode,struct file *file, +unsigned int cmd,unsigned long arg)we use
unlocked_ioctl. Ths should thus be:
static long adx_wdt_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
Done.
Quoted 12 lines
Quoted 12 lines
drivers/watchdog/adx_wdt.c new file +12
-0+{ +struct adx_wdt *wdt = file->private_data; +void __user *argp = (void __user *)arg; +unsigned long __user *p = argp; +unsigned long timeout =0 ; +unsigned int options; +int ret = -EINVAL ; + +switch (cmd) { +case WDIOC_GETSUPPORT : +if (copy_to_user (argp, &adx_wdt_info,sizeof (adx_wdt_info))) +return -EFAULT ;
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 164
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+else +return 0 ; + +case WDIOC_GETSTATUS : +case WDIOC_GETBOOTSTATUS : +return put_user (!!(0 ), p);Why not just "return
put_user(0, p);"?
Done.
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 219
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+static const struct file_operations adx_wdt_fops = { + .owner =THIS_MODULE , + .llseek = no_llseek, + .open = adx_wdt_open, + .release = adx_wdt_release, + .ioctl = adx_wdt_ioctl,And then this becomes: .unlocked_ioctl =
adx_wdt_ioctl,
Done.
Quoted 16 lines
Quoted 16 lines
drivers/watchdog/adx_wdt.c new file +16
-0+ .write = adx_wdt_write, +}; + +static struct miscdevice adx_wdt_miscdev = { + .minor =WATCHDOG_MINOR , + .name ="watchdog" , + .fops = &adx_wdt_fops, +}; + +static irqreturn_t adx_wdt_interrupt (int irq,void *dev_id) +{ +struct adx_wdt *wdt = dev_id; +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +writel (wdt->timeout, wdt->base +ADX_WDT_TIMEOUT );
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 239
Quoted 4 lines
drivers/watchdog/adx_wdt.c new file +4
-0+spin_unlock_irqrestore (&wdt->lock, flags); + +return IRQ_HANDLED ; +}What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt. It's userspace that needs to control wether or not we are in a stable situation or not. See discussion with Wan ZongShun 2 to 3 weeks ago.
The watchdog can also be programmed to only trigger an interrupt instead of resetting the hardware on timeout. In that case the timeout register needns to be written to clear the interrupt.
Quoted 53 lines
Quoted 53 lines
drivers/watchdog/adx_wdt.c new file +53
-0+ +static int __devinit adx_wdt_probe (struct platform_device *pdev) +{ +struct resource *res; +struct adx_wdt *wdt; +int ret =0 ; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) { +dev_err (&pdev->dev,"cannot allocate WDT structure\n" ); +return -ENOMEM ; + } + +spin_lock_init (&wdt->lock); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"cannot obtain I/O memory region\n" ); +return -ENXIO ; + } + + res =devm_request_mem_region (&pdev->dev, res->start, + res->end - res->start +1 , res->name); +if (!res) { +dev_err (&pdev->dev,"cannot request I/O memory region\n" ); +return -ENXIO ; + } + + wdt->base =devm_ioremap_nocache (&pdev->dev, res->start, + res->end - res->start +1 ); +if (!wdt->base) { +dev_err (&pdev->dev,"cannot remap I/O memory region\n" ); +return -ENXIO ; + } + + res =platform_get_resource (pdev,IORESOURCE_IRQ ,0 ); +if (!res) { +dev_err (&pdev->dev,"cannot obtain IRQ\n" ); +return -ENXIO ; + } + + wdt->irq = res->start; + + ret =devm_request_irq (&pdev->dev, wdt->irq, adx_wdt_interrupt, +IRQF_SHARED ,WATCHDOG_NAME , wdt); +if (ret <0 ) { +dev_err (&pdev->dev,"cannot request IRQ\n" ); +return -ENXIO ; + } + + ret =misc_register (&adx_wdt_miscdev); +if (ret) { + dev_err(&pdev->dev, "cannot register miscdev on minor %d "
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 298
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+ "(err=%d)\n", WATCHDOG_MINOR, ret); +return ret; + } + +platform_set_drvdata (pdev, wdt); + adx_wdt_dev = pdev;Please put these 2 lines in front of the
misc_register.
Done.
Quoted 5 lines
Quoted 5 lines
drivers/watchdog/adx_wdt.c new file +5
-0+ +return 0 ; +} + +static int __devexit adx_wdt_remove(struct platform_device *pdev)
Quoted 8 lines
drivers/watchdog/adx_wdt.c line 309
Quoted 6 lines
drivers/watchdog/adx_wdt.c new file +6
-0+{ +struct adx_wdt *wdt =platform_get_drvdata (pdev); + +platform_set_drvdata (pdev,NULL ); +misc_deregister (&adx_wdt_miscdev); +adx_wdt_stop (wdt);and put the
platform_set_drvdataafter theadx_wdt_stop.
Done.
Quoted 7 lines
drivers/watchdog/adx_wdt.c line 144
Quoted 4 lines
drivers/watchdog/adx_wdt.c new file +4
-0+ +return 0 ; +} +Thanks in advance,
Wim.
Thanks for the review.
Thierry
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
This patch adds support for the watchdog timer on Avionic Design Xanthos boards.
--- drivers/watchdog/Kconfig | 7 + drivers/watchdog/Makefile | 1 + drivers/watchdog/adx_wdt.c | 376 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 384 insertions(+), 0 deletions(-)
drivers/watchdog/Kconfig +7 -0
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG To compile this driver as a module, choose M here: the module will be called stmp3xxx_wdt. +config ADX_WATCHDOG + tristate "Avionic Design Xanthos watchdog" + depends on ARCH_PXA_ADX + help + Say Y here if you want support for the watchdog timer on Avionic + Design Xanthos boards. + # AVR32 Architecture config AT32AP700X_WDT
drivers/watchdog/Makefile +1 -0
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o +obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
drivers/watchdog/adx_wdt.c new file +376 -0
376 lines not on this page · raw message
Signature · 1 line
tg: (422bef8..) adx/watchdog (depends on: adx/master)
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
In reply to Thierry RedingHi Thierry,
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/watchdog/adx_wdt.c new file +7
-0+static irqreturn_t adx_wdt_interrupt (int irq,void *dev_id) +{ +struct adx_wdt *wdt = dev_id; +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +writel (wdt->timeout, wdt->base +ADX_WDT_TIMEOUT );Quoted 8 lines
drivers/watchdog/adx_wdt.c line 239
Quoted 4 lines
drivers/watchdog/adx_wdt.c new file +4
-0+spin_unlock_irqrestore (&wdt->lock, flags); + +return IRQ_HANDLED ; +}What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt. It's userspace that needs to control wether or not we are in a stable situation or not. See discussion with Wan ZongShun 2 to 3 weeks ago.
Quoted 3 lines
The watchdog can also be programmed to only trigger an interrupt instead of resetting the hardware on timeout. In that case the timeout register needns to be written to clear the interrupt.
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
Thanks in advance,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 2 lines · Wim Van Sebroeck
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
b.g.
Signature · 2 lines
Bill Gatliff bgat at billgatliff.com
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Bill,
Quoted 5 lines
Quoted 2 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
Kind regards,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 12 lines · Wim Van Sebroeck
Hi Bill,
Quoted 6 lines
Quoted 3 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
All I'm saying is that it isn't always a given that a watchdog timeout should universally issue a hardware reset. I've worked with systems where doing so would be a Really Bad Idea, and what we wanted instead was the ability to know when the watchdog timer had expired so that we could wrest control of the system away from the application.
Put another way, it isn't always the case that the _system_ has become unstable. It's more likely that the _application_ has, and there are other ways to fix that than by issuing a hardware reset.
In the bigger picture, getting all the benefits out of a watchdog timer while at the same time avoiding their limitations can be a tricky business. They're misused far more often than used properly, unfortunately. I sense that you and I both recognize that.
b.g.
Signature · 2 lines
Bill Gatliff bgat at billgatliff.com
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Bill,
Quoted 15 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
All I'm saying is that it isn't always a given that a watchdog timeout should universally issue a hardware reset. I've worked with systems where doing so would be a Really Bad Idea, and what we wanted instead was the ability to know when the watchdog timer had expired so that we could wrest control of the system away from the application.
Quoted 8 lines
Put another way, it isn't always the case that the _system_ has become unstable. It's more likely that the _application_ has, and there are other ways to fix that than by issuing a hardware reset.
In the bigger picture, getting all the benefits out of a watchdog timer while at the same time avoiding their limitations can be a tricky business. They're misused far more often than used properly, unfortunately. I sense that you and I both recognize that.
The original watchdog API was designed to make sure that unstable systems, systems that are going in a deadlock situation, can be recovered by a reboot. And this is still how we use the watchdog API. The goal is to keep the system running.
So if (and I can imagine that certain embedded applications might need this) a system is wanted to restart unstable applications, then (for me) this means that you want another functionality then what we have now. This can be an extension of the current watchdog API, but it's not said that this is the best solution.
For me the core function of a linux watchdog device driver is and remains to make sure that a system get's rebooted if it crashes.
Monitoring applications and other pheripherals looks to me additional finetuning of your system (and can indeed be usefull or needed). For applications monitoring there are imho other tools allready available (example: we used a heavily modified version of spong in the late 90's to test mail, dns, radius, ...).
Kind regards,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 5 lines · Wim Van Sebroeck
So if (and I can imagine that certain embedded applications might need this) a system is wanted to restart unstable applications, then (for me) this means that you want another functionality then what we have now. This can be an extension of the current watchdog API, but it's not said that this is the best solution.
Given the presentation of the hardware there's a fairly strong expectation from the user side that the functionality would appear via the watchdog API. In common configurations there's a very strong integration between the less serious watchdog actions and the shutdowns or resets that they generate so trying to run them via different APIs would cause issues - each API would want some idea what the other was doing.
Indeed, one common use of the options that don't cause a hard reset or shutdown is to extend the length of time that the watchdog can run for - watchdogs that support this functionality normally take configurable actions on both of the first two expiries of the watchdog (sometimes more). A system might configure the first expiry to generate an interrupt and only reset the system if the watchdog still hasn't been updated after another period. Sometimes the first period will be used to try to give the system a chance to recover more gracefully than a hard reset allows but a lot of the time it'll just be ignored so the watchdog can run for longer before it resets.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
In reply to Wim Van SebroeckQuoted 11 lines · Wim Van Sebroeck
Hi Bill,
Quoted 5 lines
Quoted 2 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
In this case, writing the timeout value to the watchdog is not actually a keepalive. This is in fact the mechanism used to reset/acknowledge the watchdog's interrupt.
Thierry
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Thierry,
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
Quoted 7 lines
Quoted 3 lines
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
In this case, writing the timeout value to the watchdog is not actually a keepalive. This is in fact the mechanism used to reset/acknowledge the watchdog's interrupt.
I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt. They can be set via toggling the correct bits of some configuration register.
How does this hardware works? Is there a datasheet available somewhere?
Kind regards,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 12 lines · Wim Van Sebroeck
Hi Thierry,
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
... because a hard reboot might not be the way he wants to resolve the situation.
If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
Quoted 8 lines
Quoted 3 lines
In this case, writing the timeout value to the watchdog is not actually a keepalive. This is in fact the mechanism used to reset/acknowledge the watchdog's interrupt.
I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt. They can be set via toggling the correct bits of some configuration register.
How does this hardware works? Is there a datasheet available somewhere?
It works exactly the same way. The bit defined by ADX_WDT_CONTROL_nRESET controls this behaviour. If set, the watchdog will only generate an interrupt, if cleared (the default) it will reboot the system.
There's currently no real datasheet available. The register definitions at the top of adx_wdt.c are pretty much all of the documentation we currently have, I'm afraid.
Quoted 2 lines
Kind regards,
Wim.
Thierry
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Thierry,
Quoted 5 lines
Quoted 4 lines
I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt. They can be set via toggling the correct bits of some configuration register.
How does this hardware works? Is there a datasheet available somewhere?
Quoted 7 lines
It works exactly the same way. The bit defined by
ADX_WDT_CONTROL_nRESETcontrols this behaviour. If set, the watchdog will only generate an interrupt, if cleared (the default) it will reboot the system.There's currently no real datasheet available. The register definitions at the top of
adx_wdt.care pretty much all of the documentation we currently have, I'm afraid.
I looked at the register definitions and then searched in your code where the ADX_WDT_CONTROL_nRESET bit of the watchdog device was initialized. I didn't find it.
So to get this driver in line with how the other drivers work, we should still do the following:
1) initialize the device at startup so that the ADX_WDT_CONTROL_nRESET is set correctly (and the device should also be disabled if it isn't done yet).
2) we can get rid of the interrupt handling because we will use the reboot behaviour.
Can you adapt your code?
Thanks in advance,
Wim.
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
This patch adds support for the watchdog timer on Avionic Design Xanthos boards.
--- drivers/watchdog/Kconfig | 7 + drivers/watchdog/Makefile | 1 + drivers/watchdog/adx_wdt.c | 354 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 0 deletions(-)
drivers/watchdog/Kconfig +7 -0
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG To compile this driver as a module, choose M here: the module will be called stmp3xxx_wdt. +config ADX_WATCHDOG + tristate "Avionic Design Xanthos watchdog" + depends on ARCH_PXA_ADX + help + Say Y here if you want support for the watchdog timer on Avionic + Design Xanthos boards. + # AVR32 Architecture config AT32AP700X_WDT
drivers/watchdog/Makefile +1 -0
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o +obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
drivers/watchdog/adx_wdt.c new file +354 -0
354 lines not on this page · raw message
Signature · 1 line
tg: (74fca6a..) adx/watchdog (depends on: adx/master)
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Thierry,
it's in the linux-watchdog-next tree now.
Kind regards,
Wim.
Quoted 404 lines
This patch adds support for the watchdog timer on Avionic Design Xanthos boards.
Signed-off-by Thierry Reding <[email protected]>Acked-by Russell King <[email protected]>--- drivers/watchdog/Kconfig | 7 + drivers/watchdog/Makefile | 1 + drivers/watchdog/adx_wdt.c | 354 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 0 deletions(-)drivers/watchdog/Kconfig +7
-0@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG To compile this driver as a module, choose M here: the module will be called stmp3xxx_wdt. +config ADX_WATCHDOG + tristate "Avionic Design Xanthos watchdog" + depends on ARCH_PXA_ADX + help + Say Y here if you want support for the watchdog timer on Avionic + Design Xanthos boards. + # AVR32 Architecture config AT32AP700X_WDTdrivers/watchdog/Makefile +1
-0@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o +obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.odrivers/watchdog/adx_wdt.c new file +354
-0@@ -0,0 +1,354 @@ +/* +* Copyright (C) 2008-2009 Avionic Design GmbH +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#include <linux/fs.h> +#include <linux/io.h> +#include <linux/miscdevice.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/types.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> + +#define WATCHDOG_NAME "adx-wdt" + +/* register offsets */ +#define ADX_WDT_CONTROL 0x00 +#define ADX_WDT_CONTROL_ENABLE (1 << 0) +#define ADX_WDT_CONTROL_nRESET (1 << 1) +#define ADX_WDT_TIMEOUT 0x08 + +static struct platform_device *adx_wdt_dev; +static unsigned long driver_open; + +#define WDT_STATE_STOP 0 +#define WDT_STATE_START 1 + +struct adx_wdt { +void __iomem *base; +unsigned long timeout; +unsigned int state; +unsigned int wake; +spinlock_t lock; +}; + +static struct watchdog_info adx_wdt_info = { + .identity ="Avionic Design Xanthos Watchdog" , + .options =WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING , +}; + +static void adx_wdt_start_locked (struct adx_wdt *wdt) +{ +u32 ctrl; + + ctrl =readl (wdt->base +ADX_WDT_CONTROL ); + ctrl |=ADX_WDT_CONTROL_ENABLE ; +writel (ctrl, wdt->base +ADX_WDT_CONTROL ); + wdt->state =WDT_STATE_START ; +} + +static void adx_wdt_start (struct adx_wdt *wdt) +{ +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +adx_wdt_start_locked (wdt); +spin_unlock_irqrestore (&wdt->lock, flags); +} + +static void adx_wdt_stop_locked (struct adx_wdt *wdt) +{ +u32 ctrl; + + ctrl =readl (wdt->base +ADX_WDT_CONTROL ); + ctrl &= ~ADX_WDT_CONTROL_ENABLE ; +writel (ctrl, wdt->base +ADX_WDT_CONTROL ); + wdt->state =WDT_STATE_STOP ; +} + +static void adx_wdt_stop (struct adx_wdt *wdt) +{ +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +adx_wdt_stop_locked (wdt); +spin_unlock_irqrestore (&wdt->lock, flags); +} + +static void adx_wdt_set_timeout (struct adx_wdt *wdt,unsigned long seconds) +{ +unsigned long timeout = seconds *1000 ; +unsigned long flags; +unsigned int state; + +spin_lock_irqsave (&wdt->lock, flags); + state = wdt->state; +adx_wdt_stop_locked (wdt); +writel (timeout, wdt->base +ADX_WDT_TIMEOUT ); + +if (state ==WDT_STATE_START ) +adx_wdt_start_locked (wdt); + + wdt->timeout = timeout; +spin_unlock_irqrestore (&wdt->lock, flags); +} + +static void adx_wdt_get_timeout (struct adx_wdt *wdt,unsigned long *seconds) +{ + *seconds = wdt->timeout /1000 ; +} + +static void adx_wdt_keepalive (struct adx_wdt *wdt) +{ +unsigned long flags; + +spin_lock_irqsave (&wdt->lock, flags); +writel (wdt->timeout, wdt->base +ADX_WDT_TIMEOUT ); +spin_unlock_irqrestore (&wdt->lock, flags); +} + +static int adx_wdt_open (struct inode *inode,struct file *file) +{ +struct adx_wdt *wdt =platform_get_drvdata (adx_wdt_dev); + +if (test_and_set_bit (0 , &driver_open)) +return -EBUSY ; + + file->private_data = wdt; +adx_wdt_set_timeout (wdt,30 ); +adx_wdt_start (wdt); + +return nonseekable_open (inode, file); +} + +static int adx_wdt_release (struct inode *inode,struct file *file) +{ +struct adx_wdt *wdt = file->private_data; + +adx_wdt_stop (wdt); +clear_bit (0 , &driver_open); + +return 0 ; +} + +static long adx_wdt_ioctl (struct file *file,unsigned int cmd,unsigned long arg) +{ +struct adx_wdt *wdt = file->private_data; +void __user *argp = (void __user *)arg; +unsigned long __user *p = argp; +unsigned long seconds =0 ; +unsigned int options; +long ret = -EINVAL ; + +switch (cmd) { +case WDIOC_GETSUPPORT : +if (copy_to_user (argp, &adx_wdt_info,sizeof (adx_wdt_info))) +return -EFAULT ; +else +return 0 ; + +case WDIOC_GETSTATUS : +case WDIOC_GETBOOTSTATUS : +return put_user (0 , p); + +case WDIOC_KEEPALIVE : +adx_wdt_keepalive (wdt); +return 0 ; + +case WDIOC_SETTIMEOUT : +if (get_user (seconds, p)) +return -EFAULT ; + +adx_wdt_set_timeout (wdt, seconds); + +/* fallthrough */ +case WDIOC_GETTIMEOUT : +adx_wdt_get_timeout (wdt, &seconds); +return put_user (seconds, p); + +case WDIOC_SETOPTIONS : +if (copy_from_user (&options, argp,sizeof (options))) +return -EFAULT ; + +if (options &WDIOS_DISABLECARD ) { +adx_wdt_stop (wdt); + ret =0 ; + } + +if (options &WDIOS_ENABLECARD ) { +adx_wdt_start (wdt); + ret =0 ; + } + +return ret; + +default : +break ; + } + +return -ENOTTY ; +} + +static ssize_t adx_wdt_write (struct file *file,const char __user *data, +size_t len,loff_t *ppos) +{ +struct adx_wdt *wdt = file->private_data; + +if (len) +adx_wdt_keepalive (wdt); + +return len; +} + +static const struct file_operations adx_wdt_fops = { + .owner =THIS_MODULE , + .llseek = no_llseek, + .open = adx_wdt_open, + .release = adx_wdt_release, + .unlocked_ioctl = adx_wdt_ioctl, + .write = adx_wdt_write, +}; + +static struct miscdevice adx_wdt_miscdev = { + .minor =WATCHDOG_MINOR , + .name ="watchdog" , + .fops = &adx_wdt_fops, +}; + +static int __devinit adx_wdt_probe (struct platform_device *pdev) +{ +struct resource *res; +struct adx_wdt *wdt; +int ret =0 ; +u32 ctrl; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) { +dev_err (&pdev->dev,"cannot allocate WDT structure\n" ); +return -ENOMEM ; + } + +spin_lock_init (&wdt->lock); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"cannot obtain I/O memory region\n" ); +return -ENXIO ; + } + + res =devm_request_mem_region (&pdev->dev, res->start, + res->end - res->start +1 , res->name); +if (!res) { +dev_err (&pdev->dev,"cannot request I/O memory region\n" ); +return -ENXIO ; + } + + wdt->base =devm_ioremap_nocache (&pdev->dev, res->start, + res->end - res->start +1 ); +if (!wdt->base) { +dev_err (&pdev->dev,"cannot remap I/O memory region\n" ); +return -ENXIO ; + } + +/* disable watchdog and reboot on timeout */ + ctrl =readl (wdt->base +ADX_WDT_CONTROL ); + ctrl &= ~ADX_WDT_CONTROL_ENABLE ; + ctrl &= ~ADX_WDT_CONTROL_nRESET; +writel (ctrl, wdt->base +ADX_WDT_CONTROL ); + +platform_set_drvdata (pdev, wdt); + adx_wdt_dev = pdev; + + ret =misc_register (&adx_wdt_miscdev); +if (ret) { +dev_err (&pdev->dev,"cannot register miscdev on minor %d " +"(err=%d)\n" ,WATCHDOG_MINOR , ret); +return ret; + } + +return 0 ; +} + +static int __devexit adx_wdt_remove(struct platform_device *pdev) +{ +struct adx_wdt *wdt =platform_get_drvdata (pdev); + +misc_deregister (&adx_wdt_miscdev); +adx_wdt_stop (wdt); +platform_set_drvdata (pdev,NULL ); + +return 0 ; +} + +static void adx_wdt_shutdown (struct platform_device *pdev) +{ +struct adx_wdt *wdt =platform_get_drvdata (pdev); +adx_wdt_stop (wdt); +} + +#ifdef CONFIG_PM +static int adx_wdt_suspend (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct adx_wdt *wdt =platform_get_drvdata (pdev); + + wdt->wake = (wdt->state ==WDT_STATE_START ) ?1 :0 ; +adx_wdt_stop (wdt); + +return 0 ; +} + +static int adx_wdt_resume (struct device *dev) +{ +struct platform_device *pdev =to_platform_device (dev); +struct adx_wdt *wdt =platform_get_drvdata (pdev); + +if (wdt->wake) +adx_wdt_start (wdt); + +return 0 ; +} + +static struct dev_pm_ops adx_wdt_pm_ops = { + .suspend = adx_wdt_suspend, + .resume = adx_wdt_resume, +}; + +# define ADX_WDT_PM_OPS (&adx_wdt_pm_ops) +#else +# define ADX_WDT_PM_OPS NULL +#endif + +static struct platform_driver adx_wdt_driver = { + .probe = adx_wdt_probe, + .remove =__devexit_p (adx_wdt_remove), + .shutdown = adx_wdt_shutdown, + .driver = { + .name =WATCHDOG_NAME , + .owner =THIS_MODULE , + .pm =ADX_WDT_PM_OPS , + }, +}; + +static int __init adx_wdt_init (void ) +{ +return platform_driver_register (&adx_wdt_driver); +} + +static void __exit adx_wdt_exit(void) +{ +platform_driver_unregister (&adx_wdt_driver); +} + +module_init (adx_wdt_init); +module_exit (adx_wdt_exit); + +MODULE_DESCRIPTION ("Avionic Design Xanthos Watchdog Driver" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_AUTHOR ("Thierry Reding <[email protected]>" ); +MODULE_ALIAS_MISCDEV (WATCHDOG_MINOR );Signature · 1 line
tg: (74fca6a..) adx/watchdog (depends on: adx/master)
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Quoted 6 lines · Wim Van Sebroeck
Hi Thierry,
it's in the linux-watchdog-next tree now.
Kind regards,
Wim.
Great, thanks!
Thierry
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
In reply to Wim Van SebroeckQuoted 5 lines · Wim Van Sebroeck
Hi Thierry,
Quoted 1 line
+ *
linux/drivers/watchdog/adx_wdt.cPlease change this to
adx_wdt.c. If the tree get's moved then we don't want to review every driver to get this "fixed".
Is it okay with you if I just drop this line altogether? It really just noise anyway.
Thierry
PATCH 1/1 watchdog: Add support for the Avionic Design Xanthos watchdog timer.
Hi Thierry,
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+ *
linux/drivers/watchdog/adx_wdt.cPlease change this to
adx_wdt.c. If the tree get's moved then we don't want to review every driver to get this "fixed".Is it okay with you if I just drop this line altogether? It really just noise anyway.
No problem :-).
Kind regards,
Wim.