[RFC PATCH 0/5] Reset controller API to reset IP modules on i.MX5 and i.MX6

[RFC PATCH 0/5] Reset controller API to reset IP modules on i.MX5 and i.MX6

[Philipp Zabel]

The system reset controller (SRC) on i.MX51, i.MX53, and i.MX6q controls
reset lines to the GPU, VPU, IPU, and OpenVG IP modules.

The following patches add a simple API for devices to request being reset
by separate reset controller hardware. They implement the reset signal
device tree binding proposed by Stephen Warren. Contrary to Tegra hardware,
the i.MX SRC has self-deasserting reset bits, so I've included both
ops to manually assert/deassert a reset line, as well as a reset
operation that is supposed to assert the reset line and wait for it to
deassert.

The i.MX SRC is enhanced to provide a reset controller and the IPU driver
is made to request being reset by calling the device_reset(&pdev->dev)
convenience wrapper during probing.

regards
Philipp

---
 .../devicetree/bindings/reset/fsl,imx-src.txt      |   49 ++++
 Documentation/devicetree/bindings/reset/reset.txt  |   75 ++++++
 .../bindings/staging/imx-drm/fsl-imx-drm.txt       |    3 +
 arch/arm/boot/dts/imx51.dtsi                       |    7 +
 arch/arm/boot/dts/imx53.dtsi                       |    7 +
 arch/arm/boot/dts/imx6q.dtsi                       |    3 +
 arch/arm/mach-imx/Kconfig                          |    1 +
 arch/arm/mach-imx/common.h                         |    3 +-
 arch/arm/mach-imx/mach-imx6q.c                     |    2 +-
 arch/arm/mach-imx/mm-imx5.c                        |    2 +
 arch/arm/mach-imx/src.c                            |   86 ++++++-
 drivers/Kconfig                                    |    2 +
 drivers/Makefile                                   |    2 +
 drivers/reset/Kconfig                              |   10 +
 drivers/reset/Makefile                             |    2 +
 drivers/reset/core.c                               |  241 ++++++++++++++++++++
 drivers/staging/imx-drm/ipu-v3/ipu-common.c        |   12 +-
 include/linux/imx-src.h                            |    6 +
 18 files changed, 507 insertions(+), 6 deletions(-)

[PATCH 1/7] dt: describe base reset signal binding

[Philipp Zabel]

From: Stephen Warren <swarren@nvidia.com>

This binding is intended to represent the hardware reset signals present
internally in most IC (SoC, FPGA, ...) designs.

Such a binding would allow the creation of a "reset subsystem", which
could replace APIs such as the following Tegra-specific API:

void tegra_periph_reset_deassert(struct clk *c);
void tegra_periph_reset_assert(struct clk *c);

(Note that at present, Tegra couples reset assertion with the clock for
the affected peripheral module. However, reset and clocking are two
separate, yet admittedly related, concepts).

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 Documentation/devicetree/bindings/reset/reset.txt |   75 +++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/reset.txt

diff --git a/Documentation/devicetree/bindings/reset/reset.txt b/Documentation/devicetree/bindings/reset/reset.txt
new file mode 100644
index 0000000..31db6ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/reset.txt
@@ -0,0 +1,75 @@
+= Reset Signal Device Tree Bindings =
+
+This binding is intended to represent the hardware reset signals present
+internally in most IC (SoC, FPGA, ...) designs. Reset signals for whole
+standalone chips are most likely better represented as GPIOs, although there
+are likely to be exceptions to this rule.
+
+Hardware blocks typically receive a reset signal. This signal is generated by
+a reset provider (e.g. power management or clock module) and received by a
+reset consumer (the module being reset, or a module managing when a sub-
+ordinate module is reset). This binding exists to represent the provider and
+consumer, and provide a way to couple the two together.
+
+A reset signal is represented by the phandle of the provider, plus a reset
+specifier - a list of DT cells that represents the reset signal within the
+provider. The length (number of cells) and semantics of the reset specifier
+are dictated by the binding of the reset provider, although common schemes
+are described below.
+
+A word on where to place reset signal consumers in device tree: It is possible
+in hardware for a reset signal to affect multiple logically separate HW blocks
+at once. In this case, it would be unwise to represent this reset signal in
+the DT node of each affected HW block, since if activated, an unrelated block
+may be reset. Instead, reset signals should be represented in the DT node
+where it makes most sense to control it; this may be a bus node if all
+children of the bus are affected by the reset signal, or an individual HW
+block node for dedicated reset signals. The intent of this binding is to give
+appropriate software access to the reset signals in order to manage the HW,
+rather than to slavishly enumerate the reset signal that affects each HW
+block.
+
+= Reset providers =
+
+Required properties:
+#reset-cells:	Number of cells in a reset specifier; Typically 0 for nodes
+		with a single reset output and 1 for nodes with multiple
+		reset outputs.
+
+For example:
+
+	rst: reset-controller {
+		#reset-cells = <1>;
+	};
+
+= Reset consumers =
+
+Required properties:
+resets:		List of phandle and reset specifier pairs, one pair
+		for each reset signal that affects the device, or that the
+		device manages. Note: if the reset provider specifies '0' for
+		#reset-cells, then only the phandle portion of the pair will
+		appear.
+
+Optional properties:
+reset-names:	List of reset signal name strings sorted in the same order as
+		the resets property. Consumers drivers will use reset-names to
+		match reset signal names with reset specifiers.
+
+For example:
+
+	device {
+		resets = <&rst 20>;
+		reset-names = "reset";
+	};
+
+This represents a device with a single reset signal named "reset".
+
+	bus {
+		resets = <&rst 10> <&rst 11> <&rst 12> <&rst 11>;
+		reset-names = "i2s1", "i2s2", "dma", "mixer";
+	};
+
+This represents a bus that controls the reset signal of each of four sub-
+ordinate devices. Consider for example a bus that fails to operate unless no
+child device has reset asserted.
-- 
1.7.10.4

[PATCH 1/7] dt: describe base reset signal binding

[Stephen Warren]

On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> This binding is intended to represent the hardware reset signals present
> internally in most IC (SoC, FPGA, ...) designs.
> 
> Such a binding would allow the creation of a "reset subsystem", which
> could replace APIs such as the following Tegra-specific API:
> 
> void tegra_periph_reset_deassert(struct clk *c);
> void tegra_periph_reset_assert(struct clk *c);
> 
> (Note that at present, Tegra couples reset assertion with the clock for
> the affected peripheral module. However, reset and clocking are two
> separate, yet admittedly related, concepts).
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

You probably want to update the patch description; it was somewhat
appropriate when I sent the patch as an RFC to see what people thought
about the idea, but now you're actually providing the whole thing a more
specific description would be better.

Your S-o-b line should be present in all patches you send; add it after
mine. See Documentation/SubmittingPatches section "Sign your work" item
(c) (or perhaps (b) if you modified it).

[PATCH 2/7] reset: Add reset controller API

[Philipp Zabel]

This adds a simple API for devices to request being reset
by separate reset controller hardware and implements the
reset signal device tree binding.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/Kconfig        |    2 +
 drivers/Makefile       |    2 +
 drivers/reset/Kconfig  |   10 ++
 drivers/reset/Makefile |    2 +
 drivers/reset/core.c   |  241 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 257 insertions(+)
 create mode 100644 drivers/reset/Kconfig
 create mode 100644 drivers/reset/Makefile
 create mode 100644 drivers/reset/core.c

diff --git a/drivers/Kconfig b/drivers/Kconfig
index f5fb072..51f73ae 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -158,4 +158,6 @@ source "drivers/irqchip/Kconfig"
 
 source "drivers/ipack/Kconfig"
 
+source "drivers/reset/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 346ecc5..870bf7e 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -42,6 +42,8 @@ ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
 obj-y				+= clocksource/
 endif
 
