watchdog: ARM mpcore Improvements
Subsystems (2)
v3 cover watchdog: ARM mpcore Improvements
** This still doesn't make it workable **
Hi,
ARM mpcore watchdog isn't workable and so is marked broken in the first patch of the series. Reasons are mentioned in 01/14 and important links are shared too.
Other are basically improvements which I have written more than a year back. I am just pushing them through as they still improve the driver (or broken driver). They still don't fix the issues pointed out by Marc Zyngier earlier. But as these are generic improvements I don't see why they can't be applied, even to a still broken driver.
I don't want somebody to waste time reinventing these patches, rather they can work on fixing the issues due to which this driver is broken.
I don't have hardware & time to fix the BROKEN part of this driver now :(
Link to the last version, nothing much changed after that. Just a resend:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/089076.html
Viresh Kumar (14):
watchdog/mpcore_wdt: Mark it as BROKEN watchdog/mpcore_wdt: convert to watchdog core watchdog/mpcore_wdt: Fix multiline comments watchdog/mpcore_wdt: Arrange #includes in alphabetical order watchdog/mpcore_wdt: Set default heartbeat in probe instead of init watchdog/mpcore_wdt: convert to use module_platform_driver() watchdog/mpcore_wdt: Add support for dev_pm_ops interface watchdog/mpcore_wdt: disable wdt in suspend only if it is busy watchdog/mpcore_wdt: replace (__raw_)readl/writel with lighter
*_relaxed variants
watchdog/mpcore_wdt: Add support for WDIOC_GETBOOTSTATUS IOCTL watchdog/mpcore_wdt: Add clock framework support watchdog/mpcore_wdt: use correct clk_rate to program timeout watchdog/mpcore_wdt: Start registers from 0x00 instead of 0x20 watchdog/mpcore_wdt: Add DT probing support for ARM mpcore watchdog
arch/arm/include/asm/smp_twd.h | 7 - drivers/watchdog/Kconfig | 3 +- drivers/watchdog/mpcore_wdt.c | 529 ++++++++++++++++++++--------------------- 3 files changed, 256 insertions(+), 283 deletions(-)
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
This driver was broken since ever.
- Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu interrupt (usually interrupt #30), and the GIC configuration should flag it as such. With this setup, request_irq() should fail, and the right API is request_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq().
- Nothing ensures the userspace ioctl() will end-up kicking the watchdog on the right CPU.
Was last discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html
Lets mark it broken until somebody with this hardware gets up and fixes it.
--- drivers/watchdog/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/Kconfig +1 -1
@@ -223,7 +223,7 @@ config DW_WATCHDOG config MPCORE_WATCHDOG tristate "MPcore watchdog" - depends on HAVE_ARM_TWD + depends on HAVE_ARM_TWD && BROKEN help Watchdog timer embedded into the MPcore system.
Signature · 1 line
1.7.12.rc2.18.g61b472e
Guenter Roeck
maintainerPATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Quoted 7 lines · Viresh Kumar
This driver was broken since ever.
- Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu interrupt (usually interrupt #30), and the GIC configuration should flag it as such. With this setup,
request_irq()should fail, and the right API isrequest_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq().
Quoted 9 lines
- Nothing ensures the userspace
ioctl()will end-up kicking the watchdog on the right CPU.Was last discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html
Lets mark it broken until somebody with this hardware gets up and fixes it.
I must be missing something. What is the point of the remaining patches in this case ?
Guenter
Quoted 27 lines
Suggested-by Marc Zyngier <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- drivers/watchdog/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/watchdog/Kconfig +1
-1@@ -223,7 +223,7 @@ config DW_WATCHDOG config MPCORE_WATCHDOG tristate "MPcore watchdog" - depends on HAVE_ARM_TWD + depends on HAVE_ARM_TWD && BROKEN help Watchdog timer embedded into the MPcore system.Signature · 6 lines
1.7.12.rc2.18.g61b472e -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Quoted 11 lines · Guenter Roeck
Quoted 10 lines · Viresh Kumar
This driver was broken since ever.
- Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu interrupt (usually interrupt #30), and the GIC configuration should flag it as such. With this setup,
request_irq()should fail, and the right API isrequest_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq().- Nothing ensures the userspace
ioctl()will end-up kicking the watchdog on the right CPU.
Quoted 8 lines
Quoted 6 lines
Was last discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html
Lets mark it broken until somebody with this hardware gets up and fixes it.
I must be missing something. What is the point of the remaining patches in this case ?
Indeed. This looks like pointless churn to me, unless someone actually picks up the driver and fixes it for good.
If nobody cares enough about it, then maybe it should be moved into staging and eventually retired...
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Quoted 18 lines · Marc Zyngier
Quoted 17 lines · Guenter Roeck
Quoted 16 lines · Viresh Kumar
This driver was broken since ever.
- Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu interrupt (usually interrupt #30), and the GIC configuration should flag it as such. With this setup,
request_irq()should fail, and the right API isrequest_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq().- Nothing ensures the userspace
ioctl()will end-up kicking the watchdog on the right CPU.Was last discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html
Lets mark it broken until somebody with this hardware gets up and fixes it.
Quoted 8 lines
Quoted 2 lines
I must be missing something. What is the point of the remaining patches in this case ?
Indeed. This looks like pointless churn to me, unless someone actually picks up the driver and fixes it for good.
If nobody cares enough about it, then maybe it should be moved into staging and eventually retired...
That was a year ago, and nobody has done anything to the driver. Just remove it -- if someone wants to do the work later on it's easy to revert the commit and start over.
Keeping code in the kernel but marking it BROKEN is only useful if we think someone will fix it soon. It seems very unlikely in this case.
-Olof
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Wow!! So many replies, let me reply to everyone in this chain.
Quoted 8 lines · Olof Johansson
Quoted 7 lines · Marc Zyngier
Quoted 6 lines · Guenter Roeck
Quoted 2 lines · Viresh Kumar
Lets mark it broken until somebody with this hardware gets up and fixes it.
I must be missing something. What is the point of the remaining patches in this case ?
In case somebody wakes up and tries to fix this driver, he doesn't have to write stuff which I already wrote. That's it. This stuff was pending in my tree for more than a year now and I wanted to get rid of it (without deleting it) :)
Quoted 7 lines
Quoted 5 lines
Indeed. This looks like pointless churn to me, unless someone actually picks up the driver and fixes it for good.
If nobody cares enough about it, then maybe it should be moved into staging and eventually retired...
Quoted 6 lines
That was a year ago, and nobody has done anything to the driver. Just remove it -- if someone wants to do the work later on it's easy to revert the commit and start over.
Keeping code in the kernel but marking it BROKEN is only useful if we think someone will fix it soon. It seems very unlikely in this case.
I believed that this is the driver which will be used by all cortex family, i.e. all ARM SMP platforms, isn't it? I am sure atleast the A9 family had this.
If no, then which ones are the real users of this driver/hardware? If yes, Why isn't anybody using this?
I will send a patch that will delete this driver and will provide link to my patches, in case somebody wants it back in future.
--
Viresh
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Quoted 8 lines · Viresh Kumar
Wow!! So many replies, let me reply to everyone in this chain.
Quoted 4 lines · Olof Johansson
Quoted 2 lines · Marc Zyngier
Quoted 1 line · Guenter Roeck
On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Lets mark it broken until somebody with this hardware gets up and fixes it.
I must be missing something. What is the point of the remaining patches in this
case ?In case somebody wakes up and tries to fix this driver, he doesn't have
to
Quoted 1 line
write stuff which I already wrote. That's it. This stuff was pending in
my
Quoted 15 lines
tree
for more than a year now and I wanted to get rid of it (without deleting it) :)Quoted 11 lines
Quoted 5 lines
Indeed. This looks like pointless churn to me, unless someone actually picks up the driver and fixes it for good.
If nobody cares enough about it, then maybe it should be moved into staging and eventually retired...
That was a year ago, and nobody has done anything to the driver. Just remove it -- if someone wants to do the work later on it's easy to revert the commit and start over.
Quoted 6 lines
Quoted 2 lines
Keeping code in the kernel but marking it BROKEN is only useful if we think someone will fix it soon. It seems very unlikely in this case.
I believed that this is the driver which will be used by all cortex family, i.e.
all ARM SMP platforms, isn't it? I am sure atleast the A9 family had
this.
ARM11, A5 and A9 in their MP configurations only.
Quoted 2 lines
If no, then which ones are the real users of this driver/hardware? If yes, Why isn't anybody using this?
Because, as Russell mentioned, the piece of IP doesn't fit our watchdog model at all (per-CPU watchdog???), and most SoCs/boards have a separate watchdog anyway.
Quoted 1 line
I will send a patch that will delete this driver and will provide link
to
Quoted 2 lines
my
patches, in case somebody wants it back in future.
Thanks.
M.
Signature · 1 line
Fast, cheap, reliable. Pick two.
PATCH v3 1/14 watchdog/mpcore_wdt: Mark it as BROKEN
Quoted 8 lines · Marc Zyngier
ARM11, A5 and A9 in their MP configurations only.
Quoted 2 lines
If no, then which ones are the real users of this driver/hardware? If yes, Why isn't anybody using this?
Because, as Russell mentioned, the piece of IP doesn't fit our watchdog model at all (per-CPU watchdog???), and most SoCs/boards have a separate watchdog anyway.
I believe we should use mpcore watchdogs. Atleast on the platforms I have worked (SPEAr), they don't have an external watchdog.
But yes, this would require somebody, with a A5/9/ARM11 board, no separate watchdog and a customer requirement, to work on it :)
I will get rid of it.
Thanks.
PATCH v3 2/14 watchdog/mpcore_wdt: convert to watchdog core
In reply to Viresh KumarThis patch converts existing mpcore watchdog driver to use already in place common infrastructure present in watchdog core. With this lot of code goes away.
--- drivers/watchdog/Kconfig | 1 + drivers/watchdog/mpcore_wdt.c | 275 ++++++++++-------------------------------- 2 files changed, 68 insertions(+), 208 deletions(-)
drivers/watchdog/Kconfig +1 -0
@@ -224,6 +224,7 @@ config DW_WATCHDOG config MPCORE_WATCHDOG tristate "MPcore watchdog" depends on HAVE_ARM_TWD && BROKEN + select WATCHDOG_CORE help Watchdog timer embedded into the MPcore system.
drivers/watchdog/mpcore_wdt.c +67 -208
406 lines not on this page · raw message
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 3/14 watchdog/mpcore_wdt: Fix multiline comments
In reply to Viresh KumarIn case of few multiline comment a 'tab' is present instead of 'space' after the '*' in the second column. Replace tab with space here.
--- drivers/watchdog/mpcore_wdt.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)
drivers/watchdog/mpcore_wdt.c +20 -22
@@ -1,23 +1,21 @@/* -* Watchdog driver for the mpcore watchdog timer +* Watchdog driver for the mpcore watchdog timer * -* (c) Copyright 2004 ARM Limited +* (c) Copyright 2004 ARM Limited * -* Based on the SoftDog driver: -* (c) Copyright 1996 Alan Cox <[email protected]>, -* All Rights Reserved. +* Based on the SoftDog driver: +* (c) Copyright 1996 Alan Cox <[email protected]>, All Rights Reserved. * -* This program is free software; you can redistribute it and/or -* modify it under the terms of the GNU General Public License -* as published by the Free Software Foundation; either version -* 2 of the License, or (at your option) any later version. +* This program is 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. * -* Neither Alan Cox nor CymruNet Ltd. admit liability nor provide -* warranty for any of this software. This material is provided -* "AS-IS" and at no charge. -* -* (c) Copyright 1995 Alan Cox <[email protected]> +* Neither Alan Cox nor CymruNet Ltd. admit liability nor provide +* warranty for any of this software. This material is provided +* "AS-IS" and at no charge. * +* (c) Copyright 1995 Alan Cox <[email protected]> */ #define pr_fmt (fmt) KBUILD_MODNAME ": " fmt
@@ -67,8 +65,8 @@ MODULE_PARM_DESC(mpcore_noboot, "MPcore watchdog action, " __MODULE_STRING(ONLY_TESTING) ")");/* -* This is the interrupt handler. Note that we only use this -* in testing mode, so don't actually do a reboot here. +* This is the interrupt handler. Note that we only use this +* in testing mode, so don't actually do a reboot here. */ static irqreturn_t mpcore_wdt_fire (int irq,void *arg) {
@@ -85,11 +83,11 @@ static irqreturn_t mpcore_wdt_fire(int irq, void *arg) }/* -* mpcore_wdt_ping - reload the timer +* mpcore_wdt_ping - reload the timer * -* Note that the spec says a DIFFERENT value must be written to the reload -* register each time. The "perturb" variable deals with this by adding 1 -* to the count every other time the function is called. +* Note that the spec says a DIFFERENT value must be written to the reload +* register each time. The "perturb" variable deals with this by adding 1 to +* the count every other time the function is called. */ static int mpcore_wdt_ping (struct watchdog_device *wdd) {
@@ -165,8 +163,8 @@ static const struct watchdog_ops mpcore_wdt_ops = { };/* -* System shutdown handler. Turn off the watchdog if we're -* restarting or halting the system. +* System shutdown handler. Turn off the watchdog if we're restarting or +* halting the system. */ static void mpcore_wdt_shutdown (struct platform_device *pdev) {
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 4/14 watchdog/mpcore_wdt: Arrange #includes in alphabetical order
In reply to Viresh Kumar--- drivers/watchdog/mpcore_wdt.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/watchdog/mpcore_wdt.c +6 -6
@@ -20,16 +20,16 @@#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt -#include <linux/module.h> -#include <linux/moduleparam.h> -#include <linux/types.h> -#include <linux/watchdog.h> -#include <linux/reboot.h> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/moduleparam.h> #include <linux/platform_device.h> +#include <linux/reboot.h> #include <linux/slab.h> -#include <linux/io.h> +#include <linux/types.h> +#include <linux/watchdog.h> #include <asm/smp_twd.h>
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 5/14 watchdog/mpcore_wdt: Set default heartbeat in probe instead of init
In reply to Viresh KumarCurrently mpcore_margin is handled in mpcore_wdt_init routine, which will be called even if we haven't added any device for watchdog.
This patch moves this code into probe routine, so that it only gets hit once we add device for this driver.
This also uses dev_info() style print statements instead of printk()
--- drivers/watchdog/mpcore_wdt.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
drivers/watchdog/mpcore_wdt.c +15 -14
@@ -224,6 +224,21 @@ static int mpcore_wdt_probe(struct platform_device *pdev) goto err_register; } +/* +* Check that the mpcore_margin value is within it's range; if not reset +* to the default +*/ +if (mpcore_margin <MIN_TIME || mpcore_margin >MAX_TIME ) { + mpcore_margin =TIMER_MARGIN ; +dev_info (wdt->dev,"mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n" , +TIMER_MARGIN ); + } + +mpcore_wdt_set_heartbeat (NULL , mpcore_margin); +dev_info (wdt->dev,"MPcore Watchdog Timer: 0.1. mpcore_noboot=%d " +"mpcore_margin=%d sec (nowayout= %d)\n" , mpcore_noboot, + mpcore_margin, nowayout); +return 0 ; err_register:
@@ -281,20 +296,6 @@ static struct platform_driver mpcore_wdt_driver = {static int __init mpcore_wdt_init (void ) { -/* -* Check that the mpcore_margin value is within it's range; -* if not reset to the default -*/ -if (mpcore_margin <MIN_TIME || mpcore_margin >MAX_TIME ) { - mpcore_margin =TIMER_MARGIN ; -pr_info ("mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n" , -TIMER_MARGIN ); - } - -mpcore_wdt_set_heartbeat (NULL , mpcore_margin); -pr_info ("MPcore Watchdog Timer: 0.1. mpcore_noboot=%d mpcore_margin=%d sec (nowayout= %d)\n" , - mpcore_noboot, mpcore_margin, nowayout); -return platform_driver_register (&mpcore_wdt_driver); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 6/14 watchdog/mpcore_wdt: convert to use module_platform_driver()
In reply to Viresh KumarThis patch converts the mpcore_wdt driver to use the module_platform_driver() macro which makes the code smaller and a bit simpler.
--- drivers/watchdog/mpcore_wdt.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
drivers/watchdog/mpcore_wdt.c +1 -12
@@ -294,18 +294,7 @@ static struct platform_driver mpcore_wdt_driver = { }, }; -static int __init mpcore_wdt_init (void ) -{ -return platform_driver_register (&mpcore_wdt_driver); -} - -static void __exit mpcore_wdt_exit(void) -{ -platform_driver_unregister (&mpcore_wdt_driver); -} - -module_init (mpcore_wdt_init); -module_exit (mpcore_wdt_exit); +module_platform_driver (mpcore_wdt_driver);MODULE_AUTHOR ("ARM Limited" );MODULE_DESCRIPTION ("MPcore Watchdog Device Driver" );
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 7/14 watchdog/mpcore_wdt: Add support for dev_pm_ops interface
In reply to Viresh KumarThis patch adds dev_pm_ops support for standby/slepp/hibernate.
--- drivers/watchdog/mpcore_wdt.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +9 -9
@@ -26,6 +26,7 @@#include <linux/module.h> #include <linux/moduleparam.h> #include <linux/platform_device.h> +#include <linux/pm.h> #include <linux/reboot.h> #include <linux/slab.h> #include <linux/types.h>
@@ -257,40 +258,39 @@ static int mpcore_wdt_remove(struct platform_device *pdev) }#ifdef CONFIG_PM -static int mpcore_wdt_suspend (struct platform_device *pdev,pm_message_t msg) +static int mpcore_wdt_suspend (struct device *dev) { -struct mpcore_wdt *wdt =platform_get_drvdata (pdev); +struct mpcore_wdt *wdt =dev_get_drvdata (dev);mpcore_wdt_stop (&wdt->wdd);/* Turn the WDT off */ return 0 ; } -static int mpcore_wdt_resume (struct platform_device *pdev) +static int mpcore_wdt_resume (struct device *dev) { -struct mpcore_wdt *wdt =platform_get_drvdata (pdev); +struct mpcore_wdt *wdt =dev_get_drvdata (dev);if (watchdog_active (&wdt->wdd))mpcore_wdt_start (&wdt->wdd);return 0 ; } -#else -#define mpcore_wdt_suspend NULL -#define mpcore_wdt_resume NULL#endif +static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend, + mpcore_wdt_resume); +/* work with hotplug and coldplug */ MODULE_ALIAS ("platform:mpcore_wdt" );static struct platform_driver mpcore_wdt_driver = { .probe = mpcore_wdt_probe, .remove = mpcore_wdt_remove, - .suspend = mpcore_wdt_suspend, - .resume = mpcore_wdt_resume, .shutdown = mpcore_wdt_shutdown, .driver = { .owner =THIS_MODULE , .name ="mpcore_wdt" , + .pm = &mpcore_wdt_dev_pm_ops, }, };
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 8/14 watchdog/mpcore_wdt: disable wdt in suspend only if it is busy
In reply to Viresh KumarWe don't need to call mpcore_wdt_stop() in suspend, if wdt is not BUSY. So, call mpcore_wdt_stop() conditionally in suspend.
--- drivers/watchdog/mpcore_wdt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/watchdog/mpcore_wdt.c +3 -1
@@ -261,7 +261,9 @@ static int mpcore_wdt_remove(struct platform_device *pdev)static int mpcore_wdt_suspend (struct device *dev) {struct mpcore_wdt *wdt =dev_get_drvdata (dev); -mpcore_wdt_stop (&wdt->wdd);/* Turn the WDT off */ + +if (watchdog_active (&wdt->wdd)) +mpcore_wdt_stop (&wdt->wdd);/* Turn the WDT off */ return 0 ; }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 9/14 watchdog/mpcore_wdt: replace (__raw_)readl/writel with lighter *_relaxed variants
In reply to Viresh Kumarreadl/writel versions for ARMV6+ contain memory barrier instruction for synchronizing DMA buffers. These are not required at least on this module. So use lighter _relaxed variants.
Also, __raw_* variants don't have take care about endianess, so replace them too with _relaxed variants.
--- drivers/watchdog/mpcore_wdt.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +9 -9
@@ -74,10 +74,10 @@ static irqreturn_t mpcore_wdt_fire(int irq, void *arg)struct mpcore_wdt *wdt = arg;/* Check it really was our interrupt */ -if (readl (wdt->base +TWD_WDOG_INTSTAT )) { +if (readl_relaxed (wdt->base +TWD_WDOG_INTSTAT )) {dev_crit (wdt->dev,"Triggered - Reboot ignored\n" );/* Clear the interrupt on the watchdog */ -writel (1 , wdt->base +TWD_WDOG_INTSTAT ); +writel_relaxed (1 , wdt->base +TWD_WDOG_INTSTAT );return IRQ_HANDLED ; }return IRQ_NONE ;
@@ -97,12 +97,12 @@ static int mpcore_wdt_ping(struct watchdog_device *wdd)spin_lock (&wdt->lock);/* Assume prescale is set to 256 */ - count =__raw_readl (wdt->base +TWD_WDOG_COUNTER ); + count =readl_relaxed (wdt->base +TWD_WDOG_COUNTER ); count = (0xFFFFFFFFU - count) * (HZ /5 ); count = (count /256 ) * mpcore_margin;/* Reload the counter */ -writel (count + wdt->perturb, wdt->base +TWD_WDOG_LOAD ); +writel_relaxed (count + wdt->perturb, wdt->base +TWD_WDOG_LOAD ); wdt->perturb = wdt->perturb ?0 :1 ;spin_unlock (&wdt->lock);
@@ -114,9 +114,9 @@ static int mpcore_wdt_stop(struct watchdog_device *wdd)struct mpcore_wdt *wdt =watchdog_get_drvdata (wdd);spin_lock (&wdt->lock); -writel (0x12345678 , wdt->base +TWD_WDOG_DISABLE ); -writel (0x87654321 , wdt->base +TWD_WDOG_DISABLE ); -writel (0x0 , wdt->base +TWD_WDOG_CONTROL ); +writel_relaxed (0x12345678 , wdt->base +TWD_WDOG_DISABLE ); +writel_relaxed (0x87654321 , wdt->base +TWD_WDOG_DISABLE ); +writel_relaxed (0x0 , wdt->base +TWD_WDOG_CONTROL );spin_unlock (&wdt->lock);return 0 ;
@@ -133,10 +133,10 @@ static int mpcore_wdt_start(struct watchdog_device *wdd)if (mpcore_noboot) {/* Enable watchdog - prescale=256, watchdog mode=0, enable=1 */ -writel (0x0000FF01 , wdt->base +TWD_WDOG_CONTROL ); +writel_relaxed (0x0000FF01 , wdt->base +TWD_WDOG_CONTROL ); }else {/* Enable watchdog - prescale=256, watchdog mode=1, enable=1 */ -writel (0x0000FF09 , wdt->base +TWD_WDOG_CONTROL ); +writel_relaxed (0x0000FF09 , wdt->base +TWD_WDOG_CONTROL ); }return 0 ;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 10/14 watchdog/mpcore_wdt: Add support for WDIOC_GETBOOTSTATUS IOCTL
In reply to Viresh Kumarmpcore watchdog can give last boot status, whether we got a watchdog reset or not, via bit 0 of TWD_WDOG_RESETSTAT register. This patch adds support to return correct status if WDIOC_GETBOOTSTATUS cmd is called with ioctl.
--- arch/arm/include/asm/smp_twd.h | 2 ++ drivers/watchdog/mpcore_wdt.c | 4 ++++ 2 files changed, 6 insertions(+)
arch/arm/include/asm/smp_twd.h +2 -0
@@ -18,6 +18,8 @@#define TWD_TIMER_CONTROL_PERIODIC (1 << 1)#define TWD_TIMER_CONTROL_IT_ENABLE (1 << 2) +#define TWD_WDOG_RESETSTAT_MASK 0x1 +#include <linux/ioport.h> struct twd_local_timer {
drivers/watchdog/mpcore_wdt.c +4 -0
@@ -151,6 +151,7 @@ static int mpcore_wdt_set_heartbeat(struct watchdog_device *wdd, unsigned int t)static const struct watchdog_info mpcore_wdt_info = { .options =WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING | +WDIOF_CARDRESET |WDIOF_MAGICCLOSE , .identity ="MPcore Watchdog" , };
@@ -216,6 +217,9 @@ static int mpcore_wdt_probe(struct platform_device *pdev)platform_set_drvdata (pdev, wdt);watchdog_set_drvdata (&wdt->wdd, wdt); + wdt->wdd.bootstatus = (readl_relaxed (wdt->base +TWD_WDOG_RESETSTAT ) & +TWD_WDOG_RESETSTAT_MASK ) ?WDIOF_CARDRESET :0 ; +mpcore_wdt_stop (&wdt->wdd); ret =watchdog_register_device (&wdt->wdd);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 11/14 watchdog/mpcore_wdt: Add clock framework support
In reply to Viresh KumarThis patch adds in clk framework support for wdt driver. If clk_get fails for some platform, then we continue without clk_* API's.
--- drivers/watchdog/mpcore_wdt.c | 47 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-)
drivers/watchdog/mpcore_wdt.c +39 -8
@@ -20,6 +20,8 @@#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt +#include <linux/clk.h> +#include <linux/err.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/io.h>
@@ -39,6 +41,7 @@ struct mpcore_wdt {struct device *dev;void __iomem *base;spinlock_t lock; +struct clk *clk;int irq;unsigned int perturb; };
@@ -109,15 +112,23 @@ static int mpcore_wdt_ping(struct watchdog_device *wdd)return 0 ; } -static int mpcore_wdt_stop (struct watchdog_device *wdd) +static void __mpcore_wdt_stop (struct mpcore_wdt *wdt) { -struct mpcore_wdt *wdt =watchdog_get_drvdata (wdd); -spin_lock (&wdt->lock);writel_relaxed (0x12345678 , wdt->base +TWD_WDOG_DISABLE );writel_relaxed (0x87654321 , wdt->base +TWD_WDOG_DISABLE );writel_relaxed (0x0 , wdt->base +TWD_WDOG_CONTROL );spin_unlock (&wdt->lock); +} + +static int mpcore_wdt_stop (struct watchdog_device *wdd) +{ +struct mpcore_wdt *wdt =watchdog_get_drvdata (wdd); + +__mpcore_wdt_stop (wdt); + +if (!IS_ERR (wdt->clk)) +clk_disable_unprepare (wdt->clk);return 0 ; }
@@ -125,9 +136,18 @@ static int mpcore_wdt_stop(struct watchdog_device *wdd)static int mpcore_wdt_start (struct watchdog_device *wdd) {struct mpcore_wdt *wdt =watchdog_get_drvdata (wdd); +int ret;dev_info (wdt->dev,"enabling watchdog\n" ); +if (!IS_ERR (wdt->clk)) { + ret =clk_prepare_enable (wdt->clk); +if (ret) { +dev_err (wdt->dev,"clock prepare-enabled failed" ); +return ret; + } + } +/* This loads the count register but does NOT start the count yet */ mpcore_wdt_ping (wdd);
@@ -173,7 +193,7 @@ static void mpcore_wdt_shutdown(struct platform_device *pdev)struct mpcore_wdt *wdt =platform_get_drvdata (pdev);if (system_state ==SYSTEM_RESTART || system_state ==SYSTEM_HALT ) -mpcore_wdt_stop (&wdt->wdd); +__mpcore_wdt_stop (wdt); }static int mpcore_wdt_probe (struct platform_device *pdev)
@@ -217,6 +237,17 @@ static int mpcore_wdt_probe(struct platform_device *pdev)platform_set_drvdata (pdev, wdt);watchdog_set_drvdata (&wdt->wdd, wdt); + wdt->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (wdt->clk)) { +dev_warn (&pdev->dev,"Clock not found\n" ); + }else { + ret =clk_prepare_enable (wdt->clk); +if (ret) { +dev_err (wdt->dev,"clock prepare-enable failed" ); + goto err_set_drvdata; + } + } + wdt->wdd.bootstatus = (readl_relaxed (wdt->base +TWD_WDOG_RESETSTAT ) &TWD_WDOG_RESETSTAT_MASK ) ?WDIOF_CARDRESET :0 ;
@@ -226,7 +257,7 @@ static int mpcore_wdt_probe(struct platform_device *pdev)if (ret) {dev_err (wdt->dev,"watchdog_register_device() failed: %d\n" , ret); - goto err_register; + goto err_set_drvdata; }/*
@@ -246,7 +277,7 @@ static int mpcore_wdt_probe(struct platform_device *pdev)return 0 ; -err_register: +err_set_drvdata:watchdog_set_drvdata (&wdt->wdd,NULL );return ret;
@@ -267,7 +298,7 @@ static int mpcore_wdt_suspend(struct device *dev)struct mpcore_wdt *wdt =dev_get_drvdata (dev);if (watchdog_active (&wdt->wdd)) -mpcore_wdt_stop (&wdt->wdd);/* Turn the WDT off */ +return mpcore_wdt_stop (&wdt->wdd);/* Turn the WDT off */ return 0 ; }
@@ -277,7 +308,7 @@ static int mpcore_wdt_resume(struct device *dev)struct mpcore_wdt *wdt =dev_get_drvdata (dev);if (watchdog_active (&wdt->wdd)) -mpcore_wdt_start (&wdt->wdd); +return mpcore_wdt_start (&wdt->wdd);return 0 ; }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 12/14 watchdog/mpcore_wdt: use correct clk_rate to program timeout
In reply to Viresh KumarCurrently, mpcore wdt driver doesn't use exact clk_rate to program timeout in hardware. This patch provides two ways of doing so:
- either use clk_get_rate() if clk_get passes
- use clk_rate passed via module param
--- arch/arm/include/asm/smp_twd.h | 10 +++++ drivers/watchdog/mpcore_wdt.c | 99 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 90 insertions(+), 19 deletions(-)
arch/arm/include/asm/smp_twd.h +10 -0
@@ -13,11 +13,21 @@#define TWD_WDOG_RESETSTAT 0x30#define TWD_WDOG_DISABLE 0x34 +#define TWD_WDOG_LOAD_MIN 0x00000000 +#define TWD_WDOG_LOAD_MAX 0xFFFFFFFF +#define TWD_TIMER_CONTROL_ENABLE (1 << 0)#define TWD_TIMER_CONTROL_ONESHOT (0 << 1)#define TWD_TIMER_CONTROL_PERIODIC (1 << 1)#define TWD_TIMER_CONTROL_IT_ENABLE (1 << 2) +#define TWD_WDOG_CONTROL_ENABLE (1 << 0) +#define TWD_WDOG_CONTROL_IRQ_ENABLE (1 << 2) +#define TWD_WDOG_CONTROL_WDT_MODE (1 << 3) +#define TWD_WDOG_CONTROL_WDT_PRESCALE (x) ((x) << 8) +#define TWD_WDOG_CONTROL_PRESCALE_MIN 0x00 +#define TWD_WDOG_CONTROL_PRESCALE_MAX 0xFF +#define TWD_WDOG_RESETSTAT_MASK 0x1#include <linux/ioport.h>
drivers/watchdog/mpcore_wdt.c +80 -19
156 lines not on this page · raw message
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 13/14 watchdog/mpcore_wdt: Start registers from 0x00 instead of 0x20
In reply to Viresh KumarTWD_WDOG is at offset 0x20 from TWD base address. Current register offsets contain this extra 0x20 offset, i.e. users were required to pass base address of TWD instead of WDOG to WDOG driver.
Change this, so that users can pass base address of WDOG to WDOG driver instead of TWD module. For this, subtract 0x20 from offsets of WDOG registers.
This could break any current users of TWD_WDOG, but i couldn't find any users of this driver in current Linux tree. So, haven't fixed any platform code. If some platforms are broken please report to me, so that we can get them fixed in this patch only.
--- arch/arm/include/asm/smp_twd.h | 19 ------------------- drivers/watchdog/mpcore_wdt.c | 24 +++++++++++++++++++++++- 2 files changed, 23 insertions(+), 20 deletions(-)
arch/arm/include/asm/smp_twd.h +0 -19
@@ -6,30 +6,11 @@#define TWD_TIMER_CONTROL 0x08#define TWD_TIMER_INTSTAT 0x0C -#define TWD_WDOG_LOAD 0x20 -#define TWD_WDOG_COUNTER 0x24 -#define TWD_WDOG_CONTROL 0x28 -#define TWD_WDOG_INTSTAT 0x2C -#define TWD_WDOG_RESETSTAT 0x30 -#define TWD_WDOG_DISABLE 0x34 - -#define TWD_WDOG_LOAD_MIN 0x00000000 -#define TWD_WDOG_LOAD_MAX 0xFFFFFFFF -#define TWD_TIMER_CONTROL_ENABLE (1 << 0)#define TWD_TIMER_CONTROL_ONESHOT (0 << 1)#define TWD_TIMER_CONTROL_PERIODIC (1 << 1)#define TWD_TIMER_CONTROL_IT_ENABLE (1 << 2) -#define TWD_WDOG_CONTROL_ENABLE (1 << 0) -#define TWD_WDOG_CONTROL_IRQ_ENABLE (1 << 2) -#define TWD_WDOG_CONTROL_WDT_MODE (1 << 3) -#define TWD_WDOG_CONTROL_WDT_PRESCALE (x) ((x) << 8) -#define TWD_WDOG_CONTROL_PRESCALE_MIN 0x00 -#define TWD_WDOG_CONTROL_PRESCALE_MAX 0xFF - -#define TWD_WDOG_RESETSTAT_MASK 0x1 -#include <linux/ioport.h> struct twd_local_timer {
drivers/watchdog/mpcore_wdt.c +23 -1
@@ -34,7 +34,29 @@#include <linux/types.h> #include <linux/watchdog.h> -#include <asm/smp_twd.h> +/* +* TWD_WDOG is at offset 0x20 from TWD base address. Following register offsets +* doesn't contain this extra 0x20 offset, i.e. users of TWD_WDOG must pass base +* address of WDOG to WDOG driver instead of TWD module. +*/ +#define TWD_WDOG_LOAD 0x00 +#define TWD_WDOG_COUNTER 0x04 +#define TWD_WDOG_CONTROL 0x08 +#define TWD_WDOG_INTSTAT 0x0C +#define TWD_WDOG_RESETSTAT 0x10 +#define TWD_WDOG_DISABLE 0x14 + +#define TWD_WDOG_LOAD_MIN 0x00000000 +#define TWD_WDOG_LOAD_MAX 0xFFFFFFFF + +#define TWD_WDOG_CONTROL_ENABLE (1 << 0) +#define TWD_WDOG_CONTROL_IRQ_ENABLE (1 << 2) +#define TWD_WDOG_CONTROL_WDT_MODE (1 << 3) +#define TWD_WDOG_CONTROL_WDT_PRESCALE (x) ((x) << 8) +#define TWD_WDOG_CONTROL_PRESCALE_MIN 0x00 +#define TWD_WDOG_CONTROL_PRESCALE_MAX 0xFF + +#define TWD_WDOG_RESETSTAT_MASK 0x1struct mpcore_wdt {struct watchdog_device wdd;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH v3 14/14 watchdog/mpcore_wdt: Add DT probing support for ARM mpcore watchdog
In reply to Viresh KumarThis patch adds Device tree probing support for ARM Mpcore watchdog. Its binding were already documented in Documentation/devicetree/bindings/arm/twd.txt.
--- drivers/watchdog/mpcore_wdt.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
drivers/watchdog/mpcore_wdt.c +12 -0
@@ -27,6 +27,7 @@#include <linux/io.h> #include <linux/module.h> #include <linux/moduleparam.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/reboot.h>
@@ -403,6 +404,16 @@ static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,/* work with hotplug and coldplug */ MODULE_ALIAS ("platform:mpcore_wdt" ); +#ifdef CONFIG_OF +static const struct of_device_id mpcore_wdt_id_table[] = { + { .compatible ="arm,cortex-a9-twd-wdt" }, + { .compatible ="arm,cortex-a5-twd-wdt" }, + { .compatible ="arm,arm11mp-twd-wdt" }, + {} +}; +MODULE_DEVICE_TABLE (of, mpcore_wdt_id_table); +#endif +static struct platform_driver mpcore_wdt_driver = { .probe = mpcore_wdt_probe, .remove = mpcore_wdt_remove,
@@ -411,6 +422,7 @@ static struct platform_driver mpcore_wdt_driver = { .owner = THIS_MODULE, .name = "mpcore_wdt", .pm = &mpcore_wdt_dev_pm_ops, + .of_match_table = of_match_ptr(mpcore_wdt_id_table), }, };
Signature · 1 line
1.7.12.rc2.18.g61b472e
v3 cover watchdog: ARM mpcore Improvements
In reply to Viresh KumarQuoted 2 lines · Viresh Kumar
ARM mpcore watchdog isn't workable and so is marked broken in the first patch of the series. Reasons are mentioned in 01/14 and important links are shared too.
The only way this is usable is if a watchdog user binds to a particular CPU before opening this device, and remains bound to that user. The irq-requesting thing is something that has changed since the driver was originally added.
I suspect there are no users of this driver, and it should simply be deleted. It doesn't really fit our watchdog API. If anyone wants to resurect it, it's in git, your patches are in the list archives...