| Message ID | 1468570524-18222-3-git-send-email-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/15/2016 01:15 AM, Rasmus Villemoes wrote: > A bootloader may start the watchdog device before handing control to > the kernel - in that case, we should tell the kernel about it so the > watchdog framework can keep it alive until userspace opens > /dev/watchdog0. > Separate note: The maximum timeout for this watchdog is 15 seconds. Given that, it might be useful to set max_hw_heartbeat_ms instead of max_timeout. Separate patch, though. > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/watchdog/bcm2835_wdt.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c > index 4dddd82..9a08334 100644 > --- a/drivers/watchdog/bcm2835_wdt.c > +++ b/drivers/watchdog/bcm2835_wdt.c > @@ -55,6 +55,15 @@ struct bcm2835_wdt { > static unsigned int heartbeat; > static bool nowayout = WATCHDOG_NOWAYOUT; > > +static bool bcm2835_wdt_is_running(struct bcm2835_wdt *wdt) > +{ > + uint32_t cur; > + > + cur = readl(wdt->base + PM_RSTC); > + > + return !!(cur & PM_RSTC_WRCFG_FULL_RESET); > +} > + > static int bcm2835_wdt_start(struct watchdog_device *wdog) > { > struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); > @@ -70,6 +79,7 @@ static int bcm2835_wdt_start(struct watchdog_device *wdog) > PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC); > > spin_unlock_irqrestore(&wdt->lock, flags); > + set_bit(WDOG_HW_RUNNING, &wdog->status); > You don't need to set this bit here unless the watchdog can not be stopped. > return 0; > } > @@ -79,6 +89,7 @@ static int bcm2835_wdt_stop(struct watchdog_device *wdog) > struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); > > writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC); > + clear_bit(WDOG_HW_RUNNING, &wdog->status); ... and since you clear the bit, it can be stopped. Both setting and resetting the bit is therefore not necessary. > return 0; > } > > @@ -181,6 +192,17 @@ static int bcm2835_wdt_probe(struct platform_device *pdev) > watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev); > watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout); > bcm2835_wdt_wdd.parent = &pdev->dev; > + if (bcm2835_wdt_is_running(wdt)) { > + /* > + * The currently active timeout value (set by the > + * bootloader) may be different from the module > + * heartbeat parameter or the value in device > + * tree. But we just need to set WDOG_HW_RUNNING, > + * because then the framework will "immediately" ping > + * the device, updating the timeout. > + */ > + set_bit(WDOG_HW_RUNNING, &bcm2835_wdt_wdd.status); > + } > err = watchdog_register_device(&bcm2835_wdt_wdd); > if (err) { > dev_err(dev, "Failed to register watchdog device"); >
On 2016-07-15 15:46, Guenter Roeck wrote: > On 07/15/2016 01:15 AM, Rasmus Villemoes wrote: >> >> +static bool bcm2835_wdt_is_running(struct bcm2835_wdt *wdt) >> +{ >> + uint32_t cur; >> + >> + cur = readl(wdt->base + PM_RSTC); >> + >> + return !!(cur & PM_RSTC_WRCFG_FULL_RESET); >> +} >> + >> static int bcm2835_wdt_start(struct watchdog_device *wdog) >> { >> struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); >> @@ -70,6 +79,7 @@ static int bcm2835_wdt_start(struct watchdog_device >> *wdog) >> PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC); >> >> spin_unlock_irqrestore(&wdt->lock, flags); >> + set_bit(WDOG_HW_RUNNING, &wdog->status); >> > You don't need to set this bit here unless the watchdog can not be stopped. > >> return 0; >> } >> @@ -79,6 +89,7 @@ static int bcm2835_wdt_stop(struct watchdog_device >> *wdog) >> struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); >> >> writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC); >> + clear_bit(WDOG_HW_RUNNING, &wdog->status); > > ... and since you clear the bit, it can be stopped. Both setting and > resetting the bit > is therefore not necessary. Well, if the bit isn't cleared here, but it was set during probe(), the framework will (re)start this watchdog (and keep it fed) since there's no separate ping method. I suppose that's reasonable semantics if the watchdog was running at boot (and I like how that ends up interacting with my open_deadline proposal), but probably a little too subtle. This would also change if the ->start method was broken up into separate ping and start methods, which it seems that it could be. If we do clear the bit here, I think it's neater to set it in start as well, even if that doesn't really have any effect. Rasmus
On Wed, Jul 20, 2016 at 11:37:55PM +0200, Rasmus Villemoes wrote: > On 2016-07-15 15:46, Guenter Roeck wrote: > >On 07/15/2016 01:15 AM, Rasmus Villemoes wrote: > >> > >>+static bool bcm2835_wdt_is_running(struct bcm2835_wdt *wdt) > >>+{ > >>+ uint32_t cur; > >>+ > >>+ cur = readl(wdt->base + PM_RSTC); > >>+ > >>+ return !!(cur & PM_RSTC_WRCFG_FULL_RESET); > >>+} > >>+ > >> static int bcm2835_wdt_start(struct watchdog_device *wdog) > >> { > >> struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); > >>@@ -70,6 +79,7 @@ static int bcm2835_wdt_start(struct watchdog_device > >>*wdog) > >> PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC); > >> > >> spin_unlock_irqrestore(&wdt->lock, flags); > >>+ set_bit(WDOG_HW_RUNNING, &wdog->status); > >> > >You don't need to set this bit here unless the watchdog can not be stopped. > > > >> return 0; > >> } > >>@@ -79,6 +89,7 @@ static int bcm2835_wdt_stop(struct watchdog_device > >>*wdog) > >> struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); > >> > >> writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC); > >>+ clear_bit(WDOG_HW_RUNNING, &wdog->status); > > > >... and since you clear the bit, it can be stopped. Both setting and > >resetting the bit > >is therefore not necessary. > > Well, if the bit isn't cleared here, but it was set during probe(), the > framework will (re)start this watchdog (and keep it fed) since there's no > separate ping method. I suppose that's reasonable semantics if the watchdog > was running at boot (and I like how that ends up interacting with my > open_deadline proposal), but probably a little too subtle. This would also > change if the ->start method was broken up into separate ping and start > methods, which it seems that it could be. > > If we do clear the bit here, I think it's neater to set it in start as well, > even if that doesn't really have any effect. > The problem is different. The core should clear the bit on close if there is a stop function, and if calling the stop function does not return an error. Guenter
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c index 4dddd82..9a08334 100644 --- a/drivers/watchdog/bcm2835_wdt.c +++ b/drivers/watchdog/bcm2835_wdt.c @@ -55,6 +55,15 @@ struct bcm2835_wdt { static unsigned int heartbeat; static bool nowayout = WATCHDOG_NOWAYOUT; +static bool bcm2835_wdt_is_running(struct bcm2835_wdt *wdt) +{ + uint32_t cur; + + cur = readl(wdt->base + PM_RSTC); + + return !!(cur & PM_RSTC_WRCFG_FULL_RESET); +} + static int bcm2835_wdt_start(struct watchdog_device *wdog) { struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); @@ -70,6 +79,7 @@ static int bcm2835_wdt_start(struct watchdog_device *wdog) PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC); spin_unlock_irqrestore(&wdt->lock, flags); + set_bit(WDOG_HW_RUNNING, &wdog->status); return 0; } @@ -79,6 +89,7 @@ static int bcm2835_wdt_stop(struct watchdog_device *wdog) struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog); writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC); + clear_bit(WDOG_HW_RUNNING, &wdog->status); return 0; } @@ -181,6 +192,17 @@ static int bcm2835_wdt_probe(struct platform_device *pdev) watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev); watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout); bcm2835_wdt_wdd.parent = &pdev->dev; + if (bcm2835_wdt_is_running(wdt)) { + /* + * The currently active timeout value (set by the + * bootloader) may be different from the module + * heartbeat parameter or the value in device + * tree. But we just need to set WDOG_HW_RUNNING, + * because then the framework will "immediately" ping + * the device, updating the timeout. + */ + set_bit(WDOG_HW_RUNNING, &bcm2835_wdt_wdd.status); + } err = watchdog_register_device(&bcm2835_wdt_wdd); if (err) { dev_err(dev, "Failed to register watchdog device");
A bootloader may start the watchdog device before handing control to the kernel - in that case, we should tell the kernel about it so the watchdog framework can keep it alive until userspace opens /dev/watchdog0. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/watchdog/bcm2835_wdt.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)