+obj-$(CONFIG_RESET_CONTROLLER)	+= reset/
+
 # tty/ comes before char/ so that the VT console is the boot-time
 # default.
 obj-y				+= tty/
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
new file mode 100644
index 0000000..82dc89e
--- /dev/null
+++ b/drivers/reset/Kconfig
@@ -0,0 +1,10 @@
+menuconfig RESET_CONTROLLER
+	bool "Reset Controller Support"
+	help
+	  Generic Reset Controller support.
+
+	  This framework is designed to abstract reset handling of devices
+	  via GPIOs or SoC-internal reset controller modules.
+
+	  If unsure, say no.
+
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
new file mode 100644
index 0000000..9a7b6df
--- /dev/null
+++ b/drivers/reset/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_RESET_CONTROLLER) += core.o
+
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
new file mode 100644
index 0000000..f0ed61b
--- /dev/null
+++ b/drivers/reset/core.c
@@ -0,0 +1,241 @@
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/reset.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+
+static DEFINE_MUTEX(reset_controller_list_mutex);
+static LIST_HEAD(reset_controller_list);
+
+/**
+ * struct reset_control - a reset control
+ *
+ * @id: ID of the reset controller in the reset
+ *      controller device
+ */
+struct reset_control {
+	struct reset_controller_dev *rcdev;
+	unsigned int id;
+};
+
+/**
+ * reset_controller_register - register a reset controller
+ *
+ * @ops: a pointer to struct reset_controller_ops callbacks
+ *
+ * Returns a struct reset_controller_dev or IS_ERR() condition
+ * containing errno.
+ */
+int reset_controller_register(struct reset_controller_dev *rcdev)
+{
+	mutex_lock(&reset_controller_list_mutex);
+	list_add(&rcdev->list, &reset_controller_list);
+	mutex_unlock(&reset_controller_list_mutex);
+
+	return 0;
+}
+
+/**
+ * reset_control_reset - reset the controlled device
+ * @rstc: reset controller
+ */
+int reset_control_reset(struct reset_control *rstc)
+{
+	if (rstc->rcdev->ops->reset)
+		return rstc->rcdev->ops->reset(rstc->id);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_reset);
+
+/**
+ * reset_control_assert - asserts the reset line
+ * @rstc: reset controller
+ */
+int reset_control_assert(struct reset_control *rstc)
+{
+	if (rstc->rcdev->ops->assert)
+		return rstc->rcdev->ops->assert(rstc->id);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_assert);
+
+/**
+ * reset_control_deassert - deasserts the reset line
+ * @rstc: reset controller
+ */
+int reset_control_deassert(struct reset_control *rstc)
+{
+	if (rstc->rcdev->ops->deassert)
+		return rstc->rcdev->ops->deassert(rstc->id);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_deassert);
+
+/**
+ * reset_control_is_asserted - deasserts the reset line
+ * @rstc: reset controller
+ */
+int reset_control_is_asserted(struct reset_control *rstc)
+{
+	if (rstc->rcdev->ops->is_asserted)
+		return rstc->rcdev->ops->is_asserted(rstc->id);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_is_asserted);
+
+/**
+ * reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ *
+ * Use of id names is optional.
+ */
+struct reset_control *reset_control_get(struct device *dev, const char *id)
+{
+	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
+	struct reset_controller_dev *r, *rcdev;
+	struct device_node *rcdev_node;
+	struct of_phandle_args args;
+	int rcdev_index;
+	int ret;
+	int i;
+
+	if (!dev)
+		return ERR_PTR(-EINVAL);
+
+	rcdev_node = NULL;
+	for (i = 0; rcdev_node == NULL; i++) {
+		ret = of_parse_phandle_with_args(dev->of_node, "resets",
+						 "#reset-cells", i, &args);
+		if (ret)
+			return ERR_PTR(ret);
+		of_node_put(args.np);
+		if (args.args_count <= 0)
+			return ERR_PTR(-EINVAL);
+
+		if (id) {
+			const char *reset_name;
+			ret = of_property_read_string_index(dev->of_node,
+							    "reset-names", i,
+							    &reset_name);
+			if (ret)
+				return ERR_PTR(ret);
+			if (strcmp(id, reset_name) != 0)
+				continue;
+		}
+
+		rcdev_node = args.np;
+		rcdev_index = args.args[0];
+	}
+
+	mutex_lock(&reset_controller_list_mutex);
+	rcdev = NULL;
+	list_for_each_entry(r, &reset_controller_list, list) {
+		if (rcdev_node == r->of_node) {
+			rcdev = r;
+			break;
+		}
+	}
+	mutex_unlock(&reset_controller_list_mutex);
+
+	if (!rcdev)
+		return ERR_PTR(-ENODEV);
+
+	try_module_get(rcdev->owner);
+
+	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
+	if (!rstc)
+		return ERR_PTR(-ENOMEM);
+
+	rstc->rcdev = rcdev;
+	rstc->id = rcdev_index;
+
+	return rstc;
+}
+EXPORT_SYMBOL_GPL(reset_control_get);
+
+/**
+ * reset_control_put - free the reset controller
+ * @reset: reset controller
+ */
+
+void reset_control_put(struct reset_control *rstc)
+{
+	if (rstc == NULL || IS_ERR(rstc))
+		return;
+
+	kfree(rstc);
+	module_put(rstc->rcdev->owner);
+}
+EXPORT_SYMBOL_GPL(reset_control_put);
+
+static void devm_reset_control_release(struct device *dev, void *res)
+{
+	reset_control_put(*(struct reset_control **)res);
+}
+
+/**
+ * devm_reset_control_get - resource managed reset_control_get()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Managed reset_control_get(). For reset controllers returned from this
+ * function, reset_control_put() is called automatically on driver detach.
+ * See reset_control_get() for more information.
+ */
+struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
+{
+	struct reset_control **ptr, *rstc;
+
+	ptr = devres_alloc(devm_reset_control_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	rstc = reset_control_get(dev, id);
+	if (!IS_ERR(rstc)) {
+		*ptr = rstc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return rstc;
+}
+EXPORT_SYMBOL_GPL(devm_reset_control_get);
+
+/**
+ * device_reset - find reset controller associated with the device
+ *                and perform reset
+ * @dev: device to be reset by the controller
+ *
+ * Convenience wrapper for reset_control_get() and reset_control_reset().
+ * This is useful for the common case of devices with single, dedicated reset
+ * lines.
+ */
+int device_reset(struct device *dev)
+{
+	struct reset_control *rstc;
+	int ret;
+
+	rstc = reset_control_get(dev, NULL);
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);
+
+	ret = reset_control_reset(rstc);
+
+	kfree(rstc);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(device_reset);
-- 
1.7.10.4

[PATCH 2/7] reset: Add reset controller API

[Sascha Hauer]

On Wed, Jan 16, 2013 at 05:13:02PM +0100, Philipp Zabel wrote:
> This adds a simple API for devices to request being reset
> by separate reset controller hardware and implements the
> reset signal device tree binding.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/Kconfig        |    2 +
>  drivers/Makefile       |    2 +
>  drivers/reset/Kconfig  |   10 ++
>  drivers/reset/Makefile |    2 +
>  drivers/reset/core.c   |  241 ++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 257 insertions(+)
>  create mode 100644 drivers/reset/Kconfig
>  create mode 100644 drivers/reset/Makefile
>  create mode 100644 drivers/reset/core.c
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index f5fb072..51f73ae 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -158,4 +158,6 @@ source "drivers/irqchip/Kconfig"
>  
>  source "drivers/ipack/Kconfig"
>  
> +source "drivers/reset/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 346ecc5..870bf7e 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -42,6 +42,8 @@ ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
>  obj-y				+= clocksource/
>  endif
>  
> +obj-$(CONFIG_RESET_CONTROLLER)	+= reset/
> +
>  # tty/ comes before char/ so that the VT console is the boot-time
>  # default.
>  obj-y				+= tty/
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> new file mode 100644
> index 0000000..82dc89e
> --- /dev/null
> +++ b/drivers/reset/Kconfig
> @@ -0,0 +1,10 @@
> +menuconfig RESET_CONTROLLER
> +	bool "Reset Controller Support"
> +	help
> +	  Generic Reset Controller support.
> +
> +	  This framework is designed to abstract reset handling of devices
> +	  via GPIOs or SoC-internal reset controller modules.
> +
> +	  If unsure, say no.
> +
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> new file mode 100644
> index 0000000..9a7b6df
> --- /dev/null
> +++ b/drivers/reset/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_RESET_CONTROLLER) += core.o
> +
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> new file mode 100644
> index 0000000..f0ed61b
> --- /dev/null
> +++ b/drivers/reset/core.c
> @@ -0,0 +1,241 @@
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/reset.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +
> +static DEFINE_MUTEX(reset_controller_list_mutex);
> +static LIST_HEAD(reset_controller_list);
> +
> +/**
> + * struct reset_control - a reset control
> + *
> + * @id: ID of the reset controller in the reset
> + *      controller device
> + */
> +struct reset_control {
> +	struct reset_controller_dev *rcdev;
> +	unsigned int id;
> +};
> +
> +/**
> + * reset_controller_register - register a reset controller
> + *
> + * @ops: a pointer to struct reset_controller_ops callbacks
> + *
> + * Returns a struct reset_controller_dev or IS_ERR() condition
> + * containing errno.

The comment seems wrong.

> + */
> +int reset_controller_register(struct reset_controller_dev *rcdev)
> +{
> +	mutex_lock(&reset_controller_list_mutex);
> +	list_add(&rcdev->list, &reset_controller_list);
> +	mutex_unlock(&reset_controller_list_mutex);
> +
> +	return 0;
> +}
> +
> +/**
> + * reset_control_reset - reset the controlled device
> + * @rstc: reset controller
> + */
> +int reset_control_reset(struct reset_control *rstc)
> +{
> +	if (rstc->rcdev->ops->reset)
> +		return rstc->rcdev->ops->reset(rstc->id);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_reset);
> +
> +/**
> + * reset_control_assert - asserts the reset line
> + * @rstc: reset controller
> + */
> +int reset_control_assert(struct reset_control *rstc)
> +{
> +	if (rstc->rcdev->ops->assert)
> +		return rstc->rcdev->ops->assert(rstc->id);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_assert);
> +
> +/**
> + * reset_control_deassert - deasserts the reset line
> + * @rstc: reset controller
> + */
> +int reset_control_deassert(struct reset_control *rstc)
> +{
> +	if (rstc->rcdev->ops->deassert)
> +		return rstc->rcdev->ops->deassert(rstc->id);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_deassert);
> +
> +/**
> + * reset_control_is_asserted - deasserts the reset line

copy-paste from the function above?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/7] reset: Add reset controller API

[Stephen Warren]

On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> This adds a simple API for devices to request being reset
> by separate reset controller hardware and implements the
> reset signal device tree binding.


> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile

> +obj-$(CONFIG_RESET_CONTROLLER) += core.o
> +

nit: blank line at EOF.

> diff --git a/drivers/reset/core.c b/drivers/reset/core.c

> +/**
> + * reset_control_reset - reset the controlled device
> + * @rstc: reset controller
> + */
> +int reset_control_reset(struct reset_control *rstc)

What time period is the reset signal asserted for; should there be a
parameter to control this?

> +{
> +	if (rstc->rcdev->ops->reset)
> +		return rstc->rcdev->ops->reset(rstc->id);
> +
> +	return 0;
> +}

If there's no op, shouldn't the function fail?

> +/**
> + * reset_control_is_asserted - deasserts the reset line

Comment seems wrong.

Is this API useful; why wouldn't drivers just assert to de-assert it
based on their needs?

> +/**
> + * reset_control_get - Lookup and obtain a reference to a reset controller.
> + * @dev: device to be reset by the controller
> + * @id: reset line name
> + *
> + * Returns a struct reset_control or IS_ERR() condition containing errno.

OK, so NULL can't ever (legally) be returned.

> +struct reset_control *reset_control_get(struct device *dev, const char *id)

> +	rcdev_node = NULL;
> +	for (i = 0; rcdev_node == NULL; i++) {
...
> +		rcdev_node = args.np;
> +		rcdev_index = args.args[0];

Even though the loop condition tests rcdev_node, it'd be a lot easier to
realize that the loop bails out here because of that if you explicitly
wrote a break; here. Otherwise, it took a while for me to realize.

> +void reset_control_put(struct reset_control *rstc)
> +{
> +	if (rstc == NULL || IS_ERR(rstc))
> +		return;

... so (re: two comments above) you don't need to check for NULL here;
if someone passes NULL in here, they're passing some value that
reset_control_get() never passed back, so this API is free to do
whatever it wants...

> +	kfree(rstc);
> +	module_put(rstc->rcdev->owner);

You need to module_put() first, or copy rstc->rcdev, since otherwise
you're reading *rstc after it's freed.

This implementation only supports device tree. People might want the
APIs to work on non-device-tree systems too, although I guess that
should be easy enough to retrofit without changing the driver-visible API...

[PATCH 2/7] reset: Add reset controller API

[Philipp Zabel]

Hi Stephen,

Am Mittwoch, den 16.01.2013, 15:15 -0700 schrieb Stephen Warren:
> On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> > This adds a simple API for devices to request being reset
> > by separate reset controller hardware and implements the
> > reset signal device tree binding.
> 
> 
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> 
> > +obj-$(CONFIG_RESET_CONTROLLER) += core.o
> > +
> 
> nit: blank line at EOF.

Ok.

> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> 
> > +/**
> > + * reset_control_reset - reset the controlled device
> > + * @rstc: reset controller
> > + */
> > +int reset_control_reset(struct reset_control *rstc)
> 
> What time period is the reset signal asserted for; should there be a
> parameter to control this?

"The reset controller knows".

On i.MX, the SRC does all necessary things in hardware (and optionally
causes an interrupt to signal when it's ready - right now the SRC driver
just polls for the reset line to deassert).

For reset controllers that need to be manually asserted/deasserted, and
still want to provide the .reset callback, we should think about some
timing configuration. But IMHO that should be separate from the API.

I've thought about adding an example gpio-reset controller driver, which
knows about the necessary delays and just toggles gpios. This could be
represented in the device tree as follows:

	gpio_reset: gpio_reset {
		compatible = "gpio-reset"
		gpios = <&gpioa 0 0>, <&gpioc 5 0>, <&gpiod 3 1>;
		reset-delays = <10>, <100>, <50>;
		#reset-cells = <1>;
	};

	device-to-be-reset {
		resets = <&gpio_reset 2>; /* GPIOD3, active-low, 50ms */
	};

Or one could add another reset-cell to configure the delay:

	gpio_reset: gpio_reset {
		compatible = "gpio-reset"
		gpios = <&gpioa 0 0>, <&gpioc 5 0>, <&gpiod 3 1>;
		#reset-cells = <2>;
	};

	device-to-be-reset {
		resets = <&gpio_reset 2 50>; /* GPIOD3, active-low, 50ms */
	};

I'd prefer the former over the latter. In any case, I think the timing
information should reside either in the reset controller or in the
framework.

> > +{
> > +	if (rstc->rcdev->ops->reset)
> > +		return rstc->rcdev->ops->reset(rstc->id);
> > +
> > +	return 0;
> > +}
> 
> If there's no op, shouldn't the function fail?

Yes, I'll change that.

> > +/**
> > + * reset_control_is_asserted - deasserts the reset line
> 
> Comment seems wrong.
> 
> Is this API useful; why wouldn't drivers just assert to de-assert it
> based on their needs?

Probably not, I'll drop this.

> > +/**
> > + * reset_control_get - Lookup and obtain a reference to a reset controller.
> > + * @dev: device to be reset by the controller
> > + * @id: reset line name
> > + *
> > + * Returns a struct reset_control or IS_ERR() condition containing errno.
> 
> OK, so NULL can't ever (legally) be returned.

Right.

> > +struct reset_control *reset_control_get(struct device *dev, const char *id)
> 
> > +	rcdev_node = NULL;
> > +	for (i = 0; rcdev_node == NULL; i++) {
> ...
> > +		rcdev_node = args.np;
> > +		rcdev_index = args.args[0];
> 
> Even though the loop condition tests rcdev_node, it'd be a lot easier to
> realize that the loop bails out here because of that if you explicitly
> wrote a break; here. Otherwise, it took a while for me to realize.

Ok.

> > +void reset_control_put(struct reset_control *rstc)
> > +{
> > +	if (rstc == NULL || IS_ERR(rstc))
> > +		return;
> 
> ... so (re: two comments above) you don't need to check for NULL here;
> if someone passes NULL in here, they're passing some value that
> reset_control_get() never passed back, so this API is free to do
> whatever it wants...

Yes, I'll remove it.

> > +	kfree(rstc);
> > +	module_put(rstc->rcdev->owner);
> 
> You need to module_put() first, or copy rstc->rcdev, since otherwise
> you're reading *rstc after it's freed.

Ow, sorry for wasting your time with this one.

> This implementation only supports device tree. People might want the
> APIs to work on non-device-tree systems too, although I guess that
> should be easy enough to retrofit without changing the driver-visible API...

Indeed.

regards
Philipp

[PATCH 2/7] reset: Add reset controller API

[Stephen Warren]

On 01/17/2013 03:45 AM, Philipp Zabel wrote:
> Hi Stephen,
> 
> Am Mittwoch, den 16.01.2013, 15:15 -0700 schrieb Stephen Warren:
>> On 01/16/2013 09:13 AM, Philipp Zabel wrote:
>>> This adds a simple API for devices to request being reset
>>> by separate reset controller hardware and implements the
>>> reset signal device tree binding.
>>
>>
>>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>>
>>> +obj-$(CONFIG_RESET_CONTROLLER) += core.o
>>> +
>>
>> nit: blank line at EOF.
> 
> Ok.
> 
>>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>>
>>> +/**
>>> + * reset_control_reset - reset the controlled device
>>> + * @rstc: reset controller
>>> + */
>>> +int reset_control_reset(struct reset_control *rstc)
>>
>> What time period is the reset signal asserted for; should there be a
>> parameter to control this?
> 
> "The reset controller knows".
> 
> On i.MX, the SRC does all necessary things in hardware (and optionally
> causes an interrupt to signal when it's ready - right now the SRC driver
> just polls for the reset line to deassert).

OK, I guess that does make sense. As you say, for an on-SoC reset
controller that auto-clears the reset bit, it obviously has the timing
embedded in HW, and for any other kind of reset controller, we can just
provide configuration to the controller somehow.

> For reset controllers that need to be manually asserted/deasserted, and
> still want to provide the .reset callback, we should think about some
> timing configuration. But IMHO that should be separate from the API.
> 
> I've thought about adding an example gpio-reset controller driver, which
> knows about the necessary delays and just toggles gpios. This could be
> represented in the device tree as follows:
> 
> 	gpio_reset: gpio_reset {
> 		compatible = "gpio-reset"
> 		gpios = <&gpioa 0 0>, <&gpioc 5 0>, <&gpiod 3 1>;
> 		reset-delays = <10>, <100>, <50>;
> 		#reset-cells = <1>;
> 	};
> 
> 	device-to-be-reset {
> 		resets = <&gpio_reset 2>; /* GPIOD3, active-low, 50ms */
> 	};
> 
> Or one could add another reset-cell to configure the delay:
> 
> 	gpio_reset: gpio_reset {
> 		compatible = "gpio-reset"
> 		gpios = <&gpioa 0 0>, <&gpioc 5 0>, <&gpiod 3 1>;
> 		#reset-cells = <2>;
> 	};
> 
> 	device-to-be-reset {
> 		resets = <&gpio_reset 2 50>; /* GPIOD3, active-low, 50ms */
> 	};
> 
> I'd prefer the former over the latter. In any case, I think the timing
> information should reside either in the reset controller or in the
> framework.

Sure. I think the choice of properties in the controller node vs. extra
#reset-cells is probably an individual design decision for the specific
controller driver and DT binding; it would even be possible to have one
driver support either based on its #reset-cells value, but probably
over-complex and not worth doing. Although actually, when you start
thinking about DT overlays and so on, extra #reset-cells begins to make
more sense, since the values are distributed, which makes it easier to
set them from the overlay DT file...

[PATCH 2/7] reset: Add reset controller API

[Shawn Guo]

On Wed, Jan 16, 2013 at 05:13:02PM +0100, Philipp Zabel wrote:
> +#include <linux/reset.h>
> +#include <linux/reset-controller.h>

I failed to find these two headers in the series.

Shawn

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Philipp Zabel]

