| Message ID | 20240619120920.2703605-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain | expand |
Hi Claudiu, Thank you for the patch. On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The watchdog restart handler is called with interrupts disabled. In > rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog > clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in > atomic context should not happen. The clock drivers for all the > micro-architectures where the RZ/G2L watchdog driver is used are not > implementing struct clk_ops::prepare(). Even so, to be sure we are > not hitted by this at some point, keep the watchdog clocks prepared > and only enable them in restart handler. It is guaranteed that > clk_enable() can be called in atomic context. > > Reported-by: Ulf Hansson <ulf.hansson@linaro.org> > Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index 2a35f890a288..6e3d7512f38c 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -166,8 +166,8 @@ 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); > + clk_enable(priv->pclk); > + clk_enable(priv->osc_clk); > I think we need to add a check before enabling the clocks: if (!watchdog_active(wdev)) { clk_enable(priv->pclk); clk_enable(priv->osc_clk); } > if (priv->devtype == WDT_RZG2L) { > ret = reset_control_deassert(priv->rstc); > @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = { > .restart = rzg2l_wdt_restart, > }; > > +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv) > +{ > + int ret; > + > + ret = clk_prepare(priv->pclk); > + if (ret) > + return ret; > + > + ret = clk_prepare(priv->osc_clk); > + if (ret) > + clk_unprepare(priv->pclk); > + > + return ret; > +} > + > static void rzg2l_wdt_pm_disable(void *data) > { > - struct watchdog_device *wdev = data; > + struct rzg2l_wdt_priv *priv = data; > > - pm_runtime_disable(wdev->parent); > + pm_runtime_disable(priv->wdev.parent); > + clk_unprepare(priv->osc_clk); > + clk_unprepare(priv->pclk); > } > All the above chunk can go away if we use devm_clk_get_prepared() while requesting the clocks in the probe. Cheers, Prabhakar > static int rzg2l_wdt_probe(struct platform_device *pdev) > @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > > priv->devtype = (uintptr_t)of_device_get_match_data(dev); > > + ret = rzg2l_clks_prepare(priv); > + if (ret) > + return ret; > + > pm_runtime_enable(&pdev->dev); > > priv->wdev.info = &rzg2l_wdt_ident; > @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > > watchdog_set_drvdata(&priv->wdev, priv); > dev_set_drvdata(dev, priv); > - ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev); > + ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv); > if (ret) > return ret; > > -- > 2.39.2 > >
Hi, Prabhakar, On 20.06.2024 18:31, Lad, Prabhakar wrote: > Hi Claudiu, > > Thank you for the patch. > > On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The watchdog restart handler is called with interrupts disabled. In >> rzg2l_wdt_restart() we call clk_prepare_enable() to enable the watchdog >> clocks. The prepare part of clk_prepare_enable() may sleep. Sleep in >> atomic context should not happen. The clock drivers for all the >> micro-architectures where the RZ/G2L watchdog driver is used are not >> implementing struct clk_ops::prepare(). Even so, to be sure we are >> not hitted by this at some point, keep the watchdog clocks prepared >> and only enable them in restart handler. It is guaranteed that >> clk_enable() can be called in atomic context. >> >> Reported-by: Ulf Hansson <ulf.hansson@linaro.org> >> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/watchdog/rzg2l_wdt.c | 31 ++++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c >> index 2a35f890a288..6e3d7512f38c 100644 >> --- a/drivers/watchdog/rzg2l_wdt.c >> +++ b/drivers/watchdog/rzg2l_wdt.c >> @@ -166,8 +166,8 @@ 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); >> + clk_enable(priv->pclk); >> + clk_enable(priv->osc_clk); >> > I think we need to add a check before enabling the clocks: > > if (!watchdog_active(wdev)) { Agree, this should be better. > clk_enable(priv->pclk); > clk_enable(priv->osc_clk); > } > >> if (priv->devtype == WDT_RZG2L) { >> ret = reset_control_deassert(priv->rstc); >> @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = { >> .restart = rzg2l_wdt_restart, >> }; >> >> +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv) >> +{ >> + int ret; >> + >> + ret = clk_prepare(priv->pclk); >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare(priv->osc_clk); >> + if (ret) >> + clk_unprepare(priv->pclk); >> + >> + return ret; >> +} >> + >> static void rzg2l_wdt_pm_disable(void *data) >> { >> - struct watchdog_device *wdev = data; >> + struct rzg2l_wdt_priv *priv = data; >> >> - pm_runtime_disable(wdev->parent); >> + pm_runtime_disable(priv->wdev.parent); >> + clk_unprepare(priv->osc_clk); >> + clk_unprepare(priv->pclk); >> } >> > All the above chunk can go away if we use devm_clk_get_prepared() > while requesting the clocks in the probe. Indeed, I missed devm_clk_get_prepared(). Thank you for your review, Claudiu Beznea > > Cheers, > Prabhakar > >> static int rzg2l_wdt_probe(struct platform_device *pdev) >> @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) >> >> priv->devtype = (uintptr_t)of_device_get_match_data(dev); >> >> + ret = rzg2l_clks_prepare(priv); >> + if (ret) >> + return ret; >> + >> pm_runtime_enable(&pdev->dev); >> >> priv->wdev.info = &rzg2l_wdt_ident; >> @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) >> >> watchdog_set_drvdata(&priv->wdev, priv); >> dev_set_drvdata(dev, priv); >> - ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev); >> + ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv); >> if (ret) >> return ret; >> >> -- >> 2.39.2 >> >>
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 2a35f890a288..6e3d7512f38c 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -166,8 +166,8 @@ 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); + clk_enable(priv->pclk); + clk_enable(priv->osc_clk); if (priv->devtype == WDT_RZG2L) { ret = reset_control_deassert(priv->rstc); @@ -226,11 +226,28 @@ static const struct watchdog_ops rzg2l_wdt_ops = { .restart = rzg2l_wdt_restart, }; +static int rzg2l_clks_prepare(struct rzg2l_wdt_priv *priv) +{ + int ret; + + ret = clk_prepare(priv->pclk); + if (ret) + return ret; + + ret = clk_prepare(priv->osc_clk); + if (ret) + clk_unprepare(priv->pclk); + + return ret; +} + static void rzg2l_wdt_pm_disable(void *data) { - struct watchdog_device *wdev = data; + struct rzg2l_wdt_priv *priv = data; - pm_runtime_disable(wdev->parent); + pm_runtime_disable(priv->wdev.parent); + clk_unprepare(priv->osc_clk); + clk_unprepare(priv->pclk); } static int rzg2l_wdt_probe(struct platform_device *pdev) @@ -275,6 +292,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) priv->devtype = (uintptr_t)of_device_get_match_data(dev); + ret = rzg2l_clks_prepare(priv); + if (ret) + return ret; + pm_runtime_enable(&pdev->dev); priv->wdev.info = &rzg2l_wdt_ident; @@ -287,7 +308,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(&priv->wdev, priv); dev_set_drvdata(dev, priv); - ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv->wdev); + ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, &priv); if (ret) return ret;