Reset controller API to reset IP modules on i.MX5 and i.MX6
Subsystems (6)
Philipp Zabel
maintainercover Reset controller API to reset IP modules on i.MX5 and i.MX6
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(-)
Philipp Zabel
maintainerPATCH 1/7 dt: describe base reset signal binding
From: Stephen Warren <[email protected]>
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).
--- Documentation/devicetree/bindings/reset/reset.txt | 75 +++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/reset.txt
Documentation/devicetree/bindings/reset/reset.txt new file +75 -0
@@ -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.
Signature · 1 line
1.7.10.4
PATCH 1/7 dt: describe base reset signal binding
Quoted 8 lines · Philipp Zabel
From: Stephen Warren <[email protected]>
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:
Quoted 8 lines
void
tegra_periph_reset_deassert(struct clk *c);
voidtegra_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 <[email protected]>
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).
Philipp Zabel
maintainerPATCH 2/7 reset: Add reset controller API
In reply to Philipp ZabelThis adds a simple API for devices to request being reset by separate reset controller hardware and implements the reset signal device tree binding.
--- 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
drivers/Kconfig +2 -0
@@ -158,4 +158,6 @@ source "drivers/irqchip/Kconfig" source "drivers/ipack/Kconfig" +source "drivers/reset/Kconfig" + endmenu
drivers/Makefile +2 -0
@@ -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/
drivers/reset/Kconfig new file +10 -0
@@ -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. +
drivers/reset/Makefile new file +2 -0
@@ -0,0 +1,2 @@ +obj-$(CONFIG_RESET_CONTROLLER) += core.o +
drivers/reset/core.c new file +241 -0
241 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Sascha Hauer
maintainerPATCH 2/7 reset: Add reset controller API
Quoted 91 lines · 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 <[email protected]>--- 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.cdrivers/Kconfig +2
-0@@ -158,4 +158,6 @@ source "drivers/irqchip/Kconfig" source "drivers/ipack/Kconfig" +source "drivers/reset/Kconfig" + endmenudrivers/Makefile +2
-0@@ -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/drivers/reset/Kconfig new file +10
-0@@ -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. +drivers/reset/Makefile new file +2
-0@@ -0,0 +1,2 @@ +obj-$(CONFIG_RESET_CONTROLLER) += core.o +drivers/reset/core.c new file +21
-0@@ -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;
drivers/reset/core.c line 31
Quoted 10 lines
drivers/reset/core.c new file +10
-0+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.
Quoted 41 lines
drivers/reset/core.c new file +41
-0+*/ +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)
drivers/reset/core.c line 82
Quoted 10 lines
drivers/reset/core.c new file +10
-0+{ +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
Signature · 4 lines
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
In reply to Philipp Zabeldrivers/reset/Makefile line 2
Quoted 9 lines · 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.
drivers/reset/Makefile +2
-0+obj-$(CONFIG_RESET_CONTROLLER) += core.o +
nit: blank line at EOF.
drivers/reset/core.c line 46
Quoted 7 lines
drivers/reset/core.c +5
-0+/** +* 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?
drivers/reset/core.c line 52
Quoted 6 lines
drivers/reset/core.c +6
-0+{ +if (rstc->rcdev->ops->reset) +return rstc->rcdev->ops->reset (rstc->id); + +return 0 ; +}
If there's no op, shouldn't the function fail?
Quoted 2 lines
+/**
+ *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?
Quoted 6 lines
+/**
+ *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 structreset_controlorIS_ERR()condition containing errno.
OK, so NULL can't ever (legally) be returned.
Quoted 4 lines
drivers/reset/core.c +3
-0+struct reset_control *reset_control_get (struct device *dev,const char *id) + rcdev_node =NULL ; +for (i =0 ; rcdev_node ==NULL ; i++) {
...
drivers/reset/core.c line 138
Quoted 2 lines
drivers/reset/core.c +2
-0+ 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.
drivers/reset/core.c line 175
Quoted 4 lines
drivers/reset/core.c +4
-0+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...
drivers/reset/core.c line 178
Quoted 2 lines
drivers/reset/core.c +2
-0+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...
Philipp Zabel
maintainerPATCH 2/7 reset: Add reset controller API
Hi Stephen,
Am Mittwoch, den 16.01.2013, 15:15 -0700 schrieb Stephen Warren:
Quoted 6 lines · Philipp Zabel
Quoted 3 lines
This adds a simple API for devices to request being reset by separate reset controller hardware and implements the reset signal device tree binding.
Quoted 6 lines
drivers/reset/Makefile line 2
Quoted 4 lines
drivers/reset/Makefile +2
-0+obj-$(CONFIG_RESET_CONTROLLER) += core.o +nit: blank line at EOF.
Ok.
Quoted 10 lines
drivers/reset/core.c line 46
Quoted 7 lines
drivers/reset/core.c +5
-0+/** +* 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.
Quoted 8 lines
drivers/reset/core.c line 52
Quoted 6 lines
drivers/reset/core.c +6
-0+{ +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.
Quoted 7 lines
Quoted 2 lines
+/**
+ *reset_control_is_asserted- deasserts the reset lineComment 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.
Quoted 8 lines
Quoted 6 lines
+/**
+ *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 structreset_controlorIS_ERR()condition containing errno.OK, so NULL can't ever (legally) be returned.
Right.
Quoted 11 lines
Quoted 4 lines
drivers/reset/core.c +3
-0+struct reset_control *reset_control_get (struct device *dev,const char *id) + rcdev_node =NULL ; +for (i =0 ; rcdev_node ==NULL ; i++) {...
drivers/reset/core.c line 138
Quoted 2 lines
drivers/reset/core.c +2
-0+ 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.
Quoted 9 lines
drivers/reset/core.c line 175
Quoted 4 lines
drivers/reset/core.c +4
-0+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.
Quoted 5 lines
drivers/reset/core.c line 178
Quoted 2 lines
drivers/reset/core.c +2
-0+kfree (rstc); +module_put (rstc->rcdev->owner);You need to
module_put()first, or copyrstc->rcdev, since otherwise you're reading *rstc after it's freed.
Ow, sorry for wasting your time with this one.
Quoted 3 lines
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
Quoted 29 lines · Philipp Zabel
Hi Stephen,
Am Mittwoch, den 16.01.2013, 15:15 -0700 schrieb Stephen Warren:
Quoted 6 lines · Philipp Zabel
Quoted 3 lines
This adds a simple API for devices to request being reset by separate reset controller hardware and implements the reset signal device tree binding.
Quoted 6 lines
drivers/reset/Makefile line 2
Quoted 4 lines
drivers/reset/Makefile +2
-0+obj-$(CONFIG_RESET_CONTROLLER) += core.o +nit: blank line at EOF.
Ok.
Quoted 10 lines
drivers/reset/core.c line 46
Quoted 7 lines
drivers/reset/core.c +5
-0+/** +* 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?
Quoted 5 lines
"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.
Quoted 31 lines
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 */ };
Quoted 3 lines
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
In reply to Philipp Zabeldrivers/reset/core.c line 8
Quoted 2 lines · Philipp Zabel
drivers/reset/core.c new file +2
-0+#include <linux/reset.h> +#include <linux/reset-controller.h>
I failed to find these two headers in the series.
Shawn
Philipp Zabel
maintainerPATCH 3/7 ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)
In reply to Philipp ZabelThe 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.
--- .../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
Documentation/devicetree/bindings/reset/fsl,imx-src.txt new file +49 -0
@@ -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
arch/arm/mach-imx/src.c +72 -0
94 lines not on this page · raw message
include/linux/imx-src.h new file +6 -0
@@ -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__ */
Signature · 1 line
1.7.10.4
PATCH 3/7 ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)
Quoted 4 lines · Philipp Zabel
#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.
Quoted 1 line
#include <
asm/smp_plat.h>
...
include/linux/imx-src.h line 6
Quoted 9 lines
include/linux/imx-src.h new file +6
-0@@ -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)
In reply to Philipp ZabelQuoted 6 lines · 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 <[email protected]>
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 <[email protected]>
Product Development Analyst, Genesi USA, Inc.
Philipp Zabel
maintainerPATCH 3/7 ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)
Hi Matt,
thank you for your comments.
Am Freitag, den 18.01.2013, 13:57 -0600 schrieb Matt Sealey:
Quoted 16 lines · Philipp Zabel
Quoted 6 lines
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 <[email protected]>Hi Philipp,
I'm so glad someone actually sat down and coded this :)
arch/arm/mach-imx/src.c +3
-0+ +static int imx_src_reset (unsigned long sw_reset_idx) +{
Quoted 4 lines
Having a name like
imx_src_resetseems 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)
Quoted 59 lines
arch/arm/mach-imx/src.c +5
-0+ 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_ONor 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
-ETIMEto 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_errthe problem unit, and thenWARN_ONinside 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.
Quoted 4 lines
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.
Quoted 10 lines
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)
Quoted 19 lines · Philipp Zabel
Hi Matt,
thank you for your comments.
Am Freitag, den 18.01.2013, 13:57 -0600 schrieb Matt Sealey:
Quoted 10 lines · Philipp Zabel
arch/arm/mach-imx/src.c +3
-0+ +static int imx_src_reset (unsigned long sw_reset_idx) +{Having a name like
imx_src_resetseems 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 structreset_controller_devpointer as an argument next time:
Quoted 2 lines
static int imx_src_reset_module(struct
reset_controller_dev*rcdev,
unsigned longsw_reset_idx)
Yes, sure.
Quoted 3 lines
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 andWARN_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?
Signature · 2 lines
Matt Sealey <[email protected]> Product Development Analyst, Genesi USA, Inc.
PATCH 3/7 ARM i.MX6q: Add GPU, VPU, IPU, and OpenVG resets to System Reset Controller (SRC)
Quoted 15 lines · Matt Sealey
Quoted 3 lines
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 andWARN_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.
Quoted 2 lines
Shawn, Fabio?
Just talked to hardware people, the reset should finish in a few clock cycles, so even 1 millisecond should be enough.
Shawn
Philipp Zabel
maintainerPATCH 4/7 ARM i.MX6q: Link system reset controller (SRC) to IPU in DT
In reply to Philipp Zabel--- arch/arm/boot/dts/imx6q.dtsi | 3 +++ 1 file changed, 3 insertions(+)
arch/arm/boot/dts/imx6q.dtsi +3 -0
@@ -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>; }; }; };
Signature · 1 line
1.7.10.4
PATCH 4/7 ARM i.MX6q: Link system reset controller (SRC) to IPU in DT
Quoted 26 lines · Philipp Zabel
Signed-off-by Philipp Zabel <[email protected]>--- arch/arm/boot/dts/imx6q.dtsi | 3 +++ 1 file changed, 3 insertions(+)arch/arm/boot/dts/imx6q.dtsi +2
-0@@ -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 {
arch/arm/boot/dts/imx6q.dtsi line 1060
Quoted 4 lines
arch/arm/boot/dts/imx6q.dtsi +1
-0@@ -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
Quoted 6 lines
arch/arm/boot/dts/imx6q.dtsi +0
-0}; }; };Signature · 1 line
1.7.10.4
Philipp Zabel
maintainerPATCH 5/7 staging: drm/imx: Use SRC to reset IPU
In reply to Philipp ZabelRequest the System Reset Controller to reset the IPU if specified via device tree phandle.
--- .../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(-)
Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt +3 -0
@@ -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
drivers/staging/imx-drm/ipu-v3/ipu-common.c +9 -3
@@ -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:
Signature · 1 line
1.7.10.4
Philipp Zabel
maintainerPATCH 6/7 ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53
In reply to Philipp ZabelThe 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.
--- 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(-)
arch/arm/mach-imx/Kconfig +1 -0
@@ -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
arch/arm/mach-imx/common.h +2 -1
@@ -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 );
arch/arm/mach-imx/mach-imx6q.c +1 -1
@@ -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); }
arch/arm/mach-imx/mm-imx5.c +2 -0
@@ -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 )
arch/arm/mach-imx/src.c +13 -1
@@ -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;
Signature · 1 line
1.7.10.4
PATCH 6/7 ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53
Quoted 17 lines · 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 <[email protected]>--- 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(-)arch/arm/mach-imx/Kconfig +0
-0
arch/arm/mach-imx/Kconfig config SOC_IMX53 · line 832
Quoted 4 lines
arch/arm/mach-imx/Kconfig +1
-0@@ -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?
Quoted 22 lines
arch/arm/mach-imx/Kconfig +0
-0select PINCTRL select PINCTRL_IMX53 select SOC_IMX5arch/arm/mach-imx/common.h +2
-1@@ -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 );arch/arm/mach-imx/mach-imx6q.c +0
-0
arch/arm/mach-imx/mach-imx6q.c static const struct of_device_id imx6q_irq_match[] __initconst = { · line 232
Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +1
-1@@ -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 ...
Quoted 28 lines
arch/arm/mach-imx/mach-imx6q.c +0
-0imx_gpc_init ();of_irq_init (imx6q_irq_match); }arch/arm/mach-imx/mm-imx5.c +2
-0@@ -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 )arch/arm/mach-imx/src.c +0
-0
arch/arm/mach-imx/src.c void imx_src_prepare_restart(void) · line 132
Quoted 9 lines
arch/arm/mach-imx/src.c +5
-1@@ -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.
arch/arm/mach-imx/src.c void imx_src_prepare_restart(void) · line 134
Quoted 2 lines
arch/arm/mach-imx/src.c +2
-0+ 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
Quoted 12 lines
arch/arm/mach-imx/src.c +6
-0+ + imx_reset_controller.of_node = np; +reset_controller_register (&imx_reset_controller); +} + +void __init imx6q_src_init (void ) {struct device_node *np;u32 val;Signature · 1 line
1.7.10.4
Philipp Zabel
maintainerPATCH 6/7 ARM i.MX5: Add System Reset Controller (SRC) support for i.MX51 and i.MX53
Hi Shawn,
thank you for your comments.
Am Donnerstag, den 17.01.2013, 14:12 +0800 schrieb Shawn Guo:
Quoted 13 lines · Philipp Zabel
Quoted 12 lines
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 <[email protected]>--- 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(-)
Quoted 12 lines
arch/arm/mach-imx/Kconfig config SOC_IMX53 · line 832
Quoted 9 lines
arch/arm/mach-imx/Kconfig +1
-0@@ -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_SRCPlease 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.
Quoted 75 lines
Quoted 22 lines
arch/arm/mach-imx/Kconfig +0
-0select PINCTRL select PINCTRL_IMX53 select SOC_IMX5arch/arm/mach-imx/common.h +2
-1@@ -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 );arch/arm/mach-imx/mach-imx6q.c +0
-0arch/arm/mach-imx/mach-imx6q.c static const struct of_device_id imx6q_irq_match[] __initconst = { · line 232
Quoted 5 lines
arch/arm/mach-imx/mach-imx6q.c +1
-1@@ -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 ...
Quoted 28 lines
arch/arm/mach-imx/mach-imx6q.c +0
-0imx_gpc_init ();of_irq_init (imx6q_irq_match); }arch/arm/mach-imx/mm-imx5.c +2
-0@@ -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 )arch/arm/mach-imx/src.c +0
-0arch/arm/mach-imx/src.c void imx_src_prepare_restart(void) · line 132
Quoted 9 lines
arch/arm/mach-imx/src.c +5
-1@@ -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
Quoted 6 lines
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_initandimx6q_src_initwhich 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.
Quoted 6 lines
arch/arm/mach-imx/src.c void imx_src_prepare_restart(void) · line 134
Quoted 2 lines
arch/arm/mach-imx/src.c +2
-0+ 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
Philipp Zabel
maintainerPATCH 7/7 ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree
In reply to Philipp ZabelAlso, link SRC to IPU via phandle.
--- arch/arm/boot/dts/imx51.dtsi | 7 +++++++ arch/arm/boot/dts/imx53.dtsi | 7 +++++++ 2 files changed, 14 insertions(+)
arch/arm/boot/dts/imx51.dtsi +7 -0
@@ -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>;
arch/arm/boot/dts/imx53.dtsi +7 -0
@@ -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>;
Signature · 1 line
1.7.10.4
PATCH 7/7 ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree
Quoted 1 line · Philipp Zabel
Also, link SRC to IPU via phandle.
Aside from the comments I already made, the series,
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
Quoted 10 lines · Stephen Warren
Quoted 1 line · Philipp Zabel
Also, link SRC to IPU via phandle.
Aside from the comments I already made, the series,
Reviewed-by Stephen Warren <[email protected]>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
Philipp Zabel
maintainerPATCH 7/7 ARM i.MX5: Add system reset controller (SRC) to i.MX51 and i.MX53 device tree
In reply to Stephen WarrenAm Mittwoch, den 16.01.2013, 15:19 -0700 schrieb Stephen Warren:
Quoted 5 lines · Philipp Zabel
Quoted 1 line
Also, link SRC to IPU via phandle.
Aside from the comments I already made, the series,
Reviewed-by Stephen Warren <[email protected]>
Thank you.
Quoted 2 lines
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
cover Reset controller API to reset IP modules on i.MX5 and i.MX6
In reply to Philipp ZabelDear Philipp Zabel,
Quoted 11 lines
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.
Quoted 6 lines
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