[U-Boot] [PATCH v3 1/3] reset: add reset driver for HiSilicon platform

[Joe Hershberger]

Hi Shawn,

On Sun, Mar 10, 2019 at 3:53 AM Shawn Guo <shawn.guo at linaro.org> wrote:
>
> It adds a Driver Model compatible reset driver for HiSlicon platform.
> The driver implements a custom .of_xlate function, and uses .data field
> as reset register offset and .id field as bit shift.
>
> Signed-off-by: Shawn Guo <shawn.guo at linaro.org>
> Reviewed-by: Igor Opaniuk <igor.opaniuk at linaro.org>
> ---
>  drivers/reset/Kconfig           |   6 ++
>  drivers/reset/Makefile          |   1 +
>  drivers/reset/reset-hisilicon.c | 111 ++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+)
>  create mode 100644 drivers/reset/reset-hisilicon.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index a81e76769604..6ec6f39c85f0 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -121,4 +121,10 @@ config RESET_SUNXI
>           This enables support for common reset driver for
>           Allwinner SoCs.
>
> +config RESET_HISILICON
> +       bool "Reset controller driver for HiSilicon SoCs"
> +       depends on DM_RESET
> +       help
> +         Support for reset controller on HiSilicon SoCs.
> +
>  endmenu
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4fad7d412985..7fec75bb4923 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_RESET_MEDIATEK) += reset-mediatek.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> +obj-$(CONFIG_RESET_HISILICON) += reset-hisilicon.o
> diff --git a/drivers/reset/reset-hisilicon.c b/drivers/reset/reset-hisilicon.c
> new file mode 100644
> index 000000000000..7b0c11fbc82e
> --- /dev/null
> +++ b/drivers/reset/reset-hisilicon.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#include <asm/io.h>
> +#include <common.h>
> +#include <dm.h>
> +#include <dt-bindings/reset/hisi-reset.h>

Where does this file come from?


> +#include <reset-uclass.h>
> +
> +struct hisi_reset_priv {
> +       void __iomem *base;
> +};
> +
> +static int hisi_reset_deassert(struct reset_ctl *rst)
> +{
> +       struct hisi_reset_priv *priv = dev_get_priv(rst->dev);
> +       u32 offset = rst->data & 0xffff;
> +       u32 shift = rst->data >> 16;
> +       int polarity = rst->id;
> +       u32 val;
> +
> +       val = readl(priv->base + offset);
> +       if (polarity == HISI_RESET_ACTIVE_HIGH)
> +               val &= ~BIT(shift);
> +       else
> +               val |= BIT(shift);
> +       writel(val, priv->base + offset);
> +
> +       return 0;
> +}
> +
> +static int hisi_reset_assert(struct reset_ctl *rst)
> +{
> +       struct hisi_reset_priv *priv = dev_get_priv(rst->dev);
> +       u32 offset = rst->data & 0xffff;
> +       u32 shift = rst->data >> 16;
> +       int polarity = rst->id;
> +       u32 val;
> +
> +       val = readl(priv->base + offset);
> +       if (polarity == HISI_RESET_ACTIVE_HIGH)
> +               val |= BIT(shift);
> +       else
> +               val &= ~BIT(shift);
> +       writel(val, priv->base + offset);
> +
> +       return 0;
> +}
> +
> +static int hisi_reset_free(struct reset_ctl *rst)
> +{
> +       return 0;
> +}
> +
> +static int hisi_reset_request(struct reset_ctl *rst)
> +{
> +       return 0;
> +}
> +
> +static int hisi_reset_of_xlate(struct reset_ctl *rst,
> +                              struct ofnode_phandle_args *args)
> +{
> +       if (args->args_count != 3) {
> +               debug("Invalid args_count: %d\n", args->args_count);
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Encode register offset in .data[15..0] and bit shift in
> +        * .data[31..16], and use .id field as polarity.
> +        */

I don't like going through these contortions to avoid changing the
struct in reset.h

I think you should add a "polarity" field to that struct and instead
of defining a specific constant for HISI_RESET_ACTIVE_HIGH, instead
make a generic one that everyone can use, such as the ASSERT_CLEAR and
friends in Linux.

I also hope you can get rid of the register offset and either include
it in the DT base address if it is something that needs to be selected
or simply make a #define for the 0xCC for what the register is called
and go from there. If both are not acceptable, I think it makes sense
to use "data" as the register.

> +       rst->data = (args->args[1] << 16) | (args->args[0] & 0xffff);
> +       rst->id = args->args[2];

I think "id" should be used to hold the "shift" or bit number of the reset.




> +
> +       return 0;
> +}
> +
> +static const struct reset_ops hisi_reset_reset_ops = {
> +       .of_xlate = hisi_reset_of_xlate,
> +       .request = hisi_reset_request,
> +       .free = hisi_reset_free,
> +       .rst_assert = hisi_reset_assert,
> +       .rst_deassert = hisi_reset_deassert,
> +};
> +
> +static const struct udevice_id hisi_reset_ids[] = {
> +       { .compatible = "hisilicon,hi3798cv200-reset" },
> +       { }
> +};
> +
> +static int hisi_reset_probe(struct udevice *dev)
> +{
> +       struct hisi_reset_priv *priv = dev_get_priv(dev);
> +
> +       priv->base = dev_remap_addr(dev);
> +       if (!priv->base)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(hisi_reset) = {
> +       .name = "hisilicon_reset",
> +       .id = UCLASS_RESET,
> +       .of_match = hisi_reset_ids,
> +       .ops = &hisi_reset_reset_ops,
> +       .probe = hisi_reset_probe,
> +       .priv_auto_alloc_size = sizeof(struct hisi_reset_priv),
> +};
> --
> 2.18.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot