* [PATCH 0/4] watchdog: sama5d4: fix issues
@ 2017-03-02 17:31 Alexandre Belloni
2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Alexandre Belloni @ 2017-03-02 17:31 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog,
linux-arm-kernel, linux-kernel, Alexandre Belloni
Hi,
This is a rework of how the watchdog is getting programmed. Currently,
there are multiple issue that have the same symptoms: the watchdog is
unexpectidly resetting the SoCs when configuring it.
The first issue was how WDDIS was handled. To sum it up, the watchdog
has to be configured when removing WDDIS instead of first removing it
then configuring it. It is solved by only configuring the IP when
enabling the watchdog.
Note that there were no issue updating the timeout with the watchdog
running.
The second issue is how the write are synchronized inside the IP. The
datasheet state that iit is necessary to wait 3 slow clock periods
between a write to CR and subsequents writes to CR and MR. This was not
done in the driver. Also, it apears it is necessary to wait the same
amount of time between a write to MR and a write to CR or MR.
Finally, a simplification of the probe is done and a comment is added in
the resume function to explain why the reset may be delayed after a
suspend/resume cycle (but it will still happen).
Before the series, the watchdog would reset the SoC after a few
configurations. Now, I've lett a test progam run that managed to do more
than 1 000 000 configurations with the watchdog enabled and the same
while disabled.
Alexandre Belloni (4):
watchdog: sama5d4: fix WDDIS handling
watchdog: sama5d4: fix race condition
watchodg: sama5d4: simplify probe
watchdog: sama5d4: Add comment explaining what happens on resume
drivers/watchdog/sama5d4_wdt.c | 96 ++++++++++++++++++++++++++++++------------
1 file changed, 68 insertions(+), 28 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling 2017-03-02 17:31 [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni @ 2017-03-02 17:31 ` Alexandre Belloni 2017-03-04 15:04 ` Guenter Roeck 2017-03-07 2:05 ` Wenyou.Yang 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Alexandre Belloni @ 2017-03-02 17:31 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel, Alexandre Belloni The datasheet states: "When setting the WDDIS bit, and while it is set, the fields WDV and WDD must not be modified." Because the whole configuration is already cached inside .mr, wait for the user to enable the watchdog to configure it so it is enabled and configured at the same time (what the IP is actually expecting). When the watchdog is already enabled, it is not an issue to reconfigure it. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index f709962018ac..5cee20caca78 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS)) + #define wdt_read(wdt, field) \ readl_relaxed((wdt)->reg_base + (field)) @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd, wdt->mr &= ~AT91_WDT_WDD; wdt->mr |= AT91_WDT_SET_WDV(value); wdt->mr |= AT91_WDT_SET_WDD(value); - wdt_write(wdt, AT91_WDT_MR, wdt->mr); + + /* + * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When + * setting the WDDIS bit, and while it is set, the fields WDV and WDD + * must not be modified. + * If the watchdog is enabled, then the timeout can be updated. Else, + * wait that the user enables it. + */ + if (wdt_enabled) + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS); wdd->timeout = timeout; @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) { - struct watchdog_device *wdd = &wdt->wdd; - u32 value = WDT_SEC2TICKS(wdd->timeout); u32 reg; - /* - * Because the fields WDV and WDD must not be modified when the WDDIS - * bit is set, so clear the WDDIS bit before writing the WDT_MR. + * When booting and resuming, the bootloader may have changed the + * watchdog configuration. + * If the watchdog is already running, we can safely update it. + * Else, we have to disable it properly. */ - reg = wdt_read(wdt, AT91_WDT_MR); - reg &= ~AT91_WDT_WDDIS; - wdt_write(wdt, AT91_WDT_MR, reg); - - wdt->mr |= AT91_WDT_SET_WDD(value); - wdt->mr |= AT91_WDT_SET_WDV(value); - - wdt_write(wdt, AT91_WDT_MR, wdt->mr); - + if (wdt_enabled) { + wdt_write(wdt, AT91_WDT_MR, wdt->mr); + } else { + reg = wdt_read(wdt, AT91_WDT_MR); + if (!(reg & AT91_WDT_WDDIS)) + wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); + } return 0; } @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) struct resource *res; void __iomem *regs; u32 irq = 0; + u32 timeout; int ret; wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); @@ -221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) return ret; } + timeout = WDT_SEC2TICKS(wdd->timeout); + + wdt->mr |= AT91_WDT_SET_WDD(timeout); + wdt->mr |= AT91_WDT_SET_WDV(timeout); + ret = sama5d4_wdt_init(wdt); if (ret) return ret; @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev) { struct sama5d4_wdt *wdt = dev_get_drvdata(dev); - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS); - if (wdt->mr & AT91_WDT_WDDIS) - wdt_write(wdt, AT91_WDT_MR, wdt->mr); + sama5d4_wdt_init(wdt); return 0; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling 2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni @ 2017-03-04 15:04 ` Guenter Roeck 2017-03-07 2:05 ` Wenyou.Yang 1 sibling, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-03-04 15:04 UTC (permalink / raw) To: Alexandre Belloni Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On 03/02/2017 09:31 AM, Alexandre Belloni wrote: > The datasheet states: "When setting the WDDIS bit, and while it is set, the > fields WDV and WDD must not be modified." > > Because the whole configuration is already cached inside .mr, wait for the > user to enable the watchdog to configure it so it is enabled and configured > at the same time (what the IP is actually expecting). > > When the watchdog is already enabled, it is not an issue to reconfigure it. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 18 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index f709962018ac..5cee20caca78 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout, > "Watchdog cannot be stopped once started (default=" > __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS)) > + > #define wdt_read(wdt, field) \ > readl_relaxed((wdt)->reg_base + (field)) > > @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd, > wdt->mr &= ~AT91_WDT_WDD; > wdt->mr |= AT91_WDT_SET_WDV(value); > wdt->mr |= AT91_WDT_SET_WDD(value); > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + > + /* > + * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: When > + * setting the WDDIS bit, and while it is set, the fields WDV and WDD > + * must not be modified. > + * If the watchdog is enabled, then the timeout can be updated. Else, > + * wait that the user enables it. > + */ > + if (wdt_enabled) > + wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS); > > wdd->timeout = timeout; > > @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt) > > static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > { > - struct watchdog_device *wdd = &wdt->wdd; > - u32 value = WDT_SEC2TICKS(wdd->timeout); > u32 reg; > - > /* > - * Because the fields WDV and WDD must not be modified when the WDDIS > - * bit is set, so clear the WDDIS bit before writing the WDT_MR. > + * When booting and resuming, the bootloader may have changed the > + * watchdog configuration. > + * If the watchdog is already running, we can safely update it. > + * Else, we have to disable it properly. > */ > - reg = wdt_read(wdt, AT91_WDT_MR); > - reg &= ~AT91_WDT_WDDIS; > - wdt_write(wdt, AT91_WDT_MR, reg); > - > - wdt->mr |= AT91_WDT_SET_WDD(value); > - wdt->mr |= AT91_WDT_SET_WDV(value); > - > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > - > + if (wdt_enabled) { > + wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + } else { > + reg = wdt_read(wdt, AT91_WDT_MR); > + if (!(reg & AT91_WDT_WDDIS)) > + wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); > + } > return 0; > } > > @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > struct resource *res; > void __iomem *regs; > u32 irq = 0; > + u32 timeout; > int ret; > > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > @@ -221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > return ret; > } > > + timeout = WDT_SEC2TICKS(wdd->timeout); > + > + wdt->mr |= AT91_WDT_SET_WDD(timeout); > + wdt->mr |= AT91_WDT_SET_WDV(timeout); > + > ret = sama5d4_wdt_init(wdt); > if (ret) > return ret; > @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev) > { > struct sama5d4_wdt *wdt = dev_get_drvdata(dev); > > - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS); > - if (wdt->mr & AT91_WDT_WDDIS) > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + sama5d4_wdt_init(wdt); > > return 0; > } > ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling 2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni 2017-03-04 15:04 ` Guenter Roeck @ 2017-03-07 2:05 ` Wenyou.Yang 1 sibling, 0 replies; 17+ messages in thread From: Wenyou.Yang @ 2017-03-07 2:05 UTC (permalink / raw) To: alexandre.belloni, linux Cc: wim, Nicolas.Ferre, linux-watchdog, linux-arm-kernel, linux-kernel > -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > Sent: 2017年3月3日 1:31 > To: Guenter Roeck <linux@roeck-us.net> > Cc: Wim Van Sebroeck <wim@iguana.be>; Nicolas Ferre - M43238 > <Nicolas.Ferre@microchip.com>; Wenyou Yang - A41535 > <Wenyou.Yang@microchip.com>; linux-watchdog@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni > <alexandre.belloni@free-electrons.com> > Subject: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling > > The datasheet states: "When setting the WDDIS bit, and while it is set, the fields > WDV and WDD must not be modified." > > Because the whole configuration is already cached inside .mr, wait for the user to > enable the watchdog to configure it so it is enabled and configured at the same > time (what the IP is actually expecting). > > When the watchdog is already enabled, it is not an issue to reconfigure it. Indeed, thanks. Acked-by: Wenyou.Yang <wenyou.yang@microchip.com> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/watchdog/sama5d4_wdt.c | 48 ++++++++++++++++++++++++++---------- > ------ > 1 file changed, 30 insertions(+), 18 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index f709962018ac..5cee20caca78 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout, > "Watchdog cannot be stopped once started (default=" > __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS)) > + > #define wdt_read(wdt, field) \ > readl_relaxed((wdt)->reg_base + (field)) > > @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct > watchdog_device *wdd, > wdt->mr &= ~AT91_WDT_WDD; > wdt->mr |= AT91_WDT_SET_WDV(value); > wdt->mr |= AT91_WDT_SET_WDD(value); > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + > + /* > + * WDDIS has to be 0 when updating WDD/WDV. The datasheet states: > When > + * setting the WDDIS bit, and while it is set, the fields WDV and WDD > + * must not be modified. > + * If the watchdog is enabled, then the timeout can be updated. Else, > + * wait that the user enables it. > + */ > + if (wdt_enabled) > + wdt_write(wdt, AT91_WDT_MR, wdt->mr & > ~AT91_WDT_WDDIS); > > wdd->timeout = timeout; > > @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np, > struct sama5d4_wdt *wdt) > > static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) { > - struct watchdog_device *wdd = &wdt->wdd; > - u32 value = WDT_SEC2TICKS(wdd->timeout); > u32 reg; > - > /* > - * Because the fields WDV and WDD must not be modified when the > WDDIS > - * bit is set, so clear the WDDIS bit before writing the WDT_MR. > + * When booting and resuming, the bootloader may have changed the > + * watchdog configuration. > + * If the watchdog is already running, we can safely update it. > + * Else, we have to disable it properly. > */ > - reg = wdt_read(wdt, AT91_WDT_MR); > - reg &= ~AT91_WDT_WDDIS; > - wdt_write(wdt, AT91_WDT_MR, reg); > - > - wdt->mr |= AT91_WDT_SET_WDD(value); > - wdt->mr |= AT91_WDT_SET_WDV(value); > - > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > - > + if (wdt_enabled) { > + wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + } else { > + reg = wdt_read(wdt, AT91_WDT_MR); > + if (!(reg & AT91_WDT_WDDIS)) > + wdt_write(wdt, AT91_WDT_MR, reg | > AT91_WDT_WDDIS); > + } > return 0; > } > > @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device > *pdev) > struct resource *res; > void __iomem *regs; > u32 irq = 0; > + u32 timeout; > int ret; > > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); @@ - > 221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > return ret; > } > > + timeout = WDT_SEC2TICKS(wdd->timeout); > + > + wdt->mr |= AT91_WDT_SET_WDD(timeout); > + wdt->mr |= AT91_WDT_SET_WDV(timeout); > + > ret = sama5d4_wdt_init(wdt); > if (ret) > return ret; > @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev) { > struct sama5d4_wdt *wdt = dev_get_drvdata(dev); > > - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS); > - if (wdt->mr & AT91_WDT_WDDIS) > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + sama5d4_wdt_init(wdt); > > return 0; > } > -- > 2.11.0 Best Regards, Wenyou Yang ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/4] watchdog: sama5d4: fix race condition 2017-03-02 17:31 [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni 2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni @ 2017-03-02 17:31 ` Alexandre Belloni 2017-03-02 17:42 ` Guenter Roeck ` (2 more replies) 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni 2017-03-02 17:31 ` [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume Alexandre Belloni 3 siblings, 3 replies; 17+ messages in thread From: Alexandre Belloni @ 2017-03-02 17:31 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel, Alexandre Belloni WDT_MR and WDT_CR must not updated within three slow clock periods after the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed before writing those registers. wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the IP. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index 5cee20caca78..362fd229786d 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -6,6 +6,7 @@ * Licensed under GPLv2. */ +#include <linux/delay.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> @@ -29,6 +30,7 @@ struct sama5d4_wdt { struct watchdog_device wdd; void __iomem *reg_base; u32 mr; + unsigned long last_ping; }; static int wdt_timeout = WDT_DEFAULT_TIMEOUT; @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout, #define wdt_read(wdt, field) \ readl_relaxed((wdt)->reg_base + (field)) -#define wdt_write(wtd, field, val) \ - writel_relaxed((val), (wdt)->reg_base + (field)) +/* 4 slow clock periods is 4/32768 = 122.07µs*/ +#define WDT_DELAY usecs_to_jiffies(123) + +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) +{ + /* + * WDT_CR and WDT_MR must not be modified within three slow clock + * periods following a restart of the watchdog performed by a write + * access in WDT_CR. + */ + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) + usleep_range(30, 125); + writel_relaxed(val, wdt->reg_base + field); + wdt->last_ping = jiffies; +} + +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val) +{ + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) + udelay(123); + writel_relaxed(val, wdt->reg_base + field); + wdt->last_ping = jiffies; +} static int sama5d4_wdt_start(struct watchdog_device *wdd) { @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) * Else, we have to disable it properly. */ if (wdt_enabled) { - wdt_write(wdt, AT91_WDT_MR, wdt->mr); + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); } else { reg = wdt_read(wdt, AT91_WDT_MR); if (!(reg & AT91_WDT_WDDIS)) - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); + wdt_write_nosleep(wdt, AT91_WDT_MR, + reg | AT91_WDT_WDDIS); } return 0; } @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) wdd->ops = &sama5d4_wdt_ops; wdd->min_timeout = MIN_WDT_TIMEOUT; wdd->max_timeout = MAX_WDT_TIMEOUT; + wdt->last_ping = jiffies; watchdog_set_drvdata(wdd, wdt); -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] watchdog: sama5d4: fix race condition 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni @ 2017-03-02 17:42 ` Guenter Roeck 2017-03-02 17:54 ` Alexandre Belloni 2017-03-04 15:05 ` Guenter Roeck 2017-03-07 2:06 ` Wenyou.Yang 2 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2017-03-02 17:42 UTC (permalink / raw) To: Alexandre Belloni Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On Thu, Mar 02, 2017 at 06:31:12PM +0100, Alexandre Belloni wrote: > WDT_MR and WDT_CR must not updated within three slow clock periods after > the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed > before writing those registers. > wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the > IP. > Would it be possible to use min_hw_heartbeat_ms for this purpose ? Thanks, Guenter > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index 5cee20caca78..362fd229786d 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -6,6 +6,7 @@ > * Licensed under GPLv2. > */ > > +#include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -29,6 +30,7 @@ struct sama5d4_wdt { > struct watchdog_device wdd; > void __iomem *reg_base; > u32 mr; > + unsigned long last_ping; > }; > > static int wdt_timeout = WDT_DEFAULT_TIMEOUT; > @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout, > #define wdt_read(wdt, field) \ > readl_relaxed((wdt)->reg_base + (field)) > > -#define wdt_write(wtd, field, val) \ > - writel_relaxed((val), (wdt)->reg_base + (field)) > +/* 4 slow clock periods is 4/32768 = 122.07µs*/ > +#define WDT_DELAY usecs_to_jiffies(123) > + > +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) > +{ > + /* > + * WDT_CR and WDT_MR must not be modified within three slow clock > + * periods following a restart of the watchdog performed by a write > + * access in WDT_CR. > + */ > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + usleep_range(30, 125); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > + > +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val) > +{ > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + udelay(123); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > > static int sama5d4_wdt_start(struct watchdog_device *wdd) > { > @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > * Else, we have to disable it properly. > */ > if (wdt_enabled) { > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > } else { > reg = wdt_read(wdt, AT91_WDT_MR); > if (!(reg & AT91_WDT_WDDIS)) > - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); > + wdt_write_nosleep(wdt, AT91_WDT_MR, > + reg | AT91_WDT_WDDIS); > } > return 0; > } > @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > wdd->ops = &sama5d4_wdt_ops; > wdd->min_timeout = MIN_WDT_TIMEOUT; > wdd->max_timeout = MAX_WDT_TIMEOUT; > + wdt->last_ping = jiffies; > > watchdog_set_drvdata(wdd, wdt); > > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] watchdog: sama5d4: fix race condition 2017-03-02 17:42 ` Guenter Roeck @ 2017-03-02 17:54 ` Alexandre Belloni 0 siblings, 0 replies; 17+ messages in thread From: Alexandre Belloni @ 2017-03-02 17:54 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On 02/03/2017 at 09:42:24 -0800, Guenter Roeck wrote: > On Thu, Mar 02, 2017 at 06:31:12PM +0100, Alexandre Belloni wrote: > > WDT_MR and WDT_CR must not updated within three slow clock periods after > > the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed > > before writing those registers. > > wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the > > IP. > > > > Would it be possible to use min_hw_heartbeat_ms for this purpose ? > I don't think so unless you want the core to also use that delay between __watchdog_ping, watchdog_start, watchdog_set_timeout and watchdog_stop. Currently, it is only between two calls of __watchdog_ping, > Thanks, > Guenter > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > --- > > drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > > index 5cee20caca78..362fd229786d 100644 > > --- a/drivers/watchdog/sama5d4_wdt.c > > +++ b/drivers/watchdog/sama5d4_wdt.c > > @@ -6,6 +6,7 @@ > > * Licensed under GPLv2. > > */ > > > > +#include <linux/delay.h> > > #include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/kernel.h> > > @@ -29,6 +30,7 @@ struct sama5d4_wdt { > > struct watchdog_device wdd; > > void __iomem *reg_base; > > u32 mr; > > + unsigned long last_ping; > > }; > > > > static int wdt_timeout = WDT_DEFAULT_TIMEOUT; > > @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout, > > #define wdt_read(wdt, field) \ > > readl_relaxed((wdt)->reg_base + (field)) > > > > -#define wdt_write(wtd, field, val) \ > > - writel_relaxed((val), (wdt)->reg_base + (field)) > > +/* 4 slow clock periods is 4/32768 = 122.07µs*/ > > +#define WDT_DELAY usecs_to_jiffies(123) > > + > > +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) > > +{ > > + /* > > + * WDT_CR and WDT_MR must not be modified within three slow clock > > + * periods following a restart of the watchdog performed by a write > > + * access in WDT_CR. > > + */ > > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > > + usleep_range(30, 125); > > + writel_relaxed(val, wdt->reg_base + field); > > + wdt->last_ping = jiffies; > > +} > > + > > +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val) > > +{ > > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > > + udelay(123); > > + writel_relaxed(val, wdt->reg_base + field); > > + wdt->last_ping = jiffies; > > +} > > > > static int sama5d4_wdt_start(struct watchdog_device *wdd) > > { > > @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > > * Else, we have to disable it properly. > > */ > > if (wdt_enabled) { > > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > > } else { > > reg = wdt_read(wdt, AT91_WDT_MR); > > if (!(reg & AT91_WDT_WDDIS)) > > - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); > > + wdt_write_nosleep(wdt, AT91_WDT_MR, > > + reg | AT91_WDT_WDDIS); > > } > > return 0; > > } > > @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > > wdd->ops = &sama5d4_wdt_ops; > > wdd->min_timeout = MIN_WDT_TIMEOUT; > > wdd->max_timeout = MAX_WDT_TIMEOUT; > > + wdt->last_ping = jiffies; > > > > watchdog_set_drvdata(wdd, wdt); > > > > -- > > 2.11.0 > > -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] watchdog: sama5d4: fix race condition 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni 2017-03-02 17:42 ` Guenter Roeck @ 2017-03-04 15:05 ` Guenter Roeck 2017-03-07 2:06 ` Wenyou.Yang 2 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-03-04 15:05 UTC (permalink / raw) To: Alexandre Belloni Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On 03/02/2017 09:31 AM, Alexandre Belloni wrote: > WDT_MR and WDT_CR must not updated within three slow clock periods after > the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed > before writing those registers. > wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the > IP. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index 5cee20caca78..362fd229786d 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -6,6 +6,7 @@ > * Licensed under GPLv2. > */ > > +#include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -29,6 +30,7 @@ struct sama5d4_wdt { > struct watchdog_device wdd; > void __iomem *reg_base; > u32 mr; > + unsigned long last_ping; > }; > > static int wdt_timeout = WDT_DEFAULT_TIMEOUT; > @@ -49,8 +51,29 @@ MODULE_PARM_DESC(nowayout, > #define wdt_read(wdt, field) \ > readl_relaxed((wdt)->reg_base + (field)) > > -#define wdt_write(wtd, field, val) \ > - writel_relaxed((val), (wdt)->reg_base + (field)) > +/* 4 slow clock periods is 4/32768 = 122.07µs*/ > +#define WDT_DELAY usecs_to_jiffies(123) > + > +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) > +{ > + /* > + * WDT_CR and WDT_MR must not be modified within three slow clock > + * periods following a restart of the watchdog performed by a write > + * access in WDT_CR. > + */ > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + usleep_range(30, 125); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > + > +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 val) > +{ > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + udelay(123); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > > static int sama5d4_wdt_start(struct watchdog_device *wdd) > { > @@ -164,11 +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > * Else, we have to disable it properly. > */ > if (wdt_enabled) { > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > } else { > reg = wdt_read(wdt, AT91_WDT_MR); > if (!(reg & AT91_WDT_WDDIS)) > - wdt_write(wdt, AT91_WDT_MR, reg | AT91_WDT_WDDIS); > + wdt_write_nosleep(wdt, AT91_WDT_MR, > + reg | AT91_WDT_WDDIS); > } > return 0; > } > @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > wdd->ops = &sama5d4_wdt_ops; > wdd->min_timeout = MIN_WDT_TIMEOUT; > wdd->max_timeout = MAX_WDT_TIMEOUT; > + wdt->last_ping = jiffies; > > watchdog_set_drvdata(wdd, wdt); > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 2/4] watchdog: sama5d4: fix race condition 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni 2017-03-02 17:42 ` Guenter Roeck 2017-03-04 15:05 ` Guenter Roeck @ 2017-03-07 2:06 ` Wenyou.Yang 2 siblings, 0 replies; 17+ messages in thread From: Wenyou.Yang @ 2017-03-07 2:06 UTC (permalink / raw) To: alexandre.belloni, linux Cc: wim, Nicolas.Ferre, linux-watchdog, linux-arm-kernel, linux-kernel > -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > Sent: 2017年3月3日 1:31 > To: Guenter Roeck <linux@roeck-us.net> > Cc: Wim Van Sebroeck <wim@iguana.be>; Nicolas Ferre - M43238 > <Nicolas.Ferre@microchip.com>; Wenyou Yang - A41535 > <Wenyou.Yang@microchip.com>; linux-watchdog@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni > <alexandre.belloni@free-electrons.com> > Subject: [PATCH 2/4] watchdog: sama5d4: fix race condition > > WDT_MR and WDT_CR must not updated within three slow clock periods after > the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed > before writing those registers. > wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the IP. Indeed. Acked-by: Wenyou.Yang <wenyou.yang@microchip.com> Thanks > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/watchdog/sama5d4_wdt.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index 5cee20caca78..362fd229786d 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -6,6 +6,7 @@ > * Licensed under GPLv2. > */ > > +#include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -29,6 +30,7 @@ struct sama5d4_wdt { > struct watchdog_device wdd; > void __iomem *reg_base; > u32 mr; > + unsigned long last_ping; > }; > > static int wdt_timeout = WDT_DEFAULT_TIMEOUT; @@ -49,8 +51,29 @@ > MODULE_PARM_DESC(nowayout, #define wdt_read(wdt, field) \ > readl_relaxed((wdt)->reg_base + (field)) > > -#define wdt_write(wtd, field, val) \ > - writel_relaxed((val), (wdt)->reg_base + (field)) > +/* 4 slow clock periods is 4/32768 = 122.07µs*/ > +#define WDT_DELAY usecs_to_jiffies(123) > + > +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) { > + /* > + * WDT_CR and WDT_MR must not be modified within three slow clock > + * periods following a restart of the watchdog performed by a write > + * access in WDT_CR. > + */ > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + usleep_range(30, 125); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > + > +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32 > +val) { > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + udelay(123); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > > static int sama5d4_wdt_start(struct watchdog_device *wdd) { @@ -164,11 > +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt) > * Else, we have to disable it properly. > */ > if (wdt_enabled) { > - wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > } else { > reg = wdt_read(wdt, AT91_WDT_MR); > if (!(reg & AT91_WDT_WDDIS)) > - wdt_write(wdt, AT91_WDT_MR, reg | > AT91_WDT_WDDIS); > + wdt_write_nosleep(wdt, AT91_WDT_MR, > + reg | AT91_WDT_WDDIS); > } > return 0; > } > @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device > *pdev) > wdd->ops = &sama5d4_wdt_ops; > wdd->min_timeout = MIN_WDT_TIMEOUT; > wdd->max_timeout = MAX_WDT_TIMEOUT; > + wdt->last_ping = jiffies; > > watchdog_set_drvdata(wdd, wdt); > > -- > 2.11.0 Best Regards, Wenyou Yang ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-02 17:31 [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni 2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni @ 2017-03-02 17:31 ` Alexandre Belloni 2017-03-02 19:29 ` Alexander Dahl ` (2 more replies) 2017-03-02 17:31 ` [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume Alexandre Belloni 3 siblings, 3 replies; 17+ messages in thread From: Alexandre Belloni @ 2017-03-02 17:31 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel, Alexandre Belloni Because the only way to use the driver is to have a device tree enabling it, pdev->dev.of_node will never be NULL. Remove the unnecessary check. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/watchdog/sama5d4_wdt.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index 362fd229786d..d710014f3b7d 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -228,15 +228,13 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) wdt->reg_base = regs; - if (pdev->dev.of_node) { - irq = irq_of_parse_and_map(pdev->dev.of_node, 0); - if (!irq) - dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (!irq) + dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); - ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt); - if (ret) - return ret; - } + ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt); + if (ret) + return ret; if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler, -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni @ 2017-03-02 19:29 ` Alexander Dahl 2017-03-03 8:03 ` Thomas Petazzoni 2017-03-04 15:06 ` Guenter Roeck 2 siblings, 0 replies; 17+ messages in thread From: Alexander Dahl @ 2017-03-02 19:29 UTC (permalink / raw) To: Alexandre Belloni; +Cc: linux-watchdog, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 395 bytes --] Hei hei, there's a typo in the subject you may want to fix in a v2: watchodg → watchdog Greets Alex -- »With the first link, the chain is forged. The first speech censured, the first thought forbidden, the first freedom denied, chains us all irrevocably.« (Jean-Luc Picard, quoting Judge Aaron Satie) *** GnuPG-FP: C28E E6B9 0263 95CF 8FAF 08FA 34AD CD00 7221 5CC6 *** [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni 2017-03-02 19:29 ` Alexander Dahl @ 2017-03-03 8:03 ` Thomas Petazzoni 2017-03-03 9:29 ` Alexandre Belloni 2017-03-04 15:06 ` Guenter Roeck 2 siblings, 1 reply; 17+ messages in thread From: Thomas Petazzoni @ 2017-03-03 8:03 UTC (permalink / raw) To: Alexandre Belloni Cc: Guenter Roeck, linux-watchdog, Wenyou.Yang, linux-kernel, Nicolas Ferre, Wim Van Sebroeck, linux-arm-kernel Hello, On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote: > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); Any reason to use irq_of_parse_and_map() over the more conventional platform_get_irq() ? Best regards, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-03 8:03 ` Thomas Petazzoni @ 2017-03-03 9:29 ` Alexandre Belloni 2017-03-03 14:14 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Alexandre Belloni @ 2017-03-03 9:29 UTC (permalink / raw) To: Thomas Petazzoni Cc: Guenter Roeck, linux-watchdog, Wenyou.Yang, linux-kernel, Nicolas Ferre, Wim Van Sebroeck, linux-arm-kernel On 03/03/2017 at 09:03:25 +0100, Thomas Petazzoni wrote: > On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote: > > > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > > Any reason to use irq_of_parse_and_map() over the more conventional > platform_get_irq() ? > No particular reason but I'm just removing the if (pdev->dev.of_node) -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-03 9:29 ` Alexandre Belloni @ 2017-03-03 14:14 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-03-03 14:14 UTC (permalink / raw) To: Alexandre Belloni, Thomas Petazzoni Cc: linux-watchdog, Wenyou.Yang, linux-kernel, Nicolas Ferre, Wim Van Sebroeck, linux-arm-kernel On 03/03/2017 01:29 AM, Alexandre Belloni wrote: > On 03/03/2017 at 09:03:25 +0100, Thomas Petazzoni wrote: >> On Thu, 2 Mar 2017 18:31:13 +0100, Alexandre Belloni wrote: >> >>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); >> >> Any reason to use irq_of_parse_and_map() over the more conventional >> platform_get_irq() ? >> > > No particular reason but I'm just removing the if (pdev->dev.of_node) > A function call change would (should) be a separate patch. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] watchodg: sama5d4: simplify probe 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni 2017-03-02 19:29 ` Alexander Dahl 2017-03-03 8:03 ` Thomas Petazzoni @ 2017-03-04 15:06 ` Guenter Roeck 2 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-03-04 15:06 UTC (permalink / raw) To: Alexandre Belloni Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On 03/02/2017 09:31 AM, Alexandre Belloni wrote: > Because the only way to use the driver is to have a device tree enabling > it, pdev->dev.of_node will never be NULL. Remove the unnecessary check. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/sama5d4_wdt.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index 362fd229786d..d710014f3b7d 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -228,15 +228,13 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) > > wdt->reg_base = regs; > > - if (pdev->dev.of_node) { > - irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > - if (!irq) > - dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (!irq) > + dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); > > - ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt); > - if (ret) > - return ret; > - } > + ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt); > + if (ret) > + return ret; > > if ((wdt->mr & AT91_WDT_WDFIEN) && irq) { > ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler, > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume 2017-03-02 17:31 [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni ` (2 preceding siblings ...) 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni @ 2017-03-02 17:31 ` Alexandre Belloni 2017-03-04 15:07 ` Guenter Roeck 3 siblings, 1 reply; 17+ messages in thread From: Alexandre Belloni @ 2017-03-02 17:31 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel, Alexandre Belloni Because suspending to RAM may lose the register values, they are restored on resume. This is currently done unconditionally because there is currently no way to know (from the driver) whether they have really been lost or are still valid. Writing MR also pings the watchdog and this may not be what is expected so add a comment explaining why it happens. Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/watchdog/sama5d4_wdt.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index d710014f3b7d..0ae947c3d7bc 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -300,6 +300,11 @@ static int sama5d4_wdt_resume(struct device *dev) { struct sama5d4_wdt *wdt = dev_get_drvdata(dev); + /* + * FIXME: writing MR also pings the watchdog which may not be desired. + * This should only be done when the registers are lost on suspend but + * there is no way to get this information right now. + */ sama5d4_wdt_init(wdt); return 0; -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume 2017-03-02 17:31 ` [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume Alexandre Belloni @ 2017-03-04 15:07 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2017-03-04 15:07 UTC (permalink / raw) To: Alexandre Belloni Cc: Wim Van Sebroeck, Nicolas Ferre, Wenyou.Yang, linux-watchdog, linux-arm-kernel, linux-kernel On 03/02/2017 09:31 AM, Alexandre Belloni wrote: > Because suspending to RAM may lose the register values, they are restored > on resume. This is currently done unconditionally because there is > currently no way to know (from the driver) whether they have really been > lost or are still valid. Writing MR also pings the watchdog and this may > not be what is expected so add a comment explaining why it happens. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/sama5d4_wdt.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c > index d710014f3b7d..0ae947c3d7bc 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -300,6 +300,11 @@ static int sama5d4_wdt_resume(struct device *dev) > { > struct sama5d4_wdt *wdt = dev_get_drvdata(dev); > > + /* > + * FIXME: writing MR also pings the watchdog which may not be desired. > + * This should only be done when the registers are lost on suspend but > + * there is no way to get this information right now. > + */ > sama5d4_wdt_init(wdt); > > return 0; > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-03-07 2:15 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-02 17:31 [PATCH 0/4] watchdog: sama5d4: fix issues Alexandre Belloni 2017-03-02 17:31 ` [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling Alexandre Belloni 2017-03-04 15:04 ` Guenter Roeck 2017-03-07 2:05 ` Wenyou.Yang 2017-03-02 17:31 ` [PATCH 2/4] watchdog: sama5d4: fix race condition Alexandre Belloni 2017-03-02 17:42 ` Guenter Roeck 2017-03-02 17:54 ` Alexandre Belloni 2017-03-04 15:05 ` Guenter Roeck 2017-03-07 2:06 ` Wenyou.Yang 2017-03-02 17:31 ` [PATCH 3/4] watchodg: sama5d4: simplify probe Alexandre Belloni 2017-03-02 19:29 ` Alexander Dahl 2017-03-03 8:03 ` Thomas Petazzoni 2017-03-03 9:29 ` Alexandre Belloni 2017-03-03 14:14 ` Guenter Roeck 2017-03-04 15:06 ` Guenter Roeck 2017-03-02 17:31 ` [PATCH 4/4] watchdog: sama5d4: Add comment explaining what happens on resume Alexandre Belloni 2017-03-04 15:07 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).