[PATCH v3 0/2] Add support for SYSCON reset

[PATCH v3 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.

Changes from v2:
 - Rebased on v4.7-rc1
 - Removed the need to give reset specifier nodes an index address

Changes from v1:
 - Reset control information is now described in the reset node, this
   keeps the reset information centralized for easy verification
 - Other small fixups

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

 .../devicetree/bindings/reset/syscon-reset.txt     | 105 ++++++++
 drivers/reset/Kconfig                              |  10 +
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-syscon.c                       | 289 +++++++++++++++++++++
 include/dt-bindings/reset/syscon.h                 |  23 ++
 5 files changed, 428 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
 create mode 100644 drivers/reset/reset-syscon.c
 create mode 100644 include/dt-bindings/reset/syscon.h

-- 
2.8.3

[PATCH v3 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     | 105 +++++++++++++++++++++
 include/dt-bindings/reset/syscon.h                 |  23 +++++
 2 files changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
 create mode 100644 include/dt-bindings/reset/syscon.h

diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt
new file mode 100644
index 0000000..cec6e32
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt
@@ -0,0 +1,105 @@
+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 be a child of a syscon
+node and have the following properties.
+
+Required properties:
+--------------------
+ - compatible		: Should be "syscon-reset"
+ - #reset-cells		: Should be 1. Please see the reset consumer node below
+			  for usage details
+
+SysCon Reset Child Node
+============================
+Each reset provider/controller node should have a child node for each reset
+it would like to expose to consumers.
+
+Required properties:
+--------------------
+ - reset-control	: Contains the reset control register information
+			  Should contain 3 cells defined as:
+			    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 or
+				      RESET_ASSERT_SET for resets that are
+				      asserted when the bit is set, 0 or
+				      RESET_ASSERT_CLEAR for resets that are
+				      asserted when the bit is cleared
+
+Optional properties:
+--------------------
+ - reset-status		: Contains the reset status register information. The
+			  contents of this property are the equivalent to
+			  reset-control as defined above. If this property is
+			  not present and the toggle flag is not set, the
+			  reset register is assumed to be the same as the
+			  control register
+ - toggle		: Mark this reset as a toggle only reset, this is used
+			  when no status register is available.
+
+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 to the reset controller node and a phandle to a
+		  reset specifier node as defined above.
+
+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", "simple-mfd";
+			reg = <0x02350000 0x1000>;
+
+			pscrst: psc-reset {
+				compatible = "syscon-reset";
+				#reset-cells = <1>;
+
+				pscrst-dsp: dsp {
+					reset-control = <0xa3c 8 RESET_ASSERT_CLEAR>;
+					reset-status = <0x83c 8 RESET_ASSERT_CLEAR>;
+				};
+
+				pscrst-example: example {
+					reset-control = <0xa40 5 RESET_ASSERT_SET>;
+					toggle;
+				};
+			};
+		};
+
+		dsp0: dsp0 {
+			...
+			resets = <&pscrst &pscrst-dsp>;
+			...
+		};
+	};
+};
diff --git a/include/dt-bindings/reset/syscon.h b/include/dt-bindings/reset/syscon.h
new file mode 100644
index 0000000..9927779
--- /dev/null
+++ b/include/dt-bindings/reset/syscon.h
@@ -0,0 +1,23 @@
+/*
+ * Syscon Reset definitions
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __DT_BINDINGS_RESET_SYSCON_H__
+#define __DT_BINDINGS_RESET_SYSCON_H__
+
+#define RESET_ASSERT_CLEAR	0x0
+#define RESET_ASSERT_SET	0x1
+
+#endif
-- 
2.8.3

[PATCH v3 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 | 289 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 300 insertions(+)
 create mode 100644 drivers/reset/reset-syscon.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 0b2733d..73072e2 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -15,5 +15,15 @@ menuconfig RESET_CONTROLLER
 config RESET_OXNAS
 	bool
 
+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 f173fc3..63eb0c3 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/
 obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
 obj-$(CONFIG_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.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..724b54f
--- /dev/null
+++ b/drivers/reset/reset-syscon.c
@@ -0,0 +1,289 @@
+/*
+ * SYSCON regmap reset driver
+ *
+ * Copyright (C) 2015-2016 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
+ * @toggle: flag to indicate this reset has no readable status register
+ */
+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;
+	bool toggle;
+};
+
+/**
+ * struct syscon_reset_data - reset controller information structure
+ * @rcdev: reset controller entity
+ * @dev: reset controller device pointer
+ * @regmap: 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 *regmap;
+	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->regmap, 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, true if reset is asserted, else a
+ * corresponding error value
+ */
+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;
+
+	if (control->toggle)
+		return -ENOSYS; /* status not supported for this reset */
+
+	ret = regmap_read(data->regmap, 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,
+};
+
+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;
+	phandle phandle = reset_spec->args[0];
+	struct device_node *node;
+	const __be32 *list;
+	int size;
+
+	control = devm_kzalloc(data->dev, sizeof(*control), GFP_KERNEL);
+	if (!control)
+		return -ENOMEM;
+
+	node = of_find_node_by_phandle(phandle);
+	if (!node) {
+		pr_err("could not find reset node by phandle %#x\n", phandle);
+		devm_kfree(data->dev, control);
+		return -ENOENT;
+	}
+
+	list = of_get_property(node, "reset-control", &size);
+	if (!list || size != (3 * sizeof(*list))) {
+		of_node_put(node);
+		devm_kfree(data->dev, control);
+		return -EINVAL;
+	}
+	control->offset = be32_to_cpup(list++);
+	control->reset_bit = be32_to_cpup(list++);
+	control->assert_high = be32_to_cpup(list) == 1;
+
+	if (of_find_property(node, "reset-toggle", NULL)) {
+		control->toggle = true;
+		goto done;
+	}
+	control->toggle = false;
+
+	list = of_get_property(node, "reset-status", &size);
+	if (!list) {
+		/* use control register values */
+		control->status_offset = control->offset;
+		control->status_reset_bit = control->reset_bit;
+		control->status_assert_high = control->assert_high;
+		goto done;
+	}
+	if (size != (3 * sizeof(*list))) {
+		of_node_put(node);
+		devm_kfree(data->dev, control);
+		return -EINVAL;
+	}
+	control->status_offset = be32_to_cpup(list++);
+	control->status_reset_bit = be32_to_cpup(list++);
+	control->status_assert_high = be32_to_cpup(list) == 1;
+
+done:
+	of_node_put(node);
+	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 *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *regmap;
+
+	if (!np)
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data->rcdev.ops = &syscon_reset_ops;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.of_node = np;
+	data->rcdev.of_reset_n_cells = 1;
+	data->rcdev.of_xlate = syscon_reset_of_xlate;
+	data->dev = dev;
+	data->regmap = regmap;
+	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[] = {
+	{ .compatible = "syscon-reset", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, syscon_reset_of_match);
+
+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,
+	},
+};
+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.8.3

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

