diff mbox

[v3] ARM: ep93xx: use more reliable CPLD watchdog for reset on ts72xx

Message ID 1306868578-3883-1-git-send-email-ynezz@true.cz (mailing list archive)
State New, archived
Headers show

Commit Message

Petr Štetiar May 31, 2011, 7:02 p.m. UTC
On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
available, so use this one to reset the board instead of the soft reset in
CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
so far.

Cc: Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <ryan@bluewatersys.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 arch/arm/mach-ep93xx/include/mach/system.h |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

Comments

Hartley Sweeten May 31, 2011, 7:24 p.m. UTC | #1
On Tuesday, May 31, 2011 12:03 PM, Petr Štetiar wrote:
> On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
> available, so use this one to reset the board instead of the soft reset in
> CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
> while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
> so far.
> 
> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ryan Mallon <ryan@bluewatersys.com>
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  arch/arm/mach-ep93xx/include/mach/system.h |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
> index 6d661fe..2969786 100644
> --- a/arch/arm/mach-ep93xx/include/mach/system.h
> +++ b/arch/arm/mach-ep93xx/include/mach/system.h
> @@ -2,7 +2,10 @@
>   * arch/arm/mach-ep93xx/include/mach/system.h
>   */
>  
> +#include <linux/io.h>
> +
>  #include <mach/hardware.h>
> +#include <mach/ts72xx.h>
>  
>  static inline void arch_idle(void)
>  {
> @@ -13,11 +16,16 @@ static inline void arch_reset(char mode, const char *cmd)
>  {
>  	local_irq_disable();
>  
> -	/*
> -	 * Set then clear the SWRST bit to initiate a software reset
> -	 */
> -	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
> -	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
> +	/* It's more reliable to use CPLD watchdog to perform the reset */
> +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> +	    board_is_ts7300() || board_is_ts7400()) {
> +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
> +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
> +	} else {
> +		/* Set then clear the SWRST bit to initiate a software reset */
> +		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
> +		ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
> +	}
>  
>  	while (1)
>  		;

That looks better.

I pulled out the ts-7200 manual to verify the watchdog reset.  Your feeding
the watchdog then changing the timeout period to 250ms.  Without the "else"
the syscon would have tried to reset the board when the SWRST bit is toggled.

I have no way of testing this patch (no ts boards) but as long as Ryan has no
objections you have my:

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Petr Štetiar May 31, 2011, 7:55 p.m. UTC | #2
H Hartley Sweeten <hartleys@visionengravers.com> [2011-05-31 14:24:04]:

> I pulled out the ts-7200 manual to verify the watchdog reset.  Your feeding
> the watchdog then changing the timeout period to 250ms.  Without the "else"
> the syscon would have tried to reset the board when the SWRST bit is toggled.

Yep, that missing "else" was a good catch, thanks.

This feed first then set behaviour is "logic", but it's correct (250ms is the
smallest possible value), from that manual:

   In order to load the WDT Control register, the WDT must first be “fed”, and
   then within 30 uS, the WDT control register must be written. Writes to this
   register without first doing a “WDT feed”, have no affect.

(Please don't laugh at that 30us constraint).

> I have no way of testing this patch (no ts boards) but as long as Ryan has no
> objections you have my:

I'm using it[1] since 2.6.19-rc6-git10 (maybe earlier). Funny, that I didn't
forget to add that "else" in this old patch.

And as I see it, you and few other guys really deserve some free TS boards for
sure (at least the damn board as a thank you), but Lennert could probably tell
you more about their "we only leech the work from the comunity, but don't give
anything back" attitude...

1. http://ynezz.ibawizard.net/ts72xx/ts72xx_cpld_wdt_reset.diff.gz

> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks.

-- ynezz
Hartley Sweeten May 31, 2011, 8:02 p.m. UTC | #3
On Tuesday, May 31, 2011 12:55 PM, Petr Štetiar wrote:
> H Hartley Sweeten <hartleys@visionengravers.com> [2011-05-31 14:24:04]:
>
>> I pulled out the ts-7200 manual to verify the watchdog reset.  Your feeding
>> the watchdog then changing the timeout period to 250ms.  Without the "else"
>> the syscon would have tried to reset the board when the SWRST bit is toggled.
>
> Yep, that missing "else" was a good catch, thanks.
>
> This feed first then set behaviour is "logic", but it's correct (250ms is the
> smallest possible value), from that manual:
>
>    In order to load the WDT Control register, the WDT must first be “fed”, and
>    then within 30 uS, the WDT control register must be written. Writes to this
>    register without first doing a “WDT feed”, have no affect.
>
> (Please don't laugh at that 30us constraint).

;-)

