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.
>
>
>> 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@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
>> + * 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
>>
>
