[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 at mvista.com]

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 at mvista.com]

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 at mvista.com]

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 at mvista.com]

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 at mvista.com]

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

[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.

[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.

[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 0/6] arm_smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Marc Zyngier]

Hi Vitaly,

On 05/07/11 20:00, Vitaly Kuzmichev wrote:
> 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.

My take on this is that the calibration loop should simply be killed
once Colin's patch using the "smp_twd" clock is merged and relevant
platforms updated. Once this is done, the watchdog driver should use the
clock directly, without exporting a symbol.

> 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.

Be careful here. This branch is a work in progress, likely to change
very quickly, contains code that has not been posted to the ML yet, and
will probably eat your pet for breakfast. As far as mainline merging is
concerned, there is still a long way to go (see the GIC consolidation
patches, on which the local_timers_as_devices branch relies).

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

Thanks for that.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Russell King - ARM Linux]

On Wed, Jul 06, 2011 at 11:05:51AM +0100, Marc Zyngier wrote:
> Hi Vitaly,
> 
> On 05/07/11 20:00, Vitaly Kuzmichev wrote:
> > 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.
> 
> My take on this is that the calibration loop should simply be killed
> once Colin's patch using the "smp_twd" clock is merged and relevant
> platforms updated. Once this is done, the watchdog driver should use the
> clock directly, without exporting a symbol.

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.

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

[Catalin Marinas]

(sorry for reposting - did something wrong with the SMTP configuration
and the original mail was rejected as spam by most recipients)

On Wed, Jul 06, 2011 at 11:14:56AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 06, 2011 at 11:05:51AM +0100, Marc Zyngier wrote:
> > On 05/07/11 20:00, Vitaly Kuzmichev wrote:
> > > 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.
> > 
> > My take on this is that the calibration loop should simply be killed
> > once Colin's patch using the "smp_twd" clock is merged and relevant
> > platforms updated. Once this is done, the watchdog driver should use the
> > clock directly, without exporting a symbol.
> 
> 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.

-- 
Catalin

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

[Russell King - ARM Linux]

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.