>> I have no way of testing this patch (no ts boards) but as long as Ryan has no
>> objections you have my:
>
> I'm using it[1] since 2.6.19-rc6-git10 (maybe earlier). Funny, that I didn't
> forget to add that "else" in this old patch.
>
> And as I see it, you and few other guys really deserve some free TS boards for
> sure (at least the damn board as a thank you), but Lennert could probably tell
> you more about their "we only leech the work from the comunity, but don't give
> anything back" attitude...

Yah, the ep93xx port has some bad history.  It's old history, lets move on...

If they want to give me one I'll be happy to take it.  I can even go pick it up,
they are only 44 miles away from my on the other side of the valley....

Regards,
Hartley
Ryan Mallon May 31, 2011, 9:42 p.m. UTC | #4
On 01/06/11 05:24, H Hartley Sweeten wrote:
> On Tuesday, May 31, 2011 12:03 PM, Petr Štetiar wrote:
>> On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
>> available, so use this one to reset the board instead of the soft reset in
>> CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
>> while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
>> so far.
>>
>> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: Ryan Mallon <ryan@bluewatersys.com>
>> Signed-off-by: Petr Štetiar <ynezz@true.cz>
>> ---
>>  arch/arm/mach-ep93xx/include/mach/system.h |   18 +++++++++++++-----
>>  1 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
>> index 6d661fe..2969786 100644
>> --- a/arch/arm/mach-ep93xx/include/mach/system.h
>> +++ b/arch/arm/mach-ep93xx/include/mach/system.h
>> @@ -2,7 +2,10 @@
>>   * arch/arm/mach-ep93xx/include/mach/system.h
>>   */
>>  
>> +#include <linux/io.h>
>> +
>>  #include <mach/hardware.h>
>> +#include <mach/ts72xx.h>
>>  
>>  static inline void arch_idle(void)
>>  {
>> @@ -13,11 +16,16 @@ static inline void arch_reset(char mode, const char *cmd)
>>  {
>>  	local_irq_disable();
>>  
>> -	/*
>> -	 * Set then clear the SWRST bit to initiate a software reset
>> -	 */
>> -	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>> -	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>> +	/* It's more reliable to use CPLD watchdog to perform the reset */
>> +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
>> +	    board_is_ts7300() || board_is_ts7400()) {
>> +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
>> +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
>> +	} else {
>> +		/* Set then clear the SWRST bit to initiate a software reset */
>> +		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>> +		ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>> +	}
>>  
>>  	while (1)
>>  		;
> 
> That looks better.
> 
> I pulled out the ts-7200 manual to verify the watchdog reset.  Your feeding
> the watchdog then changing the timeout period to 250ms.  Without the "else"
> the syscon would have tried to reset the board when the SWRST bit is toggled.
> 
> I have no way of testing this patch (no ts boards) but as long as Ryan has no
> objections you have my:

My only (nitpicky) complaint is to move the CPLD comment inside the if
block so that it is more clear that it relates to the ts7xxx boards.
Otherwise the patch is fine.

> 
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Hartley Sweeten May 31, 2011, 10:41 p.m. UTC | #5
On Tuesday, May 31, 2011 2:43 PM, Ryan Mallon wrote:
> On 01/06/11 05:24, H Hartley Sweeten wrote:
>> On Tuesday, May 31, 2011 12:03 PM, Petr Štetiar wrote:
>>> On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
>>> available, so use this one to reset the board instead of the soft reset in
>>> CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
>>> while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
>>> so far.
>>>
>>> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
>>> Cc: Ryan Mallon <ryan@bluewatersys.com>
>>> Signed-off-by: Petr Štetiar <ynezz@true.cz>
>>> ---
>>>  arch/arm/mach-ep93xx/include/mach/system.h |   18 +++++++++++++-----
>>>  1 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
>>> index 6d661fe..2969786 100644
>>> --- a/arch/arm/mach-ep93xx/include/mach/system.h
>>> +++ b/arch/arm/mach-ep93xx/include/mach/system.h
>>> @@ -2,7 +2,10 @@
>>>   * arch/arm/mach-ep93xx/include/mach/system.h
>>>   */
>>>  
>>> +#include <linux/io.h>
>>> +
>>>  #include <mach/hardware.h>
>>> +#include <mach/ts72xx.h>
>>>  
>>>  static inline void arch_idle(void)
>>>  {
>>> @@ -13,11 +16,16 @@ static inline void arch_reset(char mode, const char *cmd)
>>>  {
>>>  	local_irq_disable();
>>>  
>>> -	/*
>>> -	 * Set then clear the SWRST bit to initiate a software reset
>>> -	 */
>>> -	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>>> -	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>>> +	/* It's more reliable to use CPLD watchdog to perform the reset */
>>> +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
>>> +	    board_is_ts7300() || board_is_ts7400()) {
>>> +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
>>> +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
>>> +	} else {
>>> +		/* Set then clear the SWRST bit to initiate a software reset */
>>> +		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>>> +		ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
>>> +	}
>>>  
>>>  	while (1)
>>>  		;
>> 
>> That looks better.
>> 
>> I pulled out the ts-7200 manual to verify the watchdog reset.  Your feeding
>> the watchdog then changing the timeout period to 250ms.  Without the "else"
>> the syscon would have tried to reset the board when the SWRST bit is toggled.
>> 
>> I have no way of testing this patch (no ts boards) but as long as Ryan has no
>> objections you have my:
>
> My only (nitpicky) complaint is to move the CPLD comment inside the if
> block so that it is more clear that it relates to the ts7xxx boards.
> Otherwise the patch is fine.

Valid point.  The CPLD watchdog reset is a ts-xxxx specific thing.

Petr, please move the comment as Ryan suggests.

Regards,
Hartley
Mika Westerberg June 2, 2011, 9:54 a.m. UTC | #6
On Tue, May 31, 2011 at 09:02:58PM +0200, Petr Štetiar wrote:
> On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
> available, so use this one to reset the board instead of the soft reset in
> CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
> while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
> so far.

Where can I find the whole series? I was going to test this on my TS-7260 but
I only found this patch from my inbox and it doesn't apply without the
previous one :-(
Petr Štetiar June 2, 2011, 10:56 a.m. UTC | #7
Mika Westerberg <mika.westerberg@iki.fi> [2011-06-02 12:54:55]:

> Where can I find the whole series? I was going to test this on my TS-7260 but
> I only found this patch from my inbox and it doesn't apply without the
> previous one :-(

I'll push that wip branch on github later today, and will give you the link. Thanks.

-- ynezz
Petr Štetiar June 3, 2011, 6:34 p.m. UTC | #8
Mika Westerberg <mika.westerberg@iki.fi> [2011-06-02 12:54:55]:

> On Tue, May 31, 2011 at 09:02:58PM +0200, Petr Štetiar wrote:
> > On all ep93xx based boards from Technologic Systems, there's CPLD watchdog
> > available, so use this one to reset the board instead of the soft reset in
> > CPU.  I've seen some weird lockups with the soft reset on ep93xx in the past,
> > while the reset via CPLD watchdog seems to be rock solid (tm) and works fine
> > so far.
> 
> Where can I find the whole series? I was going to test this on my TS-7260 but
> I only found this patch from my inbox and it doesn't apply without the
> previous one :-(

Please clone git://github.com/ynezz/linux-2.6.git, there's branch ts72xx-wip
and you need to cherry-pick this two commits:

d3c3c086b21b11063efc27c30acb08c03b9fbe23
cd23b7132d441a0bc23e74ef04ad74c4616e13c9

-- ynezz
diff mbox

Patch

diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
index 6d661fe..2969786 100644
--- a/arch/arm/mach-ep93xx/include/mach/system.h
+++ b/arch/arm/mach-ep93xx/include/mach/system.h
@@ -2,7 +2,10 @@ 
  * arch/arm/mach-ep93xx/include/mach/system.h
  */
 
+#include <linux/io.h>
+
 #include <mach/hardware.h>
+#include <mach/ts72xx.h>
 
 static inline void arch_idle(void)
 {
@@ -13,11 +16,16 @@  static inline void arch_reset(char mode, const char *cmd)
 {
 	local_irq_disable();
 
-	/*
-	 * Set then clear the SWRST bit to initiate a software reset
-	 */
-	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
-	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
+	/* It's more reliable to use CPLD watchdog to perform the reset */
+	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
+	    board_is_ts7300() || board_is_ts7400()) {
+		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
+		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
+	} else {
+		/* Set then clear the SWRST bit to initiate a software reset */
+		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
+		ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_SWRST);
+	}
 
 	while (1)
 		;