[U-Boot] [PATCH v4 1/4] aspeed: Add drivers common to all Aspeed SoCs

Simon Glass sjg at chromium.org
Thu Jan 26 15:23:45 CET 2017


On 18 January 2017 at 14:44, Maxim Sloyko <maxims at 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 at 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 at 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_CONTROL
> +       select DM
> +
>  endchoice
>
> +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_ARM1176
> +       help
> +         The 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_AST2500
> +       help
> +         The 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 TIMER
> +       default y if ARCH_ASPEED
> +       help
> +         Select 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 hardware
> +         counters, they are all treated as a single device by this driver.
> +         This is mostly because they all share several registers which
> +         makes 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];
> +       else
> +               return &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


More information about the U-Boot mailing list