[Suman Anna]

Hi Andrew,

On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
> 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 | 289 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 300 insertions(+)
>  create mode 100644 drivers/reset/reset-syscon.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 0b2733d..73072e2 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -15,5 +15,15 @@ menuconfig RESET_CONTROLLER
>  config RESET_OXNAS
>  	bool
>  
> +config SYSCON_RESET
> +	tristate "SYSCON Reset Driver"
> +	depends on RESET_CONTROLLER

Please add a depends on HAS_IOMEM here that you seem to have missed from
our local patches.

regards
Suman

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

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

[Andrew F. Davis]

On 06/15/2016 11:51 AM, Suman Anna wrote:
> Hi Andrew,
> 
> On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
>> 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 | 289 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 300 insertions(+)
>>  create mode 100644 drivers/reset/reset-syscon.c
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 0b2733d..73072e2 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -15,5 +15,15 @@ menuconfig RESET_CONTROLLER
>>  config RESET_OXNAS
>>  	bool
>>  
>> +config SYSCON_RESET
>> +	tristate "SYSCON Reset Driver"
>> +	depends on RESET_CONTROLLER
> 
> Please add a depends on HAS_IOMEM here that you seem to have missed from
> our local patches.
> 

Will add for v4.

Thanks,
Andrew

Re: [PATCH v3 0/2] Add support for SYSCON reset

[Andrew F. Davis]

On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
> 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.
> 
> Changes from v2:
>  - Rebased on v4.7-rc1
>  - Removed the need to give reset specifier nodes an index address
> 
> Changes from v1:
>  - Reset control information is now described in the reset node, this
>    keeps the reset information centralized for easy verification
>  - Other small fixups
> 
> Andrew F. Davis (2):
>   Documentation: dt: reset: Add syscon reset binding
>   reset: add a SYSCON based reset driver
> 
>  .../devicetree/bindings/reset/syscon-reset.txt     | 105 ++++++++
>  drivers/reset/Kconfig                              |  10 +
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/reset-syscon.c                       | 289 +++++++++++++++++++++
>  include/dt-bindings/reset/syscon.h                 |  23 ++
>  5 files changed, 428 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>  create mode 100644 drivers/reset/reset-syscon.c
>  create mode 100644 include/dt-bindings/reset/syscon.h
> 

