Re: [PATCH 03/17] aspeed: Watchdog Timer Driver

[Simon Glass <sjg@chromium.org>]

Hi Maxim,

On 16 March 2017 at 15:36, Maxim Sloyko <maxims@google.com> wrote:
> This driver supports ast2500 and ast2400 SoCs.
> Only ast2500 supports reset_mask and thus the option of resettting
> individual peripherals using WDT.
>
> Signed-off-by: Maxim Sloyko <maxims@google.com>
> ---
>
>  arch/arm/include/asm/arch-aspeed/wdt.h |  53 ++++++++++++--
>  arch/arm/mach-aspeed/ast_wdt.c         |  40 ++++++++---

I feel that all of the code in this file should move into the driver file below.

>  drivers/watchdog/Kconfig               |  13 ++++
>  drivers/watchdog/Makefile              |   1 +
>  drivers/watchdog/ast_wdt.c             | 125 +++++++++++++++++++++++++++++++++
>  5 files changed, 219 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/watchdog/ast_wdt.c
>
> diff --git a/arch/arm/include/asm/arch-aspeed/wdt.h b/arch/arm/include/asm/arch-aspeed/wdt.h
> index b292a0e67b..981fa05a56 100644
> --- a/arch/arm/include/asm/arch-aspeed/wdt.h
> +++ b/arch/arm/include/asm/arch-aspeed/wdt.h
> @@ -67,15 +67,60 @@ struct ast_wdt {
>         u32 timeout_status;
>         u32 clr_timeout_status;
>         u32 reset_width;
> -#ifdef CONFIG_ASPEED_AST2500
> +       /* On pre-ast2500 SoCs this register is reserved. */
>         u32 reset_mask;
> -#else
> -       u32 reserved0;
> -#endif
>  };
>
> +/**
> + * Given flags parameter passed to wdt_reset or wdt_start uclass functions,
> + * gets Reset Mode value from it.
> + *
> + * @flags: flags parameter passed into wdt_reset or wdt_start
> + * @return Reset Mode value
> + */
> +u32 ast_reset_mode_from_flags(ulong flags);
> +
> +/**
> + * Given flags parameter passed to wdt_reset or wdt_start uclass functions,
> + * gets Reset Mask value from it. Reset Mask is only supported on ast2500
> + *
> + * @flags: flags parameter passed into wdt_reset or wdt_start
> + * @return Reset Mask value
> + */
> +u32 ast_reset_mask_from_flags(ulong flags);
> +
> +/**
> + * Given Reset Mask and Reset Mode values, converts them to flags,
> + * suitable for passing into wdt_start or wdt_reset uclass functions.
> + *
> + * On ast2500 Reset Mask is 25 bits wide and Reset Mode is 2 bits wide, so they
> + * can both be packed into single 32 bits wide value.
> + *
> + * @reset_mode: Reset Mode
> + * @reset_mask: Reset Mask
> + */
> +ulong ast_flags_from_reset_mode_mask(u32 reset_mode, u32 reset_mask);
> +
> +#ifndef CONFIG_WDT
> +/**
> + * Stop WDT
> + *
> + * @wdt: watchdog to stop
> + *
> + * When using driver model this function has different signature
> + */
>  void wdt_stop(struct ast_wdt *wdt);
> +
> +/**
> + * Stop WDT
> + *
> + * @wdt: watchdog to start
> + * @timeout    watchdog timeout in number of clock ticks
> + *
> + * When using driver model this function has different signature
> + */
>  void wdt_start(struct ast_wdt *wdt, u32 timeout);
> +#endif  /* CONFIG_WDT */
>
>  /**
>   * Reset peripherals specified by mask
> diff --git a/arch/arm/mach-aspeed/ast_wdt.c b/arch/arm/mach-aspeed/ast_wdt.c
> index 22481ab7ea..895fba3366 100644
> --- a/arch/arm/mach-aspeed/ast_wdt.c
> +++ b/arch/arm/mach-aspeed/ast_wdt.c
> @@ -9,6 +9,27 @@
>  #include <asm/arch/wdt.h>
>  #include <linux/err.h>
>
> +u32 ast_reset_mode_from_flags(ulong flags)
> +{
> +       return flags & WDT_CTRL_RESET_MASK;
> +}
> +
> +u32 ast_reset_mask_from_flags(ulong flags)
> +{
> +       return flags >> 2;
> +}

I'm not sure those two functions are worth it. Who calls them?

> +
> +ulong ast_flags_from_reset_mode_mask(u32 reset_mode, u32 reset_mask)

function comment?

> +{
> +       ulong ret = reset_mode & WDT_CTRL_RESET_MASK;
> +
> +       if (ret == WDT_CTRL_RESET_SOC)
> +               ret |= (reset_mask << 2);
> +
> +       return ret;
> +}
> +
> +#ifndef CONFIG_WDT
>  void wdt_stop(struct ast_wdt *wdt)
>  {
>         clrbits_le32(&wdt->ctrl, WDT_CTRL_EN);
> @@ -26,15 +47,7 @@ void wdt_start(struct ast_wdt *wdt, u32 timeout)
>         setbits_le32(&wdt->ctrl,
>                      WDT_CTRL_EN | WDT_CTRL_RESET | WDT_CTRL_CLK1MHZ);
>  }
> -
> -struct ast_wdt *ast_get_wdt(u8 wdt_number)
> -{
> -       if (wdt_number > CONFIG_WDT_NUM - 1)
> -               return ERR_PTR(-EINVAL);
> -
> -       return (struct ast_wdt *)(WDT_BASE +
> -                                 sizeof(struct ast_wdt) * wdt_number);
> -}
> +#endif  /* CONFIG_WDT */
>
>  int ast_wdt_reset_masked(struct ast_wdt *wdt, u32 mask)
>  {
> @@ -57,3 +70,12 @@ int ast_wdt_reset_masked(struct ast_wdt *wdt, u32 mask)
>         return -EINVAL;
>  #endif
>  }
> +
> +struct ast_wdt *ast_get_wdt(u8 wdt_number)

