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

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

[Philipp Zabel]

Reset operations for simple reset controllers with reset lines that can
be controlled by toggling bits in (mostly) contiguous register ranges
using read-modify-write cycles under a spinlock. So far this covers the
socfpga, stm32, and sunxi drivers.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/Kconfig         |  6 +++
 drivers/reset/Makefile        |  1 +
 drivers/reset/reset-simple.c  | 92 +++++++++++++++++++++++++++++++++++++++++
 drivers/reset/reset-simple.h  | 53 ++++++++++++++++++++++++
 drivers/reset/reset-socfpga.c | 96 +++++--------------------------------------
 drivers/reset/reset-stm32.c   | 73 ++++----------------------------
 drivers/reset/reset-sunxi.c   | 82 +++++-------------------------------
 7 files changed, 181 insertions(+), 222 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 d21c07ccc94e5..d968becd0474c 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -62,21 +62,27 @@ config RESET_PISTACHIO
 	help
 	  This enables the reset driver for ImgTec Pistachio SoCs.
 
+config RESET_SIMPLE
+	bool
+
 config RESET_SOCFPGA
 	bool "SoCFPGA Reset Driver" if COMPILE_TEST
 	default ARCH_SOCFPGA
+	select RESET_SIMPLE
 	help
 	  This enables the reset controller driver for Altera SoCFPGAs.
 
 config RESET_STM32
 	bool "STM32 Reset Driver" if COMPILE_TEST
 	default ARCH_STM32
+	select RESET_SIMPLE
 	help
 	  This enables the RCC reset controller driver for STM32 MCUs.
 
 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 02a74db943397..f5cc015f20956 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -10,6 +10,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..2160e84fe216b
--- /dev/null
+++ b/drivers/reset/reset-simple.c
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ */
+
+#include <linux/io.h>
+#include <linux/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_clear(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);
+	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 reset_simple_set(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);
+	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 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));
+}
+
+const struct reset_control_ops reset_simple_ops = {
+	.assert		= reset_simple_set,
+	.deassert	= reset_simple_clear,
+	.status		= reset_simple_status,
+};
+
+const struct reset_control_ops reset_simple_ops_inv = {
+	.assert		= reset_simple_clear,
+	.deassert	= reset_simple_set,
+};
diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
new file mode 100644
index 0000000000000..421a0795a2372
--- /dev/null
+++ b/drivers/reset/reset-simple.h
@@ -0,0 +1,53 @@
+/*
+ * Simple Reset Controller ops
+ *
+ * Based on Allwinner SoCs Reset Controller driver
+ *
+ * Copyright 2013 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __RESET_SIMPLE_H__
+#define __RESET_SIMPLE_H__
+
+#include <linux/io.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+
+struct reset_simple_data {
+	spinlock_t			lock;
+	void __iomem			*membase;
+	struct reset_controller_dev	rcdev;
+};
+
+extern const struct reset_control_ops reset_simple_ops;
+extern const struct reset_control_ops reset_simple_ops_inv;
+
+static inline int devm_reset_controller_register_simple(struct device *dev,
+							void __iomem *membase,
+							unsigned int nr_resets,
+							bool inverted)
+{
+	struct reset_simple_data *data;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	spin_lock_init(&data->lock);
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = nr_resets;
+	data->rcdev.ops = inverted ? &reset_simple_ops_inv : &reset_simple_ops;
+	data->rcdev.of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+#endif /* __RESET_SIMPLE_H__ */
diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index 07224c0198920..6031a3873bc8f 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -25,83 +25,16 @@
 #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));
+#include "reset-simple.h"
 
-	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,
-};
+#define NR_BANKS		8
 
 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;
