LinuxLists.cc - [PATCH v3 0/5] Unify simple reset drivers

[permalink] [raw]

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

The reset-simple driver can be used without changes.

Signed-off-by: Philipp Zabel <[email protected]>
---
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 <[email protected]>
- * 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

2017-08-16 09:47:27

[permalink] [raw]

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

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 <[email protected]>
---
- 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 <[email protected]>
- *
- * based on
- * Allwinner SoCs Reset Controller driver
- *
- * Copyright 2013 Maxime Ripard
- *
- * Maxime Ripard <[email protected]>
- *
- * 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

2017-08-16 09:47:54

[permalink] [raw]

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

The reset-simple driver can be used without changes.

Signed-off-by: Philipp Zabel <[email protected]>
---
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: [email protected] (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 <[email protected]>
- *
- * 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

2017-08-16 09:47:53

[permalink] [raw]

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

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 <[email protected]>
---
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

2017-08-16 09:47:26

[permalink] [raw]

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

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 <[email protected]>
---
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 <[email protected]>
+ *
+ * Based on Allwinner SoCs Reset Controller driver
+ *
+ * Copyright 2013 Maxime Ripard
+ *
+ * Maxime Ripard <[email protected]>
+ *
+ * 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 <[email protected]>
+ *
+ * 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

2017-08-16 11:30:30

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <[email protected]>
> + *
> + * 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 <[email protected]>
> + *
> + * 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);
>

2017-08-16 12:12:39

[permalink] [raw]

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

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)

2017-08-16 12:52:09

by Eugeniy Paltsev

[permalink] [raw]

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

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 <[email protected]>
> [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

2017-08-16 12:55:55

[permalink] [raw]

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

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 <[email protected]>
>> [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)

2017-08-16 13:04:01

[permalink] [raw]

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

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 <[email protected]>
>> [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---------------------
>

2017-08-16 15:11:40

[permalink] [raw]

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

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

2017-08-16 15:17:49

[permalink] [raw]

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

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.

2017-08-16 16:41:29

[permalink] [raw]

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

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)

2017-08-16 16:46:22

[permalink] [raw]

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

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.

2017-08-16 20:46:17

[permalink] [raw]

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

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

2017-08-16 20:47:03

[permalink] [raw]

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

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 <[email protected]>

Reviewed-by: Alexandru Gagniuc <[email protected]>

> ---
> 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 <[email protected]>
> + *
> + * Based on Allwinner SoCs Reset Controller driver
> + *
> + * Copyright 2013 Maxime Ripard
> + *
> + * Maxime Ripard <[email protected]>
> + *
> + * 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 <[email protected]>
> + *
> + * 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);
>

2017-08-16 20:50:19

[permalink] [raw]

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

On 08/16/2017 02:46 AM, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
>
> Signed-off-by: Philipp Zabel <[email protected]>

Reviewed-by: Alexandru Gagniuc <[email protected]>

> ---
> 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 <[email protected]>
> - * 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);
>

2017-08-16 20:51:01

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] reset: zx2967: use the reset-simple driver

On 08/16/2017 02:47 AM, Philipp Zabel wrote:
> The reset-simple driver can be used without changes.
>
> Signed-off-by: Philipp Zabel <[email protected]>

Reviewed-by: Alexandru Gagniuc <[email protected]>

> ---
> 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: [email protected] (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 <[email protected]>
> - *
> - * 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);
>

2017-08-16 20:52:47

[permalink] [raw]

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

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 <[email protected]>
>
> Reviewed-by: Alexandru Gagniuc <[email protected]>
>
>> ---
>> 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)

2017-08-16 20:55:44

[permalink] [raw]

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

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 <[email protected]>
>>
>> Reviewed-by: Alexandru Gagniuc <[email protected]>
>>
>>> ---
>>> 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
>

2017-08-16 21:01:08

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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);
>
>

2017-08-17 09:20:04

[permalink] [raw]

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

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 <[email protected]>
>>>
>>> Reviewed-by: Alexandru Gagniuc <[email protected]>
>>>
>>>> ---
>>>> 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
>>

2017-08-21 08:21:07

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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

2017-08-21 08:38:18

[permalink] [raw]

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

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 <[email protected]>
> > > >
> > > > > > > > Reviewed-by: Alexandru Gagniuc <[email protected]>
> > > >
> > > > > ---
> > > > > 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

2017-08-21 08:45:31