2022-08-25 08:47:36

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver

From: Alice Guo <[email protected]>

Alice Guo (1):
watchdog: imx93: add watchdog timer on imx93

Anson Huang (1):
watchdog: imx7ulp: Move suspend/resume to noirq phase

Jacky Bai (1):
watchdog: imx7ulp: Add explict memory barrier for unlock sequence

Jason Liu (1):
watchdog: imx7ulp_wdt: init wdog when it was active

Ye Li (3):
watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
watchdog: imx7ulp_wdt: Fix RCS timeout issue
watchdog: imx7ulp_wdt: Handle wdog reconfigure failure

drivers/watchdog/imx7ulp_wdt.c | 212 ++++++++++++++++++++++++++-------
1 file changed, 168 insertions(+), 44 deletions(-)

--
2.17.1


2022-08-25 08:47:47

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence

From: Jacky Bai <[email protected]>

When reconfiguring the WDOG Timer of i.MX7ULP, there is a certain
probability causes it to reset. The reason is that the CMD32EN of the
WDOG Timer of i.MX7ULP is disabled in bootloader. The unlock sequence
are two 16-bit writes to the CNT register within 16 bus clocks. Adding
mb() is to guarantee that two 16-bit writes are finished within 16 bus
clocks. Memory barriers cannot be added between these two 16-bit writes
so that writel_relaxed is used.

Suggested-by: Ye Li <[email protected]>
Signed-off-by: Jacky Bai <[email protected]>
Signed-off-by: Alice Guo <[email protected]>
Reviewed-by: Ye Li <[email protected]>
---

Changes for v2:
- add the reason why memory barriers are added for unlock sequence in commit log

drivers/watchdog/imx7ulp_wdt.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 014f497ea0dc..b8ac0cb04d2f 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -179,9 +179,13 @@ static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
int ret;

local_irq_disable();
+
+ mb();
/* unlock the wdog for reconfiguration */
writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+ mb();
+
ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
if (ret)
goto init_out;
--
2.17.1

2022-08-25 08:48:18

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue

From: Ye Li <[email protected]>

According to measure on i.MX7ULP and i.MX8ULP, the RCS done needs
about 3400us and 6700us respectively. So current 20us timeout is
not enough. When reconfiguring is on-going, unlock and configure CS
will lead to unknown result.

Increase the wait timeout value to 10ms and check the return value
of RCS wait to fix the issue

Signed-off-by: Ye Li <[email protected]>
Signed-off-by: Alice Guo <[email protected]>
Reviewed-by: Jacky Bai <[email protected]>
Acked-by: Jason Liu <[email protected]>
---

Changes for v2:
- none

drivers/watchdog/imx7ulp_wdt.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index a0f6b8cea78f..12715c248688 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -39,7 +39,7 @@
#define DEFAULT_TIMEOUT 60
#define MAX_TIMEOUT 128
#define WDOG_CLOCK_RATE 1000
-#define WDOG_WAIT_TIMEOUT 20
+#define WDOG_WAIT_TIMEOUT 10000

static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0000);
@@ -80,7 +80,7 @@ static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool 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);
+ ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);

enable_out:
local_irq_enable();
@@ -127,7 +127,9 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
if (ret)
goto timeout_out;
writel(val, wdt->base + WDOG_TOVAL);
- imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
+ ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
+ if (ret)
+ goto timeout_out;

wdog->timeout = timeout;

--
2.17.1

2022-08-25 08:48:24

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 7/7] watchdog: imx93: add watchdog timer on imx93

From: Alice Guo <[email protected]>

The WDOG clocks are sourced from lpo_clk, and lpo_clk is the fixed
32KHz. TOVAL contains the 16-bit value used to set the timeout period of
the watchdog. When the timeout period exceeds 2 seconds, the value
written to the TOVAL register is larger than 16-bit can represent.
Enabling watchdog prescaler can solve this problem.

Two points need to be aware of:
1. watchdog prescaler enables a fixed 256 pre-scaling of watchdog
counter reference clock
2. reconfiguration takes about 55ms on imx93

Reviewed-by: Jacky Bai <[email protected]>
Signed-off-by: Alice Guo <[email protected]>
---

Changes for v2:
- none

