All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] imx: don't clobber reset cause
@ 2015-02-04 21:51 Eric Nelson
  2015-02-05 16:17 ` Stefano Babic
  2015-02-05 16:28 ` Bill Pringlemeir
  0 siblings, 2 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-04 21:51 UTC (permalink / raw)
  To: u-boot

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:
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-04 21:51 [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
@ 2015-02-05 16:17 ` Stefano Babic
  2015-02-05 16:28 ` Bill Pringlemeir
  1 sibling, 0 replies; 28+ messages in thread
From: Stefano Babic @ 2015-02-05 16:17 UTC (permalink / raw)
  To: u-boot

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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-04 21:51 [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
  2015-02-05 16:17 ` Stefano Babic
@ 2015-02-05 16:28 ` Bill Pringlemeir
  2015-02-05 17:16   ` Eric Nelson
  2015-02-05 17:19   ` Stefano Babic
  1 sibling, 2 replies; 28+ messages in thread
From: Bill Pringlemeir @ 2015-02-05 16:28 UTC (permalink / raw)
  To: u-boot

On  4 Feb 2015, eric.nelson at 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 16:28 ` Bill Pringlemeir
@ 2015-02-05 17:16   ` Eric Nelson
  2015-02-05 17:19   ` Stefano Babic
  1 sibling, 0 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 17:16 UTC (permalink / raw)
  To: u-boot

Hi Bill,

On 02/05/2015 09:28 AM, Bill Pringlemeir wrote:
> On  4 Feb 2015, eric.nelson at 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 16:28 ` Bill Pringlemeir
  2015-02-05 17:16   ` Eric Nelson
@ 2015-02-05 17:19   ` Stefano Babic
  2015-02-05 17:22     ` Eric Nelson
  1 sibling, 1 reply; 28+ messages in thread
From: Stefano Babic @ 2015-02-05 17:19 UTC (permalink / raw)
  To: u-boot

Hi Bill,

On 05/02/2015 17:28, Bill Pringlemeir wrote:
> On  4 Feb 2015, eric.nelson at 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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 17:19   ` Stefano Babic
@ 2015-02-05 17:22     ` Eric Nelson
  2015-02-05 17:52       ` Stefano Babic
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 17:22 UTC (permalink / raw)
  To: u-boot

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 at 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 17:22     ` Eric Nelson
@ 2015-02-05 17:52       ` Stefano Babic
  2015-02-05 18:22         ` Eric Nelson
  0 siblings, 1 reply; 28+ messages in thread
From: Stefano Babic @ 2015-02-05 17:52 UTC (permalink / raw)
  To: u-boot

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


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 17:52       ` Stefano Babic
@ 2015-02-05 18:22         ` Eric Nelson
  2015-02-05 18:49           ` Stefano Babic
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 18:22 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 18:22         ` Eric Nelson
@ 2015-02-05 18:49           ` Stefano Babic
  2015-02-05 22:57             ` [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable Eric Nelson
                               ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Stefano Babic @ 2015-02-05 18:49 UTC (permalink / raw)
  To: u-boot

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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 18:49           ` Stefano Babic
@ 2015-02-05 22:57             ` Eric Nelson
  2015-02-06 20:36               ` Eric Nelson
  2015-02-05 22:58             ` Eric Nelson
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 22:57 UTC (permalink / raw)
  To: u-boot

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:
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 18:49           ` Stefano Babic
  2015-02-05 22:57             ` [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable Eric Nelson
@ 2015-02-05 22:58             ` Eric Nelson
  2015-02-05 23:06               ` Troy Kisky
  2015-02-06 20:40               ` Eric Nelson
  2015-02-05 23:01             ` [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
  2015-02-10 17:19             ` Eric Nelson
  3 siblings, 2 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 22:58 UTC (permalink / raw)
  To: u-boot

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)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 18:49           ` Stefano Babic
  2015-02-05 22:57             ` [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable Eric Nelson
  2015-02-05 22:58             ` Eric Nelson
@ 2015-02-05 23:01             ` Eric Nelson
  2015-02-05 23:17               ` Troy Kisky
  2015-02-10 17:19             ` Eric Nelson
  3 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 23:01 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 22:58             ` Eric Nelson
@ 2015-02-05 23:06               ` Troy Kisky
  2015-02-05 23:14                 ` Eric Nelson
  2015-02-06 20:40               ` Eric Nelson
  1 sibling, 1 reply; 28+ messages in thread
From: Troy Kisky @ 2015-02-05 23:06 UTC (permalink / raw)
  To: u-boot

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;
>  	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";

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)
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 23:06               ` Troy Kisky
@ 2015-02-05 23:14                 ` Eric Nelson
  2015-02-05 23:17                   ` Bill Pringlemeir
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-05 23:14 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 23:01             ` [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
@ 2015-02-05 23:17               ` Troy Kisky
  0 siblings, 0 replies; 28+ messages in thread
From: Troy Kisky @ 2015-02-05 23:17 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 23:14                 ` Eric Nelson
@ 2015-02-05 23:17                   ` Bill Pringlemeir
  0 siblings, 0 replies; 28+ messages in thread
From: Bill Pringlemeir @ 2015-02-05 23:17 UTC (permalink / raw)
  To: u-boot

On  5 Feb 2015, eric.nelson at 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 22:57             ` [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable Eric Nelson
@ 2015-02-06 20:36               ` Eric Nelson
  0 siblings, 0 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-06 20:36 UTC (permalink / raw)
  To: u-boot

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-05 22:58             ` Eric Nelson
  2015-02-05 23:06               ` Troy Kisky
@ 2015-02-06 20:40               ` Eric Nelson
  2015-02-06 20:56                 ` Bill Pringlemeir
  1 sibling, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-06 20:40 UTC (permalink / raw)
  To: u-boot

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=28ccd29594ed77976f45837039e40618e527a94f;hb=HEAD#l205

That won't be invoked if the O/S is started with "go" though
(often done with QNX or Windows).

Regards,


Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable
  2015-02-06 20:40               ` Eric Nelson
@ 2015-02-06 20:56                 ` Bill Pringlemeir
  0 siblings, 0 replies; 28+ messages in thread
From: Bill Pringlemeir @ 2015-02-06 20:56 UTC (permalink / raw)
  To: u-boot


> 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 at 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.

> 	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;hb=HEAD#l205

> 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-05 18:49           ` Stefano Babic
                               ` (2 preceding siblings ...)
  2015-02-05 23:01             ` [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
@ 2015-02-10 17:19             ` Eric Nelson
  2015-02-10 18:08               ` Bill Pringlemeir
  2015-02-11  9:07               ` Stefano Babic
  3 siblings, 2 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-10 17:19 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-10 17:19             ` Eric Nelson
@ 2015-02-10 18:08               ` Bill Pringlemeir
  2015-02-11  9:07               ` Stefano Babic
  1 sibling, 0 replies; 28+ messages in thread
From: Bill Pringlemeir @ 2015-02-10 18:08 UTC (permalink / raw)
  To: u-boot


On 10 Feb 2015, eric.nelson at 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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-10 17:19             ` Eric Nelson
  2015-02-10 18:08               ` Bill Pringlemeir
@ 2015-02-11  9:07               ` Stefano Babic
  2015-02-11 14:45                 ` Eric Nelson
  1 sibling, 1 reply; 28+ messages in thread
From: Stefano Babic @ 2015-02-11  9:07 UTC (permalink / raw)
  To: u-boot

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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH] imx: don't clobber reset cause
  2015-02-11  9:07               ` Stefano Babic
@ 2015-02-11 14:45                 ` Eric Nelson
  2015-02-15 21:37                   ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Eric Nelson
  0 siblings, 1 reply; 28+ messages in thread
From: Eric Nelson @ 2015-02-11 14:45 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause()
  2015-02-11 14:45                 ` Eric Nelson
@ 2015-02-15 21:37                   ` Eric Nelson
  2015-02-15 21:37                     ` [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause Eric Nelson
  2015-02-17  9:54                     ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Stefano Babic
  0 siblings, 2 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-15 21:37 UTC (permalink / raw)
  To: u-boot

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);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause
  2015-02-15 21:37                   ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Eric Nelson
@ 2015-02-15 21:37                     ` Eric Nelson
  2015-02-17  9:34                       ` Stefano Babic
  2015-02-17  9:54                       ` Stefano Babic
  2015-02-17  9:54                     ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Stefano Babic
  1 sibling, 2 replies; 28+ messages in thread
From: Eric Nelson @ 2015-02-15 21:37 UTC (permalink / raw)
  To: u-boot

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;
 }
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause
  2015-02-15 21:37                     ` [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause Eric Nelson
@ 2015-02-17  9:34                       ` Stefano Babic
  2015-02-17  9:54                       ` Stefano Babic
  1 sibling, 0 replies; 28+ messages in thread
From: Stefano Babic @ 2015-02-17  9:34 UTC (permalink / raw)
  To: u-boot

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

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause()
  2015-02-15 21:37                   ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Eric Nelson
  2015-02-15 21:37                     ` [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause Eric Nelson
@ 2015-02-17  9:54                     ` Stefano Babic
  1 sibling, 0 replies; 28+ messages in thread
From: Stefano Babic @ 2015-02-17  9:54 UTC (permalink / raw)
  To: u-boot

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


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause
  2015-02-15 21:37                     ` [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause Eric Nelson
  2015-02-17  9:34                       ` Stefano Babic
@ 2015-02-17  9:54                       ` Stefano Babic
  1 sibling, 0 replies; 28+ messages in thread
From: Stefano Babic @ 2015-02-17  9:54 UTC (permalink / raw)
  To: u-boot

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



-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2015-02-17  9:54 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-04 21:51 [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
2015-02-05 16:17 ` Stefano Babic
2015-02-05 16:28 ` Bill Pringlemeir
2015-02-05 17:16   ` Eric Nelson
2015-02-05 17:19   ` Stefano Babic
2015-02-05 17:22     ` Eric Nelson
2015-02-05 17:52       ` Stefano Babic
2015-02-05 18:22         ` Eric Nelson
2015-02-05 18:49           ` Stefano Babic
2015-02-05 22:57             ` [U-Boot] [PATCH] imx: save reset cause in 'reset_cause' environment variable Eric Nelson
2015-02-06 20:36               ` Eric Nelson
2015-02-05 22:58             ` Eric Nelson
2015-02-05 23:06               ` Troy Kisky
2015-02-05 23:14                 ` Eric Nelson
2015-02-05 23:17                   ` Bill Pringlemeir
2015-02-06 20:40               ` Eric Nelson
2015-02-06 20:56                 ` Bill Pringlemeir
2015-02-05 23:01             ` [U-Boot] [PATCH] imx: don't clobber reset cause Eric Nelson
2015-02-05 23:17               ` Troy Kisky
2015-02-10 17:19             ` Eric Nelson
2015-02-10 18:08               ` Bill Pringlemeir
2015-02-11  9:07               ` Stefano Babic
2015-02-11 14:45                 ` Eric Nelson
2015-02-15 21:37                   ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Eric Nelson
2015-02-15 21:37                     ` [U-Boot] [PATCH 2/2] nitrogen6x: set environment variable reset_cause Eric Nelson
2015-02-17  9:34                       ` Stefano Babic
2015-02-17  9:54                       ` Stefano Babic
2015-02-17  9:54                     ` [U-Boot] [PATCH 1/2] ARM: i.MX: provide access to reset cause through get_imx_reset_cause() Stefano Babic

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.