linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S
@ 2024-01-31 10:20 Claudiu
  2024-01-31 10:20 ` [PATCH v2 01/11] clk: renesas: r9a08g045: Add clock and reset support for watchdog Claudiu
                   ` (10 more replies)
  0 siblings, 11 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>


Hi,

Series adds watchdog support for Renesas RZ/G3S (R9A08G045) SoC.

Patches do the following:
- patch 1/11 selects CONFIG_PM for the watchdog driver
- patch 2/11 adds clock and reset support for watchdog
- patches 3-7/11 adds fixes and cleanup for the watchdog driver
- patch 8/11 adds suspend to RAM to the watchdog driver (to be used by
  RZ/G3S)
- patch 9/11 documents the RZ/G3S support
- patches 10-11/11 add device tree support

It is expected that the clock and device tree support will go through
Geert's tree while the rest of the patches through the watchdog tree.

Thank you,
Claudiu Beznea

Changes in v2:
- added patch "watchdog: rzg2l_wdt: Select PM"
- propagate the return status of rzg2l_wdt_start() to it's callers
  in patch "watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()" 
- propagate the return status of rzg2l_wdt_stop() to it's callers
  in patch "watchdog: rzg2l_wdt: Check return status of pm_runtime_put()" 
- removed pm_ptr() from patch "watchdog: rzg2l_wdt: Add suspend/resume support"
- s/G2UL/G2L in patch "dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support"
- collected tags

Claudiu Beznea (11):
  clk: renesas: r9a08g045: Add clock and reset support for watchdog
  watchdog: rzg2l_wdt: Select PM
  watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
  watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop
  watchdog: rzg2l_wdt: Remove comparison with zero
  watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset
  watchdog: rzg2l_wdt: Add suspend/resume support
  dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support
  arm64: dts: renesas: r9a08g045: Add watchdog node
  arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface

 .../bindings/watchdog/renesas,wdt.yaml        |   1 +
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    |  14 +++
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     |   5 +
 drivers/clk/renesas/r9a08g045-cpg.c           |   3 +
 drivers/watchdog/Kconfig                      |   1 +
 drivers/watchdog/rzg2l_wdt.c                  | 111 ++++++++++--------
 6 files changed, 85 insertions(+), 50 deletions(-)

-- 
2.39.2


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

* [PATCH v2 01/11] clk: renesas: r9a08g045: Add clock and reset support for watchdog
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM Claudiu
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

RZ/G3S has a watchdog module accessible by the Cortex-A core. Add clock
and reset support for it.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 drivers/clk/renesas/r9a08g045-cpg.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index 2582ba95256e..c3e6da2de197 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -193,6 +193,8 @@ static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
 	DEF_MOD("ia55_pclk",		R9A08G045_IA55_PCLK, R9A08G045_CLK_P2, 0x518, 0),
 	DEF_MOD("ia55_clk",		R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1),
 	DEF_MOD("dmac_aclk",		R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0),
+	DEF_MOD("wdt0_pclk",		R9A08G045_WDT0_PCLK, R9A08G045_CLK_P0, 0x548, 0),
+	DEF_MOD("wdt0_clk",		R9A08G045_WDT0_CLK, R9A08G045_OSCCLK, 0x548, 1),
 	DEF_MOD("sdhi0_imclk",		R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0),
 	DEF_MOD("sdhi0_imclk2",		R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1),
 	DEF_MOD("sdhi0_clk_hs",		R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2),