The SRC has auto-deasserting reset bits that control reset lines to
the GPU, VPU, IPU, and OpenVG IP modules. This patch adds a reset
controller that can be controlled by those devices using the
reset controller API.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/reset/fsl,imx-src.txt      |   49 +++++++++++++
 arch/arm/mach-imx/src.c                            |   72 ++++++++++++++++++++
 include/linux/imx-src.h                            |    6 ++
 3 files changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
 create mode 100644 include/linux/imx-src.h

diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
new file mode 100644
index 0000000..1330177
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
@@ -0,0 +1,49 @@
+Freescale i.MX System Reset Controller
+======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: Should be "fsl,<chip>-src"
+- reg: should be register base and length as documented in the
+  datasheet
+- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
+  in this order.
+- #reset-cells: 1, see below
+
+example:
+
+src: src at 020d8000 {
+        compatible = "fsl,imx6q-src";
+        reg = <0x020d8000 0x4000>;
+        interrupts = <0 91 0x04 0 96 0x04>;
+        #reset-cells = <1>;
+};
+
+Specifying reset lines connected to IP modules
+==============================================
+
+The system reset controller can be used to reset the GPU, VPU,
+IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
+nodes should specify the reset line on the SRC in their resets
+property, containing a phandle to the SRC device node and a
+RESET_INDEX specifying which module to reset, as described in
+reset.txt
+
+example:
+
+        ipu1: ipu at 02400000 {
+                resets = <&src 2>;
+        };
+        ipu2: ipu at 02800000 {
+                resets = <&src 4>;
+        };
+
+The following RESET_INDEX values are valid for i.MX5:
+GPU_RESET     0
+VPU_RESET     1
+IPU1_RESET    2
+OPEN_VG_RESET 3
+The following additional RESET_INDEX value is valid for i.MX6:
+IPU2_RESET    4
diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
index e15f155..41687c6 100644
--- a/arch/arm/mach-imx/src.c
+++ b/arch/arm/mach-imx/src.c
@@ -15,16 +15,85 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/smp.h>
+#include <linux/reset-controller.h>
 #include <asm/smp_plat.h>
 
 #define SRC_SCR				0x000
 #define SRC_GPR1			0x020
 #define BP_SRC_SCR_WARM_RESET_ENABLE	0
