[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version

[Michal Simek]

On 05. 04. 19 15:00, Stefan Roese wrote:
> Hi Michal,
> 
> On 05.04.19 14:07, Michal Simek wrote:
> 
> <snip>
> 
>>> +++ b/drivers/watchdog/wdt-uclass.c
>>> @@ -10,6 +10,12 @@
>>>   #include <dm/device-internal.h>
>>>   #include <dm/lists.h>
>>>   +#ifndef WDT_DEFAULT_TIMEOUT
>>> +#define WDT_DEFAULT_TIMEOUT    60
>>> +#endif
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>   int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>   {
>>>       const struct wdt_ops *ops = device_get_ops(dev);
>>> @@ -63,6 +69,67 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
>>>       return ret;
>>>   }
>>>   +#if defined(CONFIG_WATCHDOG)
>>
>> This is not enough because you can enable it just for full u-boot but
>> not for SPL.
> 
> Why not? CONFIG_WATCHDOG can be enabled with or without
> CONFIG_SPL_WATCHDOG_SUPPORT being enabled.
> 
>> It means you should also handle CONFIG_SPL_WATCHDOG_SUPPORT
>> macros.
>>
>> Just try to enable CADENCE_WDT, WDT and Watchdog for zcu100 and build
> 
> I just tried this for xilinx_zynqmp_zcu100_revC_defconfig. Per default
> it has enabled:

ah right. I have patch in my queue to disable it by default.

Anyway this is what I am getting.

[u-boot](mainline)$ git log -n 3 --oneline
672eafbf08a0 watchdog: Implement generic watchdog_reset() version
7b5b40e4858b watchdog: Move watchdog_dev to data section (BSS may not be
cleared)
0e708abcbba1 Merge branch 'master' of git://git.denx.de/u-boot-usb
[u-boot](mainline)$ make xilinx_zynqmp_zcu100_revC_defconfig >/dev/null
&& make -j >/dev/null
lib/built-in.o: In function `inflateReset':
/mnt/disk/u-boot/lib/zlib/inflate.c:28: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:28:(.text.inflateReset+0x58):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `inflateEnd':
/mnt/disk/u-boot/lib/zlib/inflate.c:936: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:936:(.text.inflateEnd+0x2c):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `inflate':
/mnt/disk/u-boot/lib/zlib/inflate.c:546: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:546:(.text.inflate+0x7b4):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:724: undefined reference to
`watchdog_reset'
/mnt/disk/u-boot/lib/zlib/inflate.c:724:(.text.inflate+0xcc4):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
lib/built-in.o: In function `udelay':
/mnt/disk/u-boot/lib/time.c:167: undefined reference to `watchdog_reset'
/mnt/disk/u-boot/lib/time.c:167:(.text.udelay+0x1c): relocation
truncated to fit: R_AARCH64_CALL26 against undefined symbol `watchdog_reset'
drivers/built-in.o:/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:
more undefined references to `watchdog_reset' follow
drivers/built-in.o: In function `_debug_uart_putc':
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x4c):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printch+0x54):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x58):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
/mnt/disk/u-boot/drivers/serial/serial_zynq.c:230:(.text.printascii+0x60):
relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol
`watchdog_reset'
make[1]: *** [spl/u-boot-spl] Error 1
make: *** [spl/u-boot-spl] Error 2


snip

>>> +static void watchdog_start(void)
>>> +{
>>> +    u32 timeout = WDT_DEFAULT_TIMEOUT;
>>> +
>>> +    /*
>>> +     * Use only the first watchdog device in U-Boot to trigger the
>>> +     * watchdog reset
>>> +     */
>>> +    if (gd->watchdog_dev) {
>>
>> I hope that gd structure is cleared somewhere.
> 
> I had this also in mind with respect to BSS not being cleared very
> early. Its cleared in board_init_f_init_reserve(), so quite early
> before board_init_f() is called.


good.

>  
>>> +        debug("Only one watchdog device used!\n");
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Init watchdog: This will call the probe function of the
>>> +     * watchdog driver, enabling the use of the device
>>> +     */
>>> +    if (uclass_get_device(UCLASS_WDT, 0,
>>> +                  (struct udevice **)&gd->watchdog_dev)) {
>>> +        debug("Watchdog: Not found!\n");
>>> +        return;
>>> +    }
>>
>> I think you should c&p what I have done in zynq/zynqmp. It means check
>> watchdog0 alias first because this is supposed to be the first primary
>> watchdog.
>>
>> zynq/zynqmp/mb code
>>         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;
>>                 }
>>         }
> 
> In my (internal) first version, I had exactly this code included. I
> removed the first part, as it makes no real sense to call it at this
> stage (wdt_post_bind() for the first WDT found in the system). My
> understanding is, that wdt_post_bind() will get called for all WDT
> found in the system. My current logic here is, to only register the
> first WDT found to the global watchdog_dev in GD. To prevent that
> multiple WDT are used to service the U-Boot watchdog via WATCHDOG_RESET.
> 
> Or does the DM subsystem automatically bind the watchdog0 alias first
> in its binding sequence?
> 
> I do not have a system with multiple watchdogs here, so its not that
> easy to test this. I could simulate this though somehow...

IIRC current code will gives you the first found watchdog which doesn't
need to be that one you want to use here. On DT based platform it is the
first watchdog found from top and there is no way around.

On Zynq,ZynqMP and Microblaze you have have multiple watchdogs, PS or PL
based and you need to have functionality to select which was should be
used.

  
>>
>>
>>> +
>>> +    if (CONFIG_IS_ENABLED(OF_CONTROL)) {
>>> +        timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
>>> +                           WDT_DEFAULT_TIMEOUT);
>>> +    }
>>
>> You should remove them from drivers.
>>
>> drivers/watchdog/at91sam9_wdt.c:119:    priv->timeout =
>> dev_read_u32_default(dev, "timeout-sec",
>> drivers/watchdog/cdns_wdt.c:238:        priv->timeout =
>> dev_read_u32_default(dev, "timeout-sec",
> 
> Yes, its on my plan. I've also some additions for the AT91 driver on
> the list since a few days, which will most likely get applied before
> this patch. I'll send a cleanup patch to remove those now superfluous
> DT handling later.

I think it will be good to add timeout-sec to core and removing it from
drivers as one patch separated from this change.

Thanks,
Michal