Hi Claudiu,
> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@xxxxxxxxx>
> 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@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)
>
> 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
> >>>>
> >>>
