[PATCH v3 0/5] Unify simple reset drivers

[PATCH v3 0/5] Unify simple reset drivers

[Philipp Zabel]

Hi,

This series introduces common reset ops 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.

Parts of the sunxi driver and the socfpga, stm32, and zx2967 drivers are
merged into a single reset-simple driver.
The sunxi driver is kept around to register the early reset controllers,
but it reuses the exported reset_simple_ops.

The first patch depends on [1] or commit 726cc791c9b9 ("reset: sunxi: fix
number of reset lines") in linux-next. On linux-next, the second patch will
trivially conflict with commit 7799167b7a14 ("regulator: Convert to using
%pOF instead of full_name").

Changes since v2 [2]:
 - Use of_device_get_match_data instead of of_match_device,
   add kerneldoc comment for struct reset_simple_devdata and struct
   reset_simple_data, suggested by Chen-Yu.
 - Add reg_offset and nr_reset override to struct reset_simple_devdata,
   instead of special casing socfpga, always check "altr,modrst-offset"
   device tree property if a default reg_offset is set, based on
   Alexandru's advice.
 - Rename "*inverted" properties to "*active_low".

[1] https://patchwork.kernel.org/patch/9895433/
[2] https://patchwork.kernel.org/patch/9895833/
    https://patchwork.kernel.org/patch/9895819/
    https://patchwork.kernel.org/patch/9895831/
    https://patchwork.kernel.org/patch/9895815/
    https://patchwork.kernel.org/patch/9895841/

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  | 188 ++++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-simple.h  |  45 ++++++++++
 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, 249 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 v3 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 v2:
 - Add kerneldoc comment for struct reset_simple_devdata and struct
   reset_simple_data.
 - Rename "inverted" properties to "active_low".
 - Use of_device_get_match_data instead of of_match_device.
---
 drivers/reset/Kconfig        |  11 ++++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-simple.c | 134 +++++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-simple.h |  41 +++++++++++++
 drivers/reset/reset-sunxi.c  | 104 ++-------------------------------
 5 files changed, 193 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..70c57c0758c39
--- /dev/null
+++ b/drivers/reset/reset-simple.c
@@ -0,0 +1,134 @@
+/*
+ * 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->active_low)
+		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 - simple reset controller properties
+ * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
+ *              are set to assert the reset.
+ */
+struct reset_simple_devdata {
+	bool active_low;
+};
+
+static const struct reset_simple_devdata reset_simple_active_low = {
+	.active_low = true,
+};
+
+static const struct of_device_id reset_simple_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-clock-reset",
+		.data = &reset_simple_active_low },
+	{ /* sentinel */ },
+};
+
+static int reset_simple_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct reset_simple_devdata *devdata;
+	struct reset_simple_data *data;
+	void __iomem *membase;
+	struct resource *res;
+
+	devdata = of_device_get_match_data(dev);
+
+	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->active_low = devdata->active_low;
+
+	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..39af2014b5f12
--- /dev/null
+++ b/drivers/reset/reset-simple.h
@@ -0,0 +1,41 @@
+/*
+ * 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 - driver data for simple reset controllers
+ * @lock: spinlock to protect registers during read-modify-write cycles
+ * @membase: memory mapped I/O register range
+ * @rcdev: reset controller device base structure
+ * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
+ *              are set to assert the reset. Note that this says nothing about
+ *              the voltage level of the actual reset line.
+ */
+struct reset_simple_data {
+	spinlock_t			lock;
+	void __iomem			*membase;
+	struct reset_controller_dev	rcdev;
+	bool				active_low;
+};
+
+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..db9a1a75523f4 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->active_low = 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 v3 1/5] reset: add reset-simple to unify socfpga, stm32, sunxi, and zx2967

[Andre Przywara]

Hi Philipp,

sorry for the delay, I was on holidays.
Thanks for putting together the series, this looks very good to me.

One comment below...

On 16/08/17 10:46, 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 v2:
>  - Add kerneldoc comment for struct reset_simple_devdata and struct
>    reset_simple_data.
>  - Rename "inverted" properties to "active_low".
>  - Use of_device_get_match_data instead of of_match_device.
> ---
>  drivers/reset/Kconfig        |  11 ++++
>  drivers/reset/Makefile       |   1 +
>  drivers/reset/reset-simple.c | 134 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/reset/reset-simple.h |  41 +++++++++++++
>  drivers/reset/reset-sunxi.c  | 104 ++-------------------------------
>  5 files changed, 193 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..70c57c0758c39
> --- /dev/null
> +++ b/drivers/reset/reset-simple.c
> @@ -0,0 +1,134 @@
> +/*
> + * 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->active_low)
> +		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 - simple reset controller properties
> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
> + *              are set to assert the reset.
> + */
> +struct reset_simple_devdata {
> +	bool active_low;
> +};
> +
> +static const struct reset_simple_devdata reset_simple_active_low = {
> +	.active_low = true,
> +};
> +
> +static const struct of_device_id reset_simple_dt_ids[] = {
> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
> +		.data = &reset_simple_active_low },

