[PATCH 4/4] ARM: ep93xx: use more reliable CPLD watchdog for reset on ts72xx

[PATCH 4/4] ARM: ep93xx: use more reliable CPLD watchdog for reset on ts72xx

[Ryan Mallon]

On 29/05/11 20:35, 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 |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
> index 6d661fe..4d02ddd 100644
> --- a/arch/arm/mach-ep93xx/include/mach/system.h
> +++ b/arch/arm/mach-ep93xx/include/mach/system.h
> @@ -3,6 +3,10 @@
>   */
>  
>  #include <mach/hardware.h>
> +#ifdef CONFIG_MACH_TS72XX
> +#include <linux/io.h>
> +#include <mach/ts72xx.h>
> +#endif
>  
>  static inline void arch_idle(void)
>  {
> @@ -13,11 +17,22 @@ static inline void arch_reset(char mode, const char *cmd)
>  {
>  	local_irq_disable();
>  
> +#ifdef CONFIG_MACH_TS72XX
> +	/*
> +	 * 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);
> +#endif

Do we need the ifdef here? Isn't the if (board_is_xxx) test enough?

~Ryan

[PATCH 4/4] ARM: ep93xx: use more reliable CPLD watchdog for reset on ts72xx

[Petr Štetiar]

Ryan Mallon <ryan@bluewatersys.com> [2011-05-31 18:47:57]:

> > +#ifdef CONFIG_MACH_TS72XX
> > +	/*
> > +	 * 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);
> > +#endif
> 
> Do we need the ifdef here? Isn't the if (board_is_xxx) test enough?

Yes, sure, that the test is enough. The rationale behind that ifdef is, that I
was just thinking, that it's not necessary to drag-in more code (althought
very small) in the kernel for other ep93xx machines. If it's ok without, I'll
make v2.

-- ynezz

[PATCH 4/4] ARM: ep93xx: use more reliable CPLD watchdog for reset on ts72xx

[H Hartley Sweeten]

On Tuesday, May 31, 2011 2:37 AM, Petr ?tetiar wrote:
>
> Ryan Mallon <ryan@bluewatersys.com> [2011-05-31 18:47:57]:
>
>>> +#ifdef CONFIG_MACH_TS72XX
>>> +	/*
>>> +	 * 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);
>>> +#endif
>> 
>> Do we need the ifdef here? Isn't the if (board_is_xxx) test enough?
>
> Yes, sure, that the test is enough. The rationale behind that ifdef is, that I
> was just thinking, that it's not necessary to drag-in more code (althought
> very small) in the kernel for other ep93xx machines. If it's ok without, I'll
> make v2.

For the non ts-xxxx machines the code will reduce to..

	if (0 || 0 || 0 || 0 || 0) {
		...
	} else {
		...
	}

The compiler should then toss the first block of code.  Net effect, no code
increase.

Please both #ifdef CONFIG_MACH_TS72XX and reorder the #include's.


#include <linux/io.h>

#include <mach/hardware.h>
#include <mach/ts72xx.h>


Regards,
Hartley

[PATCH 3/4] ARM: ep93xx: add model detection for ts-7300 and ts-7400 boards

[H Hartley Sweeten]

On Sunday, May 29, 2011 3:36 AM, Petr ?tetiar wrote:
> Subject: [PATCH 3/4] ARM: ep93xx: add model detection for ts-7300 and ts-7400 boards
> 
> Cc: Ryan Mallon <ryan@bluewatersys.com>
> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
> Signed-off-by: Petr ?tetiar <ynezz@true.cz>
> ---
>  arch/arm/mach-ep93xx/include/mach/ts72xx.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/include/mach/ts72xx.h b/arch/arm/mach-ep93xx/include/mach/ts72xx.h
> index 0eabec6..ee7f875 100644
> --- a/arch/arm/mach-ep93xx/include/mach/ts72xx.h
> +++ b/arch/arm/mach-ep93xx/include/mach/ts72xx.h
> @@ -20,6 +20,8 @@
>  #define TS72XX_MODEL_TS7200		0x00
>  #define TS72XX_MODEL_TS7250		0x01
>  #define TS72XX_MODEL_TS7260		0x02
> +#define TS72XX_MODEL_TS7300		0x03
> +#define TS72XX_MODEL_TS7400		0x04
>  
>  
>  #define TS72XX_OPTIONS_PHYS_BASE	0x22400000
> @@ -66,6 +68,16 @@ static inline int board_is_ts7260(void)
>  	return __raw_readb(TS72XX_MODEL_VIRT_BASE) == TS72XX_MODEL_TS7260;
>  }
>  
> +static inline int board_is_ts7300(void)
> +{
> +	return __raw_readb(TS72XX_MODEL_VIRT_BASE) == TS72XX_MODEL_TS7300;
> +}
> +
> +static inline int board_is_ts7400(void)
> +{
> +	return __raw_readb(TS72XX_MODEL_VIRT_BASE) == TS72XX_MODEL_TS7400;
> +}
> +
>  static inline int is_max197_installed(void)
>  {
>  	return !!(__raw_readb(TS72XX_OPTIONS_VIRT_BASE) &

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

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

[Petr Štetiar]

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 |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
index 6d661fe..8d31c4d 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)
 {
@@ -14,6 +17,15 @@ static inline void arch_reset(char mode, const char *cmd)
 	local_irq_disable();
 
 	/*
+	 * 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);
+	}
+
+	/*
 	 * Set then clear the SWRST bit to initiate a software reset
 	 */
 	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);
-- 
1.7.1

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

[H Hartley Sweeten]

On Tuesday, May 31, 2011 11:34 AM, 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 |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
> index 6d661fe..8d31c4d 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)
>  {
> @@ -14,6 +17,15 @@ static inline void arch_reset(char mode, const char *cmd)
>  	local_irq_disable();
>  
>  	/*
> +	 * 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);
> +	}

Your missing the 'else' here.  Without it the ts-xxxx boards will fiddle with
the CPLD watchdog to do the reset but the SWRST bit might still get toggled if
the reset does not occur quick enough.  This might result in you still getting
the "weird" lockup.

> +
> +	/*
>  	 * Set then clear the SWRST bit to initiate a software reset
>  	 */
>  	ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_SWRST);

