> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@xxxxxxxxx>
> Sent: Tuesday, August 27, 2024 1:33 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; geert+renesas@xxxxxxxxx; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxx; wim@xxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; ulf.hansson@xxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> watchdog@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> >> 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@xxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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
> >>
> >
