[PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Tommy Lin]

Original MPcore watchdog setting about load register (offset 0x20) is different
from CNS3xxx data sheet. The CNS3xxx data sheet says watchdog has following
features:
1. The Watchdog Counter Register (offset 0x24) is a down counter.
2. The timer interval is calculated using following equation:
   (PRESCALER_value+1) X (Load_value+1) X 2 / CPU CLK_frequency
Thus the watchdog load register control in CNS3xxx way is add to MPcore watchdog
source. The original control method is also kept if the CPU architecture is not
CNS3xxx.

Signed-off-by: Tommy Lin <tommy.lin@caviumnetworks.com>
---
 drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 2b4af22..816e0c5 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -42,6 +42,9 @@ struct mpcore_wdt {
 	int		irq;
 	unsigned int	perturb;
 	char		expect_close;
+#ifdef CONFIG_ARCH_CNS3XXX
+	unsigned long	reload_unit; /* ticks per second */
+#endif
 };
 
 static struct platform_device *mpcore_wdt_dev;
@@ -98,10 +101,14 @@ static void mpcore_wdt_keepalive(struct mpcore_wdt *wdt)
 	unsigned long count;
 
 	spin_lock(&wdt_lock);
+#ifdef CONFIG_ARCH_CNS3XXX
+	count = wdt->reload_unit * mpcore_margin;
+#else
 	/* Assume prescale is set to 256 */
 	count =  __raw_readl(wdt->base + TWD_WDOG_COUNTER);
 	count = (0xFFFFFFFFU - count) * (HZ / 5);
 	count = (count / 256) * mpcore_margin;
+#endif
 
 	/* Reload the counter */
 	writel(count + wdt->perturb, wdt->base + TWD_WDOG_LOAD);
@@ -375,6 +382,11 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev)
 		goto err_irq;
 	}
 
+#ifdef CONFIG_ARCH_CNS3XXX
+	/* Assume prescale is set to 256 */
+	wdt->reload_unit = cns3xxx_cpu_clock() * 1000000 / 256 / 2;
+#endif
+
 	mpcore_wdt_stop(wdt);
 	platform_set_drvdata(dev, wdt);
 	mpcore_wdt_dev = dev;
-- 
1.7.6

Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Anton Vorontsov]

Hello,

thanks for the patches.

On Tue, Jul 05, 2011 at 08:05:48PM +0800, Tommy Lin wrote:
> Original MPcore watchdog setting about load register (offset 0x20) is different
> from CNS3xxx data sheet. The CNS3xxx data sheet says watchdog has following
> features:
> 1. The Watchdog Counter Register (offset 0x24) is a down counter.
> 2. The timer interval is calculated using following equation:
>    (PRESCALER_value+1) X (Load_value+1) X 2 / CPU CLK_frequency
> Thus the watchdog load register control in CNS3xxx way is add to MPcore watchdog
> source. The original control method is also kept if the CPU architecture is not
> CNS3xxx.
> 
> Signed-off-by: Tommy Lin <tommy.lin@caviumnetworks.com>
> ---
>  drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index 2b4af22..816e0c5 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
> @@ -42,6 +42,9 @@ struct mpcore_wdt {
>  	int		irq;
>  	unsigned int	perturb;
>  	char		expect_close;
> +#ifdef CONFIG_ARCH_CNS3XXX
> +	unsigned long	reload_unit; /* ticks per second */
> +#endif

Nope, these #ifdefs are not acceptible. Kernel should support multi-arch
images soon.

[...]
> +#ifdef CONFIG_ARCH_CNS3XXX
> +	/* Assume prescale is set to 256 */
> +	wdt->reload_unit = cns3xxx_cpu_clock() * 1000000 / 256 / 2;
> +#endif

You probably want to pass reload_unit via platform_data, and also
check (in runtime) that the watchdog is CNS3xxx. Or if reload_unit
!= 0.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Tommy Lin]