Is there any additional feedback for this driver? I normally try to
refrain from pings, but this may begin to block further upstreaming of
IPs that uses this reset if it can not be taken this cycle.

If there is still an objection to calling this a generic reset solution
we would not strongly object to relabeling this to something implying
more TI exclusivity.

Thanks,
Andrew

Re: [PATCH v3 0/2] Add support for SYSCON reset

[Philipp Zabel]

Hi Andrew,

Am Mittwoch, den 15.06.2016, 10:48 -0500 schrieb Andrew F. Davis:
> On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
> > 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.
> > 
> > Changes from v2:
> >  - Rebased on v4.7-rc1
> >  - Removed the need to give reset specifier nodes an index address
> > 
> > Changes from v1:
> >  - Reset control information is now described in the reset node, this
> >    keeps the reset information centralized for easy verification
> >  - Other small fixups
> > 
> > Andrew F. Davis (2):
> >   Documentation: dt: reset: Add syscon reset binding
> >   reset: add a SYSCON based reset driver
> > 
> >  .../devicetree/bindings/reset/syscon-reset.txt     | 105 ++++++++
> >  drivers/reset/Kconfig                              |  10 +
> >  drivers/reset/Makefile                             |   1 +
> >  drivers/reset/reset-syscon.c                       | 289 +++++++++++++++++++++
> >  include/dt-bindings/reset/syscon.h                 |  23 ++
> >  5 files changed, 428 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
> >  create mode 100644 drivers/reset/reset-syscon.c
> >  create mode 100644 include/dt-bindings/reset/syscon.h
> > 
> 
> Is there any additional feedback for this driver? I normally try to
> refrain from pings, but this may begin to block further upstreaming of
> IPs that uses this reset if it can not be taken this cycle.

There's still Rob's concern that this binding needs a device tree node
per single register bit in the worst case, which seems a bit overkill.

I'd still prefer to have this information hidden in the drivers, but if
you absolutely have to put it in the device tree, maybe an approach more
similar to pinctrl-simple, where you could list all resets, one per
line, in a single property, would be more acceptable:

pscrst: reset-controller {
	compatible = "ti,<chip>-pscrst", "syscon-reset";

	/* syscon-reset,bits = <ctrl_reg ctrl_bit stat_reg stat_bit flags>; */
	syscon-reset,bits = <0xa3c 8 0x83c 8 RESET_ASSERT_CLEAR  /* 0: pcrst-dsp */
	                     0xa40 5 0     0 RESET_TRIGGER_SET>; /* 1: pcrst-example */
};

dsp0: dsp {
	resets = <&pscrst 0>;
};

> If there is still an objection to calling this a generic reset solution
> we would not strongly object to relabeling this to something implying
> more TI exclusivity.

Good. Right now there don't seem to be that many reset controllers in
the wild for which this binding would be a good fit. If this turns out
to be useful for others, we can add more compatibles to the driver.

regards
Philipp

Re: [PATCH v3 0/2] Add support for SYSCON reset

[Rob Herring]

On Wed, Jun 15, 2016 at 11:45 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Andrew,
>
> Am Mittwoch, den 15.06.2016, 10:48 -0500 schrieb Andrew F. Davis:
>> On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
>> > 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.
>> >
>> > Changes from v2:
>> >  - Rebased on v4.7-rc1
>> >  - Removed the need to give reset specifier nodes an index address
>> >
>> > Changes from v1:
>> >  - Reset control information is now described in the reset node, this
>> >    keeps the reset information centralized for easy verification
>> >  - Other small fixups
>> >
>> > Andrew F. Davis (2):
>> >   Documentation: dt: reset: Add syscon reset binding
>> >   reset: add a SYSCON based reset driver
>> >
>> >  .../devicetree/bindings/reset/syscon-reset.txt     | 105 ++++++++
>> >  drivers/reset/Kconfig                              |  10 +
>> >  drivers/reset/Makefile                             |   1 +
>> >  drivers/reset/reset-syscon.c                       | 289 +++++++++++++++++++++
>> >  include/dt-bindings/reset/syscon.h                 |  23 ++
>> >  5 files changed, 428 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>> >  create mode 100644 drivers/reset/reset-syscon.c
>> >  create mode 100644 include/dt-bindings/reset/syscon.h
>> >
>>
>> Is there any additional feedback for this driver? I normally try to
>> refrain from pings, but this may begin to block further upstreaming of
>> IPs that uses this reset if it can not be taken this cycle.
>
> There's still Rob's concern that this binding needs a device tree node
> per single register bit in the worst case, which seems a bit overkill.

Yes, that's still my concern with this.

