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

login
register
mail settings
Submitter Philipp Zabel
Date Jan. 16, 2013, 4:13 p.m.
Message ID <1358352787-15441-4-git-send-email-p.zabel@pengutronix.de>
Download mbox | patch
Permalink /patch/212795/
State New
Headers show

Comments

Philipp Zabel - Jan. 16, 2013, 4:13 p.m.
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
Shawn Guo - Jan. 17, 2013, 5:31 a.m.
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
Matt Sealey - Jan. 18, 2013, 7:57 p.m.
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.
Philipp Zabel - Jan. 21, 2013, 9:52 a.m.
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
Matt Sealey - Jan. 21, 2013, 5:47 p.m.
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?
Shawn Guo - Jan. 22, 2013, 7:50 a.m.
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

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@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@02400000 {
+                resets = <&src 2>;
+        };
+        ipu2: ipu@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__ */