[PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures

[PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures

[Rasmus Villemoes]

These are never modified, so might as well be const.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/bcm2835_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
index 2e6164c..733e402 100644
--- a/drivers/watchdog/bcm2835_wdt.c
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -96,7 +96,7 @@ static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
 	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
 }
 
-static struct watchdog_ops bcm2835_wdt_ops = {
+static const struct watchdog_ops bcm2835_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	bcm2835_wdt_start,
 	.stop =		bcm2835_wdt_stop,
@@ -104,7 +104,7 @@ static struct watchdog_ops bcm2835_wdt_ops = {
 	.get_timeleft =	bcm2835_wdt_get_timeleft,
 };
 
-static struct watchdog_info bcm2835_wdt_info = {
+static const struct watchdog_info bcm2835_wdt_info = {
 	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
 			WDIOF_KEEPALIVEPING,
 	.identity =	"Broadcom BCM2835 Watchdog timer",
-- 
2.5.0

[PATCH 2/3] watchdog: bcm2835_wdt: remove redundant ->set_timeout callback

[Rasmus Villemoes]

bcm2835_wdt_set_timeout does exactly what the watchdog framework does
in the absence of a ->set_timeout callback (see watchdog_set_timeout
in watchdog_dev.c), so remove it.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/bcm2835_wdt.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
index 733e402..4dddd82 100644
--- a/drivers/watchdog/bcm2835_wdt.c
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -82,12 +82,6 @@ static int bcm2835_wdt_stop(struct watchdog_device *wdog)
 	return 0;
 }
 
-static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
-{
-	wdog->timeout = t;
-	return 0;
-}
-
 static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
 {
 	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
@@ -100,7 +94,6 @@ static const struct watchdog_ops bcm2835_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	bcm2835_wdt_start,
 	.stop =		bcm2835_wdt_stop,
-	.set_timeout =	bcm2835_wdt_set_timeout,
 	.get_timeleft =	bcm2835_wdt_get_timeleft,
 };
 
-- 
2.5.0

Re: [PATCH 2/3] watchdog: bcm2835_wdt: remove redundant ->set_timeout callback

[Guenter Roeck]

On 07/15/2016 01:15 AM, Rasmus Villemoes wrote:
> bcm2835_wdt_set_timeout does exactly what the watchdog framework does
> in the absence of a ->set_timeout callback (see watchdog_set_timeout
> in watchdog_dev.c), so remove it.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/bcm2835_wdt.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> index 733e402..4dddd82 100644
> --- a/drivers/watchdog/bcm2835_wdt.c
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -82,12 +82,6 @@ static int bcm2835_wdt_stop(struct watchdog_device *wdog)
>   	return 0;
>   }
>
> -static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> -{
> -	wdog->timeout = t;
> -	return 0;
> -}
> -
>   static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
>   {
>   	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> @@ -100,7 +94,6 @@ static const struct watchdog_ops bcm2835_wdt_ops = {
>   	.owner =	THIS_MODULE,
>   	.start =	bcm2835_wdt_start,
>   	.stop =		bcm2835_wdt_stop,
> -	.set_timeout =	bcm2835_wdt_set_timeout,
>   	.get_timeleft =	bcm2835_wdt_get_timeleft,
>   };
>
>

Re: [PATCH 2/3] watchdog: bcm2835_wdt: remove redundant ->set_timeout callback

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Rasmus Villemoes <rasmus.villemoes@prevas.dk> writes:

> bcm2835_wdt_set_timeout does exactly what the watchdog framework does
> in the absence of a ->set_timeout callback (see watchdog_set_timeout
> in watchdog_dev.c), so remove it.

These first two patches are:

Acked-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/3] watchdog: bcm2835_wdt: remove redundant ->set_timeout callback

[Wim Van Sebroeck]

Hi Rasmus,

> bcm2835_wdt_set_timeout does exactly what the watchdog framework does
> in the absence of a ->set_timeout callback (see watchdog_set_timeout
> in watchdog_dev.c), so remove it.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/bcm2835_wdt.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> index 733e402..4dddd82 100644
> --- a/drivers/watchdog/bcm2835_wdt.c
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -82,12 +82,6 @@ static int bcm2835_wdt_stop(struct watchdog_device *wdog)
>  	return 0;
>  }
>  
> -static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> -{
> -	wdog->timeout = t;
> -	return 0;
> -}
> -
>  static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
>  {
>  	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> @@ -100,7 +94,6 @@ static const struct watchdog_ops bcm2835_wdt_ops = {
>  	.owner =	THIS_MODULE,
>  	.start =	bcm2835_wdt_start,
>  	.stop =		bcm2835_wdt_stop,
> -	.set_timeout =	bcm2835_wdt_set_timeout,
>  	.get_timeleft =	bcm2835_wdt_get_timeleft,
>  };
>  
> -- 
> 2.5.0
> 

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.

[PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate

[Rasmus Villemoes]

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(+)

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");
-- 
2.5.0

Re: [PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate

[Guenter Roeck]

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");
>

Re: [PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate

[Rasmus Villemoes]

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

Re: [PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate

[Guenter Roeck]

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

Re: [PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures

[Guenter Roeck]

On 07/15/2016 01:15 AM, Rasmus Villemoes wrote:
> These are never modified, so might as well be const.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/bcm2835_wdt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> index 2e6164c..733e402 100644
> --- a/drivers/watchdog/bcm2835_wdt.c
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -96,7 +96,7 @@ static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
>   	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
>   }
>
> -static struct watchdog_ops bcm2835_wdt_ops = {
> +static const struct watchdog_ops bcm2835_wdt_ops = {
>   	.owner =	THIS_MODULE,
>   	.start =	bcm2835_wdt_start,
>   	.stop =		bcm2835_wdt_stop,
> @@ -104,7 +104,7 @@ static struct watchdog_ops bcm2835_wdt_ops = {
>   	.get_timeleft =	bcm2835_wdt_get_timeleft,
>   };
>
> -static struct watchdog_info bcm2835_wdt_info = {
> +static const struct watchdog_info bcm2835_wdt_info = {
>   	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
>   			WDIOF_KEEPALIVEPING,
>   	.identity =	"Broadcom BCM2835 Watchdog timer",
>

Re: [PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures

[Wim Van Sebroeck]

Hi Rasmus,

> These are never modified, so might as well be const.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/watchdog/bcm2835_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> index 2e6164c..733e402 100644
> --- a/drivers/watchdog/bcm2835_wdt.c
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -96,7 +96,7 @@ static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
>  	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
>  }
>  
> -static struct watchdog_ops bcm2835_wdt_ops = {
> +static const struct watchdog_ops bcm2835_wdt_ops = {
>  	.owner =	THIS_MODULE,
>  	.start =	bcm2835_wdt_start,
>  	.stop =		bcm2835_wdt_stop,
> @@ -104,7 +104,7 @@ static struct watchdog_ops bcm2835_wdt_ops = {
>  	.get_timeleft =	bcm2835_wdt_get_timeleft,
>  };
>  
> -static struct watchdog_info bcm2835_wdt_info = {
> +static const struct watchdog_info bcm2835_wdt_info = {
>  	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
>  			WDIOF_KEEPALIVEPING,
>  	.identity =	"Broadcom BCM2835 Watchdog timer",
> -- 
> 2.5.0
> 

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.