linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver
@ 2022-08-16  4:36 Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 1/7] watchdog: imx7ulp: Move suspend/resume to noirq phase Alice Guo (OSS)
                   ` (6 more replies)
  0 siblings, 7 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Alice Guo <alice.guo@nxp.com>

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/7] watchdog: imx7ulp: Move suspend/resume to noirq phase
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence Alice Guo (OSS)
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Anson Huang <Anson.Huang@nxp.com>

The i.MX7ULP's watchdog is enabled by default when out of reset, so the
resume callback which is to disable watchdog should be called earlier
to avoid unexpected timeout, move suspend/resume callback to noirq phase.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Tested-by: Peter Chen <peter.chen@nxp.com>
Tested-by: Li Jun <jun.li@nxp.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 922b60374295..014f497ea0dc 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -255,7 +255,7 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev)
 	return devm_watchdog_register_device(dev, wdog);
 }
 
-static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
+static int __maybe_unused imx7ulp_wdt_suspend_noirq(struct device *dev)
 {
 	struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
 
@@ -267,7 +267,7 @@ static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused imx7ulp_wdt_resume(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;
@@ -286,8 +286,10 @@ static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
-			 imx7ulp_wdt_resume);
+static const struct dev_pm_ops imx7ulp_wdt_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx7ulp_wdt_suspend_noirq,
+				      imx7ulp_wdt_resume_noirq)
+};
 
 static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
 	{ .compatible = "fsl,imx7ulp-wdt", },
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 1/7] watchdog: imx7ulp: Move suspend/resume to noirq phase Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-16  6:23   ` Marco Felsch
  2022-08-16  4:36 ` [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init Alice Guo (OSS)
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Jacky Bai <ping.bai@nxp.com>

Add explict memory barrier for the wdog unlock sequence.

Suggested-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Ye Li <ye.li@nxp.com>
---
 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 1/7] watchdog: imx7ulp: Move suspend/resume to noirq phase Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-22 14:05   ` Guenter Roeck
  2022-08-16  4:36 ` [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue Alice Guo (OSS)
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Ye Li <ye.li@nxp.com>

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 <ye.li@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Acked-by: Jason Liu <jason.hui.liu@nxp.com>
---
 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
                   ` (2 preceding siblings ...)
  2022-08-16  4:36 ` [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-22 14:09   ` Guenter Roeck
  2022-08-16  4:36 ` [PATCH 5/7] watchdog: imx7ulp_wdt: Handle wdog reconfigure failure Alice Guo (OSS)
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Ye Li <ye.li@nxp.com>

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 <ye.li@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Acked-by: Jason Liu <jason.hui.liu@nxp.com>
---
 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 5/7] watchdog: imx7ulp_wdt: Handle wdog reconfigure failure
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
                   ` (3 preceding siblings ...)
  2022-08-16  4:36 ` [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 6/7] watchdog: imx7ulp_wdt: init wdog when it was active Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 7/7] watchdog: imx93: add watchdog timer on imx93 Alice Guo (OSS)
  6 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Ye Li <ye.li@nxp.com>

Current driver may meet reconfigure failure caused by below reasons:

1. The wdog on iMX7ULP has different behavior after RCS valid. It needs
   to wait more than 2.5 wdog clock for clock sync before next
   reconfiguration, while imx8ulp wdog does not need such delay.

2. After unlock, there is 128 bus clock window opened for reconfiguration,
   but on iMX8ULP, the HW can't guarantee the latency. So it is possible
   the window is closed before the writing arrives to wdog.

3. If the PRES is enabled, the RCS valid time becomes x256 to the time
   of PRES disabled. It is about 1715ms on iMX8ULP. So We have to increase
   the RCS timeout and can't wait it in IRQ disabled.

The patch updates the driver to handle failures

1. Using different wait for unlock and RCS. Unlock valid time is very short
   and only related to bus clock. It must be in IRQ disabled to avoid
   being interrupted in 128 clock window. But for RCS time, it is longer
   and ok for IRQ enabled.

2. Add retry for any reconfigure failure with default 5 times.

3. Add "fsl,imx8ulp-wdt" compatile string for iMX8ULP and afterwards
   platform which don't need more 2.5 wdog clock after RCS valid.
   For imx7ulp, add post delay of 2.5 clock after RCS valid.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 163 ++++++++++++++++++++++++++-------
 1 file changed, 129 insertions(+), 34 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 12715c248688..51eaaf24bd8f 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -14,7 +14,9 @@
 #include <linux/watchdog.h>
 
 #define WDOG_CS			0x0
+#define WDOG_CS_FLG		BIT(14)
 #define WDOG_CS_CMD32EN		BIT(13)
+#define WDOG_CS_PRES		BIT(12)
 #define WDOG_CS_ULK		BIT(11)
 #define WDOG_CS_RCS		BIT(10)
 #define LPO_CLK			0x1
@@ -39,7 +41,11 @@
 #define DEFAULT_TIMEOUT	60
 #define MAX_TIMEOUT	128
 #define WDOG_CLOCK_RATE	1000
-#define WDOG_WAIT_TIMEOUT	10000
+#define WDOG_ULK_WAIT_TIMEOUT	1000
+#define WDOG_RCS_WAIT_TIMEOUT	10000
+#define WDOG_RCS_POST_WAIT 3000
+
+#define RETRY_MAX 5
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0000);
@@ -50,40 +56,82 @@ struct imx7ulp_wdt_device {
 	struct watchdog_device wdd;
 	void __iomem *base;
 	struct clk *clk;
+	bool post_rcs_wait;
 };
 
-static int imx7ulp_wdt_wait(void __iomem *base, u32 mask)
+static int imx7ulp_wdt_wait_ulk(void __iomem *base)
 {
 	u32 val = readl(base + WDOG_CS);
 
-	if (!(val & mask) && readl_poll_timeout_atomic(base + WDOG_CS, val,
-						       val & mask, 0,
-						       WDOG_WAIT_TIMEOUT))
+	if (!(val & WDOG_CS_ULK) &&
+	    readl_poll_timeout_atomic(base + WDOG_CS, val,
+				      val & WDOG_CS_ULK, 0,
+				      WDOG_ULK_WAIT_TIMEOUT))
 		return -ETIMEDOUT;
 
 	return 0;
 }
 
-static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
+static int imx7ulp_wdt_wait_rcs(struct imx7ulp_wdt_device *wdt)
 {
-	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+	int ret = 0;
+	u32 val = readl(wdt->base + WDOG_CS);
+	u64 timeout = (val & WDOG_CS_PRES) ?
+		WDOG_RCS_WAIT_TIMEOUT * 256 : WDOG_RCS_WAIT_TIMEOUT;
+	unsigned long wait_min = (val & WDOG_CS_PRES) ?
+		WDOG_RCS_POST_WAIT * 256 : WDOG_RCS_POST_WAIT;
 
+	if (!(val & WDOG_CS_RCS) &&
+	    readl_poll_timeout(wdt->base + WDOG_CS, val, val & WDOG_CS_RCS, 0,
+			       timeout))
+		ret = -ETIMEDOUT;
+
+	/* Wait 2.5 clocks after RCS done */
+	if (wdt->post_rcs_wait)
+		usleep_range(wait_min, wait_min + 2000);
+
+	return ret;
+}
+
+static int _imx7ulp_wdt_enable(struct imx7ulp_wdt_device *wdt, bool enable)
+{
 	u32 val = readl(wdt->base + WDOG_CS);
 	int ret;
 
 	local_irq_disable();
 	writel(UNLOCK, wdt->base + WDOG_CNT);
-	ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
+	ret = imx7ulp_wdt_wait_ulk(wdt->base);
 	if (ret)
 		goto enable_out;
 	if (enable)
 		writel(val | WDOG_CS_EN, wdt->base + WDOG_CS);
 	else
 		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
-	ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
+
+	local_irq_enable();
+	ret = imx7ulp_wdt_wait_rcs(wdt);
+
+	return ret;
 
 enable_out:
 	local_irq_enable();
+	return ret;
+}
+
+static int imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+	int ret;
+	u32 val;
+	u32 loop = RETRY_MAX;
+
+	do {
+		ret = _imx7ulp_wdt_enable(wdt, enable);
+		val = readl(wdt->base + WDOG_CS);
+	} while (--loop > 0 && ((!!(val & WDOG_CS_EN)) != enable || ret));
+
+	if (loop == 0)
+		return -EBUSY;
 
 	return ret;
 }
@@ -114,28 +162,44 @@ static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
 	return imx7ulp_wdt_enable(wdog, false);
 }
 
-static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
-				   unsigned int timeout)
+static int _imx7ulp_wdt_set_timeout(struct imx7ulp_wdt_device *wdt,
+				   unsigned int toval)
 {
-	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
-	u32 val = WDOG_CLOCK_RATE * timeout;
 	int ret;
 
 	local_irq_disable();
 	writel(UNLOCK, wdt->base + WDOG_CNT);
-	ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK);
+	ret = imx7ulp_wdt_wait_ulk(wdt->base);
 	if (ret)
 		goto timeout_out;
-	writel(val, wdt->base + WDOG_TOVAL);
-	ret = imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS);
-	if (ret)
-		goto timeout_out;
-
-	wdog->timeout = timeout;
+	writel(toval, wdt->base + WDOG_TOVAL);
+	local_irq_enable();
+	ret = imx7ulp_wdt_wait_rcs(wdt);
+	return ret;
 
 timeout_out:
 	local_irq_enable();
+	return ret;
+}
 
+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 val;
+	int ret;
+	u32 loop = RETRY_MAX;
+
+	do {
+		ret = _imx7ulp_wdt_set_timeout(wdt, toval);
+		val = readl(wdt->base + WDOG_TOVAL);
+	} while (--loop > 0 && (val != toval || ret));
+
+	if (loop == 0)
+		return -EBUSY;
+
+	wdog->timeout = timeout;
 	return ret;
 }
 
@@ -175,38 +239,59 @@ static const struct watchdog_info imx7ulp_wdt_info = {
 		    WDIOF_MAGICCLOSE,
 };
 
-static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout, unsigned int cs)
 {
 	u32 val;
 	int ret;
 
 	local_irq_disable();
 
-	val = readl(base + WDOG_CS);
+	val = readl(wdt->base + WDOG_CS);
 	if (val & WDOG_CS_CMD32EN) {
-		writel(UNLOCK, base + WDOG_CNT);
+		writel(UNLOCK, wdt->base + WDOG_CNT);
 	} else {
 		mb();
 		/* unlock the wdog for reconfiguration */
-		writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
-		writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+		writel_relaxed(UNLOCK_SEQ0, wdt->base + WDOG_CNT);
+		writel_relaxed(UNLOCK_SEQ1, wdt->base + WDOG_CNT);
 		mb();
 	}
 
-	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
+	ret = imx7ulp_wdt_wait_ulk(wdt->base);
 	if (ret)
 		goto init_out;
 
 	/* set an initial timeout value in TOVAL */
-	writel(timeout, base + WDOG_TOVAL);
-	/* enable 32bit command sequence and reconfigure */
-	val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE |
-	      WDOG_CS_WAIT | WDOG_CS_STOP;
-	writel(val, base + WDOG_CS);
-	imx7ulp_wdt_wait(base, WDOG_CS_RCS);
+	writel(timeout, wdt->base + WDOG_TOVAL);
+	writel(cs, wdt->base + WDOG_CS);
+	local_irq_enable();
+	ret = imx7ulp_wdt_wait_rcs(wdt);
+
+	return ret;
 
 init_out:
 	local_irq_enable();
+	return ret;
+}
+
+static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout)
+{
+	/* enable 32bit command sequence and reconfigure */
+	u32 val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE |
+		  WDOG_CS_WAIT | WDOG_CS_STOP;
+	u32 cs, toval;
+	int ret;
+	u32 loop = RETRY_MAX;
+
+	do {
+		ret = _imx7ulp_wdt_init(wdt, timeout, val);
+		toval = readl(wdt->base + WDOG_TOVAL);
+		cs = readl(wdt->base + WDOG_CS);
+		cs &= ~(WDOG_CS_FLG | WDOG_CS_ULK | WDOG_CS_RCS);
+	} while (--loop > 0 && (cs != val || toval != timeout || ret));
+
+	if (loop == 0)
+		return -EBUSY;
 
 	return ret;
 }
@@ -239,6 +324,15 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev)
 		return PTR_ERR(imx7ulp_wdt->clk);
 	}
 
+	imx7ulp_wdt->post_rcs_wait = true;
+	if (of_device_is_compatible(dev->of_node,
+				    "fsl,imx8ulp-wdt")) {
+		dev_info(dev, "imx8ulp wdt probe\n");
+		imx7ulp_wdt->post_rcs_wait = false;
+	} else {
+		dev_info(dev, "imx7ulp wdt probe\n");
+	}
+
 	ret = clk_prepare_enable(imx7ulp_wdt->clk);
 	if (ret)
 		return ret;
@@ -259,7 +353,7 @@ 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->base, wdog->timeout * WDOG_CLOCK_RATE);
+	ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * WDOG_CLOCK_RATE);
 	if (ret)
 		return ret;
 
@@ -289,7 +383,7 @@ static int __maybe_unused imx7ulp_wdt_resume_noirq(struct device *dev)
 		return ret;
 
 	if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
-		imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
+		imx7ulp_wdt_init(imx7ulp_wdt, timeout);
 
 	if (watchdog_active(&imx7ulp_wdt->wdd))
 		imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
@@ -303,6 +397,7 @@ static const struct dev_pm_ops imx7ulp_wdt_pm_ops = {
 };
 
 static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
+	{ .compatible = "fsl,imx8ulp-wdt", },
 	{ .compatible = "fsl,imx7ulp-wdt", },
 	{ /* sentinel */ }
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 6/7] watchdog: imx7ulp_wdt: init wdog when it was active
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
                   ` (4 preceding siblings ...)
  2022-08-16  4:36 ` [PATCH 5/7] watchdog: imx7ulp_wdt: Handle wdog reconfigure failure Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  2022-08-16  4:36 ` [PATCH 7/7] watchdog: imx93: add watchdog timer on imx93 Alice Guo (OSS)
  6 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Jason Liu <jason.hui.liu@nxp.com>

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 <jason.hui.liu@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Ye Li <ye.li@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Tested-by: Jacky Bai <ping.bai@nxp.com>
---
 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 51eaaf24bd8f..58508f69d933 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 7/7] watchdog: imx93: add watchdog timer on imx93
  2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
                   ` (5 preceding siblings ...)
  2022-08-16  4:36 ` [PATCH 6/7] watchdog: imx7ulp_wdt: init wdog when it was active Alice Guo (OSS)
