| Message ID | 1390295561-3466-4-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia <ezequiel.garcia@free-electrons.com> wrote: > Since the timer control register is shared with the clocksource driver, > use the recently introduced atomic_io_clear_set() to access such register. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/watchdog/orion_wdt.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c > index f7722a4..cf64510 100644 > --- a/drivers/watchdog/orion_wdt.c > +++ b/drivers/watchdog/orion_wdt.c > @@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev) > > static int orion_wdt_start(struct watchdog_device *wdt_dev) > { > - u32 reg; > - > spin_lock(&wdt_lock); Shouldn't this spin_lock be dropped now? Regards, Fabio Estevam
On Tue, Jan 21, 2014 at 07:19:52AM -0200, Fabio Estevam wrote: > On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia > <ezequiel.garcia@free-electrons.com> wrote: > > Since the timer control register is shared with the clocksource driver, > > use the recently introduced atomic_io_clear_set() to access such register. > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > drivers/watchdog/orion_wdt.c | 20 ++++---------------- > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c > > index f7722a4..cf64510 100644 > > --- a/drivers/watchdog/orion_wdt.c > > +++ b/drivers/watchdog/orion_wdt.c > > @@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev) > > > > static int orion_wdt_start(struct watchdog_device *wdt_dev) > > { > > - u32 reg; > > - > > spin_lock(&wdt_lock); > > Shouldn't this spin_lock be dropped now? > Hm... yes. The watchdog core uses a mutex to serialize all the watchdog hooks: start, stop, ping, set_timeout, etc. So it seems you're right, the spinlock should be dropped entirely as I see no need for it. Thanks for the feedback,
On Tuesday 21 January 2014 06:12:29 Ezequiel Garcia wrote: > writel(~WDT_INT_REQ, BRIDGE_CAUSE); > > /* Enable watchdog timer */ > - reg = readl(wdt_reg + TIMER_CTRL); > - reg |= WDT_EN; > - writel(reg, wdt_reg + TIMER_CTRL); > + atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN); As mentioned in my comment for patch 2, it seems that the exported orion_timer_ctrl_clrset() function would be more appropriate for this. > /* Enable reset on watchdog */ > - reg = readl(RSTOUTn_MASK); > - reg |= WDT_RESET_OUT_EN; > - writel(reg, RSTOUTn_MASK); > + atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN); And this register already has an abstraction in arch/arm/mach-mvebu/system-controller.c. I would prefer to find a way to generalize that. I can see multiple ways to do that: * Turn the "marvell,armada-370-xp-system-controller" device into a "syscon" device that can be used through regmap from other parts of the kernel. * Move arch/arm/mach-mvebu/system-controller.c into drivers/soc (we don't have this directory yet, but have discussed it in the past) to export soc-specific functions from there. * Move all of the system-controller implementation into the wdt driver and require that this driver be enabled in order to do regular system reset. The system-controller can register itself as a reboot hook, so you can remove the ".restart = mvebu_restart" part from mach-mvebu. Arnd
On Tue, Jan 21, 2014 at 10:54:37AM +0100, Arnd Bergmann wrote: > On Tuesday 21 January 2014 06:12:29 Ezequiel Garcia wrote: > > writel(~WDT_INT_REQ, BRIDGE_CAUSE); > > > > /* Enable watchdog timer */ > > - reg = readl(wdt_reg + TIMER_CTRL); > > - reg |= WDT_EN; > > - writel(reg, wdt_reg + TIMER_CTRL); > > + atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN); > > As mentioned in my comment for patch 2, it seems that the exported > orion_timer_ctrl_clrset() function would be more appropriate for this. Hi Arnd As Sebastian points out, Armada 370 XP does not have this function. > > > /* Enable reset on watchdog */ > > - reg = readl(RSTOUTn_MASK); > > - reg |= WDT_RESET_OUT_EN; > > - writel(reg, RSTOUTn_MASK); > > + atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN); > > And this register already has an abstraction in > arch/arm/mach-mvebu/system-controller.c. I would prefer to find a way > to generalize that. I can see multiple ways to do that: Please be aware that arch/arm/mach-mvebu/system-controller.c is currently DT only. We need a solution which works for both with DT and without DT, since orion5x and mv78xx0 none DT boards are not going away soon. Andrew
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c index f7722a4..cf64510 100644 --- a/drivers/watchdog/orion_wdt.c +++ b/drivers/watchdog/orion_wdt.c @@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev) static int orion_wdt_start(struct watchdog_device *wdt_dev) { - u32 reg; - spin_lock(&wdt_lock); /* Set watchdog duration */ @@ -72,14 +70,10 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev) writel(~WDT_INT_REQ, BRIDGE_CAUSE); /* Enable watchdog timer */ - reg = readl(wdt_reg + TIMER_CTRL); - reg |= WDT_EN; - writel(reg, wdt_reg + TIMER_CTRL); + atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN); /* Enable reset on watchdog */ - reg = readl(RSTOUTn_MASK); - reg |= WDT_RESET_OUT_EN; - writel(reg, RSTOUTn_MASK); + atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN); spin_unlock(&wdt_lock); return 0; @@ -87,19 +81,13 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev) static int orion_wdt_stop(struct watchdog_device *wdt_dev) { - u32 reg; - spin_lock(&wdt_lock); /* Disable reset on watchdog */ - reg = readl(RSTOUTn_MASK); - reg &= ~WDT_RESET_OUT_EN; - writel(reg, RSTOUTn_MASK); + atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0); /* Disable watchdog timer */ - reg = readl(wdt_reg + TIMER_CTRL); - reg &= ~WDT_EN; - writel(reg, wdt_reg + TIMER_CTRL); + atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0); spin_unlock(&wdt_lock); return 0;
Since the timer control register is shared with the clocksource driver, use the recently introduced atomic_io_clear_set() to access such register. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/watchdog/orion_wdt.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)