From: Guenter Roeck <linux@roeck-us.net>
To: Anson Huang <anson.huang@nxp.com>,
"wim@linux-watchdog.org" <wim@linux-watchdog.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
"festevam@gmail.com" <festevam@gmail.com>,
"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
Date: Wed, 29 Jul 2020 08:49:12 -0700 [thread overview]
Message-ID: <2e962b44-d1eb-f060-f1d2-a079322d4c22@roeck-us.net> (raw)
In-Reply-To: <DB3PR0402MB391641A2991A7651B72283D0F5700@DB3PR0402MB3916.eurprd04.prod.outlook.com>
On 7/29/20 8:32 AM, Anson Huang wrote:
> Hi, Guenter
>
>
>> Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence
>> for wdog operations
>>
>> On 7/28/20 7:20 PM, Anson Huang wrote:
>>> According to reference manual, the i.MX7ULP WDOG's operations should
>>> follow below sequence:
>>>
>>> 1. disable global interrupts;
>>> 2. unlock the wdog and wait unlock bit set; 3. reconfigure the wdog
>>> and wait for reconfiguration bit set; 4. enabel global interrupts.
>>>
>>> Strictly follow the recommended sequence can make it more robust.
>>>
>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>> ---
>>> Changes since V1:
>>> - use readl_poll_timeout_atomic() instead of usleep_ranges() since IRQ is
>> disabled.
>>> ---
>>> drivers/watchdog/imx7ulp_wdt.c | 29 +++++++++++++++++++++++++++++
>>> 1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/imx7ulp_wdt.c
>>> b/drivers/watchdog/imx7ulp_wdt.c index 7993c8c..7d2b12e 100644
>>> --- a/drivers/watchdog/imx7ulp_wdt.c
>>> +++ b/drivers/watchdog/imx7ulp_wdt.c
>>> @@ -5,6 +5,7 @@
>>>
>>> #include <linux/clk.h>
>>> #include <linux/io.h>
>>> +#include <linux/iopoll.h>
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> @@ -36,6 +37,7 @@
>>> #define DEFAULT_TIMEOUT 60
>>> #define MAX_TIMEOUT 128
>>> #define WDOG_CLOCK_RATE 1000
>>> +#define WDOG_WAIT_TIMEOUT 10000
>>>
>>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> module_param(nowayout,
>>> bool, 0000); @@ -48,17 +50,31 @@ struct imx7ulp_wdt_device {
>>> struct clk *clk;
>>> };
>>>
>>> +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
>>> + u32 val = readl(base + WDOG_CS);
>>> +
>>> + if (!(val & mask))
>>> + WARN_ON(readl_poll_timeout_atomic(base + WDOG_CS, val,
>>> + val & mask, 0,
>>> + WDOG_WAIT_TIMEOUT));
>>> +}
>>> +
>>> static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool
>>> enable) {
>>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>>>
>>> u32 val = readl(wdt->base + WDOG_CS);
>>>
>>> + local_irq_disable();
>>> writel(UNLOCK, wdt->base + WDOG_CNT);
>>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
>>> if (enable)
>>> writel(val | WDOG_CS_EN, wdt->base + WDOG_CS);
>>> else
>>> writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
>>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
>>> + local_irq_enable();
>>> }
>>>
>>> static bool imx7ulp_wdt_is_enabled(void __iomem *base) @@ -72,7
>>> +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) {
>>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>>>
>>> + local_irq_disable();
>>> + writel(UNLOCK, wdt->base + WDOG_CNT);
>>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
>>> writel(REFRESH, wdt->base + WDOG_CNT);
>>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
>>
>> Per reference manual (section 59.5.4), the waits are not required here, and
>> neither is the unlock. For practical purposes, disabling interrupts is useless as
>> well since the refresh write operation is just a single register write.
>
> Correct, the example in reference manual does NOT have this flow for refresh, but
> I checked with our design team yestoday, their validation code indeed has this flow,
> that is why I added it for refresh operation as well.
If it is needed, they'll need to update the manual. Urgently.
Really, missing the information that the watchdog must be unlocked
in order to refresh the timer would not just be be a minor flaw
in the manual. Either it is needed and must be mentioned (because
the watchdog would otherwise simply not work), or it isn't needed
and should not be done.
Thanks,
Guenter
> I will do a test on our EVK board, and if it works without this flow, I will remove them
> In V3.
>
> Thanks,
> Anson
>
>
>
>
next prev parent reply other threads:[~2020-07-29 15:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-29 2:20 [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Anson Huang
2020-07-29 2:20 ` [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode Anson Huang
2020-07-29 4:15 ` [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Guenter Roeck
2020-07-29 4:50 ` Anson Huang
2020-07-29 5:02 ` Anson Huang
2020-07-29 14:55 ` Guenter Roeck
2020-07-29 15:13 ` Anson Huang
2020-07-29 14:13 ` Guenter Roeck
2020-07-29 14:17 ` Anson Huang
2020-07-29 15:15 ` Guenter Roeck
2020-07-29 15:32 ` Anson Huang
2020-07-29 15:49 ` Guenter Roeck [this message]
2020-07-30 2:04 ` Anson Huang
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=2e962b44-d1eb-f060-f1d2-a079322d4c22@roeck-us.net \
--to=linux@roeck-us.net \
--cc=anson.huang@nxp.com \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=wim@linux-watchdog.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).