Can we have a additional generic compatible string here? New users of
this driver then wouldn't need to explicitly enter their name into the
driver, but could just use the generic name as a fallback. This would
enable the driver without any Linux code change just by adding a DT node.

	compatible = "nexell,s5p6818-reset", "simple-reset";

Whenever we need a quirk (now or in the future), we can add the specific
name into this structure along with the required workarounds.

Cheers,
Andre.

> +	{ /* sentinel */ },
> +};
> +
> +static int reset_simple_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct reset_simple_devdata *devdata;
> +	struct reset_simple_data *data;
> +	void __iomem *membase;
> +	struct resource *res;
> +
> +	devdata = of_device_get_match_data(dev);
> +
> +	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->active_low = devdata->active_low;
> +
> +	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..39af2014b5f12
> --- /dev/null
> +++ b/drivers/reset/reset-simple.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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 - driver data for simple reset controllers
> + * @lock: spinlock to protect registers during read-modify-write cycles
> + * @membase: memory mapped I/O register range
> + * @rcdev: reset controller device base structure
> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
> + *              are set to assert the reset. Note that this says nothing about
> + *              the voltage level of the actual reset line.
> + */
> +struct reset_simple_data {
> +	spinlock_t			lock;
> +	void __iomem			*membase;
> +	struct reset_controller_dev	rcdev;
> +	bool				active_low;
> +};
> +
> +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..db9a1a75523f4 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->active_low = 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);
> 

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

[Andreas Färber]

Hi Andre,

Am 16.08.2017 um 13:30 schrieb Andre Przywara:
> On 16/08/17 10:46, Philipp Zabel wrote:
>> +/**
>> + * struct reset_simple_devdata - simple reset controller properties
>> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
>> + *              are set to assert the reset.
>> + */
>> +struct reset_simple_devdata {
>> +	bool active_low;
>> +};
>> +
>> +static const struct reset_simple_devdata reset_simple_active_low = {
>> +	.active_low = true,
>> +};
>> +
>> +static const struct of_device_id reset_simple_dt_ids[] = {
>> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
>> +		.data = &reset_simple_active_low },
> 
> Can we have a additional generic compatible string here? New users of
> this driver then wouldn't need to explicitly enter their name into the
> driver, but could just use the generic name as a fallback. This would
> enable the driver without any Linux code change just by adding a DT node.
> 
> 	compatible = "nexell,s5p6818-reset", "simple-reset";
> 
> Whenever we need a quirk (now or in the future), we can add the specific
> name into this structure along with the required workarounds.

Same question about binding here. However the way it is done today, we
would also need some optional active-low property then or two different
compatible strings, as this is currently controlled via the DT matches.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

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

[Philipp Zabel]

On Wed, 2017-08-16 at 14:12 +0200, Andreas F?rber wrote:
> Hi Andre,
> 
> Am 16.08.2017 um 13:30 schrieb Andre Przywara:
> > On 16/08/17 10:46, Philipp Zabel wrote:
> > > +/**
> > > + * struct reset_simple_devdata - simple reset controller properties
> > > + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
> > > + *??????????????are set to assert the reset.
> > > + */
> > > +struct reset_simple_devdata {
> > > > > > +	bool active_low;
> > > +};
> > > +
> > > +static const struct reset_simple_devdata reset_simple_active_low = {
> > > > > > +	.active_low = true,
> > > +};
> > > +
> > > +static const struct of_device_id reset_simple_dt_ids[] = {
> > > > > > +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
> > > +		.data = &reset_simple_active_low },
> > 
> > Can we have a additional generic compatible string here? New users of
> > this driver then wouldn't need to explicitly enter their name into the
> > driver, but could just use the generic name as a fallback. This would
> > enable the driver without any Linux code change just by adding a DT node.
> > 
> > 	compatible = "nexell,s5p6818-reset", "simple-reset";
> > 
> > Whenever we need a quirk (now or in the future), we can add the specific
> > name into this structure along with the required workarounds.
> 
> Same question about binding here. However the way it is done today, we
> would also need some optional active-low property then or two different
> compatible strings, as this is currently controlled via the DT matches.