drivers/watchdog/imx7ulp_wdt.c | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index dee02c2a52c9..2897902090b3 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -9,6 +9,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
#include <linux/watchdog.h>
@@ -52,11 +53,17 @@ module_param(nowayout, bool, 0000);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

+struct imx_wdt_hw_feature {
+ bool prescaler_enable;
+ u32 wdog_clock_rate;
+};
+
struct imx7ulp_wdt_device {
struct watchdog_device wdd;
void __iomem *base;
struct clk *clk;
bool post_rcs_wait;
+ const struct imx_wdt_hw_feature *hw;
};

static int imx7ulp_wdt_wait_ulk(void __iomem *base)
@@ -179,7 +186,7 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int timeout)
{
struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
- u32 toval = WDOG_CLOCK_RATE * timeout;
+ u32 toval = wdt->hw->wdog_clock_rate * timeout;
u32 val;
int ret;
u32 loop = RETRY_MAX;
@@ -276,6 +283,9 @@ static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout
int ret;
u32 loop = RETRY_MAX;

+ if (wdt->hw->prescaler_enable)
+ val |= WDOG_CS_PRES;
+
do {
ret = _imx7ulp_wdt_init(wdt, timeout, val);
toval = readl(wdt->base + WDOG_TOVAL);
@@ -346,7 +356,9 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev)
watchdog_stop_on_reboot(wdog);
watchdog_stop_on_unregister(wdog);
watchdog_set_drvdata(wdog, imx7ulp_wdt);
- ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * WDOG_CLOCK_RATE);
+
+ imx7ulp_wdt->hw = of_device_get_match_data(dev);
+ ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * imx7ulp_wdt->hw->wdog_clock_rate);
if (ret)
return ret;

@@ -368,7 +380,7 @@ static int __maybe_unused imx7ulp_wdt_suspend_noirq(struct device *dev)
static int __maybe_unused imx7ulp_wdt_resume_noirq(struct device *dev)
{
struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
- u32 timeout = imx7ulp_wdt->wdd.timeout * WDOG_CLOCK_RATE;
+ u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->hw->wdog_clock_rate;
int ret;

ret = clk_prepare_enable(imx7ulp_wdt->clk);
@@ -389,9 +401,20 @@ static const struct dev_pm_ops imx7ulp_wdt_pm_ops = {
imx7ulp_wdt_resume_noirq)
};

+static const struct imx_wdt_hw_feature imx7ulp_wdt_hw = {
+ .prescaler_enable = false,
+ .wdog_clock_rate = 1000,
+};
+
+static const struct imx_wdt_hw_feature imx93_wdt_hw = {
+ .prescaler_enable = true,
+ .wdog_clock_rate = 125,
+};
+
static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
- { .compatible = "fsl,imx8ulp-wdt", },
- { .compatible = "fsl,imx7ulp-wdt", },
+ { .compatible = "fsl,imx8ulp-wdt", .data = &imx7ulp_wdt_hw, },
+ { .compatible = "fsl,imx7ulp-wdt", .data = &imx7ulp_wdt_hw, },
+ { .compatible = "fsl,imx93-wdt", .data = &imx93_wdt_hw, },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
--
2.17.1

2022-08-25 08:49:02

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 6/7] watchdog: imx7ulp_wdt: init wdog when it was active

From: Jason Liu <[email protected]>

Paired with suspend, we can only init wdog again when it was active
and ping it once to avoid the watchdog timeout after it resumed.

Signed-off-by: Jason Liu <[email protected]>
Signed-off-by: Alice Guo <[email protected]>
Reviewed-by: Ye Li <[email protected]>
Reviewed-by: Jacky Bai <[email protected]>
Tested-by: Jacky Bai <[email protected]>
---

Changes for v2:
- none

drivers/watchdog/imx7ulp_wdt.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 0cafa86fff7f..dee02c2a52c9 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -136,13 +136,6 @@ static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
return ret;
}

-static bool imx7ulp_wdt_is_enabled(void __iomem *base)
-{
- u32 val = readl(base + WDOG_CS);
-
- return val & WDOG_CS_EN;
-}
-
static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
{
struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
@@ -382,11 +375,11 @@ static int __maybe_unused imx7ulp_wdt_resume_noirq(struct device *dev)
if (ret)
return ret;

- if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
+ if (watchdog_active(&imx7ulp_wdt->wdd)) {
imx7ulp_wdt_init(imx7ulp_wdt, timeout);
-
- if (watchdog_active(&imx7ulp_wdt->wdd))
imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
+ imx7ulp_wdt_ping(&imx7ulp_wdt->wdd);
+ }

return 0;
}
--
2.17.1

