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

[PATCH RFC 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 series proposes a new API called
dev_pm_genpd_resume_restart_dev() that is intended to be called in
scenarios like this. In this RFC series the
dev_pm_genpd_resume_restart_dev() checks if the system is in
SYSTEM_RESTART context and call dev_pm_genpd_resume(). I also wanted to
mark the device as a restart device with a new member in struct dev_pm_info
(similar to struct dev_pm_info::syscore) and check it in the newly
introduced API but then I told myself maybe it would be better to keep it
simpler for the moment.

Please let me know how do you consider this.

Along with it, series addresses the usage of clk_prepare_enable() in
rzg2l_wdt_restart() reported by Ulf Hansson at [1] and use the
dev_pm_genpd_resume_restart_dev() in rzg2l_wdt driver.

Please note that series is built on top of [1].

A similar approach (using directly the dev_pm_genpd_resume() function in
rzg2l_wdt was proposed at [2]). This series was posted separatelly to
avoid blocking the initial support for the RZ/G3S SoC.

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/all/20240531065723.1085423-1-claudiu.beznea.uj@bp.renesas.com/
[2] https://lore.kernel.org/all/20240410134044.2138310-10-claudiu.beznea.uj@bp.renesas.com/

Claudiu Beznea (3):
  pmdomain: core: Add a helper to power on the restart devices
  watchdog: rzg2l_wdt: Keep the clocks prepared
  watchdog: rzg2l_wdt: Power on the PM domain in rzg2l_wdt_restart()

 drivers/pmdomain/core.c      | 18 +++++++++++++++
 drivers/watchdog/rzg2l_wdt.c | 43 +++++++++++++++++++++++++++++++-----
 include/linux/pm_domain.h    |  2 ++
 3 files changed, 58 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH RFC 1/3] pmdomain: core: Add a helper to power on the restart devices

[Claudiu]

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

Restart devices (e.g., watchdog) may be part of a software-controlled power
domain. In the case of watchdog devices, if they implement the
struct watchdog::restart() API, this is called though:

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

The watchdog_restart_notifier() is called with local interrupts disabled
and SMP disabled (machine_restart() calls local_irq_disable() and
smp_send_stop() before calling do_kernel_restart()).

If the restart device (e.g., watchdog) is part of a software-controlled
power domain and this domain is off at the moment of restart, we need to
power it on before configuring the watchdog device.

Add the dev_pm_genpd_resume_restart_dev() function to power on a restart
device in these scenarios.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pmdomain/core.c   | 18 ++++++++++++++++++
 include/linux/pm_domain.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 83d978743659..d05bd72f6cfe 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -1540,6 +1540,24 @@ void dev_pm_genpd_resume(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_resume);
 
+/**
+ * dev_pm_genpd_resume_restart_dev - Try to synchronously resume the genpd for
+ * a reset device
+ * @dev: The reset device that is attached to the genpd, which needs to be
+ * resumed.
+ *
+ * This routine should tipicaly be called for a restart device (e.g. watchdog)
+ * that needs to be resumed during system restart phase.
+ */
+void dev_pm_genpd_resume_restart_dev(struct device *dev)
+{
+	if (system_state != SYSTEM_RESTART)
+		return;
+
+	dev_pm_genpd_resume(dev);
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_resume_restart_dev);
+
 #else /* !CONFIG_PM_SLEEP */
 
 #define genpd_prepare		NULL
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 71e4f0fb8867..9f8ecfa0bf3c 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -347,9 +347,11 @@ static inline void dev_pm_genpd_synced_poweroff(struct device *dev)
 #ifdef CONFIG_PM_GENERIC_DOMAINS_SLEEP
 void dev_pm_genpd_suspend(struct device *dev);
 void dev_pm_genpd_resume(struct device *dev);
+void dev_pm_genpd_resume_restart_dev(struct device *dev);
 #else
 static inline void dev_pm_genpd_suspend(struct device *dev) {}
 static inline void dev_pm_genpd_resume(struct device *dev) {}
