| Message ID | 1307125685-5110-1-git-send-email-ynezz@true.cz |
|---|---|
| State | New |
| Headers | show |
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
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
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?
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
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.
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 --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) ;