2022-08-25 08:51:40

by Alice Guo (OSS)

[permalink] [raw]
Subject: [PATCH v2 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init

From: Ye Li <[email protected]>

When bootloader has enabled the CMD32EN bit, switch to use 32bits
unlock command to unlock the CS register. Using 32bits command will
help on avoiding 16 bus cycle window violation for two 16 bits
commands.

Signed-off-by: Ye Li <[email protected]>
Signed-off-by: Alice Guo <[email protected]>
Reviewed-by: Jacky Bai <[email protected]>
Acked-by: Jason Liu <[email protected]>
---

Changes for v2:
- none

drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index b8ac0cb04d2f..a0f6b8cea78f 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -180,11 +180,16 @@ static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)

local_irq_disable();

- mb();
- /* unlock the wdog for reconfiguration */
- writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
- writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
- mb();
+ val = readl(base + WDOG_CS);
+ if (val & WDOG_CS_CMD32EN) {
+ writel(UNLOCK, base + WDOG_CNT);
+ } else {
+ mb();
+ /* unlock the wdog for reconfiguration */
+ writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
+ writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+ mb();
+ }

ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
if (ret)
--
2.17.1

2022-09-06 02:30:34

by Alice Guo (OSS)

[permalink] [raw]
Subject: RE: [PATCH v2 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver

A friendly ping...

Best Regards,
Alice Guo

> -----Original Message-----
> From: Alice Guo (OSS) <[email protected]>
> Sent: Thursday, August 25, 2022 4:33 PM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; [email protected];
> [email protected]
> Subject: [PATCH v2 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG
> timer driver
>
> From: Alice Guo <[email protected]>
>
> Alice Guo (1):
> watchdog: imx93: add watchdog timer on imx93
>
> Anson Huang (1):
> watchdog: imx7ulp: Move suspend/resume to noirq phase
>
> Jacky Bai (1):
> watchdog: imx7ulp: Add explict memory barrier for unlock sequence
>
> Jason Liu (1):
> watchdog: imx7ulp_wdt: init wdog when it was active
>
> Ye Li (3):
> watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
> watchdog: imx7ulp_wdt: Fix RCS timeout issue
> watchdog: imx7ulp_wdt: Handle wdog reconfigure failure
>
> drivers/watchdog/imx7ulp_wdt.c | 212
> ++++++++++++++++++++++++++-------
> 1 file changed, 168 insertions(+), 44 deletions(-)
>
> --
> 2.17.1

2022-09-25 17:44:00

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence

On Thu, Aug 25, 2022 at 04:32:51PM +0800, Alice Guo (OSS) wrote:
> From: Jacky Bai <[email protected]>
>
> When reconfiguring the WDOG Timer of i.MX7ULP, there is a certain
> probability causes it to reset. The reason is that the CMD32EN of the
> WDOG Timer of i.MX7ULP is disabled in bootloader. The unlock sequence
> are two 16-bit writes to the CNT register within 16 bus clocks. Adding
> mb() is to guarantee that two 16-bit writes are finished within 16 bus
> clocks. Memory barriers cannot be added between these two 16-bit writes
> so that writel_relaxed is used.
>
> Suggested-by: Ye Li <[email protected]>
> Signed-off-by: Jacky Bai <[email protected]>
> Signed-off-by: Alice Guo <[email protected]>
> Reviewed-by: Ye Li <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Changes for v2:
> - add the reason why memory barriers are added for unlock sequence in commit log
>
> drivers/watchdog/imx7ulp_wdt.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 014f497ea0dc..b8ac0cb04d2f 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -179,9 +179,13 @@ static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
> int ret;
>
> local_irq_disable();
> +
> + mb();
> /* unlock the wdog for reconfiguration */
> writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> + mb();
> +
> ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> if (ret)
> goto init_out;
> --
> 2.17.1
>

2022-09-25 17:46:19

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 6/7] watchdog: imx7ulp_wdt: init wdog when it was active

On Thu, Aug 25, 2022 at 04:32:55PM +0800, Alice Guo (OSS) wrote:
> From: Jason Liu <[email protected]>
>
> Paired with suspend, we can only init wdog again when it was active
> and ping it once to avoid the watchdog timeout after it resumed.
>
> Signed-off-by: Jason Liu <[email protected]>
> Signed-off-by: Alice Guo <[email protected]>
> Reviewed-by: Ye Li <[email protected]>
> Reviewed-by: Jacky Bai <[email protected]>
> Tested-by: Jacky Bai <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Changes for v2:
> - none
>
> drivers/watchdog/imx7ulp_wdt.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 0cafa86fff7f..dee02c2a52c9 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -136,13 +136,6 @@ static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
> return ret;
> }
>
> -static bool imx7ulp_wdt_is_enabled(void __iomem *base)
> -{
> - u32 val = readl(base + WDOG_CS);
> -
> - return val & WDOG_CS_EN;
> -}
> -
> static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
> {
> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> @@ -382,11 +375,11 @@ static int __maybe_unused imx7ulp_wdt_resume_noirq(struct device *dev)
> if (ret)
> return ret;
>
> - if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
> + if (watchdog_active(&imx7ulp_wdt->wdd)) {
> imx7ulp_wdt_init(imx7ulp_wdt, timeout);
> -
> - if (watchdog_active(&imx7ulp_wdt->wdd))
> imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
> + imx7ulp_wdt_ping(&imx7ulp_wdt->wdd);
> + }
>
> return 0;
> }
> --
> 2.17.1
>

