[PATCH 0/4] ARM: smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[PATCH 0/4] ARM: smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[vkuzmichev]

From: Vitaly Kuzmichev <vkuzmichev@mvista.com>

The series of patches fixes various bugs in ARM MPCORE watchdog setup.
They also introduce some changes in common SMP_TWD timer setup code.
The PATCH 1/4 adds an exported function to obtain calibrated timer rate
in mpcore_wdt. However, this solution might be unacceptable, so we may
discuss another 2 variants:
 1) use clock framework
   This may require to fix all MPCORE boards clock implementations.
 2) copy calibration loop to mpcore_wdt
   However, this increases boot time.


Vitaly Kuzmichev (4):
  ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading
  ARM: smp_twd: Fix typo in twd_timer_rate printout
  ARM: mpcore_wdt: Fix WDIOC_SETOPTIONS handling
  ARM: mpcore_wdt: Fix timer mode setup

 arch/arm/include/asm/smp_twd.h |    7 +++++++
 arch/arm/kernel/smp_twd.c      |    8 +++++++-
 drivers/watchdog/mpcore_wdt.c  |   29 +++++++++++++++++++++--------
 3 files changed, 35 insertions(+), 9 deletions(-)

-- 
1.7.2.2

[PATCH 1/4] ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading

[vkuzmichev]

From: Vitaly Kuzmichev <vkuzmichev@mvista.com>

Although upstream commit "98af057 ARM: 6126/1: ARM mpcore_wdt: fix
build failure and other fixes" resolved long standing mpcore_wdt
driver build problems, it introduced an error in the relationship
between the MPcore watchdog timer clock rate and mpcore_margin,
"MPcore timer margin in seconds", such that watchdog timeouts are now
arbitrary rather than the number of seconds specified by mpcore_margin.
This change restores mpcore_wdt_keepalive() to its equivalent
implementation prior to commit 98af057 such that watchdog timeouts now
occur as specified by mpcore_margin.
The variable 'mpcore_timer_rate' which caused that build failure was
replaced by 'twd_timer_rate'. Adding exported function to obtain
twd_timer_rate value in mpcore_wdt driver.
A build error will not occur because MPCORE_WATCHDOG needed to build
'mpcore_wdt' depends on HAVE_ARM_TWD needed to build 'smp_twd'.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 arch/arm/include/asm/smp_twd.h |    1 +
 arch/arm/kernel/smp_twd.c      |    6 ++++++
 drivers/watchdog/mpcore_wdt.c  |   11 ++++++++---
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index 634f357..c6328d8 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -25,5 +25,6 @@ extern void __iomem *twd_base;
 void twd_timer_stop(void);
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+unsigned long twd_get_timer_rate(void);
 
 #endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 35882fb..7bc1173 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -160,3 +160,9 @@ void twd_timer_stop(void)
 	__raw_writel(0, twd_base + TWD_TIMER_CONTROL);
 }
 #endif
+
+unsigned long twd_get_timer_rate(void)
+{
+	return twd_timer_rate;
+}
+EXPORT_SYMBOL(twd_get_timer_rate);
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index b8ec7ac..1bd3529 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -47,6 +47,11 @@ struct mpcore_wdt {
 static struct platform_device *mpcore_wdt_dev;
 static DEFINE_SPINLOCK(wdt_lock);
 
+static unsigned long mpcore_wdt_rate;
+module_param_named(rate, mpcore_wdt_rate, ulong, 0);
+MODULE_PARM_DESC(rate,
+	"MPcore watchdog timer rate in Hz. (default=0 - autodetect)");
+
 #define TIMER_MARGIN	60
 static int mpcore_margin = TIMER_MARGIN;
 module_param(mpcore_margin, int, 0);
@@ -99,9 +104,7 @@ static void mpcore_wdt_keepalive(struct mpcore_wdt *wdt)
 
 	spin_lock(&wdt_lock);
 	/* Assume prescale is set to 256 */
-	count =  __raw_readl(wdt->base + TWD_WDOG_COUNTER);
-	count = (0xFFFFFFFFU - count) * (HZ / 5);
-	count = (count / 256) * mpcore_margin;
+	count = (mpcore_wdt_rate / 256) * mpcore_margin;
 
 	/* Reload the counter */
 	writel(count + wdt->perturb, wdt->base + TWD_WDOG_LOAD);
@@ -434,6 +437,8 @@ static int __init mpcore_wdt_init(void)
 		printk(KERN_INFO "mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n",
 			TIMER_MARGIN);
 	}
