[PATCH v2 0/5] Unify simple reset drivers

[PATCH v2 0/5] Unify simple reset drivers

[Philipp Zabel]

Hi,

from time to time people notice that we are slowly accumulating reset drivers
for simple reset controllers. "Simple" in this context means that they allow to
directly control reset lines by setting and clearing bits in a single register
or in a contiguous register range that is exclusive to the reset controller.

I had previously suggested to add common reset ops for them [1], and Andre
advised to combine them into a single platform driver instead. This series
does that. It should allow to replace the socfpga, stm32, and zx2967 drivers
entirely.
The SoCFPGA device tree quirk parsing is now contained in reset-simple.c.
The sunxi driver is kept around to register the early reset controllers,
but it reuses the exported reset_simple_ops.

[1] https://patchwork.kernel.org/patch/9610709/

regards
Philipp

Philipp Zabel (5):
  reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967
  reset: socfpga: use the reset-simple driver
  reset: stm32: use the reset-simple driver
  reset: zx2967: use the reset-simple driver
  reset: simple: read back to make sure changes are applied

 MAINTAINERS                   |   1 -
 drivers/reset/Kconfig         |  24 +++---
 drivers/reset/Makefile        |   4 +-
 drivers/reset/reset-simple.c  | 183 ++++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-simple.h  |  33 ++++++++
 drivers/reset/reset-socfpga.c | 154 -----------------------------------
 drivers/reset/reset-stm32.c   | 108 -------------------------
 drivers/reset/reset-sunxi.c   | 104 ++----------------------
 drivers/reset/reset-zx2967.c  |  99 -----------------------
 9 files changed, 232 insertions(+), 478 deletions(-)
 create mode 100644 drivers/reset/reset-simple.c
 create mode 100644 drivers/reset/reset-simple.h
 delete mode 100644 drivers/reset/reset-socfpga.c
 delete mode 100644 drivers/reset/reset-stm32.c
 delete mode 100644 drivers/reset/reset-zx2967.c

-- 
2.11.0

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Philipp Zabel]

Split reusable parts out of the sunxi driver, to add a driver for simple
reset controllers with reset lines that can be controlled by toggling
bits in exclusive, contiguous register ranges using read-modify-write
cycles under a spinlock. The separate sunxi driver still remains to
register the early reset controllers, but it reuses the reset-simple
ops.

The following patches will replace compatible reset drivers with
reset-simple, extending it where necessary.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Turn reset-simple into a complete platform driver.
- Move common code out of assert and deassert ops into a helper function.
- Add inverted flag to support both clear- and set-to-assert reset bits.
- Remove status readback, will be added in the next patch.
- Split out SoCFPGA and STM32 conversion into separate patches.
---
 drivers/reset/Kconfig        |  11 ++++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-simple.c | 129 +++++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-simple.h |  32 +++++++++++
 drivers/reset/reset-sunxi.c  | 104 ++--------------------------------
 5 files changed, 179 insertions(+), 98 deletions(-)
 create mode 100644 drivers/reset/reset-simple.c
 create mode 100644 drivers/reset/reset-simple.h

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 52d5251660b9b..f7ba01a71daee 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -68,6 +68,16 @@ config RESET_PISTACHIO
 	help
 	  This enables the reset driver for ImgTec Pistachio SoCs.
 
+config RESET_SIMPLE
+	bool "Simple Reset Controller Driver" if COMPILE_TEST
+	default ARCH_SUNXI
+	help
+	  This enables a simple reset controller driver for reset lines that
+	  that can be asserted and deasserted by toggling bits in a contiguous,
+	  exclusive register space.
+
+	  Currently this driver supports Allwinner SoCs.
+
 config RESET_SOCFPGA
 	bool "SoCFPGA Reset Driver" if COMPILE_TEST
 	default ARCH_SOCFPGA
@@ -83,6 +93,7 @@ config RESET_STM32
 config RESET_SUNXI
 	bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
 	default ARCH_SUNXI
+	select RESET_SIMPLE
 	help
 	  This enables the reset driver for Allwinner SoCs.
 
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index b62783f50fe5b..ab4af99c3dfdc 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_RESET_MESON) += reset-meson.o
 obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
 obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
+obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_RESET_STM32) += reset-stm32.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
new file mode 100644
index 0000000000000..08fc79d79e86d
--- /dev/null
+++ b/drivers/reset/reset-simple.c
@@ -0,0 +1,129 @@
+/*
+ * Simple Reset Controller Driver
+ *
+ * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
+ *
+ * Based on Allwinner SoCs Reset Controller driver
+ *
+ * Copyright 2013 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+
+#include "reset-simple.h"
+
+static inline struct reset_simple_data *
+to_reset_simple_data(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct reset_simple_data, rcdev);
+}
+
+static int reset_simple_set(struct reset_controller_dev *rcdev,
+			    unsigned long id, bool assert)
+{
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	reg = readl(data->membase + (bank * reg_width));
+	if (assert ^ data->inverted)
+		reg |= BIT(offset);
+	else
+		reg &= ~BIT(offset);
+	writel(reg, data->membase + (bank * reg_width));
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static int reset_simple_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	return reset_simple_set(rcdev, id, true);
+}
+
+static int reset_simple_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	return reset_simple_set(rcdev, id, false);
+}
+
+const struct reset_control_ops reset_simple_ops = {
+	.assert		= reset_simple_assert,
+	.deassert	= reset_simple_deassert,
+};
+
+struct reset_simple_devdata {
+	bool inverted;
+};
+
+static const struct reset_simple_devdata reset_simple_inverted = {
+	.inverted = true,
+};
+
+static const struct of_device_id reset_simple_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-clock-reset",
+		.data = &reset_simple_inverted },
+	{ /* sentinel */ },
+};
+
+static int reset_simple_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id =
+		of_match_device(of_match_ptr(reset_simple_dt_ids), dev);
+	const struct reset_simple_devdata *devdata = of_id->data;
+	struct reset_simple_data *data;
+	void __iomem *membase;
+	struct resource *res;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(membase))
+		return PTR_ERR(membase);
+
+	spin_lock_init(&data->lock);
+	data->membase = membase;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE;
+	data->rcdev.ops = &reset_simple_ops;
+	data->rcdev.of_node = dev->of_node;
+
+	if (devdata)
+		data->inverted = devdata->inverted;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static struct platform_driver reset_simple_driver = {
+	.probe	= reset_simple_probe,
+	.driver = {
+		.name		= "simple-reset",
+		.of_match_table	= reset_simple_dt_ids,
+	},
+};
+builtin_platform_driver(reset_simple_driver);
diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
new file mode 100644
index 0000000000000..a26dc62b2f349
--- /dev/null
+++ b/drivers/reset/reset-simple.h
@@ -0,0 +1,32 @@
+/*
+ * Simple Reset Controller ops
+ *
+ * Based on Allwinner SoCs Reset Controller driver
+ *
+ * Copyright 2013 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __RESET_SIMPLE_H__
+#define __RESET_SIMPLE_H__
+
+#include <linux/io.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+
+struct reset_simple_data {
+	spinlock_t			lock;
+	void __iomem			*membase;
+	struct reset_controller_dev	rcdev;
+	bool				inverted;
+};
+
+extern const struct reset_control_ops reset_simple_ops;
+
+#endif /* __RESET_SIMPLE_H__ */
diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
index 2c7dd1fd08df6..320e4e76ea8cd 100644
--- a/drivers/reset/reset-sunxi.c
+++ b/drivers/reset/reset-sunxi.c
@@ -22,64 +22,11 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
-struct sunxi_reset_data {
-	spinlock_t			lock;
-	void __iomem			*membase;
-	struct reset_controller_dev	rcdev;
-};
-
-static int sunxi_reset_assert(struct reset_controller_dev *rcdev,
-			      unsigned long id)
-{
-	struct sunxi_reset_data *data = container_of(rcdev,
-						     struct sunxi_reset_data,
-						     rcdev);
-	int reg_width = sizeof(u32);
-	int bank = id / (reg_width * BITS_PER_BYTE);
-	int offset = id % (reg_width * BITS_PER_BYTE);
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * reg_width));
-	writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static int sunxi_reset_deassert(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	struct sunxi_reset_data *data = container_of(rcdev,
-						     struct sunxi_reset_data,
-						     rcdev);
-	int reg_width = sizeof(u32);
-	int bank = id / (reg_width * BITS_PER_BYTE);
-	int offset = id % (reg_width * BITS_PER_BYTE);
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * reg_width));
-	writel(reg | BIT(offset), data->membase + (bank * reg_width));
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static const struct reset_control_ops sunxi_reset_ops = {
-	.assert		= sunxi_reset_assert,
-	.deassert	= sunxi_reset_deassert,
-};
+#include "reset-simple.h"
 
 static int sunxi_reset_init(struct device_node *np)
 {
-	struct sunxi_reset_data *data;
+	struct reset_simple_data *data;
 	struct resource res;
 	resource_size_t size;
 	int ret;
@@ -108,8 +55,9 @@ static int sunxi_reset_init(struct device_node *np)
 
 	data->rcdev.owner = THIS_MODULE;
 	data->rcdev.nr_resets = size * 8;
-	data->rcdev.ops = &sunxi_reset_ops;
+	data->rcdev.ops = &reset_simple_ops;
 	data->rcdev.of_node = np;
+	data->inverted = true;
 
 	return reset_controller_register(&data->rcdev);
 
@@ -122,6 +70,8 @@ static int sunxi_reset_init(struct device_node *np)
  * These are the reset controller we need to initialize early on in
  * our system, before we can even think of using a regular device
  * driver for it.
+ * The controllers that we can register through the regular device
+ * model are handled by the simple reset driver directly.
  */
 static const struct of_device_id sunxi_early_reset_dt_ids[] __initconst = {
 	{ .compatible = "allwinner,sun6i-a31-ahb1-reset", },
@@ -135,45 +85,3 @@ void __init sun6i_reset_init(void)
 	for_each_matching_node(np, sunxi_early_reset_dt_ids)
 		sunxi_reset_init(np);
 }
-
-/*
- * And these are the controllers we can register through the regular
- * device model.
- */
-static const struct of_device_id sunxi_reset_dt_ids[] = {
-	 { .compatible = "allwinner,sun6i-a31-clock-reset", },
-	 { /* sentinel */ },
-};
-
-static int sunxi_reset_probe(struct platform_device *pdev)
-{
-	struct sunxi_reset_data *data;
-	struct resource *res;
-
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(data->membase))
-		return PTR_ERR(data->membase);
-
-	spin_lock_init(&data->lock);
-
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = resource_size(res) * 8;
-	data->rcdev.ops = &sunxi_reset_ops;
-	data->rcdev.of_node = pdev->dev.of_node;
-
-	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
-}
-
-static struct platform_driver sunxi_reset_driver = {
-	.probe	= sunxi_reset_probe,
-	.driver = {
-		.name		= "sunxi-reset",
-		.of_match_table	= sunxi_reset_dt_ids,
-	},
-};
-builtin_platform_driver(sunxi_reset_driver);
-- 
2.11.0

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Dinh Nguyen]

Hi Phillip,

This patch is failing to apply on both v4.13-rc4 and linux-next:

error: patch failed: drivers/reset/reset-sunxi.c:108
error: drivers/reset/reset-sunxi.c: patch does not apply
Patch failed at 0001 reset: add reset-simple to unify socfpga, stm32,
sunxi, and zx2967


On 08/11/2017 08:06 AM, Philipp Zabel wrote:
> Split reusable parts out of the sunxi driver, to add a driver for simple
> reset controllers with reset lines that can be controlled by toggling
> bits in exclusive, contiguous register ranges using read-modify-write
> cycles under a spinlock. The separate sunxi driver still remains to
> register the early reset controllers, but it reuses the reset-simple
> ops.
> 
> The following patches will replace compatible reset drivers with
> reset-simple, extending it where necessary.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
> - Turn reset-simple into a complete platform driver.
> - Move common code out of assert and deassert ops into a helper function.
> - Add inverted flag to support both clear- and set-to-assert reset bits.
> - Remove status readback, will be added in the next patch.
> - Split out SoCFPGA and STM32 conversion into separate patches.
> ---

[snip]

> @@ -108,8 +55,9 @@ static int sunxi_reset_init(struct device_node *np)
>  
>  	data->rcdev.owner = THIS_MODULE;
>  	data->rcdev.nr_resets = size * 8;

I think the problem the above line. On both linux-next and v4.13-rc4:

data->rcdev.nr_resets = size * 32;

Dinh

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Philipp Zabel]

Hi Dinh,

On Fri, 2017-08-11 at 10:39 -0500, Dinh Nguyen wrote:
> Hi Phillip,
> 
> This patch is failing to apply on both v4.13-rc4 and linux-next:
> 
> error: patch failed: drivers/reset/reset-sunxi.c:108
> error: drivers/reset/reset-sunxi.c: patch does not apply
> Patch failed at 0001 reset: add reset-simple to unify socfpga, stm32,
> sunxi, and zx2967

