[PATCH 0/3] watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain

[PATCH 0/3] watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain

[Claudiu]

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

Hi,

Watchdog device available on RZ/G3S SoC is part of a software-controlled
power domain. The watchdog driver implements struct
watchdog_ops::restart() handler which is called in atomic context via
this call chain:

kernel_restart() ->
  machine_restart() ->
    do_kernel_restart() ->
      atomic_notifier_call_chain() ->
        watchdog_restart_notifier()
          rzg2l_wdt_restart()

When the rzg2l_wdt_restart() is called it may happen that the watchdog
clocks to be disabled and the associated power domain to be off.
Accessing watchdog registers in this state leads to aborts and system
blocks.

To solve this issue the watchdog power domain was marked as IRQ safe
as well as watchdog device (as proposed by Ulf Hansson). Along with
it the clk_prepare_enable() calls from the watchdog restart() handler
were removed and all is based now on pm_runtime_resume_and_get()
as explained in patch 03/03.

Series contains also power domain driver changes to be able to
register the watchdog PM domain as an IRQ safe one.

Initial RFC series for solving this issue was posted at [1].

It is safe to merge watchdog and PM domain driver changes though
different trees.

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/all/20240619120920.2703605-1-claudiu.beznea.uj@bp.renesas.com/

Changes since RFC:
- dropped patches 01/03, 02/03 from RFC
- adjusted power domain driver to be able to register the
  watchdog PM domain as an IRQ safe one
- drop clock prepare approach from watchdog driver presented in RFC
  and rely only on pm_runtime_resume_and_get()
- mark the watchdog device as IRQ safe

Claudiu Beznea (3):
  clk: renesas: rzg2l-cpg: Use GENPD_FLAG_* flags instead of local ones
  clk: renesas: rzg2l-cpg: Mark watchdog and always-on domains as IRQ
    safe
  watchdog: rzg2l_wdt: Power on the watchdog domain in the restart
    handler

 drivers/clk/renesas/r9a08g045-cpg.c | 43 ++++++++++++-----------------
 drivers/clk/renesas/rzg2l-cpg.c     | 13 +++++----
 drivers/clk/renesas/rzg2l-cpg.h     | 10 ++-----
 drivers/watchdog/rzg2l_wdt.c        | 21 ++++++++++++--
 4 files changed, 47 insertions(+), 40 deletions(-)

-- 
2.39.2

[PATCH 1/3] clk: renesas: rzg2l-cpg: Use GENPD_FLAG_* flags instead of local ones

[Claudiu]

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

For watchdog PM domain it is necessary to provide GENPD_FLAG_IRQ_SAFE flag
to be able to power on the watchdog PM domain from atomic context. For
this, adjust the current infrastructure to be able to provide GENPD_FLAG_*
for individual PM domains.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/clk/renesas/r9a08g045-cpg.c | 44 +++++++++++------------------
 drivers/clk/renesas/rzg2l-cpg.c     | 13 +++++----
 drivers/clk/renesas/rzg2l-cpg.h     | 10 ++-----
 3 files changed, 28 insertions(+), 39 deletions(-)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index 213499fc8fb5..ec0672651fe0 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/pm_domain.h>
 
 #include <dt-bindings/clock/r9a08g045-cpg.h>
 
@@ -258,52 +259,41 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 	/* Keep always-on domain on the first position for proper domains registration. */
 	DEF_PD("always-on",	R9A08G045_PD_ALWAYS_ON,
 				DEF_REG_CONF(0, 0),
-				RZG2L_PD_F_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("gic",		R9A08G045_PD_GIC,
 				DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
-				RZG2L_PD_F_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("ia55",		R9A08G045_PD_IA55,
 				DEF_REG_CONF(CPG_BUS_PERI_CPU_MSTOP, BIT(13)),
-				RZG2L_PD_F_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("dmac",		R9A08G045_PD_DMAC,
 				DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
-				RZG2L_PD_F_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("wdt0",		R9A08G045_PD_WDT0,
-				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
 	DEF_PD("sdhi0",		R9A08G045_PD_SDHI0,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
 	DEF_PD("sdhi1",		R9A08G045_PD_SDHI1,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(1)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(1)), 0),
 	DEF_PD("sdhi2",		R9A08G045_PD_SDHI2,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(11)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(11)), 0),
 	DEF_PD("eth0",		R9A08G045_PD_ETHER0,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(2)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(2)), 0),
 	DEF_PD("eth1",		R9A08G045_PD_ETHER1,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(3)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(3)), 0),
 	DEF_PD("i2c0",		R9A08G045_PD_I2C0,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(10)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(10)), 0),
 	DEF_PD("i2c1",		R9A08G045_PD_I2C1,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(11)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(11)), 0),
 	DEF_PD("i2c2",		R9A08G045_PD_I2C2,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(12)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(12)), 0),
 	DEF_PD("i2c3",		R9A08G045_PD_I2C3,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)), 0),
 	DEF_PD("scif0",		R9A08G045_PD_SCIF0,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)),
-				RZG2L_PD_F_NONE),
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)), 0),
 	DEF_PD("vbat",		R9A08G045_PD_VBAT,
 				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(8)),
-				RZG2L_PD_F_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON),
 };
 
 const struct rzg2l_cpg_info r9a08g045_cpg_info = {
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 88bf39e8c79c..b97996e93042 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -1680,11 +1680,13 @@ static int rzg2l_cpg_power_off(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd, bool always_on)
+static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd, u32 genpd_flags,
+				     bool always_on)
 {
 	struct dev_power_governor *governor;
 
-	pd->genpd.flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
+	pd->genpd.flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP |
+			   genpd_flags;
 	pd->genpd.attach_dev = rzg2l_cpg_attach_dev;
 	pd->genpd.detach_dev = rzg2l_cpg_detach_dev;
 	if (always_on) {
@@ -1712,7 +1714,7 @@ static int __init rzg2l_cpg_add_clk_domain(struct rzg2l_cpg_priv *priv)
 
 	pd->genpd.name = np->name;
 	pd->priv = priv;
-	ret = rzg2l_cpg_pd_setup(pd, true);
+	ret = rzg2l_cpg_pd_setup(pd, 0, true);
 	if (ret)
 		return ret;
 
@@ -1777,7 +1779,8 @@ static int __init rzg2l_cpg_add_pm_domains(struct rzg2l_cpg_priv *priv)
 		return ret;
 
 	for (unsigned int i = 0; i < info->num_pm_domains; i++) {
-		bool always_on = !!(info->pm_domains[i].flags & RZG2L_PD_F_ALWAYS_ON);
+		u32 genpd_flags = info->pm_domains[i].genpd_flags;
+		bool always_on = !!(genpd_flags & GENPD_FLAG_ALWAYS_ON);
 		struct rzg2l_cpg_pd *pd;
 
 		pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
@@ -1789,7 +1792,7 @@ static int __init rzg2l_cpg_add_pm_domains(struct rzg2l_cpg_priv *priv)
 		pd->id = info->pm_domains[i].id;
 		pd->priv = priv;
 
-		ret = rzg2l_cpg_pd_setup(pd, always_on);
+		ret = rzg2l_cpg_pd_setup(pd, genpd_flags, always_on);
 		if (ret)
 			return ret;
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index ecfe7e7ea8a1..881a89b5a710 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -270,14 +270,14 @@ struct rzg2l_cpg_pm_domain_conf {
  * struct rzg2l_cpg_pm_domain_init_data - PM domain init data
  * @name: PM domain name
  * @conf: PM domain configuration
- * @flags: RZG2L PM domain flags (see RZG2L_PD_F_*)
+ * @genpd_flags: genpd flags (see GENPD_FLAG_*)
  * @id: PM domain ID (similar to the ones defined in
  *      include/dt-bindings/clock/<soc-id>-cpg.h)
  */
 struct rzg2l_cpg_pm_domain_init_data {
 	const char * const name;
 	struct rzg2l_cpg_pm_domain_conf conf;
-	u32 flags;
+	u32 genpd_flags;
 	u16 id;
 };
 
@@ -288,13 +288,9 @@ struct rzg2l_cpg_pm_domain_init_data {
 		.conf = { \
 			.mstop = (_mstop_conf), \
 		}, \
-		.flags = (_flags), \
+		.genpd_flags = (_flags), \
 	}
 
-/* Power domain flags. */
-#define RZG2L_PD_F_ALWAYS_ON	BIT(0)
-#define RZG2L_PD_F_NONE		(0)
-
 /**
  * struct rzg2l_cpg_info - SoC-specific CPG Description
  *
-- 
2.39.2

[PATCH 2/3] clk: renesas: rzg2l-cpg: Mark watchdog and always-on PM domains as IRQ safe

[Claudiu]

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

If the watchdog is part of a dedicated power domain (as it may be on
RZ/G3S) the watchdog PM domain need to be powered on in the watchdog
restart handler. Currently, only the clocks are enabled in the watchdog
restart handler. To be able to also power on the PM domain we need to
call pm_runtime_resume_and_get() on the watchdog restart handler, mark
the watchdog device as IRQ safe and register the watchdog PM domain
with GENPD_FLAG_IRQ_SAFE.

Register watchdog PM domain as IRQ safe. Along with it the always-on
PM domain (parent of the watchdog domain) was marked as IRQ safe.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/clk/renesas/r9a08g045-cpg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index ec0672651fe0..8e4f17c21dd7 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -259,7 +259,7 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 	/* Keep always-on domain on the first position for proper domains registration. */
 	DEF_PD("always-on",	R9A08G045_PD_ALWAYS_ON,
 				DEF_REG_CONF(0, 0),
-				GENPD_FLAG_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_IRQ_SAFE),
 	DEF_PD("gic",		R9A08G045_PD_GIC,
 				DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
 				GENPD_FLAG_ALWAYS_ON),
@@ -270,7 +270,8 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 				DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
 				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("wdt0",		R9A08G045_PD_WDT0,
-				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
+				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
+				GENPD_FLAG_IRQ_SAFE),
 	DEF_PD("sdhi0",		R9A08G045_PD_SDHI0,
 				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
 	DEF_PD("sdhi1",		R9A08G045_PD_SDHI1,
-- 
2.39.2

[PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[Claudiu]

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

On RZ/G3S the watchdog can be part of a software-controlled PM domain. In
this case, the watchdog device need to be powered on in
struct watchdog_ops::restart API. This can be done though
pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog
device are marked as IRQ safe. We mark the watchdog PM domain as IRQ safe
with GENPD_FLAG_IRQ_SAFE when the watchdog PM domain is registered and the
watchdog device though pm_runtime_irq_safe().

Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
context'") pm_runtime_get_sync() was used in watchdog restart handler
(which is similar to pm_runtime_resume_and_get() except the later one
handles the runtime resume errors).

Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
context'") dropped the pm_runtime_get_sync() and replaced it with
clk_prepare_enable() to avoid invalid wait context due to genpd_lock()
in genpd_runtime_resume() being called from atomic context. But
clk_prepare_enable() doesn't fit for this either (as reported by
Ulf Hansson) as clk_prepare() can also sleep (it just not throw invalid
wait context warning as it is not written for this).

Because the watchdog device is marked now as IRQ safe (though this patch)
the irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume() returns
1 for devices not registering an IRQ safe PM domain for watchdog (as the
watchdog device is IRQ safe, PM domain is not and watchdog PM domain is
always-on), this being the case of RZ/G2 devices that uses this driver,
we can now drop also the clk_prepare_enable() calls in restart handler and
rely on pm_runtime_resume_and_get().

Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in
watchdog restart handler.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 2a35f890a288..e9e0408c96f7 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/units.h>
@@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 	int ret;
 
-	clk_prepare_enable(priv->pclk);
-	clk_prepare_enable(priv->osc_clk);
+	/*
+	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
+	 * domain that is currently powered off. In this case we need to power
+	 * it on before accessing registers. Along with this the clocks will be
+	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
+	 * need to be on for the reboot to happen.
+	 *
+	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device trees
+	 * where PM domains are registered like on RZ/G2 devices) it is safe
+	 * to call pm_runtime_resume_and_get() as the
+	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
+	 * returns non zero value and the genpd_lock() is avoided, thus, there
+	 * will be no invalid wait context reported by lockdep.
+	 */
+	ret = pm_runtime_resume_and_get(wdev->parent);
+	if (ret)
+		return ret;
 
 	if (priv->devtype == WDT_RZG2L) {
 		ret = reset_control_deassert(priv->rstc);
@@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
+	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
-- 
2.39.2

Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[Guenter Roeck]

On 8/26/24 08:25, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On RZ/G3S the watchdog can be part of a software-controlled PM domain. In
> this case, the watchdog device need to be powered on in
> struct watchdog_ops::restart API. This can be done though
> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog
> device are marked as IRQ safe. We mark the watchdog PM domain as IRQ safe
> with GENPD_FLAG_IRQ_SAFE when the watchdog PM domain is registered and the
> watchdog device though pm_runtime_irq_safe().
> 
> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") pm_runtime_get_sync() was used in watchdog restart handler
> (which is similar to pm_runtime_resume_and_get() except the later one
> handles the runtime resume errors).
> 
> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") dropped the pm_runtime_get_sync() and replaced it with
> clk_prepare_enable() to avoid invalid wait context due to genpd_lock()
> in genpd_runtime_resume() being called from atomic context. But
> clk_prepare_enable() doesn't fit for this either (as reported by
> Ulf Hansson) as clk_prepare() can also sleep (it just not throw invalid
> wait context warning as it is not written for this).
> 
> Because the watchdog device is marked now as IRQ safe (though this patch)
> the irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume() returns
> 1 for devices not registering an IRQ safe PM domain for watchdog (as the
> watchdog device is IRQ safe, PM domain is not and watchdog PM domain is
> always-on), this being the case of RZ/G2 devices that uses this driver,
> we can now drop also the clk_prepare_enable() calls in restart handler and
> rely on pm_runtime_resume_and_get().
> 
> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in
> watchdog restart handler.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

RE: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[Biju Das]

Hi Claudiu,

Thanks for the feedback.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Monday, August 26, 2024 4:25 PM
> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On RZ/G3S the watchdog can be part of a software-controlled PM domain. In this case, the watchdog
> device need to be powered on in struct watchdog_ops::restart API. This can be done though
> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog device are marked as IRQ safe.
> We mark the watchdog PM domain as IRQ safe with GENPD_FLAG_IRQ_SAFE when the watchdog PM domain is
> registered and the watchdog device though pm_runtime_irq_safe().
> 
> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") pm_runtime_get_sync() was used in watchdog restart handler (which is similar to
> pm_runtime_resume_and_get() except the later one handles the runtime resume errors).
> 
> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") dropped the pm_runtime_get_sync() and replaced it with
> clk_prepare_enable() to avoid invalid wait context due to genpd_lock() in genpd_runtime_resume() being
> called from atomic context. But
> clk_prepare_enable() doesn't fit for this either (as reported by Ulf Hansson) as clk_prepare() can
> also sleep (it just not throw invalid wait context warning as it is not written for this).
> 
> Because the watchdog device is marked now as IRQ safe (though this patch) the
> irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume() returns
> 1 for devices not registering an IRQ safe PM domain for watchdog (as the watchdog device is IRQ safe,
> PM domain is not and watchdog PM domain is always-on), this being the case of RZ/G2 devices that uses

RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is not applicable for
RZ/G2{H,M,N,E}devices.


> this driver, we can now drop also the clk_prepare_enable() calls in restart handler and rely on
> pm_runtime_resume_and_get().
> 
> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in watchdog restart handler.

Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??

Cheers,
Biju

> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
> 2a35f890a288..e9e0408c96f7 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  #include <linux/units.h>
> @@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>  	int ret;
> 
> -	clk_prepare_enable(priv->pclk);
> -	clk_prepare_enable(priv->osc_clk);
> +	/*
> +	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
> +	 * domain that is currently powered off. In this case we need to power
> +	 * it on before accessing registers. Along with this the clocks will be
> +	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
> +	 * need to be on for the reboot to happen.
> +	 *
> +	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device trees
> +	 * where PM domains are registered like on RZ/G2 devices) it is safe
> +	 * to call pm_runtime_resume_and_get() as the
> +	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
> +	 * returns non zero value and the genpd_lock() is avoided, thus, there
> +	 * will be no invalid wait context reported by lockdep.
> +	 */
> +	ret = pm_runtime_resume_and_get(wdev->parent);
> +	if (ret)
> +		return ret;
> 
>  	if (priv->devtype == WDT_RZG2L) {
>  		ret = reset_control_deassert(priv->rstc);
> @@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
> 
>  	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
> 
> +	pm_runtime_irq_safe(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> 
>  	priv->wdev.info = &rzg2l_wdt_ident;
> --
> 2.39.2
> 

Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[claudiu beznea]

Hi, Biju,

On 27.08.2024 15:15, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Monday, August 26, 2024 4:25 PM
>> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On RZ/G3S the watchdog can be part of a software-controlled PM domain. In this case, the watchdog
>> device need to be powered on in struct watchdog_ops::restart API. This can be done though
>> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog device are marked as IRQ safe.
>> We mark the watchdog PM domain as IRQ safe with GENPD_FLAG_IRQ_SAFE when the watchdog PM domain is
>> registered and the watchdog device though pm_runtime_irq_safe().
>>
>> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
>> context'") pm_runtime_get_sync() was used in watchdog restart handler (which is similar to
>> pm_runtime_resume_and_get() except the later one handles the runtime resume errors).
>>
>> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
>> context'") dropped the pm_runtime_get_sync() and replaced it with
>> clk_prepare_enable() to avoid invalid wait context due to genpd_lock() in genpd_runtime_resume() being
>> called from atomic context. But
>> clk_prepare_enable() doesn't fit for this either (as reported by Ulf Hansson) as clk_prepare() can
>> also sleep (it just not throw invalid wait context warning as it is not written for this).
>>
>> Because the watchdog device is marked now as IRQ safe (though this patch) the
>> irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume() returns
>> 1 for devices not registering an IRQ safe PM domain for watchdog (as the watchdog device is IRQ safe,
>> PM domain is not and watchdog PM domain is always-on), this being the case of RZ/G2 devices that uses
> 
> RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is not applicable for
> RZ/G2{H,M,N,E}devices.

OK, but I said "RZ/G2 devices that uses this driver". Here are included
RZ/{G2L,G2LC,G2UL,V2L} AFAICT.

> 
> 
>> this driver, we can now drop also the clk_prepare_enable() calls in restart handler and rely on
>> pm_runtime_resume_and_get().
>>
>> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in watchdog restart handler.
> 
> Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
>> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??

Not sure... I thought about it, too. I chose to have it like this thinking
that:

1/ that may not apply cleanly as it depends on other cleanups done on this
   driver, e.g. commit d8997ed79ed7 ("watchdog: rzg2l_wdt: Rely on the
   reset driver for doing proper reset") so it may be worthless for
   backport machinery
2/ There is actually no seen bug reported by lockdep (as the clk_prepare()
   doesn't handle it)

Don't know, I can reply here and add it. Applying this patch with b4 will
take care of it. But not sure about it.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
> 
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index
>> 2a35f890a288..e9e0408c96f7 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/reset.h>
>>  #include <linux/units.h>
>> @@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>  	int ret;
>>
>> -	clk_prepare_enable(priv->pclk);
>> -	clk_prepare_enable(priv->osc_clk);
>> +	/*
>> +	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
>> +	 * domain that is currently powered off. In this case we need to power
>> +	 * it on before accessing registers. Along with this the clocks will be
>> +	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
>> +	 * need to be on for the reboot to happen.
>> +	 *
>> +	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device trees
>> +	 * where PM domains are registered like on RZ/G2 devices) it is safe
>> +	 * to call pm_runtime_resume_and_get() as the
>> +	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
>> +	 * returns non zero value and the genpd_lock() is avoided, thus, there
>> +	 * will be no invalid wait context reported by lockdep.
>> +	 */
>> +	ret = pm_runtime_resume_and_get(wdev->parent);
>> +	if (ret)
>> +		return ret;
>>
>>  	if (priv->devtype == WDT_RZG2L) {
>>  		ret = reset_control_deassert(priv->rstc);
>> @@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>>
>>  	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>>
>> +	pm_runtime_irq_safe(&pdev->dev);
>>  	pm_runtime_enable(&pdev->dev);
>>
>>  	priv->wdev.info = &rzg2l_wdt_ident;
>> --
>> 2.39.2
>>
> 

RE: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[Biju Das]

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, August 27, 2024 1:33 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be; mturquette@baylibre.com;
> sboyd@kernel.org; wim@linux-watchdog.org; linux@roeck-us.net; ulf.hansson@linaro.org
> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org; linux-pm@vger.kernel.org; Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
> 
> Hi, Biju,
> 
> On 27.08.2024 15:15, Biju Das wrote:
> > Hi Claudiu,
> >
> > Thanks for the feedback.
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Monday, August 26, 2024 4:25 PM
> >> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog
> >> domain in the restart handler
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> On RZ/G3S the watchdog can be part of a software-controlled PM
> >> domain. In this case, the watchdog device need to be powered on in
> >> struct watchdog_ops::restart API. This can be done though
> >> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog device are marked as IRQ
> safe.
> >> We mark the watchdog PM domain as IRQ safe with GENPD_FLAG_IRQ_SAFE
> >> when the watchdog PM domain is registered and the watchdog device though pm_runtime_irq_safe().
> >>
> >> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid
> >> wait
> >> context'") pm_runtime_get_sync() was used in watchdog restart handler
> >> (which is similar to
> >> pm_runtime_resume_and_get() except the later one handles the runtime resume errors).
> >>
> >> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> >> context'") dropped the pm_runtime_get_sync() and replaced it with
> >> clk_prepare_enable() to avoid invalid wait context due to
> >> genpd_lock() in genpd_runtime_resume() being called from atomic
> >> context. But
> >> clk_prepare_enable() doesn't fit for this either (as reported by Ulf
> >> Hansson) as clk_prepare() can also sleep (it just not throw invalid wait context warning as it is
> not written for this).
> >>
> >> Because the watchdog device is marked now as IRQ safe (though this
> >> patch) the
> >> irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume()
> >> returns
> >> 1 for devices not registering an IRQ safe PM domain for watchdog (as
> >> the watchdog device is IRQ safe, PM domain is not and watchdog PM
> >> domain is always-on), this being the case of RZ/G2 devices that uses
> >
> > RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is
> > not applicable for RZ/G2{H,M,N,E}devices.
> 
> OK, but I said "RZ/G2 devices that uses this driver". Here are included RZ/{G2L,G2LC,G2UL,V2L} AFAICT.

OK. Not sure you missed the same terminology on comment section, see below??

> 
> >
> >
> >> this driver, we can now drop also the clk_prepare_enable() calls in
> >> restart handler and rely on pm_runtime_resume_and_get().
> >>
> >> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in watchdog restart handler.
> >
> > Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt:
> > Fix 'BUG: Invalid wait
> >> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??
> 
> Not sure... I thought about it, too. I chose to have it like this thinking
> that:
> 
> 1/ that may not apply cleanly as it depends on other cleanups done on this
>    driver, e.g. commit d8997ed79ed7 ("watchdog: rzg2l_wdt: Rely on the
>    reset driver for doing proper reset") so it may be worthless for
>    backport machinery
> 2/ There is actually no seen bug reported by lockdep (as the clk_prepare()
>    doesn't handle it)
> 
> Don't know, I can reply here and add it. Applying this patch with b4 will take care of it. But not
> sure about it.

Maybe leave it.

> >
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index
> >> 2a35f890a288..e9e0408c96f7 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -12,6 +12,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/pm_domain.h>
> >>  #include <linux/pm_runtime.h>
> >>  #include <linux/reset.h>
> >>  #include <linux/units.h>
> >> @@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> >>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >>  	int ret;
> >>
> >> -	clk_prepare_enable(priv->pclk);
> >> -	clk_prepare_enable(priv->osc_clk);
> >> +	/*
> >> +	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
> >> +	 * domain that is currently powered off. In this case we need to power
> >> +	 * it on before accessing registers. Along with this the clocks will be
> >> +	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
> >> +	 * need to be on for the reboot to happen.
> >> +	 *
> >> +	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device trees

NitPick: For the rest of RZ/G2 devices that uses this driver (This will make sure
It is not meant for RZ/G2{H,M,N,E} devices)

Cheers,
Biju



> >> +	 * where PM domains are registered like on RZ/G2 devices) it is safe
> >> +	 * to call pm_runtime_resume_and_get() as the
> >> +	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
> >> +	 * returns non zero value and the genpd_lock() is avoided, thus, there
> >> +	 * will be no invalid wait context reported by lockdep.
> >> +	 */
> >> +	ret = pm_runtime_resume_and_get(wdev->parent);
> >> +	if (ret)
> >> +		return ret;
> >>
> >>  	if (priv->devtype == WDT_RZG2L) {
> >>  		ret = reset_control_deassert(priv->rstc);
> >> @@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct platform_device
> >> *pdev)
> >>
> >>  	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
> >>
> >> +	pm_runtime_irq_safe(&pdev->dev);
> >>  	pm_runtime_enable(&pdev->dev);
> >>
> >>  	priv->wdev.info = &rzg2l_wdt_ident;
> >> --
> >> 2.39.2
> >>
> >

Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[claudiu beznea]

On 27.08.2024 15:51, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, August 27, 2024 1:33 PM
>> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be; mturquette@baylibre.com;
>> sboyd@kernel.org; wim@linux-watchdog.org; linux@roeck-us.net; ulf.hansson@linaro.org
>> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> watchdog@vger.kernel.org; linux-pm@vger.kernel.org; Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> Subject: Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
>>
>> Hi, Biju,
>>
>> On 27.08.2024 15:15, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for the feedback.
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Monday, August 26, 2024 4:25 PM
>>>> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog
>>>> domain in the restart handler
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> On RZ/G3S the watchdog can be part of a software-controlled PM
>>>> domain. In this case, the watchdog device need to be powered on in
>>>> struct watchdog_ops::restart API. This can be done though
>>>> pm_runtime_resume_and_get() API if the watchdog PM domain and watchdog device are marked as IRQ
>> safe.
>>>> We mark the watchdog PM domain as IRQ safe with GENPD_FLAG_IRQ_SAFE
>>>> when the watchdog PM domain is registered and the watchdog device though pm_runtime_irq_safe().
>>>>
>>>> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid
>>>> wait
>>>> context'") pm_runtime_get_sync() was used in watchdog restart handler
>>>> (which is similar to
>>>> pm_runtime_resume_and_get() except the later one handles the runtime resume errors).
>>>>
>>>> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
>>>> context'") dropped the pm_runtime_get_sync() and replaced it with
>>>> clk_prepare_enable() to avoid invalid wait context due to
>>>> genpd_lock() in genpd_runtime_resume() being called from atomic
>>>> context. But
>>>> clk_prepare_enable() doesn't fit for this either (as reported by Ulf
>>>> Hansson) as clk_prepare() can also sleep (it just not throw invalid wait context warning as it is
>> not written for this).
>>>>
>>>> Because the watchdog device is marked now as IRQ safe (though this
>>>> patch) the
>>>> irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume()
>>>> returns
>>>> 1 for devices not registering an IRQ safe PM domain for watchdog (as
>>>> the watchdog device is IRQ safe, PM domain is not and watchdog PM
>>>> domain is always-on), this being the case of RZ/G2 devices that uses
>>>
>>> RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is
>>> not applicable for RZ/G2{H,M,N,E}devices.
>>
>> OK, but I said "RZ/G2 devices that uses this driver". Here are included RZ/{G2L,G2LC,G2UL,V2L} AFAICT.
> 
> OK. Not sure you missed the same terminology on comment section, see below??
> 
>>
>>>
>>>
>>>> this driver, we can now drop also the clk_prepare_enable() calls in
>>>> restart handler and rely on pm_runtime_resume_and_get().
>>>>
>>>> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in watchdog restart handler.
>>>
>>> Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt:
>>> Fix 'BUG: Invalid wait
>>>> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??
>>
>> Not sure... I thought about it, too. I chose to have it like this thinking
>> that:
>>
>> 1/ that may not apply cleanly as it depends on other cleanups done on this
>>    driver, e.g. commit d8997ed79ed7 ("watchdog: rzg2l_wdt: Rely on the
>>    reset driver for doing proper reset") so it may be worthless for
>>    backport machinery
>> 2/ There is actually no seen bug reported by lockdep (as the clk_prepare()
>>    doesn't handle it)
>>
>> Don't know, I can reply here and add it. Applying this patch with b4 will take care of it. But not
>> sure about it.
> 
> Maybe leave it.
> 
>>>
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
>>>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
>>>> b/drivers/watchdog/rzg2l_wdt.c index
>>>> 2a35f890a288..e9e0408c96f7 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -12,6 +12,7 @@
>>>>  #include <linux/module.h>
>>>>  #include <linux/of.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/pm_domain.h>
>>>>  #include <linux/pm_runtime.h>
>>>>  #include <linux/reset.h>
>>>>  #include <linux/units.h>
>>>> @@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>>>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>>>  	int ret;
>>>>
>>>> -	clk_prepare_enable(priv->pclk);
>>>> -	clk_prepare_enable(priv->osc_clk);
>>>> +	/*
>>>> +	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
>>>> +	 * domain that is currently powered off. In this case we need to power
>>>> +	 * it on before accessing registers. Along with this the clocks will be
>>>> +	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
>>>> +	 * need to be on for the reboot to happen.
>>>> +	 *
>>>> +	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device trees
> 
> NitPick: For the rest of RZ/G2 devices that uses this driver (This will make sure
> It is not meant for RZ/G2{H,M,N,E} devices)

If one considers this driver is used by RZ/G2{H,M,N,E} when reaching this
point then surely is in the wrong place.

RZ/Five is also uses this driver. Later, maybe devices compatible with this
driver will be added and this comment will not fit. Later, will we be
updating the comment for that? I'm not a fan of it.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
> 
> 
> 
>>>> +	 * where PM domains are registered like on RZ/G2 devices) it is safe
>>>> +	 * to call pm_runtime_resume_and_get() as the
>>>> +	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
>>>> +	 * returns non zero value and the genpd_lock() is avoided, thus, there
>>>> +	 * will be no invalid wait context reported by lockdep.
>>>> +	 */
>>>> +	ret = pm_runtime_resume_and_get(wdev->parent);
>>>> +	if (ret)
>>>> +		return ret;
>>>>
>>>>  	if (priv->devtype == WDT_RZG2L) {
>>>>  		ret = reset_control_deassert(priv->rstc);
>>>> @@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>  	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>>>>
>>>> +	pm_runtime_irq_safe(&pdev->dev);
>>>>  	pm_runtime_enable(&pdev->dev);
>>>>
>>>>  	priv->wdev.info = &rzg2l_wdt_ident;
>>>> --
>>>> 2.39.2
>>>>
>>>

RE: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, August 27, 2024 2:35 PM
> Subject: Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog domain in the restart handler
> 
> 
> 
> On 27.08.2024 15:51, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, August 27, 2024 1:33 PM
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; geert+renesas@glider.be;
> >> mturquette@baylibre.com; sboyd@kernel.org; wim@linux-watchdog.org;
> >> linux@roeck-us.net; ulf.hansson@linaro.org
> >> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux- watchdog@vger.kernel.org;
> >> linux-pm@vger.kernel.org; Claudiu Beznea
> >> <claudiu.beznea.uj@bp.renesas.com>
> >> Subject: Re: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog
> >> domain in the restart handler
> >>
> >> Hi, Biju,
> >>
> >> On 27.08.2024 15:15, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>> Thanks for the feedback.
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu <claudiu.beznea@tuxon.dev>
> >>>> Sent: Monday, August 26, 2024 4:25 PM
> >>>> Subject: [PATCH 3/3] watchdog: rzg2l_wdt: Power on the watchdog
> >>>> domain in the restart handler
> >>>>
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> On RZ/G3S the watchdog can be part of a software-controlled PM
> >>>> domain. In this case, the watchdog device need to be powered on in
> >>>> struct watchdog_ops::restart API. This can be done though
> >>>> pm_runtime_resume_and_get() API if the watchdog PM domain and
> >>>> watchdog device are marked as IRQ
> >> safe.
> >>>> We mark the watchdog PM domain as IRQ safe with GENPD_FLAG_IRQ_SAFE
> >>>> when the watchdog PM domain is registered and the watchdog device though pm_runtime_irq_safe().
> >>>>
> >>>> Before commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid
> >>>> wait
> >>>> context'") pm_runtime_get_sync() was used in watchdog restart
> >>>> handler (which is similar to
> >>>> pm_runtime_resume_and_get() except the later one handles the runtime resume errors).
> >>>>
> >>>> Commit e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait
> >>>> context'") dropped the pm_runtime_get_sync() and replaced it with
> >>>> clk_prepare_enable() to avoid invalid wait context due to
> >>>> genpd_lock() in genpd_runtime_resume() being called from atomic
> >>>> context. But
> >>>> clk_prepare_enable() doesn't fit for this either (as reported by
> >>>> Ulf
> >>>> Hansson) as clk_prepare() can also sleep (it just not throw invalid
> >>>> wait context warning as it is
> >> not written for this).
> >>>>
> >>>> Because the watchdog device is marked now as IRQ safe (though this
> >>>> patch) the
> >>>> irq_safe_dev_in_sleep_domain() call from genpd_runtime_resume()
> >>>> returns
> >>>> 1 for devices not registering an IRQ safe PM domain for watchdog
> >>>> (as the watchdog device is IRQ safe, PM domain is not and watchdog
> >>>> PM domain is always-on), this being the case of RZ/G2 devices that
> >>>> uses
> >>>
> >>> RZ/G2L alike devices or be specific RZ/{G2L,G2LC,G2UL,V2L} as it is
> >>> not applicable for RZ/G2{H,M,N,E}devices.
> >>
> >> OK, but I said "RZ/G2 devices that uses this driver". Here are included RZ/{G2L,G2LC,G2UL,V2L}
> AFAICT.
> >
> > OK. Not sure you missed the same terminology on comment section, see below??
> >
> >>
> >>>
> >>>
> >>>> this driver, we can now drop also the clk_prepare_enable() calls in
> >>>> restart handler and rely on pm_runtime_resume_and_get().
> >>>>
> >>>> Thus, drop clk_prepare_enable() and use pm_runtime_resume_and_get() in watchdog restart handler.
> >>>
> >>> Can this patch be fix for Commit e4cf89596c1f ("watchdog: rzg2l_wdt:
> >>> Fix 'BUG: Invalid wait
> >>>> context'") on RZ/{G2L,G2LC,G2UL,V2L} SoC??
> >>
> >> Not sure... I thought about it, too. I chose to have it like this
> >> thinking
> >> that:
> >>
> >> 1/ that may not apply cleanly as it depends on other cleanups done on this
> >>    driver, e.g. commit d8997ed79ed7 ("watchdog: rzg2l_wdt: Rely on the
> >>    reset driver for doing proper reset") so it may be worthless for
> >>    backport machinery
> >> 2/ There is actually no seen bug reported by lockdep (as the clk_prepare()
> >>    doesn't handle it)
> >>
> >> Don't know, I can reply here and add it. Applying this patch with b4
> >> will take care of it. But not sure about it.
> >
> > Maybe leave it.
> >
> >>>
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>> ---
> >>>>  drivers/watchdog/rzg2l_wdt.c | 21 +++++++++++++++++++--
> >>>>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >>>> b/drivers/watchdog/rzg2l_wdt.c index
> >>>> 2a35f890a288..e9e0408c96f7 100644
> >>>> --- a/drivers/watchdog/rzg2l_wdt.c
> >>>> +++ b/drivers/watchdog/rzg2l_wdt.c
> >>>> @@ -12,6 +12,7 @@
> >>>>  #include <linux/module.h>
> >>>>  #include <linux/of.h>
> >>>>  #include <linux/platform_device.h>
> >>>> +#include <linux/pm_domain.h>
> >>>>  #include <linux/pm_runtime.h>
> >>>>  #include <linux/reset.h>
> >>>>  #include <linux/units.h>
> >>>> @@ -166,8 +167,23 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> >>>>  	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >>>>  	int ret;
> >>>>
> >>>> -	clk_prepare_enable(priv->pclk);
> >>>> -	clk_prepare_enable(priv->osc_clk);
> >>>> +	/*
> >>>> +	 * In case of RZ/G3S the watchdog device may be part of an IRQ safe power
> >>>> +	 * domain that is currently powered off. In this case we need to power
> >>>> +	 * it on before accessing registers. Along with this the clocks will be
> >>>> +	 * enabled. We don't undo the pm_runtime_resume_and_get() as the device
> >>>> +	 * need to be on for the reboot to happen.
> >>>> +	 *
> >>>> +	 * For the rest of RZ/G2 devices (and for RZ/G3S with old device
> >>>> +trees
> >
> > NitPick: For the rest of RZ/G2 devices that uses this driver (This
> > will make sure It is not meant for RZ/G2{H,M,N,E} devices)
> 
> If one considers this driver is used by RZ/G2{H,M,N,E} when reaching this point then surely is in the
> wrong place.

So strictly speaking, then it is RZ/G3S and rest of the devices.
as RZ/G2 is not applicable here as we have RZ/V2L and RZ/Five apart
from RZ/{G2L,G2LC,G2UL}.

> 
> RZ/Five is also uses this driver. Later, maybe devices compatible with this driver will be added and
> this comment will not fit. Later, will we be updating the comment for that? I'm not a fan of it.

The comment RZ/G2 made confusion as the driver supports RZ/V2L and RZ/Five SoCs as well.
Also, it gives an impression about RZ/G2{H,M,N,E} devices.

Maybe replace "For the rest of RZ/G2 devices"->"For the rest of devices" should be sufficient.
as it covers RZ/{G2L,G2LC,G2UL}, RZ/V2L and RZ/Five SoCs.

Cheers.
Biju




> >
> >
> >
> >>>> +	 * where PM domains are registered like on RZ/G2 devices) it is safe
> >>>> +	 * to call pm_runtime_resume_and_get() as the
> >>>> +	 * irq_safe_dev_in_sleep_domain() call in genpd_runtime_resume()
> >>>> +	 * returns non zero value and the genpd_lock() is avoided, thus, there
> >>>> +	 * will be no invalid wait context reported by lockdep.
> >>>> +	 */
> >>>> +	ret = pm_runtime_resume_and_get(wdev->parent);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>>
> >>>>  	if (priv->devtype == WDT_RZG2L) {
> >>>>  		ret = reset_control_deassert(priv->rstc);
> >>>> @@ -275,6 +291,7 @@ static int rzg2l_wdt_probe(struct
> >>>> platform_device
> >>>> *pdev)
> >>>>
> >>>>  	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
> >>>>
> >>>> +	pm_runtime_irq_safe(&pdev->dev);
> >>>>  	pm_runtime_enable(&pdev->dev);
> >>>>
> >>>>  	priv->wdev.info = &rzg2l_wdt_ident;
> >>>> --
> >>>> 2.39.2
> >>>>
> >>>