[U-Boot] [PATCH 02/17] dm: Simple Watchdog uclass

Simon Glass sjg at chromium.org
Tue Mar 21 23:22:03 UTC 2017


Hi Maxim,

On 16 March 2017 at 15:36, Maxim Sloyko <maxims at google.com> wrote:
> This is a simple uclass for Watchdog Timers. It has four operations:
> start, restart, reset, stop. Drivers must implement start, restart and
> stop operations, while implementing reset is optional: It's default
> implementation expires watchdog timer in one clock tick.
>
> Signed-off-by: Maxim Sloyko <maxims at google.com>
> ---
>
>  drivers/watchdog/Kconfig      | 11 +++++
>  drivers/watchdog/Makefile     |  1 +
>  drivers/watchdog/wdt-uclass.c | 79 +++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h        |  1 +
>  include/wdt.h                 | 97 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 189 insertions(+)
>  create mode 100644 drivers/watchdog/wdt-uclass.c
>  create mode 100644 include/wdt.h
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e69de29bb2..0d7366f3df 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -0,0 +1,11 @@
> +menu "Watchdog Timer Support"
> +
> +config WDT
> +       bool "Enable driver model for watchdog timer drivers"
> +       depends on DM
> +       help
> +         Enable driver model for watchdog timer. At the moment the API
> +         is very simple and only supports four operations:
> +         start, restart, stop and reset (expire immediately).
> +         What exactly happens when the timer expires is up to a particular
> +         device/driver.

My main comment is that I think using 'reset' to mean 'reset the
board' instead of 'reset the wdt' is confusing w.r.t the rest of
U-Boot. Resetting the board could be handled by something like
expire_now(), perhaps?

Also please add a fake sandbox wdt driver and a test/dm test.

> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a007ae8234..1aabcb97ae 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_XILINX_TB_WATCHDOG) += xilinx_tb_wdt.o
>  obj-$(CONFIG_BFIN_WATCHDOG)  += bfin_wdt.o
>  obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o
>  obj-$(CONFIG_DESIGNWARE_WATCHDOG) += designware_wdt.o
> +obj-$(CONFIG_WDT) += wdt-uclass.o
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> new file mode 100644
> index 0000000000..98a8b529f9
> --- /dev/null
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -0,0 +1,79 @@
> +/*
> + * Copyright 2017 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <wdt.h>
> +#include <dm/lists.h>
> +#include <dm/device-internal.h>

nit: move this up one

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * Implement a simple watchdog uclass. Watchdog is basically a timer that
> + * is used to detect or recover from malfunction. During normal operation
> + * the watchdog would be regularly reset to prevent it from timing out.
> + * If, due to a hardware fault or program error, the computer fails to reset
> + * the watchdog, the timer will elapse and generate a timeout signal.
> + * The timeout signal is used to initiate corrective action or actions,
> + * which typically include placing the system in a safe, known state.

This comment might be more useful in the header file.

> + */
> +
> +int wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> +       const struct wdt_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->start)
> +               return -ENOSYS;
> +
> +       return ops->start(dev, timeout, flags);
> +}
> +
> +int wdt_stop(struct udevice *dev)
> +{
> +       const struct wdt_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->stop)
> +               return -ENOSYS;
> +
> +       return ops->stop(dev);
> +}
> +
> +int wdt_restart(struct udevice *dev)
> +{
> +       const struct wdt_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->restart)
> +               return -ENOSYS;
> +
> +       return ops->restart(dev);
> +}
> +
> +int wdt_reset(struct udevice *dev, ulong flags)
> +{
> +       const struct wdt_ops *ops;
> +
> +       debug("WDT Resettting: %lu\n", flags);
> +       ops = device_get_ops(dev);
> +       if (ops->reset) {
> +               return ops->reset(dev, flags);
> +       } else {
> +               if (!ops->start)
> +                       return -ENOSYS;
> +
> +               ops->start(dev, 1, flags);

Check error here. Is the intent to start the watchdog and force it to
reset the machine?

> +               while (1)
> +                       ;

Can you use hang() here?

> +       }
> +
> +       return 0;
> +}
> +
> +UCLASS_DRIVER(wdt) = {
> +       .id             = UCLASS_WDT,
> +       .name           = "wdt",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 8c92d0b030..b73a7fd436 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -83,6 +83,7 @@ enum uclass_id {
>         UCLASS_VIDEO,           /* Video or LCD device */
>         UCLASS_VIDEO_BRIDGE,    /* Video bridge, e.g. DisplayPort to LVDS */
>         UCLASS_VIDEO_CONSOLE,   /* Text console driver for video device */
> +       UCLASS_WDT,             /* Watchdot Timer driver */
>
>         UCLASS_COUNT,
>         UCLASS_INVALID = -1,
> diff --git a/include/wdt.h b/include/wdt.h
> new file mode 100644
> index 0000000000..1da5a962df
> --- /dev/null
> +++ b/include/wdt.h
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright 2017 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef _WDT_H_
> +#define _WDT_H_
> +
> +/*
> + * Start the timer
> + *
> + * @dev: WDT Device
> + * @timeout: Number of ticks before timer expires
> + * @flags: Driver specific flags. This might be used to specify
> + * which action needs to be executed when the timer expires
> + * @return: 0 if OK, -ve on error
> + */
> +int wdt_start(struct udevice *dev, u64 timeout, ulong flags);
> +
> +/*
> + * Stop the timer
> + *

What does stopping the timer mean? Does it pause so that the watchdog
is now disabled? If so, how to enable it again?

> + * @dev: WDT Device
> + * @return: 0 if OK, -ve on error
> + */
> +int wdt_stop(struct udevice *dev);
> +
> +/*
> + * Restart the timer, typically restoring the counter to
> + * the value configured by start()
> + *
> + * @dev: WDT Device
> + * @return: 0 if OK, -ve on error
> + */
> +int wdt_restart(struct udevice *dev);

At present in U-Boot this operation is called 'reset'. It might be
better to keep the same name.

> +
> +/*
> + * Expire the timer, thus executing its action immediately
> + *
> + * Will either use chosen wdt, based on reset-wdt
> + * chosen property, or the first one available.

What does this refer to? Also, the function title is a little vague.
Perhaps somewhere you should include that it may (for example) reset
the board?

> + *
> + * @flags: Driver specific flags
> + * @return 0 if OK -ve on error. If wdt action is system reset,
> + * this function may never return.

This doesn't seem to match the args.

> + */
> +int wdt_reset(struct udevice *dev, ulong flags);
> +
> +/*
> + * struct wdt_ops - Driver model wdt operations
> + *
> + * The uclass interface is implemented by all wdt devices which use
> + * driver model.
> + */
> +struct wdt_ops {
> +       /*
> +        * Start the timer
> +        *
> +        * @dev: WDT Device
> +        * @timeout: Number of ticks before the timer expires
> +        * @flags: Driver specific flags. This might be used to specify
> +        * which action needs to be executed when the timer expires
> +        * @return: 0 if OK, -ve on error
> +        */
> +       int (*start)(struct udevice *dev, u64 timeout, ulong flags);
> +       /*
> +        * Stop the timer
> +        *
> +        * @dev: WDT Device
> +        * @return: 0 if OK, -ve on error
> +        */
> +       int (*stop)(struct udevice *dev);
> +       /*
> +        * Restart the timer, typically restoring the counter to
> +        * the value configured by start()
> +        *
> +        * @dev: WDT Device
> +        * @return: 0 if OK, -ve on error
> +        */
> +       int (*restart)(struct udevice *dev);
> +       /*
> +        * Expire the timer, thus executing the action immediately (optional)
> +        *
> +        * If this function is not provided, default implementation

a default

> +        * will be used for wdt_reset(), which is set the counter to 1

s/set/sets/

> +        * and wait forever. This is good enough for system level

a system-level

> +        * reset, but not good enough for resetting peripherals.

Why not? I don't really understand this bit.

> +        *
> +        * @dev: WDT Device
> +        * @flags: Driver specific flags
> +        * @return 0 if OK -ve on error. May not return.
> +        */
> +       int (*reset)(struct udevice *dev, ulong flags);
> +};
> +
> +#endif  /* _WDT_H_ */
> --
> 2.12.0.367.g23dc2f6d3c-goog
>

Regards,
Simon


More information about the U-Boot mailing list