+	void __iomem *membase;
 	u32 modrst_offset;
 
 	/*
@@ -114,29 +47,20 @@ static int socfpga_reset_probe(struct platform_device *pdev)
 		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);
+	membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(membase))
+		return PTR_ERR(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;
+	membase += modrst_offset;
 
-	return devm_reset_controller_register(dev, &data->rcdev);
+	return devm_reset_controller_register_simple(&pdev->dev, membase,
+						     NR_BANKS * BITS_PER_LONG,
+						     false);
 }
 
 static const struct of_device_id socfpga_reset_dt_ids[] = {
diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c
index 3a7c8527e66af..15d89d66863d8 100644
--- a/drivers/reset/reset-stm32.c
+++ b/drivers/reset/reset-stm32.c
@@ -16,58 +16,7 @@
 #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,
-};
+#include "reset-simple.h"
 
 static const struct of_device_id stm32_reset_dt_ids[] = {
 	 { .compatible = "st,stm32-rcc", },
@@ -76,26 +25,22 @@ static const struct of_device_id stm32_reset_dt_ids[] = {
 
 static int stm32_reset_probe(struct platform_device *pdev)
 {
-	struct stm32_reset_data *data;
+	struct reset_simple_data *data;
 	struct resource *res;
+	void __iomem *membase;
 
 	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;
+	membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(membase))
+		return PTR_ERR(membase);
 
-	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+	return devm_reset_controller_register_simple(&pdev->dev, membase,
+						     resource_size(res) * 8,
+						     false);
 }
 
 static struct platform_driver stm32_reset_driver = {
diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
index cd585cd2f04dd..60745dd6f70a6 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,7 +55,7 @@ static int sunxi_reset_init(struct device_node *np)
 
 	data->rcdev.owner = THIS_MODULE;
 	data->rcdev.nr_resets = size * 32;
-	data->rcdev.ops = &sunxi_reset_ops;
+	data->rcdev.ops = &reset_simple_ops_inv;
 	data->rcdev.of_node = np;
 
 	return reset_controller_register(&data->rcdev);
@@ -147,26 +94,17 @@ static const struct of_device_id sunxi_reset_dt_ids[] = {
 
 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;
+	void __iomem *membase;
 
 	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) * 32;
-	data->rcdev.ops = &sunxi_reset_ops;
-	data->rcdev.of_node = pdev->dev.of_node;
+	membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(membase))
+		return PTR_ERR(membase);
 
-	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+	return devm_reset_controller_register_simple(&pdev->dev, membase,
+						     resource_size(res) * 32,
+						     true);
 }
 
 static struct platform_driver sunxi_reset_driver = {
-- 
2.11.0

[PATCH 2/2] 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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index 2160e84fe216b..e32289bdaf0c6 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -33,13 +33,16 @@ static int reset_simple_clear(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));
-	writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
+	reg = readl(addr);
+	writel(reg & ~BIT(offset), addr);
+	/* Read back to make sure the write doesn't linger in a store buffer */
+	readl(addr);
 
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -53,13 +56,16 @@ 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));
-	writel(reg | BIT(offset), data->membase + (bank * reg_width));
+	reg = readl(addr);
+	writel(reg | BIT(offset), 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

Re: [PATCH 2/2] reset: simple: read back to make sure changes are applied

[Andre Przywara]

Hi,

On 08/03/17 09:54, 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
> 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 | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index 2160e84fe216b..e32289bdaf0c6 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -33,13 +33,16 @@ static int reset_simple_clear(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));
> -	writel(reg & ~BIT(offset), data->membase + (bank * reg_width));
> +	reg = readl(addr);
> +	writel(reg & ~BIT(offset), addr);
> +	/* Read back to make sure the write doesn't linger in a store buffer */
> +	readl(addr);

Nit: "Read back to make sure the write doesn't linger in a store
buffer", sounds somewhat dodgy.
What about: "Read back to make sure the write has reached the device."?

And I wonder if we actually need to check the returned value to make
sure the device has actually changed the state? Or is this too paranoid?

Cheers,
Andre.

>  	spin_unlock_irqrestore(&data->lock, flags);
>  
> @@ -53,13 +56,16 @@ 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));
> -	writel(reg | BIT(offset), data->membase + (bank * reg_width));
> +	reg = readl(addr);
> +	writel(reg | BIT(offset), addr);
> +	/* Read back to make sure the write doesn't linger in a store buffer */
> +	readl(addr);
>  
>  	spin_unlock_irqrestore(&data->lock, flags);
>  
> 

Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi

[Andre Przywara]

Hi,

On 08/03/17 09:54, Philipp Zabel wrote:
> Reset operations for simple reset controllers with reset lines that can
> be controlled by toggling bits in (mostly) contiguous register ranges
> using read-modify-write cycles under a spinlock. So far this covers the
> socfpga, stm32, and sunxi drivers.

Wow, that looks nice, thanks for that.

But can't we go one step further and unify those driver into one file then?
And either have different probe functions to cover the different DT
requirements or to just have one unified probe checking for the super
set of all properties?

