[U-Boot] [PATCH] imx: don't clobber reset cause
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr); - writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
On 04/02/2015 22:51, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr);
writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
Acked-by: Stefano Babic sbabic@denx.de
Best regards, Stefano Babic
On 4 Feb 2015, eric.nelson@boundarydevices.com wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr);
writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
Fwiw, Bill Pringlemeir.
Hi Bill,
On 02/05/2015 09:28 AM, Bill Pringlemeir wrote:
On 4 Feb 2015, eric.nelson@boundarydevices.com wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr);
writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Understood.
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
This patch seems a pre-cursor to anything else, since blindly clearing the bits doesn't allow them to be used by code or script in U-Boot or an OS.
Regards,
Eric
Hi Bill,
On 05/02/2015 17:28, Bill Pringlemeir wrote:
On 4 Feb 2015, eric.nelson@boundarydevices.com wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr);
writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
I have assumed (maybe wrong ?) that the reason for the patch is to let the OS reading these bits.
Best regards, Stefano Babic
Hi Stefano,
On 02/05/2015 10:19 AM, Stefano Babic wrote:
Hi Bill,
On 05/02/2015 17:28, Bill Pringlemeir wrote:
On 4 Feb 2015, eric.nelson@boundarydevices.com wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
If a particular system wants to clear it out, this should be done later after there's an opportunity for code or boot commands to read the value.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..3e0a582 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -30,7 +30,6 @@ char *get_reset_cause(void) struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr);
writel(cause, &src_regs->srsr);
switch (cause) { case 0x00001:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
I have assumed (maybe wrong ?) that the reason for the patch is to let the OS reading these bits.
In some cases (Windows Embedded), yes.
In the Linux case, we'll likely pass the value to the kernel through the kernel command-line, so it's available to userspace.
I'm not aware of any kernel functionality for this at the moment.
Regards,
Eric
Hi Eric,
On 05/02/2015 18:22, Eric Nelson wrote:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
I have assumed (maybe wrong ?) that the reason for the patch is to let the OS reading these bits.
In some cases (Windows Embedded), yes.
In the Linux case, we'll likely pass the value to the kernel through the kernel command-line, so it's available to userspace.
I'm not aware of any kernel functionality for this at the moment.
It remains the issue raised by Bill (thanks for that). If the bits are not reset, we can determine the cause only after POR, but not anymore after a warm start. Can you maybe use the IRAM to pass the information to Windows ?
Best regards, Stefano Babic
Hi Stefano,
On 02/05/2015 10:52 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 18:22, Eric Nelson wrote:
There is very similar code in 'arch/arm/cpu/armv7/vf610/generic.c'. The write is for a hard power on case where these reason registers are full of weird bogus values (at least on Vybrid; I suspect on iMx). In the case of a non-POR, the register bits are good. However, if you don't clear the status, on the next reset it may have multiple registers bits even though you really want to know the last reason (bit).
Another option would be to clear the value and store the 'cause' somewhere for other U-Boot users. Unless you wanted to read this from an OS? I think both files should behave the same, all else equal.
I have assumed (maybe wrong ?) that the reason for the patch is to let the OS reading these bits.
In some cases (Windows Embedded), yes.
In the Linux case, we'll likely pass the value to the kernel through the kernel command-line, so it's available to userspace.
I'm not aware of any kernel functionality for this at the moment.
It remains the issue raised by Bill (thanks for that). If the bits are not reset, we can determine the cause only after POR, but not anymore after a warm start. Can you maybe use the IRAM to pass the information to Windows ?
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
Regards,
Eric
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
Regards, Stefano
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
Stash the string representation in the environment variable "reset_cause".
The value is stored in hex, and the values may vary with new silicon. As of this reading the following bitfields are common to at least i.MX5 and i.MX6 processors:
bit meaning --- ------- 0 reset pin or power-on 2 reset from csu_reset_b input 3 reset from ipp_user_reset_b 4 watchdog reset 5 JTAG High-Z requested reset 6 JTAG software reset 16 Warm boot: result of a software initiated boot
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- arch/arm/imx-common/cpu.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..31d6e61 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -32,6 +32,8 @@ char *get_reset_cause(void) cause = readl(&src_regs->srsr); writel(cause, &src_regs->srsr);
+ setenv_hex("reset_cause", cause); + switch (cause) { case 0x00001: case 0x00011:
Hi all,
On 02/05/2015 03:57 PM, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
Stash the string representation in the environment variable "reset_cause".
<snip>
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..31d6e61 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -32,6 +32,8 @@ char *get_reset_cause(void) cause = readl(&src_regs->srsr); writel(cause, &src_regs->srsr);
- setenv_hex("reset_cause", cause);
Nak.
This routine is called before the environment is initialized.
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
Stash the string representation in the environment variable "reset_cause".
Values include: "POR" - power on reset "CSU" - reset was the result of the csu_reset_b input "IPP-USER" - ipp_user_reset_b qualified reset "WDOG" - watchdog reset "JTAG-HIGH-Z" - HIGH-Z reset from JTAG "JTAG-SW" - software reset from JTAG "WARM-BOOT" - WARM boot was initiated by software
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- arch/arm/imx-common/cpu.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..4a54051 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -28,6 +28,7 @@ char *get_reset_cause(void) { u32 cause; struct src *src_regs = (struct src *)SRC_BASE_ADDR; + char *rval = "unknown";
cause = readl(&src_regs->srsr); writel(cause, &src_regs->srsr); @@ -35,22 +36,28 @@ char *get_reset_cause(void) switch (cause) { case 0x00001: case 0x00011: - return "POR"; + rval = "POR"; + break; case 0x00004: - return "CSU"; + rval = "CSU"; + break; case 0x00008: - return "IPP USER"; + rval = "IPP-USER"; + break; case 0x00010: - return "WDOG"; + rval = "WDOG"; + break; case 0x00020: - return "JTAG HIGH-Z"; + rval = "JTAG-HIGH-Z"; + break; case 0x00040: - return "JTAG SW"; + rval = "JTAG-SW"; + break; case 0x10000: - return "WARM BOOT"; - default: - return "unknown reset"; + rval = "WARM-BOOT"; } + setenv("reset_cause", rval); + return rval; }
#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
On 2/5/2015 3:58 PM, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
Stash the string representation in the environment variable "reset_cause".
Values include: "POR" - power on reset "CSU" - reset was the result of the csu_reset_b input "IPP-USER" - ipp_user_reset_b qualified reset "WDOG" - watchdog reset "JTAG-HIGH-Z" - HIGH-Z reset from JTAG "JTAG-SW" - software reset from JTAG "WARM-BOOT" - WARM boot was initiated by software
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
arch/arm/imx-common/cpu.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..4a54051 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -28,6 +28,7 @@ char *get_reset_cause(void) { u32 cause; struct src *src_regs = (struct src *)SRC_BASE_ADDR;
char *rval = "unknown";
cause = readl(&src_regs->srsr); writel(cause, &src_regs->srsr);
@@ -35,22 +36,28 @@ char *get_reset_cause(void) switch (cause) { case 0x00001: case 0x00011:
return "POR";
rval = "POR";break;
return "CSU";
rval = "CSU";break;
return "IPP USER";
rval = "IPP-USER";break;
return "WDOG";
rval = "WDOG";break;
return "JTAG HIGH-Z";
rval = "JTAG-HIGH-Z";break;
return "JTAG SW";
rval = "JTAG-SW";break;
return "WARM BOOT";- default:
return "unknown reset";
rval = "WARM-BOOT";
Instead of removing default, can we have a hex value, something like sprintf(buf, "unknown(0x%x)", cause);
}
- setenv("reset_cause", rval);
- return rval;
}
#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
Hi Troy,
On 02/05/2015 04:06 PM, Troy Kisky wrote:
On 2/5/2015 3:58 PM, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
Stash the string representation in the environment variable "reset_cause".
Values include: "POR" - power on reset "CSU" - reset was the result of the csu_reset_b input "IPP-USER" - ipp_user_reset_b qualified reset "WDOG" - watchdog reset "JTAG-HIGH-Z" - HIGH-Z reset from JTAG "JTAG-SW" - software reset from JTAG "WARM-BOOT" - WARM boot was initiated by software
<snip>
switch (cause) { case 0x00001: case 0x00011:
return "POR";
rval = "POR";break;<snip>
- default:
return "unknown reset";
rval = "WARM-BOOT";Instead of removing default, can we have a hex value, something like sprintf(buf, "unknown(0x%x)", cause);
Of course (if we go that route).
I thought of this when typing up the list of values for the commit comment.
The human readable form is harder to handle on the receiving side though, which is why I favor hex.
Regards,
Eric
On 5 Feb 2015, eric.nelson@boundarydevices.com wrote:
[snip]
Of course (if we go that route).
I thought of this when typing up the list of values for the commit comment.
The human readable form is harder to handle on the receiving side though, which is why I favor hex.
I also prefer the hex. I have had very weird values in this register. For a normal user, we can guess and display a string. Sometimes in the case of double/triple resets, there can be very strange values. You can get these when DMA goes crazy (because of bad code or otherwise).
Fwiw, Bill Pringlemeir.
Hi all,
On 02/05/2015 03:58 PM, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
<snip>
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..4a54051 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -28,6 +28,7 @@ char *get_reset_cause(void) { u32 cause; struct src *src_regs = (struct src *)SRC_BASE_ADDR;
char *rval = "unknown";
cause = readl(&src_regs->srsr);
<snip>
}
- setenv("reset_cause", rval);
- return rval;
Nak.
This routine is called before the environment is initialized.
There's no way to set the environment here, which I think means that this patch is a pre-cursor to anything else.
http://patchwork.ozlabs.org/patch/436492/ If we feel the need to reset it before an O/S boots, there is a common spot here: http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/imx-common/cpu.c;h=28ccd2...
That won't be invoked if the O/S is started with "go" though (often done with QNX or Windows).
Regards,
Eric
On 02/05/2015 03:58 PM, Eric Nelson wrote:
The cause of a reset is generally useful, and shouldn't be blindly cleared in the process of displaying it as a part of the boot announcement.
<snip>
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..4a54051 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -28,6 +28,7 @@ char *get_reset_cause(void) { u32 cause; struct src *src_regs = (struct src *)SRC_BASE_ADDR;
char *rval = "unknown";
cause = readl(&src_regs->srsr);
<snip>
}
- setenv("reset_cause", rval);
- return rval;
On 6 Feb 2015, eric.nelson@boundarydevices.com wrote:
Nak.
This routine is called before the environment is initialized.
Okay, that makes sense we can't do this.
There's no way to set the environment here, which I think means that this patch is a pre-cursor to anything else.
If we feel the need to reset it before an O/S boots, there is a common spot here: http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/imx-common/cpu.c;hb=HEAD#...
That won't be invoked if the O/S is started with "go" though (often done with QNX or Windows).
The 'write' is to prevent against multiple bits set. Say for instance you remove this check. Someone has U-boot siting at a command line for a long time before they boot an OS. Occasionally, some memory/hardware error happens and the device resets. In this case, the reason will be from multiple resets.
I think it is good to write/clear (and record) the reason early. So, I think just having 'cause' as a global or accessible when the environment is ready would solve your problem.
Writing does two things,
1. clears the 'reset cause' to prevent someone else to look at. 2. primes the 'cause' for the next reset; the bits are sticky. w1c
I think we miss the last point with this patch. The 'cause' just needs to be recorded and kept around until the environment is up (or however we want to pass it around).
Recording it early and clearing is best as you can know what the 'last cause' was. Otherwise, you might get multiple causes in the register.
Err, maybe I mis-understand something? If you do this patch and have multiple soft resets (watchdog, CSU, JTAG, ipp, warm, etc) are the bits accumulated?
Fwiw, Bill.
Hi Stefano,
On 02/05/2015 11:49 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
Okay. Here are two options:
The first one stores the value in 'reset_cause' as a hex value, and is generally more extensible: http://patchwork.ozlabs.org/patch/436972/
The second stores it as a human-readable string using roughly the same names as were previously printed. I changed the names slightly to avoid embedded whitespace so the values can be appended to bootargs without escapes: http://patchwork.ozlabs.org/patch/436974/
I prefer the first, but don't have a strong opinion one way or the other.
Regards,
Eric
On 2/5/2015 4:01 PM, Eric Nelson wrote:
Hi Stefano,
On 02/05/2015 11:49 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
Okay. Here are two options:
The first one stores the value in 'reset_cause' as a hex value, and is generally more extensible: http://patchwork.ozlabs.org/patch/436972/
The second stores it as a human-readable string using roughly the same names as were previously printed. I changed the names slightly to avoid embedded whitespace so the values can be appended to bootargs without escapes: http://patchwork.ozlabs.org/patch/436974/
I prefer the first, but don't have a strong opinion one way or the other.
Regards,
My feelings are the same.
Troy
Hi Stefano,
On 02/05/2015 11:49 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
I posted a couple of additional options and received no comment from you.
Neither of them works as-is because of the ordering of events (print_cpuinfo() is called before restoring the environment), so your suggestion would require an additional call at startup which currently doesn't exist across i.MX boards.
The primary argument against the original patch was that bits **could** accumulate. In practice, I believe this to be a bit of a red herring, since two bits cover essentially all of the normal conditions:
bit 0 - power on bit 4 - watchdog
The watchdog flag is set with reboot under Linux and reset in U-Boot, so we could re-work the switch statement to do the right thing. In fact, it appears broken now because it has 0x11 displaying "POR", when I believe that should be "WDOG".
Other bits could conceivably accumulate, but I don't see the value of worrying about cases like "JTAG_RESET".
The reason we're pursuing this at all is because we'd like to know the difference between a restart caused by power interruption and a system lockup, and we'd like to do this under Linux or Windows Embedded.
Without a patch, things are pretty much broken unless we're screen-scraping.
Please advise,
Eric
On 10 Feb 2015, eric.nelson@boundarydevices.com wrote:
I posted a couple of additional options and received no comment from you.
Neither of them works as-is because of the ordering of events (print_cpuinfo() is called before restoring the environment), so your suggestion would require an additional call at startup which currently doesn't exist across i.MX boards.
The primary argument against the original patch was that bits **could** accumulate. In practice, I believe this to be a bit of a red herring, since two bits cover essentially all of the normal conditions:
bit 0 - power on bit 4 - watchdog
Yes, the normal case is easy. Odd things can happen during software development. We both agree you could miss something; maybe only whether that is important is not clear. People using the CSU to protect errant memory writes to disabled peripherals might be useful. From the imx6 RM,
Software has to take care to clear this register by writing one after every reset that occurs so that the register will contain the information of recently occurred reset.
The watchdog flag is set with reboot under Linux and reset in U-Boot, so we could re-work the switch statement to do the right thing. In fact, it appears broken now because it has 0x11 displaying "POR", when I believe that should be "WDOG".
I am pretty sure that the register is full of garbage on a 'POR', so the 'POR' bit overrides everything; at least on the Vybrid. This is why it is important to clear the bits. The imx6DL-RM seems to say the same,
When the system comes out of reset, this register will have bits set corresponding to all the reset sources that occurred during system reset.
Other bits could conceivably accumulate, but I don't see the value of worrying about cases like "JTAG_RESET".
The reason we're pursuing this at all is because we'd like to know the difference between a restart caused by power interruption and a system lockup, and we'd like to do this under Linux or Windows Embedded.
Please don't mis-understand. I see value in what you are trying to accomplish. I just want to make sure the information that gets reported is robust and correct. It would probably be nice if the Vybrid followed the same pattern; but maybe they are different? From reading the RMs they seem the same.
Regards, Bill Pringlemeir.
Hi Eric,
On 10/02/2015 18:19, Eric Nelson wrote:
Hi Stefano,
On 02/05/2015 11:49 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
I posted a couple of additional options and received no comment from you.
Sorry for that.
Neither of them works as-is because of the ordering of events (print_cpuinfo() is called before restoring the environment), so your suggestion would require an additional call at startup which currently doesn't exist across i.MX boards.
Right, I know. For that reason I suggested to you to save the result somewhere but not into a variable, at least not at the time print_cpuinfo() is called.
I think we can split the issue into two parts:
- detecting the reset reason. It makes absolutely sense to check the reason as soon as possible to avoid to miss an event, and resetting the cause is also a must. This is what current code does, and it is correct that the reason is read by the bootloader and not by the OS. In this way, we do not miss events and we know the last reason.
- we need to propagate this information to the OS in a way the OS can understand. Anyway, this does not mean that the OS must reread from the srsr register.
I admit, I do not know the interface with WinCE - I cannot help a lot for that. But assume we can have something similar we have with Linux. If we want to go on with a U-Boot variable, it is not required that the variable is assigned at the moment the reason is read. I think there are some other example in U-Boot (maybe "dieid#" for TI soc ?), where the variable is assigned later.
I do not think that resetting the flags in arch_preboot_os() is a good idea. This is a hack, and passing parameters has nothing to do with turning off peripherals.
The primary argument against the original patch was that bits **could** accumulate. In practice, I believe this to be a bit of a red herring, since two bits cover essentially all of the normal conditions:
bit 0 - power on bit 4 - watchdog
Let's say that my board has an issue (maybe hardware, maybe not..) and after a first reset, the board resets twice. It can be a problem with power supply, can be something different. First time bits are on, and if I do not clear the bits, I cannot know the reson when it happens again.
The watchdog flag is set with reboot under Linux and reset in U-Boot, so we could re-work the switch statement to do the right thing.
I slightly disagree. You are perfectly right when everything works as it should work.
In fact, it appears broken now because it has 0x11 displaying "POR", when I believe that should be "WDOG".
I do not know now, but of course reset reason have priorities. If "POR" is set, it has advantage on "WDOG". But if after a WDOG the POR bit is set, it is another issue, not related to this one.
Other bits could conceivably accumulate, but I don't see the value of worrying about cases like "JTAG_RESET".
The reason we're pursuing this at all is because we'd like to know the difference between a restart caused by power interruption and a system lockup, and we'd like to do this under Linux or Windows Embedded.
Agree, but I do not think we reach the goal simply clearing the bits and hoping to have always the good case. Multiple reset in U-Boot (and I see a lot of them...) are then hidden (not the reset, but the cause, of course !).
I understand the goal, we have to find a suitable ways to pass the information to the OS, that is.
Best regards, Stefano
Hi Stefano,
On 02/11/2015 02:07 AM, Stefano Babic wrote:
Hi Eric,
On 10/02/2015 18:19, Eric Nelson wrote:
Hi Stefano,
On 02/05/2015 11:49 AM, Stefano Babic wrote:
Hi Eric,
On 05/02/2015 19:22, Eric Nelson wrote:
Certainly, but it seems wrong to make a decision about where and how this might get passed to an O/S in code.
If we want to generalize it, I'd be inclined to add commands to query (into a variable) and clear the reset cause.
That would still require this patch though.
I do not think there should be a command. The cause must be directly associated to the variable, and the reset cause cleared.
I posted a couple of additional options and received no comment from you.
Sorry for that.
No worries, and thanks for the feedback.
Neither of them works as-is because of the ordering of events (print_cpuinfo() is called before restoring the environment), so your suggestion would require an additional call at startup which currently doesn't exist across i.MX boards.
Right, I know. For that reason I suggested to you to save the result somewhere but not into a variable, at least not at the time print_cpuinfo() is called.
I think we can split the issue into two parts:
- detecting the reset reason. It makes absolutely sense to check the
reason as soon as possible to avoid to miss an event, and resetting the cause is also a must. This is what current code does, and it is correct that the reason is read by the bootloader and not by the OS. In this way, we do not miss events and we know the last reason.
Saving the value is the easy part.
- we need to propagate this information to the OS in a way the OS can
understand. Anyway, this does not mean that the OS must reread from the srsr register.
I admit, I do not know the interface with WinCE - I cannot help a lot for that. But assume we can have something similar we have with Linux. If we want to go on with a U-Boot variable, it is not required that the variable is assigned at the moment the reason is read. I think there are some other example in U-Boot (maybe "dieid#" for TI soc ?), where the variable is assigned later.
Thanks for the pointer. The use of the dieid# variable shows what I'd like to avoid though. There are 17 different calls to read this value through the dieid_num_r() routine in various board files.
I'll look again to see if there's some other common spot that can be used to read a saved value into an environment variable for i.MX5x and i.MX6 CPUs.
I do not think that resetting the flags in arch_preboot_os() is a good idea. This is a hack, and passing parameters has nothing to do with turning off peripherals.
Agreed.
The primary argument against the original patch was that bits **could** accumulate. In practice, I believe this to be a bit of a red herring, since two bits cover essentially all of the normal conditions:
bit 0 - power on bit 4 - watchdog
Let's say that my board has an issue (maybe hardware, maybe not..) and after a first reset, the board resets twice. It can be a problem with power supply, can be something different. First time bits are on, and if I do not clear the bits, I cannot know the reson when it happens again.
The bits are cleared now, so you won't know unless you're watching the console.
There's also data loss when converting from the register value to string (priority discussed below).
The watchdog flag is set with reboot under Linux and reset in U-Boot, so we could re-work the switch statement to do the right thing.
I slightly disagree. You are perfectly right when everything works as it should work.
In fact, it appears broken now because it has 0x11 displaying "POR", when I believe that should be "WDOG".
I do not know now, but of course reset reason have priorities. If "POR" is set, it has advantage on "WDOG". But if after a WDOG the POR bit is set, it is another issue, not related to this one.
I think there's an errata for i.MX6 when booting to SD or eMMC which may cause the ROM boot loader to reset via watchdog.
Other bits could conceivably accumulate, but I don't see the value of worrying about cases like "JTAG_RESET".
The reason we're pursuing this at all is because we'd like to know the difference between a restart caused by power interruption and a system lockup, and we'd like to do this under Linux or Windows Embedded.
Agree, but I do not think we reach the goal simply clearing the bits and hoping to have always the good case. Multiple reset in U-Boot (and I see a lot of them...) are then hidden (not the reset, but the cause, of course !).
I understand the goal, we have to find a suitable ways to pass the information to the OS, that is.
Again, thanks for the feedback.
Regards,
Eric
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- This patch set replaces http://patchwork.ozlabs.org/patch/436492/.
arch/arm/imx-common/cpu.c | 10 +++++++++- arch/arm/include/asm/arch-imx/cpu.h | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index 28ccd29..067d08f 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -24,13 +24,16 @@ #include <fsl_esdhc.h> #endif
-char *get_reset_cause(void) +static u32 reset_cause = -1; + +static char *get_reset_cause(void) { u32 cause; struct src *src_regs = (struct src *)SRC_BASE_ADDR;
cause = readl(&src_regs->srsr); writel(cause, &src_regs->srsr); + reset_cause = cause;
switch (cause) { case 0x00001: @@ -53,6 +56,11 @@ char *get_reset_cause(void) } }
+u32 get_imx_reset_cause(void) +{ + return reset_cause; +} + #if defined(CONFIG_MX53) || defined(CONFIG_MX6) #if defined(CONFIG_MX53) #define MEMCTL_BASE ESDCTL_BASE_ADDR diff --git a/arch/arm/include/asm/arch-imx/cpu.h b/arch/arm/include/asm/arch-imx/cpu.h index 254136e..4715f4e 100644 --- a/arch/arm/include/asm/arch-imx/cpu.h +++ b/arch/arm/include/asm/arch-imx/cpu.h @@ -17,3 +17,5 @@ #define CS0_64M_CS1_64M 1 #define CS0_64M_CS1_32M_CS2_32M 2 #define CS0_32M_CS1_32M_CS2_32M_CS3_32M 3 + +u32 get_imx_reset_cause(void);
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- This patch replaces the following two patches, but only applies to Nitrogen6x and SABRE Lite boards: http://patchwork.ozlabs.org/patch/436972/ http://patchwork.ozlabs.org/patch/436974/ board/boundary/nitrogen6x/nitrogen6x.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c index e8ea256..d46b8db 100644 --- a/board/boundary/nitrogen6x/nitrogen6x.c +++ b/board/boundary/nitrogen6x/nitrogen6x.c @@ -1018,5 +1018,6 @@ int misc_init_r(void) #ifdef CONFIG_CMD_BMODE add_board_boot_modes(board_boot_modes); #endif + setenv_hex("reset_cause", get_imx_reset_cause()); return 0; }
Hi Eric,
On 15/02/2015 22:37, Eric Nelson wrote:
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
This patch replaces the following two patches, but only applies to Nitrogen6x and SABRE Lite boards: http://patchwork.ozlabs.org/patch/436972/ http://patchwork.ozlabs.org/patch/436974/ board/boundary/nitrogen6x/nitrogen6x.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c index e8ea256..d46b8db 100644 --- a/board/boundary/nitrogen6x/nitrogen6x.c +++ b/board/boundary/nitrogen6x/nitrogen6x.c @@ -1018,5 +1018,6 @@ int misc_init_r(void) #ifdef CONFIG_CMD_BMODE add_board_boot_modes(board_boot_modes); #endif
- setenv_hex("reset_cause", get_imx_reset_cause()); return 0;
}
After our previous discussion:
Acked-by: Stefano Babic sbabic@denx.de
Best regards, Stefano Babic
On 15/02/2015 22:37, Eric Nelson wrote:
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
Applied to u-boot-imx, thanks !
Best regards, Stefano Babic
On 15/02/2015 22:37, Eric Nelson wrote:
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com
Applied to u-boot-imx, thanks !
Best regards, Stefano Babic
participants (4)
-
Bill Pringlemeir -
Eric Nelson -
Stefano Babic -
Troy Kisky