[PATCH 0/3] SP805 updates for Versatile Express

[PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

Here are some updates to get the SP805 watchdog working on the ARM
Versatile Express.  The first two patches fix observed issues; the
third fixes a potential (but not observed) issue with asynchronous
posted writes.

All three patches are independent and should be applicable in any
order, with some fuzz on the third.

[PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

Here are some updates to get the SP805 watchdog working on the ARM
Versatile Express.  The first two patches fix observed issues; the
third fixes a potential (but not observed) issue with asynchronous
posted writes.

All three patches are independent and should be applicable in any
order, with some fuzz on the third.

[PATCH 1/3] ARM: vexpress: Add clock definition for the SP805.

[Nick Bowler]

It seems that an entry for the SP805 watchdog in the table of clocks was
missing.  This results in the sp805_wdt driver rejecting the device with
the following errors:

  sp805-wdt mb:wdt: Clock not found
  sp805-wdt mb:wdt: Probe Failed!!!
  sp805-wdt: probe of mb:wdt failed with error -2

While not obviously stated in the hardware docs, the onboard SP810's
"REFCLK" is connected to a 32.768KHz crystal, and this drives the
watchdog.  Add a struct clk and corresponding lookup entry for it.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 arch/arm/mach-vexpress/v2m.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 9e6b93b..d0d267a 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -318,6 +318,10 @@ static struct clk v2m_sp804_clk = {
 	.rate	= 1000000,
 };
 
+static struct clk v2m_ref_clk = {
+	.rate   = 32768,
+};
+
 static struct clk dummy_apb_pclk;
 
 static struct clk_lookup v2m_lookups[] = {
@@ -348,6 +352,9 @@ static struct clk_lookup v2m_lookups[] = {
 	}, {	/* CLCD */
 		.dev_id		= "mb:clcd",
 		.clk		= &osc1_clk,
+	}, {	/* SP805 WDT */
+		.dev_id		= "mb:wdt",
+		.clk		= &v2m_ref_clk,
 	}, {	/* SP804 timers */
 		.dev_id		= "sp804",
 		.con_id		= "v2m-timer0",
-- 
1.7.3.4

[PATCH 1/3] ARM: vexpress: Add clock definition for the SP805.

[Nick Bowler]

It seems that an entry for the SP805 watchdog in the table of clocks was
missing.  This results in the sp805_wdt driver rejecting the device with
the following errors:

  sp805-wdt mb:wdt: Clock not found
  sp805-wdt mb:wdt: Probe Failed!!!
  sp805-wdt: probe of mb:wdt failed with error -2

While not obviously stated in the hardware docs, the onboard SP810's
"REFCLK" is connected to a 32.768KHz crystal, and this drives the
watchdog.  Add a struct clk and corresponding lookup entry for it.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 arch/arm/mach-vexpress/v2m.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 9e6b93b..d0d267a 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -318,6 +318,10 @@ static struct clk v2m_sp804_clk = {
 	.rate	= 1000000,
 };
 
+static struct clk v2m_ref_clk = {
+	.rate   = 32768,
+};
+
 static struct clk dummy_apb_pclk;
 
 static struct clk_lookup v2m_lookups[] = {
@@ -348,6 +352,9 @@ static struct clk_lookup v2m_lookups[] = {
 	}, {	/* CLCD */
 		.dev_id		= "mb:clcd",
 		.clk		= &osc1_clk,
+	}, {	/* SP805 WDT */
+		.dev_id		= "mb:wdt",
+		.clk		= &v2m_ref_clk,
 	}, {	/* SP804 timers */
 		.dev_id		= "sp804",
 		.con_id		= "v2m-timer0",
-- 
1.7.3.4

[PATCH 2/3] watchdog: sp805: Don't write 0 to the load value register.

[Nick Bowler]

At least on the Versatile Express' V2M, calling wdt_disable followed by
wdt_enable, for instance by running the following sequence:

  echo V > /dev/watchdog; echo V > /dev/watchdog

results in an immediate reset.  The wdt_disable function writes 0 to the
load register; while the watchdog interrupts are disabled at this point,
this special value is defined to trigger an interrupt immediately.  It
appears that in this instance, the reset happens when the interrupts
are subsequently enabled by wdt_enable.

Putting in a short delay after writing a new load value in wdt_enable
solves the issue, but it seems cleaner to simply never write 0 to the
load register at all: according to the hardware docs, writing 0 to the
control register suffices to stop the counter, and the write of 0 to
the load register is questionable anyway since this register resets to
0xffffffff.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 drivers/watchdog/sp805_wdt.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 0d80e08..c1e099a 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -144,7 +144,6 @@ static void wdt_disable(void)
 
 	writel(UNLOCK, wdt->base + WDTLOCK);
 	writel(0, wdt->base + WDTCONTROL);
-	writel(0, wdt->base + WDTLOAD);
 	writel(LOCK, wdt->base + WDTLOCK);
 
 	spin_unlock(&wdt->lock);
-- 
1.7.3.4

[PATCH 2/3] watchdog: sp805: Don't write 0 to the load value register.

[Nick Bowler]

At least on the Versatile Express' V2M, calling wdt_disable followed by
wdt_enable, for instance by running the following sequence:

  echo V > /dev/watchdog; echo V > /dev/watchdog

results in an immediate reset.  The wdt_disable function writes 0 to the
load register; while the watchdog interrupts are disabled at this point,
this special value is defined to trigger an interrupt immediately.  It
appears that in this instance, the reset happens when the interrupts
are subsequently enabled by wdt_enable.

Putting in a short delay after writing a new load value in wdt_enable
solves the issue, but it seems cleaner to simply never write 0 to the
load register at all: according to the hardware docs, writing 0 to the
control register suffices to stop the counter, and the write of 0 to
the load register is questionable anyway since this register resets to
0xffffffff.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 drivers/watchdog/sp805_wdt.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 0d80e08..c1e099a 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -144,7 +144,6 @@ static void wdt_disable(void)
 
 	writel(UNLOCK, wdt->base + WDTLOCK);
 	writel(0, wdt->base + WDTCONTROL);
-	writel(0, wdt->base + WDTLOAD);
 	writel(LOCK, wdt->base + WDTLOCK);
 
 	spin_unlock(&wdt->lock);
-- 
1.7.3.4

[PATCH 3/3] watchdog: sp805: Flush posted writes in enable/disable.

[Nick Bowler]

There are no reads in these functions, so if MMIO writes are posted,
the writes in enable/disable may not have completed by the time these
functions return.  If the functions run from different CPUs, it's
in theory possible for the writes to be interleaved, which would be
disastrous for this driver.

At the very least, we need an mmiowb() before releasing the lock, but
since it seems desirable for the watchdog timer to be actually stopped
or reset when these functions return, read the lock register to force
the writes out.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 drivers/watchdog/sp805_wdt.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index c1e099a..cc2cfbe 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -134,6 +134,8 @@ static void wdt_enable(void)
 	writel(INT_ENABLE | RESET_ENABLE, wdt->base + WDTCONTROL);
 	writel(LOCK, wdt->base + WDTLOCK);
 
+	/* Flush posted writes. */
+	readl(wdt->base + WDTLOCK);
 	spin_unlock(&wdt->lock);
 }
 
@@ -146,6 +148,8 @@ static void wdt_disable(void)
 	writel(0, wdt->base + WDTCONTROL);
 	writel(LOCK, wdt->base + WDTLOCK);
 
+	/* Flush posted writes. */
+	readl(wdt->base + WDTLOCK);
 	spin_unlock(&wdt->lock);
 }
 
-- 
1.7.3.4

[PATCH 3/3] watchdog: sp805: Flush posted writes in enable/disable.

[Nick Bowler]

There are no reads in these functions, so if MMIO writes are posted,
the writes in enable/disable may not have completed by the time these
functions return.  If the functions run from different CPUs, it's
in theory possible for the writes to be interleaved, which would be
disastrous for this driver.

At the very least, we need an mmiowb() before releasing the lock, but
since it seems desirable for the watchdog timer to be actually stopped
or reset when these functions return, read the lock register to force
the writes out.

Signed-off-by: Nick Bowler <nbowler@elliptictech.com>
---
 drivers/watchdog/sp805_wdt.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index c1e099a..cc2cfbe 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -134,6 +134,8 @@ static void wdt_enable(void)
 	writel(INT_ENABLE | RESET_ENABLE, wdt->base + WDTCONTROL);
 	writel(LOCK, wdt->base + WDTLOCK);
 
+	/* Flush posted writes. */
+	readl(wdt->base + WDTLOCK);
 	spin_unlock(&wdt->lock);
 }
 
@@ -146,6 +148,8 @@ static void wdt_disable(void)
 	writel(0, wdt->base + WDTCONTROL);
 	writel(LOCK, wdt->base + WDTLOCK);
 
+	/* Flush posted writes. */
+	readl(wdt->base + WDTLOCK);
 	spin_unlock(&wdt->lock);
 }
 
-- 
1.7.3.4

Re: [PATCH 0/3] SP805 updates for Versatile Express

[Wim Van Sebroeck]

Hi Nick,

> Here are some updates to get the SP805 watchdog working on the ARM
> Versatile Express.  The first two patches fix observed issues; the
> third fixes a potential (but not observed) issue with asynchronous
> posted writes.
> 
> All three patches are independent and should be applicable in any
> order, with some fuzz on the third.

looks good at first glance.

Russel: what's your opinion?

Kind regards,
Wim.

[PATCH 0/3] SP805 updates for Versatile Express

[Wim Van Sebroeck]

Hi Nick,

> Here are some updates to get the SP805 watchdog working on the ARM
> Versatile Express.  The first two patches fix observed issues; the
> third fixes a potential (but not observed) issue with asynchronous
> posted writes.
> 
> All three patches are independent and should be applicable in any
> order, with some fuzz on the third.

looks good at first glance.

Russel: what's your opinion?

Kind regards,
Wim.

Re: [PATCH 0/3] SP805 updates for Versatile Express

[Russell King - ARM Linux]

On Fri, Jul 15, 2011 at 10:59:54PM +0200, Wim Van Sebroeck wrote:
> Hi Nick,
> 
> > Here are some updates to get the SP805 watchdog working on the ARM
> > Versatile Express.  The first two patches fix observed issues; the
> > third fixes a potential (but not observed) issue with asynchronous
> > posted writes.
> > 
> > All three patches are independent and should be applicable in any
> > order, with some fuzz on the third.
> 
> looks good at first glance.
> 
> Russel: what's your opinion?

Win,

This series looks fine.  However, looking through the driver, there's
a few oddities:

static u32 wdt_timeleft(void)
{
        u64 load, rate;

        rate = clk_get_rate(wdt->clk);

        spin_lock(&wdt->lock);
        load = readl(wdt->base + WDTVALUE);

        /*If the interrupt is inactive then time left is WDTValue + WDTLoad. */
        if (!(readl(wdt->base + WDTRIS) & INT_MASK))
                load += wdt->load_val + 1;
        spin_unlock(&wdt->lock);

        return div_u64(load, rate);
}

clk_get_rate returns an unsigned long, not a u64.  There's no point
expanding it to a 64-bit int for div_u64 - rate might as well be
declared as an unsigned long.

Same goes for wdt_setload, and there stuff like "div_u64(rate, 2)"
can become normal maths rather than the special 64-bit stuff.

Then there's:

        if (!request_mem_region(adev->res.start, resource_size(&adev->res),
                                "sp805_wdt")) {
                dev_warn(&adev->dev, "Failed to get memory region resource\n");
                ret = -ENOENT;
                goto err;
        }

Is amba_request_regions()/amba_release_regions() not good enough to
handle requesting/releasing these regions?

[PATCH 0/3] SP805 updates for Versatile Express

[Russell King - ARM Linux]

On Fri, Jul 15, 2011 at 10:59:54PM +0200, Wim Van Sebroeck wrote:
> Hi Nick,
> 
> > Here are some updates to get the SP805 watchdog working on the ARM
> > Versatile Express.  The first two patches fix observed issues; the
> > third fixes a potential (but not observed) issue with asynchronous
> > posted writes.
> > 
> > All three patches are independent and should be applicable in any
> > order, with some fuzz on the third.
> 
> looks good at first glance.
> 
> Russel: what's your opinion?

Win,

This series looks fine.  However, looking through the driver, there's
a few oddities:

static u32 wdt_timeleft(void)
{
        u64 load, rate;

        rate = clk_get_rate(wdt->clk);

        spin_lock(&wdt->lock);
        load = readl(wdt->base + WDTVALUE);

        /*If the interrupt is inactive then time left is WDTValue + WDTLoad. */
        if (!(readl(wdt->base + WDTRIS) & INT_MASK))
                load += wdt->load_val + 1;
        spin_unlock(&wdt->lock);

        return div_u64(load, rate);
}

clk_get_rate returns an unsigned long, not a u64.  There's no point
expanding it to a 64-bit int for div_u64 - rate might as well be
declared as an unsigned long.

Same goes for wdt_setload, and there stuff like "div_u64(rate, 2)"
can become normal maths rather than the special 64-bit stuff.

Then there's:

        if (!request_mem_region(adev->res.start, resource_size(&adev->res),
                                "sp805_wdt")) {
                dev_warn(&adev->dev, "Failed to get memory region resource\n");
                ret = -ENOENT;
                goto err;
        }

Is amba_request_regions()/amba_release_regions() not good enough to
handle requesting/releasing these regions?

Re: [PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

On 2011-07-18 10:44 +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 15, 2011 at 10:59:54PM +0200, Wim Van Sebroeck wrote:
> > looks good at first glance.
> > 
> > Russel: what's your opinion?
> 
> Win,
> 
> This series looks fine.  However, looking through the driver, there's
> a few oddities:

Indeed, there are likely several other ways in which this driver can be
improved.  My goal was to get it working on our board first. :)

So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
patch tracker.  I suppose #2 and #3 should go through the watchdog tree?

Thanks,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

[PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

On 2011-07-18 10:44 +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 15, 2011 at 10:59:54PM +0200, Wim Van Sebroeck wrote:
> > looks good at first glance.
> > 
> > Russel: what's your opinion?
> 
> Win,
> 
> This series looks fine.  However, looking through the driver, there's
> a few oddities:

Indeed, there are likely several other ways in which this driver can be
improved.  My goal was to get it working on our board first. :)

So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
patch tracker.  I suppose #2 and #3 should go through the watchdog tree?

Thanks,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [PATCH 0/3] SP805 updates for Versatile Express

[Wim Van Sebroeck]

Hi Nick, Russel,

> > > looks good at first glance.
> > > 
> > > Russel: what's your opinion?
> > 
> > Win,
> > 
> > This series looks fine.  However, looking through the driver, there's
> > a few oddities:
> 
> Indeed, there are likely several other ways in which this driver can be
> improved.  My goal was to get it working on our board first. :)

Feel free to continue improving the driver like Russel suggested. :-)

