From: rasmus.villemoes@prevas.dk (Rasmus Villemoes)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate
Date: Wed, 20 Jul 2016 23:37:55 +0200 [thread overview]
Message-ID: <578FEF33.9020801@prevas.dk> (raw)
In-Reply-To: <5788E935.6080608@roeck-us.net>
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
next prev parent reply other threads:[~2016-07-20 21:37 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-15 8:15 [PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures Rasmus Villemoes
2016-07-15 8:15 ` [PATCH 2/3] watchdog: bcm2835_wdt: remove redundant ->set_timeout callback Rasmus Villemoes
2016-07-15 13:47 ` Guenter Roeck
2016-07-15 18:58 ` Eric Anholt
2016-07-15 8:15 ` [PATCH 3/3] watchdog: bcm2835_wdt: set WDOG_HW_RUNNING bit when appropriate Rasmus Villemoes
2016-07-15 13:46 ` Guenter Roeck
2016-07-20 21:37 ` Rasmus Villemoes [this message]
2016-07-20 23:47 ` Guenter Roeck
2016-07-15 13:47 ` [PATCH 1/3] watchdog: bcm2835_wdt: constify _ops and _info structures Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=578FEF33.9020801@prevas.dk \
--to=rasmus.villemoes@prevas.dk \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).