[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.

[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

[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.

[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

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

[Vitaly Kuzmichev]

Hi Marc,

On 07/06/2011 02:05 PM, Marc Zyngier wrote:
[...]
>> 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.
> 
> Be careful here. This branch is a work in progress, likely to change
> very quickly, contains code that has not been posted to the ML yet, and
> will probably eat your pet for breakfast. As far as mainline merging is
> concerned, there is still a long way to go (see the GIC consolidation
> patches, on which the local_timers_as_devices branch relies).

I understand.
When do you plan to finish this job?
Would not it be better to prepare two sets of patches: first one for
linux-2.6/master with exported function, second for arm-platforms.git
with removing exported function and replacing use of it by
clk_get_rate(smp_twd) call?
Is there a chance in this case, that my patches will be merged much
earlier than yours?
If not, I will just rework patch 2/6 to remove exported and use only
clock interface.


Thanks,
Vitaly.

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

[Marc Zyngier]

On 06/07/11 13:27, Vitaly Kuzmichev wrote:
> Hi Marc,
> 
> On 07/06/2011 02:05 PM, Marc Zyngier wrote:
> [...]
>>> 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.
>>
>> Be careful here. This branch is a work in progress, likely to change
>> very quickly, contains code that has not been posted to the ML yet, and
>> will probably eat your pet for breakfast. As far as mainline merging is
>> concerned, there is still a long way to go (see the GIC consolidation
>> patches, on which the local_timers_as_devices branch relies).
> 
> I understand.
> When do you plan to finish this job?
> Would not it be better to prepare two sets of patches: first one for
> linux-2.6/master with exported function, second for arm-platforms.git
> with removing exported function and replacing use of it by
> clk_get_rate(smp_twd) call?
> Is there a chance in this case, that my patches will be merged much
> earlier than yours?

Your guess is as good as mine. It all depends on people's bandwidth to
review long series of patches (the particular branch you used is a merge
of 3 different series), so I'd expect a smaller, less intrusive series
to make it quicker than my monster patch sets... ;-)

> If not, I will just rework patch 2/6 to remove exported and use only
> clock interface.

Make sure all TWD users are updated with the new clock before you merge
it then, or people are going to be surprised.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Vitaly Kuzmichev]

Hi Marc,

On 07/06/2011 04:39 PM, Marc Zyngier wrote:
> On 06/07/11 13:27, Vitaly Kuzmichev wrote:
>> Hi Marc,
>>
>> On 07/06/2011 02:05 PM, Marc Zyngier wrote:
>> [...]
>>>> 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.
>>>
>>> Be careful here. This branch is a work in progress, likely to change
>>> very quickly, contains code that has not been posted to the ML yet, and
>>> will probably eat your pet for breakfast. As far as mainline merging is
>>> concerned, there is still a long way to go (see the GIC consolidation
>>> patches, on which the local_timers_as_devices branch relies).
>>
>> I understand.
>> When do you plan to finish this job?
>> Would not it be better to prepare two sets of patches: first one for
>> linux-2.6/master with exported function, second for arm-platforms.git
>> with removing exported function and replacing use of it by
>> clk_get_rate(smp_twd) call?
>> Is there a chance in this case, that my patches will be merged much
>> earlier than yours?
> 
> Your guess is as good as mine. It all depends on people's bandwidth to
> review long series of patches (the particular branch you used is a merge
> of 3 different series), so I'd expect a smaller, less intrusive series
> to make it quicker than my monster patch sets... ;-)

Ok, but then we are coming back to the V1 series containing the
following patches:
  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

The patches V2 5/6 (Add cpufreq notifier to reload counter) and V2 6/6
(Move declarations in a separate header) are useless without smp_twd
clock and can be threw out.

I will rebase and update V1 series and repost them soon.

Thanks,
Vitaly.

[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 2/6] arm_smp_twd: mpcore_wdt: Fix watchdog counter loading

[Arnd Bergmann]

On Tuesday 05 July 2011, Vitaly Kuzmichev wrote:
> +/* 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);

I would generally prefer to make new exported symbols EXPORT_SYMBOL_GPL
instead of EXPORT_SYMBOL.

	Arnd

[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 3/6] mpcore_wdt: Fix WDIOC_SETOPTIONS handling

[Sergei Shtylyov]

Hello.

On 05-07-2011 23:00, Vitaly Kuzmichev wrote:

> 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

    I think you need parens around this.

> +			|| cmd == WDIOC_SETOPTIONS) {
>   		ret = copy_from_user(&uarg, (void __user *)arg, _IOC_SIZE(cmd));
>   		if (ret)
>   			return -EFAULT;

WBR, Sergei

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

[Arnd Bergmann]

On Tuesday 05 July 2011, Vitaly Kuzmichev wrote:
> 
> 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>

Is the new watchdog driver core ready for 3.1? If so, the best
solution would be to get rid of the entire ioctl function in
the mpcore_wdt driver and just use the core for that.

Otherwise, this patch is probably the best solution in the meantime.

	Arnd

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

[Vitaly Kuzmichev]

Hi Arnd,

On 07/06/2011 04:22 PM, Arnd Bergmann wrote:
> On Tuesday 05 July 2011, Vitaly Kuzmichev wrote:
>>
>> 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>
> 
> Is the new watchdog driver core ready for 3.1? If so, the best
> solution would be to get rid of the entire ioctl function in
> the mpcore_wdt driver and just use the core for that.

In this case the whole mpcore_wdt driver should be rewritten.
And then this should be done in a separate set of patches, I think.
And this may take more time than is seems now.
So I would keep this patch.


Thanks,
Vitaly.

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

[Russell King - ARM Linux]

On Tue, Jul 05, 2011 at 11:00:37PM +0400, Vitaly Kuzmichev wrote:
> 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'.

That looks like something which needs fixing in watchdog land.  And it's
not too difficult:

1. Change the existing WDIOC_SETOPTIONS to WDIOC_SETOPTIONS_OLD
2. Add a new, correctly defined WDIOC_SETOPTIONS.
3. Update all drivers to use WDIOC_SETOPTIONS.
4. Provide a helper which deals with WDIOC_SETOPTIONS_OLD and translates
   it to the proper WDIOC_SETOPTIONS call (this could be just:

	if (cmd == WDIOC_SETOPTIONS_OLD) {
		static int first = 1;
		if (first)
			pr_warn("%s:%d used old WDIOC_SETOPTIONS call.  Please rebuild\n",
				current->comm, current->pid);
		first = 0;
		cmd = WDIOC_SETOPTIONS;
	}

   before any argument copies.

This means you preserve existing compatibility with userspace, yet gain
a path to a non-broken ioctl interface.

As stuff gets rebuilt and replaced, you'll eventually be able to remove
the above.  That period used to be two years for such simple changes.

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

[Russell King - ARM Linux]

Hopefully, someone can let Wim know his email is broken... Also deleted
the broken arm at kernel.org address.

This is the mail system at host ylaen.iguana.be.

I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.

For further assistance, please send mail to <postmaster>

If you do so, please include this problem report. You can
delete your own text from the attached returned message.

                   The mail system

<wim@infomag.iguana.be> (expanded from <wim@iguana.be>): Host or domain name
    not found. Name service error for name=infomag.iguana.be type=AAAA: Host
    found but no data record of requested type

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

[Arnd Bergmann]

On Wednesday 06 July 2011, Russell King - ARM Linux wrote:
> That looks like something which needs fixing in watchdog land.  And it's
> not too difficult:
> 
> 1. Change the existing WDIOC_SETOPTIONS to WDIOC_SETOPTIONS_OLD
> 2. Add a new, correctly defined WDIOC_SETOPTIONS.
> 3. Update all drivers to use WDIOC_SETOPTIONS.
> 4. Provide a helper which deals with WDIOC_SETOPTIONS_OLD and translates
>    it to the proper WDIOC_SETOPTIONS call (this could be just:
> 
>         if (cmd == WDIOC_SETOPTIONS_OLD) {
>                 static int first = 1;
>                 if (first)
>                         pr_warn("%s:%d used old WDIOC_SETOPTIONS call.  Please rebuild\n",
>                                 current->comm, current->pid);
>                 first = 0;
>                 cmd = WDIOC_SETOPTIONS;
>         }
> 
>    before any argument copies.
> 
> This means you preserve existing compatibility with userspace, yet gain
> a path to a non-broken ioctl interface.
> 
> As stuff gets rebuilt and replaced, you'll eventually be able to remove
> the above.  That period used to be two years for such simple changes.

Since Wim is currently doing a new core driver to be used by watchdog
drivers anyway, that sounds like a good idea to fix afterwards.
Note that the mpcore_wdt is the only driver that checks the direction
bit, so it's unlikely to cause problems for anything else.

> Hopefully, someone can let Wim know his email is broken...

This should get through to him I hope. Wim, see below.

> Also deleted
> the broken arm at kernel.org address.

Ok, sorry about that. I need to finally ask the kernel.org team to
set that up, I announced it too early.

	Arnd

> This is the mail system at host ylaen.iguana.be.
> 
> I'm sorry to have to inform you that your message could not
> be delivered to one or more recipients. It's attached below.
> 
> For further assistance, please send mail to <postmaster>
> 
> If you do so, please include this problem report. You can
> delete your own text from the attached returned message.
> 
>                    The mail system
> 
> <wim@infomag.iguana.be> (expanded from <wim@iguana.be>): Host or domain name
>     not found. Name service error for name=infomag.iguana.be type=AAAA: Host
>     found but no data record of requested type
> 

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

[Arnd Bergmann]

On Wednesday 06 July 2011, Arnd Bergmann wrote:
> > Hopefully, someone can let Wim know his email is broken...
> 
> This should get through to him I hope. Wim, see below.
> 

No, same thing here:

On Wednesday 06 July 2011, Mail Delivery System wrote:
> This is the mail system at host ylaen.iguana.be.
> 
> I'm sorry to have to inform you that your message could not
> be delivered to one or more recipients. It's attached below.
> 

	Arnd

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

[Wim Van Sebroeck]

Hi All,

> No, same thing here:
> 
> On Wednesday 06 July 2011, Mail Delivery System wrote:
> > This is the mail system at host ylaen.iguana.be.
> > 
> > I'm sorry to have to inform you that your message could not
> > be delivered to one or more recipients. It's attached below.
> > 

Should be fixed now. It was something DNS related on ylaen.iguana.be apparently.
I hope I didn't miss to much e-mail...

Kind regards,
Wim.

[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 5/6] mpcore_wdt: Add cpufreq notifier to reload counter

[Sergei Shtylyov]

Hello.

On 05-07-2011 23:00, Vitaly Kuzmichev wrote:

> 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
[...]
> @@ -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);

    Need empty line here...

> +	/*
> +	 * 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;

    ... and here.

>   	/*
>   	 * Check that the margin value is within it's range;
>   	 * if not reset to the default

WBR, Sergei

[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

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

[Arnd Bergmann]

On Tuesday 05 July 2011, Vitaly Kuzmichev wrote:
> 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

I don't see the point in this. IMHO it's better to leave the definitions
in the same file that uses them, because they are not shared across
multiple files.

If you intend to share them in the future, you should explain that
in the changelog.

	Arnd

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

[Vitaly Kuzmichev]

Hi Arnd,

On 07/06/2011 03:58 PM, Arnd Bergmann wrote:
> On Tuesday 05 July 2011, Vitaly Kuzmichev wrote:
>> 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
> 
> I don't see the point in this. IMHO it's better to leave the definitions
> in the same file that uses them, because they are not shared across
> multiple files.
> 
> If you intend to share them in the future, you should explain that
> in the changelog.

The patch is aimed to resolve checkpatch warning on "extern" function
prototype:

WARNING: externs should be avoided in .c files
#44: FILE: drivers/watchdog/mpcore_wdt.c:37:
+unsigned long twd_timer_get_rate(void);

If it's ok to leave this warning I would also like to throw out this patch.

Thanks,
Vitaly.

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

[Arnd Bergmann]

On Wednesday 06 July 2011, Vitaly Kuzmichev wrote:
> The patch is aimed to resolve checkpatch warning on "extern" function
> prototype:
> 
> WARNING: externs should be avoided in .c files
> #44: FILE: drivers/watchdog/mpcore_wdt.c:37:
> +unsigned long twd_timer_get_rate(void);
> 
> If it's ok to leave this warning I would also like to throw out this patch.

Ah, I see. That part is indeed an interface, so the declaration should be
in a header file that gets included by both the clocksource and the watchdog
driver.

However, you should not put all the local declarations in the header,
and the header needs to be in a location that gets included by
drivers/clocksource/arm_smp_twd.c as well. In this case, I think
it makes more sense to name that header based on the driver that
exports the function, not based on the driver that uses it,
or you can add it to an *appropriate* existing header file, if you
can find one.

An obvious choice would be arch/arm/include/asm/smp_twd.h.

	Arnd

[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.

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

[Sergei Shtylyov]

Hello.

On 05-07-2011 23:00, Vitaly Kuzmichev wrote:

> 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

    Are the declarations only used by mpcore_wdt.c? If so, why we need a header?

WBR, Sergei

[PATCH V3 0/4] 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 SMP_TWD timer setup code.
The PATCH V3 2/4 adds an exported function to obtain twd timer rate in
mpcore_wdt. It was decided that this solution is the best at this time,
and later it will be removed while conversion smp_twd driver into
the clocksource/arm_smp_twd as well as calibration loop will be replaced
by smp_twd clock. The mpcore_wdt will also use clk interface instead.

In comparing with V2 series this (V3) is based on linux-2.6/master branch.
Also the patches PATCH V2 5/6 and PATCH V2 6/6 were thrown out since they
are useless with current swp_twd driver.
Also it was decided to keep PATCH V2 3/6 as is due to upcoming restructuring
of watchdog drivers, but added parentheses inside 'if' condition.

Vitaly Kuzmichev (4):
  ARM: smp_twd: Fix typo in 'twd_timer_rate' printing
  ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading
  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      |    9 ++++++++-
 drivers/watchdog/mpcore_wdt.c  |   22 ++++++++++++++--------
 3 files changed, 29 insertions(+), 9 deletions(-)

-- 
1.7.3.4

[PATCH V3 1/4] 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 the patch:
    twd_timer_rate / 1000000 = 123
    (twd_timer_rate / 1000000) % 100 = 23
    Result: 123.23MHz.
 After being fixed:
    twd_timer_rate / 1000000 = 123
    (twd_timer_rate / 10000) % 100 = 45
    Result: 123.45MHz.

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 60636f4..2c277d4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -115,7 +115,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 V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
> To get hundredths of MHz the rate needs to be divided by 10'000.
> Here is an example:
>  twd_timer_rate = 123456789
>  Before the patch:
>     twd_timer_rate / 1000000 = 123
>     (twd_timer_rate / 1000000) % 100 = 23
>     Result: 123.23MHz.
>  After being fixed:
>     twd_timer_rate / 1000000 = 123
>     (twd_timer_rate / 10000) % 100 = 45
>     Result: 123.45MHz.
> 
> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>

-> patch system please.

Thanks.

[PATCH V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Vitaly Kuzmichev]

Hi Russell,

On 07/07/2011 04:39 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
>> To get hundredths of MHz the rate needs to be divided by 10'000.
>> Here is an example:
>>  twd_timer_rate = 123456789
>>  Before the patch:
>>     twd_timer_rate / 1000000 = 123
>>     (twd_timer_rate / 1000000) % 100 = 23
>>     Result: 123.23MHz.
>>  After being fixed:
>>     twd_timer_rate / 1000000 = 123
>>     (twd_timer_rate / 10000) % 100 = 45
>>     Result: 123.45MHz.
>>
>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> 
> -> patch system please.

For a such newbie as me it's hard to understand this comment, sorry :)
Could you explain, please?

Thanks,
Vitaly.

[PATCH V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Vitaly Kuzmichev]

Hi,

On 07/07/2011 04:50 PM, Vitaly Kuzmichev wrote:
> Hi Russell,
> 
> On 07/07/2011 04:39 PM, Russell King - ARM Linux wrote:
>> On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
>>> To get hundredths of MHz the rate needs to be divided by 10'000.
>>> Here is an example:
>>>  twd_timer_rate = 123456789
>>>  Before the patch:
>>>     twd_timer_rate / 1000000 = 123
>>>     (twd_timer_rate / 1000000) % 100 = 23
>>>     Result: 123.23MHz.
>>>  After being fixed:
>>>     twd_timer_rate / 1000000 = 123
>>>     (twd_timer_rate / 10000) % 100 = 45
>>>     Result: 123.45MHz.
>>>
>>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
>>
>> -> patch system please.
> 
> For a such newbie as me it's hard to understand this comment, sorry :)
> Could you explain, please?

Ignore my question, I've learned.

Thanks,
Vitaly.

[PATCH V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Vitaly Kuzmichev]

Hi Russell,

On 07/07/2011 04:39 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
>> To get hundredths of MHz the rate needs to be divided by 10'000.
>> Here is an example:
>>  twd_timer_rate = 123456789
>>  Before the patch:
>>     twd_timer_rate / 1000000 = 123
>>     (twd_timer_rate / 1000000) % 100 = 23
>>     Result: 123.23MHz.
>>  After being fixed:
>>     twd_timer_rate / 1000000 = 123
>>     (twd_timer_rate / 10000) % 100 = 45
>>     Result: 123.45MHz.
>>
>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> 
> -> patch system please.

Should I submit other patches into this system too?

Thanks,
Vitaly.

[PATCH V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 08:20:49PM +0400, Vitaly Kuzmichev wrote:
> Hi Russell,
> 
> On 07/07/2011 04:39 PM, Russell King - ARM Linux wrote:
> > On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
> >> To get hundredths of MHz the rate needs to be divided by 10'000.
> >> Here is an example:
> >>  twd_timer_rate = 123456789
> >>  Before the patch:
> >>     twd_timer_rate / 1000000 = 123
> >>     (twd_timer_rate / 1000000) % 100 = 23
> >>     Result: 123.23MHz.
> >>  After being fixed:
> >>     twd_timer_rate / 1000000 = 123
> >>     (twd_timer_rate / 10000) % 100 = 45
> >>     Result: 123.45MHz.
> >>
> >> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> > 
> > -> patch system please.
> 
> Should I submit other patches into this system too?

No, the other patches I've yet to look at, and I think there's issues
still to be resolved.

Eg, ISTR a patch to make smp_twd.c selectable on non-SMP platforms so
that the mpcore watchdog can be used on non-SMP platforms.  While that
fixes the build issues, functionally its broken.  I don't think that
the mpcore watchdog should be asking the smp_twd code in any case for
the clock rate.

If we get the issue with knowing the TWD clock rate on ARMs development
platforms sorted out, then we can provide a struct clk for it, which
means everyone can finally move to using the clk API to get the mpcore
watchdog/twd clock rate.

[PATCH V3 1/4] ARM: smp_twd: Fix typo in 'twd_timer_rate' printing

[Vitaly Kuzmichev]

Hi Russell,

On 07/10/2011 02:47 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 07, 2011 at 08:20:49PM +0400, Vitaly Kuzmichev wrote:
>> Hi Russell,
>>
>> On 07/07/2011 04:39 PM, Russell King - ARM Linux wrote:
>>> On Thu, Jul 07, 2011 at 04:23:13PM +0400, Vitaly Kuzmichev wrote:
>>>> To get hundredths of MHz the rate needs to be divided by 10'000.
>>>> Here is an example:
>>>>  twd_timer_rate = 123456789
>>>>  Before the patch:
>>>>     twd_timer_rate / 1000000 = 123
>>>>     (twd_timer_rate / 1000000) % 100 = 23
>>>>     Result: 123.23MHz.
>>>>  After being fixed:
>>>>     twd_timer_rate / 1000000 = 123
>>>>     (twd_timer_rate / 10000) % 100 = 45
>>>>     Result: 123.45MHz.
>>>>
>>>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
>>>
>>> -> patch system please.
>>
>> Should I submit other patches into this system too?
> 
> No, the other patches I've yet to look at, and I think there's issues
> still to be resolved.
> 
> Eg, ISTR a patch to make smp_twd.c selectable on non-SMP platforms so
> that the mpcore watchdog can be used on non-SMP platforms.  While that
> fixes the build issues, functionally its broken.  I don't think that
> the mpcore watchdog should be asking the smp_twd code in any case for
> the clock rate.

I'm guessing that you mean here both cases when non-SMP kernel runs on
multi-core and on single-core (I'm not sure whether they really exist)
MPCore CPUs, do you? I think in both these cases TWD timer(s) should
always be presented in MPCore. So it is wrong to call this timer as
SMP_TWD and make it dependent on CONFIG_SMP, isn't it?

I just mean, probably the fact that mpcore_wdt depends on swp_twd is not
a real issue, and my patch is correct, but does not fix everything. In
this case it's better than nothing, better than watchdog that does not
work properly either on SMP, or on non-SMP platforms.
Note that my patch does not introduce new dependencies, it just relies
on the one that already exists.

As regarding to new clocksource/arm_smp_twd driver, if it can be
compiled for non-SMP kernel/platform the mpcore_wdt will also be able to
do the same (even with only my patch). But in the case of clk interface
mpcore_wdt will be dependent on TWD's clk structure anyway. So I can
think only of one case when it can be fully independent from TWD: having
own calibration loop.

> If we get the issue with knowing the TWD clock rate on ARMs development
> platforms sorted out, then we can provide a struct clk for it, which
> means everyone can finally move to using the clk API to get the mpcore
> watchdog/twd clock rate.

I really like this. And I will be happy if this happens before than I'm
expecting :)
Ok, lets assume that this happened. Can you guarantee that the one who
will fix the mpcore_wdt to use clk interface also will not reuse the
formula that was there before? :)


Thanks,
Vitaly.

[PATCH V3 2/4] 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.

MPCORE_WATCHDOG needed to build 'mpcore_wdt' already depends on
HAVE_ARM_TWD needed to build 'smp_twd', so from the point of view of
'mpcore_wdt' driver the exported function will always exist.

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

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index fed9981..38130b1 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -24,5 +24,6 @@ extern void __iomem *twd_base;
 
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+unsigned long twd_timer_get_rate(void);
 
 #endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 2c277d4..c341ebc 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -142,3 +142,10 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 }
+
+/* Needed by mpcore_wdt */
+unsigned long twd_timer_get_rate(void)
+{
+	return twd_timer_rate;
+}
+EXPORT_SYMBOL(twd_timer_get_rate);
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 2b4af22..efde50b 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -99,9 +99,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 V3 2/4] ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading

[Arnd Bergmann]

On Thursday 07 July 2011, Vitaly Kuzmichev wrote:
> 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.
> 
> MPCORE_WATCHDOG needed to build 'mpcore_wdt' already depends on
> HAVE_ARM_TWD needed to build 'smp_twd', so from the point of view of
> 'mpcore_wdt' driver the exported function will always exist.
> 
> Signed-off-by: Valentine Barshak <vbarshak@mvista.com>
> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>

Looks good now,

Acked-by: Arnd Bergmann <arnd@arndb.de>

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

[Vitaly Kuzmichev]

Hi Arnd,

On 07/07/2011 04:44 PM, Arnd Bergmann wrote:
> On Thursday 07 July 2011, Vitaly Kuzmichev wrote:
>> 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.
>>
>> MPCORE_WATCHDOG needed to build 'mpcore_wdt' already depends on
>> HAVE_ARM_TWD needed to build 'smp_twd', so from the point of view of
>> 'mpcore_wdt' driver the exported function will always exist.
>>
>> Signed-off-by: Valentine Barshak <vbarshak@mvista.com>
>> Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
> 
> Looks good now,

Unfortunately, I forgot to change to change EXPORT_SYMBOL ->
EXPORT_SYMBOL_GPL.
Ignore this patch and review PATCH V4 2/4 instead.
Sorry for troubles.