@ 2022-08-16  4:36 ` Alice Guo (OSS)
  6 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-16  4:36 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, festevam
  Cc: kernel, linux-imx, linux-watchdog, linux-arm-kernel, linux-kernel

From: Alice Guo <alice.guo@nxp.com>

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 <ping.bai@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
---
 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 58508f69d933..4694aa4b8cfb 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-16  4:36 ` [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence Alice Guo (OSS)
@ 2022-08-16  6:23   ` Marco Felsch
  2022-08-22  7:49     ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-16  6:23 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, linux, shawnguo, s.hauer, festevam, linux-arm-kernel,
	linux-kernel, linux-imx, kernel, linux-watchdog

On 22-08-16, Alice Guo (OSS) wrote:
> From: Jacky Bai <ping.bai@nxp.com>
> 
> Add explict memory barrier for the wdog unlock sequence.

Did you inspected any failures? It's not enough to say what you did, you
need to specify the why as well.

Regards,
  Marco


> 
> Suggested-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Reviewed-by: Ye Li <ye.li@nxp.com>
> ---
>  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
> 
> 
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-16  6:23   ` Marco Felsch
@ 2022-08-22  7:49     ` Alice Guo (OSS)
  2022-08-22  8:00       ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-22  7:49 UTC (permalink / raw)
  To: Marco Felsch
  Cc: wim, linux, shawnguo, s.hauer, festevam, linux-arm-kernel,
	linux-kernel, dl-linux-imx, kernel, linux-watchdog

> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: Tuesday, August 16, 2022 2:24 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: wim@linux-watchdog.org; linux@roeck-us.net; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> On 22-08-16, Alice Guo (OSS) wrote:
> > From: Jacky Bai <ping.bai@nxp.com>
> >
> > Add explict memory barrier for the wdog unlock sequence.
> 
> Did you inspected any failures? It's not enough to say what you did, you need
> to specify the why as well.
> 
> Regards,
>   Marco

Hi,

Two 16-bit writes of unlocking the Watchdog should be completed within a certain time. The first mb() is used to ensure that previous instructions are completed.
The second mb() is used to ensure that the unlock sequence cannot be affected by subsequent instructions. The reason will be added in the commit log of v2.

Best Regards,
Alice Guo

> 
> >
> > Suggested-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > Reviewed-by: Ye Li <ye.li@nxp.com>
> > ---
> >  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
> >
> >
> >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-22  7:49     ` Alice Guo (OSS)
@ 2022-08-22  8:00       ` Marco Felsch
  2022-08-22 14:03         ` Guenter Roeck
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-22  8:00 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, linux, shawnguo, s.hauer, festevam, linux-arm-kernel,
	linux-kernel, dl-linux-imx, kernel, linux-watchdog

On 22-08-22, Alice Guo (OSS) wrote:
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@pengutronix.de>
> > Sent: Tuesday, August 16, 2022 2:24 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: wim@linux-watchdog.org; linux@roeck-us.net; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-watchdog@vger.kernel.org
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > On 22-08-16, Alice Guo (OSS) wrote:
> > > From: Jacky Bai <ping.bai@nxp.com>
> > >
> > > Add explict memory barrier for the wdog unlock sequence.
> > 
> > Did you inspected any failures? It's not enough to say what you did, you need
> > to specify the why as well.
> > 
> > Regards,
> >   Marco
> 
> Hi,
> 
> Two 16-bit writes of unlocking the Watchdog should be completed within a certain time. The first mb() is used to ensure that previous instructions are completed.
> The second mb() is used to ensure that the unlock sequence cannot be affected by subsequent instructions. The reason will be added in the commit log of v2.

Hi,

I know what memory barriers are. My question was, did you see any
issues? Since the driver is used mainline and no one reported issues.

Also just don't use the *_relaxed() versions is more common, than adding
mb() calls around *_relaxed() versions.

Regards,
  Marco

> 
> Best Regards,
> Alice Guo
> 
> > 
> > >
> > > Suggested-by: Ye Li <ye.li@nxp.com>
> > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > ---
> > >  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
> > >
> > >
> > >
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-22  8:00       ` Marco Felsch
@ 2022-08-22 14:03         ` Guenter Roeck
  2022-08-23  5:38           ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Guenter Roeck @ 2022-08-22 14:03 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Alice Guo (OSS),
	wim, shawnguo, s.hauer, festevam, linux-arm-kernel, linux-kernel,
	dl-linux-imx, kernel, linux-watchdog

On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:
> On 22-08-22, Alice Guo (OSS) wrote:
> > > -----Original Message-----
> > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > Cc: wim@linux-watchdog.org; linux@roeck-us.net; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; festevam@gmail.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-watchdog@vger.kernel.org
> > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > > unlock sequence
> > > 
> > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > From: Jacky Bai <ping.bai@nxp.com>
> > > >
> > > > Add explict memory barrier for the wdog unlock sequence.
> > > 
> > > Did you inspected any failures? It's not enough to say what you did, you need
> > > to specify the why as well.
> > > 
> > > Regards,
> > >   Marco
> > 
> > Hi,
> > 
> > Two 16-bit writes of unlocking the Watchdog should be completed within a certain time. The first mb() is used to ensure that previous instructions are completed.
> > The second mb() is used to ensure that the unlock sequence cannot be affected by subsequent instructions. The reason will be added in the commit log of v2.
> 
> Hi,
> 
> I know what memory barriers are. My question was, did you see any
> issues? Since the driver is used mainline and no one reported issues.
> 
> Also just don't use the *_relaxed() versions is more common, than adding
> mb() calls around *_relaxed() versions.
> 

Agreed with both. The series is a bit short in explaining _why_ the
changes are made.

Guenter

> Regards,
>   Marco
> 
> > 
> > Best Regards,
> > Alice Guo
> > 
> > > 
> > > >
> > > > Suggested-by: Ye Li <ye.li@nxp.com>
> > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > ---
> > > >  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
> > > >
> > > >
> > > >
> > 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
  2022-08-16  4:36 ` [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init Alice Guo (OSS)
@ 2022-08-22 14:05   ` Guenter Roeck
  2022-08-23  5:46     ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Guenter Roeck @ 2022-08-22 14:05 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel

On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> From: Ye Li <ye.li@nxp.com>
> 
> 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 <ye.li@nxp.com>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> ---
>  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();

Now this is intermixing writel() with writel_relaxed(), making
the code all but impossible to understand.

Guenter

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
  2022-08-16  4:36 ` [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue Alice Guo (OSS)
@ 2022-08-22 14:09   ` Guenter Roeck
  2022-08-23  5:59     ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Guenter Roeck @ 2022-08-22 14:09 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel

On Tue, Aug 16, 2022 at 12:36:40PM +0800, Alice Guo (OSS) wrote:
> From: Ye Li <ye.li@nxp.com>
> 
> 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
> 

You'll have to find a better solution. An active (non-sleep) wait of
10 ms is unacceptable.

Guenter

> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-22 14:03         ` Guenter Roeck
@ 2022-08-23  5:38           ` Alice Guo (OSS)
  2022-08-23  9:10             ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-23  5:38 UTC (permalink / raw)
  To: Guenter Roeck, Marco Felsch
  Cc: Alice Guo (OSS),
	wim, shawnguo, s.hauer, festevam, linux-arm-kernel, linux-kernel,
	dl-linux-imx, kernel, linux-watchdog



> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, August 22, 2022 10:04 PM
> To: Marco Felsch <m.felsch@pengutronix.de>
> Cc: Alice Guo (OSS) <alice.guo@oss.nxp.com>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:
> > On 22-08-22, Alice Guo (OSS) wrote:
> > > > -----Original Message-----
> > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > Cc: wim@linux-watchdog.org; linux@roeck-us.net;
> > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > > linux-arm-kernel@lists.infradead.org;
> > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > barrier for unlock sequence
> > > >
> > > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > > From: Jacky Bai <ping.bai@nxp.com>
> > > > >
> > > > > Add explict memory barrier for the wdog unlock sequence.
> > > >
> > > > Did you inspected any failures? It's not enough to say what you
> > > > did, you need to specify the why as well.
> > > >
> > > > Regards,
> > > >   Marco
> > >
> > > Hi,
> > >
> > > Two 16-bit writes of unlocking the Watchdog should be completed within a
> certain time. The first mb() is used to ensure that previous instructions are
> completed.
> > > The second mb() is used to ensure that the unlock sequence cannot be
> affected by subsequent instructions. The reason will be added in the commit
> log of v2.
> >
> > Hi,
> >
> > I know what memory barriers are. My question was, did you see any
> > issues? Since the driver is used mainline and no one reported issues.
> >
> > Also just don't use the *_relaxed() versions is more common, than
> > adding
> > mb() calls around *_relaxed() versions.
> >
> 
> Agreed with both. The series is a bit short in explaining _why_ the changes are
> made.
> 
> Guenter
> 
> > Regards,
> >   Marco
> >
> > >

Hi Guenter and Marco,

1. did you see any issues?
This WDOG Timer first appeared in i.MX7ULP, no one report issues probably because few people use i.MX7ULP. This issue was found when we did a stress test on it. When we reconfigure the WDOG Timer, there is a certain probability that it reset. The reason for the error is that when WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit writes (0xC520, 0xD928) to the CNT register within 16 bus clocks, and improper unlock sequence causes the WDOG to reset. Adding mb() is to guarantee that two 16-bit writes are finished within 16 bus clocks.

2. Also just don't use the *_relaxed() versions is more common, than adding mb() calls around *_relaxed() versions.
Memory barriers cannot be added between two 16-bit writes. I do not know the reason.

Best Regards,
Alice Guo

> > > Best Regards,
> > > Alice Guo
> > >
> > > >
> > > > >
> > > > > Suggested-by: Ye Li <ye.li@nxp.com>
> > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > > > Reviewed-by: Ye Li <ye.li@nxp.com>
> > > > > ---
> > > > >  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
> > > > >
> > > > >
> > > > >
> > >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
  2022-08-22 14:05   ` Guenter Roeck
@ 2022-08-23  5:46     ` Alice Guo (OSS)
  2022-08-23 12:05       ` Guenter Roeck
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-23  5:46 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, shawnguo, s.hauer, festevam, kernel, dl-linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel



> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, August 22, 2022 10:06 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog
> init
> 
> On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> > From: Ye Li <ye.li@nxp.com>
> >
> > 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 <ye.li@nxp.com>
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > ---
> >  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();
> 
> Now this is intermixing writel() with writel_relaxed(), making the code all but
> impossible to understand.
> 
> Guenter

Hi Guenter,

Intermixing writel() with writel_relaxed() is unavoidable here. Because there cannot be a memory barrier between writing UNLOCK_SEQ0 and writing UNLOCK_SEQ1. This may be determined by hardware design.

Best Regards,
Alice Guo
 
> > +	}
> >
> >  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> >  	if (ret)
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
  2022-08-22 14:09   ` Guenter Roeck
@ 2022-08-23  5:59     ` Alice Guo (OSS)
  2022-08-23 12:06       ` Guenter Roeck
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-23  5:59 UTC (permalink / raw)
  To: Guenter Roeck, Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, dl-linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel



> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, August 22, 2022 10:10 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
> 
> On Tue, Aug 16, 2022 at 12:36:40PM +0800, Alice Guo (OSS) wrote:
> > From: Ye Li <ye.li@nxp.com>
> >
> > 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
> >
> 
> You'll have to find a better solution. An active (non-sleep) wait of
> 10 ms is unacceptable.
> 
> Guenter

Hi Guenter,

Sorry. I think this patch should be merged with " watchdog: imx7ulp_wdt: Handle wdog reconfigure failure", but I didn't merge them. I will send v2.

Best Regards,
Alice Guo

> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > ---
> >  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
> >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-23  5:38           ` Alice Guo (OSS)
@ 2022-08-23  9:10             ` Marco Felsch
  2022-08-23 12:02               ` Guenter Roeck
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-23  9:10 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

On 22-08-23, Alice Guo (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, August 22, 2022 10:04 PM
> > To: Marco Felsch <m.felsch@pengutronix.de>
> > Cc: Alice Guo (OSS) <alice.guo@oss.nxp.com>; wim@linux-watchdog.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-watchdog@vger.kernel.org
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:
> > > On 22-08-22, Alice Guo (OSS) wrote:
> > > > > -----Original Message-----
> > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > Cc: wim@linux-watchdog.org; linux@roeck-us.net;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > > barrier for unlock sequence
> > > > >
> > > > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > > > From: Jacky Bai <ping.bai@nxp.com>
> > > > > >
> > > > > > Add explict memory barrier for the wdog unlock sequence.
> > > > >
> > > > > Did you inspected any failures? It's not enough to say what you
> > > > > did, you need to specify the why as well.
> > > > >
> > > > > Regards,
> > > > >   Marco
> > > >
> > > > Hi,
> > > >
> > > > Two 16-bit writes of unlocking the Watchdog should be completed within a
> > certain time. The first mb() is used to ensure that previous instructions are
> > completed.
> > > > The second mb() is used to ensure that the unlock sequence cannot be
> > affected by subsequent instructions. The reason will be added in the commit
> > log of v2.
> > >
> > > Hi,
> > >
> > > I know what memory barriers are. My question was, did you see any
> > > issues? Since the driver is used mainline and no one reported issues.
> > >
> > > Also just don't use the *_relaxed() versions is more common, than
> > > adding
> > > mb() calls around *_relaxed() versions.
> > >
> > 
> > Agreed with both. The series is a bit short in explaining _why_ the changes are
> > made.
> > 
> > Guenter
> > 
> > > Regards,
> > >   Marco
> > >
> > > >
> 
> Hi Guenter and Marco,
> 
> 1. did you see any issues?
> This WDOG Timer first appeared in i.MX7ULP, no one report issues
> probably because few people use i.MX7ULP. This issue was found when we
> did a stress test on it. When we reconfigure the WDOG Timer, there is
> a certain probability that it reset. The reason for the error is that
> when WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit writes
> (0xC520, 0xD928) to the CNT register within 16 bus clocks, and
> improper unlock sequence causes the WDOG to reset. Adding mb() is to
> guarantee that two 16-bit writes are finished within 16 bus clocks.

After this explanation the whole imx7ulp_wdt_init() seems a bit buggy
because writel_relaxed() as well as writel() are 32bit access functions.
So the very first thing to do is to enable the 32-bit mode.

Also this is a explanation worth to be added to the commit message ;)

> 2. Also just don't use the *_relaxed() versions is more common, than
> adding mb() calls around *_relaxed() versions.  Memory barriers cannot
> be added between two 16-bit writes. I do not know the reason.

As written above, writel() as well as writel_relaxed() are not 16-bit
access functions.

So to me it looks as you need first to ensure that 32-bit access mode is
enabled. After that you can write drop the to writel_relaxed() functions
and instead just write:

   writel(UNLOCK, wdt->base + WDOG_CNT);

Also why do we need to unlock the watchdog during imx7ulp_wdt_init()?
This is handled just fine during imx7ulp_wdt_enable() and during
imx7ulp_wdt_set_timeout(). So just drop those relaxed writes and
everything should be fine.

Regards,
  Marco

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-23  9:10             ` Marco Felsch
@ 2022-08-23 12:02               ` Guenter Roeck
  2022-08-24  6:24                 ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Guenter Roeck @ 2022-08-23 12:02 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Alice Guo (OSS),
	wim, shawnguo, s.hauer, festevam, linux-arm-kernel, linux-kernel,
	dl-linux-imx, kernel, linux-watchdog

On Tue, Aug 23, 2022 at 11:10:27AM +0200, Marco Felsch wrote:
> On 22-08-23, Alice Guo (OSS) wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > > Sent: Monday, August 22, 2022 10:04 PM
> > > To: Marco Felsch <m.felsch@pengutronix.de>
> > > Cc: Alice Guo (OSS) <alice.guo@oss.nxp.com>; wim@linux-watchdog.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-watchdog@vger.kernel.org
> > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > > unlock sequence
> > > 
> > > On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:
> > > > On 22-08-22, Alice Guo (OSS) wrote:
> > > > > > -----Original Message-----
> > > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > > Cc: wim@linux-watchdog.org; linux@roeck-us.net;
> > > > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > > > > linux-arm-kernel@lists.infradead.org;
> > > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > > > barrier for unlock sequence
> > > > > >
> > > > > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > > > > From: Jacky Bai <ping.bai@nxp.com>
> > > > > > >
> > > > > > > Add explict memory barrier for the wdog unlock sequence.
> > > > > >
> > > > > > Did you inspected any failures? It's not enough to say what you
> > > > > > did, you need to specify the why as well.
> > > > > >
> > > > > > Regards,
> > > > > >   Marco
> > > > >
> > > > > Hi,
> > > > >
> > > > > Two 16-bit writes of unlocking the Watchdog should be completed within a
> > > certain time. The first mb() is used to ensure that previous instructions are
> > > completed.
> > > > > The second mb() is used to ensure that the unlock sequence cannot be
> > > affected by subsequent instructions. The reason will be added in the commit
> > > log of v2.
> > > >
> > > > Hi,
> > > >
> > > > I know what memory barriers are. My question was, did you see any
> > > > issues? Since the driver is used mainline and no one reported issues.
> > > >
> > > > Also just don't use the *_relaxed() versions is more common, than
> > > > adding
> > > > mb() calls around *_relaxed() versions.
> > > >
> > > 
> > > Agreed with both. The series is a bit short in explaining _why_ the changes are
> > > made.
> > > 
> > > Guenter
> > > 
> > > > Regards,
> > > >   Marco
> > > >
> > > > >
> > 
> > Hi Guenter and Marco,
> > 
> > 1. did you see any issues?
> > This WDOG Timer first appeared in i.MX7ULP, no one report issues
> > probably because few people use i.MX7ULP. This issue was found when we
> > did a stress test on it. When we reconfigure the WDOG Timer, there is
> > a certain probability that it reset. The reason for the error is that
> > when WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit writes
> > (0xC520, 0xD928) to the CNT register within 16 bus clocks, and
> > improper unlock sequence causes the WDOG to reset. Adding mb() is to
> > guarantee that two 16-bit writes are finished within 16 bus clocks.
> 
> After this explanation the whole imx7ulp_wdt_init() seems a bit buggy
> because writel_relaxed() as well as writel() are 32bit access functions.
> So the very first thing to do is to enable the 32-bit mode.
> 
Agreed. This is much better than having extra code to deal with
both 16-bit and 32-bit access.

> Also this is a explanation worth to be added to the commit message ;)
> 

Definitely. Also, the use of mb(), if it should indeed be needed,
would have to be explained in a code comment.

Thanks,
Guenter

> > 2. Also just don't use the *_relaxed() versions is more common, than
> > adding mb() calls around *_relaxed() versions.  Memory barriers cannot
> > be added between two 16-bit writes. I do not know the reason.
> 
> As written above, writel() as well as writel_relaxed() are not 16-bit
> access functions.
> 
> So to me it looks as you need first to ensure that 32-bit access mode is
> enabled. After that you can write drop the to writel_relaxed() functions
> and instead just write:
> 
>    writel(UNLOCK, wdt->base + WDOG_CNT);
> 
> Also why do we need to unlock the watchdog during imx7ulp_wdt_init()?
> This is handled just fine during imx7ulp_wdt_enable() and during
> imx7ulp_wdt_set_timeout(). So just drop those relaxed writes and
> everything should be fine.
> 
> Regards,
>   Marco

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init
  2022-08-23  5:46     ` Alice Guo (OSS)
@ 2022-08-23 12:05       ` Guenter Roeck
  0 siblings, 0 replies; 31+ messages in thread
From: Guenter Roeck @ 2022-08-23 12:05 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, dl-linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel

On Tue, Aug 23, 2022 at 05:46:55AM +0000, Alice Guo (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, August 22, 2022 10:06 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog
> > init
> > 
> > On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> > > From: Ye Li <ye.li@nxp.com>
> > >
> > > 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 <ye.li@nxp.com>
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > > ---
> > >  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();
> > 
> > Now this is intermixing writel() with writel_relaxed(), making the code all but
> > impossible to understand.
> > 
> > Guenter
> 
> Hi Guenter,
> 
> Intermixing writel() with writel_relaxed() is unavoidable here. Because there cannot be a memory barrier between writing UNLOCK_SEQ0 and writing UNLOCK_SEQ1. This may be determined by hardware design.
> 

If it is indeed impossible to configure the watchdog for 32-bit
access mode, that needs to be explained in the code and backed up,
for example with a reference to the documentation. Similar,
it needs to be documented in the code why writel() does not work
here and why mb() is needed.

Thanks,
Guenter

> Best Regards,
> Alice Guo
>  
> > > +	}
> > >
> > >  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> > >  	if (ret)
> > > --
> > > 2.17.1
> > >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
  2022-08-23  5:59     ` Alice Guo (OSS)
@ 2022-08-23 12:06       ` Guenter Roeck
  2022-08-24  6:44         ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Guenter Roeck @ 2022-08-23 12:06 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, dl-linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel

On Tue, Aug 23, 2022 at 05:59:11AM +0000, Alice Guo (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, August 22, 2022 10:10 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
> > 
> > On Tue, Aug 16, 2022 at 12:36:40PM +0800, Alice Guo (OSS) wrote:
> > > From: Ye Li <ye.li@nxp.com>
> > >
> > > 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
> > >
> > 
> > You'll have to find a better solution. An active (non-sleep) wait of
> > 10 ms is unacceptable.
> > 
> > Guenter
> 
> Hi Guenter,
> 
> Sorry. I think this patch should be merged with " watchdog: imx7ulp_wdt: Handle wdog reconfigure failure", but I didn't merge them. I will send v2.
> 

That doesn't change the fact that a 10 ms hot wait is unacceptable.

Thanks,
Guenter

> Best Regards,
> Alice Guo
> 
> > > Signed-off-by: Ye Li <ye.li@nxp.com>
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > > ---
> > >  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
> > >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-23 12:02               ` Guenter Roeck
@ 2022-08-24  6:24                 ` Alice Guo (OSS)
  2022-08-24  8:03                   ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-24  6:24 UTC (permalink / raw)
  To: Guenter Roeck, Marco Felsch
  Cc: Alice Guo (OSS),
	wim, shawnguo, s.hauer, festevam, linux-arm-kernel, linux-kernel,
	dl-linux-imx, kernel, linux-watchdog



> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, August 23, 2022 8:02 PM
> To: Marco Felsch <m.felsch@pengutronix.de>
> Cc: Alice Guo (OSS) <alice.guo@oss.nxp.com>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> On Tue, Aug 23, 2022 at 11:10:27AM +0200, Marco Felsch wrote:
> > On 22-08-23, Alice Guo (OSS) wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > > > Sent: Monday, August 22, 2022 10:04 PM
> > > > To: Marco Felsch <m.felsch@pengutronix.de>
> > > > Cc: Alice Guo (OSS) <alice.guo@oss.nxp.com>;
> > > > wim@linux-watchdog.org; shawnguo@kernel.org;
> > > > s.hauer@pengutronix.de; festevam@gmail.com;
> > > > linux-arm-kernel@lists.infradead.org;
> > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > barrier for unlock sequence
> > > >
> > > > On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:
> > > > > On 22-08-22, Alice Guo (OSS) wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > > > Cc: wim@linux-watchdog.org; linux@roeck-us.net;
> > > > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > > > > > festevam@gmail.com; linux-arm-kernel@lists.infradead.org;
> > > > > > > linux-kernel@vger.kernel.org; dl-linux-imx
> > > > > > > <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > > > > > linux-watchdog@vger.kernel.org
> > > > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict
> > > > > > > memory barrier for unlock sequence
> > > > > > >
> > > > > > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > > > > > From: Jacky Bai <ping.bai@nxp.com>
> > > > > > > >
> > > > > > > > Add explict memory barrier for the wdog unlock sequence.
> > > > > > >
> > > > > > > Did you inspected any failures? It's not enough to say what
> > > > > > > you did, you need to specify the why as well.
> > > > > > >
> > > > > > > Regards,
> > > > > > >   Marco
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Two 16-bit writes of unlocking the Watchdog should be
> > > > > > completed within a
> > > > certain time. The first mb() is used to ensure that previous
> > > > instructions are completed.
> > > > > > The second mb() is used to ensure that the unlock sequence
> > > > > > cannot be
> > > > affected by subsequent instructions. The reason will be added in
> > > > the commit log of v2.
> > > > >
> > > > > Hi,
> > > > >
> > > > > I know what memory barriers are. My question was, did you see
> > > > > any issues? Since the driver is used mainline and no one reported
> issues.
> > > > >
> > > > > Also just don't use the *_relaxed() versions is more common,
> > > > > than adding
> > > > > mb() calls around *_relaxed() versions.
> > > > >
> > > >
> > > > Agreed with both. The series is a bit short in explaining _why_
> > > > the changes are made.
> > > >
> > > > Guenter
> > > >
> > > > > Regards,
> > > > >   Marco
> > > > >
> > > > > >
> > >
> > > Hi Guenter and Marco,
> > >
> > > 1. did you see any issues?
> > > This WDOG Timer first appeared in i.MX7ULP, no one report issues
> > > probably because few people use i.MX7ULP. This issue was found when
> > > we did a stress test on it. When we reconfigure the WDOG Timer,
> > > there is a certain probability that it reset. The reason for the
> > > error is that when WDOG_CS[CMD32EN] is 0, the unlock sequence is two
> > > 16-bit writes (0xC520, 0xD928) to the CNT register within 16 bus
> > > clocks, and improper unlock sequence causes the WDOG to reset.
> > > Adding mb() is to guarantee that two 16-bit writes are finished within 16
> bus clocks.
> >
> > After this explanation the whole imx7ulp_wdt_init() seems a bit buggy
> > because writel_relaxed() as well as writel() are 32bit access functions.
> > So the very first thing to do is to enable the 32-bit mode.
> >
> Agreed. This is much better than having extra code to deal with both 16-bit
> and 32-bit access.
> 
> > Also this is a explanation worth to be added to the commit message ;)
> >
> 
> Definitely. Also, the use of mb(), if it should indeed be needed, would have to
> be explained in a code comment.
> 
> Thanks,
> Guenter

Hi Marco and Guenter,

Thank you for your comments. I plan to enable support for 32-bit unlock command write words in bootloader. In this way, there is no need to distinguish whether the unlock command is a 32-bit command or a 16-bit command in driver.

Best Regards,
Alice Guo

> > > 2. Also just don't use the *_relaxed() versions is more common, than
> > > adding mb() calls around *_relaxed() versions.  Memory barriers
> > > cannot be added between two 16-bit writes. I do not know the reason.
> >
> > As written above, writel() as well as writel_relaxed() are not 16-bit
> > access functions.
> >
> > So to me it looks as you need first to ensure that 32-bit access mode
> > is enabled. After that you can write drop the to writel_relaxed()
> > functions and instead just write:
> >
> >    writel(UNLOCK, wdt->base + WDOG_CNT);
> >
> > Also why do we need to unlock the watchdog during imx7ulp_wdt_init()?
> > This is handled just fine during imx7ulp_wdt_enable() and during
> > imx7ulp_wdt_set_timeout(). So just drop those relaxed writes and
> > everything should be fine.
> >
> > Regards,
> >   Marco

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
  2022-08-23 12:06       ` Guenter Roeck
@ 2022-08-24  6:44         ` Alice Guo (OSS)
  0 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-24  6:44 UTC (permalink / raw)
  To: Guenter Roeck, Alice Guo (OSS)
  Cc: wim, shawnguo, s.hauer, festevam, kernel, dl-linux-imx,
	linux-watchdog, linux-arm-kernel, linux-kernel

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, August 23, 2022 8:07 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue
> 
> On Tue, Aug 23, 2022 at 05:59:11AM +0000, Alice Guo (OSS) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > > Sent: Monday, August 22, 2022 10:10 PM
> > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > > dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout
> > > issue
> > >
> > > On Tue, Aug 16, 2022 at 12:36:40PM +0800, Alice Guo (OSS) wrote:
> > > > From: Ye Li <ye.li@nxp.com>
> > > >
> > > > 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
> > > >
> > >
> > > You'll have to find a better solution. An active (non-sleep) wait of
> > > 10 ms is unacceptable.
> > >
> > > Guenter
> >
> > Hi Guenter,
> >
> > Sorry. I think this patch should be merged with " watchdog: imx7ulp_wdt:
> Handle wdog reconfigure failure", but I didn't merge them. I will send v2.
> >
> 
> That doesn't change the fact that a 10 ms hot wait is unacceptable.
> 
> Thanks,
> Guenter

Hi Guenter,

I plan to use readl_poll_timeout to wait for RCS(Reconfiguration Success). I think the sleep_us of readl_poll_timeout can be used to avoid a 10 ms hot wait.

Best Regards,
Alice Guo

> > Best Regards,
> > Alice Guo
> >
> > > > Signed-off-by: Ye Li <ye.li@nxp.com>
> > > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > > > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > > > ---
> > > >  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
> > > >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-24  6:24                 ` Alice Guo (OSS)
@ 2022-08-24  8:03                   ` Marco Felsch
  2022-08-24  8:40                     ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-24  8:03 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

Hi Alice,

On 22-08-24, Alice Guo (OSS) wrote:

...

> > > > Hi Guenter and Marco,
> > > >
> > > > 1. did you see any issues?
> > > > This WDOG Timer first appeared in i.MX7ULP, no one report issues
> > > > probably because few people use i.MX7ULP. This issue was found when
> > > > we did a stress test on it. When we reconfigure the WDOG Timer,
> > > > there is a certain probability that it reset. The reason for the
> > > > error is that when WDOG_CS[CMD32EN] is 0, the unlock sequence is two
> > > > 16-bit writes (0xC520, 0xD928) to the CNT register within 16 bus
> > > > clocks, and improper unlock sequence causes the WDOG to reset.
> > > > Adding mb() is to guarantee that two 16-bit writes are finished within 16
> > bus clocks.
> > >
> > > After this explanation the whole imx7ulp_wdt_init() seems a bit buggy
> > > because writel_relaxed() as well as writel() are 32bit access functions.
> > > So the very first thing to do is to enable the 32-bit mode.
> > >
> > Agreed. This is much better than having extra code to deal with both 16-bit
> > and 32-bit access.
> > 
> > > Also this is a explanation worth to be added to the commit message ;)
> > >
> > 
> > Definitely. Also, the use of mb(), if it should indeed be needed, would have to
> > be explained in a code comment.
> > 
> > Thanks,
> > Guenter
> 
> Hi Marco and Guenter,
> 
> Thank you for your comments. I plan to enable support for 32-bit
> unlock command write words in bootloader. In this way, there is no
> need to distinguish whether the unlock command is a 32-bit command or
> a 16-bit command in driver.

Please don't move this into the bootloader, enabling it within the init
seq. is just fine. If you move it into the bootloader then you can't
ensure that the bit is set since there are plenty of bootloaders out
there.

As I said, just drop the "16bit" unlock sequence from the init function
because the unlock is handled just fine in all the watchdog_ops.

Regards,
  Marco

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-24  8:03                   ` Marco Felsch
@ 2022-08-24  8:40                     ` Alice Guo (OSS)
  2022-08-24  9:06                       ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-24  8:40 UTC (permalink / raw)
  To: Marco Felsch, Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: Wednesday, August 24, 2022 4:04 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> Hi Alice,
> 
> On 22-08-24, Alice Guo (OSS) wrote:
> 
> ...
> 
> > > > > Hi Guenter and Marco,
> > > > >
> > > > > 1. did you see any issues?
> > > > > This WDOG Timer first appeared in i.MX7ULP, no one report issues
> > > > > probably because few people use i.MX7ULP. This issue was found
> > > > > when we did a stress test on it. When we reconfigure the WDOG
> > > > > Timer, there is a certain probability that it reset. The reason
> > > > > for the error is that when WDOG_CS[CMD32EN] is 0, the unlock
> > > > > sequence is two 16-bit writes (0xC520, 0xD928) to the CNT
> > > > > register within 16 bus clocks, and improper unlock sequence causes the
> WDOG to reset.
> > > > > Adding mb() is to guarantee that two 16-bit writes are finished
> > > > > within 16
> > > bus clocks.
> > > >
> > > > After this explanation the whole imx7ulp_wdt_init() seems a bit
> > > > buggy because writel_relaxed() as well as writel() are 32bit access
> functions.
> > > > So the very first thing to do is to enable the 32-bit mode.
> > > >
> > > Agreed. This is much better than having extra code to deal with both
> > > 16-bit and 32-bit access.
> > >
> > > > Also this is a explanation worth to be added to the commit message
> > > > ;)
> > > >
> > >
> > > Definitely. Also, the use of mb(), if it should indeed be needed,
> > > would have to be explained in a code comment.
> > >
> > > Thanks,
> > > Guenter
> >
> > Hi Marco and Guenter,
> >
> > Thank you for your comments. I plan to enable support for 32-bit
> > unlock command write words in bootloader. In this way, there is no
> > need to distinguish whether the unlock command is a 32-bit command or
> > a 16-bit command in driver.
> 
> Please don't move this into the bootloader, enabling it within the init seq. is
> just fine. If you move it into the bootloader then you can't ensure that the bit is
> set since there are plenty of bootloaders out there.
> 
> As I said, just drop the "16bit" unlock sequence from the init function because
> the unlock is handled just fine in all the watchdog_ops.
> 
> Regards,
>   Marco

Hi Marco,

Sorry, I did not tell you that all watchdog control bits, timeout value, and window value cannot be set until the watchdog is unlocked. Support for 32-bit unlock command write words in enabled in imx7ulp_wdt_init now.

Best Regards,
Alice Guo




^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-24  8:40                     ` Alice Guo (OSS)
@ 2022-08-24  9:06                       ` Marco Felsch
  2022-08-24 10:05                         ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-24  9:06 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

Hi Alice,

On 22-08-24, Alice Guo (OSS) wrote:
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@pengutronix.de>
> > Sent: Wednesday, August 24, 2022 4:04 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-watchdog@vger.kernel.org
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > Hi Alice,
> > 
> > On 22-08-24, Alice Guo (OSS) wrote:
> > 
> > ...
> > 
> > > > > > Hi Guenter and Marco,
> > > > > >
> > > > > > 1. did you see any issues?
> > > > > > This WDOG Timer first appeared in i.MX7ULP, no one report issues
> > > > > > probably because few people use i.MX7ULP. This issue was found
> > > > > > when we did a stress test on it. When we reconfigure the WDOG
> > > > > > Timer, there is a certain probability that it reset. The reason
> > > > > > for the error is that when WDOG_CS[CMD32EN] is 0, the unlock
> > > > > > sequence is two 16-bit writes (0xC520, 0xD928) to the CNT
> > > > > > register within 16 bus clocks, and improper unlock sequence causes the
> > WDOG to reset.
> > > > > > Adding mb() is to guarantee that two 16-bit writes are finished
> > > > > > within 16
> > > > bus clocks.
> > > > >
> > > > > After this explanation the whole imx7ulp_wdt_init() seems a bit
> > > > > buggy because writel_relaxed() as well as writel() are 32bit access
> > functions.
> > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > >
> > > > Agreed. This is much better than having extra code to deal with both
> > > > 16-bit and 32-bit access.
> > > >
> > > > > Also this is a explanation worth to be added to the commit message
> > > > > ;)
> > > > >
> > > >
> > > > Definitely. Also, the use of mb(), if it should indeed be needed,
> > > > would have to be explained in a code comment.
> > > >
> > > > Thanks,
> > > > Guenter
> > >
> > > Hi Marco and Guenter,
> > >
> > > Thank you for your comments. I plan to enable support for 32-bit
> > > unlock command write words in bootloader. In this way, there is no
> > > need to distinguish whether the unlock command is a 32-bit command or
> > > a 16-bit command in driver.
> > 
> > Please don't move this into the bootloader, enabling it within the init seq. is
> > just fine. If you move it into the bootloader then you can't ensure that the bit is
> > set since there are plenty of bootloaders out there.
> > 
> > As I said, just drop the "16bit" unlock sequence from the init function because
> > the unlock is handled just fine in all the watchdog_ops.
> > 
> > Regards,
> >   Marco
> 
> Hi Marco,
> 
> Sorry, I did not tell you that all watchdog control bits, timeout
> value, and window value cannot be set until the watchdog is unlocked.

You don't have to according the RM:
8<----------------------------------------------------------------------
59.5.2 Disable Watchdog after Reset

All of watchdog registers are unlocked by reset. Therefore, unlock
sequence is unnecessary, but it needs to write all of watchdog registers
to make the new configuration take effect. The code snippet below shows
an example of disabling watchdog after reset.
8<----------------------------------------------------------------------

also the RM tells us:
8<----------------------------------------------------------------------
59.4.3.1 Configuring the Watchdog Once

The new configuration takes effect only after all registers except CNT
are written after reset. Otherwise, the WDOG uses the reset values by
default. If window mode is not used (CS[WIN] is 0), writing to WIN is
not required to make the new configuration take effect.
8<----------------------------------------------------------------------

> Support for 32-bit unlock command write words in enabled in
> imx7ulp_wdt_init now.

So.. after reading the IMX7ULP RM, which was not my intention, I found
out that most of the WDOG_CS regiter bits are write-once bits. This
means if you didn't set it within the bootloader you still in case
"59.4.3.1".

So the imx7ulp_wdt_init() function just needs to check if the
WDOG_CS_UPDATE bit was set. If it is not the case, then you need to
write the WDOG_CS register as currently done. If the bit is set, than
you need know that the bootloader did the job for you and you can exit
imx7ulp_wdt_init() early. In both cases the unlock is not required.

Can you please check/test if this is working for you?

Regards,
  Marco

> Best Regards,
> Alice Guo
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-24  9:06                       ` Marco Felsch
@ 2022-08-24 10:05                         ` Alice Guo (OSS)
  2022-08-25  7:50                           ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-24 10:05 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog


> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: Wednesday, August 24, 2022 5:06 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> Hi Alice,
> 
> On 22-08-24, Alice Guo (OSS) wrote:
> > > -----Original Message-----
> > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > Sent: Wednesday, August 24, 2022 4:04 PM
> > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-watchdog@vger.kernel.org
> > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > barrier for unlock sequence
> > >
> > > Hi Alice,
> > >
> > > On 22-08-24, Alice Guo (OSS) wrote:
> > >
> > > ...
> > >
> > > > > > > Hi Guenter and Marco,
> > > > > > >
> > > > > > > 1. did you see any issues?
> > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one report
> > > > > > > issues probably because few people use i.MX7ULP. This issue
> > > > > > > was found when we did a stress test on it. When we
> > > > > > > reconfigure the WDOG Timer, there is a certain probability
> > > > > > > that it reset. The reason for the error is that when
> > > > > > > WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit
> > > > > > > writes (0xC520, 0xD928) to the CNT register within 16 bus
> > > > > > > clocks, and improper unlock sequence causes the
> > > WDOG to reset.
> > > > > > > Adding mb() is to guarantee that two 16-bit writes are
> > > > > > > finished within 16
> > > > > bus clocks.
> > > > > >
> > > > > > After this explanation the whole imx7ulp_wdt_init() seems a
> > > > > > bit buggy because writel_relaxed() as well as writel() are
> > > > > > 32bit access
> > > functions.
> > > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > > >
> > > > > Agreed. This is much better than having extra code to deal with
> > > > > both 16-bit and 32-bit access.
> > > > >
> > > > > > Also this is a explanation worth to be added to the commit
> > > > > > message
> > > > > > ;)
> > > > > >
> > > > >
> > > > > Definitely. Also, the use of mb(), if it should indeed be
> > > > > needed, would have to be explained in a code comment.
> > > > >
> > > > > Thanks,
> > > > > Guenter
> > > >
> > > > Hi Marco and Guenter,
> > > >
> > > > Thank you for your comments. I plan to enable support for 32-bit
> > > > unlock command write words in bootloader. In this way, there is no
> > > > need to distinguish whether the unlock command is a 32-bit command
> > > > or a 16-bit command in driver.
> > >
> > > Please don't move this into the bootloader, enabling it within the
> > > init seq. is just fine. If you move it into the bootloader then you
> > > can't ensure that the bit is set since there are plenty of bootloaders out
> there.
> > >
> > > As I said, just drop the "16bit" unlock sequence from the init
> > > function because the unlock is handled just fine in all the watchdog_ops.
> > >
> > > Regards,
> > >   Marco
> >
> > Hi Marco,
> >
> > Sorry, I did not tell you that all watchdog control bits, timeout
> > value, and window value cannot be set until the watchdog is unlocked.
> 
> You don't have to according the RM:
> 8<----------------------------------------------------------------------
> 59.5.2 Disable Watchdog after Reset
> 
> All of watchdog registers are unlocked by reset. Therefore, unlock sequence is
> unnecessary, but it needs to write all of watchdog registers to make the new
> configuration take effect. The code snippet below shows an example of
> disabling watchdog after reset.
> 8<----------------------------------------------------------------------
> 
> also the RM tells us:
> 8<----------------------------------------------------------------------
> 59.4.3.1 Configuring the Watchdog Once
> 
> The new configuration takes effect only after all registers except CNT are
> written after reset. Otherwise, the WDOG uses the reset values by default. If
> window mode is not used (CS[WIN] is 0), writing to WIN is not required to
> make the new configuration take effect.
> 8<----------------------------------------------------------------------
> 
> > Support for 32-bit unlock command write words in enabled in
> > imx7ulp_wdt_init now.
> 
> So.. after reading the IMX7ULP RM, which was not my intention, I found out
> that most of the WDOG_CS regiter bits are write-once bits. This means if you
> didn't set it within the bootloader you still in case "59.4.3.1".
> 
> So the imx7ulp_wdt_init() function just needs to check if the
> WDOG_CS_UPDATE bit was set. If it is not the case, then you need to write the
> WDOG_CS register as currently done. If the bit is set, than you need know that
> the bootloader did the job for you and you can exit
> imx7ulp_wdt_init() early. In both cases the unlock is not required.
> 
> Can you please check/test if this is working for you?
> 
> Regards,
>   Marco
> 

Hi Marco,

Rom code has already configured the WDOG once, so we cannot use " Configuring the Watchdog Once".

Best Regards,
Alice Guo

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-24 10:05                         ` Alice Guo (OSS)
@ 2022-08-25  7:50                           ` Marco Felsch
  2022-08-25  8:08                             ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-25  7:50 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

On 22-08-24, Alice Guo (OSS) wrote:
> 
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@pengutronix.de>
> > Sent: Wednesday, August 24, 2022 5:06 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-watchdog@vger.kernel.org
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > Hi Alice,
> > 
> > On 22-08-24, Alice Guo (OSS) wrote:
> > > > -----Original Message-----
> > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > Sent: Wednesday, August 24, 2022 4:04 PM
> > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > > linux-watchdog@vger.kernel.org
> > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > barrier for unlock sequence
> > > >
> > > > Hi Alice,
> > > >
> > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > Hi Guenter and Marco,
> > > > > > > >
> > > > > > > > 1. did you see any issues?
> > > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one report
> > > > > > > > issues probably because few people use i.MX7ULP. This issue
> > > > > > > > was found when we did a stress test on it. When we
> > > > > > > > reconfigure the WDOG Timer, there is a certain probability
> > > > > > > > that it reset. The reason for the error is that when
> > > > > > > > WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit
> > > > > > > > writes (0xC520, 0xD928) to the CNT register within 16 bus
> > > > > > > > clocks, and improper unlock sequence causes the
> > > > WDOG to reset.
> > > > > > > > Adding mb() is to guarantee that two 16-bit writes are
> > > > > > > > finished within 16
> > > > > > bus clocks.
> > > > > > >
> > > > > > > After this explanation the whole imx7ulp_wdt_init() seems a
> > > > > > > bit buggy because writel_relaxed() as well as writel() are
> > > > > > > 32bit access
> > > > functions.
> > > > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > > > >
> > > > > > Agreed. This is much better than having extra code to deal with
> > > > > > both 16-bit and 32-bit access.
> > > > > >
> > > > > > > Also this is a explanation worth to be added to the commit
> > > > > > > message
> > > > > > > ;)
> > > > > > >
> > > > > >
> > > > > > Definitely. Also, the use of mb(), if it should indeed be
> > > > > > needed, would have to be explained in a code comment.
> > > > > >
> > > > > > Thanks,
> > > > > > Guenter
> > > > >
> > > > > Hi Marco and Guenter,
> > > > >
> > > > > Thank you for your comments. I plan to enable support for 32-bit
> > > > > unlock command write words in bootloader. In this way, there is no
> > > > > need to distinguish whether the unlock command is a 32-bit command
> > > > > or a 16-bit command in driver.
> > > >
> > > > Please don't move this into the bootloader, enabling it within the
> > > > init seq. is just fine. If you move it into the bootloader then you
> > > > can't ensure that the bit is set since there are plenty of bootloaders out
> > there.
> > > >
> > > > As I said, just drop the "16bit" unlock sequence from the init
> > > > function because the unlock is handled just fine in all the watchdog_ops.
> > > >
> > > > Regards,
> > > >   Marco
> > >
> > > Hi Marco,
> > >
> > > Sorry, I did not tell you that all watchdog control bits, timeout
> > > value, and window value cannot be set until the watchdog is unlocked.
> > 
> > You don't have to according the RM:
> > 8<----------------------------------------------------------------------
> > 59.5.2 Disable Watchdog after Reset
> > 
> > All of watchdog registers are unlocked by reset. Therefore, unlock sequence is
> > unnecessary, but it needs to write all of watchdog registers to make the new
> > configuration take effect. The code snippet below shows an example of
> > disabling watchdog after reset.
> > 8<----------------------------------------------------------------------
> > 
> > also the RM tells us:
> > 8<----------------------------------------------------------------------
> > 59.4.3.1 Configuring the Watchdog Once
> > 
> > The new configuration takes effect only after all registers except CNT are
> > written after reset. Otherwise, the WDOG uses the reset values by default. If
> > window mode is not used (CS[WIN] is 0), writing to WIN is not required to
> > make the new configuration take effect.
> > 8<----------------------------------------------------------------------
> > 
> > > Support for 32-bit unlock command write words in enabled in
> > > imx7ulp_wdt_init now.
> > 
> > So.. after reading the IMX7ULP RM, which was not my intention, I found out
> > that most of the WDOG_CS regiter bits are write-once bits. This means if you
> > didn't set it within the bootloader you still in case "59.4.3.1".
> > 
> > So the imx7ulp_wdt_init() function just needs to check if the
> > WDOG_CS_UPDATE bit was set. If it is not the case, then you need to write the
> > WDOG_CS register as currently done. If the bit is set, than you need know that
> > the bootloader did the job for you and you can exit
> > imx7ulp_wdt_init() early. In both cases the unlock is not required.
> > 
> > Can you please check/test if this is working for you?
> > 
> > Regards,
> >   Marco
> > 
> 
> Hi Marco,
> 
> Rom code has already configured the WDOG once, so we cannot use "
> Configuring the Watchdog Once".

What? How does the ROM code configure the WDOG? Also this would be worth a
comment within the code. Also still assume that this "16bit unlock" seq.
is useless since you writing 32bit anyway.

Regards,
  Marco

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-25  7:50                           ` Marco Felsch
@ 2022-08-25  8:08                             ` Alice Guo (OSS)
  2022-08-25  9:01                               ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-25  8:08 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog


> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: Thursday, August 25, 2022 3:50 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> On 22-08-24, Alice Guo (OSS) wrote:
> >
> > > -----Original Message-----
> > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > Sent: Wednesday, August 24, 2022 5:06 PM
> > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-watchdog@vger.kernel.org
> > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > barrier for unlock sequence
> > >
> > > Hi Alice,
> > >
> > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > > -----Original Message-----
> > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > Sent: Wednesday, August 24, 2022 4:04 PM
> > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> festevam@gmail.com;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > > barrier for unlock sequence
> > > > >
> > > > > Hi Alice,
> > > > >
> > > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > > > Hi Guenter and Marco,
> > > > > > > > >
> > > > > > > > > 1. did you see any issues?
> > > > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one
> > > > > > > > > report issues probably because few people use i.MX7ULP.
> > > > > > > > > This issue was found when we did a stress test on it.
> > > > > > > > > When we reconfigure the WDOG Timer, there is a certain
> > > > > > > > > probability that it reset. The reason for the error is
> > > > > > > > > that when WDOG_CS[CMD32EN] is 0, the unlock sequence is
> > > > > > > > > two 16-bit writes (0xC520, 0xD928) to the CNT register
> > > > > > > > > within 16 bus clocks, and improper unlock sequence
> > > > > > > > > causes the
> > > > > WDOG to reset.
> > > > > > > > > Adding mb() is to guarantee that two 16-bit writes are
> > > > > > > > > finished within 16
> > > > > > > bus clocks.
> > > > > > > >
> > > > > > > > After this explanation the whole imx7ulp_wdt_init() seems
> > > > > > > > a bit buggy because writel_relaxed() as well as writel()
> > > > > > > > are 32bit access
> > > > > functions.
> > > > > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > > > > >
> > > > > > > Agreed. This is much better than having extra code to deal
> > > > > > > with both 16-bit and 32-bit access.
> > > > > > >
> > > > > > > > Also this is a explanation worth to be added to the commit
> > > > > > > > message
> > > > > > > > ;)
> > > > > > > >
> > > > > > >
> > > > > > > Definitely. Also, the use of mb(), if it should indeed be
> > > > > > > needed, would have to be explained in a code comment.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Guenter
> > > > > >
> > > > > > Hi Marco and Guenter,
> > > > > >
> > > > > > Thank you for your comments. I plan to enable support for
> > > > > > 32-bit unlock command write words in bootloader. In this way,
> > > > > > there is no need to distinguish whether the unlock command is
> > > > > > a 32-bit command or a 16-bit command in driver.
> > > > >
> > > > > Please don't move this into the bootloader, enabling it within
> > > > > the init seq. is just fine. If you move it into the bootloader
> > > > > then you can't ensure that the bit is set since there are plenty
> > > > > of bootloaders out
> > > there.
> > > > >
> > > > > As I said, just drop the "16bit" unlock sequence from the init
> > > > > function because the unlock is handled just fine in all the
> watchdog_ops.
> > > > >
> > > > > Regards,
> > > > >   Marco
> > > >
> > > > Hi Marco,
> > > >
> > > > Sorry, I did not tell you that all watchdog control bits, timeout
> > > > value, and window value cannot be set until the watchdog is unlocked.
> > >
> > > You don't have to according the RM:
> > > 8<------------------------------------------------------------------
> > > ----
> > > 59.5.2 Disable Watchdog after Reset
> > >
> > > All of watchdog registers are unlocked by reset. Therefore, unlock
> > > sequence is unnecessary, but it needs to write all of watchdog
> > > registers to make the new configuration take effect. The code
> > > snippet below shows an example of disabling watchdog after reset.
> > > 8<------------------------------------------------------------------
> > > ----
> > >
> > > also the RM tells us:
> > > 8<------------------------------------------------------------------
> > > ----
> > > 59.4.3.1 Configuring the Watchdog Once
> > >
> > > The new configuration takes effect only after all registers except
> > > CNT are written after reset. Otherwise, the WDOG uses the reset
> > > values by default. If window mode is not used (CS[WIN] is 0),
> > > writing to WIN is not required to make the new configuration take effect.
> > > 8<------------------------------------------------------------------
> > > ----
> > >
> > > > Support for 32-bit unlock command write words in enabled in
> > > > imx7ulp_wdt_init now.
> > >
> > > So.. after reading the IMX7ULP RM, which was not my intention, I
> > > found out that most of the WDOG_CS regiter bits are write-once bits.
> > > This means if you didn't set it within the bootloader you still in case
> "59.4.3.1".
> > >
> > > So the imx7ulp_wdt_init() function just needs to check if the
> > > WDOG_CS_UPDATE bit was set. If it is not the case, then you need to
> > > write the WDOG_CS register as currently done. If the bit is set,
> > > than you need know that the bootloader did the job for you and you
> > > can exit
> > > imx7ulp_wdt_init() early. In both cases the unlock is not required.
> > >
> > > Can you please check/test if this is working for you?
> > >
> > > Regards,
> > >   Marco
> > >
> >
> > Hi Marco,
> >
> > Rom code has already configured the WDOG once, so we cannot use "
> > Configuring the Watchdog Once".
> 
> What? How does the ROM code configure the WDOG? Also this would be
> worth a comment within the code. Also still assume that this "16bit unlock"
> seq.
> is useless since you writing 32bit anyway.
> 
> Regards,
>   Marco

Hi Marco,

The ROM code of i.MX7ULP configures the WDOG to support 16-bit unlock command. I plan to add a comment to explain it in code, and keep "mb(); writel_relaxed; writel_relaxed; mb()" unchanged.

Best Regards,
Alice Guo









^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-25  8:08                             ` Alice Guo (OSS)
@ 2022-08-25  9:01                               ` Marco Felsch
  2022-08-25 10:11                                 ` Alice Guo (OSS)
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2022-08-25  9:01 UTC (permalink / raw)
  To: Alice Guo (OSS)
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog

On 22-08-25, Alice Guo (OSS) wrote:
> 
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@pengutronix.de>
> > Sent: Thursday, August 25, 2022 3:50 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-watchdog@vger.kernel.org
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > On 22-08-24, Alice Guo (OSS) wrote:
> > >
> > > > -----Original Message-----
> > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > Sent: Wednesday, August 24, 2022 5:06 PM
> > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > > linux-watchdog@vger.kernel.org
> > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > barrier for unlock sequence
> > > >
> > > > Hi Alice,
> > > >
> > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > > > -----Original Message-----
> > > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > Sent: Wednesday, August 24, 2022 4:04 PM
> > > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> > festevam@gmail.com;
> > > > > > linux-arm-kernel@lists.infradead.org;
> > > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > > > barrier for unlock sequence
> > > > > >
> > > > > > Hi Alice,
> > > > > >
> > > > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > > > Hi Guenter and Marco,
> > > > > > > > > >
> > > > > > > > > > 1. did you see any issues?
> > > > > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one
> > > > > > > > > > report issues probably because few people use i.MX7ULP.
> > > > > > > > > > This issue was found when we did a stress test on it.
> > > > > > > > > > When we reconfigure the WDOG Timer, there is a certain
> > > > > > > > > > probability that it reset. The reason for the error is
> > > > > > > > > > that when WDOG_CS[CMD32EN] is 0, the unlock sequence is
> > > > > > > > > > two 16-bit writes (0xC520, 0xD928) to the CNT register
> > > > > > > > > > within 16 bus clocks, and improper unlock sequence
> > > > > > > > > > causes the
> > > > > > WDOG to reset.
> > > > > > > > > > Adding mb() is to guarantee that two 16-bit writes are
> > > > > > > > > > finished within 16
> > > > > > > > bus clocks.
> > > > > > > > >
> > > > > > > > > After this explanation the whole imx7ulp_wdt_init() seems
> > > > > > > > > a bit buggy because writel_relaxed() as well as writel()
> > > > > > > > > are 32bit access
> > > > > > functions.
> > > > > > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > > > > > >
> > > > > > > > Agreed. This is much better than having extra code to deal
> > > > > > > > with both 16-bit and 32-bit access.
> > > > > > > >
> > > > > > > > > Also this is a explanation worth to be added to the commit
> > > > > > > > > message
> > > > > > > > > ;)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Definitely. Also, the use of mb(), if it should indeed be
> > > > > > > > needed, would have to be explained in a code comment.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Guenter
> > > > > > >
> > > > > > > Hi Marco and Guenter,
> > > > > > >
> > > > > > > Thank you for your comments. I plan to enable support for
> > > > > > > 32-bit unlock command write words in bootloader. In this way,
> > > > > > > there is no need to distinguish whether the unlock command is
> > > > > > > a 32-bit command or a 16-bit command in driver.
> > > > > >
> > > > > > Please don't move this into the bootloader, enabling it within
> > > > > > the init seq. is just fine. If you move it into the bootloader
> > > > > > then you can't ensure that the bit is set since there are plenty
> > > > > > of bootloaders out
> > > > there.
> > > > > >
> > > > > > As I said, just drop the "16bit" unlock sequence from the init
> > > > > > function because the unlock is handled just fine in all the
> > watchdog_ops.
> > > > > >
> > > > > > Regards,
> > > > > >   Marco
> > > > >
> > > > > Hi Marco,
> > > > >
> > > > > Sorry, I did not tell you that all watchdog control bits, timeout
> > > > > value, and window value cannot be set until the watchdog is unlocked.
> > > >
> > > > You don't have to according the RM:
> > > > 8<------------------------------------------------------------------
> > > > ----
> > > > 59.5.2 Disable Watchdog after Reset
> > > >
> > > > All of watchdog registers are unlocked by reset. Therefore, unlock
> > > > sequence is unnecessary, but it needs to write all of watchdog
> > > > registers to make the new configuration take effect. The code
> > > > snippet below shows an example of disabling watchdog after reset.
> > > > 8<------------------------------------------------------------------
> > > > ----
> > > >
> > > > also the RM tells us:
> > > > 8<------------------------------------------------------------------
> > > > ----
> > > > 59.4.3.1 Configuring the Watchdog Once
> > > >
> > > > The new configuration takes effect only after all registers except
> > > > CNT are written after reset. Otherwise, the WDOG uses the reset
> > > > values by default. If window mode is not used (CS[WIN] is 0),
> > > > writing to WIN is not required to make the new configuration take effect.
> > > > 8<------------------------------------------------------------------
> > > > ----
> > > >
> > > > > Support for 32-bit unlock command write words in enabled in
> > > > > imx7ulp_wdt_init now.
> > > >
> > > > So.. after reading the IMX7ULP RM, which was not my intention, I
> > > > found out that most of the WDOG_CS regiter bits are write-once bits.
> > > > This means if you didn't set it within the bootloader you still in case
> > "59.4.3.1".
> > > >
> > > > So the imx7ulp_wdt_init() function just needs to check if the
> > > > WDOG_CS_UPDATE bit was set. If it is not the case, then you need to
> > > > write the WDOG_CS register as currently done. If the bit is set,
> > > > than you need know that the bootloader did the job for you and you
> > > > can exit
> > > > imx7ulp_wdt_init() early. In both cases the unlock is not required.
> > > >
> > > > Can you please check/test if this is working for you?
> > > >
> > > > Regards,
> > > >   Marco
> > > >
> > >
> > > Hi Marco,
> > >
> > > Rom code has already configured the WDOG once, so we cannot use "
> > > Configuring the Watchdog Once".
> > 
> > What? How does the ROM code configure the WDOG? Also this would be
> > worth a comment within the code. Also still assume that this "16bit unlock"
> > seq.
> > is useless since you writing 32bit anyway.
> > 
> > Regards,
> >   Marco
> 
> Hi Marco,
> 
> The ROM code of i.MX7ULP configures the WDOG to support 16-bit unlock
> command. I plan to add a comment to explain it in code, and keep
> "mb(); writel_relaxed; writel_relaxed; mb()" unchanged.