+	if (!mpcore_wdt_rate)
+		mpcore_wdt_rate = twd_get_timer_rate();
 
 	printk(banner, mpcore_noboot, mpcore_margin, nowayout);
 
-- 
1.7.2.2

[PATCH 2/4] ARM: smp_twd: Fix typo in twd_timer_rate printout

[vkuzmichev]

From: Vitaly Kuzmichev <vkuzmichev@mvista.com>

To get hundredths of MHz the rate needs to be divided by 10'000.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 arch/arm/kernel/smp_twd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 7bc1173..931b0e3 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -114,7 +114,7 @@ static void __cpuinit twd_calibrate_rate(void)
 		twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
 
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
-			(twd_timer_rate / 100000) % 100);
+			(twd_timer_rate / 10000) % 100);
 	}
 
 	load = twd_timer_rate / HZ;
-- 
1.7.2.2

[PATCH 3/4] ARM: mpcore_wdt: Fix WDIOC_SETOPTIONS handling

[vkuzmichev]

From: Vitaly Kuzmichev <vkuzmichev@mvista.com>

According to the include/linux/watchdog.h WDIOC_SETOPTIONS is
classified as 'read from device' ioctl call:
  #define WDIOC_SETOPTIONS        _IOR(WATCHDOG_IOCTL_BASE, 4, int)

However, the driver 'mpcore_wdt' performs 'copy_from_user' only if
_IOC_WRITE is set, thus the local variable 'uarg' which is used in
WDIOC_SETOPTIONS handling remains uninitialized.

The proper way to fix this is to bind WDIOC_SETOPTIONS to _IOW,
but this will break compatibility.
So adding additional condition for performing 'copy_from_user'.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/watchdog/mpcore_wdt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 1bd3529..6d3ac06 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -238,7 +238,8 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 	if (_IOC_DIR(cmd) && _IOC_SIZE(cmd) > sizeof(uarg))
 		return -ENOTTY;
 
