Re: [PATCH v2] watchdog: sunxi_wdt: Allow watchdog to remain enabled after probe

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

Hi,

please send new versions as separate e-mail thread.

Dne petek, 14. februar 2025 ob 12:22:54 Srednjeevropski standardni čas je Regis Dargent napisal(a):
> If the watchdog is already running during probe, let it run on, read its
> configured timeout, and set its status so that it is correctly handled by the
> kernel.
> 
> Signed-off-by: Regis Dargent <regis.dargent@gmail.com>
> ---
>  drivers/watchdog/sunxi_wdt.c | 48 +++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)

You should add changelog here.

> 
> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
> index b85354a99582..0094bcd063c5 100644
> --- a/drivers/watchdog/sunxi_wdt.c
> +++ b/drivers/watchdog/sunxi_wdt.c
> @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  		timeout++;
>  
>  	sunxi_wdt->wdt_dev.timeout = timeout;
> +	sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0;
>  
>  	reg = readl(wdt_base + regs->wdt_mode);
>  	reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift);
> @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev,
>  	return 0;
>  }
>  
> +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev)
> +{
> +	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> +	void __iomem *wdt_base = sunxi_wdt->wdt_base;
> +	const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs;
> +	int i;
> +	u32 reg;
> +
> +	reg = readl(wdt_base + regs->wdt_mode);
> +	reg >>= regs->wdt_timeout_shift;
> +	reg &= WDT_TIMEOUT_MASK;
> +
> +	/* Start at 0 which actually means 0.5s */
> +	for (i = 0; ((i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg)); i++)

Drop parenthesis in condition.

> +		;
> +	if (i == 0) {
> +		wdt_dev->timeout = 1;
> +		wdt_dev->max_hw_heartbeat_ms = 500;
> +	} else {
> +		wdt_dev->timeout = i;
> +		wdt_dev->max_hw_heartbeat_ms = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sunxi_wdt_stop(struct watchdog_device *wdt_dev)
>  {
>  	struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev);
> @@ -192,11 +219,22 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev)
>  	return 0;
>  }
>  
> +static bool sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt)
> +{
> +	u32 reg;
> +	void __iomem *wdt_base = wdt->wdt_base;
> +	const struct sunxi_wdt_reg *regs = wdt->wdt_regs;

Inverse christmas tree.

> +
> +	reg = readl(wdt_base + regs->wdt_mode);
> +	return !!(reg & WDT_MODE_EN);
> +}
> +
>  static const struct watchdog_info sunxi_wdt_info = {
>  	.identity	= DRV_NAME,
>  	.options	= WDIOF_SETTIMEOUT |
>  			  WDIOF_KEEPALIVEPING |
> -			  WDIOF_MAGICCLOSE,
> +			  WDIOF_MAGICCLOSE |
> +			  WDIOF_SETTIMEOUT,

Drop this change. WDIOF_SETTIMEOUT is already defined.

>  };
>  
>  static const struct watchdog_ops sunxi_wdt_ops = {
> @@ -275,8 +313,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
>  
>  	watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt);
>  
> -	sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> -
> +	if (sunxi_wdt_enabled(sunxi_wdt)) {

Also check for IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED). This will
allow user to select behaviour via Kconfig.

Best regards,
Jernej

> +		sunxi_wdt_read_timeout(&sunxi_wdt->wdt_dev);
> +		set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status);
> +	} else {
> +		sunxi_wdt_stop(&sunxi_wdt->wdt_dev);
> +	}
>  	watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev);
>  	err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev);
>  	if (unlikely(err))
>