[U-Boot] [RFC PATCH 04/13] reset: add driver for generic reset controllers

[Simon Glass]

Hi Andre,

On 29 November 2017 at 18:25, Andre Przywara <andre.przywara at arm.com> wrote:
> From: Amit Singh Tomar <amittomer25 at gmail.com>
>
> The simplest and most generic form of a reset controller just exposes
> multiple MMIO registers, where each bit toggles a separate reset line.
> Add a generic driver to describe this kind of reset controller.
>
> This is used on the Nexell S5P6818, for instance, but also by other
> SoCs.
>
> Signed-off-by: Amit Singh Tomar <amittomer25 at gmail.com>
> [Andre: make more generic, let it cover multiple registers, slight rework]
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
>  drivers/reset/Kconfig         |   6 +++
>  drivers/reset/Makefile        |   1 +
>  drivers/reset/reset-generic.c | 111 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+)
>  create mode 100644 drivers/reset/reset-generic.c

Reviewed-by: Simon Glass <sjg at chromium.org>

Is there a DT binding for this?

Also please see below.

>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index ce46e2752c..3032b0064c 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,6 +12,12 @@ config DM_RESET
>           although driving such reset isgnals using GPIOs may be more
>           appropriate in this case.
>
> +config GENERIC_RESET
> +        bool "Generic Reset controller driver"
> +        depends on DM_RESET
> +        help
> +          Support Generic reset controller.

What is this? This help needs to be expanded with details.

> +
>  config SANDBOX_RESET
>         bool "Enable the sandbox reset test driver"
>         depends on DM_MAILBOX && SANDBOX
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 252cefeed5..81680837ef 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_TEGRA186_RESET) += tegra186-reset.o
>  obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
>  obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o
> +obj-$(CONFIG_GENERIC_RESET) += reset-generic.o
> diff --git a/drivers/reset/reset-generic.c b/drivers/reset/reset-generic.c
> new file mode 100644
> index 0000000000..0a7740202d
> --- /dev/null
> +++ b/drivers/reset/reset-generic.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (C) 2017 Amit Singh Tomar <amittomer25 at gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <reset-uclass.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <linux/sizes.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct generic_reset_priv {
> +       void __iomem *membase;
> +       int max_reset;

comment

> +};
> +
> +#define BITS_PER_BYTE 8

blank line here

> +static int generic_reset_toggle(struct reset_ctl *rst, bool assert)
> +{
> +       struct generic_reset_priv *priv = dev_get_priv(rst->dev);
> +       int reg_width = sizeof(u32);
> +       int bank, offset;
> +       u32 reg;
> +
> +       if (rst->id >= priv->max_reset)
> +               return -EINVAL;
> +
> +       bank = rst->id / (reg_width * BITS_PER_BYTE);
> +       offset = rst->id % (reg_width * BITS_PER_BYTE);
> +
> +       reg = readl(priv->membase + (bank * reg_width));

Can you put that expression in a local var?


> +       if (assert)
> +               writel(reg & ~BIT(offset), priv->membase + (bank * reg_width));
> +       else
> +               writel(reg | BIT(offset), priv->membase + (bank * reg_width));
> +
> +       return 0;
> +}
> +
> +static int generic_reset_assert(struct reset_ctl *rst)
> +{
> +       return generic_reset_toggle(rst, true);
> +}
> +
> +static int generic_reset_deassert(struct reset_ctl *rst)
> +{
> +       return generic_reset_toggle(rst, false);
> +}
> +
> +static int generic_reset_free(struct reset_ctl *rst)
> +{
> +       return 0;
> +}
> +
> +static int generic_reset_request(struct reset_ctl *rst)
> +{
> +       struct generic_reset_priv *priv = dev_get_priv(rst->dev);
> +
> +       if (rst->id >= priv->max_reset)
> +               return -EINVAL;

OK I suppose. How about -ENOENT?

> +
> +       return generic_reset_assert(rst);
> +}
> +
> +struct reset_ops generic_reset_reset_ops = {
> +       .free = generic_reset_free,
> +       .request = generic_reset_request,
> +       .rst_assert = generic_reset_assert,
> +       .rst_deassert = generic_reset_deassert,
> +};
> +
> +static const struct udevice_id generic_reset_ids[] = {
> +       { .compatible = "generic-reset" },
> +       { .compatible = "nexell,s5p6818-reset" },
> +       { }
> +};
> +
> +static int generic_reset_probe(struct udevice *dev)
> +{
> +       struct generic_reset_priv *priv = dev_get_priv(dev);
> +       fdt_addr_t addr;
> +       fdt_size_t size;
> +
> +       addr = devfdt_get_addr_size_index(dev, 0, &size);

Can we use dev_read_... here?

> +       if (addr == FDT_ADDR_T_NONE)
> +               return -EINVAL;
> +
> +       priv->max_reset = dev_read_u32_default(dev, "num-resets", -1);
> +       if (priv->max_reset == -1)
> +               priv->max_reset = size * BITS_PER_BYTE;
> +
> +       priv->membase = devm_ioremap(dev, addr, size);
> +       if (!priv->membase)
> +               return -EFAULT;
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(generic_reset) = {
> +       .name = "generic_reset",
> +       .id = UCLASS_RESET,
> +       .of_match = generic_reset_ids,
> +       .ops = &generic_reset_reset_ops,
> +       .probe = generic_reset_probe,
> +       .priv_auto_alloc_size = sizeof(struct generic_reset_priv),
> +};
> --
> 2.14.1
>

Regards,
Simon