Re: [PATCH] kvx: watchdog: Add early watchdog init

[Julian Vetter <jvetter@kalrayinc.com>]

Hello Ahmad,

On 10/23/24 11:25, Ahmad Fatoum wrote:
> Hello Julian,
> 
> On 23.10.24 10:55, Julian Vetter wrote:
>> From: Jules Maselbas <jmaselbas@kalray.eu>
>>
>> Add support for a watchdog that starts as early as possible in barebox.
>> So, if there is an issue the board is reset properly.
>>> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
>> ---
>>   drivers/watchdog/Kconfig   | 13 +++++++++++++
>>   drivers/watchdog/kvx_wdt.c | 16 ++++++++++++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 762e37c9c2..b1924b5b05 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -89,6 +89,19 @@ config WATCHDOG_KVX
>>   	help
>>   	  Add support for the KVX core watchdog.
>>   
>> +if WATCHDOG_KVX
> 
> `if' and `depends on' are equivalent, so just drop this line alongside
> the endif.

Yes, thanks. Fixed.

> 
>> +config WATCHDOG_KVX_EARLY_INIT
>> +	bool "KVX Core watchdog early init"
>> +	depends on WATCHDOG_KVX
>> +	help
>> +	  Activate the watchdog with early initcall
>> +
>> +config WATCHDOG_KVX_EARLY_TIMEOUT
>> +	int "KVX Core watchdog early timeout in cycles"
>> +	depends on WATCHDOG_KVX_EARLY_INIT
>> +	default 100000000
>> +endif
>> +
>>   config WATCHDOG_BCM2835
>>   	bool "Watchdog for BCM283x SoCs"
>>   	depends on ARCH_BCM283X || COMPILE_TEST
>> diff --git a/drivers/watchdog/kvx_wdt.c b/drivers/watchdog/kvx_wdt.c
>> index be6b08b71c..3e66aa38ba 100644
>> --- a/drivers/watchdog/kvx_wdt.c
>> +++ b/drivers/watchdog/kvx_wdt.c
>> @@ -91,3 +91,19 @@ static struct driver kvx_wdt_driver = {
>>   	.of_compatible	= DRV_OF_COMPAT(kvx_wdt_of_match),
>>   };
>>   device_platform_driver(kvx_wdt_driver);
>> +
>> +#ifdef CONFIG_WATCHDOG_KVX_EARLY_INIT
> 
> Move this down before the core_initcall
> 
>> +static int kvx_wdt_early_init(void)
> 
> Add __maybe_unused here.
> 
> That way compile test in CI tests this function even if option is disabled.
> 

I have tried what you proposed but the CONFIG_WATCHDOG_KVX_EARLY_TIMEOUT 
is only defined if we enable the CONFIG_WATCHDOG_KVX_EARLY_INIT. So, it 
fails if we have the watchdog enabled but not the EARLY_INIT. I could 
either keep the #ifdef before the function. So, it will not be covered 
by compile tests or I add something like the following to the function:

int wtd_timeout = 0;

#ifdef CONFIG_WATCHDOG_KVX_EARLY_INIT
wtd_timeout = CONFIG_WATCHDOG_KVX_EARLY_TIMEOUT
#endif

What do you think?

Thank you.

>> +{
>> +	/* Set Start watchdog counting */
>> +	kvx_sfr_set(WDV, CONFIG_WATCHDOG_KVX_EARLY_TIMEOUT);
>> +	kvx_sfr_set(WDR, 0);
>> +
>> +	/* Start watchdog counting */
>> +	kvx_sfr_set_field(TCR, WUI, 1);
>> +	kvx_sfr_set_field(TCR, WCE, 1);
>> +
>> +	return 0;
>> +}
>> +core_initcall(kvx_wdt_early_init);
>> +#endif
> 
>