I'd like to decouple this from the issue at hand, which is de-
duplicating simple reset code without device tree changes.

I'll make a separate suggestion for a simple binding on top of this
series.

regards
Philipp

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

[Andre Przywara]

Hi,

On 16/08/17 16:11, Philipp Zabel wrote:
> On Wed, 2017-08-16 at 14:12 +0200, Andreas F?rber wrote:
>> Hi Andre,
>>
>> Am 16.08.2017 um 13:30 schrieb Andre Przywara:
>>> On 16/08/17 10:46, Philipp Zabel wrote:
>>>> +/**
>>>> + * struct reset_simple_devdata - simple reset controller properties
>>>> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
>>>> + *              are set to assert the reset.
>>>> + */
>>>> +struct reset_simple_devdata {
>>>>>>> +	bool active_low;
>>>> +};
>>>> +
>>>> +static const struct reset_simple_devdata reset_simple_active_low = {
>>>>>>> +	.active_low = true,
>>>> +};
>>>> +
>>>> +static const struct of_device_id reset_simple_dt_ids[] = {
>>>>>>> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
>>>> +		.data = &reset_simple_active_low },
>>>
>>> Can we have a additional generic compatible string here? New users of
>>> this driver then wouldn't need to explicitly enter their name into the
>>> driver, but could just use the generic name as a fallback. This would
>>> enable the driver without any Linux code change just by adding a DT node.
>>>
>>> 	compatible = "nexell,s5p6818-reset", "simple-reset";
>>>
>>> Whenever we need a quirk (now or in the future), we can add the specific
>>> name into this structure along with the required workarounds.
>>
>> Same question about binding here. However the way it is done today, we
>> would also need some optional active-low property then or two different
>> compatible strings, as this is currently controlled via the DT matches.
> 
> I'd like to decouple this from the issue at hand, which is de-
> duplicating simple reset code without device tree changes.

Agreed, this is an orthogonal issue, actually being enabled by this series.

> I'll make a separate suggestion for a simple binding on top of this
> series.

Thanks! Happy to review it.
I actually have a user at hand already ...

Cheers,
Andre.

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

[Andreas Färber]

Am 16.08.2017 um 17:11 schrieb Philipp Zabel:
> On Wed, 2017-08-16 at 14:12 +0200, Andreas F?rber wrote:
>> Am 16.08.2017 um 13:30 schrieb Andre Przywara:
>>> On 16/08/17 10:46, Philipp Zabel wrote:
>>>> +/**
>>>> + * struct reset_simple_devdata - simple reset controller properties
>>>> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
>>>> + *              are set to assert the reset.
>>>> + */
>>>> +struct reset_simple_devdata {
>>>>>>> +	bool active_low;
>>>> +};
>>>> +
>>>> +static const struct reset_simple_devdata reset_simple_active_low = {
>>>>>>> +	.active_low = true,
>>>> +};
>>>> +
>>>> +static const struct of_device_id reset_simple_dt_ids[] = {
>>>>>>> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
>>>> +		.data = &reset_simple_active_low },
>>>
>>> Can we have a additional generic compatible string here? New users of
>>> this driver then wouldn't need to explicitly enter their name into the
>>> driver, but could just use the generic name as a fallback. This would
>>> enable the driver without any Linux code change just by adding a DT node.
>>>
>>> 	compatible = "nexell,s5p6818-reset", "simple-reset";
>>>
>>> Whenever we need a quirk (now or in the future), we can add the specific
>>> name into this structure along with the required workarounds.
>>
>> Same question about binding here. However the way it is done today, we
>> would also need some optional active-low property then or two different
>> compatible strings, as this is currently controlled via the DT matches.
> 
> I'd like to decouple this from the issue at hand, which is de-
> duplicating simple reset code without device tree changes.
> 
> I'll make a separate suggestion for a simple binding on top of this
> series.

Okay, I'll continue using my own driver then, until it's clear how
exactly I'm supposed to piggy-back on this new driver.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

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

[Andre Przywara]

Hi,

