Hi Maxim,
On 16 March 2017 at 15:36, Maxim Sloyko <max...@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 <max...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
