| Message ID | 1358352787-15441-4-git-send-email-p.zabel@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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?
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
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__ */
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