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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