2022-09-25 17:46:24

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init

On Thu, Aug 25, 2022 at 04:32:52PM +0800, Alice Guo (OSS) wrote:
> From: Ye Li <[email protected]>
>
> When bootloader has enabled the CMD32EN bit, switch to use 32bits
> unlock command to unlock the CS register. Using 32bits command will
> help on avoiding 16 bus cycle window violation for two 16 bits
> commands.
>
> Signed-off-by: Ye Li <[email protected]>
> Signed-off-by: Alice Guo <[email protected]>
> Reviewed-by: Jacky Bai <[email protected]>
> Acked-by: Jason Liu <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Changes for v2:
> - none
>
> drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index b8ac0cb04d2f..a0f6b8cea78f 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -180,11 +180,16 @@ static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
>
> local_irq_disable();
>
> - mb();
> - /* unlock the wdog for reconfiguration */
> - writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> - writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> - mb();
> + val = readl(base + WDOG_CS);
> + if (val & WDOG_CS_CMD32EN) {
> + writel(UNLOCK, base + WDOG_CNT);
> + } else {
> + mb();
> + /* unlock the wdog for reconfiguration */
> + writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> + writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> + mb();
> + }
>
> ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> if (ret)
> --
> 2.17.1
>

2022-09-25 17:54:37

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue

On Thu, Aug 25, 2022 at 04:32:53PM +0800, Alice Guo (OSS) wrote:
> From: Ye Li <[email protected]>
>
> According to measure on i.MX7ULP and i.MX8ULP, the RCS done needs
> about 3400us and 6700us respectively. So current 20us timeout is
> not enough. When reconfiguring is on-going, unlock and configure CS
> will lead to unknown result.
>
> Increase the wait timeout value to 10ms and check the return value
> of RCS wait to fix the issue
>
> Signed-off-by: Ye Li <[email protected]>
> Signed-off-by: Alice Guo <[email protected]>
> Reviewed-by: Jacky Bai <[email protected]>
> Acked-by: Jason Liu <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Changes for v2:
> - none
>
> drivers/watchdog/imx7ulp_wdt.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index a0f6b8cea78f..12715c248688 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -39,7 +39,7 @@
> #define DEFAULT_TIMEOUT 60
> #define MAX_TIMEOUT 128
> #define WDOG_CLOCK_RATE 1000
> -#define WDOG_WAIT_TIMEOUT 20
> +#define WDOG_WAIT_TIMEOUT 10000
>
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0000);
> @@ -80,7 +80,7 @@ static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool 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);
> + ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
>
> enable_out:
> local_irq_enable();
> @@ -127,7 +127,9 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> if (ret)
> goto timeout_out;
> writel(val, wdt->base + WDOG_TOVAL);
> - imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
> + ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
> + if (ret)
> + goto timeout_out;
>
> wdog->timeout = timeout;
>
> --
> 2.17.1
>