As said, the writel_relaxed() is also 32bit writes. Please just use the
correct APIs for the stuff you wanna do. In your case, if you want 16bit
access functions you need to use writew() function and you can drop the
mb(). But after reading the RM again, I think you can go with writel()
since 32bit writes are okay, but you need to pass the two 16bit unlock
commands. For that you just need to ensure that the timing is correct,
as noted in the RM.

Also I read that the WDOG1 is only programmed in serial downloader mode
to reset the device if no activity was found on the serial/usb bus. Also
this only happen if the fuse for it was burned, but the RM says also:
"The watchdog is enabled by default after reset.". Can you please read
the Watchdog status register and post it here?

Also after after reading the RM I found the ULK bit "This read-only bit
indicates whether WDOG is unlocked or not." If this bit is 0 the device
is locked and we need a unlock command else we can just write the config
to the device. The unlock command can than be: two writel() commands
which are adding the barriers for free.

Regards,
  Marco


> Best Regards,
> Alice Guo
> 
> 
> 
> 
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
  2022-08-25  9:01                               ` Marco Felsch
@ 2022-08-25 10:11                                 ` Alice Guo (OSS)
  0 siblings, 0 replies; 31+ messages in thread
From: Alice Guo (OSS) @ 2022-08-25 10:11 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Guenter Roeck, wim, shawnguo, s.hauer, festevam,
	linux-arm-kernel, linux-kernel, dl-linux-imx, kernel,
	linux-watchdog


> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: Thursday, August 25, 2022 5:02 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> unlock sequence
> 
> On 22-08-25, Alice Guo (OSS) wrote:
> >
> > > -----Original Message-----
> > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > Sent: Thursday, August 25, 2022 3:50 PM
> > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-watchdog@vger.kernel.org
> > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > barrier for unlock sequence
> > >
> > > On 22-08-24, Alice Guo (OSS) wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > Sent: Wednesday, August 24, 2022 5:06 PM
> > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> festevam@gmail.com;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel@pengutronix.de; linux-watchdog@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory
> > > > > barrier for unlock sequence
> > > > >
> > > > > Hi Alice,
> > > > >
> > > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > > Sent: Wednesday, August 24, 2022 4:04 PM
> > > > > > > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > > > > > > Cc: Guenter Roeck <linux@roeck-us.net>;
> > > > > > > wim@linux-watchdog.org; shawnguo@kernel.org;
> > > > > > > s.hauer@pengutronix.de;
> > > festevam@gmail.com;
> > > > > > > linux-arm-kernel@lists.infradead.org;
> > > > > > > linux-kernel@vger.kernel.org; dl-linux-imx
> > > > > > > <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > > > > > linux-watchdog@vger.kernel.org
> > > > > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict
> > > > > > > memory barrier for unlock sequence
> > > > > > >
> > > > > > > Hi Alice,
> > > > > > >
> > > > > > > On 22-08-24, Alice Guo (OSS) wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > > > > Hi Guenter and Marco,
> > > > > > > > > > >
> > > > > > > > > > > 1. did you see any issues?
> > > > > > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one
> > > > > > > > > > > report issues probably because few people use i.MX7ULP.
> > > > > > > > > > > This issue was found when we did a stress test on it.
> > > > > > > > > > > When we reconfigure the WDOG Timer, there is a
> > > > > > > > > > > certain probability that it reset. The reason for
> > > > > > > > > > > the error is that when WDOG_CS[CMD32EN] is 0, the
> > > > > > > > > > > unlock sequence is two 16-bit writes (0xC520,
> > > > > > > > > > > 0xD928) to the CNT register within 16 bus clocks,
> > > > > > > > > > > and improper unlock sequence causes the
> > > > > > > WDOG to reset.
> > > > > > > > > > > Adding mb() is to guarantee that two 16-bit writes
> > > > > > > > > > > are finished within 16
> > > > > > > > > bus clocks.
> > > > > > > > > >
> > > > > > > > > > After this explanation the whole imx7ulp_wdt_init()
> > > > > > > > > > seems a bit buggy because writel_relaxed() as well as
> > > > > > > > > > writel() are 32bit access
> > > > > > > functions.
> > > > > > > > > > So the very first thing to do is to enable the 32-bit mode.
> > > > > > > > > >
> > > > > > > > > Agreed. This is much better than having extra code to
> > > > > > > > > deal with both 16-bit and 32-bit access.
> > > > > > > > >
> > > > > > > > > > Also this is a explanation worth to be added to the
> > > > > > > > > > commit message
> > > > > > > > > > ;)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Definitely. Also, the use of mb(), if it should indeed
> > > > > > > > > be needed, would have to be explained in a code comment.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Guenter
> > > > > > > >
> > > > > > > > Hi Marco and Guenter,
> > > > > > > >
> > > > > > > > Thank you for your comments. I plan to enable support for
> > > > > > > > 32-bit unlock command write words in bootloader. In this
> > > > > > > > way, there is no need to distinguish whether the unlock
> > > > > > > > command is a 32-bit command or a 16-bit command in driver.
> > > > > > >
> > > > > > > Please don't move this into the bootloader, enabling it
> > > > > > > within the init seq. is just fine. If you move it into the
> > > > > > > bootloader then you can't ensure that the bit is set since
> > > > > > > there are plenty of bootloaders out
> > > > > there.
> > > > > > >
> > > > > > > As I said, just drop the "16bit" unlock sequence from the
> > > > > > > init function because the unlock is handled just fine in all
> > > > > > > the
> > > watchdog_ops.
> > > > > > >
> > > > > > > Regards,
> > > > > > >   Marco
> > > > > >
> > > > > > Hi Marco,
> > > > > >
> > > > > > Sorry, I did not tell you that all watchdog control bits,
> > > > > > timeout value, and window value cannot be set until the watchdog is
> unlocked.
> > > > >
> > > > > You don't have to according the RM:
> > > > > 8<--------------------------------------------------------------
> > > > > ----
> > > > > ----
> > > > > 59.5.2 Disable Watchdog after Reset
> > > > >
> > > > > All of watchdog registers are unlocked by reset. Therefore,
> > > > > unlock sequence is unnecessary, but it needs to write all of
> > > > > watchdog registers to make the new configuration take effect.
> > > > > The code snippet below shows an example of disabling watchdog after
> reset.
> > > > > 8<--------------------------------------------------------------
> > > > > ----
> > > > > ----
> > > > >
> > > > > also the RM tells us:
> > > > > 8<--------------------------------------------------------------
> > > > > ----
> > > > > ----
> > > > > 59.4.3.1 Configuring the Watchdog Once
> > > > >
> > > > > The new configuration takes effect only after all registers
> > > > > except CNT are written after reset. Otherwise, the WDOG uses the
> > > > > reset values by default. If window mode is not used (CS[WIN] is
> > > > > 0), writing to WIN is not required to make the new configuration take
> effect.
> > > > > 8<--------------------------------------------------------------
> > > > > ----
> > > > > ----
> > > > >
> > > > > > Support for 32-bit unlock command write words in enabled in
> > > > > > imx7ulp_wdt_init now.
> > > > >
> > > > > So.. after reading the IMX7ULP RM, which was not my intention, I
> > > > > found out that most of the WDOG_CS regiter bits are write-once bits.
> > > > > This means if you didn't set it within the bootloader you still
> > > > > in case
> > > "59.4.3.1".
> > > > >
> > > > > So the imx7ulp_wdt_init() function just needs to check if the
> > > > > WDOG_CS_UPDATE bit was set. If it is not the case, then you need
> > > > > to write the WDOG_CS register as currently done. If the bit is
> > > > > set, than you need know that the bootloader did the job for you
> > > > > and you can exit
> > > > > imx7ulp_wdt_init() early. In both cases the unlock is not required.
> > > > >
> > > > > Can you please check/test if this is working for you?
> > > > >
> > > > > Regards,
> > > > >   Marco
> > > > >
> > > >
> > > > Hi Marco,
> > > >
> > > > Rom code has already configured the WDOG once, so we cannot use "
> > > > Configuring the Watchdog Once".
> > >
> > > What? How does the ROM code configure the WDOG? Also this would be
> > > worth a comment within the code. Also still assume that this "16bit
> unlock"
> > > seq.
> > > is useless since you writing 32bit anyway.
> > >
> > > Regards,
> > >   Marco
> >
> > Hi Marco,
> >
> > The ROM code of i.MX7ULP configures the WDOG to support 16-bit unlock
> > command. I plan to add a comment to explain it in code, and keep
> > "mb(); writel_relaxed; writel_relaxed; mb()" unchanged.
> 
> As said, the writel_relaxed() is also 32bit writes. Please just use the correct APIs
> for the stuff you wanna do. In your case, if you want 16bit access functions you
> need to use writew() function and you can drop the mb(). But after reading the
> RM again, I think you can go with writel() since 32bit writes are okay, but you
> need to pass the two 16bit unlock commands. For that you just need to ensure
> that the timing is correct, as noted in the RM.
> 
> Also I read that the WDOG1 is only programmed in serial downloader mode to
> reset the device if no activity was found on the serial/usb bus. Also this only
> happen if the fuse for it was burned, but the RM says also:
> "The watchdog is enabled by default after reset.". Can you please read the
> Watchdog status register and post it here?
> 
> Also after after reading the RM I found the ULK bit "This read-only bit indicates
> whether WDOG is unlocked or not." If this bit is 0 the device is locked and we
> need a unlock command else we can just write the config to the device. The
> unlock command can than be: two writel() commands which are adding the
> barriers for free.
> 
> Regards,
>   Marco