Sorry, I should have mentioned this is based on?[1].

[1] https://patchwork.kernel.org/patch/9895433/

That patch should make it into linux-next the next time it is rebuilt.
Right now it only sits in the reset/next branch at

    https://git.pengutronix.de/git/pza/linux reset/next

regards
Philipp

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Chen-Yu Tsai]

On Fri, Aug 11, 2017 at 9:06 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Split reusable parts out of the sunxi driver, to add a driver for simple
> reset controllers with reset lines that can be controlled by toggling
> bits in exclusive, contiguous register ranges using read-modify-write
> cycles under a spinlock. The separate sunxi driver still remains to
> register the early reset controllers, but it reuses the reset-simple
> ops.
>
> The following patches will replace compatible reset drivers with
> reset-simple, extending it where necessary.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
> - Turn reset-simple into a complete platform driver.
> - Move common code out of assert and deassert ops into a helper function.
> - Add inverted flag to support both clear- and set-to-assert reset bits.
> - Remove status readback, will be added in the next patch.
> - Split out SoCFPGA and STM32 conversion into separate patches.
> ---
>  drivers/reset/Kconfig        |  11 ++++
>  drivers/reset/Makefile       |   1 +
>  drivers/reset/reset-simple.c | 129 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/reset/reset-simple.h |  32 +++++++++++
>  drivers/reset/reset-sunxi.c  | 104 ++--------------------------------
>  5 files changed, 179 insertions(+), 98 deletions(-)
>  create mode 100644 drivers/reset/reset-simple.c
>  create mode 100644 drivers/reset/reset-simple.h
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 52d5251660b9b..f7ba01a71daee 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -68,6 +68,16 @@ config RESET_PISTACHIO
>         help
>           This enables the reset driver for ImgTec Pistachio SoCs.
>
> +config RESET_SIMPLE
> +       bool "Simple Reset Controller Driver" if COMPILE_TEST
> +       default ARCH_SUNXI
> +       help
> +         This enables a simple reset controller driver for reset lines that
> +         that can be asserted and deasserted by toggling bits in a contiguous,
> +         exclusive register space.
> +
> +         Currently this driver supports Allwinner SoCs.
> +
>  config RESET_SOCFPGA
>         bool "SoCFPGA Reset Driver" if COMPILE_TEST
>         default ARCH_SOCFPGA
> @@ -83,6 +93,7 @@ config RESET_STM32
>  config RESET_SUNXI
>         bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
>         default ARCH_SUNXI
> +       select RESET_SIMPLE
>         help
>           This enables the reset driver for Allwinner SoCs.
>
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index b62783f50fe5b..ab4af99c3dfdc 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
>  obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_RESET_STM32) += reset-stm32.o
>  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> new file mode 100644
> index 0000000000000..08fc79d79e86d
> --- /dev/null
> +++ b/drivers/reset/reset-simple.c
> @@ -0,0 +1,129 @@
> +/*
> + * Simple Reset Controller Driver
> + *
> + * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +#include "reset-simple.h"
> +
> +static inline struct reset_simple_data *
> +to_reset_simple_data(struct reset_controller_dev *rcdev)
> +{
> +       return container_of(rcdev, struct reset_simple_data, rcdev);
> +}
> +
> +static int reset_simple_set(struct reset_controller_dev *rcdev,
> +                           unsigned long id, bool assert)
> +{
> +       struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +       int reg_width = sizeof(u32);
> +       int bank = id / (reg_width * BITS_PER_BYTE);
> +       int offset = id % (reg_width * BITS_PER_BYTE);
> +       unsigned long flags;
> +       u32 reg;
> +
> +       spin_lock_irqsave(&data->lock, flags);
> +
> +       reg = readl(data->membase + (bank * reg_width));
> +       if (assert ^ data->inverted)
> +               reg |= BIT(offset);
> +       else
> +               reg &= ~BIT(offset);
> +       writel(reg, data->membase + (bank * reg_width));
> +
> +       spin_unlock_irqrestore(&data->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int reset_simple_assert(struct reset_controller_dev *rcdev,
> +                              unsigned long id)
> +{
> +       return reset_simple_set(rcdev, id, true);
> +}
> +
> +static int reset_simple_deassert(struct reset_controller_dev *rcdev,
> +                                unsigned long id)
> +{
> +       return reset_simple_set(rcdev, id, false);
> +}
> +
> +const struct reset_control_ops reset_simple_ops = {
> +       .assert         = reset_simple_assert,
> +       .deassert       = reset_simple_deassert,
> +};
> +
> +struct reset_simple_devdata {
> +       bool inverted;
> +};
> +
> +static const struct reset_simple_devdata reset_simple_inverted = {
> +       .inverted = true,
> +};
> +
> +static const struct of_device_id reset_simple_dt_ids[] = {
> +       { .compatible = "allwinner,sun6i-a31-clock-reset",
> +               .data = &reset_simple_inverted },
> +       { /* sentinel */ },
> +};
> +
> +static int reset_simple_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       const struct of_device_id *of_id =
> +               of_match_device(of_match_ptr(reset_simple_dt_ids), dev);
> +       const struct reset_simple_devdata *devdata = of_id->data;

Just use of_device_get_match_data().

> +       struct reset_simple_data *data;
> +       void __iomem *membase;
> +       struct resource *res;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       membase = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(membase))
> +               return PTR_ERR(membase);
> +
> +       spin_lock_init(&data->lock);
> +       data->membase = membase;
> +       data->rcdev.owner = THIS_MODULE;
> +       data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE;
> +       data->rcdev.ops = &reset_simple_ops;
> +       data->rcdev.of_node = dev->of_node;
> +
> +       if (devdata)
> +               data->inverted = devdata->inverted;
> +
> +       return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +static struct platform_driver reset_simple_driver = {
> +       .probe  = reset_simple_probe,
> +       .driver = {
> +               .name           = "simple-reset",
> +               .of_match_table = reset_simple_dt_ids,
> +       },
> +};
> +builtin_platform_driver(reset_simple_driver);
> diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
> new file mode 100644
> index 0000000000000..a26dc62b2f349
> --- /dev/null
> +++ b/drivers/reset/reset-simple.h
> @@ -0,0 +1,32 @@
> +/*
> + * Simple Reset Controller ops
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __RESET_SIMPLE_H__
> +#define __RESET_SIMPLE_H__
> +
> +#include <linux/io.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +struct reset_simple_data {
> +       spinlock_t                      lock;
> +       void __iomem                    *membase;
> +       struct reset_controller_dev     rcdev;
> +       bool                            inverted;

You should document this option. "Inverted" by itself does not
say a whole lot, as there is no mention about what the normal
or non-inverted behavior is. Is the reset active low (assert
reset when bit is cleared)? Or active high (assert reset when
bit is set)?

ChenYu

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Philipp Zabel]

On Fri, 2017-08-11 at 23:51 +0800, Chen-Yu Tsai wrote:
> On Fri, Aug 11, 2017 at 9:06 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > +static int reset_simple_probe(struct platform_device *pdev)
> > +{
> > +???????struct device *dev = &pdev->dev;
> > +???????const struct of_device_id *of_id =
> > +???????????????of_match_device(of_match_ptr(reset_simple_dt_ids), dev);
> > +???????const struct reset_simple_devdata *devdata = of_id->data;
> 
> Just use of_device_get_match_data().

Will do that, thanks.

> > +struct reset_simple_data {
> > +???????spinlock_t??????????????????????lock;
> > +???????void __iomem????????????????????*membase;
> > +???????struct reset_controller_dev?????rcdev;
> > +???????bool????????????????????????????inverted;
> 
> You should document this option. "Inverted" by itself does not
> say a whole lot, as there is no mention about what the normal
> or non-inverted behavior is. Is the reset active low (assert
> reset when bit is cleared)? Or active high (assert reset when
> bit is set)?

You are right. Also, maybe I should rename this to
"bool active_low;" to avoid confusion where it is used.

regards
Philipp

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Alexandru Gagniuc]

Hi Phillip,

On 08/11/2017 06:06 AM, Philipp Zabel wrote:
[snip]

> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 52d5251660b9b..f7ba01a71daee 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -68,6 +68,16 @@ config RESET_PISTACHIO
>  	help
>  	  This enables the reset driver for ImgTec Pistachio SoCs.
>
> +config RESET_SIMPLE
> +	bool "Simple Reset Controller Driver" if COMPILE_TEST
> +	default ARCH_SUNXI

This seems like a list with the potential to grow unbounded. I think it 
would be better for the platforms to 'select RESET_SIMPLE' rather than 
trying to guess all users in one big kconfig line.

[snip]

> +
> +struct reset_simple_devdata {
> +	bool inverted;
> +};
> +
> +static const struct reset_simple_devdata reset_simple_inverted = {
> +	.inverted = true,

Hmm. I think it would be useful for new devices if there were have a way 
to specify this in devicetree.

> +};
> +
> +static const struct of_device_id reset_simple_dt_ids[] = {
> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
> +		.data = &reset_simple_inverted },
> +	{ /* sentinel */ },

Are plans to have a "simple-reset" compatible binding for new devices?

Alex

[snip]

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Philipp Zabel]

Hi Alexandru,

On Fri, 2017-08-11 at 09:04 -0700, Alexandru Gagniuc wrote:
> Hi Phillip,
> 
> On 08/11/2017 06:06 AM, Philipp Zabel wrote:
> [snip]
> 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 52d5251660b9b..f7ba01a71daee 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -68,6 +68,16 @@ config RESET_PISTACHIO
> > > > ?	help
> > > > ?	??This enables the reset driver for ImgTec Pistachio SoCs.
> > 
> > +config RESET_SIMPLE
> > > > +	bool "Simple Reset Controller Driver" if COMPILE_TEST
> > +	default ARCH_SUNXI
> 
> This seems like a list with the potential to grow unbounded. I think it?
> would be better for the platforms to 'select RESET_SIMPLE' rather than?
> trying to guess all users in one big kconfig line.

That is a good point. Before that can be done, though, I have to hide
the RESET_CONTROLLER symbol and make drivers select it, instead of
hiding them behind "if RESET_CONTROLLER".

> > +struct reset_simple_devdata {
> > > > +	bool inverted;
> > +};
> > +
> > +static const struct reset_simple_devdata reset_simple_inverted = {
> > +	.inverted = true,
> 
> Hmm. I think it would be useful for new devices if there were have a way?
> to specify this in devicetree.

This is a separate issue, and it should be discussed including the
device tree list.

It could be as simple as a boolean "active-low;" property. But then
absence of that property would not mean active-high for existing
active-low drivers, which is confusing.

It could also reuse the flags already defined in
dt-bindings/reset/ti-syscon.h and have some kind of
    reset-flags = <ASSERT_SET | DEASSERT_CLEAR | STATUS_SET>;
property.

Suggestions welcome.

> > +};
> > +
> > +static const struct of_device_id reset_simple_dt_ids[] = {
> > +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
> > +		.data = &reset_simple_inverted },
> > +	{ /* sentinel */ },
> 
> Are plans to have a "simple-reset" compatible binding for new
> devices?

This would go hand in hand with the above discussion.
I'm not convinced this properly describes the hardware, and I'd like to
avoid leaking Linuxisms into the device trees. But this is a decision
for the device tree maintainers, and if they like it, I'm going to
comply.

regards
Philipp

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Eugeniy Paltsev]

Hi Philipp,

On Fri, 2017-08-11 at 18:37 +0200, Philipp Zabel wrote:
> > > +struct reset_simple_devdata {
> > > > > +	bool inverted;
> > > 
> > > +};
> > > +
> > > +static const struct reset_simple_devdata reset_simple_inverted =
> > > {
> > > +	.inverted = true,
> > 
> > Hmm. I think it would be useful for new devices if there were have
> > a way?
> > to specify this in devicetree.
> 
> This is a separate issue, and it should be discussed including the
> device tree list.
> 
> It could be as simple as a boolean "active-low;" property. But then
> absence of that property would not mean active-high for existing
> active-low drivers, which is confusing.
> 
> It could also reuse the flags already defined in
> dt-bindings/reset/ti-syscon.h and have some kind of
> ????reset-flags = <ASSERT_SET | DEASSERT_CLEAR | STATUS_SET>;
> property.
> 
> Suggestions welcome.
> 

Imho,

reset-type = <ACTIVE_LOW | ACTIVE_HIGH>;

looks good.
-- 
?Eugeniy Paltsev

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Alexandru Gagniuc]

Hi Philipp,

On 08/11/2017 06:06 AM, Philipp Zabel wrote:
[snip]

> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 52d5251660b9b..f7ba01a71daee 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -68,6 +68,16 @@ config RESET_PISTACHIO
>  	help
>  	  This enables the reset driver for ImgTec Pistachio SoCs.
>
> +config RESET_SIMPLE
> +	bool "Simple Reset Controller Driver" if COMPILE_TEST

Another question. Is there a reason this is depended on COMPILE_TEST ?

