[RFC PATCH 2/8] watchdog: Integrate watchdog triggering into the cyclic framework

[Stefan Roese]

On 29.08.22 09:38, Rasmus Villemoes wrote:
> On 29/08/2022 08.23, Stefan Roese wrote:
>> This patch integrates the watchdog triggering into the recently added
>> cyclic infrastructure. Each watchdog device that shall be triggered
>> registers it's own cyclic function. This way, multiple watchdog devices
>> are still supported, each via a cyclic function with separate trigger
>> intervals.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> ---
>>   drivers/watchdog/Kconfig      |  2 +
>>   drivers/watchdog/wdt-uclass.c | 80 +++++++++++++++++++++--------------
>>   2 files changed, 50 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 50e6a1efba51..e55deaf906b5 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -3,6 +3,7 @@ menu "Watchdog Timer Support"
>>   config WATCHDOG
>>   	bool "Enable U-Boot watchdog reset"
>>   	depends on !HW_WATCHDOG
>> +	select CYCLIC
>>   	help
>>   	  This option enables U-Boot watchdog support where U-Boot is using
>>   	  watchdog_reset function to service watchdog device in U-Boot. Enable
>> @@ -74,6 +75,7 @@ config WDT
>>   	bool "Enable driver model for watchdog timer drivers"
>>   	depends on DM
>>   	imply WATCHDOG
>> +	select CYCLIC
>>   	help
>>   	  Enable driver model for watchdog timer. At the moment the API
>>   	  is very simple and only supports four operations:
>> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
>> index dbf556467d3c..1bf1298d0ecf 100644
>> --- a/drivers/watchdog/wdt-uclass.c
>> +++ b/drivers/watchdog/wdt-uclass.c
>> @@ -6,6 +6,7 @@
>>   #define LOG_CATEGORY UCLASS_WDT
>>   
>>   #include <common.h>
>> +#include <cyclic.h>
>>   #include <dm.h>
>>   #include <errno.h>
>>   #include <hang.h>
>> @@ -38,8 +39,33 @@ struct wdt_priv {
>>   	bool running;
>>   	/* No autostart */
>>   	bool noautostart;
>> +
>> +	struct cyclic_info *cyclic;
>>   };
>>   
>> +static void wdt_cyclic(void *ctx)
>> +{
>> +	struct udevice *dev = ctx;
>> +	struct wdt_priv *priv;
>> +	struct uclass *uc;
>> +
>> +	/* Exit if GD is not ready or watchdog is not initialized yet */
>> +	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>> +		return;
> 
> Hm, by the time this callback can get called, which is certainly not
> before it has been registered, aren't we sure that these conditions are
> met? They were clearly needed in the "old" watchdog_reset because that
> could end up being invoked more or less from _everywhere_. But here I
> think it's redundant.

Agreed. I was unsure while moving this code and then forgot about it.
I'll drop this these checks in the next version.

>>   int initr_watchdog(void)
>> @@ -105,8 +128,28 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>   	ret = ops->start(dev, timeout_ms, flags);
>>   	if (ret == 0) {
>>   		struct wdt_priv *priv = dev_get_uclass_priv(dev);
>> +		char str[16];
>>   
>>   		priv->running = true;
>> +
>> +		memset(str, 0, 16);
>> +		if (IS_ENABLED(CONFIG_WATCHDOG)) {
>> +			/* Register the watchdog driver as a cyclic function */
>> +			priv->cyclic = cyclic_register(wdt_cyclic,
>> +						       priv->reset_period * 1000,
>> +						       dev->name, dev);
>> +			if (!priv->cyclic) {
>> +				printf("cyclic_register for %s failed\n",
>> +				       dev->name);
>> +			} else {
>> +				snprintf(str, 16, "all %ldms",
>> +					 priv->reset_period);
>> +			}
>> +		}
>> +
>> +		printf("WDT:   Started %s with%s servicing %s (%ds timeout)\n",
> 
> "servicing all 50ms" doesn't sound right to me. "servicing every 50ms"
> perhaps?

Sound better, thanks.

> Also, even if !priv->cyclic we still seem to end here, which
> doesn't seem right.

Good catch. I'll fix this accordingly.

Thanks,
Stefan