+#define BP_SRC_SCR_SW_GPU_RST		1
+#define BP_SRC_SCR_SW_VPU_RST		2
+#define BP_SRC_SCR_SW_IPU1_RST		3
+#define BP_SRC_SCR_SW_OPEN_VG_RST	4
+#define BP_SRC_SCR_SW_IPU2_RST		12
 #define BP_SRC_SCR_CORE1_RST		14
 #define BP_SRC_SCR_CORE1_ENABLE		22
 
 static void __iomem *src_base;
 
+static int sw_reset_bits[5] = {
+	BP_SRC_SCR_SW_GPU_RST,
+	BP_SRC_SCR_SW_VPU_RST,
+	BP_SRC_SCR_SW_IPU1_RST,
+	BP_SRC_SCR_SW_OPEN_VG_RST,
+	BP_SRC_SCR_SW_IPU2_RST
+};
+
+static int imx_src_reset(unsigned long sw_reset_idx)
+{
+	unsigned long timeout;
+	int bit;
+	u32 val;
+
+	if (!src_base)
+		return -ENODEV;
+
+	if (sw_reset_idx >= ARRAY_SIZE(sw_reset_bits))
+		return -EINVAL;
+
+	bit = 1 << sw_reset_bits[sw_reset_idx];
+
+	val = readl_relaxed(src_base + SRC_SCR);
+	val |= bit;
+	writel_relaxed(val, src_base + SRC_SCR);
+
+	timeout = jiffies + msecs_to_jiffies(1000);
+	while (readl(src_base + SRC_SCR) & bit) {
+		if (time_after(jiffies, timeout))
+			return -ETIME;
+		cpu_relax();
+	}
+
+	return 0;
+}
+
+static int imx_src_is_asserted(unsigned long sw_reset_idx)
+{
+	int bit;
+	u32 val;
+
+	if (!src_base)
+		return -ENODEV;
+
+	if (sw_reset_idx >= ARRAY_SIZE(sw_reset_bits))
+		return -EINVAL;
+
+	bit = 1 << sw_reset_bits[sw_reset_idx];
+
+	val = readl_relaxed(src_base + SRC_SCR);
+
+	return (val & bit) ? 1 : 0;
+}
+
+static struct reset_control_ops imx_src_ops = {
+	.reset = imx_src_reset,
+	.is_asserted = imx_src_is_asserted,
+};
+
+static struct reset_controller_dev imx_reset_controller = {
+	.ops = &imx_src_ops,
+};
+
 void imx_enable_cpu(int cpu, bool enable)
 {
 	u32 mask, val;
@@ -65,6 +134,9 @@ void __init imx_src_init(void)
 	src_base = of_iomap(np, 0);
 	WARN_ON(!src_base);
 
+	imx_reset_controller.of_node = np;
+	reset_controller_register(&imx_reset_controller);
+
 	/*
 	 * force warm reset sources to generate cold reset
 	 * for a more reliable restart
diff --git a/include/linux/imx-src.h b/include/linux/imx-src.h
new file mode 100644
index 0000000..b93ed96
--- /dev/null
+++ b/include/linux/imx-src.h
@@ -0,0 +1,6 @@
+#ifndef __IMX_SRC_H__
+#define __IMX_SRC_H__
+
+extern int imx_src_reset(int sw_reset_idx);
+
+#endif /* __IMX_SRC_H__ */
-- 
1.7.10.4

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Shawn Guo]

On Wed, Jan 16, 2013 at 05:13:03PM +0100, Philipp Zabel wrote:
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/smp.h>
> +#include <linux/reset-controller.h>

Move it one line above to have it sorted.

>  #include <asm/smp_plat.h>

...