Alex

[PATCH v2 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Philipp Zabel]

On Fri, 2017-08-11 at 10:47 -0700, Alexandru Gagniuc wrote:
> Hi Philipp,
> 
> On 08/11/2017 06:06 AM, Philipp Zabel wrote:
> [snip]
> 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 52d5251660b9b..f7ba01a71daee 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -68,6 +68,16 @@ config RESET_PISTACHIO
> > ?	help
> > ?	??This enables the reset driver for ImgTec Pistachio SoCs.
> > 
> > +config RESET_SIMPLE
> > +	bool "Simple Reset Controller Driver" if COMPILE_TEST
> 
> Another question. Is there a reason this is depended on COMPILE_TEST
> ?

The idea is that if compile test is disabled, you are not presented
with the option, but it is set to the correct value via the default
mechanism. That way it is normally not possible to disable necessary
reset controller drivers for enabled architectures, but the driver can
still be built even on other architectures.

It's a workaround for selecting the driver from the arch, which
currently can't be done because of the encasing menuconfig.

regards
Philipp

[PATCH v2 2/5] reset: socfpga: use the reset-simple driver

[Philipp Zabel]

Add reset line status readback, inverted status support, and socfpga
device tree quirks to the simple reset driver, and use it to replace
the socfpga driver.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

fixup! reset: socfpga: use the reset-simple driver
---
 drivers/reset/Kconfig         |  10 +--
 drivers/reset/Makefile        |   1 -
 drivers/reset/reset-simple.c  |  51 +++++++++++++-
 drivers/reset/reset-simple.h  |   1 +
 drivers/reset/reset-socfpga.c | 154 ------------------------------------------
 5 files changed, 53 insertions(+), 164 deletions(-)
 delete mode 100644 drivers/reset/reset-socfpga.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index f7ba01a71daee..78a8f6057985b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -70,19 +70,13 @@ config RESET_PISTACHIO
 
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
-	default ARCH_SUNXI
+	default ARCH_SOCFPGA || ARCH_SUNXI
 	help
 	  This enables a simple reset controller driver for reset lines that
 	  that can be asserted and deasserted by toggling bits in a contiguous,
 	  exclusive register space.
 
-	  Currently this driver supports Allwinner SoCs.
-
-config RESET_SOCFPGA
-	bool "SoCFPGA Reset Driver" if COMPILE_TEST
-	default ARCH_SOCFPGA
-	help
-	  This enables the reset controller driver for Altera SoCFPGAs.
+	  Currently this driver supports Altera SoCFPGAs and Allwinner SoCs.
 
 config RESET_STM32
 	bool "STM32 Reset Driver" if COMPILE_TEST
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index ab4af99c3dfdc..25f5f722dec01 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -12,7 +12,6 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o
 obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
 obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
-obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_RESET_STM32) += reset-stm32.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index 08fc79d79e86d..e553d434ca362 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -68,25 +68,49 @@ static int reset_simple_deassert(struct reset_controller_dev *rcdev,
 	return reset_simple_set(rcdev, id, false);
 }
 
+static int reset_simple_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 reg;
+
+	reg = readl(data->membase + (bank * reg_width));
+
+	return !(reg & BIT(offset)) ^ !data->status_inverted;
+}
+
 const struct reset_control_ops reset_simple_ops = {
 	.assert		= reset_simple_assert,
 	.deassert	= reset_simple_deassert,
+	.status		= reset_simple_status,
 };
 
 struct reset_simple_devdata {
 	bool inverted;
+	bool status_inverted;
+};
+
+static const struct reset_simple_devdata reset_simple_socfpga = {
+	.status_inverted = true,
 };
 
 static const struct reset_simple_devdata reset_simple_inverted = {
 	.inverted = true,
+	.status_inverted = true,
 };
 
 static const struct of_device_id reset_simple_dt_ids[] = {
+	{ .compatible = "altr,rst-mgr", .data = &reset_simple_socfpga },
 	{ .compatible = "allwinner,sun6i-a31-clock-reset",
 		.data = &reset_simple_inverted },
 	{ /* sentinel */ },
 };
 
+#define SOCFPGA_NR_BANKS		8
+
 static int reset_simple_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -113,8 +137,33 @@ static int reset_simple_probe(struct platform_device *pdev)
 	data->rcdev.ops = &reset_simple_ops;
 	data->rcdev.of_node = dev->of_node;
 
-	if (devdata)
+	if (devdata == &reset_simple_socfpga) {
+		u32 modrst_offset;
+
+		/*
+		 * The binding was mainlined without the required property.
+		 * Do not continue, when we encounter an old DT.
+		 */
+		if (!of_find_property(dev->of_node, "#reset-cells", NULL)) {
+			dev_err(dev, "%pOF missing #reset-cells property\n",
+				dev->of_node);
+			return -EINVAL;
+		}
+
+		if (of_property_read_u32(dev->of_node, "altr,modrst-offset",
+					 &modrst_offset)) {
+			dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
+			modrst_offset = 0x10;
+		}
+
+		data->membase += modrst_offset;
+		data->rcdev.nr_resets = SOCFPGA_NR_BANKS * 32;
+	}
+
+	if (devdata) {
 		data->inverted = devdata->inverted;
+		data->status_inverted = devdata->status_inverted;
+	}
 
 	return devm_reset_controller_register(dev, &data->rcdev);
 }
diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
index a26dc62b2f349..52dc28d4ffac3 100644
--- a/drivers/reset/reset-simple.h
+++ b/drivers/reset/reset-simple.h
@@ -25,6 +25,7 @@ struct reset_simple_data {
 	void __iomem			*membase;
 	struct reset_controller_dev	rcdev;
 	bool				inverted;
+	bool				status_inverted;
 };
 
 extern const struct reset_control_ops reset_simple_ops;
diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
deleted file mode 100644
index 07224c0198920..0000000000000
--- a/drivers/reset/reset-socfpga.c
+++ /dev/null
@@ -1,154 +0,0 @@
-/*
- * Socfpga Reset Controller Driver
- *
- * Copyright 2014 Steffen Trumtrar <s.trumtrar@pengutronix.de>
- *
- * based on
- * Allwinner SoCs Reset Controller driver
- *
- * Copyright 2013 Maxime Ripard
- *
- * Maxime Ripard <maxime.ripard@free-electrons.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#include <linux/err.h>
-#include <linux/io.h>
-#include <linux/init.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>
-#include <linux/reset-controller.h>
-#include <linux/spinlock.h>
-#include <linux/types.h>
-
-#define BANK_INCREMENT		4
-#define NR_BANKS		8
-
-struct socfpga_reset_data {
-	spinlock_t			lock;
-	void __iomem			*membase;
-	struct reset_controller_dev	rcdev;
-};
-
-static int socfpga_reset_assert(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	struct socfpga_reset_data *data = container_of(rcdev,
-						     struct socfpga_reset_data,
-						     rcdev);
-	int bank = id / BITS_PER_LONG;
-	int offset = id % BITS_PER_LONG;
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * BANK_INCREMENT));
-	writel(reg | BIT(offset), data->membase + (bank * BANK_INCREMENT));
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static int socfpga_reset_deassert(struct reset_controller_dev *rcdev,
-				  unsigned long id)
-{
-	struct socfpga_reset_data *data = container_of(rcdev,
-						     struct socfpga_reset_data,
-						     rcdev);
-
-	int bank = id / BITS_PER_LONG;
-	int offset = id % BITS_PER_LONG;
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * BANK_INCREMENT));
-	writel(reg & ~BIT(offset), data->membase + (bank * BANK_INCREMENT));
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static int socfpga_reset_status(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	struct socfpga_reset_data *data = container_of(rcdev,
-						struct socfpga_reset_data, rcdev);
-	int bank = id / BITS_PER_LONG;
-	int offset = id % BITS_PER_LONG;
-	u32 reg;
-
-	reg = readl(data->membase + (bank * BANK_INCREMENT));
-
-	return !(reg & BIT(offset));
-}
-
-static const struct reset_control_ops socfpga_reset_ops = {
-	.assert		= socfpga_reset_assert,
-	.deassert	= socfpga_reset_deassert,
-	.status		= socfpga_reset_status,
-};
-
-static int socfpga_reset_probe(struct platform_device *pdev)
-{
-	struct socfpga_reset_data *data;
-	struct resource *res;
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	u32 modrst_offset;
-
-	/*
-	 * The binding was mainlined without the required property.
-	 * Do not continue, when we encounter an old DT.
-	 */
-	if (!of_find_property(pdev->dev.of_node, "#reset-cells", NULL)) {
-		dev_err(&pdev->dev, "%s missing #reset-cells property\n",
-			pdev->dev.of_node->full_name);
-		return -EINVAL;
-	}
-
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(data->membase))
-		return PTR_ERR(data->membase);
-
-	if (of_property_read_u32(np, "altr,modrst-offset", &modrst_offset)) {
-		dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
-		modrst_offset = 0x10;
-	}
-	data->membase += modrst_offset;
-
-	spin_lock_init(&data->lock);
-
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = NR_BANKS * BITS_PER_LONG;
-	data->rcdev.ops = &socfpga_reset_ops;
-	data->rcdev.of_node = pdev->dev.of_node;
-
-	return devm_reset_controller_register(dev, &data->rcdev);
-}
-
-static const struct of_device_id socfpga_reset_dt_ids[] = {
-	{ .compatible = "altr,rst-mgr", },
-	{ /* sentinel */ },
-};
-
-static struct platform_driver socfpga_reset_driver = {
-	.probe	= socfpga_reset_probe,
-	.driver = {
-		.name		= "socfpga-reset",
-		.of_match_table	= socfpga_reset_dt_ids,
-	},
-};
-builtin_platform_driver(socfpga_reset_driver);
-- 
2.11.0

[PATCH v2 2/5] reset: socfpga: use the reset-simple driver

[Alexandru Gagniuc]

Hi Phillip,

On 08/11/2017 06:06 AM, Philipp Zabel wrote:
[snip]