Regards,
Hartley

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

[Petr Štetiar]

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)
 		;
-- 
1.7.1

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

[H Hartley Sweeten]

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>

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

[Petr Štetiar]

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

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

[H Hartley Sweeten]

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

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

[Ryan Mallon]

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>

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

[H Hartley Sweeten]

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

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

[Mika Westerberg]

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 :-(

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

[Petr Štetiar]

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

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

[Petr Štetiar]

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: Ryan Mallon <ryan@bluewatersys.com>
Acked-by: H Hartley Sweeten <hsweeten@visionengravers.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..67ec430 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);
+	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
+	    board_is_ts7300() || board_is_ts7400()) {
+		/* We use more reliable CPLD watchdog to perform the reset */
+		__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)
 		;
-- 
1.7.1

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

[Petr Štetiar]

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

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

[Olof Johansson]

Hi,

On Fri, Jun 3, 2011 at 11:28 AM, Petr ?tetiar <ynezz@true.cz> 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: Ryan Mallon <ryan@bluewatersys.com>
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.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..67ec430 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);
> + ? ? ? if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> + ? ? ? ? ? board_is_ts7300() || board_is_ts7400()) {
> + ? ? ? ? ? ? ? /* We use more reliable CPLD watchdog to perform the reset */
> + ? ? ? ? ? ? ? __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);
> + ? ? ? }

It would be nicer to introduce a function pointer here, that's filled
in by the ts72xx.c machine_init function but NULL by default:


extern void (*ep93xx_reset)(void);
...

arch_reset():

   local_irq_disable();

   if (ep93xx_reset) {
      ep93xx_reset();
   } else {
      ... current SWRST code ...
   }
...



Otherwise, other boards would add another if statement, any new ts72xx
board would need to modify the soc-common header, etc.


-Olof

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

[Petr Štetiar]

Olof Johansson <olof@lixom.net> [2011-06-03 11:43:27]:

Hi,

> It would be nicer to introduce a function pointer here, that's filled
> in by the ts72xx.c machine_init function but NULL by default:

yes, I didn't liked the modification of the shared machine code either and I
personally dislike the ifdefs also, but