> --- /dev/null
> +++ b/include/linux/imx-src.h
> @@ -0,0 +1,6 @@
> +#ifndef __IMX_SRC_H__
> +#define __IMX_SRC_H__
> +
> +extern int imx_src_reset(int sw_reset_idx);
> +
> +#endif /* __IMX_SRC_H__ */

This is header is not needed any more.

Shawn

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Matt Sealey]

On Wed, Jan 16, 2013 at 10:13 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> The SRC has auto-deasserting reset bits that control reset lines to
> the GPU, VPU, IPU, and OpenVG IP modules. This patch adds a reset
> controller that can be controlled by those devices using the
> reset controller API.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Hi Philipp,

I'm so glad someone actually sat down and coded this :)

+
+static int imx_src_reset(unsigned long sw_reset_idx)
+{

Having a name like imx_src_reset seems needlessly generic and
confusing. Surely we are performing a reset on an SoC unit and not
having the SRC itself reset, even if it is clearer when we look at the
argument?

+       timeout = jiffies + msecs_to_jiffies(1000);
+       while (readl(src_base + SRC_SCR) & bit) {
+               if (time_after(jiffies, timeout))
+                       return -ETIME;
+               cpu_relax();

... I do wonder though, all versions of the very-similar i.MX SRC
(i.MX51, i.MX53, i.MX6) implementing these bits actually have an
interrupt (with the same status-bit positions as the reset-enable
register) which fires when the unit actually resets.

Rather than poll with a timeout shouldn't we be waiting for the
interrupt and doing some completion event? It seems a little overly
involved to me to poll and cpu_relax() when we can just let the kernel
do whatever it likes while the hardware does the job.

It is technically impossible for the unit to "never reset" without
there being something hideously wrong elsewhere (i.e. if you ask the
VPU to reset, and it never fires the interrupt, you have far, far more
problems than just a locked VPU), but we actually should have no idea
without some empirical data (under every scenario at least) how long
it would actually take so having a timeout seems rather odd. Having a
timeout of a full second also *seems* to be far too long under the
same circumstances but I don't think anyone can predict what the real
values might be.

I looked at writing this exact same kind of code a long while back
including support for i.MX51 and i.MX53 as a cleanup for the older
version of Sascha's IPU driver, and simply never got it nice enough to
submit upstream (it is currently stuffed away in a huge backup disk
and I have no idea where the kernel tree is), but the way I handled it
was something like registering a real interrupt handler in the src
initialization function and then simply setting the bit and letting
the completion event do the work. I also did have a timeout for 5000ms
which basically would still capture any reset oddities - if we passed
5 seconds, and the unit did not reset, to start executing WARN_ON or
something to give the kernel developer (or user) a real indication
that something is *actually* hideously wrong with their board
implementation or the stability of their SoC, power rails, heatsink
etc.. or million other possibilities (any warning is at least better
than none).

I could never get the warning code to ever execute except in a
contrived test scenario (I set a reserved bit and faked that it never
fired an interrupt) but in my opinion, ANY warning on this kind of
failure to reset is better than just returning -ETIME to the reset
consumer and hoping the consumer reports a reasonable error to the
kernel log - if the SRC fails to reset a unit then this is not an
error condition so low in seriousness that telling the consumer
something timed out is adequate (based on the intended and functional
implementation of the SRC controller itself). As I said, what I
decided on was that I would return -ETIMEDOUT (the wrong code, but
bear with me, I was hacking) but before return, pr_err the problem
unit, and then WARN_ON inside the SRC driver itself, so that
everything would carry on (no system lockups or panics), but the
driver was not responsible for reporting the problem and the
seriousness was implicated in something a little more noticable than a
single line in a log.

I understand that waiting on an interrupt or completion event is not
available infrastructure in the current reset-controller code... but
maybe it should be a little more advanced than polling implementation
:D

I am not not-acking the code, and I would be overjoyed for it to go in
as-is (maybe with a function rename as above), but I would appreciate
the consideration that a reset-controller with some way of reporting a
successful reset other than polling is something that might come in
handy for other people (and i.MX SRC would be a highly desirable use
case) and at the very least in the case of the i.MX SRC, "this unit
did not reset after [possibly more than] a whole second of waiting" is
not encompassed within if (ret) pr_err("unit did not reset") in the
driver.. nor would this be an immediate and serious indication to the
driver or end-user.

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Philipp Zabel]

Hi Matt,

thank you for your comments.

Am Freitag, den 18.01.2013, 13:57 -0600 schrieb Matt Sealey:
> On Wed, Jan 16, 2013 at 10:13 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > The SRC has auto-deasserting reset bits that control reset lines to
> > the GPU, VPU, IPU, and OpenVG IP modules. This patch adds a reset
> > controller that can be controlled by those devices using the
> > reset controller API.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Hi Philipp,
> 
> I'm so glad someone actually sat down and coded this :)
> 
> +
> +static int imx_src_reset(unsigned long sw_reset_idx)
> +{
> 
> Having a name like imx_src_reset seems needlessly generic and
> confusing. Surely we are performing a reset on an SoC unit and not
> having the SRC itself reset, even if it is clearer when we look at the
> argument?

imx_src_reset_module, then? Also, I'll add the struct
reset_controller_dev pointer as an argument next time:

static int imx_src_reset_module(struct reset_controller_dev *rcdev,
                unsigned long sw_reset_idx)

> +       timeout = jiffies + msecs_to_jiffies(1000);
> +       while (readl(src_base + SRC_SCR) & bit) {
> +               if (time_after(jiffies, timeout))
> +                       return -ETIME;
> +               cpu_relax();
> 
> ... I do wonder though, all versions of the very-similar i.MX SRC
> (i.MX51, i.MX53, i.MX6) implementing these bits actually have an
> interrupt (with the same status-bit positions as the reset-enable
> register) which fires when the unit actually resets.
> 
> Rather than poll with a timeout shouldn't we be waiting for the
> interrupt and doing some completion event? It seems a little overly
> involved to me to poll and cpu_relax() when we can just let the kernel
> do whatever it likes while the hardware does the job.
> 
> It is technically impossible for the unit to "never reset" without
> there being something hideously wrong elsewhere (i.e. if you ask the
> VPU to reset, and it never fires the interrupt, you have far, far more
> problems than just a locked VPU), but we actually should have no idea
> without some empirical data (under every scenario at least) how long
> it would actually take so having a timeout seems rather odd. Having a
> timeout of a full second also *seems* to be far too long under the
> same circumstances but I don't think anyone can predict what the real
> values might be.
> 
> I looked at writing this exact same kind of code a long while back
> including support for i.MX51 and i.MX53 as a cleanup for the older
> version of Sascha's IPU driver, and simply never got it nice enough to
> submit upstream (it is currently stuffed away in a huge backup disk
> and I have no idea where the kernel tree is), but the way I handled it
> was something like registering a real interrupt handler in the src
> initialization function and then simply setting the bit and letting
> the completion event do the work. I also did have a timeout for 5000ms
> which basically would still capture any reset oddities - if we passed
> 5 seconds, and the unit did not reset, to start executing WARN_ON or
> something to give the kernel developer (or user) a real indication
> that something is *actually* hideously wrong with their board
> implementation or the stability of their SoC, power rails, heatsink
> etc.. or million other possibilities (any warning is at least better
> than none).
> 
> I could never get the warning code to ever execute except in a
> contrived test scenario (I set a reserved bit and faked that it never
> fired an interrupt) but in my opinion, ANY warning on this kind of
> failure to reset is better than just returning -ETIME to the reset
> consumer and hoping the consumer reports a reasonable error to the
> kernel log - if the SRC fails to reset a unit then this is not an
> error condition so low in seriousness that telling the consumer
> something timed out is adequate (based on the intended and functional
> implementation of the SRC controller itself). As I said, what I
> decided on was that I would return -ETIMEDOUT (the wrong code, but
> bear with me, I was hacking) but before return, pr_err the problem
> unit, and then WARN_ON inside the SRC driver itself, so that
> everything would carry on (no system lockups or panics), but the
> driver was not responsible for reporting the problem and the
> seriousness was implicated in something a little more noticable than a
> single line in a log.
> 
> I understand that waiting on an interrupt or completion event is not
> available infrastructure in the current reset-controller code... but
> maybe it should be a little more advanced than polling implementation
> :D

Yes, maybe the module reset part of the SRC should be implemented as a
proper device driver in drivers/reset. Then we could use the interrupt
functionality and WARN_ON(timeout), as you suggest.

> I am not not-acking the code, and I would be overjoyed for it to go in
> as-is (maybe with a function rename as above), but I would appreciate
> the consideration that a reset-controller with some way of reporting a
> successful reset other than polling is something that might come in
> handy for other people (and i.MX SRC would be a highly desirable use
> case) and at the very least in the case of the i.MX SRC, "this unit
> did not reset after [possibly more than] a whole second of waiting" is
> not encompassed within if (ret) pr_err("unit did not reset") in the
> driver.. nor would this be an immediate and serious indication to the
> driver or end-user.

regards
Philipp

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Matt Sealey]

On Mon, Jan 21, 2013 at 3:52 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Matt,
>
> thank you for your comments.
>
> Am Freitag, den 18.01.2013, 13:57 -0600 schrieb Matt Sealey:
>> On Wed, Jan 16, 2013 at 10:13 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>
>> +
>> +static int imx_src_reset(unsigned long sw_reset_idx)
>> +{
>>
>> Having a name like imx_src_reset seems needlessly generic and
>> confusing. Surely we are performing a reset on an SoC unit and not
>> having the SRC itself reset, even if it is clearer when we look at the
>> argument?
>
> imx_src_reset_module, then? Also, I'll add the struct
> reset_controller_dev pointer as an argument next time:
>
> static int imx_src_reset_module(struct reset_controller_dev *rcdev,
>                 unsigned long sw_reset_idx)

Yes, sure.

> Yes, maybe the module reset part of the SRC should be implemented as a
> proper device driver in drivers/reset. Then we could use the interrupt
> functionality and WARN_ON(timeout), as you suggest.

That would be ideal. Maybe Shawn or Fabio can bug a hardware guy to
shed some light on what a reasonable timeout might actually be for a
module to cause such a warning. I think it should definitely cause
one.. as I said, I was using 5 seconds, you used 1 second, I don't
think a shorter delay would be unreasonable, but maybe it could take
up to 10 seconds, or maybe I am wrong - maybe it is in fact impossible
in hardware for a reset to "fail" at least visibly because the
interrupt will always fire making the warning a never-traveled path.
It is certainly not something that would be documented, so without a
view of the RTL logic here, we wouldn't know.

Shawn, Fabio?

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

[PATCH 3/7] ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)

[Shawn Guo]

On Mon, Jan 21, 2013 at 11:47:12AM -0600, Matt Sealey wrote:
> > Yes, maybe the module reset part of the SRC should be implemented as a
> > proper device driver in drivers/reset. Then we could use the interrupt
> > functionality and WARN_ON(timeout), as you suggest.
> 
> That would be ideal. Maybe Shawn or Fabio can bug a hardware guy to
> shed some light on what a reasonable timeout might actually be for a
> module to cause such a warning. I think it should definitely cause
> one.. as I said, I was using 5 seconds, you used 1 second, I don't
> think a shorter delay would be unreasonable, but maybe it could take
> up to 10 seconds, or maybe I am wrong - maybe it is in fact impossible
> in hardware for a reset to "fail" at least visibly because the
> interrupt will always fire making the warning a never-traveled path.
> It is certainly not something that would be documented, so without a
> view of the RTL logic here, we wouldn't know.
> 
> Shawn, Fabio?
> 
Just talked to hardware people, the reset should finish in a few
clock cycles, so even 1 millisecond should be enough.

Shawn

[PATCH 4/7] ARM i.MX6q: Link system reset controller (SRC) to IPU in DT

[Philipp Zabel]

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/boot/dts/imx6q.dtsi |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index d6265ca..c445959 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -555,6 +555,7 @@
 				compatible = "fsl,imx6q-src";
 				reg = <0x020d8000 0x4000>;
 				interrupts = <0 91 0x04 0 96 0x04>;
+				#reset-cells = <1>;
 			};
 
 			gpc: gpc at 020dc000 {
@@ -1046,6 +1047,7 @@
 			interrupts = <0 6 0x4 0 5 0x4>;
 			clocks = <&clks 130>, <&clks 131>, <&clks 132>;
 			clock-names = "bus", "di0", "di1";
+			resets = <&src 2>;
 		};
 
 		ipu2: ipu at 02800000 {
@@ -1055,6 +1057,7 @@
 			interrupts = <0 8 0x4 0 7 0x4>;
 			clocks = <&clks 133>, <&clks 134>, <&clks 137>;
 			clock-names = "bus", "di0", "di1";
+			reset = <&src 4>;
 		};
 	};
 };