On 16/08/17 17:41, Andreas F?rber wrote:
> Am 16.08.2017 um 17:11 schrieb Philipp Zabel:
>> On Wed, 2017-08-16 at 14:12 +0200, Andreas F?rber wrote:
>>> Am 16.08.2017 um 13:30 schrieb Andre Przywara:
>>>> On 16/08/17 10:46, Philipp Zabel wrote:
>>>>> +/**
>>>>> + * struct reset_simple_devdata - simple reset controller properties
>>>>> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
>>>>> + *              are set to assert the reset.
>>>>> + */
>>>>> +struct reset_simple_devdata {
>>>>>>>> +	bool active_low;
>>>>> +};
>>>>> +
>>>>> +static const struct reset_simple_devdata reset_simple_active_low = {
>>>>>>>> +	.active_low = true,
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id reset_simple_dt_ids[] = {
>>>>>>>> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
>>>>> +		.data = &reset_simple_active_low },
>>>>
>>>> Can we have a additional generic compatible string here? New users of
>>>> this driver then wouldn't need to explicitly enter their name into the
>>>> driver, but could just use the generic name as a fallback. This would
>>>> enable the driver without any Linux code change just by adding a DT node.
>>>>
>>>> 	compatible = "nexell,s5p6818-reset", "simple-reset";
>>>>
>>>> Whenever we need a quirk (now or in the future), we can add the specific
>>>> name into this structure along with the required workarounds.
>>>
>>> Same question about binding here. However the way it is done today, we
>>> would also need some optional active-low property then or two different
>>> compatible strings, as this is currently controlled via the DT matches.
>>
>> I'd like to decouple this from the issue at hand, which is de-
>> duplicating simple reset code without device tree changes.
>>
>> I'll make a separate suggestion for a simple binding on top of this
>> series.
> 
> Okay, I'll continue using my own driver then, until it's clear how
> exactly I'm supposed to piggy-back on this new driver.

It's not very nice, but if you can't wait, what about using:

	compatible = "your_vendor,your-reset",
		     "allwinner,sun6i-a31-clock-reset";

for now, then later adding the new generic compatible string at the end,
once we agreed on the naming?
That should give you support for all ranges of kernels.

Cheers,
Andre.

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

[Alexandru Gagniuc]

On 08/16/2017 02:46 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>

Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>

> ---
> Changes since v2:
>  - Add kerneldoc comment for struct reset_simple_devdata and struct
>    reset_simple_data.
>  - Rename "inverted" properties to "active_low".
>  - Use of_device_get_match_data instead of of_match_device.
> ---
>  drivers/reset/Kconfig        |  11 ++++
>  drivers/reset/Makefile       |   1 +
>  drivers/reset/reset-simple.c | 134 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/reset/reset-simple.h |  41 +++++++++++++
>  drivers/reset/reset-sunxi.c  | 104 ++-------------------------------
>  5 files changed, 193 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..70c57c0758c39
> --- /dev/null
> +++ b/drivers/reset/reset-simple.c
> @@ -0,0 +1,134 @@
> +/*
> + * 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->active_low)
> +		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 - simple reset controller properties
> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
> + *              are set to assert the reset.
> + */
> +struct reset_simple_devdata {
> +	bool active_low;
> +};
> +
> +static const struct reset_simple_devdata reset_simple_active_low = {
> +	.active_low = true,
> +};
> +
> +static const struct of_device_id reset_simple_dt_ids[] = {
> +	{ .compatible = "allwinner,sun6i-a31-clock-reset",
> +		.data = &reset_simple_active_low },
> +	{ /* sentinel */ },
> +};
> +
> +static int reset_simple_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct reset_simple_devdata *devdata;
> +	struct reset_simple_data *data;
> +	void __iomem *membase;
> +	struct resource *res;
> +
> +	devdata = of_device_get_match_data(dev);
> +
> +	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->active_low = devdata->active_low;
> +
> +	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..39af2014b5f12
> --- /dev/null
> +++ b/drivers/reset/reset-simple.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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 - driver data for simple reset controllers
> + * @lock: spinlock to protect registers during read-modify-write cycles
> + * @membase: memory mapped I/O register range
> + * @rcdev: reset controller device base structure
> + * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
> + *              are set to assert the reset. Note that this says nothing about
> + *              the voltage level of the actual reset line.
> + */
> +struct reset_simple_data {
> +	spinlock_t			lock;
> +	void __iomem			*membase;
> +	struct reset_controller_dev	rcdev;
> +	bool				active_low;
> +};
> +
> +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..db9a1a75523f4 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->active_low = 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);
>