> Otherwise, other boards would add another if statement, any new ts72xx
> board would need to modify the soc-common header, etc.

this is very unlikely to happen. The TS's ep93xx based products seems to be
EOL, they'll just sell them until they've parts/SOC available.

I'll leave the decision up to the maintainers.

-- ynezz

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

[Mika Westerberg]

On Fri, Jun 03, 2011 at 08:28:05PM +0200, Petr ?tetiar wrote:

> @@ -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);
> +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> +	    board_is_ts7300() || board_is_ts7400()) {
> +		/* We use more reliable CPLD watchdog to perform the reset */
> +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
> +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);

I just noticed that you are accessing the registers via *physical* address. It
currently works because arm_machine_restart() sets up 1:1 mappings in place of
userspace before arch_reset() gets called.

This might cause some problems as the register accesses are cached, or does it
make a difference in ARM920?

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

[Petr Štetiar]

Mika Westerberg <mika.westerberg@iki.fi> [2011-06-05 12:54:50]:

> > +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> > +	    board_is_ts7300() || board_is_ts7400()) {
> > +		/* We use more reliable CPLD watchdog to perform the reset */
> > +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
> > +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
> 
> I just noticed that you are accessing the registers via *physical* address. It
> currently works because arm_machine_restart() sets up 1:1 mappings in place of
> userspace before arch_reset() gets called.

Setups the 1:1 mappings, clean+invalidate cache, turns off caching and flush
the cache.

> This might cause some problems as the register accesses are cached, or does it
> make a difference in ARM920?

Caching is disabled by the caller, isn't it?

-- ynezz

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

[Mika Westerberg]

On Sun, Jun 05, 2011 at 06:07:34PM +0200, Petr ?tetiar wrote:
> Mika Westerberg <mika.westerberg@iki.fi> [2011-06-05 12:54:50]:
> 
> > > +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> > > +	    board_is_ts7300() || board_is_ts7400()) {
> > > +		/* We use more reliable CPLD watchdog to perform the reset */
> > > +		__raw_writeb(0x5, TS72XX_WDT_FEED_PHYS_BASE);
> > > +		__raw_writeb(0x1, TS72XX_WDT_CONTROL_PHYS_BASE);
> > 
> > I just noticed that you are accessing the registers via *physical* address. It
> > currently works because arm_machine_restart() sets up 1:1 mappings in place of
> > userspace before arch_reset() gets called.
> 
> Setups the 1:1 mappings, clean+invalidate cache, turns off caching and flush
> the cache.
> 
> > This might cause some problems as the register accesses are cached, or does it
> > make a difference in ARM920?
> 
> Caching is disabled by the caller, isn't it?

Yes.

I'm just wondering whether this should be done via valid mapping? Note also
that __raw_writeb() takes void __iomem * which is different that the value you
are passing.

Anyway, I tried the patch on my TS-7260 and I'm still able to reset the board
so you can add my

Tested-by: Mika Westerberg <mika.westerberg@iki.fi>

if you like.

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

[H Hartley Sweeten]

On Friday, June 03, 2011 11:28 AM, 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: Ryan Mallon <ryan@bluewatersys.com>
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.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..67ec430 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);
> +	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
> +	    board_is_ts7300() || board_is_ts7400()) {
> +		/* We use more reliable CPLD watchdog to perform the reset */
> +		__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)
>  		;

Petr,

This patch is still in Russell's patch tracker.  I just realized a possible
problem with it.

If any other ep93xx machine is selected along with MACH_TS72XX and the kernel
is booted on a non-ts72xx machine, the static mapping for the ts72xx CPLD will
not be done.  I believe this will cause a problem when doing the boad_is_*
calls due to the:

	__raw_readb(TS72XX_MODEL_VIRT_BASE)

I think the best solution is to remove the #include <mach/ts72xx.h> from this
patch and change this:

+	if (board_is_ts7200() || board_is_ts7250() || board_is_ts7260() ||
+	    board_is_ts7300() || board_is_ts7400()) {

To:

+	if (machine_is_ts72xx())

This will correctly evaluate is the kernel is booted on a MACH_TYPE_TS72XX
system.

Can you please update and test the patch?

Regards,
Hartley