-- 
1.7.10.4

[PATCH 4/7] ARM i.MX6q: Link system reset controller (SRC) to IPU in DT

[Shawn Guo]

On Wed, Jan 16, 2013 at 05:13:04PM +0100, Philipp Zabel wrote:
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx6q.dtsi |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index d6265ca..c445959 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -555,6 +555,7 @@
>  				compatible = "fsl,imx6q-src";
>  				reg = <0x020d8000 0x4000>;
>  				interrupts = <0 91 0x04 0 96 0x04>;
> +				#reset-cells = <1>;
>  			};
>  
>  			gpc: gpc at 020dc000 {
> @@ -1046,6 +1047,7 @@
>  			interrupts = <0 6 0x4 0 5 0x4>;
>  			clocks = <&clks 130>, <&clks 131>, <&clks 132>;
>  			clock-names = "bus", "di0", "di1";
> +			resets = <&src 2>;
>  		};
>  
>  		ipu2: ipu at 02800000 {
> @@ -1055,6 +1057,7 @@
>  			interrupts = <0 8 0x4 0 7 0x4>;
>  			clocks = <&clks 133>, <&clks 134>, <&clks 137>;
>  			clock-names = "bus", "di0", "di1";
> +			reset = <&src 4>;

s/reset/resets

Shawn

>  		};
>  	};
>  };
> -- 
> 1.7.10.4
> 

[PATCH 5/7] staging: drm/imx: Use SRC to reset IPU

[Philipp Zabel]

Request the System Reset Controller to reset the IPU if
specified via device tree phandle.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt    |    3 +++
 drivers/staging/imx-drm/ipu-v3/ipu-common.c                |   12 +++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
index 07654f0..f769857 100644
--- a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
+++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
@@ -8,6 +8,8 @@ Required properties:
 - interrupts: Should contain sync interrupt and error interrupt,
   in this order.
 - #crtc-cells: 1, See below
+- resets: phandle pointing to the system reset controller and
+          reset line index, see reset/fsl,imx-src.txt for details
 
 example:
 
@@ -16,6 +18,7 @@ ipu: ipu at 18000000 {
 	compatible = "fsl,imx53-ipu";
 	reg = <0x18000000 0x080000000>;
 	interrupts = <11 10>;
+	resets = <&src 2>;
 };
 
 Parallel display support
diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
index f7059cd..6a6211d 100644
--- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
+++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/init.h>
+#include <linux/reset.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/spinlock.h>
@@ -660,7 +661,7 @@ int ipu_idmac_disable_channel(struct ipuv3_channel *channel)
 }
 EXPORT_SYMBOL_GPL(ipu_idmac_disable_channel);
 
