[U-Boot] [PATCH] watchdog: Implement generic watchdog_reset() version
Stefan Roese
sr at denx.de
Fri Apr 5 15:23:44 UTC 2019
On 05.04.19 17:18, Michal Simek wrote:
> 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
Yes. You're probably missing this patch here, which I listed as
dependencies in the commit text:
[2] watchdog: Handle SPL build with watchdog disabled
https://patchwork.ozlabs.org/patch/1074098/
Please test again with this patch applied.
>
> 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.
I see. I still need to find a good solution for this. Best would be a
call into this uclass driver once all driver binding calls are finished.
Not sure if this is easy to accomplish though (I need to look deeper
still).
>
>
>>>
>>>
>>>> +
>>>> + 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.
I can remove these driver specific implementations in v2 if you
prefer this. No problem.
Thanks,
Stefan
More information about the U-Boot
mailing list