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

[Stefan Roese]

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:

$ grep 'WATCHDOG\|WDT' .config
# CONFIG_SPL_WATCHDOG_SUPPORT is not set
# CONFIG_SYSRESET_WATCHDOG is not set
CONFIG_WATCHDOG=y
# CONFIG_WATCHDOG_RESET_DISABLE is not set
# CONFIG_BCM2835_WDT is not set
# CONFIG_ULP_WATCHDOG is not set
CONFIG_WDT=y
# CONFIG_WDT_ASPEED is not set
# CONFIG_WDT_ORION is not set
CONFIG_WDT_CDNS=y
# CONFIG_XILINX_TB_WATCHDOG is not set
# CONFIG_IMX_WATCHDOG is not set
# CONFIG_WDT_AT91 is not set

I can also enable SPL_WATCHDOG_SUPPORT and it compiles without issues:

$ grep 'WATCHDOG\|WDT' .config
CONFIG_SPL_WATCHDOG_SUPPORT=y
# CONFIG_SYSRESET_WATCHDOG is not set
CONFIG_WATCHDOG=y
# CONFIG_WATCHDOG_RESET_DISABLE is not set
# CONFIG_BCM2835_WDT is not set
# CONFIG_ULP_WATCHDOG is not set
CONFIG_WDT=y
# CONFIG_WDT_ASPEED is not set
# CONFIG_WDT_ORION is not set
CONFIG_WDT_CDNS=y
# CONFIG_XILINX_TB_WATCHDOG is not set
# CONFIG_IMX_WATCHDOG is not set
# CONFIG_WDT_AT91 is not set

What am I missing?

>> +/*
>> + * Called by macro WATCHDOG_RESET. This function be called *very* early,
>> + * so we need to make sure, that the watchdog driver is ready before using
>> + * it in this function.
>> + */
>> +void watchdog_reset(void)
>> +{
>> +	static ulong next_reset;
>> +	ulong now;
>> +
>> +	/* Exit if GD is not ready or watchdog is not initialized yet */
>> +	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>> +		return;
>> +
>> +	/* Do not reset the watchdog too often */
>> +	now = get_timer(0);
>> +	if (now > next_reset) {
>> +		next_reset = now + 1000;	/* reset every 1000ms */
>> +		wdt_reset(gd->watchdog_dev);
>> +	}
>> +}
>> +
>> +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.
  
>> +		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...
  
> 
> 
>> +
>> +	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.

Thanks,
Stefan