[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+V-a8v7hxhhiT4X28kKJ5yTuMahCuCUWX_nFKd4cWL9GAWxug@mail.gmail.com>
Date: Thu, 20 Jun 2024 16:31:04 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: ulf.hansson@...aro.org, wim@...ux-watchdog.org, linux@...ck-us.net,
rafael@...nel.org, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-watchdog@...r.kernel.org, geert+renesas@...der.be,
linux-renesas-soc@...r.kernel.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH RFC 2/3] watchdog: rzg2l_wdt: Keep the clocks prepared
Hi Claudiu,
Thank you for the patch.
On Thu, Jun 20, 2024 at 9:29 AM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...aro.org>
> Closes: https://lore.kernel.org/all/CAPDyKFq1+cL1M9qGY0P58ETHUZHGymxQL0w92emUJPMe7a_GxA@mail.gmail.com
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...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
>
>
Powered by blists - more mailing lists