[PATCH v3 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>
---
 - Rename "status_inverted" properties to "status_active_low".
 - Add reg_offset and nr_reset override to struct reset_simple_devdata.
 - Drop socfpga special case, instead check "altr,modrst-offset" device
   tree property if a default reg_offset is set.
---
 drivers/reset/Kconfig         |  10 +--
 drivers/reset/Makefile        |   1 -
 drivers/reset/reset-simple.c  |  50 +++++++++++++-
 drivers/reset/reset-simple.h  |   4 ++
 drivers/reset/reset-socfpga.c | 154 ------------------------------------------
 5 files changed, 55 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 70c57c0758c39..84b08ce4def01 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -68,25 +68,58 @@ 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_active_low;
+}
+
 const struct reset_control_ops reset_simple_ops = {
 	.assert		= reset_simple_assert,
 	.deassert	= reset_simple_deassert,
+	.status		= reset_simple_status,
 };
 
 /**
  * struct reset_simple_devdata - simple reset controller properties
+ * @reg_offset: offset between base address and first reset register.
+ * @nr_resets: number of resets. If not set, default to resource size in bits.
  * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
  *              are set to assert the reset.
+ * @status_active_low: if true, bits read back as cleared while the reset is
+ *                     asserted. Otherwise, bits read back as set while the
+ *                     reset is asserted.
  */
 struct reset_simple_devdata {
+	u32 reg_offset;
+	u32 nr_resets;
 	bool active_low;
+	bool status_active_low;
+};
+
+#define SOCFPGA_NR_BANKS	8
+
+static const struct reset_simple_devdata reset_simple_socfpga = {
+	.reg_offset = 0x10,
+	.nr_resets = SOCFPGA_NR_BANKS * 32,
+	.status_active_low = true,
 };
 
 static const struct reset_simple_devdata reset_simple_active_low = {
 	.active_low = true,
+	.status_active_low = 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_active_low },
 	{ /* sentinel */ },
@@ -118,8 +151,23 @@ 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) {
+		u32 reg_offset = devdata->reg_offset;
+
+		if (reg_offset &&
+		    of_property_read_u32(dev->of_node, "altr,modrst-offset",
+					 &reg_offset)) {
+			dev_warn(dev,
+				 "missing altr,modrst-offset property, assuming 0x%x!\n",
+				 reg_offset);
+		}
+
+		data->membase += reg_offset;
+		if (devdata->nr_resets)
+			data->rcdev.nr_resets = devdata->nr_resets;
 		data->active_low = devdata->active_low;
+		data->status_active_low = devdata->status_active_low;
+	}
 
 	return devm_reset_controller_register(dev, &data->rcdev);
 }
diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
index 39af2014b5f12..8a496022baeff 100644
--- a/drivers/reset/reset-simple.h
+++ b/drivers/reset/reset-simple.h
@@ -28,12 +28,16 @@
  * @active_low: if true, bits are cleared to assert the reset. Otherwise, bits
  *              are set to assert the reset. Note that this says nothing about
  *              the voltage level of the actual reset line.
+ * @status_active_low: if true, bits read back as cleared while the reset is
+ *                     asserted. Otherwise, bits read back as set while the
+ *                     reset is asserted.
  */
 struct reset_simple_data {
 	spinlock_t			lock;
 	void __iomem			*membase;
 	struct reset_controller_dev	rcdev;
 	bool				active_low;
+	bool				status_active_low;
 };
 
 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 v3 2/5] reset: socfpga: use the reset-simple driver

[Alexandru Gagniuc]

Hi Phillip,

On 08/16/2017 02:46 AM, Philipp Zabel wrote:
[snip]
> @@ -118,8 +151,23 @@ 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) {
> +		u32 reg_offset = devdata->reg_offset;
> +
> +		if (reg_offset &&
> +		    of_property_read_u32(dev->of_node, "altr,modrst-offset",
> +					 &reg_offset)) {
> +			dev_warn(dev,
> +				 "missing altr,modrst-offset property, assuming 0x%x!\n",
> +				 reg_offset);
> +		}

I would not make reading dt properties dependent on the presence of 
devdata. That breaks being able to configure the reset controller from 
dt. I would either just read the "altr,modrst-offset" property, or read 
it when

	of_device_is_compatible(dev->of_node, "altr,rst-mgr"));

Ideally, we could have a more generic "reg-offset" binding for new reset 
controllers, but that is beyond the scope of this patch.

Alex

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

[Philipp Zabel]

On Wed, 2017-08-16 at 13:46 -0700, Alexandru Gagniuc wrote:
> Hi Phillip,
> 
> On 08/16/2017 02:46 AM, Philipp Zabel wrote:
> [snip]
> > @@ -118,8 +151,23 @@ 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) {
> > +		u32 reg_offset = devdata->reg_offset;
> > +
> > +		if (reg_offset &&
> > +		????of_property_read_u32(dev->of_node,
> > "altr,modrst-offset",
> > +					?&reg_offset)) {
> > +			dev_warn(dev,
> > +				?"missing altr,modrst-offset
> > property, assuming 0x%x!\n",
> > +				?reg_offset);
> > +		}
> 
> I would not make reading dt properties dependent on the presence of?
> devdata. That breaks being able to configure the reset controller from?
> dt. I would either just read the "altr,modrst-offset" property, or read?
> it when
> 
> 	of_device_is_compatible(dev->of_node, "altr,rst-mgr"));

