| Message ID | 1353323383-11827-4-git-send-email-sr@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stefan, While I've taken the three other patches, I'm more concerned about this one. On a general basis, I would rather see this code into the machine source file, since it doesn't directly relate to the timer driver. If at some point someone wants to write a proper watchdog driver alongside with the timer driver, I'll be fine with moving the code there, but for now, let's keep it simple. Le 19/11/2012 12:09, Stefan Roese a écrit : > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Cc: Arnd Bergmann <arnd@arndb.de> > --- > arch/arm/mach-sunxi/sunxi.c | 1 + > arch/arm/mach-sunxi/sunxi.h | 2 ++ > drivers/clocksource/sunxi_timer.c | 14 ++++++++++++++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c > index 13d4d96..6b1186c 100644 > --- a/arch/arm/mach-sunxi/sunxi.c > +++ b/arch/arm/mach-sunxi/sunxi.c > @@ -57,5 +57,6 @@ DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)") > .init_irq = sunxi_init_irq, > .handle_irq = sunxi_handle_irq, > .timer = &sunxi_timer, > + .restart = sunxi_restart, > .dt_compat = sunxi_board_dt_compat, > MACHINE_END > diff --git a/arch/arm/mach-sunxi/sunxi.h b/arch/arm/mach-sunxi/sunxi.h > index 33b5871..806c5fd 100644 > --- a/arch/arm/mach-sunxi/sunxi.h > +++ b/arch/arm/mach-sunxi/sunxi.h > @@ -17,4 +17,6 @@ > #define SUNXI_REGS_VIRT_BASE IOMEM(0xf1c00000) > #define SUNXI_REGS_SIZE (SZ_2M + SZ_1M) > > +void sunxi_restart(char mode, const char *cmd); > + > #endif /* __MACH_SUNXI_H */ > diff --git a/drivers/clocksource/sunxi_timer.c b/drivers/clocksource/sunxi_timer.c > index 3c46434..dfbf879 100644 > --- a/drivers/clocksource/sunxi_timer.c > +++ b/drivers/clocksource/sunxi_timer.c > @@ -34,6 +34,8 @@ > #define TIMER0_CTL_ONESHOT (1 << 7) > #define TIMER0_INTVAL_REG 0x14 > #define TIMER0_CNTVAL_REG 0x18 > +#define WATCH_DOG_CTRL_REG 0x90 > +#define WATCH_DOG_MODE_REG 0x94 > > #define TIMER_SCAL 16 > > @@ -103,6 +105,18 @@ static struct of_device_id sunxi_timer_dt_ids[] = { > { .compatible = "allwinner,sunxi-timer" }, > }; > > +void sunxi_restart(char mode, const char *cmd) > +{ > + /* Use watchdog to reset system */ > + > + /* Enable timer and set reset bit */ > + writel(3, timer_base + WATCH_DOG_MODE_REG); > + writel(0xa57 << 1 | 1, timer_base + WATCH_DOG_CTRL_REG); > + > + while(1) > + ; > +} Here, your code looks way more obscure and "complicated" than the one from linux-sunxi found here: https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/arch/arm/mach-sun4i/core.c#L289 Why is that? Thanks, Maxime
On Monday 19 November 2012, Stefan Roese wrote: > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Cc: Arnd Bergmann <arnd@arndb.de> Acked-by: Arnd Bergmann <arnd@arndb.de> > +void sunxi_restart(char mode, const char *cmd) > +{ > + /* Use watchdog to reset system */ > + > + /* Enable timer and set reset bit */ > + writel(3, timer_base + WATCH_DOG_MODE_REG); > + writel(0xa57 << 1 | 1, timer_base + WATCH_DOG_CTRL_REG); > + > + while(1) > + ; > +} > + > static void __init sunxi_timer_init(void) > { > struct device_node *node; We may have to revisit this if we get to the point where timer drivers can be in loadable modules. We can't do that yet, so it's probably better the way you wrote it than something else. Arnd
On 11/19/2012 04:43 PM, Maxime Ripard wrote: > While I've taken the three other patches, I'm more concerned about this one. > > On a general basis, I would rather see this code into the machine source > file, since it doesn't directly relate to the timer driver. If at some > point someone wants to write a proper watchdog driver alongside with the > timer driver, I'll be fine with moving the code there, but for now, > let's keep it simple. I thought about that too. But it would either a) result in code duplication (finding and mapping this timer device node) or b) we would have to make this "timer_base" an ugly global variable so that we can reference it from the platform code. That's why I placed it the timer source. <snip> >> +void sunxi_restart(char mode, const char *cmd) >> +{ >> + /* Use watchdog to reset system */ >> + >> + /* Enable timer and set reset bit */ >> + writel(3, timer_base + WATCH_DOG_MODE_REG); >> + writel(0xa57 << 1 | 1, timer_base + WATCH_DOG_CTRL_REG); >> + >> + while(1) >> + ; >> +} > > Here, your code looks way more obscure and "complicated" than the one > from linux-sunxi found here: > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/arch/arm/mach-sun4i/core.c#L289 > > Why is that? "Way more obscure" sounds a bit exaggerated for 3 lines of code. ;) The delays have been removed as they are not necessary. So its even "cleaner" than the code that you referenced. And the code at linux-sunxi also has incorrect register definitions (CTRL_REG is not 0x94 but 0x90). The main difference is the added write to the real CTRL_REG (0x90) to rearm the wdog. This has been suggested by Henrik Norström, who has done most of the U-Boot work (preparing for mainlining as well). So since Arnd seem to be fine with this patch, I suggest to leave it as is for now. Thanks, Stefan
diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c index 13d4d96..6b1186c 100644 --- a/arch/arm/mach-sunxi/sunxi.c +++ b/arch/arm/mach-sunxi/sunxi.c @@ -57,5 +57,6 @@ DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)") .init_irq = sunxi_init_irq, .handle_irq = sunxi_handle_irq, .timer = &sunxi_timer, + .restart = sunxi_restart, .dt_compat = sunxi_board_dt_compat, MACHINE_END diff --git a/arch/arm/mach-sunxi/sunxi.h b/arch/arm/mach-sunxi/sunxi.h index 33b5871..806c5fd 100644 --- a/arch/arm/mach-sunxi/sunxi.h +++ b/arch/arm/mach-sunxi/sunxi.h @@ -17,4 +17,6 @@ #define SUNXI_REGS_VIRT_BASE IOMEM(0xf1c00000) #define SUNXI_REGS_SIZE (SZ_2M + SZ_1M) +void sunxi_restart(char mode, const char *cmd); + #endif /* __MACH_SUNXI_H */ diff --git a/drivers/clocksource/sunxi_timer.c b/drivers/clocksource/sunxi_timer.c index 3c46434..dfbf879 100644 --- a/drivers/clocksource/sunxi_timer.c +++ b/drivers/clocksource/sunxi_timer.c @@ -34,6 +34,8 @@ #define TIMER0_CTL_ONESHOT (1 << 7) #define TIMER0_INTVAL_REG 0x14 #define TIMER0_CNTVAL_REG 0x18 +#define WATCH_DOG_CTRL_REG 0x90 +#define WATCH_DOG_MODE_REG 0x94 #define TIMER_SCAL 16 @@ -103,6 +105,18 @@ static struct of_device_id sunxi_timer_dt_ids[] = { { .compatible = "allwinner,sunxi-timer" }, }; +void sunxi_restart(char mode, const char *cmd) +{ + /* Use watchdog to reset system */ + + /* Enable timer and set reset bit */ + writel(3, timer_base + WATCH_DOG_MODE_REG); + writel(0xa57 << 1 | 1, timer_base + WATCH_DOG_CTRL_REG); + + while(1) + ; +} + static void __init sunxi_timer_init(void) { struct device_node *node;
Signed-off-by: Stefan Roese <sr@denx.de> Cc: Maxime Ripard <maxime.ripard@free-electrons.com> Cc: Arnd Bergmann <arnd@arndb.de> --- arch/arm/mach-sunxi/sunxi.c | 1 + arch/arm/mach-sunxi/sunxi.h | 2 ++ drivers/clocksource/sunxi_timer.c | 14 ++++++++++++++ 3 files changed, 17 insertions(+)