Re: [PATCH] watchdog: da9052_wdt: respect TWDMIN

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 1/29/25 01:24, Marcus Folkesson wrote:
> From: Marcus Folkesson <marcus.folkesson@xxxxxxxxxxxxx>
>
> We have to wait at least the minimium time for the watchdog window
> (TWDMIN) before writings to the wdt register after the
> watchdog is activated.
> Otherwise the chip will assert TWD_ERROR and power down to reset mode.

This is missing a key explanation:

Why does setting @min_hw_heartbeat_ms in struct watchdog_device
not work for this driver ?

Guenter

> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> ---
> The current implementation does not respect the time window for all
> cases not it handle when the jiffie wrap around.
> E.g. setting a new timeout and immediately perform a 'ping' causes the
> chip to assert TWD_ERROR.
> ---
>   drivers/watchdog/da9052_wdt.c | 33 +++++++++++++++++++++++----------
>   1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/da9052_wdt.c b/drivers/watchdog/da9052_wdt.c
> index 77039f2f0be54273df1666fe40c413b6c89285a1..5ca957a6c2219d99cb3e653205e3f556f7cfb56f 100644
> --- a/drivers/watchdog/da9052_wdt.c
> +++ b/drivers/watchdog/da9052_wdt.c
> @@ -45,6 +45,21 @@ static const struct {
>   	{ 7, 131 },
>   };
>   
> +static void da9052_wdt_wait_for_twdmin(struct watchdog_device *wdt_dev)
> +{
> +	struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
> +	unsigned long msecs, twdmin, jnow = jiffies;
> +
> +	/*
> +	 * The host must wait at least TWDMIN ms between writings to the watchdog
> +	 * register or the DA9052 will assert TWD_ERROR and power down to RESET mode.
> +	 */
> +	twdmin  = driver_data->jpast + msecs_to_jiffies(DA9052_TWDMIN);
> +	if (time_before(jnow, twdmin)) {
> +		msecs = jiffies_to_msecs(twdmin - jnow);
> +		mdelay(msecs);
> +	}
> +}
>   
>   static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   				  unsigned int timeout)
> @@ -53,6 +68,7 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   	struct da9052 *da9052 = driver_data->da9052;
>   	int ret, i;
>   
> +	da9052_wdt_wait_for_twdmin(wdt_dev);
>   	/*
>   	 * Disable the Watchdog timer before setting
>   	 * new time out.
> @@ -89,8 +105,8 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev,
>   		}
>   
>   		wdt_dev->timeout = timeout;
> -		driver_data->jpast = jiffies;
>   	}
> +	driver_data->jpast = jiffies;
>   
>   	return 0;
>   }
> @@ -109,16 +125,9 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev)
>   {
>   	struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
>   	struct da9052 *da9052 = driver_data->da9052;
> -	unsigned long msec, jnow = jiffies;
>   	int ret;
>   
> -	/*
> -	 * We have a minimum time for watchdog window called TWDMIN. A write
> -	 * to the watchdog before this elapsed time should cause an error.
> -	 */
> -	msec = (jnow - driver_data->jpast) * 1000/HZ;
> -	if (msec < DA9052_TWDMIN)
> -		mdelay(msec);
> +	da9052_wdt_wait_for_twdmin(wdt_dev);
>   
>   	/* Reset the watchdog timer */
>   	ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG,
> @@ -130,8 +139,11 @@ static int da9052_wdt_ping(struct watchdog_device *wdt_dev)
>   	 * FIXME: Reset the watchdog core, in general PMIC
>   	 * is supposed to do this
>   	 */
> -	return da9052_reg_update(da9052, DA9052_CONTROL_D_REG,
> +	ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG,
>   				 DA9052_CONTROLD_WATCHDOG, 0 << 7);
> +
> +	driver_data->jpast = jiffies;
> +	return ret;
>   }
>   
>   static const struct watchdog_info da9052_wdt_info = {
> @@ -187,6 +199,7 @@ static int da9052_wdt_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	driver_data->jpast = jiffies;
>   	return devm_watchdog_register_device(dev, &driver_data->wdt);
>   }
>   
>
> ---
> base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> change-id: 20250129-da9052-661dfbb7de38
>
> Best regards,