Checking the compatible again is unnecessary, we already have obtained
devdata depending on the compatible value. I had effectively the same
check in v2:

	if (devdata == &reset_simple_socfpga)

I can change back to that, if you prefer. Note that the

	if (devdata)
		reg_offset = devdata->reg_offset;

part has to stay now that reg_offset is stored in devdata.

> Ideally, we could have a more generic "reg-offset" binding for new reset?
> controllers, but that is beyond the scope of this patch.

Agreed.

regards
Philipp

[PATCH v3 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 84b08ce4def01..e54a29b8b532d 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -120,6 +120,7 @@ static const struct reset_simple_devdata reset_simple_active_low = {
 
 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_active_low },
 	{ /* 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 v3 3/5] reset: stm32: use the reset-simple driver

[Eugeniy Paltsev]

Hi Philipp,

On Wed, 2017-08-16 at 11:46 +0200, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> [snip]
> 
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -120,6 +120,7 @@ static const struct reset_simple_devdata
> reset_simple_active_low = {
> ?
> ?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_active_low },
> ?	{ /* sentinel */ },

What about adding generic compatible strings for future use?
I mean next:

--------------------->8---------------------

static const struct of_device_id reset_simple_dt_ids[] = {
	{ .compatible = "reset-simple-active-low",
		.data = &reset_simple_active_low },
	{ .compatible = "reset-simple-active-high",
		.data = &reset_simple_active_high },
	/* ... */
	{ /* sentinel */ },
};

static const struct reset_simple_devdata reset_simple_active_high = {
	.active_low = false,
};

static const struct reset_simple_devdata reset_simple_active_low = {
	.active_low = true,
};

--------------------->8---------------------

-- 
?Eugeniy Paltsev

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

[Andreas Färber]

Hi Eugeniy,

Am 16.08.2017 um 14:52 schrieb Eugeniy Paltsev:
> On Wed, 2017-08-16 at 11:46 +0200, Philipp Zabel wrote:
>> The reset-simple driver can be used without changes.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> [snip]
>>
>> --- a/drivers/reset/reset-simple.c
>> +++ b/drivers/reset/reset-simple.c
>> @@ -120,6 +120,7 @@ static const struct reset_simple_devdata
>> reset_simple_active_low = {
>>  
>>  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_active_low },
>>  	{ /* sentinel */ },
> 
> What about adding generic compatible strings for future use?

Please see 1/5 for the same discussion, raised by Andre and me.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

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

[Andre Przywara]

Hi,

On 16/08/17 13:52, Eugeniy Paltsev wrote:
> Hi Philipp,
> 
> On Wed, 2017-08-16 at 11:46 +0200, Philipp Zabel wrote:
>> The reset-simple driver can be used without changes.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> [snip]
>>
>> --- a/drivers/reset/reset-simple.c
>> +++ b/drivers/reset/reset-simple.c
>> @@ -120,6 +120,7 @@ static const struct reset_simple_devdata
>> reset_simple_active_low = {
>>  
>>  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_active_low },
>>  	{ /* sentinel */ },
> 
> What about adding generic compatible strings for future use?

Exactly ;-) (see my previous email)
But we still need the pre-existing strings in here, of course.

> I mean next:
> 
> --------------------->8---------------------
> 
> static const struct of_device_id reset_simple_dt_ids[] = {
> 	{ .compatible = "reset-simple-active-low",
> 		.data = &reset_simple_active_low },
> 	{ .compatible = "reset-simple-active-high",
> 		.data = &reset_simple_active_high },

Either that or maybe even better define a "simple-reset" string and add
standard properties like "active-low".

Cheers,
Andre.

> 	/* ... */
> 	{ /* sentinel */ },
> };
> 
> static const struct reset_simple_devdata reset_simple_active_high = {
> 	.active_low = false,
> };
> 
> static const struct reset_simple_devdata reset_simple_active_low = {
> 	.active_low = true,
> };
> 
> --------------------->8---------------------
> 

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

[Alexandru Gagniuc]

On 08/16/2017 02:46 AM, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>

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

If this series gets respun, could you please look into removing the 
ARCH_ dependency here?

Alex