Also ....

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/reset/Kconfig         |  6 +++
>  drivers/reset/Makefile        |  1 +
>  drivers/reset/reset-simple.c  | 92 +++++++++++++++++++++++++++++++++++++++++
>  drivers/reset/reset-simple.h  | 53 ++++++++++++++++++++++++
>  drivers/reset/reset-socfpga.c | 96 +++++--------------------------------------
>  drivers/reset/reset-stm32.c   | 73 ++++----------------------------
>  drivers/reset/reset-sunxi.c   | 82 +++++-------------------------------
>  7 files changed, 181 insertions(+), 222 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 d21c07ccc94e5..d968becd0474c 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -62,21 +62,27 @@ config RESET_PISTACHIO
>  	help
>  	  This enables the reset driver for ImgTec Pistachio SoCs.
>  
> +config RESET_SIMPLE
> +	bool
> +
>  config RESET_SOCFPGA
>  	bool "SoCFPGA Reset Driver" if COMPILE_TEST
>  	default ARCH_SOCFPGA
> +	select RESET_SIMPLE
>  	help
>  	  This enables the reset controller driver for Altera SoCFPGAs.
>  
>  config RESET_STM32
>  	bool "STM32 Reset Driver" if COMPILE_TEST
>  	default ARCH_STM32
> +	select RESET_SIMPLE
>  	help
>  	  This enables the RCC reset controller driver for STM32 MCUs.
>  
>  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 02a74db943397..f5cc015f20956 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,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..2160e84fe216b
> --- /dev/null
> +++ b/drivers/reset/reset-simple.c
> @@ -0,0 +1,92 @@
> +/*
> + * 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.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/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_clear(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);
> +	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 reset_simple_set(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);
> +	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;
> +}

So these two functions look strikingly similar, apart from the OR / AND
NEG part.
What about a unified bit access function then and two tiny wrappers for
set/clear?

Cheers,
Andre.


> +
> +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));
> +}
> +
> +const struct reset_control_ops reset_simple_ops = {
> +	.assert		= reset_simple_set,
> +	.deassert	= reset_simple_clear,
> +	.status		= reset_simple_status,
> +};
> +
> +const struct reset_control_ops reset_simple_ops_inv = {
> +	.assert		= reset_simple_clear,
> +	.deassert	= reset_simple_set,
> +};
> diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
> new file mode 100644
> index 0000000000000..421a0795a2372
> --- /dev/null
> +++ b/drivers/reset/reset-simple.h
> @@ -0,0 +1,53 @@
> +/*
> + * Simple Reset Controller ops
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __RESET_SIMPLE_H__
> +#define __RESET_SIMPLE_H__
> +
> +#include <linux/io.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +struct reset_simple_data {
> +	spinlock_t			lock;
> +	void __iomem			*membase;
> +	struct reset_controller_dev	rcdev;
> +};
> +
> +extern const struct reset_control_ops reset_simple_ops;
> +extern const struct reset_control_ops reset_simple_ops_inv;
> +
> +static inline int devm_reset_controller_register_simple(struct device *dev,
> +							void __iomem *membase,
> +							unsigned int nr_resets,
> +							bool inverted)
> +{
> +	struct reset_simple_data *data;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&data->lock);
> +
> +	data->rcdev.owner = THIS_MODULE;
> +	data->rcdev.nr_resets = nr_resets;
> +	data->rcdev.ops = inverted ? &reset_simple_ops_inv : &reset_simple_ops;
> +	data->rcdev.of_node = dev->of_node;
> +
> +	return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +#endif /* __RESET_SIMPLE_H__ */
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> index 07224c0198920..6031a3873bc8f 100644
> --- a/drivers/reset/reset-socfpga.c
> +++ b/drivers/reset/reset-socfpga.c
> @@ -25,83 +25,16 @@
>  #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));
> +#include "reset-simple.h"
>  
> -	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,
> -};
> +#define NR_BANKS		8
>  
>  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;
> +	void __iomem *membase;
>  	u32 modrst_offset;
>  
>  	/*
> @@ -114,29 +47,20 @@ static int socfpga_reset_probe(struct platform_device *pdev)
>  		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);
> +	membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(membase))
> +		return PTR_ERR(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;
> +	membase += modrst_offset;
>  
> -	return devm_reset_controller_register(dev, &data->rcdev);
> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
> +						     NR_BANKS * BITS_PER_LONG,
> +						     false);
>  }
>  
>  static const struct of_device_id socfpga_reset_dt_ids[] = {
> diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c
> index 3a7c8527e66af..15d89d66863d8 100644
> --- a/drivers/reset/reset-stm32.c
> +++ b/drivers/reset/reset-stm32.c
> @@ -16,58 +16,7 @@
>  #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,
> -};
> +#include "reset-simple.h"
>  
>  static const struct of_device_id stm32_reset_dt_ids[] = {
>  	 { .compatible = "st,stm32-rcc", },
> @@ -76,26 +25,22 @@ static const struct of_device_id stm32_reset_dt_ids[] = {
>  
>  static int stm32_reset_probe(struct platform_device *pdev)
>  {
> -	struct stm32_reset_data *data;
> +	struct reset_simple_data *data;
>  	struct resource *res;
> +	void __iomem *membase;
>  
>  	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;
> +	membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(membase))
> +		return PTR_ERR(membase);
>  
> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
> +						     resource_size(res) * 8,
> +						     false);
>  }
>  
>  static struct platform_driver stm32_reset_driver = {
> diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
> index cd585cd2f04dd..60745dd6f70a6 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,7 +55,7 @@ static int sunxi_reset_init(struct device_node *np)
>  
>  	data->rcdev.owner = THIS_MODULE;
>  	data->rcdev.nr_resets = size * 32;
> -	data->rcdev.ops = &sunxi_reset_ops;
> +	data->rcdev.ops = &reset_simple_ops_inv;
>  	data->rcdev.of_node = np;
>  
>  	return reset_controller_register(&data->rcdev);
> @@ -147,26 +94,17 @@ static const struct of_device_id sunxi_reset_dt_ids[] = {
>  
>  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;
> +	void __iomem *membase;
>  
>  	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) * 32;
> -	data->rcdev.ops = &sunxi_reset_ops;
> -	data->rcdev.of_node = pdev->dev.of_node;
> +	membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(membase))
> +		return PTR_ERR(membase);
>  
> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
> +						     resource_size(res) * 32,
> +						     true);
>  }
>  
>  static struct platform_driver sunxi_reset_driver = {
> 

Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi

[Alexandre Torgue]

Hi Philipp,

On 03/08/2017 11:19 AM, Andre Przywara wrote:
> Hi,
>
> On 08/03/17 09:54, Philipp Zabel wrote:
>> Reset operations for simple reset controllers with reset lines that can
>> be controlled by toggling bits in (mostly) contiguous register ranges
>> using read-modify-write cycles under a spinlock. So far this covers the
>> socfpga, stm32, and sunxi drivers.
>
> Wow, that looks nice, thanks for that.
>
> But can't we go one step further and unify those driver into one file then?
> And either have different probe functions to cover the different DT
> requirements or to just have one unified probe checking for the super
> set of all properties?

I agree with Andre. It looks nice and it should be a good thing to have 
a common probe inside reset-simple.c
Maybe only "nresets" and "inverted"  DT properties are needed.

Regards

Alex



>
> Also ....
>
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>  drivers/reset/Kconfig         |  6 +++
>>  drivers/reset/Makefile        |  1 +
>>  drivers/reset/reset-simple.c  | 92 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/reset/reset-simple.h  | 53 ++++++++++++++++++++++++
>>  drivers/reset/reset-socfpga.c | 96 +++++--------------------------------------
>>  drivers/reset/reset-stm32.c   | 73 ++++----------------------------
>>  drivers/reset/reset-sunxi.c   | 82 +++++-------------------------------
>>  7 files changed, 181 insertions(+), 222 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 d21c07ccc94e5..d968becd0474c 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -62,21 +62,27 @@ config RESET_PISTACHIO
>>  	help
>>  	  This enables the reset driver for ImgTec Pistachio SoCs.
>>
>> +config RESET_SIMPLE
>> +	bool
>> +
>>  config RESET_SOCFPGA
>>  	bool "SoCFPGA Reset Driver" if COMPILE_TEST
>>  	default ARCH_SOCFPGA
>> +	select RESET_SIMPLE
>>  	help
>>  	  This enables the reset controller driver for Altera SoCFPGAs.
>>
>>  config RESET_STM32
>>  	bool "STM32 Reset Driver" if COMPILE_TEST
>>  	default ARCH_STM32
>> +	select RESET_SIMPLE
>>  	help
>>  	  This enables the RCC reset controller driver for STM32 MCUs.
>>
>>  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 02a74db943397..f5cc015f20956 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -10,6 +10,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..2160e84fe216b
>> --- /dev/null
>> +++ b/drivers/reset/reset-simple.c
>> @@ -0,0 +1,92 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/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_clear(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);
>> +	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 reset_simple_set(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);
>> +	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;
>> +}
>
> So these two functions look strikingly similar, apart from the OR / AND
> NEG part.
> What about a unified bit access function then and two tiny wrappers for
> set/clear?
>
> Cheers,
> Andre.
>
>
>> +
>> +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));
>> +}
>> +
>> +const struct reset_control_ops reset_simple_ops = {
>> +	.assert		= reset_simple_set,
>> +	.deassert	= reset_simple_clear,
>> +	.status		= reset_simple_status,
>> +};
>> +
>> +const struct reset_control_ops reset_simple_ops_inv = {
>> +	.assert		= reset_simple_clear,
>> +	.deassert	= reset_simple_set,
>> +};
>> diff --git a/drivers/reset/reset-simple.h b/drivers/reset/reset-simple.h
>> new file mode 100644
>> index 0000000000000..421a0795a2372
>> --- /dev/null
>> +++ b/drivers/reset/reset-simple.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Simple Reset Controller ops
>> + *
>> + * Based on Allwinner SoCs Reset Controller driver
>> + *
>> + * Copyright 2013 Maxime Ripard
>> + *
>> + * Maxime Ripard <maxime.ripard@free-electrons.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#ifndef __RESET_SIMPLE_H__
>> +#define __RESET_SIMPLE_H__
>> +
>> +#include <linux/io.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/spinlock.h>
>> +
>> +struct reset_simple_data {
>> +	spinlock_t			lock;
>> +	void __iomem			*membase;
>> +	struct reset_controller_dev	rcdev;
>> +};
>> +
>> +extern const struct reset_control_ops reset_simple_ops;
>> +extern const struct reset_control_ops reset_simple_ops_inv;
>> +
>> +static inline int devm_reset_controller_register_simple(struct device *dev,
>> +							void __iomem *membase,
>> +							unsigned int nr_resets,
>> +							bool inverted)
>> +{
>> +	struct reset_simple_data *data;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&data->lock);
>> +
>> +	data->rcdev.owner = THIS_MODULE;
>> +	data->rcdev.nr_resets = nr_resets;
>> +	data->rcdev.ops = inverted ? &reset_simple_ops_inv : &reset_simple_ops;
>> +	data->rcdev.of_node = dev->of_node;
>> +
>> +	return devm_reset_controller_register(dev, &data->rcdev);
>> +}
>> +
>> +#endif /* __RESET_SIMPLE_H__ */
>> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
>> index 07224c0198920..6031a3873bc8f 100644
>> --- a/drivers/reset/reset-socfpga.c
>> +++ b/drivers/reset/reset-socfpga.c
>> @@ -25,83 +25,16 @@
>>  #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));
>> +#include "reset-simple.h"
>>
>> -	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,
>> -};
>> +#define NR_BANKS		8
>>
>>  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;
>> +	void __iomem *membase;
>>  	u32 modrst_offset;
>>
>>  	/*
>> @@ -114,29 +47,20 @@ static int socfpga_reset_probe(struct platform_device *pdev)
>>  		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);
>> +	membase = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(membase))
>> +		return PTR_ERR(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;
>> +	membase += modrst_offset;
>>
>> -	return devm_reset_controller_register(dev, &data->rcdev);
>> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
>> +						     NR_BANKS * BITS_PER_LONG,
>> +						     false);
>>  }
>>
>>  static const struct of_device_id socfpga_reset_dt_ids[] = {
>> diff --git a/drivers/reset/reset-stm32.c b/drivers/reset/reset-stm32.c
>> index 3a7c8527e66af..15d89d66863d8 100644
>> --- a/drivers/reset/reset-stm32.c
>> +++ b/drivers/reset/reset-stm32.c
>> @@ -16,58 +16,7 @@
>>  #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,
>> -};
>> +#include "reset-simple.h"
>>
>>  static const struct of_device_id stm32_reset_dt_ids[] = {
>>  	 { .compatible = "st,stm32-rcc", },
>> @@ -76,26 +25,22 @@ static const struct of_device_id stm32_reset_dt_ids[] = {
>>
>>  static int stm32_reset_probe(struct platform_device *pdev)
>>  {
>> -	struct stm32_reset_data *data;
>> +	struct reset_simple_data *data;
>>  	struct resource *res;
>> +	void __iomem *membase;
>>
>>  	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;
>> +	membase = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(membase))
>> +		return PTR_ERR(membase);
>>
>> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
>> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
>> +						     resource_size(res) * 8,
>> +						     false);
>>  }
>>
>>  static struct platform_driver stm32_reset_driver = {
>> diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
>> index cd585cd2f04dd..60745dd6f70a6 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,7 +55,7 @@ static int sunxi_reset_init(struct device_node *np)
>>
>>  	data->rcdev.owner = THIS_MODULE;
>>  	data->rcdev.nr_resets = size * 32;
>> -	data->rcdev.ops = &sunxi_reset_ops;
>> +	data->rcdev.ops = &reset_simple_ops_inv;
>>  	data->rcdev.of_node = np;
>>
>>  	return reset_controller_register(&data->rcdev);
>> @@ -147,26 +94,17 @@ static const struct of_device_id sunxi_reset_dt_ids[] = {
>>
>>  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;
>> +	void __iomem *membase;
>>
>>  	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) * 32;
>> -	data->rcdev.ops = &sunxi_reset_ops;
>> -	data->rcdev.of_node = pdev->dev.of_node;
>> +	membase = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(membase))
>> +		return PTR_ERR(membase);
>>
>> -	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
>> +	return devm_reset_controller_register_simple(&pdev->dev, membase,
>> +						     resource_size(res) * 32,
>> +						     true);
>>  }
>>
>>  static struct platform_driver sunxi_reset_driver = {
>>

Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi

[Philipp Zabel]

On Wed, 2017-03-08 at 12:05 +0100, Alexandre Torgue wrote:
> Hi Philipp,
> 
> On 03/08/2017 11:19 AM, Andre Przywara wrote:
> > Hi,
> >
> > On 08/03/17 09:54, Philipp Zabel wrote:
> >> Reset operations for simple reset controllers with reset lines that can
> >> be controlled by toggling bits in (mostly) contiguous register ranges
> >> using read-modify-write cycles under a spinlock. So far this covers the
> >> socfpga, stm32, and sunxi drivers.
> >
> > Wow, that looks nice, thanks for that.
> >
> > But can't we go one step further and unify those driver into one file then?
> > And either have different probe functions to cover the different DT
> > requirements or to just have one unified probe checking for the super
> > set of all properties?
> 
> I agree with Andre. It looks nice and it should be a good thing to have 
> a common probe inside reset-simple.c
> Maybe only "nresets" and "inverted"  DT properties are needed.

Adding DT properties is not an option, as the driver would have to work
with the existing bindings. We could merge them into one file, with a
single probe function that configures different parameters depending on
the of_device_id returned by of_match_device.

regards
Philipp

Re: [PATCH 1/2] reset: add reset-simple to unify socfpga, stm32, and sunxi

[Andre Przywara]

Hi,

On 08/03/17 12:20, Philipp Zabel wrote:
> On Wed, 2017-03-08 at 12:05 +0100, Alexandre Torgue wrote:
>> Hi Philipp,
>>
>> On 03/08/2017 11:19 AM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 08/03/17 09:54, Philipp Zabel wrote:
>>>> Reset operations for simple reset controllers with reset lines that can
>>>> be controlled by toggling bits in (mostly) contiguous register ranges
>>>> using read-modify-write cycles under a spinlock. So far this covers the
>>>> socfpga, stm32, and sunxi drivers.
>>>
>>> Wow, that looks nice, thanks for that.
>>>
>>> But can't we go one step further and unify those driver into one file then?
>>> And either have different probe functions to cover the different DT
>>> requirements or to just have one unified probe checking for the super
>>> set of all properties?
>>
>> I agree with Andre. It looks nice and it should be a good thing to have 
>> a common probe inside reset-simple.c
>> Maybe only "nresets" and "inverted"  DT properties are needed.
> 
> Adding DT properties is not an option, as the driver would have to work
> with the existing bindings. We could merge them into one file, with a
> single probe function that configures different parameters depending on
> the of_device_id returned by of_match_device.

Yes, either that or we could just parse all possible properties, as
allowing additional properties on top of the documented binding doc
shouldn't hurt, I think. That's what I mean with super-set. I have to
check if this is acceptable, though.

And I think it would be worthwhile to create a generic binding (or at
least a generic compatible string) on the way, specifying all the
existing properties there, so new drivers could just use that without
having to add their own compatible string to the *driver* (but just to
the binding doc). This would have the advantage of new SoCs possibly
being supported without actual kernel changes.

Cheers,
Andre.