[PATCH 0/2] Add support for SYSCON reset

[PATCH 0/2] Add support for SYSCON reset

[Andrew F. Davis]

Some SoCs contain reset controls for modules that are memory-mapped to
areas shared with other module configuration settings. This requires
synchronization across all drivers accessing this memory area. This
series adds a generic SYSCON reset driver to allow resets toggled
by bits in memory-mapped registers through SYSCON.

Andrew F. Davis (2):
  Documentation: dt: reset: Add syscon reset binding
  reset: add a SYSCON based reset driver

 .../devicetree/bindings/reset/syscon-reset.txt     |  84 +++++++
 drivers/reset/Kconfig                              |  10 +
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-syscon.c                       | 259 +++++++++++++++++++++
 4 files changed, 354 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
 create mode 100644 drivers/reset/reset-syscon.c

-- 
2.7.0

[PATCH 0/2] Add support for SYSCON reset

[Andrew F. Davis]

Some SoCs contain reset controls for modules that are memory-mapped to
areas shared with other module configuration settings. This requires
synchronization across all drivers accessing this memory area. This
series adds a generic SYSCON reset driver to allow resets toggled
by bits in memory-mapped registers through SYSCON.

Andrew F. Davis (2):
  Documentation: dt: reset: Add syscon reset binding
  reset: add a SYSCON based reset driver

 .../devicetree/bindings/reset/syscon-reset.txt     |  84 +++++++
 drivers/reset/Kconfig                              |  10 +
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-syscon.c                       | 259 +++++++++++++++++++++
 4 files changed, 354 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
 create mode 100644 drivers/reset/reset-syscon.c

-- 
2.7.0

[PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

Add syscon reset controller binding. This will hook to the reset
framework and use syscon/regmap to set reset bits. This allows
reset control of individual SoC subsytems and devices with
memory-mapped reset registers in a common register memory
space.

Signed-off-by: Andrew F. Davis <afd@ti.com>
[s-anna@ti.com: revise the binding format]
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt

diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
new file mode 100644
index 0000000..466378a
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
@@ -0,0 +1,84 @@
+SysCon Reset Controller
+=======================
+
+Almost all SoCs have hardware modules that require reset control in addition
+to clock and power control for their functionality. The reset control is
+typically provided by means of memory-mapped I/O registers. These registers are
+sometimes a part of a larger register space region implementing various
+functionalities. This register range is best represented as a syscon node to
+allow multiple entities to access their relevant registers in the common
+register space.
+
+A SysCon Reset Controller node defines a device that uses a syscon node
+and provides reset management functionality for various hardware modules
+present on the SoC.
+
+SysCon Reset Controller Node
+============================
+Each of the reset provider/controller nodes should have the following
+properties.
+
+Required properties:
+--------------------
+ - compatible	: Should be "syscon-reset"
+ - syscon	: phandle to the syscon node containing the reset registers
+ - #reset-cells	: Should be 6. Please see the reset consumer node below for
+                  usage details
+
+SysCon Reset Consumer Nodes
+===========================
+Each of the reset consumer nodes should have the following properties,
+in addition to their own properties.
+
+Required properties:
+--------------------
+ - resets	: A phandle and reset specifier pair, one pair for each reset
+		  signal that affects the device, or that the device manages.
+		  The phandle should point to the syscon node containing the
+		  reset registers, and the reset specifier should have 6
+		  cell-values. The reset specifier contains two similar pairs
+		  of 3 cell-values each, the first of the pair containing the
+		  reset control register information, and the second of the pair
+		  containing the reset status register information. The reset
+		  control and status registers can be same on some devices/SoCs.
+
+		  Each of the pairs of 3 cell-values should have the following
+		  values:
+		     Cell #1 : register offset of the reset control/status
+		               register from the syscon register base
+		     Cell #2 : bit shift value for the reset in the respective
+		               reset control/status register
+		     Cell #3 : polarity of the reset bit. Should be 1 for resets
+		               that are asserted when the bit is set, 0 for
+		               resets that are asserted when the bit is cleared
+
+Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
+common reset controller usage by consumers.
+
+
+Example:
+--------
+The following example demonstrates a syscon node, the reset controller node
+using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
+Hawking SoC.
+
+/ {
+	soc {
+		psc: power-sleep-controller@02350000 {
+			compatible = "syscon";
+			reg = <0x02350000 0x1000>;
+		};
+
+		pscrst: psc-reset {
+			compatible = "syscon-reset";
+			syscon = <&psc>;
+			#reset-cells = <6>;
+		};
+
+		dsp0: dsp0 {
+			...
+			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
+			...
+		};
+	};
+};
-- 
2.7.0

[PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

Add syscon reset controller binding. This will hook to the reset
framework and use syscon/regmap to set reset bits. This allows
reset control of individual SoC subsytems and devices with
memory-mapped reset registers in a common register memory
space.

Signed-off-by: Andrew F. Davis <afd@ti.com>
[s-anna@ti.com: revise the binding format]
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt

diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
new file mode 100644
index 0000000..466378a
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
@@ -0,0 +1,84 @@
+SysCon Reset Controller
+=======================
+
+Almost all SoCs have hardware modules that require reset control in addition
+to clock and power control for their functionality. The reset control is
+typically provided by means of memory-mapped I/O registers. These registers are
+sometimes a part of a larger register space region implementing various
+functionalities. This register range is best represented as a syscon node to
+allow multiple entities to access their relevant registers in the common
+register space.
+
+A SysCon Reset Controller node defines a device that uses a syscon node
+and provides reset management functionality for various hardware modules
+present on the SoC.
+
+SysCon Reset Controller Node
+============================
+Each of the reset provider/controller nodes should have the following
+properties.
+
+Required properties:
+--------------------
+ - compatible	: Should be "syscon-reset"
+ - syscon	: phandle to the syscon node containing the reset registers
+ - #reset-cells	: Should be 6. Please see the reset consumer node below for
+                  usage details
+
+SysCon Reset Consumer Nodes
+===========================
+Each of the reset consumer nodes should have the following properties,
+in addition to their own properties.
+
+Required properties:
+--------------------
+ - resets	: A phandle and reset specifier pair, one pair for each reset
+		  signal that affects the device, or that the device manages.
+		  The phandle should point to the syscon node containing the
+		  reset registers, and the reset specifier should have 6
+		  cell-values. The reset specifier contains two similar pairs
+		  of 3 cell-values each, the first of the pair containing the
+		  reset control register information, and the second of the pair
+		  containing the reset status register information. The reset
+		  control and status registers can be same on some devices/SoCs.
+
+		  Each of the pairs of 3 cell-values should have the following
+		  values:
+		     Cell #1 : register offset of the reset control/status
+		               register from the syscon register base
+		     Cell #2 : bit shift value for the reset in the respective
+		               reset control/status register
+		     Cell #3 : polarity of the reset bit. Should be 1 for resets
+		               that are asserted when the bit is set, 0 for
+		               resets that are asserted when the bit is cleared
+
+Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
+common reset controller usage by consumers.
+
+
+Example:
+--------
+The following example demonstrates a syscon node, the reset controller node
+using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
+Hawking SoC.
+
+/ {
+	soc {
+		psc: power-sleep-controller@02350000 {
+			compatible = "syscon";
+			reg = <0x02350000 0x1000>;
+		};
+
+		pscrst: psc-reset {
+			compatible = "syscon-reset";
+			syscon = <&psc>;
+			#reset-cells = <6>;
+		};
+
+		dsp0: dsp0 {
+			...
+			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
+			...
+		};
+	};
+};
-- 
2.7.0

[PATCH 2/2] reset: add a SYSCON based reset driver

[Andrew F. Davis]

Add a reset-controller driver for performing reset management of
various devices present on the SoC, with the reset registers shared
between devices in a common register memory space. This driver uses
the syscon/regmap frameworks to actually implement the various reset
functionalities needed by the reset consumer devices.

Signed-off-by: Andrew F. Davis <afd@ti.com>
[s-anna@ti.com: add documentation, syscon name change]
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/reset/Kconfig        |  10 ++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-syscon.c | 259 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/reset/reset-syscon.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index df37212..5f26755 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -12,5 +12,15 @@ menuconfig RESET_CONTROLLER
 
 	  If unsure, say no.
 
+config SYSCON_RESET
+	tristate "SYSCON Reset Driver"
+	depends on RESET_CONTROLLER
+	select MFD_SYSCON
+	help
+	  This enables the reset driver support for devices with memory-mapped
+	  reset registers as part of a syscon device node. If you wish to use
+	  the reset framework for such memory-mapped devices, say Y here.
+	  Otherwise, say N.
+
 source "drivers/reset/sti/Kconfig"
 source "drivers/reset/hisilicon/Kconfig"
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 4d7178e..6b3f6e3 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_HISI) += hisilicon/
 obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
 obj-$(CONFIG_ATH79) += reset-ath79.o
+obj-$(CONFIG_SYSCON_RESET) += reset-syscon.o
diff --git a/drivers/reset/reset-syscon.c b/drivers/reset/reset-syscon.c
new file mode 100644
index 0000000..48c8c30
--- /dev/null
+++ b/drivers/reset/reset-syscon.c
@@ -0,0 +1,259 @@
+/*
+ * SYSCON regmap reset driver
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andrew F. Davis <afd@ti.com>
+ *	Suman Anna <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/idr.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+/**
+ * struct syscon_reset_control - reset control structure
+ * @offset: reset control register offset from syscon base
+ * @reset_bit: reset bit in the reset control register
+ * @assert_high: flag to indicate if setting the bit high asserts the reset
+ * @status_offset: reset status register offset from syscon base
+ * @status_reset_bit: reset status bit in the reset status register
+ * @status_assert_high: flag to indicate if a set bit represents asserted state
+ */
+struct syscon_reset_control {
+	unsigned int offset;
+	unsigned int reset_bit;
+	bool assert_high;
+	unsigned int status_offset;
+	unsigned int status_reset_bit;
+	bool status_assert_high;
+};
+
+/**
+ * struct syscon_reset_data - reset controller information structure
+ * @rcdev: reset controller entity
+ * @dev: reset controller device pointer
+ * @memory: regmap handle containing the memory-mapped reset registers
+ * @idr: idr structure for mapping ids to reset control structures
+ */
+struct syscon_reset_data {
+	struct reset_controller_dev rcdev;
+	struct device *dev;
+	struct regmap *memory;
+	struct idr idr;
+};
+
+#define to_syscon_reset_data(rcdev)	\
+	container_of(rcdev, struct syscon_reset_data, rcdev)
+
+/**
+ * syscon_reset_set() - program a device's reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to toggle
+ * @assert: boolean flag to indicate assert or deassert
+ *
+ * This is a common internal function used to assert or deassert a device's
+ * reset using the regmap API. The device's reset is asserted if the @assert
+ * argument is true, or deasserted if the @assert argument is false.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_set(struct reset_controller_dev *rcdev,
+			    unsigned long id, bool assert)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+	unsigned int mask, value;
+
+	control = idr_find(&data->idr, id);
+	if (!control)
+		return -EINVAL;
+
+	mask = BIT(control->reset_bit);
+	value = (assert == control->assert_high) ? mask : 0x0;
+
+	return regmap_update_bits(data->memory, control->offset, mask, value);
+}
+
+/**
+ * syscon_reset_assert() - assert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to be asserted
+ *
+ * This function implements the reset driver op to assert a device's reset.
+ * This invokes the function syscon_reset_set() with the corresponding
+ * parameters as passed in, but with the @assert argument set to true for
+ * asserting the reset.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	return syscon_reset_set(rcdev, id, true);
+}
+
+/**
+ * syscon_reset_deassert() - deassert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of reset to be deasserted
+ *
+ * This function implements the reset driver op to deassert a device's reset.
+ * This invokes the function syscon_reset_set() with the corresponding
+ * parameters as passed in, but with the @assert argument set to false for
+ * deasserting the reset.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	return syscon_reset_set(rcdev, id, false);
+}
+
+/**
+ * syscon_reset_status() - check device reset status
+ * @rcdev: reset controller entity
+ * @id: ID of the reset for which the status is being requested
+ *
+ * This function implements the reset driver op to return the status of a
+ * device's reset.
+ *
+ * Return: 0 if reset is deasserted, or a non-zero value if reset is asserted
+ */
+static int syscon_reset_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+	unsigned int reset_state;
+	int ret;
+
+	control = idr_find(&data->idr, id);
+	if (!control)
+		return -EINVAL;
+
+	ret = regmap_read(data->memory, control->status_offset, &reset_state);
+	if (ret)
+		return ret;
+
+	return (reset_state & BIT(control->status_reset_bit)) ==
+			control->status_assert_high;
+}
+
+static struct reset_control_ops syscon_reset_ops = {
+	.assert		= syscon_reset_assert,
+	.deassert	= syscon_reset_deassert,
+	.status		= syscon_reset_status,
+};
+
+/**
+ * syscon_reset_of_xlate() - translate a set of OF arguments to a reset ID
+ * @rcdev: reset controller entity
+ * @reset_spec: OF reset argument specifier
+ *
+ * This function performs the translation of the reset argument specifier
+ * values defined in a reset consumer device node. The function allocates a
+ * reset control structure for that device reset, that will be used by the
+ * driver for performing any reset functions on that reset. An idr structure
+ * is allocated and used to map to the reset control structure. This idr is
+ * used by the driver to do reset lookups.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_of_xlate(struct reset_controller_dev *rcdev,
+				 const struct of_phandle_args *reset_spec)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+
+	if (WARN_ON(reset_spec->args_count != 6))
+		return -EINVAL;
+
+	control = devm_kzalloc(data->dev, sizeof(*control), GFP_KERNEL);
+	if (!control)
+		return -ENOMEM;
+
+	control->offset = reset_spec->args[0];
+	control->reset_bit = reset_spec->args[1];
+	control->assert_high = reset_spec->args[2] == 1;
+	control->status_offset = reset_spec->args[3];
+	control->status_reset_bit = reset_spec->args[4];
+	control->status_assert_high = reset_spec->args[5] == 1;
+
+	return idr_alloc(&data->idr, control, 0, 0, GFP_KERNEL);
+}
+
+static int syscon_reset_probe(struct platform_device *pdev)
+{
+	struct syscon_reset_data *data;
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *memory;
+
+	if (!np)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	memory = syscon_regmap_lookup_by_phandle(np, "syscon");
+	if (IS_ERR(memory))
+		return PTR_ERR(memory);
+
+	data->rcdev.ops = &syscon_reset_ops;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.of_node = np;
+	data->rcdev.of_xlate = syscon_reset_of_xlate;
+	data->dev = &pdev->dev;
+	data->memory = memory;
+	idr_init(&data->idr);
+
+	platform_set_drvdata(pdev, data);
+
+	return reset_controller_register(&data->rcdev);
+}
+
+static int syscon_reset_remove(struct platform_device *pdev)
+{
+	struct syscon_reset_data *data = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&data->rcdev);
+
+	idr_destroy(&data->idr);
+
+	return 0;
+}
+
+static const struct of_device_id syscon_reset_of_match_table[] = {
+	{ .compatible = "syscon-reset", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, syscon_reset_of_match_table);
+
+static struct platform_driver syscon_reset_driver = {
+	.probe = syscon_reset_probe,
+	.remove = syscon_reset_remove,
+	.driver = {
+		.name = "syscon-reset",
+		.of_match_table = syscon_reset_of_match_table,
+	},
+};
+module_platform_driver(syscon_reset_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
+MODULE_DESCRIPTION("SYSCON Regmap Reset Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

[PATCH 2/2] reset: add a SYSCON based reset driver

[Andrew F. Davis]

Add a reset-controller driver for performing reset management of
various devices present on the SoC, with the reset registers shared
between devices in a common register memory space. This driver uses
the syscon/regmap frameworks to actually implement the various reset
functionalities needed by the reset consumer devices.

Signed-off-by: Andrew F. Davis <afd@ti.com>
[s-anna@ti.com: add documentation, syscon name change]
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/reset/Kconfig        |  10 ++
 drivers/reset/Makefile       |   1 +
 drivers/reset/reset-syscon.c | 259 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/reset/reset-syscon.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index df37212..5f26755 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -12,5 +12,15 @@ menuconfig RESET_CONTROLLER
 
 	  If unsure, say no.
 
+config SYSCON_RESET
+	tristate "SYSCON Reset Driver"
+	depends on RESET_CONTROLLER
+	select MFD_SYSCON
+	help
+	  This enables the reset driver support for devices with memory-mapped
+	  reset registers as part of a syscon device node. If you wish to use
+	  the reset framework for such memory-mapped devices, say Y here.
+	  Otherwise, say N.
+
 source "drivers/reset/sti/Kconfig"
 source "drivers/reset/hisilicon/Kconfig"
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 4d7178e..6b3f6e3 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_HISI) += hisilicon/
 obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
 obj-$(CONFIG_ATH79) += reset-ath79.o
+obj-$(CONFIG_SYSCON_RESET) += reset-syscon.o
diff --git a/drivers/reset/reset-syscon.c b/drivers/reset/reset-syscon.c
new file mode 100644
index 0000000..48c8c30
--- /dev/null
+++ b/drivers/reset/reset-syscon.c
@@ -0,0 +1,259 @@
+/*
+ * SYSCON regmap reset driver
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *	Andrew F. Davis <afd@ti.com>
+ *	Suman Anna <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/idr.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+/**
+ * struct syscon_reset_control - reset control structure
+ * @offset: reset control register offset from syscon base
+ * @reset_bit: reset bit in the reset control register
+ * @assert_high: flag to indicate if setting the bit high asserts the reset
+ * @status_offset: reset status register offset from syscon base
+ * @status_reset_bit: reset status bit in the reset status register
+ * @status_assert_high: flag to indicate if a set bit represents asserted state
+ */
+struct syscon_reset_control {
+	unsigned int offset;
+	unsigned int reset_bit;
+	bool assert_high;
+	unsigned int status_offset;
+	unsigned int status_reset_bit;
+	bool status_assert_high;
+};
+
+/**
+ * struct syscon_reset_data - reset controller information structure
+ * @rcdev: reset controller entity
+ * @dev: reset controller device pointer
+ * @memory: regmap handle containing the memory-mapped reset registers
+ * @idr: idr structure for mapping ids to reset control structures
+ */
+struct syscon_reset_data {
+	struct reset_controller_dev rcdev;
+	struct device *dev;
+	struct regmap *memory;
+	struct idr idr;
+};
+
+#define to_syscon_reset_data(rcdev)	\
+	container_of(rcdev, struct syscon_reset_data, rcdev)
+
+/**
+ * syscon_reset_set() - program a device's reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to toggle
+ * @assert: boolean flag to indicate assert or deassert
+ *
+ * This is a common internal function used to assert or deassert a device's
+ * reset using the regmap API. The device's reset is asserted if the @assert
+ * argument is true, or deasserted if the @assert argument is false.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_set(struct reset_controller_dev *rcdev,
+			    unsigned long id, bool assert)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+	unsigned int mask, value;
+
+	control = idr_find(&data->idr, id);
+	if (!control)
+		return -EINVAL;
+
+	mask = BIT(control->reset_bit);
+	value = (assert == control->assert_high) ? mask : 0x0;
+
+	return regmap_update_bits(data->memory, control->offset, mask, value);
+}
+
+/**
+ * syscon_reset_assert() - assert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to be asserted
+ *
+ * This function implements the reset driver op to assert a device's reset.
+ * This invokes the function syscon_reset_set() with the corresponding
+ * parameters as passed in, but with the @assert argument set to true for
+ * asserting the reset.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	return syscon_reset_set(rcdev, id, true);
+}
+
+/**
+ * syscon_reset_deassert() - deassert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of reset to be deasserted
+ *
+ * This function implements the reset driver op to deassert a device's reset.
+ * This invokes the function syscon_reset_set() with the corresponding
+ * parameters as passed in, but with the @assert argument set to false for
+ * deasserting the reset.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	return syscon_reset_set(rcdev, id, false);
+}
+
+/**
+ * syscon_reset_status() - check device reset status
+ * @rcdev: reset controller entity
+ * @id: ID of the reset for which the status is being requested
+ *
+ * This function implements the reset driver op to return the status of a
+ * device's reset.
+ *
+ * Return: 0 if reset is deasserted, or a non-zero value if reset is asserted
+ */
+static int syscon_reset_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+	unsigned int reset_state;
+	int ret;
+
+	control = idr_find(&data->idr, id);
+	if (!control)
+		return -EINVAL;
+
+	ret = regmap_read(data->memory, control->status_offset, &reset_state);
+	if (ret)
+		return ret;
+
+	return (reset_state & BIT(control->status_reset_bit)) ==
+			control->status_assert_high;
+}
+
+static struct reset_control_ops syscon_reset_ops = {
+	.assert		= syscon_reset_assert,
+	.deassert	= syscon_reset_deassert,
+	.status		= syscon_reset_status,
+};
+
+/**
+ * syscon_reset_of_xlate() - translate a set of OF arguments to a reset ID
+ * @rcdev: reset controller entity
+ * @reset_spec: OF reset argument specifier
+ *
+ * This function performs the translation of the reset argument specifier
+ * values defined in a reset consumer device node. The function allocates a
+ * reset control structure for that device reset, that will be used by the
+ * driver for performing any reset functions on that reset. An idr structure
+ * is allocated and used to map to the reset control structure. This idr is
+ * used by the driver to do reset lookups.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int syscon_reset_of_xlate(struct reset_controller_dev *rcdev,
+				 const struct of_phandle_args *reset_spec)
+{
+	struct syscon_reset_data *data = to_syscon_reset_data(rcdev);
+	struct syscon_reset_control *control;
+
+	if (WARN_ON(reset_spec->args_count != 6))
+		return -EINVAL;
+
+	control = devm_kzalloc(data->dev, sizeof(*control), GFP_KERNEL);
+	if (!control)
+		return -ENOMEM;
+
+	control->offset = reset_spec->args[0];
+	control->reset_bit = reset_spec->args[1];
+	control->assert_high = reset_spec->args[2] == 1;
+	control->status_offset = reset_spec->args[3];
+	control->status_reset_bit = reset_spec->args[4];
+	control->status_assert_high = reset_spec->args[5] == 1;
+
+	return idr_alloc(&data->idr, control, 0, 0, GFP_KERNEL);
+}
+
+static int syscon_reset_probe(struct platform_device *pdev)
+{
+	struct syscon_reset_data *data;
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *memory;
+
+	if (!np)
+		return -ENODEV;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	memory = syscon_regmap_lookup_by_phandle(np, "syscon");
+	if (IS_ERR(memory))
+		return PTR_ERR(memory);
+
+	data->rcdev.ops = &syscon_reset_ops;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.of_node = np;
+	data->rcdev.of_xlate = syscon_reset_of_xlate;
+	data->dev = &pdev->dev;
+	data->memory = memory;
+	idr_init(&data->idr);
+
+	platform_set_drvdata(pdev, data);
+
+	return reset_controller_register(&data->rcdev);
+}
+
+static int syscon_reset_remove(struct platform_device *pdev)
+{
+	struct syscon_reset_data *data = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&data->rcdev);
+
+	idr_destroy(&data->idr);
+
+	return 0;
+}
+
+static const struct of_device_id syscon_reset_of_match_table[] = {
+	{ .compatible = "syscon-reset", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, syscon_reset_of_match_table);
+
+static struct platform_driver syscon_reset_driver = {
+	.probe = syscon_reset_probe,
+	.remove = syscon_reset_remove,
+	.driver = {
+		.name = "syscon-reset",
+		.of_match_table = syscon_reset_of_match_table,
+	},
+};
+module_platform_driver(syscon_reset_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
+MODULE_DESCRIPTION("SYSCON Regmap Reset Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

Re: [PATCH 2/2] reset: add a SYSCON based reset driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Andrew,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.5-rc1 next-20160125]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andrew-F-Davis/Add-support-for-SYSCON-reset/20160126-030611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: um-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um 

All warnings (new ones prefixed by >>):

warning: (ST_IRQCHIP && SYSCON_RESET && HIP04_ETH && STMMAC_PLATFORM && DWMAC_IPQ806X && DWMAC_LPC18XX && DWMAC_ROCKCHIP && DWMAC_SOCFPGA && DWMAC_STI && TI_CPSW && PINCTRL_ROCKCHIP && PINCTRL_DOVE && POWER_RESET_KEYSTONE && POWER_RESET_SYSCON && POWER_RESET_SYSCON_POWEROFF && S3C2410_WATCHDOG && VIDEO_OMAP3 && VIDEO_S5P_FIMC && USB_XHCI_MTK && RTC_DRV_AT91SAM9 && LPC18XX_DMAMUX && VIDEO_OMAP4 && HWSPINLOCK_QCOM && ATMEL_ST && QCOM_GSBI && PHY_HI6220_USB) selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 17794 bytes --]

Re: [PATCH 2/2] reset: add a SYSCON based reset driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Andrew,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.5-rc1 next-20160125]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andrew-F-Davis/Add-support-for-SYSCON-reset/20160126-030611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: um-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um 

All warnings (new ones prefixed by >>):

warning: (ST_IRQCHIP && SYSCON_RESET && HIP04_ETH && STMMAC_PLATFORM && DWMAC_IPQ806X && DWMAC_LPC18XX && DWMAC_ROCKCHIP && DWMAC_SOCFPGA && DWMAC_STI && TI_CPSW && PINCTRL_ROCKCHIP && PINCTRL_DOVE && POWER_RESET_KEYSTONE && POWER_RESET_SYSCON && POWER_RESET_SYSCON_POWEROFF && S3C2410_WATCHDOG && VIDEO_OMAP3 && VIDEO_S5P_FIMC && USB_XHCI_MTK && RTC_DRV_AT91SAM9 && LPC18XX_DMAMUX && VIDEO_OMAP4 && HWSPINLOCK_QCOM && ATMEL_ST && QCOM_GSBI && PHY_HI6220_USB) selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 17794 bytes --]

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Rob Herring]

On Mon, Jan 25, 2016 at 01:02:43PM -0600, Andrew F. Davis wrote:
> Add syscon reset controller binding. This will hook to the reset
> framework and use syscon/regmap to set reset bits. This allows
> reset control of individual SoC subsytems and devices with
> memory-mapped reset registers in a common register memory
> space.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> [s-anna@ti.com: revise the binding format]
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
> new file mode 100644
> index 0000000..466378a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
> @@ -0,0 +1,84 @@
> +SysCon Reset Controller
> +=======================
> +
> +Almost all SoCs have hardware modules that require reset control in addition
> +to clock and power control for their functionality. The reset control is
> +typically provided by means of memory-mapped I/O registers. These registers are
> +sometimes a part of a larger register space region implementing various
> +functionalities. This register range is best represented as a syscon node to
> +allow multiple entities to access their relevant registers in the common
> +register space.
> +
> +A SysCon Reset Controller node defines a device that uses a syscon node
> +and provides reset management functionality for various hardware modules
> +present on the SoC.

This may be one of those cases that is too low level to put into DT.

> +
> +SysCon Reset Controller Node
> +============================
> +Each of the reset provider/controller nodes should have the following
> +properties.
> +
> +Required properties:
> +--------------------
> + - compatible	: Should be "syscon-reset"
> + - syscon	: phandle to the syscon node containing the reset registers
> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
> +                  usage details
> +
> +SysCon Reset Consumer Nodes
> +===========================
> +Each of the reset consumer nodes should have the following properties,
> +in addition to their own properties.
> +
> +Required properties:
> +--------------------
> + - resets	: A phandle and reset specifier pair, one pair for each reset
> +		  signal that affects the device, or that the device manages.
> +		  The phandle should point to the syscon node containing the
> +		  reset registers, and the reset specifier should have 6
> +		  cell-values. The reset specifier contains two similar pairs
> +		  of 3 cell-values each, the first of the pair containing the
> +		  reset control register information, and the second of the pair
> +		  containing the reset status register information. The reset
> +		  control and status registers can be same on some devices/SoCs.

What if there are no status bits?

> +
> +		  Each of the pairs of 3 cell-values should have the following
> +		  values:
> +		     Cell #1 : register offset of the reset control/status
> +		               register from the syscon register base
> +		     Cell #2 : bit shift value for the reset in the respective
> +		               reset control/status register
> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
> +		               that are asserted when the bit is set, 0 for
> +		               resets that are asserted when the bit is cleared
> +
> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
> +common reset controller usage by consumers.
> +
> +
> +Example:
> +--------
> +The following example demonstrates a syscon node, the reset controller node
> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
> +Hawking SoC.
> +
> +/ {
> +	soc {
> +		psc: power-sleep-controller@02350000 {
> +			compatible = "syscon";
> +			reg = <0x02350000 0x1000>;
> +		};
> +
> +		pscrst: psc-reset {
> +			compatible = "syscon-reset";
> +			syscon = <&psc>;
> +			#reset-cells = <6>;
> +		};

Any reason not to make this a child of psc?

> +
> +		dsp0: dsp0 {
> +			...
> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
> +			...
> +		};
> +	};
> +};
> -- 
> 2.7.0
> 

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 01/28/2016 09:22 PM, Rob Herring wrote:
> On Mon, Jan 25, 2016 at 01:02:43PM -0600, Andrew F. Davis wrote:
>> Add syscon reset controller binding. This will hook to the reset
>> framework and use syscon/regmap to set reset bits. This allows
>> reset control of individual SoC subsytems and devices with
>> memory-mapped reset registers in a common register memory
>> space.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> [s-anna@ti.com: revise the binding format]
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>   .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> new file mode 100644
>> index 0000000..466378a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> @@ -0,0 +1,84 @@
>> +SysCon Reset Controller
>> +=======================
>> +
>> +Almost all SoCs have hardware modules that require reset control in addition
>> +to clock and power control for their functionality. The reset control is
>> +typically provided by means of memory-mapped I/O registers. These registers are
>> +sometimes a part of a larger register space region implementing various
>> +functionalities. This register range is best represented as a syscon node to
>> +allow multiple entities to access their relevant registers in the common
>> +register space.
>> +
>> +A SysCon Reset Controller node defines a device that uses a syscon node
>> +and provides reset management functionality for various hardware modules
>> +present on the SoC.
>
> This may be one of those cases that is too low level to put into DT.
>

I can understand the worry about directly representing hardware register
functionality in DT, but I believe this case is a useful abstraction,
otherwise we end up with reset driver mods for every minor spin of a chip.

>> +
>> +SysCon Reset Controller Node
>> +============================
>> +Each of the reset provider/controller nodes should have the following
>> +properties.
>> +
>> +Required properties:
>> +--------------------
>> + - compatible	: Should be "syscon-reset"
>> + - syscon	: phandle to the syscon node containing the reset registers
>> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
>> +                  usage details
>> +
>> +SysCon Reset Consumer Nodes
>> +===========================
>> +Each of the reset consumer nodes should have the following properties,
>> +in addition to their own properties.
>> +
>> +Required properties:
>> +--------------------
>> + - resets	: A phandle and reset specifier pair, one pair for each reset
>> +		  signal that affects the device, or that the device manages.
>> +		  The phandle should point to the syscon node containing the
>> +		  reset registers, and the reset specifier should have 6
>> +		  cell-values. The reset specifier contains two similar pairs
>> +		  of 3 cell-values each, the first of the pair containing the
>> +		  reset control register information, and the second of the pair
>> +		  containing the reset status register information. The reset
>> +		  control and status registers can be same on some devices/SoCs.
>
> What if there are no status bits?
>

The last three can be the same as the first three, then if status is read we
will check if the corresponding control register bit has been set (which may
be set as non-volatile, then the cached result from the last write is read).

If there is absolutely no way to read status information then that chip will
need a less generic driver solution, but I have not seen any like this.

>> +
>> +		  Each of the pairs of 3 cell-values should have the following
>> +		  values:
>> +		     Cell #1 : register offset of the reset control/status
>> +		               register from the syscon register base
>> +		     Cell #2 : bit shift value for the reset in the respective
>> +		               reset control/status register
>> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
>> +		               that are asserted when the bit is set, 0 for
>> +		               resets that are asserted when the bit is cleared
>> +
>> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
>> +common reset controller usage by consumers.
>> +
>> +
>> +Example:
>> +--------
>> +The following example demonstrates a syscon node, the reset controller node
>> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
>> +Hawking SoC.
>> +
>> +/ {
>> +	soc {
>> +		psc: power-sleep-controller@02350000 {
>> +			compatible = "syscon";
>> +			reg = <0x02350000 0x1000>;
>> +		};
>> +
>> +		pscrst: psc-reset {
>> +			compatible = "syscon-reset";
>> +			syscon = <&psc>;
>> +			#reset-cells = <6>;
>> +		};
>
> Any reason not to make this a child of psc?
>

I have not tested that, but if it gets registered the same I see no reason
it couldn't be if someone wanted it organized that way.

>> +
>> +		dsp0: dsp0 {
>> +			...
>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>> +			...
>> +		};
>> +	};
>> +};
>> --
>> 2.7.0
>>

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 01/28/2016 09:22 PM, Rob Herring wrote:
> On Mon, Jan 25, 2016 at 01:02:43PM -0600, Andrew F. Davis wrote:
>> Add syscon reset controller binding. This will hook to the reset
>> framework and use syscon/regmap to set reset bits. This allows
>> reset control of individual SoC subsytems and devices with
>> memory-mapped reset registers in a common register memory
>> space.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> [s-anna@ti.com: revise the binding format]
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>   .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> new file mode 100644
>> index 0000000..466378a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> @@ -0,0 +1,84 @@
>> +SysCon Reset Controller
>> +=======================
>> +
>> +Almost all SoCs have hardware modules that require reset control in addition
>> +to clock and power control for their functionality. The reset control is
>> +typically provided by means of memory-mapped I/O registers. These registers are
>> +sometimes a part of a larger register space region implementing various
>> +functionalities. This register range is best represented as a syscon node to
>> +allow multiple entities to access their relevant registers in the common
>> +register space.
>> +
>> +A SysCon Reset Controller node defines a device that uses a syscon node
>> +and provides reset management functionality for various hardware modules
>> +present on the SoC.
>
> This may be one of those cases that is too low level to put into DT.
>

I can understand the worry about directly representing hardware register
functionality in DT, but I believe this case is a useful abstraction,
otherwise we end up with reset driver mods for every minor spin of a chip.

>> +
>> +SysCon Reset Controller Node
>> +============================
>> +Each of the reset provider/controller nodes should have the following
>> +properties.
>> +
>> +Required properties:
>> +--------------------
>> + - compatible	: Should be "syscon-reset"
>> + - syscon	: phandle to the syscon node containing the reset registers
>> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
>> +                  usage details
>> +
>> +SysCon Reset Consumer Nodes
>> +===========================
>> +Each of the reset consumer nodes should have the following properties,
>> +in addition to their own properties.
>> +
>> +Required properties:
>> +--------------------
>> + - resets	: A phandle and reset specifier pair, one pair for each reset
>> +		  signal that affects the device, or that the device manages.
>> +		  The phandle should point to the syscon node containing the
>> +		  reset registers, and the reset specifier should have 6
>> +		  cell-values. The reset specifier contains two similar pairs
>> +		  of 3 cell-values each, the first of the pair containing the
>> +		  reset control register information, and the second of the pair
>> +		  containing the reset status register information. The reset
>> +		  control and status registers can be same on some devices/SoCs.
>
> What if there are no status bits?
>

The last three can be the same as the first three, then if status is read we
will check if the corresponding control register bit has been set (which may
be set as non-volatile, then the cached result from the last write is read).

If there is absolutely no way to read status information then that chip will
need a less generic driver solution, but I have not seen any like this.

>> +
>> +		  Each of the pairs of 3 cell-values should have the following
>> +		  values:
>> +		     Cell #1 : register offset of the reset control/status
>> +		               register from the syscon register base
>> +		     Cell #2 : bit shift value for the reset in the respective
>> +		               reset control/status register
>> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
>> +		               that are asserted when the bit is set, 0 for
>> +		               resets that are asserted when the bit is cleared
>> +
>> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
>> +common reset controller usage by consumers.
>> +
>> +
>> +Example:
>> +--------
>> +The following example demonstrates a syscon node, the reset controller node
>> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
>> +Hawking SoC.
>> +
>> +/ {
>> +	soc {
>> +		psc: power-sleep-controller@02350000 {
>> +			compatible = "syscon";
>> +			reg = <0x02350000 0x1000>;
>> +		};
>> +
>> +		pscrst: psc-reset {
>> +			compatible = "syscon-reset";
>> +			syscon = <&psc>;
>> +			#reset-cells = <6>;
>> +		};
>
> Any reason not to make this a child of psc?
>

I have not tested that, but if it gets registered the same I see no reason
it couldn't be if someone wanted it organized that way.

>> +
>> +		dsp0: dsp0 {
>> +			...
>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>> +			...
>> +		};
>> +	};
>> +};
>> --
>> 2.7.0
>>

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Philipp Zabel]

Hi Andrew,

I like the idea to introduce a generic binding in principle, but it
should be able to cover a lot of the cases in the wild. And I'm not sure
we know this to be the case yet.

Currently we have three syscon users in drivers/reset: reset-berlin,
reset-zynq, and sti/reset-syscfg.
berlin is special in that it only has trigger bits, and no state.
zynq would fit this binding, but it already chose a binding with a
single address cell because all its resets are in a contiguous range and
the state and control bits are in the same place.
sti also chose a single address cell and a logical number to enumerate
the resets and to store the actual reset control and status bit position
in a table in the driver. Is there a reason not to follow the same
approach for ti? This approach of specifying bits in the device tree at
the consumer side doesn't allow any error handling in the driver to
determine if the bits are in fact valid.

Am Montag, den 25.01.2016, 13:02 -0600 schrieb Andrew F. Davis:
> Add syscon reset controller binding. This will hook to the reset
> framework and use syscon/regmap to set reset bits. This allows
> reset control of individual SoC subsytems and devices with
> memory-mapped reset registers in a common register memory
> space.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> [s-anna@ti.com: revise the binding format]
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
> new file mode 100644
> index 0000000..466378a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
> @@ -0,0 +1,84 @@
> +SysCon Reset Controller
> +=======================
> +
> +Almost all SoCs have hardware modules that require reset control in addition
> +to clock and power control for their functionality. The reset control is
> +typically provided by means of memory-mapped I/O registers. These registers are
> +sometimes a part of a larger register space region implementing various
> +functionalities. This register range is best represented as a syscon node to
> +allow multiple entities to access their relevant registers in the common
> +register space.

So far, so good.

> +A SysCon Reset Controller node defines a device that uses a syscon node
> +and provides reset management functionality for various hardware modules
> +present on the SoC.

But this wording is a bit strange when there is no device.

> +
> +SysCon Reset Controller Node
> +============================
> +Each of the reset provider/controller nodes should have the following
> +properties.
> +
> +Required properties:
> +--------------------
> + - compatible	: Should be "syscon-reset"

What if later an erratum turns up and something special needs to be done
(delays, special care about other bits in the same register, etc.)?
This should always contain a soc specific compatible.

> + - syscon	: phandle to the syscon node containing the reset registers

This is not needed, the reset node can be mad a child of the syscon and
then grab the regmap from its parent of_node.

> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
> +                  usage details


This is a binding for single reset bits that are spread throughout the
register space. For any syscon that has a few registers of contiguous
resets this is rather suboptimal.

> +SysCon Reset Consumer Nodes
> +===========================
> +Each of the reset consumer nodes should have the following properties,
> +in addition to their own properties.
> +
> +Required properties:
> +--------------------
> + - resets	: A phandle and reset specifier pair, one pair for each reset
> +		  signal that affects the device, or that the device manages.
> +		  The phandle should point to the syscon node containing the
> +		  reset registers, and the reset specifier should have 6
> +		  cell-values. The reset specifier contains two similar pairs

One pair, then, not two?

> +		  of 3 cell-values each, the first of the pair containing the
> +		  reset control register information, and the second of the pair
> +		  containing the reset status register information. The reset
> +		  control and status registers can be same on some devices/SoCs.
> +
> +		  Each of the pairs of 3 cell-values should have the following
> +		  values:
> +		     Cell #1 : register offset of the reset control/status
> +		               register from the syscon register base
> +		     Cell #2 : bit shift value for the reset in the respective
> +		               reset control/status register
> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
> +		               that are asserted when the bit is set, 0 for
> +		               resets that are asserted when the bit is cleared

The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
numbers in the gpio phandle bindings in the beginning has caused a lot
of problems over time.
Do you really have varying polarity across the resets?

> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
> +common reset controller usage by consumers.
> +
> +Example:
> +--------
> +The following example demonstrates a syscon node, the reset controller node
> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
> +Hawking SoC.
> +
> +/ {
> +	soc {
> +		psc: power-sleep-controller@02350000 {
> +			compatible = "syscon";

Add "simple-mfd", and then ...

> +			reg = <0x02350000 0x1000>;
> +		};
> +
> +		pscrst: psc-reset {
> +			compatible = "syscon-reset";
> +			syscon = <&psc>;
> +			#reset-cells = <6>;
> +		};

... psc-reset can be put inside power-sleep-controller, and the syscon
property can be removed.

> +		dsp0: dsp0 {
> +			...
> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;

This sounds a bit error prone and rather verbose for all controllers
that don't have control and status bits peppered randomly around the
register space.

Personally, I'd prefer if you chose to take the sti approach and maybe
share code with reset-syscfg.

regards
Philipp

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 02/02/2016 10:44 AM, Philipp Zabel wrote:
> Hi Andrew,
>
> I like the idea to introduce a generic binding in principle, but it
> should be able to cover a lot of the cases in the wild. And I'm not sure
> we know this to be the case yet.
>
> Currently we have three syscon users in drivers/reset: reset-berlin,
> reset-zynq, and sti/reset-syscfg.
> berlin is special in that it only has trigger bits, and no state.
> zynq would fit this binding, but it already chose a binding with a
> single address cell because all its resets are in a contiguous range and
> the state and control bits are in the same place.
> sti also chose a single address cell and a logical number to enumerate
> the resets and to store the actual reset control and status bit position
> in a table in the driver. Is there a reason not to follow the same
> approach for ti?

The number of reset-able modules is rather large, and we would have to
have an entry for them, and then a table of them for each chip, I
would like to avoid this as it may become unmaintainable with the number
of devices we would like to support.

We currently only need to reset one module this way currently anyway, we
will be moving away from toggling bits from the host side to perform resets,
but rather ask a power management controller to perform the reset for us
(I also have a reset driver that communicates with this controller that I
will post when the rest of the needed framework is upstreamed). So, for TI,
this syscon based driver will probably mostly be used for compatibility with
older SoCs that do not support the management controller, allowing new device
drivers to use the reset framework and still function with older SoCs.

> This approach of specifying bits in the device tree at
> the consumer side doesn't allow any error handling in the driver to
> determine if the bits are in fact valid.
>

Yes, that is lost by not having a table of all valid resets, but this would
be like many DT driver/node that allow registers/addresses to be specified
and then write/read from those.

> Am Montag, den 25.01.2016, 13:02 -0600 schrieb Andrew F. Davis:
>> Add syscon reset controller binding. This will hook to the reset
>> framework and use syscon/regmap to set reset bits. This allows
>> reset control of individual SoC subsytems and devices with
>> memory-mapped reset registers in a common register memory
>> space.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> [s-anna@ti.com: revise the binding format]
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>   .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> new file mode 100644
>> index 0000000..466378a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> @@ -0,0 +1,84 @@
>> +SysCon Reset Controller
>> +=======================
>> +
>> +Almost all SoCs have hardware modules that require reset control in addition
>> +to clock and power control for their functionality. The reset control is
>> +typically provided by means of memory-mapped I/O registers. These registers are
>> +sometimes a part of a larger register space region implementing various
>> +functionalities. This register range is best represented as a syscon node to
>> +allow multiple entities to access their relevant registers in the common
>> +register space.
>
> So far, so good.
>
>> +A SysCon Reset Controller node defines a device that uses a syscon node
>> +and provides reset management functionality for various hardware modules
>> +present on the SoC.
>
> But this wording is a bit strange when there is no device.
>

Agreed, I'll clarify this.

>> +
>> +SysCon Reset Controller Node
>> +============================
>> +Each of the reset provider/controller nodes should have the following
>> +properties.
>> +
>> +Required properties:
>> +--------------------
>> + - compatible	: Should be "syscon-reset"
>
> What if later an erratum turns up and something special needs to be done
> (delays, special care about other bits in the same register, etc.)?
> This should always contain a soc specific compatible.
>

In this case a specific reset driver would be needed for that device, this
driver only intends to cover the more traditional base cases.

>> + - syscon	: phandle to the syscon node containing the reset registers
>
> This is not needed, the reset node can be mad a child of the syscon and
> then grab the regmap from its parent of_node.
>

Rob also made this suggestion, so I'll change this as it seems to be the
way the community would like to move forward with syscon based drivers.

>> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
>> +                  usage details
>
>
> This is a binding for single reset bits that are spread throughout the
> register space. For any syscon that has a few registers of contiguous
> resets this is rather suboptimal.
>

Yes, this is only intended for when a few resets need controlling out of
a large reset space without having to directly encode the reset information
into the device driver for that hardware module.

>> +SysCon Reset Consumer Nodes
>> +===========================
>> +Each of the reset consumer nodes should have the following properties,
>> +in addition to their own properties.
>> +
>> +Required properties:
>> +--------------------
>> + - resets	: A phandle and reset specifier pair, one pair for each reset
>> +		  signal that affects the device, or that the device manages.
>> +		  The phandle should point to the syscon node containing the
>> +		  reset registers, and the reset specifier should have 6
>> +		  cell-values. The reset specifier contains two similar pairs
>
> One pair, then, not two?
>

Err, two similar triples, may make more sense, will fix.

>> +		  of 3 cell-values each, the first of the pair containing the
>> +		  reset control register information, and the second of the pair
>> +		  containing the reset status register information. The reset
>> +		  control and status registers can be same on some devices/SoCs.
>> +
>> +		  Each of the pairs of 3 cell-values should have the following
>> +		  values:
>> +		     Cell #1 : register offset of the reset control/status
>> +		               register from the syscon register base
>> +		     Cell #2 : bit shift value for the reset in the respective
>> +		               reset control/status register
>> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
>> +		               that are asserted when the bit is set, 0 for
>> +		               resets that are asserted when the bit is cleared
>
> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
> numbers in the gpio phandle bindings in the beginning has caused a lot
> of problems over time.

That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?

> Do you really have varying polarity across the resets?
>

Not on the part I'm using, but this should keep things generic.

The alternative would be to set the polarity per reset controller node, then
if a system has both it could have two controllers and the consumers would
have a phandle to the correct one, then all consumers would only need 4
instead of 6 args. Actually now that I think about it, this is probably the
way to go as most systems I would imagine only have one polarity and it still
can work for systems that do have both. I think I'll make this change.

>> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
>> +common reset controller usage by consumers.
>> +
>> +Example:
>> +--------
>> +The following example demonstrates a syscon node, the reset controller node
>> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
>> +Hawking SoC.
>> +
>> +/ {
>> +	soc {
>> +		psc: power-sleep-controller@02350000 {
>> +			compatible = "syscon";
>
> Add "simple-mfd", and then ...
>
>> +			reg = <0x02350000 0x1000>;
>> +		};
>> +
>> +		pscrst: psc-reset {
>> +			compatible = "syscon-reset";
>> +			syscon = <&psc>;
>> +			#reset-cells = <6>;
>> +		};
>
> ... psc-reset can be put inside power-sleep-controller, and the syscon
> property can be removed.
>

Agreed, will change.

>> +		dsp0: dsp0 {
>> +			...
>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>
> This sounds a bit error prone and rather verbose for all controllers
> that don't have control and status bits peppered randomly around the
> register space.
>

I was also thinking about adding the ability to have only one set of args
for control, then we just return ENOTSUPP when asked for status when only
the control register is provided.

With the above polarity change, we end up allowing:

resets = <&pscrst 0xa3c 8>;

when appropriate. This would cover many common use-cases and keep the
framework clean for unique case drivers when needed. It would eliminate
the need for many reset-berlin like drivers that only differentiate
themselves in trivial ways, like offsets/polarity, etc..

Thanks,
Andrew

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 02/02/2016 10:44 AM, Philipp Zabel wrote:
> Hi Andrew,
>
> I like the idea to introduce a generic binding in principle, but it
> should be able to cover a lot of the cases in the wild. And I'm not sure
> we know this to be the case yet.
>
> Currently we have three syscon users in drivers/reset: reset-berlin,
> reset-zynq, and sti/reset-syscfg.
> berlin is special in that it only has trigger bits, and no state.
> zynq would fit this binding, but it already chose a binding with a
> single address cell because all its resets are in a contiguous range and
> the state and control bits are in the same place.
> sti also chose a single address cell and a logical number to enumerate
> the resets and to store the actual reset control and status bit position
> in a table in the driver. Is there a reason not to follow the same
> approach for ti?

The number of reset-able modules is rather large, and we would have to
have an entry for them, and then a table of them for each chip, I
would like to avoid this as it may become unmaintainable with the number
of devices we would like to support.

We currently only need to reset one module this way currently anyway, we
will be moving away from toggling bits from the host side to perform resets,
but rather ask a power management controller to perform the reset for us
(I also have a reset driver that communicates with this controller that I
will post when the rest of the needed framework is upstreamed). So, for TI,
this syscon based driver will probably mostly be used for compatibility with
older SoCs that do not support the management controller, allowing new device
drivers to use the reset framework and still function with older SoCs.

> This approach of specifying bits in the device tree at
> the consumer side doesn't allow any error handling in the driver to
> determine if the bits are in fact valid.
>

Yes, that is lost by not having a table of all valid resets, but this would
be like many DT driver/node that allow registers/addresses to be specified
and then write/read from those.

> Am Montag, den 25.01.2016, 13:02 -0600 schrieb Andrew F. Davis:
>> Add syscon reset controller binding. This will hook to the reset
>> framework and use syscon/regmap to set reset bits. This allows
>> reset control of individual SoC subsytems and devices with
>> memory-mapped reset registers in a common register memory
>> space.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> [s-anna@ti.com: revise the binding format]
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>   .../devicetree/bindings/reset/syscon-reset.txt     | 84 ++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> new file mode 100644
>> index 0000000..466378a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
>> @@ -0,0 +1,84 @@
>> +SysCon Reset Controller
>> +=======================
>> +
>> +Almost all SoCs have hardware modules that require reset control in addition
>> +to clock and power control for their functionality. The reset control is
>> +typically provided by means of memory-mapped I/O registers. These registers are
>> +sometimes a part of a larger register space region implementing various
>> +functionalities. This register range is best represented as a syscon node to
>> +allow multiple entities to access their relevant registers in the common
>> +register space.
>
> So far, so good.
>
>> +A SysCon Reset Controller node defines a device that uses a syscon node
>> +and provides reset management functionality for various hardware modules
>> +present on the SoC.
>
> But this wording is a bit strange when there is no device.
>

Agreed, I'll clarify this.

>> +
>> +SysCon Reset Controller Node
>> +============================
>> +Each of the reset provider/controller nodes should have the following
>> +properties.
>> +
>> +Required properties:
>> +--------------------
>> + - compatible	: Should be "syscon-reset"
>
> What if later an erratum turns up and something special needs to be done
> (delays, special care about other bits in the same register, etc.)?
> This should always contain a soc specific compatible.
>

In this case a specific reset driver would be needed for that device, this
driver only intends to cover the more traditional base cases.

>> + - syscon	: phandle to the syscon node containing the reset registers
>
> This is not needed, the reset node can be mad a child of the syscon and
> then grab the regmap from its parent of_node.
>

Rob also made this suggestion, so I'll change this as it seems to be the
way the community would like to move forward with syscon based drivers.

>> + - #reset-cells	: Should be 6. Please see the reset consumer node below for
>> +                  usage details
>
>
> This is a binding for single reset bits that are spread throughout the
> register space. For any syscon that has a few registers of contiguous
> resets this is rather suboptimal.
>

Yes, this is only intended for when a few resets need controlling out of
a large reset space without having to directly encode the reset information
into the device driver for that hardware module.

>> +SysCon Reset Consumer Nodes
>> +===========================
>> +Each of the reset consumer nodes should have the following properties,
>> +in addition to their own properties.
>> +
>> +Required properties:
>> +--------------------
>> + - resets	: A phandle and reset specifier pair, one pair for each reset
>> +		  signal that affects the device, or that the device manages.
>> +		  The phandle should point to the syscon node containing the
>> +		  reset registers, and the reset specifier should have 6
>> +		  cell-values. The reset specifier contains two similar pairs
>
> One pair, then, not two?
>

Err, two similar triples, may make more sense, will fix.

>> +		  of 3 cell-values each, the first of the pair containing the
>> +		  reset control register information, and the second of the pair
>> +		  containing the reset status register information. The reset
>> +		  control and status registers can be same on some devices/SoCs.
>> +
>> +		  Each of the pairs of 3 cell-values should have the following
>> +		  values:
>> +		     Cell #1 : register offset of the reset control/status
>> +		               register from the syscon register base
>> +		     Cell #2 : bit shift value for the reset in the respective
>> +		               reset control/status register
>> +		     Cell #3 : polarity of the reset bit. Should be 1 for resets
>> +		               that are asserted when the bit is set, 0 for
>> +		               resets that are asserted when the bit is cleared
>
> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
> numbers in the gpio phandle bindings in the beginning has caused a lot
> of problems over time.

That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?

> Do you really have varying polarity across the resets?
>

Not on the part I'm using, but this should keep things generic.

The alternative would be to set the polarity per reset controller node, then
if a system has both it could have two controllers and the consumers would
have a phandle to the correct one, then all consumers would only need 4
instead of 6 args. Actually now that I think about it, this is probably the
way to go as most systems I would imagine only have one polarity and it still
can work for systems that do have both. I think I'll make this change.

>> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
>> +common reset controller usage by consumers.
>> +
>> +Example:
>> +--------
>> +The following example demonstrates a syscon node, the reset controller node
>> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2
>> +Hawking SoC.
>> +
>> +/ {
>> +	soc {
>> +		psc: power-sleep-controller@02350000 {
>> +			compatible = "syscon";
>
> Add "simple-mfd", and then ...
>
>> +			reg = <0x02350000 0x1000>;
>> +		};
>> +
>> +		pscrst: psc-reset {
>> +			compatible = "syscon-reset";
>> +			syscon = <&psc>;
>> +			#reset-cells = <6>;
>> +		};
>
> ... psc-reset can be put inside power-sleep-controller, and the syscon
> property can be removed.
>

Agreed, will change.

>> +		dsp0: dsp0 {
>> +			...
>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>
> This sounds a bit error prone and rather verbose for all controllers
> that don't have control and status bits peppered randomly around the
> register space.
>

I was also thinking about adding the ability to have only one set of args
for control, then we just return ENOTSUPP when asked for status when only
the control register is provided.

With the above polarity change, we end up allowing:

resets = <&pscrst 0xa3c 8>;

when appropriate. This would cover many common use-cases and keep the
framework clean for unique case drivers when needed. It would eliminate
the need for many reset-berlin like drivers that only differentiate
themselves in trivial ways, like offsets/polarity, etc..

Thanks,
Andrew

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Philipp Zabel]

Hi Andrew,

Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis:
[...]
> > sti also chose a single address cell and a logical number to enumerate
> > the resets and to store the actual reset control and status bit position
> > in a table in the driver. Is there a reason not to follow the same
> > approach for ti?
> 
> The number of reset-able modules is rather large, and we would have to
> have an entry for them, and then a table of them for each chip, I
> would like to avoid this as it may become unmaintainable with the number
> of devices we would like to support.

Maybe I underestimate the amount of work necessary to translate the
register bit positions from the reference manuals into tables in a
driver (after all the imx-src driver that started this framework only
has 5 reset controls registered with it), but to me this doesn't sound
like a very strong argument.

> We currently only need to reset one module this way currently anyway, we
> will be moving away from toggling bits from the host side to perform resets,
> but rather ask a power management controller to perform the reset for us
> (I also have a reset driver that communicates with this controller that I
> will post when the rest of the needed framework is upstreamed). So, for TI,
> this syscon based driver will probably mostly be used for compatibility with
> older SoCs that do not support the management controller, allowing new device
> drivers to use the reset framework and still function with older SoCs.

So this will only be used for a few legacy devices? I'd probably be less
hesitant if you proposed this as some ti chip specific binding, as right
now I just don't expect that many other devices to use this supposedly
generic binding.

> > This approach of specifying bits in the device tree at
> > the consumer side doesn't allow any error handling in the driver to
> > determine if the bits are in fact valid.
> 
> Yes, that is lost by not having a table of all valid resets, but this would
> be like many DT driver/node that allow registers/addresses to be specified
> and then write/read from those.

True. Still, I'd argue that having a list of registers and bit-shifts in
a single place (whether that is in the driver or in the reset controller
node), that can be easily checked against a manual. On the other hand
sprinkling these values around the device tree at the consumer sites
makes this much harder to review.

[...]
> >> +Required properties:
> >> +--------------------
> >> + - compatible	: Should be "syscon-reset"
> >
> > What if later an erratum turns up and something special needs to be done
> > (delays, special care about other bits in the same register, etc.)?
> > This should always contain a soc specific compatible.
>
> In this case a specific reset driver would be needed for that device, this
> driver only intends to cover the more traditional base cases.

And for that to work in a backwards compatible way, you need to have the
SoC specific compatible value already in the device tree.

[...]
> > This is a binding for single reset bits that are spread throughout the
> > register space. For any syscon that has a few registers of contiguous
> > resets this is rather suboptimal.
>
> Yes, this is only intended for when a few resets need controlling out of
> a large reset space without having to directly encode the reset information
> into the device driver for that hardware module.

How about if we came up with a way to encode the bit fields in the reset
controller node?

[...]
> > The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
> > numbers in the gpio phandle bindings in the beginning has caused a lot
> > of problems over time.
> 
> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?

Yes, this is better.

> > Do you really have varying polarity across the resets?
> 
> Not on the part I'm using, but this should keep things generic.

We already established that this binding is not generic enough for most
of the current cases, so I'm not convinced it is valuable to complicate
it for some theoretical case.

> The alternative would be to set the polarity per reset controller node, then
> if a system has both it could have two controllers and the consumers would
> have a phandle to the correct one, then all consumers would only need 4
> instead of 6 args. Actually now that I think about it, this is probably the
> way to go as most systems I would imagine only have one polarity and it still
> can work for systems that do have both. I think I'll make this change.

How about going one more step and also moving the register and bit-shift
description into a property of the reset controller node?

> >> +		dsp0: dsp0 {
> >> +			...
> >> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
> >
> > This sounds a bit error prone and rather verbose for all controllers
> > that don't have control and status bits peppered randomly around the
> > register space.
> 
> I was also thinking about adding the ability to have only one set of args
> for control, then we just return ENOTSUPP when asked for status when only
> the control register is provided.

Yes, that should work.

> With the above polarity change, we end up allowing:
> 
> resets = <&pscrst 0xa3c 8>;

Would this be a reset controller with no control bit, but just a trigger
that is triggered when the bit is set? (or cleared?).

> when appropriate. This would cover many common use-cases and keep the
> framework clean for unique case drivers when needed. It would eliminate
> the need for many reset-berlin like drivers that only differentiate
> themselves in trivial ways, like offsets/polarity, etc..

That's not a good example. reset-berlin needs a bit to be set to trigger
the reset pulse, and doesn't have support for manual assert/deassert.
Also according to the driver it doesn't signal reset status, so there is
a fixed delay needed.
Honestly, I expect most drivers in the near future to fall into two
categories: either they need special attention, or they are not a good
fit for this binding because all the resets are (at least mostly)
contiguous.

regards
Philipp

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Philipp Zabel]

Hi Andrew,

Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis:
[...]
> > sti also chose a single address cell and a logical number to enumerate
> > the resets and to store the actual reset control and status bit position
> > in a table in the driver. Is there a reason not to follow the same
> > approach for ti?
> 
> The number of reset-able modules is rather large, and we would have to
> have an entry for them, and then a table of them for each chip, I
> would like to avoid this as it may become unmaintainable with the number
> of devices we would like to support.

Maybe I underestimate the amount of work necessary to translate the
register bit positions from the reference manuals into tables in a
driver (after all the imx-src driver that started this framework only
has 5 reset controls registered with it), but to me this doesn't sound
like a very strong argument.

> We currently only need to reset one module this way currently anyway, we
> will be moving away from toggling bits from the host side to perform resets,
> but rather ask a power management controller to perform the reset for us
> (I also have a reset driver that communicates with this controller that I
> will post when the rest of the needed framework is upstreamed). So, for TI,
> this syscon based driver will probably mostly be used for compatibility with
> older SoCs that do not support the management controller, allowing new device
> drivers to use the reset framework and still function with older SoCs.

So this will only be used for a few legacy devices? I'd probably be less
hesitant if you proposed this as some ti chip specific binding, as right
now I just don't expect that many other devices to use this supposedly
generic binding.

> > This approach of specifying bits in the device tree at
> > the consumer side doesn't allow any error handling in the driver to
> > determine if the bits are in fact valid.
> 
> Yes, that is lost by not having a table of all valid resets, but this would
> be like many DT driver/node that allow registers/addresses to be specified
> and then write/read from those.

True. Still, I'd argue that having a list of registers and bit-shifts in
a single place (whether that is in the driver or in the reset controller
node), that can be easily checked against a manual. On the other hand
sprinkling these values around the device tree at the consumer sites
makes this much harder to review.

[...]
> >> +Required properties:
> >> +--------------------
> >> + - compatible	: Should be "syscon-reset"
> >
> > What if later an erratum turns up and something special needs to be done
> > (delays, special care about other bits in the same register, etc.)?
> > This should always contain a soc specific compatible.
>
> In this case a specific reset driver would be needed for that device, this
> driver only intends to cover the more traditional base cases.

And for that to work in a backwards compatible way, you need to have the
SoC specific compatible value already in the device tree.

[...]
> > This is a binding for single reset bits that are spread throughout the
> > register space. For any syscon that has a few registers of contiguous
> > resets this is rather suboptimal.
>
> Yes, this is only intended for when a few resets need controlling out of
> a large reset space without having to directly encode the reset information
> into the device driver for that hardware module.

How about if we came up with a way to encode the bit fields in the reset
controller node?

[...]
> > The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
> > numbers in the gpio phandle bindings in the beginning has caused a lot
> > of problems over time.
> 
> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?

Yes, this is better.

> > Do you really have varying polarity across the resets?
> 
> Not on the part I'm using, but this should keep things generic.

We already established that this binding is not generic enough for most
of the current cases, so I'm not convinced it is valuable to complicate
it for some theoretical case.

> The alternative would be to set the polarity per reset controller node, then
> if a system has both it could have two controllers and the consumers would
> have a phandle to the correct one, then all consumers would only need 4
> instead of 6 args. Actually now that I think about it, this is probably the
> way to go as most systems I would imagine only have one polarity and it still
> can work for systems that do have both. I think I'll make this change.

How about going one more step and also moving the register and bit-shift
description into a property of the reset controller node?

> >> +		dsp0: dsp0 {
> >> +			...
> >> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
> >
> > This sounds a bit error prone and rather verbose for all controllers
> > that don't have control and status bits peppered randomly around the
> > register space.
> 
> I was also thinking about adding the ability to have only one set of args
> for control, then we just return ENOTSUPP when asked for status when only
> the control register is provided.

Yes, that should work.

> With the above polarity change, we end up allowing:
> 
> resets = <&pscrst 0xa3c 8>;

Would this be a reset controller with no control bit, but just a trigger
that is triggered when the bit is set? (or cleared?).

> when appropriate. This would cover many common use-cases and keep the
> framework clean for unique case drivers when needed. It would eliminate
> the need for many reset-berlin like drivers that only differentiate
> themselves in trivial ways, like offsets/polarity, etc..

That's not a good example. reset-berlin needs a bit to be set to trigger
the reset pulse, and doesn't have support for manual assert/deassert.
Also according to the driver it doesn't signal reset status, so there is
a fixed delay needed.
Honestly, I expect most drivers in the near future to fall into two
categories: either they need special attention, or they are not a good
fit for this binding because all the resets are (at least mostly)
contiguous.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 02/04/2016 09:49 AM, Philipp Zabel wrote:
> Hi Andrew,
>
> Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis:
> [...]
>>> sti also chose a single address cell and a logical number to enumerate
>>> the resets and to store the actual reset control and status bit position
>>> in a table in the driver. Is there a reason not to follow the same
>>> approach for ti?
>>
>> The number of reset-able modules is rather large, and we would have to
>> have an entry for them, and then a table of them for each chip, I
>> would like to avoid this as it may become unmaintainable with the number
>> of devices we would like to support.
>
> Maybe I underestimate the amount of work necessary to translate the
> register bit positions from the reference manuals into tables in a
> driver (after all the imx-src driver that started this framework only
> has 5 reset controls registered with it), but to me this doesn't sound
> like a very strong argument.
>

You're right it's not a pain for a single device, in fact I think
we only need 2 or 3 resets currently for any given device, I was more
concerned about the number of different devices leading to hard to
maintain sets of tables. But I concede this isn't the best argument,
I'd just like as little chip layout specific information in the driver
as possible.

>> We currently only need to reset one module this way currently anyway, we
>> will be moving away from toggling bits from the host side to perform resets,
>> but rather ask a power management controller to perform the reset for us
>> (I also have a reset driver that communicates with this controller that I
>> will post when the rest of the needed framework is upstreamed). So, for TI,
>> this syscon based driver will probably mostly be used for compatibility with
>> older SoCs that do not support the management controller, allowing new device
>> drivers to use the reset framework and still function with older SoCs.
>
> So this will only be used for a few legacy devices? I'd probably be less
> hesitant if you proposed this as some ti chip specific binding, as right
> now I just don't expect that many other devices to use this supposedly
> generic binding.
>

I'll probably do that as a last resort if we cant get something more
useful from this driver.

>>> This approach of specifying bits in the device tree at
>>> the consumer side doesn't allow any error handling in the driver to
>>> determine if the bits are in fact valid.
>>
>> Yes, that is lost by not having a table of all valid resets, but this would
>> be like many DT driver/node that allow registers/addresses to be specified
>> and then write/read from those.
>
> True. Still, I'd argue that having a list of registers and bit-shifts in
> a single place (whether that is in the driver or in the reset controller
> node), that can be easily checked against a manual. On the other hand
> sprinkling these values around the device tree at the consumer sites
> makes this much harder to review.
>

Hmmm, I wouldn't be opposed to that, it's suggested a couple times below,
so I might see if that works well. I've worked out a little example,
so before I implement these changes, would something like this be more
acceptable?:

(child node of syscon node)
reset: reset {
	compatible = "syscon-reset";
	#reset-cells = <1>;
	#address-cells = <1>;
	#size-cells = <0>;
		
	dsp@0 {
		reg = <0>;
		control = <0xa44 8 RESET_ASSERT_SET>;
	};

	imu@1 {
		reg = <1>;
		control = <0xa46 7 RESET_ASSERT_SET>;
		status = <0x844 7 RESET_ASSERT_CLEAR>;
		toggle-only;
	};
};

then consumers could just

resets = <&reset 0>, <&reset 1>;
resest-names = "dsp", "imu";

like normal.

> [...]
>>>> +Required properties:
>>>> +--------------------
>>>> + - compatible	: Should be "syscon-reset"
>>>
>>> What if later an erratum turns up and something special needs to be done
>>> (delays, special care about other bits in the same register, etc.)?
>>> This should always contain a soc specific compatible.
>>
>> In this case a specific reset driver would be needed for that device, this
>> driver only intends to cover the more traditional base cases.
>
> And for that to work in a backwards compatible way, you need to have the
> SoC specific compatible value already in the device tree.
>

Then it can be added, "syscon-reset" would just be the generic fall-back when
no other more specific driver matches. This is already the case though, or are
you saying you would like the example DT to contain one?

> [...]
>>> This is a binding for single reset bits that are spread throughout the
>>> register space. For any syscon that has a few registers of contiguous
>>> resets this is rather suboptimal.
>>
>> Yes, this is only intended for when a few resets need controlling out of
>> a large reset space without having to directly encode the reset information
>> into the device driver for that hardware module.
>
> How about if we came up with a way to encode the bit fields in the reset
> controller node?
>

Above.

> [...]
>>> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
>>> numbers in the gpio phandle bindings in the beginning has caused a lot
>>> of problems over time.
>>
>> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?
>
> Yes, this is better.
>
>>> Do you really have varying polarity across the resets?
>>
>> Not on the part I'm using, but this should keep things generic.
>
> We already established that this binding is not generic enough for most
> of the current cases, so I'm not convinced it is valuable to complicate
> it for some theoretical case.
>
>> The alternative would be to set the polarity per reset controller node, then
>> if a system has both it could have two controllers and the consumers would
>> have a phandle to the correct one, then all consumers would only need 4
>> instead of 6 args. Actually now that I think about it, this is probably the
>> way to go as most systems I would imagine only have one polarity and it still
>> can work for systems that do have both. I think I'll make this change.
>
> How about going one more step and also moving the register and bit-shift
> description into a property of the reset controller node?
>

Above.

>>>> +		dsp0: dsp0 {
>>>> +			...
>>>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>>>
>>> This sounds a bit error prone and rather verbose for all controllers
>>> that don't have control and status bits peppered randomly around the
>>> register space.
>>
>> I was also thinking about adding the ability to have only one set of args
>> for control, then we just return ENOTSUPP when asked for status when only
>> the control register is provided.
>
> Yes, that should work.
>
>> With the above polarity change, we end up allowing:
>>
>> resets = <&pscrst 0xa3c 8>;
>
> Would this be a reset controller with no control bit, but just a trigger
> that is triggered when the bit is set? (or cleared?).
>

Should be addressed by above binding change.

>> when appropriate. This would cover many common use-cases and keep the
>> framework clean for unique case drivers when needed. It would eliminate
>> the need for many reset-berlin like drivers that only differentiate
>> themselves in trivial ways, like offsets/polarity, etc..
>
> That's not a good example. reset-berlin needs a bit to be set to trigger
> the reset pulse, and doesn't have support for manual assert/deassert.
> Also according to the driver it doesn't signal reset status, so there is
> a fixed delay needed.
> Honestly, I expect most drivers in the near future to fall into two
> categories: either they need special attention, or they are not a good
> fit for this binding because all the resets are (at least mostly)
> contiguous.
>

I'm thinking the same, but at least for the devices I'm working with,
this could still be a useful thing to have.

Thanks,
Andrew

> regards
> Philipp
>

Re: [PATCH 1/2] Documentation: dt: reset: Add syscon reset binding

[Andrew F. Davis]

On 02/04/2016 09:49 AM, Philipp Zabel wrote:
> Hi Andrew,
>
> Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis:
> [...]
>>> sti also chose a single address cell and a logical number to enumerate
>>> the resets and to store the actual reset control and status bit position
>>> in a table in the driver. Is there a reason not to follow the same
>>> approach for ti?
>>
>> The number of reset-able modules is rather large, and we would have to
>> have an entry for them, and then a table of them for each chip, I
>> would like to avoid this as it may become unmaintainable with the number
>> of devices we would like to support.
>
> Maybe I underestimate the amount of work necessary to translate the
> register bit positions from the reference manuals into tables in a
> driver (after all the imx-src driver that started this framework only
> has 5 reset controls registered with it), but to me this doesn't sound
> like a very strong argument.
>

You're right it's not a pain for a single device, in fact I think
we only need 2 or 3 resets currently for any given device, I was more
concerned about the number of different devices leading to hard to
maintain sets of tables. But I concede this isn't the best argument,
I'd just like as little chip layout specific information in the driver
as possible.

>> We currently only need to reset one module this way currently anyway, we
>> will be moving away from toggling bits from the host side to perform resets,
>> but rather ask a power management controller to perform the reset for us
>> (I also have a reset driver that communicates with this controller that I
>> will post when the rest of the needed framework is upstreamed). So, for TI,
>> this syscon based driver will probably mostly be used for compatibility with
>> older SoCs that do not support the management controller, allowing new device
>> drivers to use the reset framework and still function with older SoCs.
>
> So this will only be used for a few legacy devices? I'd probably be less
> hesitant if you proposed this as some ti chip specific binding, as right
> now I just don't expect that many other devices to use this supposedly
> generic binding.
>

I'll probably do that as a last resort if we cant get something more
useful from this driver.

>>> This approach of specifying bits in the device tree at
>>> the consumer side doesn't allow any error handling in the driver to
>>> determine if the bits are in fact valid.
>>
>> Yes, that is lost by not having a table of all valid resets, but this would
>> be like many DT driver/node that allow registers/addresses to be specified
>> and then write/read from those.
>
> True. Still, I'd argue that having a list of registers and bit-shifts in
> a single place (whether that is in the driver or in the reset controller
> node), that can be easily checked against a manual. On the other hand
> sprinkling these values around the device tree at the consumer sites
> makes this much harder to review.
>

Hmmm, I wouldn't be opposed to that, it's suggested a couple times below,
so I might see if that works well. I've worked out a little example,
so before I implement these changes, would something like this be more
acceptable?:

(child node of syscon node)
reset: reset {
	compatible = "syscon-reset";
	#reset-cells = <1>;
	#address-cells = <1>;
	#size-cells = <0>;
		
	dsp@0 {
		reg = <0>;
		control = <0xa44 8 RESET_ASSERT_SET>;
	};

	imu@1 {
		reg = <1>;
		control = <0xa46 7 RESET_ASSERT_SET>;
		status = <0x844 7 RESET_ASSERT_CLEAR>;
		toggle-only;
	};
};

then consumers could just

resets = <&reset 0>, <&reset 1>;
resest-names = "dsp", "imu";

like normal.

> [...]
>>>> +Required properties:
>>>> +--------------------
>>>> + - compatible	: Should be "syscon-reset"
>>>
>>> What if later an erratum turns up and something special needs to be done
>>> (delays, special care about other bits in the same register, etc.)?
>>> This should always contain a soc specific compatible.
>>
>> In this case a specific reset driver would be needed for that device, this
>> driver only intends to cover the more traditional base cases.
>
> And for that to work in a backwards compatible way, you need to have the
> SoC specific compatible value already in the device tree.
>

Then it can be added, "syscon-reset" would just be the generic fall-back when
no other more specific driver matches. This is already the case though, or are
you saying you would like the example DT to contain one?

> [...]
>>> This is a binding for single reset bits that are spread throughout the
>>> register space. For any syscon that has a few registers of contiguous
>>> resets this is rather suboptimal.
>>
>> Yes, this is only intended for when a few resets need controlling out of
>> a large reset space without having to directly encode the reset information
>> into the device driver for that hardware module.
>
> How about if we came up with a way to encode the bit fields in the reset
> controller node?
>

Above.

> [...]
>>> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using
>>> numbers in the gpio phandle bindings in the beginning has caused a lot
>>> of problems over time.
>>
>> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work?
>
> Yes, this is better.
>
>>> Do you really have varying polarity across the resets?
>>
>> Not on the part I'm using, but this should keep things generic.
>
> We already established that this binding is not generic enough for most
> of the current cases, so I'm not convinced it is valuable to complicate
> it for some theoretical case.
>
>> The alternative would be to set the polarity per reset controller node, then
>> if a system has both it could have two controllers and the consumers would
>> have a phandle to the correct one, then all consumers would only need 4
>> instead of 6 args. Actually now that I think about it, this is probably the
>> way to go as most systems I would imagine only have one polarity and it still
>> can work for systems that do have both. I think I'll make this change.
>
> How about going one more step and also moving the register and bit-shift
> description into a property of the reset controller node?
>

Above.

>>>> +		dsp0: dsp0 {
>>>> +			...
>>>> +			resets = <&pscrst 0xa3c 8 0 0x83c 8 0>;
>>>
>>> This sounds a bit error prone and rather verbose for all controllers
>>> that don't have control and status bits peppered randomly around the
>>> register space.
>>
>> I was also thinking about adding the ability to have only one set of args
>> for control, then we just return ENOTSUPP when asked for status when only
>> the control register is provided.
>
> Yes, that should work.
>
>> With the above polarity change, we end up allowing:
>>
>> resets = <&pscrst 0xa3c 8>;
>
> Would this be a reset controller with no control bit, but just a trigger
> that is triggered when the bit is set? (or cleared?).
>

Should be addressed by above binding change.

>> when appropriate. This would cover many common use-cases and keep the
>> framework clean for unique case drivers when needed. It would eliminate
>> the need for many reset-berlin like drivers that only differentiate
>> themselves in trivial ways, like offsets/polarity, etc..
>
> That's not a good example. reset-berlin needs a bit to be set to trigger
> the reset pulse, and doesn't have support for manual assert/deassert.
> Also according to the driver it doesn't signal reset status, so there is
> a fixed delay needed.
> Honestly, I expect most drivers in the near future to fall into two
> categories: either they need special attention, or they are not a good
> fit for this binding because all the resets are (at least mostly)
> contiguous.
>

I'm thinking the same, but at least for the devices I'm working with,
this could still be a useful thing to have.

Thanks,
Andrew

> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html