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

[Eugen.Hristev at microchip.com]

On 21.03.2019 14:00, Stefan Roese wrote:
> External E-Mail
> 
> 
> 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.

It should at least build :)

> 
> But the code looks cleaner without this #ifdef and if you agree, I
> will send v2 soon with this change.

I did not have time to review all the patch series yet. So more reviews 
will follow

> 
>>
>>> +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().

I do not mind if you keep it

Eugen

> 
> Thanks,
> Stefan