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