> I'd still prefer to have this information hidden in the drivers, but if
> you absolutely have to put it in the device tree, maybe an approach more
> similar to pinctrl-simple, where you could list all resets, one per
> line, in a single property, would be more acceptable:
>
> pscrst: reset-controller {
>         compatible = "ti,<chip>-pscrst", "syscon-reset";
>
>         /* syscon-reset,bits = <ctrl_reg ctrl_bit stat_reg stat_bit flags>; */

ti,reset-bits

>         syscon-reset,bits = <0xa3c 8 0x83c 8 RESET_ASSERT_CLEAR  /* 0: pcrst-dsp */
>                              0xa40 5 0     0 RESET_TRIGGER_SET>; /* 1: pcrst-example */
> };
>
> dsp0: dsp {
>         resets = <&pscrst 0>;
> };

Otherwise, this seems okay. It is more concise.

>
>> If there is still an objection to calling this a generic reset solution
>> we would not strongly object to relabeling this to something implying
>> more TI exclusivity.
>
> Good. Right now there don't seem to be that many reset controllers in
> the wild for which this binding would be a good fit. If this turns out
> to be useful for others, we can add more compatibles to the driver.

Certainly other users would make a generic solution more compelling.

Rob

Re: [PATCH v3 0/2] Add support for SYSCON reset

[Andrew F. Davis]

On 06/15/2016 09:13 PM, Rob Herring wrote:
> On Wed, Jun 15, 2016 at 11:45 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> Hi Andrew,
>>
>> Am Mittwoch, den 15.06.2016, 10:48 -0500 schrieb Andrew F. Davis:
>>> On 06/01/2016 01:41 PM, Andrew F. Davis wrote:
>>>> 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.
>>>>
>>>> Changes from v2:
>>>>  - Rebased on v4.7-rc1
>>>>  - Removed the need to give reset specifier nodes an index address
>>>>
>>>> Changes from v1:
>>>>  - Reset control information is now described in the reset node, this
>>>>    keeps the reset information centralized for easy verification
>>>>  - Other small fixups
>>>>
>>>> Andrew F. Davis (2):
>>>>   Documentation: dt: reset: Add syscon reset binding
>>>>   reset: add a SYSCON based reset driver
>>>>
>>>>  .../devicetree/bindings/reset/syscon-reset.txt     | 105 ++++++++
>>>>  drivers/reset/Kconfig                              |  10 +
>>>>  drivers/reset/Makefile                             |   1 +
>>>>  drivers/reset/reset-syscon.c                       | 289 +++++++++++++++++++++
>>>>  include/dt-bindings/reset/syscon.h                 |  23 ++
>>>>  5 files changed, 428 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt
>>>>  create mode 100644 drivers/reset/reset-syscon.c
>>>>  create mode 100644 include/dt-bindings/reset/syscon.h
>>>>
>>>
>>> Is there any additional feedback for this driver? I normally try to
>>> refrain from pings, but this may begin to block further upstreaming of
>>> IPs that uses this reset if it can not be taken this cycle.
>>
>> There's still Rob's concern that this binding needs a device tree node
>> per single register bit in the worst case, which seems a bit overkill.
> 
> Yes, that's still my concern with this.
> 
>> I'd still prefer to have this information hidden in the drivers, but if
>> you absolutely have to put it in the device tree, maybe an approach more
>> similar to pinctrl-simple, where you could list all resets, one per
>> line, in a single property, would be more acceptable:
>>
>> pscrst: reset-controller {
>>         compatible = "ti,<chip>-pscrst", "syscon-reset";
>>
>>         /* syscon-reset,bits = <ctrl_reg ctrl_bit stat_reg stat_bit flags>; */
> 
> ti,reset-bits
> 
>>         syscon-reset,bits = <0xa3c 8 0x83c 8 RESET_ASSERT_CLEAR  /* 0: pcrst-dsp */
>>                              0xa40 5 0     0 RESET_TRIGGER_SET>; /* 1: pcrst-example */
>> };
>>
>> dsp0: dsp {
>>         resets = <&pscrst 0>;
>> };
> 
> Otherwise, this seems okay. It is more concise.
> 

If this is what you would both be okay with then I'll do it this way,
pushing v4 in a moment.

>>
>>> If there is still an objection to calling this a generic reset solution
>>> we would not strongly object to relabeling this to something implying
>>> more TI exclusivity.
>>
>> Good. Right now there don't seem to be that many reset controllers in
>> the wild for which this binding would be a good fit. If this turns out
>> to be useful for others, we can add more compatibles to the driver.
> 
> Certainly other users would make a generic solution more compelling.
> 

I believe some other current reset drivers could have made use of this
without having resorted to new drivers, I guess we will have to just
wait to see if it gets traction with new platforms or not, before we can
decide if this solution is really as general as I think it can be.

Andrew