> @@ -113,8 +137,33 @@ static int reset_simple_probe(struct platform_device *pdev)
>  	data->rcdev.ops = &reset_simple_ops;
>  	data->rcdev.of_node = dev->of_node;
>
> -	if (devdata)
> +	if (devdata == &reset_simple_socfpga) {

This sort of special-case handling opens the gate to adding special-case 
handling for any new device, which somewhat defeats the purpose of a 
generic driver.

> +		u32 modrst_offset;
> +
> +		/*
> +		 * The binding was mainlined without the required property.
> +		 * Do not continue, when we encounter an old DT.
> +		 */
> +		if (!of_find_property(dev->of_node, "#reset-cells", NULL)) {
> +			dev_err(dev, "%pOF missing #reset-cells property\n",
> +				dev->of_node);
> +			return -EINVAL;
> +		}

Is the check still required in this context, since 
(devm_)reset_control_get() complains really loudly if #reset-cells is 
missing?

> +
> +		if (of_property_read_u32(dev->of_node, "altr,modrst-offset",
> +					 &modrst_offset)) {
> +			dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
> +			modrst_offset = 0x10;
> +		}

This can be a generic "reg-offset" property. If and until the 
devicetrees are updated, "altr,modrst-offset" would also have to be read 
as an alternative. Since a platform data already exists, 
reset_simple_devdata could be a better place to store the default offset 
of 0x10 for socfpga.

Alex

[PATCH v2 2/5] reset: socfpga: use the reset-simple driver

[Philipp Zabel]

Hi Alexandru,

thank you for the comments.

On Fri, 2017-08-11 at 09:28 -0700, Alexandru Gagniuc wrote:
> Hi Phillip,
> 
> On 08/11/2017 06:06 AM, Philipp Zabel wrote:
> [snip]
> 
> > @@ -113,8 +137,33 @@ static int reset_simple_probe(struct platform_device *pdev)
> > > > ?	data->rcdev.ops = &reset_simple_ops;
> > > > ?	data->rcdev.of_node = dev->of_node;
> > 
> > > > -	if (devdata)
> > +	if (devdata == &reset_simple_socfpga) {
> 
> This sort of special-case handling opens the gate to adding special-case?
> handling for any new device, which somewhat defeats the purpose of a?
> generic driver.

That is why I initially only unified the reset ops and not the whole
platorm driver.

I could partially revert this latest change and keep the reset-socfpga
driver, just reusing the reset-simple ops to avoid potential "quirk
creep" in the reset-simple driver.

Or I could drop the #reset-cells warning below and just read the
altr,modrst-offset property for any reset controller, to avoid the
special-case.

> > +		u32 modrst_offset;
> > +
> > > > +		/*
> > > > +		?* The binding was mainlined without the required property.
> > > > +		?* Do not continue, when we encounter an old DT.
> > > > +		?*/
> > > > +		if (!of_find_property(dev->of_node, "#reset-cells", NULL)) {
> > > > +			dev_err(dev, "%pOF missing #reset-cells property\n",
> > > > +				dev->of_node);
> > > > +			return -EINVAL;
> > > > +		}
> 
> Is the check still required in this context, since?
> (devm_)reset_control_get() complains really loudly if #reset-cells is?
> missing?

I'm fine with removing this. Ideally, this is something that the device
tree compiler should check already.

> > +
> > > > +		if (of_property_read_u32(dev->of_node, "altr,modrst-offset",
> > > > +					?&modrst_offset)) {
> > > > +			dev_warn(dev, "missing altr,modrst-offset property, assuming 0x10!\n");
> > > > +			modrst_offset = 0x10;
> > +		}
> 
> This can be a generic "reg-offset" property. If and until the?
> devicetrees are updated, "altr,modrst-offset" would also have to be read?
> as an alternative.

Possibly. Adding new device tree bindings is a separate discussion.

> Since a platform data already exists,?reset_simple_devdata could be a
> better place to store the default offset?of 0x10 for socfpga.

I'll change this as you suggest.

regards
Philipp

[PATCH v2 3/5] reset: stm32: use the reset-simple driver

[Philipp Zabel]

The reset-simple driver can be used without changes.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/Kconfig        |  11 ++---
 drivers/reset/Makefile       |   1 -
 drivers/reset/reset-simple.c |   1 +
 drivers/reset/reset-stm32.c  | 108 -------------------------------------------
 4 files changed, 4 insertions(+), 117 deletions(-)
 delete mode 100644 drivers/reset/reset-stm32.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 78a8f6057985b..29f4487c290fc 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -70,19 +70,14 @@ config RESET_PISTACHIO
 
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
-	default ARCH_SOCFPGA || ARCH_SUNXI
+	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_SUNXI
 	help
 	  This enables a simple reset controller driver for reset lines that
 	  that can be asserted and deasserted by toggling bits in a contiguous,
 	  exclusive register space.
 
-	  Currently this driver supports Altera SoCFPGAs and Allwinner SoCs.
-
-config RESET_STM32
-	bool "STM32 Reset Driver" if COMPILE_TEST
-	default ARCH_STM32
-	help
-	  This enables the RCC reset controller driver for STM32 MCUs.
+	  Currently this driver supports Altera SoCFPGAs, the RCC reset
+	  controller in STM32 MCUs, and Allwinner SoCs.
 
 config RESET_SUNXI
 	bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 25f5f722dec01..e8c3a032f4780 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -12,7 +12,6 @@ obj-$(CONFIG_RESET_MESON) += reset-meson.o
 obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o
 obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
-obj-$(CONFIG_RESET_STM32) += reset-stm32.o
 obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
 obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index e553d434ca362..8166aafc42478 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -104,6 +104,7 @@ static const struct reset_simple_devdata reset_simple_inverted = {
 
 static const struct of_device_id reset_simple_dt_ids[] = {
 	{ .compatible = "altr,rst-mgr", .data = &reset_simple_socfpga },
+	{ .compatible = "st,stm32-rcc", },
 	{ .compatible = "allwinner,sun6i-a31-clock-reset",
 		.data = &reset_simple_inverted },
 	{ /* sentinel */ },
diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c
deleted file mode 100644
index 3a7c8527e66af..0000000000000
--- a/drivers/reset/reset-stm32.c
+++ /dev/null
@@ -1,108 +0,0 @@
-/*
- * Copyright (C) Maxime Coquelin 2015
- * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
- * License terms:  GNU General Public License (GPL), version 2
- *
- * Heavily based on sunxi driver from Maxime Ripard.
- */
-
-#include <linux/err.h>
-#include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
-#include <linux/reset-controller.h>
-#include <linux/slab.h>
-#include <linux/spinlock.h>
-#include <linux/types.h>
-
-struct stm32_reset_data {
-	spinlock_t			lock;
-	void __iomem			*membase;
-	struct reset_controller_dev	rcdev;
-};
-
-static int stm32_reset_assert(struct reset_controller_dev *rcdev,
-			      unsigned long id)
-{
-	struct stm32_reset_data *data = container_of(rcdev,
-						     struct stm32_reset_data,
-						     rcdev);
-	int bank = id / BITS_PER_LONG;
-	int offset = id % BITS_PER_LONG;
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * 4));
-	writel(reg | BIT(offset), data->membase + (bank * 4));
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static int stm32_reset_deassert(struct reset_controller_dev *rcdev,
-				unsigned long id)
-{
-	struct stm32_reset_data *data = container_of(rcdev,
-						     struct stm32_reset_data,
-						     rcdev);
-	int bank = id / BITS_PER_LONG;
-	int offset = id % BITS_PER_LONG;
-	unsigned long flags;
-	u32 reg;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	reg = readl(data->membase + (bank * 4));
-	writel(reg & ~BIT(offset), data->membase + (bank * 4));
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
-static const struct reset_control_ops stm32_reset_ops = {
-	.assert		= stm32_reset_assert,
-	.deassert	= stm32_reset_deassert,
-};
-
-static const struct of_device_id stm32_reset_dt_ids[] = {
-	 { .compatible = "st,stm32-rcc", },
-	 { /* sentinel */ },
-};
-
-static int stm32_reset_probe(struct platform_device *pdev)
-{
-	struct stm32_reset_data *data;
-	struct resource *res;
-
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data->membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(data->membase))
-		return PTR_ERR(data->membase);
-
-	spin_lock_init(&data->lock);
-
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = resource_size(res) * 8;
-	data->rcdev.ops = &stm32_reset_ops;
-	data->rcdev.of_node = pdev->dev.of_node;
-
-	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
-}
-
-static struct platform_driver stm32_reset_driver = {
-	.probe	= stm32_reset_probe,
-	.driver = {
-		.name		= "stm32-rcc-reset",
-		.of_match_table	= stm32_reset_dt_ids,
-	},
-};
-builtin_platform_driver(stm32_reset_driver);
-- 
2.11.0

[PATCH v2 4/5] reset: zx2967: use the reset-simple driver

[Philipp Zabel]

The reset-simple driver can be used without changes.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 MAINTAINERS                  |  1 -
 drivers/reset/Kconfig        | 10 +----
 drivers/reset/Makefile       |  1 -
 drivers/reset/reset-simple.c |  1 +
 drivers/reset/reset-zx2967.c | 99 --------------------------------------------
 5 files changed, 3 insertions(+), 109 deletions(-)
 delete mode 100644 drivers/reset/reset-zx2967.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 57853844969bf..30af04516d8a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2061,7 +2061,6 @@ L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	arch/arm/mach-zx/
 F:	drivers/clk/zte/
-F:	drivers/reset/reset-zx2967.c
 F:	drivers/soc/zte/
 F:	Documentation/devicetree/bindings/arm/zte.txt
 F:	Documentation/devicetree/bindings/clock/zx296702-clk.txt
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 29f4487c290fc..6dfea01023618 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -70,14 +70,14 @@ config RESET_PISTACHIO
 
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
-	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_SUNXI
+	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_SUNXI || ARCH_ZX
 	help
 	  This enables a simple reset controller driver for reset lines that
 	  that can be asserted and deasserted by toggling bits in a contiguous,
 	  exclusive register space.
 
 	  Currently this driver supports Altera SoCFPGAs, the RCC reset
-	  controller in STM32 MCUs, and Allwinner SoCs.
+	  controller in STM32 MCUs, Allwinner SoCs, and ZTE's zx2967 family.
 
 config RESET_SUNXI
 	bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
@@ -114,12 +114,6 @@ config RESET_UNIPHIER
 	  Say Y if you want to control reset signals provided by System Control
 	  block, Media I/O block, Peripheral Block.
 
-config RESET_ZX2967
-	bool "ZTE ZX2967 Reset Driver"
-	depends on ARCH_ZX || COMPILE_TEST
-	help
-	  This enables the reset controller driver for ZTE's zx2967 family.
-
 config RESET_ZYNQ
 	bool "ZYNQ Reset Driver" if COMPILE_TEST
 	default ARCH_ZYNQ
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index e8c3a032f4780..3a70c254b5ea5 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -16,6 +16,5 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
 obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
-obj-$(CONFIG_RESET_ZX2967) += reset-zx2967.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
 
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index 8166aafc42478..d2d6ac483d61d 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -107,6 +107,7 @@ static const struct of_device_id reset_simple_dt_ids[] = {
 	{ .compatible = "st,stm32-rcc", },
 	{ .compatible = "allwinner,sun6i-a31-clock-reset",
 		.data = &reset_simple_inverted },
+	{ .compatible = "zte,zx296718-reset", .data = &reset_simple_inverted },
 	{ /* sentinel */ },
 };
 
diff --git a/drivers/reset/reset-zx2967.c b/drivers/reset/reset-zx2967.c
deleted file mode 100644
index 4f319f7753d4d..0000000000000
--- a/drivers/reset/reset-zx2967.c
+++ /dev/null
@@ -1,99 +0,0 @@
-/*
- * ZTE's zx2967 family reset controller driver
- *
- * Copyright (C) 2017 ZTE Ltd.
- *
- * Author: Baoyou Xie <baoyou.xie@linaro.org>
- *
- * License terms: GNU General Public License (GPL) version 2
- */
-
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
-#include <linux/reset-controller.h>
-
-struct zx2967_reset {
-	void __iomem			*reg_base;
-	spinlock_t			lock;
-	struct reset_controller_dev	rcdev;
-};
-
-static int zx2967_reset_act(struct reset_controller_dev *rcdev,
-			    unsigned long id, bool assert)
-{
-	struct zx2967_reset *reset = NULL;
-	int bank = id / 32;
-	int offset = id % 32;
-	u32 reg;
-	unsigned long flags;
-
-	reset = container_of(rcdev, struct zx2967_reset, rcdev);
-
-	spin_lock_irqsave(&reset->lock, flags);
-
-	reg = readl_relaxed(reset->reg_base + (bank * 4));
-	if (assert)
-		reg &= ~BIT(offset);
-	else
-		reg |= BIT(offset);
-	writel_relaxed(reg, reset->reg_base + (bank * 4));
-
-	spin_unlock_irqrestore(&reset->lock, flags);
-
-	return 0;
-}
-
-static int zx2967_reset_assert(struct reset_controller_dev *rcdev,
-			       unsigned long id)
-{
-	return zx2967_reset_act(rcdev, id, true);
-}
-
-static int zx2967_reset_deassert(struct reset_controller_dev *rcdev,
-				 unsigned long id)
-{
-	return zx2967_reset_act(rcdev, id, false);
-}
-
-static const struct reset_control_ops zx2967_reset_ops = {
-	.assert		= zx2967_reset_assert,
-	.deassert	= zx2967_reset_deassert,
-};
-
-static int zx2967_reset_probe(struct platform_device *pdev)
-{
-	struct zx2967_reset *reset;
-	struct resource *res;
-
-	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
-	if (!reset)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	reset->reg_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(reset->reg_base))
-		return PTR_ERR(reset->reg_base);
-
-	spin_lock_init(&reset->lock);
-
-	reset->rcdev.owner = THIS_MODULE;
-	reset->rcdev.nr_resets = resource_size(res) * 8;
-	reset->rcdev.ops = &zx2967_reset_ops;
-	reset->rcdev.of_node = pdev->dev.of_node;
-
-	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
-}
-
-static const struct of_device_id zx2967_reset_dt_ids[] = {
-	 { .compatible = "zte,zx296718-reset", },
-	 {},
-};
-
-static struct platform_driver zx2967_reset_driver = {
-	.probe	= zx2967_reset_probe,
-	.driver = {
-		.name		= "zx2967-reset",
-		.of_match_table	= zx2967_reset_dt_ids,
-	},
-};
-builtin_platform_driver(zx2967_reset_driver);
-- 
2.11.0

[PATCH v2 5/5] reset: simple: read back to make sure changes are applied

[Philipp Zabel]

Read back the register after setting or clearing a reset bit to make
sure that the changes are applied to the reset controller hardware.
Theoretically, this avoids the write to stay stuck in a store buffer
during the delay of an assert-delay-deassert sequence, and makes sure
that the reset really is asserted for the specified duration.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Rebased to only modify the reset_simple_set helper function.
---
 drivers/reset/reset-simple.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index d2d6ac483d61d..1324ba45e138b 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -39,17 +39,20 @@ static int reset_simple_set(struct reset_controller_dev *rcdev,
 	int reg_width = sizeof(u32);
 	int bank = id / (reg_width * BITS_PER_BYTE);
 	int offset = id % (reg_width * BITS_PER_BYTE);
+	void __iomem *addr = data->membase + (bank * reg_width);
 	unsigned long flags;
 	u32 reg;
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	reg = readl(data->membase + (bank * reg_width));
+	reg = readl(addr);
 	if (assert ^ data->inverted)
 		reg |= BIT(offset);
 	else
 		reg &= ~BIT(offset);
-	writel(reg, data->membase + (bank * reg_width));
+	writel(reg, addr);
+	/* Read back to make sure the write doesn't linger in a store buffer */
+	readl(addr);
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
-- 
2.11.0