> So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
> patch tracker.  I suppose #2 and #3 should go through the watchdog tree?

I added patch 2 and 3 to linux-2.6-watchdog-next. You can send #1 to 
Russel's patch tracker.

Kind regards,
Wim.

[PATCH 0/3] SP805 updates for Versatile Express

[Wim Van Sebroeck]

Hi Nick, Russel,

> > > looks good at first glance.
> > > 
> > > Russel: what's your opinion?
> > 
> > Win,
> > 
> > This series looks fine.  However, looking through the driver, there's
> > a few oddities:
> 
> Indeed, there are likely several other ways in which this driver can be
> improved.  My goal was to get it working on our board first. :)

Feel free to continue improving the driver like Russel suggested. :-)

> So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
> patch tracker.  I suppose #2 and #3 should go through the watchdog tree?

I added patch 2 and 3 to linux-2.6-watchdog-next. You can send #1 to 
Russel's patch tracker.

Kind regards,
Wim.

Re: [PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

On 2011-07-22 20:31 +0200, Wim Van Sebroeck wrote:
> Hi Nick, Russel,
> > Indeed, there are likely several other ways in which this driver can be
> > improved.  My goal was to get it working on our board first. :)
> 
> Feel free to continue improving the driver like Russel suggested. :-)

I'll see what I can do :)

> > So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
> > patch tracker.  I suppose #2 and #3 should go through the watchdog tree?
> 
> I added patch 2 and 3 to linux-2.6-watchdog-next. You can send #1 to 
> Russel's patch tracker.

OK, great.  I sent #1 to Russell's patch tracker a couple days ago.

Thanks,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

[PATCH 0/3] SP805 updates for Versatile Express

[Nick Bowler]

On 2011-07-22 20:31 +0200, Wim Van Sebroeck wrote:
> Hi Nick, Russel,
> > Indeed, there are likely several other ways in which this driver can be
> > improved.  My goal was to get it working on our board first. :)
> 
> Feel free to continue improving the driver like Russel suggested. :-)

I'll see what I can do :)

> > So if this series is fine, I'll submit #1 (the V2M patch) to Russel's
> > patch tracker.  I suppose #2 and #3 should go through the watchdog tree?
> 
> I added patch 2 and 3 to linux-2.6-watchdog-next. You can send #1 to 
> Russel's patch tracker.

OK, great.  I sent #1 to Russell's patch tracker a couple days ago.

Thanks,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)