diff mbox

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

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

Commit Message

Petr Štetiar June 3, 2011, 6:28 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: 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(-)

Comments

Olof Johansson June 3, 2011, 6:43 p.m. UTC | #1
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
Petr Štetiar June 5, 2011, 7:51 a.m. UTC | #2
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
Mika Westerberg June 5, 2011, 9:54 a.m. UTC | #3
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?
Petr Štetiar June 5, 2011, 4:07 p.m. UTC | #4
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
Mika Westerberg June 5, 2011, 6:18 p.m. UTC | #5
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.
Hartley Sweeten Aug. 10, 2011, 5:45 p.m. UTC | #6
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
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..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)
 		;