@@ -219,6 +221,7 @@ static const struct rzg2l_reset r9a08g045_resets[] = {
 	DEF_RST(R9A08G045_GIC600_GICRESET_N, 0x814, 0),
 	DEF_RST(R9A08G045_GIC600_DBG_GICRESET_N, 0x814, 1),
 	DEF_RST(R9A08G045_IA55_RESETN, 0x818, 0),
+	DEF_RST(R9A08G045_WDT0_PRESETN, 0x848, 0),
 	DEF_RST(R9A08G045_SDHI0_IXRST, 0x854, 0),
 	DEF_RST(R9A08G045_SDHI1_IXRST, 0x854, 1),
 	DEF_RST(R9A08G045_SDHI2_IXRST, 0x854, 2),
-- 
2.39.2


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

* [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
  2024-01-31 10:20 ` [PATCH v2 01/11] clk: renesas: r9a08g045: Add clock and reset support for watchdog Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-02-01  8:52   ` Geert Uytterhoeven
  2024-02-01 21:05   ` kernel test robot
  2024-01-31 10:20 ` [PATCH v2 03/11] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get() Claudiu
                   ` (8 subsequent siblings)
  10 siblings, 2 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The rzg2l_wdt watchdog driver cannot work w/o CONFIG_PM=y (e.g. the
clocks are enabled though pm_runtime_* specific APIs). To avoid building
a driver that don't work select CONFIG_PM.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this patch is new

 drivers/watchdog/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7d22051b15a2..495dcd1c5139 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -912,6 +912,7 @@ config RENESAS_RZG2LWDT
 	tristate "Renesas RZ/G2L WDT Watchdog"
 	depends on ARCH_RENESAS || COMPILE_TEST
 	select WATCHDOG_CORE
+	select PM
 	help
 	  This driver adds watchdog support for the integrated watchdogs in the
 	  Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.
-- 
2.39.2


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

* [PATCH v2 03/11] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
  2024-01-31 10:20 ` [PATCH v2 01/11] clk: renesas: r9a08g045: Add clock and reset support for watchdog Claudiu
  2024-01-31 10:20 ` [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put() Claudiu
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pm_runtime_get_sync() may return with error. In case it returns with error
dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
takes care of this. Thus use it.

Along with it the rzg2l_wdt_set_timeout() function was updated to
propagate the result of rzg2l_wdt_start() to its caller.

Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- propagate the return code of rzg2l_wdt_start() to it's callers

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

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 1741f98ca67c..d87d4f50180c 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -123,8 +123,11 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
 static int rzg2l_wdt_start(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
-	pm_runtime_get_sync(wdev->parent);
+	ret = pm_runtime_resume_and_get(wdev->parent);
+	if (ret)
+		return ret;
 
 	/* Initialize time out */
 	rzg2l_wdt_init_timeout(wdev);
@@ -150,6 +153,8 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 
 static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
 {
+	int ret = 0;
+
 	wdev->timeout = timeout;
 
 	/*
@@ -159,10 +164,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time
 	 */
 	if (watchdog_active(wdev)) {
 		rzg2l_wdt_stop(wdev);
-		rzg2l_wdt_start(wdev);
+		ret = rzg2l_wdt_start(wdev);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,
-- 
2.39.2


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

* [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (2 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 03/11] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get() Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:32   ` Biju Das
  2024-01-31 10:20 ` [PATCH v2 05/11] watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop Claudiu
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pm_runtime_put() may return an error code. Check its return status.

Along with it the rzg2l_wdt_set_timeout() function was updated to
propagate the result of rzg2l_wdt_stop() to its caller.

Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- propagate the return code of rzg2l_wdt_stop() to it's callers

 drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index d87d4f50180c..7bce093316c4 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev)
 static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
 	rzg2l_wdt_reset(priv);
-	pm_runtime_put(wdev->parent);
+
+	ret = pm_runtime_put(wdev->parent);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -163,7 +167,10 @@ static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int time
 	 * to reset the module) so that it is updated with new timeout values.
 	 */
 	if (watchdog_active(wdev)) {
-		rzg2l_wdt_stop(wdev);
+		ret = rzg2l_wdt_stop(wdev);
+		if (ret)
+			return ret;
+
 		ret = rzg2l_wdt_start(wdev);
 	}
 
-- 
2.39.2


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

* [PATCH v2 05/11] watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (3 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put() Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 06/11] watchdog: rzg2l_wdt: Remove comparison with zero Claudiu
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

There is no need to de-assert the reset signal on probe as the watchdog
is not used prior executing start. Also, the clocks are not enabled in
probe (pm_runtime_enable() doesn't do that), thus this is another indicator
that the watchdog wasn't used previously like this. Instead, keep the
watchdog hardware in its previous state at probe (by default it is in
reset state), enable it when it is started and move it to reset state
when it is stopped. This saves some extra power when the watchdog is
unused.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 drivers/watchdog/rzg2l_wdt.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 7bce093316c4..93a49fd0c7aa 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -129,6 +129,10 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev)
 	if (ret)
 		return ret;
 
+	ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		return ret;
+
 	/* Initialize time out */
 	rzg2l_wdt_init_timeout(wdev);
 
@@ -146,7 +150,9 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 	int ret;
 
-	rzg2l_wdt_reset(priv);
+	ret = reset_control_assert(priv->rstc);
+	if (ret)
+		return ret;
 
 	ret = pm_runtime_put(wdev->parent);
 	if (ret < 0)
@@ -186,6 +192,12 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 	clk_prepare_enable(priv->osc_clk);
 
 	if (priv->devtype == WDT_RZG2L) {
+		int ret;
+
+		ret = reset_control_deassert(priv->rstc);
+		if (ret)
+			return ret;
+
 		/* Generate Reset (WDTRSTB) Signal on parity error */
 		rzg2l_wdt_write(priv, 0, PECR);
 
@@ -236,13 +248,11 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
 	.restart = rzg2l_wdt_restart,
 };
 
-static void rzg2l_wdt_reset_assert_pm_disable(void *data)
+static void rzg2l_wdt_pm_disable(void *data)
 {
 	struct watchdog_device *wdev = data;
-	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
 	pm_runtime_disable(wdev->parent);
-	reset_control_assert(priv->rstc);
 }
 
 static int rzg2l_wdt_probe(struct platform_device *pdev)
@@ -285,10 +295,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
 				     "failed to get cpg reset");
 
-	ret = reset_control_deassert(priv->rstc);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to deassert");
-
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
 	if (priv->devtype == WDT_RZV2M) {
@@ -309,9 +315,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
 
 	watchdog_set_drvdata(&priv->wdev, priv);
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_wdt_reset_assert_pm_disable,
-				       &priv->wdev);
+	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.2


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

* [PATCH v2 06/11] watchdog: rzg2l_wdt: Remove comparison with zero
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (4 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 05/11] watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 07/11] watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset Claudiu
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

devm_add_action_or_reset() could return -ENOMEM or zero. Thus, remove
comparison with zero of the returning value to make code simpler.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 drivers/watchdog/rzg2l_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 93a49fd0c7aa..29eb47bcf984 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -316,7 +316,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(&priv->wdev, priv);
 	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	watchdog_set_nowayout(&priv->wdev, nowayout);
-- 
2.39.2


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

* [PATCH v2 07/11] watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (5 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 06/11] watchdog: rzg2l_wdt: Remove comparison with zero Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 08/11] watchdog: rzg2l_wdt: Add suspend/resume support Claudiu
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The reset driver has been adapted in commit da235d2fac21
("clk: renesas: rzg2l: Check reset monitor registers") to check the reset
monitor bits before declaring reset asserts/de-asserts as
successful/failure operations. With that, there is no need to keep the
reset workaround for RZ/V2M in place in the watchdog driver.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none

 drivers/watchdog/rzg2l_wdt.c | 39 ++++--------------------------------
 1 file changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 29eb47bcf984..42f1d5d6f07e 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -8,7 +8,6 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
-#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -54,35 +53,11 @@ struct rzg2l_wdt_priv {
 	struct reset_control *rstc;
 	unsigned long osc_clk_rate;
 	unsigned long delay;
-	unsigned long minimum_assertion_period;
 	struct clk *pclk;
 	struct clk *osc_clk;
 	enum rz_wdt_type devtype;
 };
 
-static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv)
-{
-	int err, status;
-
-	if (priv->devtype == WDT_RZV2M) {
-		/* WDT needs TYPE-B reset control */
-		err = reset_control_assert(priv->rstc);
-		if (err)
-			return err;
-		ndelay(priv->minimum_assertion_period);
-		err = reset_control_deassert(priv->rstc);
-		if (err)
-			return err;
-		err = read_poll_timeout(reset_control_status, status,
-					status != 1, 0, 1000, false,
-					priv->rstc);
-	} else {
-		err = reset_control_reset(priv->rstc);
-	}
-
-	return err;
-}
-
 static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
 {
 	/* delay timer when change the setting register */
@@ -187,13 +162,12 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
 	clk_prepare_enable(priv->pclk);
 	clk_prepare_enable(priv->osc_clk);
 
 	if (priv->devtype == WDT_RZG2L) {
-		int ret;
-
 		ret = reset_control_deassert(priv->rstc);
 		if (ret)
 			return ret;
@@ -205,7 +179,9 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 		rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
 	} else {
 		/* RZ/V2M doesn't have parity error registers */
-		rzg2l_wdt_reset(priv);
+		ret = reset_control_reset(priv->rstc);
+		if (ret)
+			return ret;
 
 		wdev->timeout = 0;
 
@@ -297,13 +273,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
-	if (priv->devtype == WDT_RZV2M) {
-		priv->minimum_assertion_period = RZV2M_A_NSEC +
-			3 * F2CYCLE_NSEC(pclk_rate) + 5 *
-			max(F2CYCLE_NSEC(priv->osc_clk_rate),
-			    F2CYCLE_NSEC(pclk_rate));
-	}
-
 	pm_runtime_enable(&pdev->dev);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
-- 
2.39.2


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

* [PATCH v2 08/11] watchdog: rzg2l_wdt: Add suspend/resume support
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (6 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 07/11] watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 09/11] dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support Claudiu
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/G3S supports deep sleep states where power to most of the IP blocks
is cut off. To ensure proper working of the watchdog when resuming from
such states, the suspend function is stopping the watchdog and the resume
function is starting it. There is no need to configure the watchdog
in case the watchdog was stopped prior to starting suspend.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- remove the usage of pm_ptr()

 drivers/watchdog/rzg2l_wdt.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 42f1d5d6f07e..c8c20cfb97a3 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -284,6 +284,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
 
 	watchdog_set_drvdata(&priv->wdev, priv);
+	dev_set_drvdata(dev, priv);
 	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev);
 	if (ret)
 		return ret;
@@ -305,10 +306,35 @@ static const struct of_device_id rzg2l_wdt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
 
+static int rzg2l_wdt_suspend_late(struct device *dev)
+{
+	struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev);
+
+	if (!watchdog_active(&priv->wdev))
+		return 0;
+
+	return rzg2l_wdt_stop(&priv->wdev);
+}
+
+static int rzg2l_wdt_resume_early(struct device *dev)
+{
+	struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev);
+
+	if (!watchdog_active(&priv->wdev))
+		return 0;
+
+	return rzg2l_wdt_start(&priv->wdev);
+}
+
+static const struct dev_pm_ops rzg2l_wdt_pm_ops = {
+	LATE_SYSTEM_SLEEP_PM_OPS(rzg2l_wdt_suspend_late, rzg2l_wdt_resume_early)
+};
+
 static struct platform_driver rzg2l_wdt_driver = {
 	.driver = {
 		.name = "rzg2l_wdt",
 		.of_match_table = rzg2l_wdt_ids,
+		.pm = &rzg2l_wdt_pm_ops,
 	},
 	.probe = rzg2l_wdt_probe,
 };
-- 
2.39.2


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

* [PATCH v2 09/11] dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (7 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 08/11] watchdog: rzg2l_wdt: Add suspend/resume support Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 10/11] arm64: dts: renesas: r9a08g045: Add watchdog node Claudiu
  2024-01-31 10:20 ` [PATCH v2 11/11] arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface Claudiu
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea, Conor Dooley

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Document the support for the watchdog IP available on RZ/G3S SoC. The
watchdog IP available on RZ/G3S SoC is identical to the one found on
RZ/G2L SoC.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags
- s/G2UL/G2L in patch description

 Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
index 951a7d54135a..220763838df0 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
@@ -29,6 +29,7 @@ properties:
               - renesas,r9a07g043-wdt    # RZ/G2UL and RZ/Five
               - renesas,r9a07g044-wdt    # RZ/G2{L,LC}
               - renesas,r9a07g054-wdt    # RZ/V2L
+              - renesas,r9a08g045-wdt    # RZ/G3S
           - const: renesas,rzg2l-wdt
 
       - items:
-- 
2.39.2


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

* [PATCH v2 10/11] arm64: dts: renesas: r9a08g045: Add watchdog node
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (8 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 09/11] dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support Claudiu
@ 2024-01-31 10:20 ` Claudiu
  2024-01-31 10:20 ` [PATCH v2 11/11] arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface Claudiu
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add the DT node for the watchdog IP accessible by Cortex-A of RZ/G3S
SoC (R9108G045).

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 5facfad96158..dfee878c0f49 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -264,6 +264,20 @@ gic: interrupt-controller@12400000 {
 			      <0x0 0x12440000 0 0x60000>;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
 		};
+
+		wdt0: watchdog@12800800 {
+			compatible = "renesas,r9a08g045-wdt", "renesas,rzg2l-wdt";
+			reg = <0 0x12800800 0 0x400>;
+			clocks = <&cpg CPG_MOD R9A08G045_WDT0_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_WDT0_CLK>;
+			clock-names = "pclk", "oscclk";
+			interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "wdt", "perrout";
+			resets = <&cpg R9A08G045_WDT0_PRESETN>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
 	};
 
 	timer {
-- 
2.39.2


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

* [PATCH v2 11/11] arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface
  2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
                   ` (9 preceding siblings ...)
  2024-01-31 10:20 ` [PATCH v2 10/11] arm64: dts: renesas: r9a08g045: Add watchdog node Claudiu
@ 2024-01-31 10:20 ` Claudiu
  10 siblings, 0 replies; 23+ messages in thread
From: Claudiu @ 2024-01-31 10:20 UTC (permalink / raw)
  To: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable the watchdog interface (accessible by Cortex-A of RZ/G3S SoC) on
RZ/G3S SMARC SoM.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index f062d4ad78b7..2b7fa5817d58 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -336,3 +336,8 @@ mux {
 		};
 	};
 };
+
+&wdt0 {
+	timeout-sec = <60>;
+	status = "okay";
+};
-- 
2.39.2


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

* RE: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:20 ` [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put() Claudiu
@ 2024-01-31 10:32   ` Biju Das
  2024-01-31 10:35     ` claudiu beznea
  0 siblings, 1 reply; 23+ messages in thread
From: Biju Das @ 2024-01-31 10:32 UTC (permalink / raw)
  To: Claudiu.Beznea, wim, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu.Beznea, Claudiu Beznea

Hi Claudiu,

Thanks for the feedback.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, January 31, 2024 10:20 AM
> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_put() may return an error code. Check its return status.
> 
> Along with it the rzg2l_wdt_set_timeout() function was updated to
> propagate the result of rzg2l_wdt_stop() to its caller.
> 
> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - propagate the return code of rzg2l_wdt_stop() to it's callers
> 
>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index d87d4f50180c..7bce093316c4 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device
> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
> 
>  	rzg2l_wdt_reset(priv);
> -	pm_runtime_put(wdev->parent);
> +
> +	ret = pm_runtime_put(wdev->parent);
> +	if (ret < 0)
> +		return ret;

Do we need to check the return code? So far we didn't hit this condition.
If you are planning to do it, then just 

return pm_runtime_put(wdev->parent);

Cheers,
Biju

> 
>  	return 0;


>  }
> @@ -163,7 +167,10 @@ static int rzg2l_wdt_set_timeout(struct
> watchdog_device *wdev, unsigned int time
>  	 * to reset the module) so that it is updated with new timeout
> values.
>  	 */
>  	if (watchdog_active(wdev)) {
> -		rzg2l_wdt_stop(wdev);
> +		ret = rzg2l_wdt_stop(wdev);
> +		if (ret)
> +			return ret;
> +
>  		ret = rzg2l_wdt_start(wdev);
>  	}
> 
> --
> 2.39.2


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

* Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:32   ` Biju Das
@ 2024-01-31 10:35     ` claudiu beznea
  2024-01-31 10:41       ` Biju Das
  0 siblings, 1 reply; 23+ messages in thread
From: claudiu beznea @ 2024-01-31 10:35 UTC (permalink / raw)
  To: Biju Das, wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

Hi, Biju,

On 31.01.2024 12:32, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, January 31, 2024 10:20 AM
>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>> pm_runtime_put()
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_put() may return an error code. Check its return status.
>>
>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>> propagate the result of rzg2l_wdt_stop() to its caller.
>>
>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>
>>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index d87d4f50180c..7bce093316c4 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device
>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>> +	int ret;
>>
>>  	rzg2l_wdt_reset(priv);
>> -	pm_runtime_put(wdev->parent);
>> +
>> +	ret = pm_runtime_put(wdev->parent);
>> +	if (ret < 0)
>> +		return ret;
> 
> Do we need to check the return code? So far we didn't hit this condition.
> If you are planning to do it, then just 
> 
> return pm_runtime_put(wdev->parent);

pm_runtime_put() may return 1 if the device is suspended (which is not
considered error) as explained here:

https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240122111115.2861835-4-claudiu.beznea.uj@bp.renesas.com/

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
> 
>>
>>  	return 0;
> 
> 
>>  }
>> @@ -163,7 +167,10 @@ static int rzg2l_wdt_set_timeout(struct
>> watchdog_device *wdev, unsigned int time
>>  	 * to reset the module) so that it is updated with new timeout
>> values.
>>  	 */
>>  	if (watchdog_active(wdev)) {
>> -		rzg2l_wdt_stop(wdev);
>> +		ret = rzg2l_wdt_stop(wdev);
>> +		if (ret)
>> +			return ret;
>> +
>>  		ret = rzg2l_wdt_start(wdev);
>>  	}
>>
>> --
>> 2.39.2
> 

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

* RE: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:35     ` claudiu beznea
@ 2024-01-31 10:41       ` Biju Das
  2024-01-31 11:00         ` claudiu beznea
  2024-01-31 13:14         ` Guenter Roeck
  0 siblings, 2 replies; 23+ messages in thread
From: Biju Das @ 2024-01-31 10:41 UTC (permalink / raw)
  To: Claudiu.Beznea, wim, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, January 31, 2024 10:36 AM
> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> Hi, Biju,
> 
> On 31.01.2024 12:32, Biju Das wrote:
> > Hi Claudiu,
> >
> > Thanks for the feedback.
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, January 31, 2024 10:20 AM
> >> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> >> pm_runtime_put()
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> pm_runtime_put() may return an error code. Check its return status.
> >>
> >> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >> propagate the result of rzg2l_wdt_stop() to its caller.
> >>
> >> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
> >> RZ/G2L")
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - propagate the return code of rzg2l_wdt_stop() to it's callers
> >>
> >>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
> >>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
> >> 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
> >> watchdog_device
> >> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
> >>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >> +	int ret;
> >>
> >>  	rzg2l_wdt_reset(priv);
> >> -	pm_runtime_put(wdev->parent);
> >> +
> >> +	ret = pm_runtime_put(wdev->parent);
> >> +	if (ret < 0)
> >> +		return ret;
> >
> > Do we need to check the return code? So far we didn't hit this
> condition.
> > If you are planning to do it, then just
> >
> > return pm_runtime_put(wdev->parent);
> 
> pm_runtime_put() may return 1 if the device is suspended (which is not
> considered error) as explained here:

Oops, I missed that discussion. Out of curiosity,
What watchdog framework/consumer is going to do with a 
Non-error return value of 1?

Cheers,
Biju

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

* Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:41       ` Biju Das
@ 2024-01-31 11:00         ` claudiu beznea
  2024-01-31 13:16           ` Guenter Roeck
  2024-01-31 13:14         ` Guenter Roeck
  1 sibling, 1 reply; 23+ messages in thread
From: claudiu beznea @ 2024-01-31 11:00 UTC (permalink / raw)
  To: Biju Das, wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea



On 31.01.2024 12:41, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, January 31, 2024 10:36 AM
>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>> pm_runtime_put()
>>
>> Hi, Biju,
>>
>> On 31.01.2024 12:32, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the feedback.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>
>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>> propagate the result of rzg2l_wdt_stop() to its caller.
>>>>
>>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
>>>> RZ/G2L")
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>
>>>>  drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>> 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>> watchdog_device
>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>> +	int ret;
>>>>
>>>>  	rzg2l_wdt_reset(priv);
>>>> -	pm_runtime_put(wdev->parent);
>>>> +
>>>> +	ret = pm_runtime_put(wdev->parent);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>> Do we need to check the return code? So far we didn't hit this
>> condition.
>>> If you are planning to do it, then just
>>>
>>> return pm_runtime_put(wdev->parent);
>>
>> pm_runtime_put() may return 1 if the device is suspended (which is not
>> considered error) as explained here:
> 
> Oops, I missed that discussion. Out of curiosity,
> What watchdog framework/consumer is going to do with a 
> Non-error return value of 1?

Looking at this:
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809

it seems that the positive values are not considered errors thus, indeed,
we may return directly:

return pm_runtime_put();

Guenter,

With this (and previous discussion from [1]), are you OK to change it like:

return pm_runtime_put();

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju

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

* Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 10:41       ` Biju Das
  2024-01-31 11:00         ` claudiu beznea
@ 2024-01-31 13:14         ` Guenter Roeck
  2024-01-31 13:38           ` Biju Das
  1 sibling, 1 reply; 23+ messages in thread
From: Guenter Roeck @ 2024-01-31 13:14 UTC (permalink / raw)
  To: Biju Das, Claudiu.Beznea, wim, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

On 1/31/24 02:41, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Wednesday, January 31, 2024 10:36 AM
>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>> pm_runtime_put()
>>
>> Hi, Biju,
>>
>> On 31.01.2024 12:32, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the feedback.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>
>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>> propagate the result of rzg2l_wdt_stop() to its caller.
>>>>
>>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
>>>> RZ/G2L")
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>
>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>> 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>> watchdog_device
>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>> +	int ret;
>>>>
>>>>   	rzg2l_wdt_reset(priv);
>>>> -	pm_runtime_put(wdev->parent);
>>>> +
>>>> +	ret = pm_runtime_put(wdev->parent);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>> Do we need to check the return code? So far we didn't hit this
>> condition.
>>> If you are planning to do it, then just
>>>
>>> return pm_runtime_put(wdev->parent);
>>
>> pm_runtime_put() may return 1 if the device is suspended (which is not
>> considered error) as explained here:
> 
> Oops, I missed that discussion. Out of curiosity,
> What watchdog framework/consumer is going to do with a
> Non-error return value of 1?
> 

You mean what the watchdog subsystem does if a driver violates its API ?
That is undefined. The API says:

* start: this is a pointer to the routine that starts the watchdog timer
   device.
   The routine needs a pointer to the watchdog timer device structure as a
   parameter. It returns zero on success or a negative errno code for failure.
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We are not going to change the API, if that is what you are suggesting.

Thanks,
Guenter


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

* Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 11:00         ` claudiu beznea
@ 2024-01-31 13:16           ` Guenter Roeck
  2024-02-01  6:11             ` claudiu beznea
  0 siblings, 1 reply; 23+ messages in thread
From: Guenter Roeck @ 2024-01-31 13:16 UTC (permalink / raw)
  To: claudiu beznea, Biju Das, wim, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

On 1/31/24 03:00, claudiu beznea wrote:
> 
> 
> On 31.01.2024 12:41, Biju Das wrote:
>> Hi Claudiu,
>>
>>> -----Original Message-----
>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>> Sent: Wednesday, January 31, 2024 10:36 AM
>>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>> pm_runtime_put()
>>>
>>> Hi, Biju,
>>>
>>> On 31.01.2024 12:32, Biju Das wrote:
>>>> Hi Claudiu,
>>>>
>>>> Thanks for the feedback.
>>>>
>>>>> -----Original Message-----
>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>>> pm_runtime_put()
>>>>>
>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>>
>>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>>> propagate the result of rzg2l_wdt_stop() to its caller.
>>>>>
>>>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
>>>>> RZ/G2L")
>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>>
>>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>>> 100644
>>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>>> watchdog_device
>>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>>> +	int ret;
>>>>>
>>>>>   	rzg2l_wdt_reset(priv);
>>>>> -	pm_runtime_put(wdev->parent);
>>>>> +
>>>>> +	ret = pm_runtime_put(wdev->parent);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>
>>>> Do we need to check the return code? So far we didn't hit this
>>> condition.
>>>> If you are planning to do it, then just
>>>>
>>>> return pm_runtime_put(wdev->parent);
>>>
>>> pm_runtime_put() may return 1 if the device is suspended (which is not
>>> considered error) as explained here:
>>
>> Oops, I missed that discussion. Out of curiosity,
>> What watchdog framework/consumer is going to do with a
>> Non-error return value of 1?
> 
> Looking at this:
> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809
> 
> it seems that the positive values are not considered errors thus, indeed,
> we may return directly:
> 
> return pm_runtime_put();
> 
> Guenter,
> 
> With this (and previous discussion from [1]), are you OK to change it like:
> 
> return pm_runtime_put();
> 

Instead of looking at the source, I would kindly ask you to look at the API.

Guenter


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

* RE: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 13:14         ` Guenter Roeck
@ 2024-01-31 13:38           ` Biju Das
  0 siblings, 0 replies; 23+ messages in thread
From: Biju Das @ 2024-01-31 13:38 UTC (permalink / raw)
  To: Guenter Roeck, Claudiu.Beznea, wim, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, geert+renesas, magnus.damm,
	mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

Hi Guenter Roeck,

Thanks for the feedback.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, January 31, 2024 1:14 PM
> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
> pm_runtime_put()
> 
> On 1/31/24 02:41, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >> Sent: Wednesday, January 31, 2024 10:36 AM
> >> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return
> >> status of
> >> pm_runtime_put()
> >>
> >> Hi, Biju,
> >>
> >> On 31.01.2024 12:32, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>> Thanks for the feedback.
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu <claudiu.beznea@tuxon.dev>
> >>>> Sent: Wednesday, January 31, 2024 10:20 AM
> >>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status
> >>>> of
> >>>> pm_runtime_put()
> >>>>
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> pm_runtime_put() may return an error code. Check its return status.
> >>>>
> >>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
> >>>> propagate the result of rzg2l_wdt_stop() to its caller.
> >>>>
> >>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
> >>>> RZ/G2L")
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
> >>>>
> >>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
> >>>>   1 file changed, 9 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
> >>>> 100644
> >>>> --- a/drivers/watchdog/rzg2l_wdt.c
> >>>> +++ b/drivers/watchdog/rzg2l_wdt.c
> >>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
> >>>> watchdog_device
> >>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
> >>>>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >>>> +	int ret;
> >>>>
> >>>>   	rzg2l_wdt_reset(priv);
> >>>> -	pm_runtime_put(wdev->parent);
> >>>> +
> >>>> +	ret = pm_runtime_put(wdev->parent);
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>
> >>> Do we need to check the return code? So far we didn't hit this
> >> condition.
> >>> If you are planning to do it, then just
> >>>
> >>> return pm_runtime_put(wdev->parent);
> >>
> >> pm_runtime_put() may return 1 if the device is suspended (which is
> >> not considered error) as explained here:
> >
> > Oops, I missed that discussion. Out of curiosity, What watchdog
> > framework/consumer is going to do with a Non-error return value of 1?
> >
> 
> You mean what the watchdog subsystem does if a driver violates its API ?
> That is undefined. The API says:

The return value of 1 from pm_runtime_put() is not an error
If it is propagated to framework, it return as an error.
So as you suggested below, the driver needs to either pass zero or error code.
But mot non-error value such as 1.

See watchdog_reboot_notifier():
 
The driver stop() callback may return 1 and reboot won't work
as it is returning NOTIFY_BAD.

ret = wdd->ops->stop(wdd);
trace_watchdog_stop(wdd, ret);
if (ret)
	return NOTIFY_BAD;

> 
> * start: this is a pointer to the routine that starts the watchdog timer
>    device.
>    The routine needs a pointer to the watchdog timer device structure as a
>    parameter. It returns zero on success or a negative errno code for
> failure.
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We are not going to change the API, if that is what you are suggesting.

OK.

Cheers,
Biju

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

* Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  2024-01-31 13:16           ` Guenter Roeck
@ 2024-02-01  6:11             ` claudiu beznea
  0 siblings, 0 replies; 23+ messages in thread
From: claudiu beznea @ 2024-02-01  6:11 UTC (permalink / raw)
  To: Guenter Roeck, Biju Das, wim, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, geert+renesas, magnus.damm, mturquette, sboyd, p.zabel
  Cc: linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea



On 31.01.2024 15:16, Guenter Roeck wrote:
> On 1/31/24 03:00, claudiu beznea wrote:
>>
>>
>> On 31.01.2024 12:41, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: Wednesday, January 31, 2024 10:36 AM
>>>> Subject: Re: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>> pm_runtime_put()
>>>>
>>>> Hi, Biju,
>>>>
>>>> On 31.01.2024 12:32, Biju Das wrote:
>>>>> Hi Claudiu,
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>>>> Sent: Wednesday, January 31, 2024 10:20 AM
>>>>>> Subject: [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of
>>>>>> pm_runtime_put()
>>>>>>
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> pm_runtime_put() may return an error code. Check its return status.
>>>>>>
>>>>>> Along with it the rzg2l_wdt_set_timeout() function was updated to
>>>>>> propagate the result of rzg2l_wdt_stop() to its caller.
>>>>>>
>>>>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for
>>>>>> RZ/G2L")
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - propagate the return code of rzg2l_wdt_stop() to it's callers
>>>>>>
>>>>>>   drivers/watchdog/rzg2l_wdt.c | 11 +++++++++--
>>>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>>>> b/drivers/watchdog/rzg2l_wdt.c index d87d4f50180c..7bce093316c4
>>>>>> 100644
>>>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct
>>>>>> watchdog_device
>>>>>> *wdev)  static int rzg2l_wdt_stop(struct watchdog_device *wdev)  {
>>>>>>       struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>>>> +    int ret;
>>>>>>
>>>>>>       rzg2l_wdt_reset(priv);
>>>>>> -    pm_runtime_put(wdev->parent);
>>>>>> +
>>>>>> +    ret = pm_runtime_put(wdev->parent);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>
>>>>> Do we need to check the return code? So far we didn't hit this
>>>> condition.
>>>>> If you are planning to do it, then just
>>>>>
>>>>> return pm_runtime_put(wdev->parent);
>>>>
>>>> pm_runtime_put() may return 1 if the device is suspended (which is not
>>>> considered error) as explained here:
>>>
>>> Oops, I missed that discussion. Out of curiosity,
>>> What watchdog framework/consumer is going to do with a
>>> Non-error return value of 1?
>>
>> Looking at this:
>> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L809
>>
>> it seems that the positive values are not considered errors thus, indeed,
>> we may return directly:
>>
>> return pm_runtime_put();
>>
>> Guenter,
>>
>> With this (and previous discussion from [1]), are you OK to change it like:
>>
>> return pm_runtime_put();
>>
> 
> Instead of looking at the source, I would kindly ask you to look at the API.

OK, I'll keep the patch as is. Thank you for your input.

Claudiu Beznea

> 
> Guenter
> 

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

* Re: [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM
  2024-01-31 10:20 ` [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM Claudiu
@ 2024-02-01  8:52   ` Geert Uytterhoeven
  2024-02-01 13:36     ` Guenter Roeck
  2024-02-01 21:05   ` kernel test robot
  1 sibling, 1 reply; 23+ messages in thread
From: Geert Uytterhoeven @ 2024-02-01  8:52 UTC (permalink / raw)
  To: Claudiu
  Cc: wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz, linux-watchdog, devicetree, linux-kernel,
	linux-renesas-soc, linux-clk, Claudiu Beznea

Hi Claudiu,

On Thu, Feb 1, 2024 at 2:30 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The rzg2l_wdt watchdog driver cannot work w/o CONFIG_PM=y (e.g. the
> clocks are enabled though pm_runtime_* specific APIs). To avoid building
> a driver that don't work select CONFIG_PM.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -912,6 +912,7 @@ config RENESAS_RZG2LWDT
>         tristate "Renesas RZ/G2L WDT Watchdog"
>         depends on ARCH_RENESAS || COMPILE_TEST
>         select WATCHDOG_CORE
> +       select PM

depends on PM

The availability of PM is architecture/platform-specific, hence it
must not be selected by individual drivers.

>         help
>           This driver adds watchdog support for the integrated watchdogs in the
>           Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM
  2024-02-01  8:52   ` Geert Uytterhoeven
@ 2024-02-01 13:36     ` Guenter Roeck
  0 siblings, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2024-02-01 13:36 UTC (permalink / raw)
  To: Geert Uytterhoeven, Claudiu
  Cc: wim, robh+dt, krzysztof.kozlowski+dt, conor+dt, geert+renesas,
	magnus.damm, mturquette, sboyd, p.zabel, biju.das.jz,
	linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, Claudiu Beznea

On 2/1/24 00:52, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Feb 1, 2024 at 2:30 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The rzg2l_wdt watchdog driver cannot work w/o CONFIG_PM=y (e.g. the
>> clocks are enabled though pm_runtime_* specific APIs). To avoid building
>> a driver that don't work select CONFIG_PM.
>>
>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -912,6 +912,7 @@ config RENESAS_RZG2LWDT
>>          tristate "Renesas RZ/G2L WDT Watchdog"
>>          depends on ARCH_RENESAS || COMPILE_TEST
>>          select WATCHDOG_CORE
>> +       select PM
> 
> depends on PM
> 

Yes, I did not want to suggest that the driver should _select_ PM.
Sorry that I wasn't more specific.

Guenter

> The availability of PM is architecture/platform-specific, hence it
> must not be selected by individual drivers.
> 
>>          help
>>            This driver adds watchdog support for the integrated watchdogs in the
>>            Renesas RZ/G2L SoCs. These watchdogs can be used to reset a system.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
> 


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

* Re: [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM
  2024-01-31 10:20 ` [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM Claudiu
  2024-02-01  8:52   ` Geert Uytterhoeven
@ 2024-02-01 21:05   ` kernel test robot
  1 sibling, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-02-01 21:05 UTC (permalink / raw)
  To: Claudiu, wim, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	geert+renesas, magnus.damm, mturquette, sboyd, p.zabel,
	biju.das.jz
  Cc: Paul Gazzillo, Necip Fazil Yildiran, oe-kbuild-all,
	linux-watchdog, devicetree, linux-kernel, linux-renesas-soc,
	linux-clk, claudiu.beznea, Claudiu Beznea

Hi Claudiu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on geert-renesas-devel/next]
[also build test WARNING on robh/for-next groeck-staging/hwmon-next linus/master v6.8-rc2]
[cannot apply to geert-renesas-drivers/renesas-clk next-20240201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Claudiu/clk-renesas-r9a08g045-Add-clock-and-reset-support-for-watchdog/20240131-182642
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
patch link:    https://lore.kernel.org/r/20240131102017.1841495-3-claudiu.beznea.uj%40bp.renesas.com
patch subject: [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM
config: m68k-kismet-CONFIG_PM-CONFIG_RENESAS_RZG2LWDT-0-0 (https://download.01.org/0day-ci/archive/20240202/202402020445.TOBlFPcS-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20240202/202402020445.TOBlFPcS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402020445.TOBlFPcS-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PM when selected by RENESAS_RZG2LWDT
   .config:35:warning: symbol value 'n' invalid for RAPIDIO_DISC_TIMEOUT
   .config:63:warning: symbol value 'n' invalid for FAT_DEFAULT_CODEPAGE
   .config:193:warning: symbol value 'n' invalid for KERNELBASE
   .config:231:warning: symbol value 'n' invalid for SATA_MOBILE_LPM_POLICY
   .config:333:warning: symbol value 'n' invalid for PSTORE_BLK_MAX_REASON
   .config:428:warning: symbol value 'n' invalid for KFENCE_SAMPLE_INTERVAL
   .config:462:warning: symbol value 'n' invalid for AIC79XX_DEBUG_MASK
   .config:605:warning: symbol value 'n' invalid for CRYPTO_DEV_QCE_SW_MAX_LEN
   .config:607:warning: symbol value 'n' invalid for DRM_XE_JOB_TIMEOUT_MIN
   .config:708:warning: symbol value 'n' invalid for PANEL_LCD_CHARSET
   .config:763:warning: symbol value 'n' invalid for INPUT_MOUSEDEV_SCREEN_Y
   .config:785:warning: symbol value 'n' invalid for SND_AC97_POWER_SAVE_DEFAULT
   .config:823:warning: symbol value 'n' invalid for DRM_I915_MAX_REQUEST_BUSYWAIT
   .config:824:warning: symbol value 'n' invalid for AIC79XX_CMDS_PER_DEVICE
   .config:870:warning: symbol value 'n' invalid for PANEL_LCD_PIN_SDA
   .config:884:warning: symbol value 'n' invalid for DRM_XE_PREEMPT_TIMEOUT_MIN
   .config:895:warning: symbol value 'n' invalid for NET_EMATCH_STACK
   .config:897:warning: symbol value 'n' invalid for VMCP_CMA_SIZE
   .config:1053:warning: symbol value 'n' invalid for USB_GADGET_STORAGE_NUM_BUFFERS
   .config:1118:warning: symbol value 'n' invalid for RCU_CPU_STALL_TIMEOUT
   .config:1143:warning: symbol value 'n' invalid for MTDRAM_ERASE_SIZE
   .config:1218:warning: symbol value 'n' invalid for SERIAL_UARTLITE_NR_UARTS
   .config:1378:warning: symbol value 'n' invalid for LEGACY_PTY_COUNT
   .config:1461:warning: symbol value 'n' invalid for PANEL_LCD_PIN_E
   .config:1505:warning: symbol value 'n' invalid for AIC7XXX_RESET_DELAY_MS
   .config:1706:warning: symbol value 'n' invalid for IBM_EMAC_POLL_WEIGHT
   .config:1783:warning: symbol value 'n' invalid for DRM_I915_STOP_TIMEOUT
   .config:1882:warning: symbol value 'n' invalid for PANEL_PROFILE
   .config:1953:warning: symbol value 'n' invalid for ROMVEC
   .config:2001:warning: symbol value 'n' invalid for KCOV_IRQ_AREA_SIZE
   .config:2016:warning: symbol value 'n' invalid for SCSI_MESH_RESET_DELAY_MS
   .config:2112:warning: symbol value 'n' invalid for RCU_FANOUT_LEAF
   .config:2204:warning: symbol value 'n' invalid for DRM_XE_TIMESLICE_MAX
   .config:2254:warning: symbol value 'n' invalid for PANEL_LCD_BWIDTH
   .config:2487:warning: symbol value 'n' invalid for PANEL_PARPORT
   .config:2536:warning: symbol value 'n' invalid for SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST_NUM
   .config:2573:warning: symbol value 'n' invalid for NOUVEAU_DEBUG_DEFAULT
   .config:2591:warning: symbol value 'n' invalid for AIC79XX_RESET_DELAY_MS
   .config:2759:warning: symbol value 'n' invalid for KCSAN_REPORT_ONCE_IN_MS
   .config:2854:warning: symbol value 'n' invalid for KCSAN_UDELAY_INTERRUPT
   .config:2878:warning: symbol value 'n' invalid for PANEL_LCD_PIN_BL
   .config:2896:warning: symbol value 'n' invalid for DEBUG_OBJECTS_ENABLE_DEFAULT
   .config:2902:warning: symbol value 'n' invalid for INITRAMFS_ROOT_GID
   .config:2933:warning: symbol value 'n' invalid for PSTORE_BLK_CONSOLE_SIZE
   .config:3007:warning: symbol value 'n' invalid for ATM_FORE200E_TX_RETRY
   .config:3013:warning: symbol value 'n' invalid for SERIAL_ALTERA_UART_BAUDRATE
   .config:3049:warning: symbol value 'n' invalid for FB_OMAP2_DSS_MIN_FCK_PER_PCK
   .config:3098:warning: symbol value 'n' invalid for BOOKE_WDT_DEFAULT_TIMEOUT
   .config:3142:warning: symbol value 'n' invalid for DUMMY_CONSOLE_ROWS
   .config:3167:warning: symbol value 'n' invalid for MTD_REDBOOT_DIRECTORY_BLOCK
   .config:3236:warning: symbol value 'n' invalid for KCSAN_UDELAY_TASK
   .config:3371:warning: symbol value 'n' invalid for MMC_BLOCK_MINORS
   .config:3374:warning: symbol value 'n' invalid for INET_TABLE_PERTURB_ORDER
   .config:3415:warning: symbol value 'n' invalid for SCSI_NCR53C8XX_SYNC
   .config:3532:warning: symbol value 'n' invalid for UCLAMP_BUCKETS_COUNT
   .config:3558:warning: symbol value 'n' invalid for SERIAL_MCF_BAUDRATE
   .config:3634:warning: symbol value 'n' invalid for DE2104X_DSL
   .config:3647:warning: symbol value 'n' invalid for BLK_DEV_RAM_COUNT
   .config:3685:warning: symbol value 'n' invalid for STACK_MAX_DEFAULT_SIZE_MB
   .config:3797:warning: symbol value 'n' invalid for RAMSIZE
   .config:3874:warning: symbol value 'n' invalid for IP_VS_SH_TAB_BITS
   .config:3993:warning: symbol value 'n' invalid for USBIP_VHCI_HC_PORTS
   .config:4101:warning: symbol value 'n' invalid for INPUT_MOUSEDEV_SCREEN_X
   .config:4174:warning: symbol value 'n' invalid for FTRACE_RECORD_RECURSION_SIZE
   .config:4216:warning: symbol value 'n' invalid for RIONET_RX_SIZE
   .config:4313:warning: symbol value 'n' invalid for RADIO_TYPHOON_PORT
   .config:4428:warning: symbol value 'n' invalid for SERIAL_TXX9_NR_UARTS
   .config:4512:warning: symbol value 'n' invalid for IBM_EMAC_TXB
   .config:4886:warning: symbol value 'n' invalid for ARCH_MMAP_RND_BITS
   .config:4922:warning: symbol value 'n' invalid for IP_VS_MH_TAB_INDEX
   .config:4964:warning: symbol value 'n' invalid for DRM_I915_FENCE_TIMEOUT
   .config:4986:warning: symbol value 'n' invalid for TTY_PRINTK_LEVEL
   .config:5135:warning: symbol value 'n' invalid for MIPS_EJTAG_FDC_KGDB_CHAN
   .config:5226:warning: symbol value 'n' invalid for KDB_DEFAULT_ENABLE
   .config:5245:warning: symbol value 'n' invalid for SERIAL_ALTERA_UART_MAXPORTS
   .config:5282:warning: symbol value 'n' invalid for PPC_EARLY_DEBUG_EHV_BC_HANDLE
   .config:5388:warning: symbol value 'n' invalid for PANEL_LCD_PIN_RW
   .config:5435:warning: symbol value 'n' invalid for CRYPTO_DEV_FSL_CAAM_INTC_TIME_THLD
   .config:5501:warning: symbol value 'n' invalid for PANEL_LCD_HWIDTH
   .config:5515:warning: symbol value 'n' invalid for MBAR
   .config:5538:warning: symbol value 'n' invalid for LOCKDEP_CHAINS_BITS
   .config:5621:warning: symbol value 'n' invalid for DRM_I915_HEARTBEAT_INTERVAL
   .config:5627:warning: symbol value 'n' invalid for KCSAN_SKIP_WATCH
   .config:5649:warning: symbol value 'n' invalid for PSTORE_BLK_KMSG_SIZE
   .config:5846:warning: symbol value 'n' invalid for SERIAL_8250_RUNTIME_UARTS
   .config:5870:warning: symbol value 'n' invalid for VECTORBASE
   .config:5922:warning: symbol value 'n' invalid for IPSBAR
   .config:5939:warning: symbol value 'n' invalid for ARCH_MMAP_RND_COMPAT_BITS
   .config:6041:warning: symbol value 'n' invalid for RCU_BOOST_DELAY
   .config:6061:warning: symbol value 'n' invalid for SERIAL_SH_SCI_NR_UARTS
   .config:6097:warning: symbol value 'n' invalid for RADIO_TRUST_PORT
   .config:6448:warning: symbol value 'n' invalid for CMA_SIZE_PERCENTAGE
   .config:6486:warning: symbol value 'n' invalid for DRM_XE_PREEMPT_TIMEOUT_MAX
   .config:6593:warning: symbol value 'n' invalid for DRM_XE_TIMESLICE_MIN
   .config:6631:warning: symbol value 'n' invalid for SCSI_NCR53C8XX_MAX_TAGS
   .config:6633:warning: symbol value 'n' invalid for DVB_MAX_ADAPTERS
   .config:6647:warning: symbol value 'n' invalid for SCSI_SYM53C8XX_DMA_ADDRESSING_MODE
   .config:6945:warning: symbol value 'n' invalid for SCSI_SYM53C8XX_MAX_TAGS
   .config:6995:warning: symbol value 'n' invalid for IBM_EMAC_RXB
   .config:7029:warning: symbol value 'n' invalid for MTD_UBI_WL_THRESHOLD

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

end of thread, other threads:[~2024-02-01 21:05 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-31 10:20 [PATCH v2 00/11] watchdog: rzg2l_wdt: Add support for RZ/G3S Claudiu
2024-01-31 10:20 ` [PATCH v2 01/11] clk: renesas: r9a08g045: Add clock and reset support for watchdog Claudiu
2024-01-31 10:20 ` [PATCH v2 02/11] watchdog: rzg2l_wdt: Select PM Claudiu
2024-02-01  8:52   ` Geert Uytterhoeven
2024-02-01 13:36     ` Guenter Roeck
2024-02-01 21:05   ` kernel test robot
2024-01-31 10:20 ` [PATCH v2 03/11] watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get() Claudiu
2024-01-31 10:20 ` [PATCH v2 04/11] watchdog: rzg2l_wdt: Check return status of pm_runtime_put() Claudiu
2024-01-31 10:32   ` Biju Das
2024-01-31 10:35     ` claudiu beznea
2024-01-31 10:41       ` Biju Das
2024-01-31 11:00         ` claudiu beznea
2024-01-31 13:16           ` Guenter Roeck
2024-02-01  6:11             ` claudiu beznea
2024-01-31 13:14         ` Guenter Roeck
2024-01-31 13:38           ` Biju Das
2024-01-31 10:20 ` [PATCH v2 05/11] watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop Claudiu
2024-01-31 10:20 ` [PATCH v2 06/11] watchdog: rzg2l_wdt: Remove comparison with zero Claudiu
2024-01-31 10:20 ` [PATCH v2 07/11] watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset Claudiu
2024-01-31 10:20 ` [PATCH v2 08/11] watchdog: rzg2l_wdt: Add suspend/resume support Claudiu
2024-01-31 10:20 ` [PATCH v2 09/11] dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support Claudiu
2024-01-31 10:20 ` [PATCH v2 10/11] arm64: dts: renesas: r9a08g045: Add watchdog node Claudiu
2024-01-31 10:20 ` [PATCH v2 11/11] arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface Claudiu

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