On Tue, Mar 21, 2017 at 4:22 PM, Simon Glass <s...@chromium.org> wrote:
>
> Hi Maxim,
>
> On 16 March 2017 at 15:36, Maxim Sloyko <max...@google.com> wrote:
> > Add Reset Driver for ast2500 SoC. This driver uses Watchdog Timer to
> > perform resets and thus depends on it. The actual Watchdog device used
> > needs to be configured in Device Tree using "aspeed,wdt" property, which
> > must be WDT phandle, for example:
> >
> > rst: reset-controller {
> > compatible = "aspeed,ast2500-reset";
> > aspeed,wdt = <&wdt1>;
> > }
> >
> > Signed-off-by: Maxim Sloyko <max...@google.com>
> > ---
> >
> > arch/arm/include/asm/arch-aspeed/scu_ast2500.h | 28 +++++++
> > drivers/reset/Kconfig | 10 +++
> > drivers/reset/Makefile | 1 +
> > drivers/reset/ast2500-reset.c | 109
> > +++++++++++++++++++++++++
> > 4 files changed, 148 insertions(+)
> > create mode 100644 drivers/reset/ast2500-reset.c
> >
>
> Reviewed-by: Simon Glass <s...@chromium.org>
>
> Nits below.
>
> > diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > index 0fa3ecb9b9..e2556f920d 100644
> > --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> > @@ -31,6 +31,34 @@
> >
> > #define SCU_MISC_UARTCLK_DIV13 (1 << 12)
> >
> > +/*
> > + * SYSRESET is actually more like a Power register,
> > + * except that corresponding bit set to 1 means that
> > + * the peripheral is off.
> > + */
> > +#define SCU_SYSRESET_XDMA (1 << 25)
> > +#define SCU_SYSRESET_MCTP (1 << 24)
> > +#define SCU_SYSRESET_ADC (1 << 23)
> > +#define SCU_SYSRESET_JTAG (1 << 22)
> > +#define SCU_SYSRESET_MIC (1 << 18)
> > +#define SCU_SYSRESET_SDIO (1 << 16)
> > +#define SCU_SYSRESET_USB11HOST (1 << 15)
> > +#define SCU_SYSRESET_USBHUB (1 << 14)
> > +#define SCU_SYSRESET_CRT (1 << 13)
> > +#define SCU_SYSRESET_MAC2 (1 << 12)
> > +#define SCU_SYSRESET_MAC1 (1 << 11)
> > +#define SCU_SYSRESET_PECI (1 << 10)
> > +#define SCU_SYSRESET_PWM (1 << 9)
> > +#define SCU_SYSRESET_PCI_VGA (1 << 8)
> > +#define SCU_SYSRESET_2D (1 << 7)
> > +#define SCU_SYSRESET_VIDEO (1 << 6)
> > +#define SCU_SYSRESET_LPC (1 << 5)
> > +#define SCU_SYSRESET_HAC (1 << 4)
> > +#define SCU_SYSRESET_USBHID (1 << 3)
> > +#define SCU_SYSRESET_I2C (1 << 2)
> > +#define SCU_SYSRESET_AHB (1 << 1)
> > +#define SCU_SYSRESET_SDRAM_WDT (1 << 0)
> > +
> > #ifndef __ASSEMBLY__
> >
> > struct ast2500_clk_priv {
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index c42b0bcf0e..eb54189d4b 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -43,4 +43,14 @@ config RESET_UNIPHIER
> > Say Y if you want to control reset signals provided by System
> > Control
> > block, Media I/O block, Peripheral Block.
> >
> > +config AST2500_RESET
> > + bool "Reset controller driver for AST2500 SoCs"
> > + depends on DM_RESET && WDT_ASPEED
> > + default y if ASPEED_AST2500
> > + help
> > + Support for reset controller on AST2500 SoC. This controller uses
> > + watchdog to reset different peripherals and thus only supports
> > + resets that are supported by watchdog. The main limitation though
> > + is that some reset signals, like I2C or MISC reset multiple
> > devices.
> > +
> > endmenu
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 5c4305cc1d..16ad7eed5b 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -8,3 +8,4 @@ obj-$(CONFIG_SANDBOX_MBOX) += sandbox-reset-test.o
> > obj-$(CONFIG_TEGRA_CAR_RESET) += tegra-car-reset.o
> > obj-$(CONFIG_TEGRA186_RESET) += tegra186-reset.o
> > obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> > +obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o
> > diff --git a/drivers/reset/ast2500-reset.c b/drivers/reset/ast2500-reset.c
> > new file mode 100644
> > index 0000000000..0afa4bd01a
> > --- /dev/null
> > +++ b/drivers/reset/ast2500-reset.c
> > @@ -0,0 +1,109 @@
> > +/*
> > + * Copyright 2017 Google, Inc
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <misc.h>
> > +#include <reset.h>
> > +#include <reset-uclass.h>
> > +#include <wdt.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/scu_ast2500.h>
> > +#include <asm/arch/wdt.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct ast2500_reset_priv {
> > + /* WDT used to perform resets. */
> > + struct udevice *wdt;
> > + struct ast2500_scu *scu;
> > +};
> > +
> > +static int ast2500_ofdata_to_platdata(struct udevice *dev)
> > +{
> > + struct ast2500_reset_priv *priv = dev_get_priv(dev);
> > + int ret;
> > +
> > + ret = uclass_get_device_by_phandle(UCLASS_WDT, dev, "aspeed,wdt",
> > + &priv->wdt);
> > + if (ret) {
> > + debug("%s: can't find WDT for reset controller", __func__);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ast2500_reset_assert(struct reset_ctl *reset_ctl)
> > +{
> > + struct ast2500_reset_priv *priv = dev_get_priv(reset_ctl->dev);
> > + u32 reset_mode, reset_mask;
> > + bool reset_sdram;
> > + int ret;
> > +
> > + if (IS_ERR(priv))
> > + return PTR_ERR(priv);
>
> This can never happen so you can drop this code.
>
> > +
> > + /*
> > + * To reset SDRAM, specifal flag in SYSRESET register
>
> a special
>
> > + * needs to be enabled first
> > + */
> > + reset_mode = ast_reset_mode_from_flags(reset_ctl->id);
> > + reset_mask = ast_reset_mask_from_flags(reset_ctl->id);
> > + reset_sdram = reset_mode == WDT_CTRL_RESET_SOC &&
> > + (reset_mask & WDT_RESET_SDRAM);
> > +
> > + if (reset_sdram) {
> > + ast_scu_unlock(priv->scu);
> > + setbits_le32(&priv->scu->sysreset_ctrl1,
> > + SCU_SYSRESET_SDRAM_WDT);
> > + ret = wdt_reset(priv->wdt, reset_ctl->id);
> > + clrbits_le32(&priv->scu->sysreset_ctrl1,
> > + SCU_SYSRESET_SDRAM_WDT);
> > + ast_scu_lock(priv->scu);
> > + } else {
> > + ret = wdt_reset(priv->wdt, reset_ctl->id);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int ast2500_reset_request(struct reset_ctl *reset_ctl)
> > +{
> > + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl,
> > + reset_ctl->dev, reset_ctl->id);
>
> Should this do anything?
It needs to be there, because it is called unconditionally from
uclass. I don't know what it supposed to do though. It does not do
anything in tegra186-reset driver that I used as an example.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ast2500_reset_probe(struct udevice *dev)
> > +{
> > + struct ast2500_reset_priv *priv = dev_get_priv(dev);
> > +
> > + priv->scu = ast_get_scu();
>
> OK, but perhaps at some point scu should be a SYSCON device accessed
> with syscon_get_first_range()?
Ack.
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct udevice_id ast2500_reset_ids[] = {
> > + { .compatible = "aspeed,ast2500-reset" },
> > + { }
> > +};
> > +
> > +struct reset_ops ast2500_reset_ops = {
> > + .rst_assert = ast2500_reset_assert,
> > + .request = ast2500_reset_request,
> > +};
> > +
> > +U_BOOT_DRIVER(ast2500_reset) = {
> > + .name = "ast2500_reset",
> > + .id = UCLASS_RESET,
> > + .of_match = ast2500_reset_ids,
> > + .probe = ast2500_reset_probe,
> > + .ops = &ast2500_reset_ops,
> > + .ofdata_to_platdata = ast2500_ofdata_to_platdata,
> > + .priv_auto_alloc_size = sizeof(struct ast2500_reset_priv),
> > +};
> > --
> > 2.12.0.367.g23dc2f6d3c-goog
> >
>
> Regards,
> Simon
--
Maxim Sloyko
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
