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

[Shawn Guo <shawn.guo@linaro.org>]

Hi Joe,

Thanks for the comments.

On Tue, Mar 19, 2019 at 06:42:06PM +0000, Joe Hershberger wrote:
> Hi Shawn,
> 
> On Sun, Mar 10, 2019 at 3:53 AM Shawn Guo <shawn.guo@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@linaro.org>
> > Reviewed-by: Igor Opaniuk <igor.opaniuk@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?

Sorry, my bad.  It's a new file in my working tree, and I forgot to add
it.  It becomes unneeded in the next version though.

> 
> 
> > +#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.

Okay, will do in the next version.  As the ASSERT_CLEAR and friends are
defined in include/dt-bindings/reset/ti-syscon.h (already copied into
U-Boot from Linux), I will just use this header.

> 
> 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.

Although the resets we need for GMAC happen to be in a single register,
the controller includes resets for other modules, that spread in
different registers.  I would like to get the driver ready for other
clients to use in the future.

> 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.

Yes.  This is exactly what v2 does - use 'data' as register offset and
'id' as shift.  Will go back to this in the next version.

Shawn

> 
> 
> 
> 
> > +
> > +       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