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@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
>>
>