Simon,
I'll address your comments in followup cleanup, where I also created -uclass for WDT. The main reason is that many pieces of the code you commenting on will be gone. That series is pretty much ready and I'll send it as soon as this one gets in and I rebase it to new master.
Thank you!
On Thu, Jan 26, 2017 at 6:23 AM, Simon Glass sjg@chromium.org wrote:
On 18 January 2017 at 14:44, Maxim Sloyko maxims@google.com wrote:
Add support for Watchdog Timer, which is compatible with AST2400 and AST2500 watchdogs. There is no uclass for Watchdog yet, so the driver does not follow the driver model. It also uses fixed clock, so no clock driver is needed.
Add support for timer for Aspeed ast2400/ast2500 devices. The driver actually controls several devices, but because all devices share the same Control Register, it is somewhat difficult to completely decouple them. Since only one timer is needed at the moment, this should be OK. The timer uses fixed clock, so does not rely on a clock driver.
Add sysreset driver, which uses watchdog timer to do resets and particular watchdog device to use is hardcoded (0)
Changes in v4:
- Expanded AST2500 description in Kconfig
- Expanded ast_timer description in Kconfig
- Added struct ast_timer_counter to timer private data
- Use dev_get_addr_ptr in timer's of_platdata
- Added helper function to wdt for resetting peripherals
Changes in v3:
- Added SYS_TEXT_BASE as Kconfig option
Changes in v2:
- Moved number of WDTs to a Kconfig option
Changes in v1:
- Merged together the patches related to aspeed common drivers and configuration
- Fixed timer driver name (was sandbox_timer)
- Removed yet nonexistent files from mach-aspeed/Makefile
Signed-off-by: Maxim Sloyko maxims@google.com
arch/arm/Kconfig | 7 +++ arch/arm/Makefile | 1 + arch/arm/include/asm/arch-aspeed/timer.h | 54 +++++++++++++++++ arch/arm/include/asm/arch-aspeed/wdt.h | 99 ++++++++++++++++++++++++++++++++ arch/arm/mach-aspeed/Kconfig | 27 +++++++++ arch/arm/mach-aspeed/Makefile | 7 +++ arch/arm/mach-aspeed/ast_wdt.c | 59 +++++++++++++++++++ drivers/sysreset/Makefile | 1 + drivers/sysreset/sysreset_ast.c | 55 ++++++++++++++++++ drivers/timer/Kconfig | 12 ++++ drivers/timer/Makefile | 1 + drivers/timer/ast_timer.c | 97 +++++++++++++++++++++++++++++++ 12 files changed, 420 insertions(+) create mode 100644 arch/arm/include/asm/arch-aspeed/timer.h create mode 100644 arch/arm/include/asm/arch-aspeed/wdt.h create mode 100644 arch/arm/mach-aspeed/Kconfig create mode 100644 arch/arm/mach-aspeed/Makefile create mode 100644 arch/arm/mach-aspeed/ast_wdt.c create mode 100644 drivers/sysreset/sysreset_ast.c create mode 100644 drivers/timer/ast_timer.c
Reviewed-by: Simon Glass sjg@chromium.org
Some comments below which you could do as part of your clean-up if you like.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 714dd8b514..135c544335 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -882,8 +882,15 @@ config TARGET_THUNDERX_88XX select OF_CONTROL select SYS_CACHE_SHIFT_7
+config ARCH_ASPEED
bool "Support Aspeed SoCs"select OF_CONTROLselect DMendchoice
+source "arch/arm/mach-aspeed/Kconfig"
source "arch/arm/mach-at91/Kconfig"
source "arch/arm/mach-bcm283x/Kconfig" diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 236debb452..cc73e1038e 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -50,6 +50,7 @@ PLATFORM_CPPFLAGS += $(arch-y) $(tune-y)
# Machine directory name. This list is sorted alphanumerically # by CONFIG_* macro name. +machine-$(CONFIG_ARCH_ASPEED) += aspeed machine-$(CONFIG_ARCH_AT91) += at91 machine-$(CONFIG_ARCH_BCM283X) += bcm283x machine-$(CONFIG_ARCH_DAVINCI) += davinci diff --git a/arch/arm/include/asm/arch-aspeed/timer.h b/arch/arm/include/asm/arch-aspeed/timer.h new file mode 100644 index 0000000000..87c5b354ec --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/timer.h @@ -0,0 +1,54 @@ +/*
- Copyright (c) 2016 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _ASM_ARCH_TIMER_H +#define _ASM_ARCH_TIMER_H
+/* Each timer has 4 control bits in ctrl1 register.
- Timer1 uses bits 0:3, Timer2 uses bits 4:7 and so on,
- such that timer X uses bits (4 * X - 4):(4 * X - 1)
- If the timer does not support PWM, bit 4 is reserved.
- */
+#define AST_TMC_EN (1 << 0) +#define AST_TMC_1MHZ (1 << 1) +#define AST_TMC_OVFINTR (1 << 2) +#define AST_TMC_PWM (1 << 3)
+/* Timers are counted from 1 in the datasheet. */ +#define AST_TMC_CTRL1_SHIFT(n) (4 * ((n) - 1))
+#define AST_TMC_RATE (1000*1000)
+#ifndef __ASSEMBLY__
+/*
- All timers share control registers, which makes it harder to make them
- separate devices. Since only one timer is needed at the moment, making
- it this just one device.
- */
+struct ast_timer_counter {
u32 status;u32 reload_val;u32 match1;u32 match2;+};
+struct ast_timer {
struct ast_timer_counter timers1[3];u32 ctrl1;u32 ctrl2;+#ifdef CONFIG_ASPEED_AST2500
u32 ctrl3;u32 ctrl1_clr;+#else
u32 reserved[2];+#endif
We don't want have to #ifdefs in drivers. The driver should support both devices and use the compatible string to determine which is used. So here I think you can get rid of 'reserved'. Perhaps add a comment that these two registers don't exist on one device?
struct ast_timer_counter timers2[5];+};
+#endif /* __ASSEMBLY__ */
+#endif /* _ASM_ARCH_TIMER_H */ diff --git a/arch/arm/include/asm/arch-aspeed/wdt.h b/arch/arm/include/asm/arch-aspeed/wdt.h new file mode 100644 index 0000000000..b292a0e67b --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/wdt.h @@ -0,0 +1,99 @@ +/*
- (C) Copyright 2016 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _ASM_ARCH_WDT_H +#define _ASM_ARCH_WDT_H
+#define WDT_BASE 0x1e785000
TODO(email): Use device tree to get this value
+/*
- Special value that needs to be written to counter_restart register to
- (re)start the timer
- */
+#define WDT_COUNTER_RESTART_VAL 0x4755
+/* Control register */ +#define WDT_CTRL_RESET_MODE_SHIFT 5 +#define WDT_CTRL_RESET_MODE_MASK 3
+#define WDT_CTRL_EN (1 << 0) +#define WDT_CTRL_RESET (1 << 1) +#define WDT_CTRL_CLK1MHZ (1 << 4) +#define WDT_CTRL_2ND_BOOT (1 << 7)
+/* Values for Reset Mode */ +#define WDT_CTRL_RESET_SOC 0 +#define WDT_CTRL_RESET_CHIP 1 +#define WDT_CTRL_RESET_CPU 2 +#define WDT_CTRL_RESET_MASK 3
+/* Reset Mask register */ +#define WDT_RESET_ARM (1 << 0) +#define WDT_RESET_COPROC (1 << 1) +#define WDT_RESET_SDRAM (1 << 2) +#define WDT_RESET_AHB (1 << 3) +#define WDT_RESET_I2C (1 << 4) +#define WDT_RESET_MAC1 (1 << 5) +#define WDT_RESET_MAC2 (1 << 6) +#define WDT_RESET_GCRT (1 << 7) +#define WDT_RESET_USB20 (1 << 8) +#define WDT_RESET_USB11_HOST (1 << 9) +#define WDT_RESET_USB11_EHCI2 (1 << 10) +#define WDT_RESET_VIDEO (1 << 11) +#define WDT_RESET_HAC (1 << 12) +#define WDT_RESET_LPC (1 << 13) +#define WDT_RESET_SDSDIO (1 << 14) +#define WDT_RESET_MIC (1 << 15) +#define WDT_RESET_CRT2C (1 << 16) +#define WDT_RESET_PWM (1 << 17) +#define WDT_RESET_PECI (1 << 18) +#define WDT_RESET_JTAG (1 << 19) +#define WDT_RESET_ADC (1 << 20) +#define WDT_RESET_GPIO (1 << 21) +#define WDT_RESET_MCTP (1 << 22) +#define WDT_RESET_XDMA (1 << 23) +#define WDT_RESET_SPI (1 << 24) +#define WDT_RESET_MISC (1 << 25)
+#ifndef __ASSEMBLY__ +struct ast_wdt {
u32 counter_status;u32 counter_reload_val;u32 counter_restart;u32 ctrl;u32 timeout_status;u32 clr_timeout_status;u32 reset_width;+#ifdef CONFIG_ASPEED_AST2500
u32 reset_mask;+#else
u32 reserved0;+#endif
Same here
+};
+void wdt_stop(struct ast_wdt *wdt); +void wdt_start(struct ast_wdt *wdt, u32 timeout);
+/**
- Reset peripherals specified by mask
- Note, that this is only supported by ast2500 SoC
- @wdt: watchdog to use for this reset
- @mask: reset mask.
@return?
- */
+int ast_wdt_reset_masked(struct ast_wdt *wdt, u32 mask);
+/**
- ast_get_wdt() - get a pointer to watchdog registers
- @wdt_number: 0-based WDT peripheral number
- @return pointer to registers or -ve error on error
- */
+struct ast_wdt *ast_get_wdt(u8 wdt_number); +#endif /* __ASSEMBLY__ */
+#endif /* _ASM_ARCH_WDT_H */ diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig new file mode 100644 index 0000000000..b72ed89af7 --- /dev/null +++ b/arch/arm/mach-aspeed/Kconfig @@ -0,0 +1,27 @@ +if ARCH_ASPEED
+config SYS_ARCH
default "arm"+config SYS_SOC
default "aspeed"+config SYS_TEXT_BASE
default 0x00000000+config ASPEED_AST2500
bool "Support Aspeed AST2500 SoC"select CPU_ARM1176helpThe Aspeed AST2500 is a ARM-based SoC with arm1176 CPU.It is used as Board Management Controller on many server boards,which is enabled by support of LPC and eSPI peripherals.+config WDT_NUM
int "Number of Watchdog Timers"default 3 if ASPEED_AST2500helpThe number of Watchdot Timers on a SoC.AST2500 has three WDTsk earlier versions have two or fewer.+endif diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile new file mode 100644 index 0000000000..a14b8f751d --- /dev/null +++ b/arch/arm/mach-aspeed/Makefile @@ -0,0 +1,7 @@ +# +# Copyright (c) 2016 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +#
+obj-$(CONFIG_ARCH_ASPEED) += ast_wdt.o diff --git a/arch/arm/mach-aspeed/ast_wdt.c b/arch/arm/mach-aspeed/ast_wdt.c new file mode 100644 index 0000000000..22481ab7ea --- /dev/null +++ b/arch/arm/mach-aspeed/ast_wdt.c @@ -0,0 +1,59 @@ +/*
- (C) Copyright 2016 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <asm/io.h> +#include <asm/arch/wdt.h> +#include <linux/err.h>
+void wdt_stop(struct ast_wdt *wdt) +{
clrbits_le32(&wdt->ctrl, WDT_CTRL_EN);+}
+void wdt_start(struct ast_wdt *wdt, u32 timeout) +{
writel(timeout, &wdt->counter_reload_val);writel(WDT_COUNTER_RESTART_VAL, &wdt->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(&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);+}
+int ast_wdt_reset_masked(struct ast_wdt *wdt, u32 mask) +{ +#ifdef CONFIG_ASPEED_AST2500
if (!mask)return -EINVAL;writel(mask, &wdt->reset_mask);clrbits_le32(&wdt->ctrl,WDT_CTRL_RESET_MASK << WDT_CTRL_RESET_MODE_SHIFT);wdt_start(wdt, 1);/* Wait for WDT to reset */while (readl(&wdt->ctrl) & WDT_CTRL_EN);wdt_stop(wdt);return 0;+#else
return -EINVAL;+#endif +} diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile index fa75cc52de..37638a8eea 100644 --- a/drivers/sysreset/Makefile +++ b/drivers/sysreset/Makefile @@ -14,3 +14,4 @@ obj-$(CONFIG_ROCKCHIP_RK3399) += sysreset_rk3399.o obj-$(CONFIG_SANDBOX) += sysreset_sandbox.o obj-$(CONFIG_ARCH_SNAPDRAGON) += sysreset_snapdragon.o obj-$(CONFIG_TARGET_XTFPGA) += sysreset_xtfpga.o +obj-$(CONFIG_ARCH_ASPEED) += sysreset_ast.o diff --git a/drivers/sysreset/sysreset_ast.c b/drivers/sysreset/sysreset_ast.c new file mode 100644 index 0000000000..a0ab12851d --- /dev/null +++ b/drivers/sysreset/sysreset_ast.c @@ -0,0 +1,55 @@ +/*
- (C) Copyright 2016 Google, Inc
- SPDX-License-Identifier: GPL-2.0
- */
+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <sysreset.h> +#include <asm/io.h> +#include <asm/arch/wdt.h> +#include <linux/err.h>
+/* Number of Watchdog Timer ticks before reset */ +#define AST_WDT_RESET_TIMEOUT 10 +#define AST_WDT_FOR_RESET 0
+static int ast_sysreset_request(struct udevice *dev, enum sysreset_t type) +{
struct ast_wdt *wdt = ast_get_wdt(AST_WDT_FOR_RESET);u32 reset_mode = 0;if (IS_ERR(wdt))return PTR_ERR(wdt);switch (type) {case SYSRESET_WARM:reset_mode = WDT_CTRL_RESET_CPU;break;case SYSRESET_COLD:reset_mode = WDT_CTRL_RESET_CHIP;break;default:return -EPROTONOSUPPORT;}/* Clear reset mode bits */clrsetbits_le32(&wdt->ctrl,(WDT_CTRL_RESET_MODE_MASK << WDT_CTRL_RESET_MODE_SHIFT),(reset_mode << WDT_CTRL_RESET_MODE_SHIFT));wdt_start(wdt, AST_WDT_RESET_TIMEOUT);return -EINPROGRESS;+}
+static struct sysreset_ops ast_sysreset = {
.request = ast_sysreset_request,+};
+U_BOOT_DRIVER(sysreset_ast) = {
.name = "ast_sysreset",.id = UCLASS_SYSRESET,.ops = &ast_sysreset,+}; diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index cb18f12fc9..cd38a6d4bd 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -46,4 +46,16 @@ config OMAP_TIMER help Select this to enable an timer for Omap devices.
+config AST_TIMER
bool "Aspeed ast2400/ast2500 timer support"depends on TIMERdefault y if ARCH_ASPEEDhelpSelect this to enable timer for Aspeed ast2400/ast2500 devices.This is a simple sys timer driver, it is compatible with lib/time.c,but does not support any interrupts. Even though SoC has 8 hardwarecounters, they are all treated as a single device by this driver.This is mostly because they all share several registers whichmakes it difficult to completely separate them.endmenu diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile index f351fbb4e0..a4b1a486b0 100644 --- a/drivers/timer/Makefile +++ b/drivers/timer/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o obj-$(CONFIG_SANDBOX_TIMER) += sandbox_timer.o obj-$(CONFIG_X86_TSC_TIMER) += tsc_timer.o obj-$(CONFIG_OMAP_TIMER) += omap-timer.o +obj-$(CONFIG_AST_TIMER) += ast_timer.o diff --git a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c new file mode 100644 index 0000000000..d7c5460cd3 --- /dev/null +++ b/drivers/timer/ast_timer.c @@ -0,0 +1,97 @@ +/*
- Copyright 2016 Google Inc.
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <timer.h> +#include <asm/io.h> +#include <asm/arch/timer.h>
+DECLARE_GLOBAL_DATA_PTR;
+#define AST_TICK_TIMER 1 +#define AST_TMC_RELOAD_VAL 0xffffffff
+struct ast_timer_priv {
struct ast_timer *regs;struct ast_timer_counter *tmc;+};
+static struct ast_timer_counter *ast_get_timer_counter(struct ast_timer *timer,
int n)+{
if (n > 3)return &timer->timers2[n - 4];elsereturn &timer->timers1[n - 1];+}
+static int ast_timer_probe(struct udevice *dev) +{
struct ast_timer_priv *priv = dev_get_priv(dev);struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);writel(AST_TMC_RELOAD_VAL, &priv->tmc->reload_val);/** Stop the timer. This will also load reload_val into* the status register.*/clrbits_le32(&priv->regs->ctrl1,AST_TMC_EN << AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));/* Start the timer from the fixed 1MHz clock. */setbits_le32(&priv->regs->ctrl1,(AST_TMC_EN | AST_TMC_1MHZ) <<AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));uc_priv->clock_rate = AST_TMC_RATE;return 0;+}
+static int ast_timer_get_count(struct udevice *dev, u64 *count) +{
struct ast_timer_priv *priv = dev_get_priv(dev);*count = AST_TMC_RELOAD_VAL - readl(&priv->tmc->status);return 0;+}
+static int ast_timer_ofdata_to_platdata(struct udevice *dev) +{
struct ast_timer_priv *priv = dev_get_priv(dev);priv->regs = dev_get_addr_ptr(dev);if (IS_ERR(priv->regs))return PTR_ERR(priv->regs);priv->tmc = ast_get_timer_counter(priv->regs, AST_TICK_TIMER);return 0;+}
+static const struct timer_ops ast_timer_ops = {
.get_count = ast_timer_get_count,+};
+static const struct udevice_id ast_timer_ids[] = {
{ .compatible = "aspeed,ast2500-timer" },{ .compatible = "aspeed,ast2400-timer" },Here you can put .data = ... and use an enum to select it. Then your driver can operate with either at runtime.
Having said that I cannot see where ast_wdt_reset_masked() is called.
{ }+};
+U_BOOT_DRIVER(ast_timer) = {
.name = "ast_timer",.id = UCLASS_TIMER,.of_match = ast_timer_ids,.probe = ast_timer_probe,.priv_auto_alloc_size = sizeof(struct ast_timer_priv),.ofdata_to_platdata = ast_timer_ofdata_to_platdata,.ops = &ast_timer_ops,.flags = DM_FLAG_PRE_RELOC,+};
2.11.0.483.g087da7b7c-goog
Regards, Simon