+static inline void dev_pm_genpd_resume_restart_dev(struct device *dev) {}
 #endif
 
 /* OF PM domain providers */
-- 
2.39.2

[PATCH RFC 2/3] watchdog: rzg2l_wdt: Keep the clocks prepared

[Claudiu]

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

The watchdog restart handler is called with interrupts disabled. In
rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
atomic context should not happen. The clock drivers for all the
micro-architectures where the RZ/G2L watchdog driver is used are not
implementing struct clk_ops::prepare(). Even so, to be sure we are
not hitted by this at some point, keep the watchdog clocks prepared
and only enable them in restart handler. It is guaranteed that
clk_enable() can be called in atomic context.

Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 2a35f890a288..6e3d7512f38c 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -166,8 +166,8 @@ 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);
+	clk_enable(priv->pclk);
+	clk_enable(priv->osc_clk);
 
 	if (priv->devtype == WDT_RZG2L) {
 		ret = reset_control_deassert(priv->rstc);
@@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
 	.restart = rzg2l_wdt_restart,
 };
 
+static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
+{
+	int ret;
+
+	ret = clk_prepare(priv->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare(priv->osc_clk);
+	if (ret)
+		clk_unprepare(priv->pclk);
+
+	return ret;
+}
+
 static void rzg2l_wdt_pm_disable(void *data)
 {
-	struct watchdog_device *wdev = data;
+	struct rzg2l_wdt_priv *priv = data;
 
-	pm_runtime_disable(wdev->parent);
+	pm_runtime_disable(priv->wdev.parent);
+	clk_unprepare(priv->osc_clk);
+	clk_unprepare(priv->pclk);
 }
 
 static int rzg2l_wdt_probe(struct platform_device *pdev)
@@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
+	ret = rzg2l_clks_prepare(priv);
+	if (ret)
+		return ret;
+
 	pm_runtime_enable(&pdev->dev);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
@@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	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);
+	ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
 	if (ret)
 		return ret;
 
-- 
2.39.2

[PATCH RFC 3/3] watchdog: rzg2l_wdt: Power on the PM domain in rzg2l_wdt_restart()

[Claudiu]

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

The rzg2l_wdt_restart() is called in atomic context. Calling
pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume
APIs is not an option as it may lead to issues as described in commit
e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'")
that removed the pm_runtime_get_sync() and enabled directly the clocks.

Starting with RZ/G3S the watchdog could be part of its own
software-controlled power domain. In case the watchdog is not used the
power domain is off and accessing watchdog registers leads to aborts.

To solve this, the patch powers on the power domain using
dev_pm_genpd_resume_restart_dev() API after enabling its clock. This is
not sleeping or taking any other locks as the watchdog power domain is not
registered with GENPD_FLAG_IRQ_SAFE flags.

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

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 6e3d7512f38c..bbdbbaa7b82b 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>
@@ -169,6 +170,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 	clk_enable(priv->pclk);
 	clk_enable(priv->osc_clk);
 
+	/*
+	 * The device may be part of a power domain that is currently
+	 * powered off. We need to power it on before accessing registers.
+	 * We don't undo the dev_pm_genpd_resume_restart_dev() as the device
+	 * need to be on for the reboot to happen. Also, as we are in atomic
+	 * context here, there is no need to increment PM runtime usage counter
+	 * (to make sure pm_runtime_active() doesn't return wrong code).
+	 */
+	if (!pm_runtime_active(wdev->parent))
+		dev_pm_genpd_resume_restart_dev(wdev->parent);
+
 	if (priv->devtype == WDT_RZG2L) {
 		ret = reset_control_deassert(priv->rstc);
 		if (ret)
-- 
2.39.2

Re: [PATCH RFC 2/3] watchdog: rzg2l_wdt: Keep the clocks prepared

[Lad, Prabhakar]

Hi Claudiu,

Thank you for the patch.

On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The watchdog restart handler is called with interrupts disabled. In
> rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
> clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
> atomic context should not happen. The clock drivers for all the
> micro-architectures where the RZ/G2L watchdog driver is used are not
> implementing struct clk_ops::prepare(). Even so, to be sure we are
> not hitted by this at some point, keep the watchdog clocks prepared
> and only enable them in restart handler. It is guaranteed that
> clk_enable() can be called in atomic context.
>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 2a35f890a288..6e3d7512f38c 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -166,8 +166,8 @@ 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);
> +       clk_enable(priv->pclk);
> +       clk_enable(priv->osc_clk);
>
I think we need to add a check before enabling the clocks:

if (!watchdog_active(wdev)) {
         clk_enable(priv->pclk);
         clk_enable(priv->osc_clk);
}

>         if (priv->devtype == WDT_RZG2L) {
>                 ret = reset_control_deassert(priv->rstc);
> @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
>         .restart = rzg2l_wdt_restart,
>  };
>
> +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
> +{
> +       int ret;
> +
> +       ret = clk_prepare(priv->pclk);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare(priv->osc_clk);
> +       if (ret)
> +               clk_unprepare(priv->pclk);
> +
> +       return ret;
> +}
> +
>  static void rzg2l_wdt_pm_disable(void *data)
>  {
> -       struct watchdog_device *wdev = data;
> +       struct rzg2l_wdt_priv *priv = data;
>
> -       pm_runtime_disable(wdev->parent);
> +       pm_runtime_disable(priv->wdev.parent);
> +       clk_unprepare(priv->osc_clk);
> +       clk_unprepare(priv->pclk);
>  }
>
All the above chunk can go away if we use devm_clk_get_prepared()
while requesting the clocks in the probe.

Cheers,
Prabhakar

>  static int rzg2l_wdt_probe(struct platform_device *pdev)
> @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>         priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>
> +       ret = rzg2l_clks_prepare(priv);
> +       if (ret)
> +               return ret;
> +
>         pm_runtime_enable(&pdev->dev);
>
>         priv->wdev.info = &rzg2l_wdt_ident;
> @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>
>         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);
> +       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
>         if (ret)
>                 return ret;
>
> --
> 2.39.2
>
>

Re: [PATCH RFC 2/3] watchdog: rzg2l_wdt: Keep the clocks prepared

[claudiu beznea]

Hi, Prabhakar,

On 20.06.2024 18:31, Lad, Prabhakar wrote:
> Hi Claudiu,
> 
> Thank you for the patch.
> 
> On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The watchdog restart handler is called with interrupts disabled. In
>> rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog
>> clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in
>> atomic context should not happen. The clock drivers for all the
>> micro-architectures where the RZ/G2L watchdog driver is used are not
>> implementing struct clk_ops::prepare(). Even so, to be sure we are
>> not hitted by this at some point, keep the watchdog clocks prepared
>> and only enable them in restart handler. It is guaranteed that
>> clk_enable() can be called in atomic context.
>>
>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++-----
>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index 2a35f890a288..6e3d7512f38c 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -166,8 +166,8 @@ 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);
>> +       clk_enable(priv->pclk);
>> +       clk_enable(priv->osc_clk);
>>
> I think we need to add a check before enabling the clocks:
> 
> if (!watchdog_active(wdev)) {

Agree, this should be better.

>          clk_enable(priv->pclk);
>          clk_enable(priv->osc_clk);
> }
> 
>>         if (priv->devtype == WDT_RZG2L) {
>>                 ret = reset_control_deassert(priv->rstc);
>> @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
>>         .restart = rzg2l_wdt_restart,
>>  };
>>
>> +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv)
>> +{
>> +       int ret;
>> +
>> +       ret = clk_prepare(priv->pclk);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = clk_prepare(priv->osc_clk);
>> +       if (ret)
>> +               clk_unprepare(priv->pclk);
>> +
>> +       return ret;
>> +}
>> +
>>  static void rzg2l_wdt_pm_disable(void *data)
>>  {
>> -       struct watchdog_device *wdev = data;
>> +       struct rzg2l_wdt_priv *priv = data;
>>
>> -       pm_runtime_disable(wdev->parent);
>> +       pm_runtime_disable(priv->wdev.parent);
>> +       clk_unprepare(priv->osc_clk);
>> +       clk_unprepare(priv->pclk);
>>  }
>>
> All the above chunk can go away if we use devm_clk_get_prepared()
> while requesting the clocks in the probe.