-	if (_IOC_DIR(cmd) & _IOC_WRITE) {
+	if (_IOC_DIR(cmd) & _IOC_WRITE
+			|| cmd == WDIOC_SETOPTIONS) {
 		ret = copy_from_user(&uarg, (void __user *)arg, _IOC_SIZE(cmd));
 		if (ret)
 			return -EFAULT;
-- 
1.7.2.2

[PATCH 4/4] ARM: mpcore_wdt: Fix timer mode setup

[vkuzmichev]

From: Vitaly Kuzmichev <vkuzmichev@mvista.com>

Allow watchdog to set its iterrupt as pending when it is configured
for timer mode.
Also add macros for all Watchdog Control Register flags.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 arch/arm/include/asm/smp_twd.h |    6 ++++++
 drivers/watchdog/mpcore_wdt.c  |   15 +++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index c6328d8..324bf0a 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -18,6 +18,12 @@
 #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_PERIODIC	(1 << 1)
+#define TWD_WDOG_CONTROL_IT_ENABLE	(1 << 2)
+#define TWD_WDOG_CONTROL_TIMER_MODE	(0 << 3)
+#define TWD_WDOG_CONTROL_WATCHDOG_MODE	(1 << 3)
+
 struct clock_event_device;
 
 extern void __iomem *twd_base;
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 6d3ac06..07d5a1c 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -123,18 +123,25 @@ static void mpcore_wdt_stop(struct mpcore_wdt *wdt)
 
 static void mpcore_wdt_start(struct mpcore_wdt *wdt)
 {
+	u32 mode;
+
 	dev_printk(KERN_INFO, wdt->dev, "enabling watchdog.\n");
 
 	/* This loads the count register but does NOT start the count yet */
 	mpcore_wdt_keepalive(wdt);
 
+	/* Setup watchdog - prescale=256, enable=1 */
+	mode = (255 << 8) | TWD_WDOG_CONTROL_ENABLE;
+
 	if (mpcore_noboot) {
-		/* Enable watchdog - prescale=256, watchdog mode=0, enable=1 */
-		writel(0x0000FF01, wdt->base + TWD_WDOG_CONTROL);
+		/* timer mode, send interrupt */
+		mode |=	TWD_WDOG_CONTROL_TIMER_MODE
+		     |  TWD_WDOG_CONTROL_IT_ENABLE;
 	} else {
-		/* Enable watchdog - prescale=256, watchdog mode=1, enable=1 */
-		writel(0x0000FF09, wdt->base + TWD_WDOG_CONTROL);
+		/* watchdog mode */
+		mode |=	TWD_WDOG_CONTROL_WATCHDOG_MODE;
 	}
+	writel(mode, wdt->base + TWD_WDOG_CONTROL);
 }
 
 static int mpcore_wdt_set_heartbeat(int t)
-- 
1.7.2.2

Re: [PATCH 0/4] ARM: smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Wim Van Sebroeck]

Hi Vitaly,

> From: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> 
> The series of patches fixes various bugs in ARM MPCORE watchdog setup.
> They also introduce some changes in common SMP_TWD timer setup code.
> The PATCH 1/4 adds an exported function to obtain calibrated timer rate
> in mpcore_wdt. However, this solution might be unacceptable, so we may
> discuss another 2 variants:
>  1) use clock framework
>    This may require to fix all MPCORE boards clock implementations.
>  2) copy calibration loop to mpcore_wdt
>    However, this increases boot time.
> 
> 
> Vitaly Kuzmichev (4):
>   ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading
>   ARM: smp_twd: Fix typo in twd_timer_rate printout
>   ARM: mpcore_wdt: Fix WDIOC_SETOPTIONS handling
>   ARM: mpcore_wdt: Fix timer mode setup
> 
>  arch/arm/include/asm/smp_twd.h |    7 +++++++
>  arch/arm/kernel/smp_twd.c      |    8 +++++++-
>  drivers/watchdog/mpcore_wdt.c  |   29 +++++++++++++++++++++--------
>  3 files changed, 35 insertions(+), 9 deletions(-)

What's the status on this? I don't recall having seen any answers from ARM community.

Kind regards,
Wim.

Re: [PATCH 0/4] ARM: smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Marc Zyngier]

On Fri, 2011-05-27 at 10:26 +0200, Wim Van Sebroeck wrote:
> Hi Vitaly,
> 
> > From: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> > 
> > The series of patches fixes various bugs in ARM MPCORE watchdog setup.
> > They also introduce some changes in common SMP_TWD timer setup code.
> > The PATCH 1/4 adds an exported function to obtain calibrated timer rate
> > in mpcore_wdt. However, this solution might be unacceptable, so we may
> > discuss another 2 variants:
> >  1) use clock framework
> >    This may require to fix all MPCORE boards clock implementations.
> >  2) copy calibration loop to mpcore_wdt
> >    However, this increases boot time.
> > 
> > 
> > Vitaly Kuzmichev (4):
> >   ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading
> >   ARM: smp_twd: Fix typo in twd_timer_rate printout
> >   ARM: mpcore_wdt: Fix WDIOC_SETOPTIONS handling
> >   ARM: mpcore_wdt: Fix timer mode setup
> > 
> >  arch/arm/include/asm/smp_twd.h |    7 +++++++
> >  arch/arm/kernel/smp_twd.c      |    8 +++++++-
> >  drivers/watchdog/mpcore_wdt.c  |   29 +++++++++++++++++++++--------
> >  3 files changed, 35 insertions(+), 9 deletions(-)
> 
> What's the status on this? I don't recall having seen any answers from ARM community.

Overall, I'm not too happy about patch #1. Exposing some "random"
internal state of the timer doesn't seem the right thing to me.

Furthermore, the internal rate of the TWD changes with the CPU
frequency. Blindly trusting the calibrated value at boot time is a nice
way to see the watchdog kicking in unexpectedly when frequency is
increased.

I suggest you have a look at this patch:
https://patchwork.kernel.org/patch/801472/