s/u8/uint/ or similar. It can only hurt code size to require the
compiler to mask arguments.

> +{
> +       if (wdt_number > CONFIG_WDT_NUM - 1)
> +               return ERR_PTR(-EINVAL);
> +
> +       return (struct ast_wdt *)(WDT_BASE +

Can this come from the device tree?

> +                                 sizeof(struct ast_wdt) * wdt_number);
> +}
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0d7366f3df..10f34f5efa 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -9,3 +9,16 @@ config WDT
>           start, restart, stop and reset (expire immediately).
>           What exactly happens when the timer expires is up to a particular
>           device/driver.
> +
> +config WDT_ASPEED
> +       bool "Aspeed ast2400/ast2500 watchdog timer support"
> +       depends on WDT
> +       default y if ARCH_ASPEED
> +       help
> +         Select this to enable watchdog timer for Aspeed ast2500/ast2400 devices.
> +         The watchdog timer is stopped when initialized. It performs reset, either
> +         full SoC reset or CPU or just some peripherals, based on the flags.
> +         It currently does not support Boot Flash Addressing Mode Detection or
> +         Second Boot.
> +
> +endmenu
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 1aabcb97ae..1d779a8446 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -16,3 +16,4 @@ 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
> +obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
> diff --git a/drivers/watchdog/ast_wdt.c b/drivers/watchdog/ast_wdt.c
> new file mode 100644
> index 0000000000..d53aada332
> --- /dev/null
> +++ b/drivers/watchdog/ast_wdt.c
> @@ -0,0 +1,125 @@
> +/*
> + * Copyright 2017 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <wdt.h>
> +#include <asm/io.h>
> +#include <asm/arch/wdt.h>
> +
> +#define WDT_AST2500    2500
> +#define WDT_AST2400    2400
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct ast_wdt_priv {
> +       struct ast_wdt *regs;
> +};
> +
> +static int ast_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> +       struct ast_wdt_priv *priv = dev_get_priv(dev);
> +       ulong driver_data = dev_get_driver_data(dev);
> +       u32 reset_mode = ast_reset_mode_from_flags(flags);
> +
> +       clrsetbits_le32(&priv->regs->ctrl,
> +                       WDT_CTRL_RESET_MASK << WDT_CTRL_RESET_MODE_SHIFT,
> +                       reset_mode << WDT_CTRL_RESET_MODE_SHIFT);
> +
> +       if (driver_data >= WDT_AST2500 && reset_mode == WDT_CTRL_RESET_SOC)
> +               writel(ast_reset_mask_from_flags(flags),
> +                      &priv->regs->reset_mask);
> +
> +       writel((u32) timeout, &priv->regs->counter_reload_val);
> +       writel(WDT_COUNTER_RESTART_VAL, &priv->regs->counter_restart);
> +       /*
> +        * Setting CLK1MHZ bit is just for compatibility with ast2400 part.
> +        * On ast2500 watchdog timer clock is fixed at 1MHz and the bit is
> +        * read-only
> +        */
> +       setbits_le32(&priv->regs->ctrl,
> +                    WDT_CTRL_EN | WDT_CTRL_RESET | WDT_CTRL_CLK1MHZ);
> +
> +       return 0;
> +}
> +
> +static int ast_wdt_stop(struct udevice *dev)
> +{
> +       struct ast_wdt_priv *priv = dev_get_priv(dev);
> +
> +       clrbits_le32(&priv->regs->ctrl, WDT_CTRL_EN);
> +
> +       return 0;
> +}
> +
> +static int ast_wdt_restart(struct udevice *dev)
> +{
> +       struct ast_wdt_priv *priv = dev_get_priv(dev);
> +
> +       writel(WDT_COUNTER_RESTART_VAL, &priv->regs->counter_restart);
> +
> +       return 0;
> +}
> +
> +static int ast_wdt_reset(struct udevice *dev, ulong flags)
> +{
> +       struct ast_wdt_priv *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       ret = ast_wdt_start(dev, 1, flags);
> +       if (ret)
> +               return ret;

This feels a bit like what the default uclass imp does?

> +
> +       while (readl(&priv->regs->ctrl) & WDT_CTRL_EN)
> +               ;

I am keen to avoid loops in drivers. Can this return -EINPROGRESS and
have the loop be in the uclass? This is how sysreset works.

> +
> +       return ast_wdt_stop(dev);
> +}
> +
> +static int ast_wdt_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct ast_wdt_priv *priv = dev_get_priv(dev);
> +
> +       priv->regs = dev_get_addr_ptr(dev);
> +       if (IS_ERR(priv->regs))

