diff mbox series

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

Message ID 20190310085127.22201-2-shawn.guo@linaro.org
State Superseded
Delegated to: Tom Rini
Headers show
Series Add Ethernet support for Poplar board | expand

Commit Message

Shawn Guo March 10, 2019, 8:51 a.m. UTC
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

Comments

Joe Hershberger March 19, 2019, 6:42 p.m. UTC | #1
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?


> +#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@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Shawn Guo March 20, 2019, 3:13 a.m. UTC | #2
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@lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
diff mbox series

Patch

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>
+#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.
+	 */
+	rst->data = (args->args[1] << 16) | (args->args[0] & 0xffff);
+	rst->id = args->args[2];
+
+	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),
+};