Indeed, I missed devm_clk_get_prepared().

Thank you for your review,
Claudiu Beznea

> 
> Cheers,
> Prabhakar
> 
>>  static int rzg2l_wdt_probe(struct platform_device *pdev)
>> @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>>
>>         priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>>
>> +       ret = rzg2l_clks_prepare(priv);
>> +       if (ret)
>> +               return ret;
>> +
>>         pm_runtime_enable(&pdev->dev);
>>
>>         priv->wdev.info = &rzg2l_wdt_ident;
>> @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>>
>>         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);
>> +       ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv);
>>         if (ret)
>>                 return ret;
>>
>> --
>> 2.39.2
>>
>>

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

[claudiu beznea]

Hi, Ulf,

Please, do you have any input/suggestions on this?

Thank you,
Claudiu Beznea

On 19.06.2024 15:09, Claudiu wrote:
> 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 series proposes a new API called
> dev_pm_genpd_resume_restart_dev() that is intended to be called in
> scenarios like this. In this RFC series the
> dev_pm_genpd_resume_restart_dev() checks if the system is in
> SYSTEM_RESTART context and call dev_pm_genpd_resume(). I also wanted to
> mark the device as a restart device with a new member in struct dev_pm_info
> (similar to struct dev_pm_info::syscore) and check it in the newly
> introduced API but then I told myself maybe it would be better to keep it
> simpler for the moment.
> 
> Please let me know how do you consider this.
> 
> Along with it, series addresses the usage of clk_prepare_enable() in
> rzg2l_wdt_restart() reported by Ulf Hansson at [1] and use the
> dev_pm_genpd_resume_restart_dev() in rzg2l_wdt driver.
> 
> Please note that series is built on top of [1].
> 
> A similar approach (using directly the dev_pm_genpd_resume() function in
> rzg2l_wdt was proposed at [2]). This series was posted separatelly to
> avoid blocking the initial support for the RZ/G3S SoC.
> 
> Thank you,
> Claudiu Beznea
> 
> [1] https://lore.kernel.org/all/20240531065723.1085423-1-claudiu.beznea.uj@bp.renesas.com/
> [2] https://lore.kernel.org/all/20240410134044.2138310-10-claudiu.beznea.uj@bp.renesas.com/
> 
> Claudiu Beznea (3):
>   pmdomain: core: Add a helper to power on the restart devices
>   watchdog: rzg2l_wdt: Keep the clocks prepared
>   watchdog: rzg2l_wdt: Power on the PM domain in rzg2l_wdt_restart()
> 
>  drivers/pmdomain/core.c      | 18 +++++++++++++++
>  drivers/watchdog/rzg2l_wdt.c | 43 +++++++++++++++++++++++++++++++-----
>  include/linux/pm_domain.h    |  2 ++
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 

Re: [PATCH RFC 3/3] watchdog: rzg2l_wdt: Power on the PM domain in rzg2l_wdt_restart()

[Ulf Hansson]

On Wed, 19 Jun 2024 at 14:09, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The rzg2l_wdt_restart() is called in atomic context. Calling
> pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume
> APIs is not an option as it may lead to issues as described in commit
> e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'")
> that removed the pm_runtime_get_sync() and enabled directly the clocks.
>
> Starting with RZ/G3S the watchdog could be part of its own
> software-controlled power domain. In case the watchdog is not used the
> power domain is off and accessing watchdog registers leads to aborts.
>
> To solve this, the patch powers on the power domain using
> dev_pm_genpd_resume_restart_dev() API after enabling its clock. This is
> not sleeping or taking any other locks as the watchdog power domain is not
> registered with GENPD_FLAG_IRQ_SAFE flags.