2022-09-25 17:54:53

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 7/7] watchdog: imx93: add watchdog timer on imx93

On Thu, Aug 25, 2022 at 04:32:56PM +0800, Alice Guo (OSS) wrote:
> From: Alice Guo <[email protected]>
>
> The WDOG clocks are sourced from lpo_clk, and lpo_clk is the fixed
> 32KHz. TOVAL contains the 16-bit value used to set the timeout period of
> the watchdog. When the timeout period exceeds 2 seconds, the value
> written to the TOVAL register is larger than 16-bit can represent.
> Enabling watchdog prescaler can solve this problem.
>
> Two points need to be aware of:
> 1. watchdog prescaler enables a fixed 256 pre-scaling of watchdog
> counter reference clock
> 2. reconfiguration takes about 55ms on imx93
>
> Reviewed-by: Jacky Bai <[email protected]>
> Signed-off-by: Alice Guo <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Changes for v2:
> - none
>
> drivers/watchdog/imx7ulp_wdt.c | 33 ++++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index dee02c2a52c9..2897902090b3 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -9,6 +9,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> #include <linux/watchdog.h>
> @@ -52,11 +53,17 @@ module_param(nowayout, bool, 0000);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +struct imx_wdt_hw_feature {
> + bool prescaler_enable;
> + u32 wdog_clock_rate;
> +};
> +
> struct imx7ulp_wdt_device {
> struct watchdog_device wdd;
> void __iomem *base;
> struct clk *clk;
> bool post_rcs_wait;
> + const struct imx_wdt_hw_feature *hw;
> };
>
> static int imx7ulp_wdt_wait_ulk(void __iomem *base)
> @@ -179,7 +186,7 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> unsigned int timeout)
> {
> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> - u32 toval = WDOG_CLOCK_RATE * timeout;
> + u32 toval = wdt->hw->wdog_clock_rate * timeout;
> u32 val;
> int ret;
> u32 loop = RETRY_MAX;
> @@ -276,6 +283,9 @@ static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout
> int ret;
> u32 loop = RETRY_MAX;
>
> + if (wdt->hw->prescaler_enable)
> + val |= WDOG_CS_PRES;
> +
> do {
> ret = _imx7ulp_wdt_init(wdt, timeout, val);
> toval = readl(wdt->base + WDOG_TOVAL);
> @@ -346,7 +356,9 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev)
> watchdog_stop_on_reboot(wdog);
> watchdog_stop_on_unregister(wdog);
> watchdog_set_drvdata(wdog, imx7ulp_wdt);
> - ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * WDOG_CLOCK_RATE);
> +
> + imx7ulp_wdt->hw = of_device_get_match_data(dev);
> + ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * imx7ulp_wdt->hw->wdog_clock_rate);
> if (ret)
> return ret;
>
> @@ -368,7 +380,7 @@ static int __maybe_unused imx7ulp_wdt_suspend_noirq(struct device *dev)
> static int __maybe_unused imx7ulp_wdt_resume_noirq(struct device *dev)
> {
> struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> - u32 timeout = imx7ulp_wdt->wdd.timeout * WDOG_CLOCK_RATE;
> + u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->hw->wdog_clock_rate;
> int ret;
>
> ret = clk_prepare_enable(imx7ulp_wdt->clk);
> @@ -389,9 +401,20 @@ static const struct dev_pm_ops imx7ulp_wdt_pm_ops = {
> imx7ulp_wdt_resume_noirq)
> };
>
> +static const struct imx_wdt_hw_feature imx7ulp_wdt_hw = {
> + .prescaler_enable = false,
> + .wdog_clock_rate = 1000,
> +};
> +
> +static const struct imx_wdt_hw_feature imx93_wdt_hw = {
> + .prescaler_enable = true,
> + .wdog_clock_rate = 125,
> +};
> +
> static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> - { .compatible = "fsl,imx8ulp-wdt", },
> - { .compatible = "fsl,imx7ulp-wdt", },
> + { .compatible = "fsl,imx8ulp-wdt", .data = &imx7ulp_wdt_hw, },
> + { .compatible = "fsl,imx7ulp-wdt", .data = &imx7ulp_wdt_hw, },
> + { .compatible = "fsl,imx93-wdt", .data = &imx93_wdt_hw, },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
> --
> 2.17.1
>