Together with a cpufreq notifier, this "smp_twd" clock looks like a much
better option. Over time, all platform would be converted to that
particular clock, and the problem would be solved in a much nicer way.

Cheers,

	M.
-- 
Reality is an implementation detail.


--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Vitaly Kuzmichev]

The series of patches fixes various bugs in ARM MPCORE watchdog setup.
They also introduce some changes in common ARM_SMP_TWD timer setup code.
The PATCH 2/6 adds an exported function to obtain twd timer rate in
mpcore_wdt. However, this solution might be unacceptable, and the
better is to copy calibration loop and clk_get_rate calling to mpcore_wdt.

The series of patches is based on arm-platforms.git/local_timers_as_devices
branch since it looks like that it is going to be merged into the mainline.

V1 series was not discussed widely because I forgot to add all necessary
maintainers in Cc.

Vitaly Kuzmichev (6):
  arm_smp_twd: Fix typo in 'twd_timer_rate' printing
  arm_smp_twd: mpcore_wdt: Fix watchdog counter loading
  mpcore_wdt: Fix WDIOC_SETOPTIONS handling
  mpcore_wdt: Fix timer mode setup
  mpcore_wdt: Add cpufreq notifier to reload counter
  mpcore_wdt: Move declarations in a separate header

 drivers/clocksource/arm_smp_twd.c |   15 +++++++-
 drivers/watchdog/mpcore_wdt.c     |   71 +++++++++++++++++++++++-------------
 drivers/watchdog/mpcore_wdt.h     |   40 +++++++++++++++++++++
 3 files changed, 99 insertions(+), 27 deletions(-)
 create mode 100644 drivers/watchdog/mpcore_wdt.h

-- 
1.7.3.4

[PATCH V2 1/6] arm_smp_twd: Fix typo in 'twd_timer_rate' printing

[Vitaly Kuzmichev]

To get hundredths of MHz the rate needs to be divided by 10'000.
Here is an example:
 twd_timer_rate = 123456789
 Before:
    twd_timer_rate / 1000000 = 123
    (twd_timer_rate / 1000000) % 100 = 23
    Result: 123.23MHz.
 Now:
    twd_timer_rate / 1000000 = 123
    (twd_timer_rate / 10000) % 100 = 45
    Result: 123.45MHz.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/clocksource/arm_smp_twd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
index 60c8022..6acbdac 100644
--- a/drivers/clocksource/arm_smp_twd.c
+++ b/drivers/clocksource/arm_smp_twd.c
@@ -176,7 +176,7 @@ static void __cpuinit twd_calibrate_rate(void)
 		twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
 
 		printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
-			(twd_timer_rate / 1000000) % 100);
+			(twd_timer_rate / 10000) % 100);
 	}
 }
 
-- 
1.7.3.4

[PATCH V2 2/6] arm_smp_twd: mpcore_wdt: Fix watchdog counter loading

[Vitaly Kuzmichev]

Although the commit "98af057092f8f0dabe63c5df08adc2bbfbddb1d2
 ARM: 6126/1: ARM mpcore_wdt: fix build failure and other fixes"
resolved long standing mpcore_wdt driver build problems, it
introduced an error in the relationship between the MPcore watchdog
timer clock rate and mpcore_margin, "MPcore timer margin in seconds",
such that watchdog timeouts are now arbitrary rather than the number
of seconds specified by mpcore_margin.

This change restores mpcore_wdt_keepalive() to its equivalent
implementation prior to commit 98af057 such that watchdog timeouts now
occur as specified by mpcore_margin.

The variable 'mpcore_timer_rate' which caused that build failure was
replaced by 'twd_timer_rate'. Adding exported function to obtain
'twd_timer_rate' value in mpcore_wdt driver.

A build error will not occur because MPCORE_WATCHDOG needed to build
'mpcore_wdt' depends on HAVE_ARM_TWD needed to build 'smp_twd'.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/clocksource/arm_smp_twd.c |   13 +++++++++++++
 drivers/watchdog/mpcore_wdt.c     |    6 +++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
index 6acbdac..32f78ff 100644
--- a/drivers/clocksource/arm_smp_twd.c
+++ b/drivers/clocksource/arm_smp_twd.c
@@ -241,6 +241,19 @@ static void __cpuinit twd_setup(void *data)
 					0xf, 0xffffffff);
 }
 
