[U-Boot] [PATCH 06/11] arm: at91: Enable watchdog support

[Stefan Roese]

On 21.03.19 11:23, Eugen.Hristev at microchip.com wrote:
> 
> 
> On 19.03.2019 17:56, Stefan Roese wrote:
>> External E-Mail
>>
>>
>> This patch enables and starts the watchdog on the AT91 platform if
>> configured. Currently the WD timeout is configured to 16 seconds,
>> which is the longest value for this timer.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Heiko Schocher <hs at denx.de>
>> Cc: Andreas Bießmann <andreas at biessmann.org>
>> Cc: Eugen Hristev <eugen.hristev at microchip.com>
>> ---
>>    arch/arm/mach-at91/clock.c | 41 ++++++++++++++++++++++++++++++++++++++
>>    1 file changed, 41 insertions(+)
>>
>> diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c
>> index 64cbc3d1ed..2d442d0092 100644
>> --- a/arch/arm/mach-at91/clock.c
>> +++ b/arch/arm/mach-at91/clock.c
>> @@ -5,6 +5,8 @@
>>     */
>>    
>>    #include <common.h>
>> +#include <dm.h>
>> +#include <wdt.h>
>>    #include <asm/io.h>
>>    #include <asm/arch/hardware.h>
>>    #include <asm/arch/at91_pmc.h>
>> @@ -118,3 +120,42 @@ void at91_pllicpr_init(u32 icpr)
>>    
>>    	writel(icpr, &pmc->pllicpr);
>>    }
>> +
>> +#if defined(CONFIG_WATCHDOG) && !defined(CONFIG_SPL_BUILD)
> 
> Hi Stefan,
> 
> Does this mean that for CONFIG_SPL_BUILD, this functions won't exist,
> thus the SPL cannot use the watchdog ?
> 
> For example, configuring CONFIG_SPL_WATCHDOG_SUPPORT=y
> makes SPL build fail :
> 
> drivers/built-in.o: In function `atmel_nand_pmecc_write_page':
> /home/eugen/u-boot-denx/drivers/mtd/nand/raw/atmel_nand.c:592: undefined
> reference to `watchdog_reset'
> drivers/built-in.o: In function `atmel_nand_pmecc_read_page':
> /home/eugen/u-boot-denx/drivers/mtd/nand/raw/atmel_nand.c:552: undefined
> reference to `watchdog_reset'
> drivers/built-in.o: In function `pmecc_err_location':
> /home/eugen/u-boot-denx/drivers/mtd/nand/raw/atmel_nand.c:416: undefined
> reference to `watchdog_reset'
> scripts/Makefile.spl:384: recipe for target 'spl/u-boot-spl' failed

Let me see, if I can change this so that this code is
available if selected in SPL as well. Even though arch_misc_init()
will not be called, so the watchdog will not be started.

But the code looks cleaner without this #ifdef and if you agree, I
will send v2 soon with this change.
  
> 
>> +static struct udevice *watchdog_dev;
>> +
>> +/* Called by macro WATCHDOG_RESET */
>> +void watchdog_reset(void)
>> +{
>> +	static ulong next_reset;
>> +	ulong now;
>> +
>> +	if (!watchdog_dev)
>> +		return;
>> +
>> +	now = get_timer(0);
>> +
>> +	/* Do not reset the watchdog too often */
>> +	if (now > next_reset) {
>> +		next_reset = now + 1000;	/* reset every 1000ms */
>> +		wdt_reset(watchdog_dev);
>> +	}
>> +}
>> +
>> +int arch_misc_init(void)
>> +{
>> +	/* Init watchdog */
>> +	if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) {
>> +		debug("Watchdog: Not found by seq!\n");
>> +		if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) {
>> +			puts("Watchdog: Not found!\n");
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	wdt_start(watchdog_dev, 16000, 0);	/* 16 seconds is max */
>> +	printf("Watchdog: Started\n");
>> +
> 
> Any reason why you use printf and puts and debug in the same function ?
> Would expect to see debug everywhere except some fatal error that should
> be printed all the time.

Good catch. This is copy-pasted from other very similar implementations.
I personally find this last status message "Watchdog: Started" quite
useful and would like to keep it. All others might be changed to debug().

Thanks,
Stefan