Would it be a problem to register the corresponding genpd using the
GENPD_FLAG_IRQ_SAFE?

Assuming it wouldn't, it looks like we should be able to make the
watchdog device irq-safe too, by calling pm_runtime_irq_safe() during
->probe().

In that case it should be okay to call pm_runtime_get_sync() in atomic
context, right?

Kind regards
Uffe

>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 6e3d7512f38c..bbdbbaa7b82b 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>
> @@ -169,6 +170,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>         clk_enable(priv->pclk);
>         clk_enable(priv->osc_clk);
>
> +       /*
> +        * The device may be part of a power domain that is currently
> +        * powered off. We need to power it on before accessing registers.
> +        * We don't undo the dev_pm_genpd_resume_restart_dev() as the device
> +        * need to be on for the reboot to happen. Also, as we are in atomic
> +        * context here, there is no need to increment PM runtime usage counter
> +        * (to make sure pm_runtime_active() doesn't return wrong code).
> +        */
> +       if (!pm_runtime_active(wdev->parent))
> +               dev_pm_genpd_resume_restart_dev(wdev->parent);
> +
>         if (priv->devtype == WDT_RZG2L) {
>                 ret = reset_control_deassert(priv->rstc);
>                 if (ret)
> --
> 2.39.2
>

Re: [PATCH RFC 3/3] watchdog: rzg2l_wdt: Power on the PM domain in rzg2l_wdt_restart()

[claudiu beznea]

Hi, Ulf,

Sorry for the late reply, I was off last week.

On 13.08.2024 16:56, Ulf Hansson wrote:
> On Wed, 19 Jun 2024 at 14:09, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The rzg2l_wdt_restart() is called in atomic context. Calling
>> pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume
>> APIs is not an option as it may lead to issues as described in commit
>> e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'")
>> that removed the pm_runtime_get_sync() and enabled directly the clocks.
>>
>> Starting with RZ/G3S the watchdog could be part of its own
>> software-controlled power domain. In case the watchdog is not used the
>> power domain is off and accessing watchdog registers leads to aborts.
>>
>> To solve this, the patch powers on the power domain using
>> dev_pm_genpd_resume_restart_dev() API after enabling its clock. This is
>> not sleeping or taking any other locks as the watchdog power domain is not
>> registered with GENPD_FLAG_IRQ_SAFE flags.
> 
> Would it be a problem to register the corresponding genpd using the
> GENPD_FLAG_IRQ_SAFE?

Should be no problem, AFIK.

> 
> Assuming it wouldn't, it looks like we should be able to make the
> watchdog device irq-safe too, by calling pm_runtime_irq_safe() during
> ->probe().
> 
> In that case it should be okay to call pm_runtime_get_sync() in atomic
> context, right?

Right! I registered the watchdog domain with GENPD_FLAG_IRQ_SAFE as well as
it's parent domain (the always-on domain) and all looks good. However I
need to run further testing to be sure nothing is broken.

Thank you,
Claudiu Beznea

> 
> Kind regards
> Uffe
> 
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index 6e3d7512f38c..bbdbbaa7b82b 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>
>> @@ -169,6 +170,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>>         clk_enable(priv->pclk);
>>         clk_enable(priv->osc_clk);
>>
>> +       /*
>> +        * The device may be part of a power domain that is currently
>> +        * powered off. We need to power it on before accessing registers.
>> +        * We don't undo the dev_pm_genpd_resume_restart_dev() as the device
>> +        * need to be on for the reboot to happen. Also, as we are in atomic
>> +        * context here, there is no need to increment PM runtime usage counter
>> +        * (to make sure pm_runtime_active() doesn't return wrong code).
>> +        */
>> +       if (!pm_runtime_active(wdev->parent))
>> +               dev_pm_genpd_resume_restart_dev(wdev->parent);
>> +
>>         if (priv->devtype == WDT_RZG2L) {
>>                 ret = reset_control_deassert(priv->rstc);
>>                 if (ret)
>> --
>> 2.39.2
>>