+/* Needed by mpcore_wdt */
+unsigned long twd_timer_get_rate(void)
+{
+	unsigned long rate;
+
+	if (!IS_ERR_OR_NULL(twd_clk))
+		rate = clk_get_rate(twd_clk);
+	else
+		rate = twd_timer_rate;
+	return rate;
+}
+EXPORT_SYMBOL(twd_timer_get_rate);
+
 static void __cpuinit twd_teardown(void *data)
 {
 	struct clock_event_device *clk = data;
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index f638206..7a3d6b2 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -33,6 +33,8 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 
+unsigned long twd_timer_get_rate(void);
+
 #define TWD_WDOG_LOAD			0x20
 #define TWD_WDOG_COUNTER		0x24
 #define TWD_WDOG_CONTROL		0x28
@@ -104,9 +106,7 @@ static void mpcore_wdt_keepalive(struct mpcore_wdt *wdt)
 
 	spin_lock(&wdt_lock);
 	/* Assume prescale is set to 256 */
-	count =  __raw_readl(wdt->base + TWD_WDOG_COUNTER);
-	count = (0xFFFFFFFFU - count) * (HZ / 5);
-	count = (count / 256) * mpcore_margin;
+	count = (twd_timer_get_rate() / 256) * mpcore_margin;
 
 	/* Reload the counter */
 	writel(count + wdt->perturb, wdt->base + TWD_WDOG_LOAD);
-- 
1.7.3.4

[PATCH V2 3/6] mpcore_wdt: Fix WDIOC_SETOPTIONS handling

[Vitaly Kuzmichev]

According to the include/linux/watchdog.h WDIOC_SETOPTIONS is
classified as 'read from device' ioctl call:
  #define WDIOC_SETOPTIONS        _IOR(WATCHDOG_IOCTL_BASE, 4, int)

However, the driver 'mpcore_wdt' performs 'copy_from_user' only if
_IOC_WRITE is set, thus the local variable 'uarg' which is used in
WDIOC_SETOPTIONS handling remains uninitialized.

The proper way to fix this is to bind WDIOC_SETOPTIONS to _IOW,
but this will break compatibility.
So adding additional condition for performing 'copy_from_user'.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/watchdog/mpcore_wdt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 7a3d6b2..b330a0a 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -240,7 +240,8 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 	if (_IOC_DIR(cmd) && _IOC_SIZE(cmd) > sizeof(uarg))
 		return -ENOTTY;
 
-	if (_IOC_DIR(cmd) & _IOC_WRITE) {
+	if (_IOC_DIR(cmd) & _IOC_WRITE
+			|| cmd == WDIOC_SETOPTIONS) {
 		ret = copy_from_user(&uarg, (void __user *)arg, _IOC_SIZE(cmd));
 		if (ret)
 			return -EFAULT;
-- 
1.7.3.4

[PATCH V2 4/6] mpcore_wdt: Fix timer mode setup

[Vitaly Kuzmichev]

Allow watchdog to set its iterrupt as pending when it is configured
for timer mode (in other words, allow emitting interrupt).
Also add macros for all Watchdog Control Register flags.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/watchdog/mpcore_wdt.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index b330a0a..11c70df 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -42,6 +42,12 @@ unsigned long twd_timer_get_rate(void);
 #define TWD_WDOG_RESETSTAT		0x30
 #define TWD_WDOG_DISABLE		0x34
 
+#define TWD_WDOG_CONTROL_ENABLE		(1 << 0)
+#define TWD_WDOG_CONTROL_PERIODIC	(1 << 1)
+#define TWD_WDOG_CONTROL_IT_ENABLE	(1 << 2)
+#define TWD_WDOG_CONTROL_TIMER_MODE	(0 << 3)
+#define TWD_WDOG_CONTROL_WATCHDOG_MODE	(1 << 3)
+
 struct mpcore_wdt {
 	unsigned long	timer_alive;
 	struct device	*dev;
@@ -125,18 +131,25 @@ static void mpcore_wdt_stop(struct mpcore_wdt *wdt)
 
 static void mpcore_wdt_start(struct mpcore_wdt *wdt)
 {
+	u32 mode;
+
 	dev_printk(KERN_INFO, wdt->dev, "enabling watchdog.\n");
 
 	/* This loads the count register but does NOT start the count yet */
 	mpcore_wdt_keepalive(wdt);
 
+	/* Setup watchdog - prescale=256, enable=1 */
+	mode = (255 << 8) | TWD_WDOG_CONTROL_ENABLE;
+
 	if (mpcore_noboot) {
-		/* Enable watchdog - prescale=256, watchdog mode=0, enable=1 */
-		writel(0x0000FF01, wdt->base + TWD_WDOG_CONTROL);
+		/* timer mode, send interrupt */
+		mode |=	TWD_WDOG_CONTROL_TIMER_MODE
+		     |  TWD_WDOG_CONTROL_IT_ENABLE;
 	} else {
-		/* Enable watchdog - prescale=256, watchdog mode=1, enable=1 */
-		writel(0x0000FF09, wdt->base + TWD_WDOG_CONTROL);
+		/* watchdog mode */
+		mode |=	TWD_WDOG_CONTROL_WATCHDOG_MODE;
 	}
+	writel(mode, wdt->base + TWD_WDOG_CONTROL);
 }
 
 static int mpcore_wdt_set_heartbeat(int t)
-- 
1.7.3.4

[PATCH V2 5/6] mpcore_wdt: Add cpufreq notifier to reload counter

[Vitaly Kuzmichev]

In the case if CPU frequency is changed when watchdog is working it begins to
acount faster or slower. To avoid watchdog triggering before the margin time
the watchdog counter needs to be reloaded.

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/watchdog/mpcore_wdt.c |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 11c70df..38b9119 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -32,6 +32,7 @@
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/cpufreq.h>
 
 unsigned long twd_timer_get_rate(void);
 
@@ -426,6 +427,25 @@ static int __devexit mpcore_wdt_remove(struct platform_device *dev)
 	return 0;
 }
 
+static int mpcore_wdt_notify(struct notifier_block *nb,
+				unsigned long state, void *data)
+{
+	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_dev);
+	/*
+	 * The mpcore_wdt counter register must be reloaded to account
+	 * properly with the new frequency.
+	 */
+	if (mpcore_wdt_dev && wdt && (state == CPUFREQ_POSTCHANGE ||
+					state == CPUFREQ_RESUMECHANGE))
+		mpcore_wdt_keepalive(wdt);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mpcore_wdt_nb = {
+	.notifier_call = mpcore_wdt_notify,
+};
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:mpcore_wdt");
 
@@ -444,6 +464,7 @@ static char banner[] __initdata = KERN_INFO "MPcore Watchdog Timer: 0.1. "
 
 static int __init mpcore_wdt_init(void)
 {
+	int ret = 0;
 	/*
 	 * Check that the margin value is within it's range;
 	 * if not reset to the default
@@ -456,11 +477,19 @@ static int __init mpcore_wdt_init(void)
 
 	printk(banner, mpcore_noboot, mpcore_margin, nowayout);
 
-	return platform_driver_register(&mpcore_wdt_driver);
+	ret = platform_driver_register(&mpcore_wdt_driver);
+
+	if (!ret)
+		ret = cpufreq_register_notifier(&mpcore_wdt_nb,
+						CPUFREQ_TRANSITION_NOTIFIER);
+
+	return ret;
 }
 
 static void __exit mpcore_wdt_exit(void)
 {
+	cpufreq_unregister_notifier(&mpcore_wdt_nb,
+					CPUFREQ_TRANSITION_NOTIFIER);
 	platform_driver_unregister(&mpcore_wdt_driver);
 }
 
-- 
1.7.3.4

[PATCH V2 6/6] mpcore_wdt: Move declarations in a separate header

[Vitaly Kuzmichev]

Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---
 drivers/watchdog/mpcore_wdt.c |   26 +-------------------------
 drivers/watchdog/mpcore_wdt.h |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 25 deletions(-)
 create mode 100644 drivers/watchdog/mpcore_wdt.h

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 38b9119..1736522 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -34,34 +34,11 @@
 #include <linux/io.h>
 #include <linux/cpufreq.h>
 
-unsigned long twd_timer_get_rate(void);
-
-#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_CONTROL_ENABLE		(1 << 0)
-#define TWD_WDOG_CONTROL_PERIODIC	(1 << 1)
-#define TWD_WDOG_CONTROL_IT_ENABLE	(1 << 2)
-#define TWD_WDOG_CONTROL_TIMER_MODE	(0 << 3)
-#define TWD_WDOG_CONTROL_WATCHDOG_MODE	(1 << 3)
-
-struct mpcore_wdt {
-	unsigned long	timer_alive;
-	struct device	*dev;
-	void __iomem	*base;
-	int		irq;
-	unsigned int	perturb;
-	char		expect_close;
-};
+#include "mpcore_wdt.h"
 
 static struct platform_device *mpcore_wdt_dev;
 static DEFINE_SPINLOCK(wdt_lock);
 
-#define TIMER_MARGIN	60
 static int mpcore_margin = TIMER_MARGIN;
 module_param(mpcore_margin, int, 0);
 MODULE_PARM_DESC(mpcore_margin,
@@ -74,7 +51,6 @@ MODULE_PARM_DESC(nowayout,
 	"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-#define ONLY_TESTING	0
 static int mpcore_noboot = ONLY_TESTING;
 module_param(mpcore_noboot, int, 0);
 MODULE_PARM_DESC(mpcore_noboot, "MPcore watchdog action, "
diff --git a/drivers/watchdog/mpcore_wdt.h b/drivers/watchdog/mpcore_wdt.h
new file mode 100644
index 0000000..694e879
--- /dev/null
+++ b/drivers/watchdog/mpcore_wdt.h
@@ -0,0 +1,40 @@
+/*
+ *	Header file for the mpcore watchdog driver
+ *
+ *      2011 (c) MontaVista Software, LLC. This file is licensed under
+ *      the terms of the GNU General Public License version 2. This program
+ *      is licensed "as is" without any warranty of any kind, whether express
+ *      or implied.
+ */
+
+#ifndef __MPCORE_WATCHDOG_H
+#define __MPCORE_WATCHDOG_H
+
+#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_CONTROL_ENABLE		(1 << 0)
+#define TWD_WDOG_CONTROL_PERIODIC	(1 << 1)
+#define TWD_WDOG_CONTROL_IT_ENABLE	(1 << 2)
+#define TWD_WDOG_CONTROL_TIMER_MODE	(0 << 3)
+#define TWD_WDOG_CONTROL_WATCHDOG_MODE	(1 << 3)
+
+#define TIMER_MARGIN	60
+#define ONLY_TESTING	0
+
+struct mpcore_wdt {
+	unsigned long	timer_alive;
+	struct device	*dev;
+	void __iomem	*base;
+	int		irq;
+	unsigned int	perturb;
+	char		expect_close;
+};
+
+unsigned long twd_timer_get_rate(void);
+
+#endif /* __MPCORE_WATCHDOG_H */
-- 
1.7.3.4

Re: [PATCH V2 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 12:16:03PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 12:05:48PM +0100, Catalin Marinas wrote:
> > On Wed, Jul 06, 2011 at 11:14:56AM +0100, Russell King - ARM Linux wrote:
> > > Do we know why the calibration was initially introduced?  FWIR, it came
> > > from the SMP group in ARM, so I guess they had a reason for it rather
> > > than copying x86.
> > 
> > I think it was introduced because the TWD frequency is half of the CPU
> > frequency but the latter may not be known - boot monitor configuration
> > could change it.
> 
> Okay, that implies we can't have a fixed frequency built into the kernel
> then.  For such platforms like the ARM dev boards (realview + vexpress),
> I expect we can read the CPU frequency from somewhere like one of the
> ICST PLLs.

So how do we do that?  I see this as a blocker on these patches.

Re: [PATCH V2 6/6] mpcore_wdt: Move declarations in a separate header

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 02:48:48PM +0200, Arnd Bergmann wrote:
> An obvious choice would be arch/arm/include/asm/smp_twd.h.

Not if its moving out of arch/arm, but that's a separate debate to be
had.

Re: [PATCH V2 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Catalin Marinas]

On Wed, Jul 06, 2011 at 12:16:03PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 12:05:48PM +0100, Catalin Marinas wrote:
> > On Wed, Jul 06, 2011 at 11:14:56AM +0100, Russell King - ARM Linux wrote:
> > > Do we know why the calibration was initially introduced?  FWIR, it came
> > > from the SMP group in ARM, so I guess they had a reason for it rather
> > > than copying x86.
> > 
> > I think it was introduced because the TWD frequency is half of the CPU
> > frequency but the latter may not be known - boot monitor configuration
> > could change it.
> 
> Okay, that implies we can't have a fixed frequency built into the kernel
> then.  For such platforms like the ARM dev boards (realview + vexpress),
> I expect we can read the CPU frequency from somewhere like one of the
> ICST PLLs.
 
That's highly platform dependent and it's not just ARM dev boards. Any
board with ARM11MPCore, Cortex-A9 or Cortex-A5 would need to find a way
to retrieve such information. If anyone is prepared to go through each
board and figure out how to get the information, it's fine by me.

-- 
Catalin

Re: [PATCH V2 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Russell King - ARM Linux]

On Mon, Jul 11, 2011 at 10:43:43AM +0100, Catalin Marinas wrote:
> On Wed, Jul 06, 2011 at 12:16:03PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 06, 2011 at 12:05:48PM +0100, Catalin Marinas wrote:
> > > On Wed, Jul 06, 2011 at 11:14:56AM +0100, Russell King - ARM Linux wrote:
> > > > Do we know why the calibration was initially introduced?  FWIR, it came
> > > > from the SMP group in ARM, so I guess they had a reason for it rather
> > > > than copying x86.
> > > 
> > > I think it was introduced because the TWD frequency is half of the CPU
> > > frequency but the latter may not be known - boot monitor configuration
> > > could change it.
> > 
> > Okay, that implies we can't have a fixed frequency built into the kernel
> > then.  For such platforms like the ARM dev boards (realview + vexpress),
> > I expect we can read the CPU frequency from somewhere like one of the
> > ICST PLLs.
>  
> That's highly platform dependent and it's not just ARM dev boards. Any
> board with ARM11MPCore, Cortex-A9 or Cortex-A5 would need to find a way
> to retrieve such information. If anyone is prepared to go through each
> board and figure out how to get the information, it's fine by me.

So, these patches are blocked until we find some way to resolve the
clocking issues on all platforms with TWD and the MPcore watchdog.

Re: [PATCH V2 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Catalin Marinas]

On Mon, Jul 11, 2011 at 10:54:24AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 10:43:43AM +0100, Catalin Marinas wrote:
> > On Wed, Jul 06, 2011 at 12:16:03PM +0100, Russell King - ARM Linux wrote:
> > > On Wed, Jul 06, 2011 at 12:05:48PM +0100, Catalin Marinas wrote:
> > > > On Wed, Jul 06, 2011 at 11:14:56AM +0100, Russell King - ARM Linux wrote:
> > > > > Do we know why the calibration was initially introduced?  FWIR, it came
> > > > > from the SMP group in ARM, so I guess they had a reason for it rather
> > > > > than copying x86.
> > > > 
> > > > I think it was introduced because the TWD frequency is half of the CPU
> > > > frequency but the latter may not be known - boot monitor configuration
> > > > could change it.
> > > 
> > > Okay, that implies we can't have a fixed frequency built into the kernel
> > > then.  For such platforms like the ARM dev boards (realview + vexpress),
> > > I expect we can read the CPU frequency from somewhere like one of the
> > > ICST PLLs.
> >  
> > That's highly platform dependent and it's not just ARM dev boards. Any
> > board with ARM11MPCore, Cortex-A9 or Cortex-A5 would need to find a way
> > to retrieve such information. If anyone is prepared to go through each
> > board and figure out how to get the information, it's fine by me.
> 
> So, these patches are blocked until we find some way to resolve the
> clocking issues on all platforms with TWD and the MPcore watchdog.
 
A way would be to mark TWD as broken on those platforms (you can still
use the global timer, with its drawbacks) and push the platform
maintainers to change their code. Once that's sorted and we know for
sure that no platform requires the TWD calibration, you can merge these
patches.

-- 
Catalin