[PATCH V3 3/4] ARM: 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 efde50b..f99e49e 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -233,7 +233,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 V3 3/4] ARM: mpcore_wdt: Fix WDIOC_SETOPTIONS handling

[Arnd Bergmann]

On Thursday 07 July 2011, Vitaly Kuzmichev wrote:
> 
> 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>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH V3 4/4] ARM: 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>
---
 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 38130b1..42d4557 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 f99e49e..23bafb1 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -118,18 +118,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 V3 0/4] ARM: smp_twd: mpcore_wdt: Fix MPCORE watchdog setup

[Wim Van Sebroeck]

Hi Vitaly,

> 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 V3 2/4 adds an exported function to obtain twd timer rate in
> mpcore_wdt. It was decided that this solution is the best at this time,
> and later it will be removed while conversion smp_twd driver into
> the clocksource/arm_smp_twd as well as calibration loop will be replaced
> by smp_twd clock. The mpcore_wdt will also use clk interface instead.
> 
> In comparing with V2 series this (V3) is based on linux-2.6/master branch.
> Also the patches PATCH V2 5/6 and PATCH V2 6/6 were thrown out since they
> are useless with current swp_twd driver.
> Also it was decided to keep PATCH V2 3/6 as is due to upcoming restructuring
> of watchdog drivers, but added parentheses inside 'if' condition.
> 
> Vitaly Kuzmichev (4):
>   ARM: smp_twd: Fix typo in 'twd_timer_rate' printing
>   ARM: smp_twd: mpcore_wdt: Fix watchdog counter loading
>   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      |    9 ++++++++-
>  drivers/watchdog/mpcore_wdt.c  |   22 ++++++++++++++--------
>  3 files changed, 29 insertions(+), 9 deletions(-)

Acked-by: Wim Van Sebroeck <wim@iguana.be>

Kind regards,
Wim.

[PATCH V4 2/4] 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.

MPCORE_WATCHDOG needed to build 'mpcore_wdt' already depends on
HAVE_ARM_TWD needed to build 'smp_twd', so from the point of view of
'mpcore_wdt' driver the exported function will always exist.

Signed-off-by: Valentine Barshak <vbarshak@mvista.com>
Signed-off-by: Vitaly Kuzmichev <vkuzmichev@mvista.com>
---

Forgot to change EXPORT_SYMBOL -> EXPORT_SYMBOL_GPL, sorry
Other patches are unchanged.

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

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index fed9981..38130b1 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -24,5 +24,6 @@ extern void __iomem *twd_base;
 
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+unsigned long twd_timer_get_rate(void);
 
 #endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 2c277d4..dd64fae 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -142,3 +142,10 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 }
+
+/* Needed by mpcore_wdt */
+unsigned long twd_timer_get_rate(void)
+{
+	return twd_timer_rate;
+}
+EXPORT_SYMBOL_GPL(twd_timer_get_rate);
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 2b4af22..efde50b 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -99,9 +99,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