-static int ipu_reset(struct ipu_soc *ipu)
+static int ipu_memory_reset(struct ipu_soc *ipu)
 {
 	unsigned long timeout;
 
@@ -1104,7 +1105,12 @@ static int ipu_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_failed_irq;
 
-	ret = ipu_reset(ipu);
+	ret = device_reset(&pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to reset: %d\n", ret);
+		goto out_failed_reset;
+	}
+	ret = ipu_memory_reset(ipu);
 	if (ret)
 		goto out_failed_reset;
 
@@ -1130,8 +1136,8 @@ static int ipu_probe(struct platform_device *pdev)
 failed_add_clients:
 	ipu_submodules_exit(ipu);
 failed_submodules_init:
-	ipu_irq_exit(ipu);
 out_failed_reset:
+	ipu_irq_exit(ipu);
 out_failed_irq:
 	clk_disable_unprepare(ipu->clk);
 failed_clk_get:
-- 
1.7.10.4

[PATCH 6/7] ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53

[Philipp Zabel]

The SRC in i.MX51 and i.MX53 is similar to the one in i.MX6q minus
the IPU2 reset line and multi core CPU reset/enable bits.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/mach-imx/Kconfig      |    1 +
 arch/arm/mach-imx/common.h     |    3 ++-
 arch/arm/mach-imx/mach-imx6q.c |    2 +-
 arch/arm/mach-imx/mm-imx5.c    |    2 ++
 arch/arm/mach-imx/src.c        |   14 +++++++++++++-
 5 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 3e628fd..d7924e5 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -829,6 +829,7 @@ config	SOC_IMX53
 	select ARCH_MX53
 	select HAVE_CAN_FLEXCAN if CAN
 	select IMX_HAVE_PLATFORM_IMX2_WDT
+	select HAVE_IMX_SRC
 	select PINCTRL
 	select PINCTRL_IMX53
 	select SOC_IMX5
diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index 7191ab4..f36be3c 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -133,7 +133,8 @@ static inline void imx_smp_prepare(void) {}
 #endif
 extern void imx_enable_cpu(int cpu, bool enable);
 extern void imx_set_cpu_jump(int cpu, void *jump_addr);
-extern void imx_src_init(void);
+extern void imx5_src_init(void);
+extern void imx6q_src_init(void);
 extern void imx_src_prepare_restart(void);
 extern void imx_gpc_init(void);
 extern void imx_gpc_pre_suspend(void);
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index cd277a0..b1e076c 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -229,7 +229,7 @@ static const struct of_device_id imx6q_irq_match[] __initconst = {
 static void __init imx6q_init_irq(void)
 {
 	l2x0_of_init(0, ~0UL);
-	imx_src_init();
+	imx6q_src_init();
 	imx_gpc_init();
 	of_irq_init(imx6q_irq_match);
 }
diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
index 79d71cf..53f87be 100644
--- a/arch/arm/mach-imx/mm-imx5.c
+++ b/arch/arm/mach-imx/mm-imx5.c
@@ -106,6 +106,7 @@ void __init imx51_init_early(void)
 	mxc_set_cpu_type(MXC_CPU_MX51);
 	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
 	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
+	imx5_src_init();
 }
 
 void __init imx53_init_early(void)
@@ -113,6 +114,7 @@ void __init imx53_init_early(void)
 	mxc_set_cpu_type(MXC_CPU_MX53);
 	mxc_iomux_v3_init(MX53_IO_ADDRESS(MX53_IOMUXC_BASE_ADDR));
 	mxc_arch_reset_init(MX53_IO_ADDRESS(MX53_WDOG1_BASE_ADDR));
+	imx5_src_init();
 }
 
 void __init mx50_init_irq(void)
diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
index 41687c6..e350250 100644
--- a/arch/arm/mach-imx/src.c
+++ b/arch/arm/mach-imx/src.c
@@ -125,7 +125,19 @@ void imx_src_prepare_restart(void)
 	writel_relaxed(0, src_base + SRC_GPR1);
 }
 
-void __init imx_src_init(void)
+void __init imx5_src_init(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx5-src");
+	src_base = of_iomap(np, 0);
+	WARN_ON(!src_base);
+
+	imx_reset_controller.of_node = np;
+	reset_controller_register(&imx_reset_controller);
+}
+
+void __init imx6q_src_init(void)
 {
 	struct device_node *np;
 	u32 val;
-- 
1.7.10.4

[PATCH 6/7] ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53

[Shawn Guo]

On Wed, Jan 16, 2013 at 05:13:06PM +0100, Philipp Zabel wrote:
> The SRC in i.MX51 and i.MX53 is similar to the one in i.MX6q minus
> the IPU2 reset line and multi core CPU reset/enable bits.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  arch/arm/mach-imx/Kconfig      |    1 +
>  arch/arm/mach-imx/common.h     |    3 ++-
>  arch/arm/mach-imx/mach-imx6q.c |    2 +-
>  arch/arm/mach-imx/mm-imx5.c    |    2 ++
>  arch/arm/mach-imx/src.c        |   14 +++++++++++++-
>  5 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 3e628fd..d7924e5 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -829,6 +829,7 @@ config	SOC_IMX53
>  	select ARCH_MX53
>  	select HAVE_CAN_FLEXCAN if CAN
>  	select IMX_HAVE_PLATFORM_IMX2_WDT
> +	select HAVE_IMX_SRC

Please sort it in name.  Should we manage to have it selected for imx51
too, since you have code added for imx51 below?

>  	select PINCTRL
>  	select PINCTRL_IMX53
>  	select SOC_IMX5
> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> index 7191ab4..f36be3c 100644
> --- a/arch/arm/mach-imx/common.h
> +++ b/arch/arm/mach-imx/common.h
> @@ -133,7 +133,8 @@ static inline void imx_smp_prepare(void) {}
>  #endif
>  extern void imx_enable_cpu(int cpu, bool enable);
>  extern void imx_set_cpu_jump(int cpu, void *jump_addr);
> -extern void imx_src_init(void);
> +extern void imx5_src_init(void);
> +extern void imx6q_src_init(void);
>  extern void imx_src_prepare_restart(void);
>  extern void imx_gpc_init(void);
>  extern void imx_gpc_pre_suspend(void);
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index cd277a0..b1e076c 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -229,7 +229,7 @@ static const struct of_device_id imx6q_irq_match[] __initconst = {
>  static void __init imx6q_init_irq(void)
>  {
>  	l2x0_of_init(0, ~0UL);
> -	imx_src_init();
> +	imx6q_src_init();

I'm not sure this is necessary.  See below ...

>  	imx_gpc_init();
>  	of_irq_init(imx6q_irq_match);
>  }
> diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
> index 79d71cf..53f87be 100644
> --- a/arch/arm/mach-imx/mm-imx5.c
> +++ b/arch/arm/mach-imx/mm-imx5.c
> @@ -106,6 +106,7 @@ void __init imx51_init_early(void)
>  	mxc_set_cpu_type(MXC_CPU_MX51);
>  	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
>  	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> +	imx5_src_init();
>  }
>  
>  void __init imx53_init_early(void)
> @@ -113,6 +114,7 @@ void __init imx53_init_early(void)
>  	mxc_set_cpu_type(MXC_CPU_MX53);
>  	mxc_iomux_v3_init(MX53_IO_ADDRESS(MX53_IOMUXC_BASE_ADDR));
>  	mxc_arch_reset_init(MX53_IO_ADDRESS(MX53_WDOG1_BASE_ADDR));
> +	imx5_src_init();
>  }
>  
>  void __init mx50_init_irq(void)
> diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
> index 41687c6..e350250 100644
> --- a/arch/arm/mach-imx/src.c
> +++ b/arch/arm/mach-imx/src.c
> @@ -125,7 +125,19 @@ void imx_src_prepare_restart(void)
>  	writel_relaxed(0, src_base + SRC_GPR1);
>  }
>  
> -void __init imx_src_init(void)
> +void __init imx5_src_init(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx5-src");

In fsl,imx-src.txt, we have 

  compatible: Should be "fsl,<chip>-src"

But imx5 is not a chip name.  I would suggest we have the imx-src driver
only look for compatible "fsl,imx51-src", and for dts

  imx51: compatible = "fsl,imx51-src";
  imx53: compatible = "fsl,imx53-src", "fsl,imx51-src";
  imx6q: compatible = "fsl,imx6q-src", "fsl,imx51-src";

so that we do not need imx5_src_init and imx6q_src_init which are
basically doing the same thing.

> +	src_base = of_iomap(np, 0);
> +	WARN_ON(!src_base);

As imx51 still supports non-DT boot, we should have a check on np.
If np is NULL, mostly likely it's a non-DT boot on imx51, and we
should bail out instead of giving a fat warning.

Shawn

> +
> +	imx_reset_controller.of_node = np;
> +	reset_controller_register(&imx_reset_controller);
> +}
> +
> +void __init imx6q_src_init(void)
>  {
>  	struct device_node *np;
>  	u32 val;
> -- 
> 1.7.10.4
> 

[PATCH 6/7] ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53

[Philipp Zabel]

Hi Shawn,

thank you for your comments.

Am Donnerstag, den 17.01.2013, 14:12 +0800 schrieb Shawn Guo:
> On Wed, Jan 16, 2013 at 05:13:06PM +0100, Philipp Zabel wrote:
> > The SRC in i.MX51 and i.MX53 is similar to the one in i.MX6q minus
> > the IPU2 reset line and multi core CPU reset/enable bits.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  arch/arm/mach-imx/Kconfig      |    1 +
> >  arch/arm/mach-imx/common.h     |    3 ++-
> >  arch/arm/mach-imx/mach-imx6q.c |    2 +-
> >  arch/arm/mach-imx/mm-imx5.c    |    2 ++
> >  arch/arm/mach-imx/src.c        |   14 +++++++++++++-
> >  5 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index 3e628fd..d7924e5 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -829,6 +829,7 @@ config	SOC_IMX53
> >  	select ARCH_MX53
> >  	select HAVE_CAN_FLEXCAN if CAN
> >  	select IMX_HAVE_PLATFORM_IMX2_WDT
> > +	select HAVE_IMX_SRC
> 
> Please sort it in name.  Should we manage to have it selected for imx51
> too, since you have code added for imx51 below?

Yes, I'll add that.

> >  	select PINCTRL
> >  	select PINCTRL_IMX53
> >  	select SOC_IMX5
> > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> > index 7191ab4..f36be3c 100644
> > --- a/arch/arm/mach-imx/common.h
> > +++ b/arch/arm/mach-imx/common.h
> > @@ -133,7 +133,8 @@ static inline void imx_smp_prepare(void) {}
> >  #endif
> >  extern void imx_enable_cpu(int cpu, bool enable);
> >  extern void imx_set_cpu_jump(int cpu, void *jump_addr);
> > -extern void imx_src_init(void);
> > +extern void imx5_src_init(void);
> > +extern void imx6q_src_init(void);
> >  extern void imx_src_prepare_restart(void);
> >  extern void imx_gpc_init(void);
> >  extern void imx_gpc_pre_suspend(void);
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> > index cd277a0..b1e076c 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -229,7 +229,7 @@ static const struct of_device_id imx6q_irq_match[] __initconst = {
> >  static void __init imx6q_init_irq(void)
> >  {
> >  	l2x0_of_init(0, ~0UL);
> > -	imx_src_init();
> > +	imx6q_src_init();
> 
> I'm not sure this is necessary.  See below ...
> 
> >  	imx_gpc_init();
> >  	of_irq_init(imx6q_irq_match);
> >  }
> > diff --git a/arch/arm/mach-imx/mm-imx5.c b/arch/arm/mach-imx/mm-imx5.c
> > index 79d71cf..53f87be 100644
> > --- a/arch/arm/mach-imx/mm-imx5.c
> > +++ b/arch/arm/mach-imx/mm-imx5.c
> > @@ -106,6 +106,7 @@ void __init imx51_init_early(void)
> >  	mxc_set_cpu_type(MXC_CPU_MX51);
> >  	mxc_iomux_v3_init(MX51_IO_ADDRESS(MX51_IOMUXC_BASE_ADDR));
> >  	mxc_arch_reset_init(MX51_IO_ADDRESS(MX51_WDOG1_BASE_ADDR));
> > +	imx5_src_init();
> >  }
> >  
> >  void __init imx53_init_early(void)
> > @@ -113,6 +114,7 @@ void __init imx53_init_early(void)
> >  	mxc_set_cpu_type(MXC_CPU_MX53);
> >  	mxc_iomux_v3_init(MX53_IO_ADDRESS(MX53_IOMUXC_BASE_ADDR));
> >  	mxc_arch_reset_init(MX53_IO_ADDRESS(MX53_WDOG1_BASE_ADDR));
> > +	imx5_src_init();
> >  }
> >  
> >  void __init mx50_init_irq(void)
> > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
> > index 41687c6..e350250 100644
> > --- a/arch/arm/mach-imx/src.c
> > +++ b/arch/arm/mach-imx/src.c
> > @@ -125,7 +125,19 @@ void imx_src_prepare_restart(void)
> >  	writel_relaxed(0, src_base + SRC_GPR1);
> >  }
> >  
> > -void __init imx_src_init(void)
> > +void __init imx5_src_init(void)
> > +{
> > +	struct device_node *np;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx5-src");
> 
> In fsl,imx-src.txt, we have 
> 
>   compatible: Should be "fsl,<chip>-src"
> 
> But imx5 is not a chip name.  I would suggest we have the imx-src driver
> only look for compatible "fsl,imx51-src", and for dts
> 
>   imx51: compatible = "fsl,imx51-src";
>   imx53: compatible = "fsl,imx53-src", "fsl,imx51-src";
>   imx6q: compatible = "fsl,imx6q-src", "fsl,imx51-src";
> 
> so that we do not need imx5_src_init and imx6q_src_init which are
> basically doing the same thing.

So we should unconditionally clear the BP_SRC_SCR_WARM_RESET_ENABLE bit
on i.MX5, too? I'll try that.

> > +	src_base = of_iomap(np, 0);
> > +	WARN_ON(!src_base);
> 
> As imx51 still supports non-DT boot, we should have a check on np.
> If np is NULL, mostly likely it's a non-DT boot on imx51, and we
> should bail out instead of giving a fat warning.

Ok. I'll apply this and the comments from your other mails before
sending the next version.

regards
Philipp

[PATCH 7/7] ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree

[Philipp Zabel]

Also, link SRC to IPU via phandle.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 arch/arm/boot/dts/imx51.dtsi |    7 +++++++
 arch/arm/boot/dts/imx53.dtsi |    7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi
index 1f5d45e..1ff0adf 100644
--- a/arch/arm/boot/dts/imx51.dtsi
+++ b/arch/arm/boot/dts/imx51.dtsi
@@ -67,6 +67,7 @@
 			compatible = "fsl,imx51-ipu";
 			reg = <0x40000000 0x20000000>;
 			interrupts = <11 10>;
+			resets = <&src 2>;
 		};
 
 		aips at 70000000 { /* AIPS1 */
@@ -448,6 +449,12 @@
 				status = "disabled";
 			};
 
