| Message ID | 20240828140602.1006438-4-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain | expand |
On Wed, 28 Aug 2024 at 16:06, Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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 for RZ/G3S with old device trees and > the rest of the SoCs that use 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. > > Acked-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > > Changes in v2: > - adjusted patch description and comment from code > - collected tags > > Changes since RFC: > - use pm_runtime_resume_and_get() and pm_runtime_irq_safe() > - drop clock prepare in probe > > drivers/watchdog/rzg2l_wdt.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index 2a35f890a288..11bbe48160ec 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,22 @@ 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 SoCs not registering a watchdog IRQ safe power > + * domain 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 +290,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 >
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 2a35f890a288..11bbe48160ec 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,22 @@ 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 SoCs not registering a watchdog IRQ safe power + * domain 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 +290,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;