Hi Marco,

1. Two 16-bit writes(0xC520, 0xD928) to the CNT register must be finished within 16 bus clocks.
The CNT register is still a 32-bit register, so it is right to use writel_relaxed().

2. After test, we found that memory barriers cannot be added between two 16-bit writes.
writel() cannot be used.

3. Software must make updates within 128 bus clocks after unlocking and before WDOG closing unlock window.
The ULK bit cannot be used to indicate that WDOG can be configured.

Best Regards,
Alice Guo

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2022-08-25 10:11 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16  4:36 [PATCH 0/7] watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 1/7] watchdog: imx7ulp: Move suspend/resume to noirq phase Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence Alice Guo (OSS)
2022-08-16  6:23   ` Marco Felsch
2022-08-22  7:49     ` Alice Guo (OSS)
2022-08-22  8:00       ` Marco Felsch
2022-08-22 14:03         ` Guenter Roeck
2022-08-23  5:38           ` Alice Guo (OSS)
2022-08-23  9:10             ` Marco Felsch
2022-08-23 12:02               ` Guenter Roeck
2022-08-24  6:24                 ` Alice Guo (OSS)
2022-08-24  8:03                   ` Marco Felsch
2022-08-24  8:40                     ` Alice Guo (OSS)
2022-08-24  9:06                       ` Marco Felsch
2022-08-24 10:05                         ` Alice Guo (OSS)
2022-08-25  7:50                           ` Marco Felsch
2022-08-25  8:08                             ` Alice Guo (OSS)
2022-08-25  9:01                               ` Marco Felsch
2022-08-25 10:11                                 ` Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init Alice Guo (OSS)
2022-08-22 14:05   ` Guenter Roeck
2022-08-23  5:46     ` Alice Guo (OSS)
2022-08-23 12:05       ` Guenter Roeck
2022-08-16  4:36 ` [PATCH 4/7] watchdog: imx7ulp_wdt: Fix RCS timeout issue Alice Guo (OSS)
2022-08-22 14:09   ` Guenter Roeck
2022-08-23  5:59     ` Alice Guo (OSS)
2022-08-23 12:06       ` Guenter Roeck
2022-08-24  6:44         ` Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 5/7] watchdog: imx7ulp_wdt: Handle wdog reconfigure failure Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 6/7] watchdog: imx7ulp_wdt: init wdog when it was active Alice Guo (OSS)
2022-08-16  4:36 ` [PATCH 7/7] watchdog: imx93: add watchdog timer on imx93 Alice Guo (OSS)

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).