+			src: src at 73fd0000 {
+				compatible = "fsl,imx5-src";
+				reg = <0x73fd0000 0x4000>;
+				#reset-cells = <1>;
+			};
+
 			clks: ccm at 73fd4000{
 				compatible = "fsl,imx51-ccm";
 				reg = <0x73fd4000 0x4000>;
diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index edc3f1e..088dc49 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -72,6 +72,7 @@
 			compatible = "fsl,imx53-ipu";
 			reg = <0x18000000 0x080000000>;
 			interrupts = <11 10>;
+			resets = <&src 2>;
 		};
 
 		aips at 50000000 { /* AIPS1 */
@@ -497,6 +498,12 @@
 				status = "disabled";
 			};
 
+			src: src at 53fd0000 {
+				compatible = "fsl,imx5-src";
+				reg = <0x53fd0000 0x4000>;
+				#reset-cells = <1>;
+			};
+
 			clks: ccm at 53fd4000{
 				compatible = "fsl,imx53-ccm";
 				reg = <0x53fd4000 0x4000>;
-- 
1.7.10.4

[PATCH 7/7] ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree

[Stephen Warren]

On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> Also, link SRC to IPU via phandle.

Aside from the comments I already made, the series,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

although I'm not 100% sure if the ordering of patches maintains a
working i.MX build all the time? Does "git bisect" work across the series?

Who would merge these patches?

[PATCH 7/7] ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree

[Shawn Guo]

On Wed, Jan 16, 2013 at 03:19:44PM -0700, Stephen Warren wrote:
> On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> > Also, link SRC to IPU via phandle.
> 
> Aside from the comments I already made, the series,
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> 
> although I'm not 100% sure if the ordering of patches maintains a
> working i.MX build all the time? Does "git bisect" work across the series?
> 
> Who would merge these patches?

I would assume IMX --> arm-soc --> mainline?

Philipp, please copy arm-soc folks (Arnd and Olof) on the next post,
so that they can get a chance to review this tiny reset subsystem.

Shawn

[PATCH 7/7] ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree

[Philipp Zabel]

Am Mittwoch, den 16.01.2013, 15:19 -0700 schrieb Stephen Warren:
> On 01/16/2013 09:13 AM, Philipp Zabel wrote:
> > Also, link SRC to IPU via phandle.
> 
> Aside from the comments I already made, the series,
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Thank you.

> although I'm not 100% sure if the ordering of patches maintains a
> working i.MX build all the time? Does "git bisect" work across the series?

Since IPU probing doesn't succeed if device_reset() fails, the IPU patch
needs to be ordered after the i.MX5 patches.

On the other hand, should device_reset() fail at all if no reset
controller was specified in the device tree?

regards
Philipp

[RFC PATCH 0/5] Reset controller API to reset IP modules on i.MX5 and i.MX6

[Marek Vasut]

Dear Philipp Zabel,

> The system reset controller (SRC) on i.MX51, i.MX53, and i.MX6q controls
> reset lines to the GPU, VPU, IPU, and OpenVG IP modules.
> 
> The following patches add a simple API for devices to request being reset
> by separate reset controller hardware. They implement the reset signal
> device tree binding proposed by Stephen Warren. Contrary to Tegra hardware,
> the i.MX SRC has self-deasserting reset bits, so I've included both
> ops to manually assert/deassert a reset line, as well as a reset
> operation that is supposed to assert the reset line and wait for it to
> deassert.
> 
> The i.MX SRC is enhanced to provide a reset controller and the IPU driver
> is made to request being reset by calling the device_reset(&pdev->dev)
> convenience wrapper during probing.
> 
> regards
> Philipp

I won't be able to test it before 23rd this month. Sorry!

Best regards,
Marek Vasut