Hi, I just found another patch that can solve the watchdog counter reload
issue. Please ignore this patch, since it should not acceptable to use
#ifdefs.

From	Vitaly Kuzmichev <vkuzmichev@mvista.com>
Subject	[PATCH V2 2/6] arm_smp_twd: mpcore_wdt: Fix watchdog counter loading
Date	Tue, 5 Jul 2011 23:00:36 +0400

--
Tommy Lin

-----Original Message-----
From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] 
Sent: Tuesday, July 05, 2011 10:06 PM
To: Tommy Lin
Cc: Wim Van Sebroeck; linux-watchdog@vger.kernel.org;
linux-kernel@vger.kernel.org; mac.lin@caviumnetworks.com; Tommy Lin
Subject: Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for
CNS3xxx hardware

Hello,

thanks for the patches.

On Tue, Jul 05, 2011 at 08:05:48PM +0800, Tommy Lin wrote:
> Original MPcore watchdog setting about load register (offset 0x20) is
different
> from CNS3xxx data sheet. The CNS3xxx data sheet says watchdog has
following
> features:
> 1. The Watchdog Counter Register (offset 0x24) is a down counter.
> 2. The timer interval is calculated using following equation:
>    (PRESCALER_value+1) X (Load_value+1) X 2 / CPU CLK_frequency
> Thus the watchdog load register control in CNS3xxx way is add to MPcore
watchdog
> source. The original control method is also kept if the CPU architecture
is not
> CNS3xxx.
> 
> Signed-off-by: Tommy Lin <tommy.lin@caviumnetworks.com>
> ---
>  drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index 2b4af22..816e0c5 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
> @@ -42,6 +42,9 @@ struct mpcore_wdt {
>  	int		irq;
>  	unsigned int	perturb;
>  	char		expect_close;
> +#ifdef CONFIG_ARCH_CNS3XXX
> +	unsigned long	reload_unit; /* ticks per second */
> +#endif

Nope, these #ifdefs are not acceptible. Kernel should support multi-arch
images soon.

[...]
> +#ifdef CONFIG_ARCH_CNS3XXX
> +	/* Assume prescale is set to 256 */
> +	wdt->reload_unit = cns3xxx_cpu_clock() * 1000000 / 256 / 2;
> +#endif

You probably want to pass reload_unit via platform_data, and also
check (in runtime) that the watchdog is CNS3xxx. Or if reload_unit
!= 0.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Anton Vorontsov]

On Wed, Jul 06, 2011 at 06:05:37PM +0800, Tommy Lin wrote:
> Hi, I just found another patch that can solve the watchdog counter reload
> issue. Please ignore this patch, since it should not acceptable to use
> #ifdefs.

OK, great. Do I understand correctly that without Vitaly's patch,
your first patch is somewhat useless, as the watchdog will not work
or will work incorrectly? So should I postpone merging patch 1/2
until Vitaly's patch hit mainline?

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Anton Vorontsov]

On Wed, Jul 06, 2011 at 03:16:16PM +0400, Anton Vorontsov wrote:
> On Wed, Jul 06, 2011 at 06:05:37PM +0800, Tommy Lin wrote:
> > Hi, I just found another patch that can solve the watchdog counter reload
> > issue. Please ignore this patch, since it should not acceptable to use
> > #ifdefs.
> 
> OK, great. Do I understand correctly that without Vitaly's patch,
> your first patch is somewhat useless, as the watchdog will not work
> or will work incorrectly? So should I postpone merging patch 1/2
> until Vitaly's patch hit mainline?

Oh, I see that the issue isn't major, so I'll just merge the first
patch.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Anton Vorontsov]

On Wed, Jul 06, 2011 at 03:28:30PM +0400, Anton Vorontsov wrote:
> On Wed, Jul 06, 2011 at 03:16:16PM +0400, Anton Vorontsov wrote:
> > On Wed, Jul 06, 2011 at 06:05:37PM +0800, Tommy Lin wrote:
> > > Hi, I just found another patch that can solve the watchdog counter reload
> > > issue. Please ignore this patch, since it should not acceptable to use
> > > #ifdefs.
> > 
> > OK, great. Do I understand correctly that without Vitaly's patch,
> > your first patch is somewhat useless, as the watchdog will not work
> > or will work incorrectly? So should I postpone merging patch 1/2
> > until Vitaly's patch hit mainline?
> 
> Oh, I see that the issue isn't major, so I'll just merge the first
> patch.

Heh. Nope. It doesn't work without SMP:

warning: (ARCH_CNS3XXX) selects HAVE_ARM_TWD which has unmet direct dependencies (SMP)

And SMP support is not in the mainline, yet.

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Lin, Tommy]

I am thinking that mpcore_wdt should work even SMP is not turned on. Did the dependency 
is neccesarry between mpcore_wdt and SMP? If CPU1 is turned off the mpcore_wdt worked
well with CPU0 only.

Best regards,
Tommy



-----Original Message-----
From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
Sent: 7/6/2011 [星期三] 7:46 下午
To: Lin, Tommy
Cc: 'Tommy Lin'; 'Wim Van Sebroeck'; linux-watchdog@vger.kernel.org; linux-kernel@vger.kernel.org; Lin, Mac
Subject: Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware
 
On Wed, Jul 06, 2011 at 03:28:30PM +0400, Anton Vorontsov wrote:
> On Wed, Jul 06, 2011 at 03:16:16PM +0400, Anton Vorontsov wrote:
> > On Wed, Jul 06, 2011 at 06:05:37PM +0800, Tommy Lin wrote:
> > > Hi, I just found another patch that can solve the watchdog counter reload
> > > issue. Please ignore this patch, since it should not acceptable to use
> > > #ifdefs.
> > 
> > OK, great. Do I understand correctly that without Vitaly's patch,
> > your first patch is somewhat useless, as the watchdog will not work
> > or will work incorrectly? So should I postpone merging patch 1/2
> > until Vitaly's patch hit mainline?
> 
> Oh, I see that the issue isn't major, so I'll just merge the first
> patch.

Heh. Nope. It doesn't work without SMP:

warning: (ARCH_CNS3XXX) selects HAVE_ARM_TWD which has unmet direct dependencies (SMP)

And SMP support is not in the mainline, yet.

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Anton Vorontsov]

On Wed, Jul 06, 2011 at 07:54:49AM -0700, Lin, Tommy wrote:
> I am thinking that mpcore_wdt should work even SMP is not turned on. Did the dependency 
> is neccesarry between mpcore_wdt and SMP? If CPU1 is turned off the mpcore_wdt worked
> well with CPU0 only.

No doubt that the watchdog itself works fine. :-) But the point is
that the patch yields a warning, so it should be fixed before merging.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for CNS3xxx hardware

[Tommy Lin]

I submit another patch that can solve the warning.
[PATCH 1/2] arch: arm: Correct the dependency of HAVE_ARM_TWD
This patch should after above one.

Should I submit another patch or just attach it in this thread?

Best regards,
Tommy Lin

-----Original Message-----
From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] 
Sent: Thursday, July 07, 2011 3:35 AM
To: Lin, Tommy
Cc: Tommy Lin; Wim Van Sebroeck; linux-watchdog@vger.kernel.org;
linux-kernel@vger.kernel.org; Lin, Mac
Subject: Re: [PATCH 2/2] watchdog: mpcore_wdt Add reload value setting for
CNS3xxx hardware

On Wed, Jul 06, 2011 at 07:54:49AM -0700, Lin, Tommy wrote:
> I am thinking that mpcore_wdt should work even SMP is not turned on. Did
the dependency 
> is neccesarry between mpcore_wdt and SMP? If CPU1 is turned off the
mpcore_wdt worked
> well with CPU0 only.

No doubt that the watchdog itself works fine. :-) But the point is
that the patch yields a warning, so it should be fixed before merging.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com