[v3,3/7] watchdog: npcm: Enable clock if provided

On the Nuvoton WPCM450 SoC, with its upcoming clock driver, peripheral
clocks are individually gated and ungated. Therefore, the watchdog
driver must be able to ungate the watchdog clock.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v3:
- Add enable/disable calls to npcm_wdt_restart handler
- Not applied due to the above change:  Acked-by: Guenter Roeck <linux@roeck-us.net>

v2:
- https://lore.kernel.org/lkml/20220429172030.398011-4-j.neuschaefer@gmx.net/
- Add clk_disable_unprepare call, suggested by Guenter Roeck

v1:
- https://lore.kernel.org/lkml/20220422183012.444674-4-j.neuschaefer@gmx.net/
---
 drivers/watchdog/npcm_wdt.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

--
2.35.1

On 5/8/22 12:43, Jonathan Neuschäfer wrote:
> On the Nuvoton WPCM450 SoC, with its upcoming clock driver, peripheral
> clocks are individually gated and ungated. Therefore, the watchdog
> driver must be able to ungate the watchdog clock.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v3:
> - Add enable/disable calls to npcm_wdt_restart handler
> - Not applied due to the above change:  Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> v2:
> - https://lore.kernel.org/lkml/20220429172030.398011-4-j.neuschaefer@gmx.net/
> - Add clk_disable_unprepare call, suggested by Guenter Roeck
> 
> v1:
> - https://lore.kernel.org/lkml/20220422183012.444674-4-j.neuschaefer@gmx.net/
> ---
>   drivers/watchdog/npcm_wdt.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index 28a24caa2627c..a1240a906ef2a 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c
> @@ -3,6 +3,7 @@
>   // Copyright (c) 2018 IBM Corp.
> 
>   #include <linux/bitops.h>
> +#include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/interrupt.h>
>   #include <linux/kernel.h>
> @@ -43,6 +44,7 @@
>   struct npcm_wdt {
>   	struct watchdog_device  wdd;
>   	void __iomem		*reg;
> +	struct clk		*clk;
>   };
> 
>   static inline struct npcm_wdt *to_npcm_wdt(struct watchdog_device *wdd)
> @@ -66,6 +68,9 @@ static int npcm_wdt_start(struct watchdog_device *wdd)
>   	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
>   	u32 val;
> 
> +	if (wdt->clk)
> +		clk_prepare_enable(wdt->clk);
> +
>   	if (wdd->timeout < 2)
>   		val = 0x800;
>   	else if (wdd->timeout < 3)
> @@ -100,6 +105,9 @@ static int npcm_wdt_stop(struct watchdog_device *wdd)
> 
>   	writel(0, wdt->reg);
> 
> +	if (wdt->clk)
> +		clk_disable_unprepare(wdt->clk);
> +
>   	return 0;
>   }
> 
> @@ -147,9 +155,15 @@ static int npcm_wdt_restart(struct watchdog_device *wdd,
>   {
>   	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
> 
> +	if (wdt->clk)
> +		clk_prepare_enable(wdt->clk);
> +
>   	writel(NPCM_WTR | NPCM_WTRE | NPCM_WTE, wdt->reg);
>   	udelay(1000);
> 
> +	if (wdt->clk)
> +		clk_disable_unprepare(wdt->clk);
> +

I am trying to understand why you stop the clock here.
If the watchdog didn't reset the system by now, for whatever reason,
you explicitly don't want it to reset the system ? If so, please add
a comment describing the reason for stopping the clock here.

Thanks,
Guenter

>   	return 0;
>   }
> 
> @@ -191,6 +205,10 @@ static int npcm_wdt_probe(struct platform_device *pdev)
>   	if (IS_ERR(wdt->reg))
>   		return PTR_ERR(wdt->reg);
> 
> +	wdt->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +	if (IS_ERR(wdt->clk))
> +		return PTR_ERR(wdt->clk);
> +
>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
>   		return irq;
> --
> 2.35.1
>

On Mon, May 09, 2022 at 07:31:31PM -0700, Guenter Roeck wrote:
> On 5/8/22 12:43, Jonathan Neuschäfer wrote:
> > On the Nuvoton WPCM450 SoC, with its upcoming clock driver, peripheral
> > clocks are individually gated and ungated. Therefore, the watchdog
> > driver must be able to ungate the watchdog clock.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> > 
> > v3:
> > - Add enable/disable calls to npcm_wdt_restart handler
> > - Not applied due to the above change:  Acked-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > v2:
> > - https://lore.kernel.org/lkml/20220429172030.398011-4-j.neuschaefer@gmx.net/
> > - Add clk_disable_unprepare call, suggested by Guenter Roeck
> > 
> > v1:
> > - https://lore.kernel.org/lkml/20220422183012.444674-4-j.neuschaefer@gmx.net/
> > ---
[...]
> > @@ -147,9 +155,15 @@ static int npcm_wdt_restart(struct watchdog_device *wdd,
> >   {
> >   	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
> > 
> > +	if (wdt->clk)
> > +		clk_prepare_enable(wdt->clk);
> > +
> >   	writel(NPCM_WTR | NPCM_WTRE | NPCM_WTE, wdt->reg);
> >   	udelay(1000);
> > 
> > +	if (wdt->clk)
> > +		clk_disable_unprepare(wdt->clk);
> > +
> 
> I am trying to understand why you stop the clock here.
> If the watchdog didn't reset the system by now, for whatever reason,
> you explicitly don't want it to reset the system ? If so, please add
> a comment describing the reason for stopping the clock here.

It was for symmetry with starting the clock, and in the hope that
udelay(1000) will be enough to reach timer expiration. (In practice it
does appear to work, although 1ms is not much.)

Upon reconsideration, I agree it's better to leave the clock running for
reset (with a comment pointing out the asymmetry).


Best regards,
Jonathan