This actually returns NULL on error.

> +               return PTR_ERR(priv->regs);
> +
> +       return 0;
> +}
> +
> +static const struct wdt_ops ast_wdt_ops = {
> +       .start = ast_wdt_start,
> +       .restart = ast_wdt_restart,
> +       .stop = ast_wdt_stop,
> +       .reset = ast_wdt_reset,
> +};
> +
> +static const struct udevice_id ast_wdt_ids[] = {
> +       { .compatible = "aspeed,wdt", .data = WDT_AST2500 },
> +       { .compatible = "aspeed,ast2500-wdt", .data = WDT_AST2500 },
> +       { .compatible = "aspeed,ast2400-wdt", .data = WDT_AST2400 },
> +       {}
> +};
> +
> +static int ast_wdt_probe(struct udevice *dev)
> +{
> +       debug("%s() wdt%u\n", __func__, dev->seq);
> +       ast_wdt_stop(dev);

check error? Why would you call stop when probing?

> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(ast_wdt) = {
> +       .name = "ast_wdt",
> +       .id = UCLASS_WDT,
> +       .of_match = ast_wdt_ids,
> +       .probe = ast_wdt_probe,
> +       .priv_auto_alloc_size = sizeof(struct ast_wdt_priv),
> +       .ofdata_to_platdata = ast_wdt_ofdata_to_platdata,
> +       .ops = &ast_wdt_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +};
> --
> 2.12.0.367.g23dc2f6d3c-goog
>

Regards,
Simon