>  	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 84b08ce4def01..e54a29b8b532d 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -120,6 +120,7 @@ static const struct reset_simple_devdata reset_simple_active_low = {
>
>  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_active_low },
>  	{ /* 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);
>

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

[Andreas Färber]

Am 16.08.2017 um 22:50 schrieb Alexandru Gagniuc:
> On 08/16/2017 02:46 AM, Philipp Zabel wrote:
>> The reset-simple driver can be used without changes.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> 
>> ---
>>  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
> 
> If this series gets respun, could you please look into removing the
> ARCH_ dependency here?

Why? It's not a dependency, just the default. Doing it here decouples it
from having to do Kconfig changes in the arm-soc tree as well.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

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

[Alexandru Gagniuc]

On 08/16/2017 01:52 PM, Andreas F?rber wrote:
> Am 16.08.2017 um 22:50 schrieb Alexandru Gagniuc:
>> On 08/16/2017 02:46 AM, Philipp Zabel wrote:
>>> The reset-simple driver can be used without changes.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>
>> Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>
>>
>>> ---
>>>  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
>>
>> If this series gets respun, could you please look into removing the
>> ARCH_ dependency here?
>
> Why?

Because the driver has to keep track of all its users, which is a 
layering violation.

> It's not a dependency, just the default. Doing it here decouples it
> from having to do Kconfig changes in the arm-soc tree as well.

Doing it here allows this list to grow without bounds.

> Regards,
> Andreas
>

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

[Andre Przywara]

Hi,

On 16/08/17 21:55, Alexandru Gagniuc wrote:
> 
> 
> On 08/16/2017 01:52 PM, Andreas F?rber wrote:
>> Am 16.08.2017 um 22:50 schrieb Alexandru Gagniuc:
>>> On 08/16/2017 02:46 AM, Philipp Zabel wrote:
>>>> The reset-simple driver can be used without changes.
>>>>
>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>
>>> Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>
>>>
>>>> ---
>>>>  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
>>>
>>> If this series gets respun, could you please look into removing the
>>> ARCH_ dependency here?
>>
>> Why?
> 
> Because the driver has to keep track of all its users, which is a
> layering violation.

"keep track of all its users": not necessarily, this is mostly for
convenience reasons to get automatic coverage for those SoCs without
changing various defconfigs - or distribution .configs, for that matter.
Especially the latter may prove to be nasty and introduce regressions.

But I see that this list may grow big and this is a bit beyond the scope
of the otherwise neat "default ARCH_xxx" trick.

So given the generic nature of this driver, can't we make this simply:
"default ARM || ARM64"
That would enable it everywhere automatically, but people can still
disable this if they know what they do.

Alternatively for ARM64 this is actually a candidate for the (one and
only) defconfig.

Cheers,
Andre.

>> It's not a dependency, just the default. Doing it here decouples it
>> from having to do Kconfig changes in the arm-soc tree as well.
> 
> Doing it here allows this list to grow without bounds.
> 
>> Regards,
>> Andreas
>>

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

[Philipp Zabel]

On Thu, 2017-08-17 at 10:19 +0100, Andre Przywara wrote:
> Hi,
> 
> On 16/08/17 21:55, Alexandru Gagniuc wrote:
> > 
> > 
> > On 08/16/2017 01:52 PM, Andreas F?rber wrote:
> > > Am 16.08.2017 um 22:50 schrieb Alexandru Gagniuc:
> > > > On 08/16/2017 02:46 AM, Philipp Zabel wrote:
> > > > > The reset-simple driver can be used without changes.
> > > > > 
> > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > 
> > > > > > > > Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> > > > 
> > > > > ---
> > > > > ?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
> > > > 
> > > > If this series gets respun, could you please look into removing the
> > > > ARCH_ dependency here?
> > > 
> > > Why?
> > 
> > Because the driver has to keep track of all its users, which is a
> > layering violation.
> 
> "keep track of all its users": not necessarily, this is mostly for
> convenience reasons to get automatic coverage for those SoCs without
> changing various defconfigs - or distribution .configs, for that matter.
> Especially the latter may prove to be nasty and introduce regressions.
> 
> But I see that this list may grow big and this is a bit beyond the scope
> of the otherwise neat "default ARCH_xxx" trick.
> 
> So given the generic nature of this driver, can't we make this simply:
> "default ARM || ARM64"
> That would enable it everywhere automatically, but people can still
> disable this if they know what they do.
> 
> Alternatively for ARM64 this is actually a candidate for the (one and
> only) defconfig.

I would like to make this driver selectable. but for that to happen,
ARCH_HAS_RESET_CONTROLLER and RESET_CONTROLLER have to be merged into a
single selectable symbol first.

Until then, I'd be fine with making this driver:
	bool "Simple Reset Controller Driver"
	default ARM || ARM64 
but in my opinion it is equally ok to have the default ARCH_* list for
a while.

regards
Philipp

[v3,3/5] reset: stm32: use the reset-simple driver

[Gabriel FERNANDEZ]

On 08/16/2017 11:46 AM, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> ---
>   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 84b08ce4def01..e54a29b8b532d 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -120,6 +120,7 @@ static const struct reset_simple_devdata reset_simple_active_low = {
>   
>   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_active_low },
>   	{ /* 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);
Acked-by: Gabriel Fernandez <gabriel.fernandez@st.com>

[PATCH v3 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 |  2 +
 drivers/reset/reset-zx2967.c | 99 --------------------------------------------
 5 files changed, 4 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 e54a29b8b532d..13e7d5559acc9 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -123,6 +123,8 @@ static const struct of_device_id reset_simple_dt_ids[] = {
 	{ .compatible = "st,stm32-rcc", },
 	{ .compatible = "allwinner,sun6i-a31-clock-reset",
 		.data = &reset_simple_active_low },
+	{ .compatible = "zte,zx296718-reset",
+		.data = &reset_simple_active_low },
 	{ /* 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 v3 4/5] reset: zx2967: use the reset-simple driver

[Alexandru Gagniuc]

On 08/16/2017 02:47 AM, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Alexandru Gagniuc <alex.g@adaptrum.com>

> ---
>  MAINTAINERS                  |  1 -
>  drivers/reset/Kconfig        | 10 +----
>  drivers/reset/Makefile       |  1 -
>  drivers/reset/reset-simple.c |  2 +
>  drivers/reset/reset-zx2967.c | 99 --------------------------------------------
>  5 files changed, 4 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 e54a29b8b532d..13e7d5559acc9 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -123,6 +123,8 @@ static const struct of_device_id reset_simple_dt_ids[] = {
>  	{ .compatible = "st,stm32-rcc", },
>  	{ .compatible = "allwinner,sun6i-a31-clock-reset",
>  		.data = &reset_simple_active_low },
> +	{ .compatible = "zte,zx296718-reset",
> +		.data = &reset_simple_active_low },
>  	{ /* 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);
>

[PATCH v3 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>
---
 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 13e7d5559acc9..d98a7e7d802d1 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->active_low)
 		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

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

[Alexandru Gagniuc]

On 08/16/2017 02:47 AM, Philipp Zabel wrote:
> 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

Is there hardware where this has been observed to happen, or is this 
purely theoretical? It would be nice to have a "this is needed on 
hardware XYZ because ABC, and doesn't affect other hardware" comment in 
the source.

> 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>
> ---
>  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 13e7d5559acc9..d98a7e7d802d1 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->active_low)
>  		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);

You're not using the returned value to check that the reset was actually 
set. This seems a very arbitrary readback workaround, which gives no 
indication if it actually succeeded or not.

Also the set() is now asymmetrical to clear(). In cases when releasing 
reset on a HW block that is about to have IO performed on it, one would 
want to make sure the reset is actually deasserted before doing any IO.

Alex

>
>  	spin_unlock_irqrestore(&data->lock, flags);
>
>

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

[Philipp Zabel]

Hi Alexandru,

On Wed, 2017-08-16 at 14:00 -0700, Alexandru Gagniuc wrote:
> 
> On 08/16/2017 02:47 AM, Philipp Zabel wrote:
> > 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
> 
> Is there hardware where this has been observed to happen, or is this?
> purely theoretical? It would be nice to have a "this is needed on?
> hardware XYZ because ABC, and doesn't affect other hardware" comment in?
> the source.

This is purely theoretical, at least in the context of reset
controllers. I'm happy to drop this patch for now.

> > 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>
> > ---
> > ?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 13e7d5559acc9..d98a7e7d802d1 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->active_low)
> > > > ?		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);
> 
> You're not using the returned value to check that the reset was actually?
> set. This seems a very arbitrary readback workaround, which gives no?
> indication if it actually succeeded or not.

True. For those reset controllers that support status readback, this
would be a better check.

> Also the set() is now asymmetrical to clear(). In cases when releasing?
> reset on a HW block that is about to have IO performed on it, one would?
> want to make sure the reset is actually deasserted before doing any IO.

Thanks for this observation, the reset_simple_set function name is
ambiguous. I'll rename it to reset_simple_update.
The function is called both for assert and deassert operations, and
whether the bit is set or cleared depends on both the assert parameter
and the active_low flag.

regards
Philipp