diff mbox

[U-Boot] imx: don't clobber reset cause

Message ID 1423086661-5860-1-git-send-email-eric.nelson@boundarydevices.com
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Eric Nelson Feb. 4, 2015, 9:51 p.m. UTC
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(-)

Comments

Stefano Babic Feb. 5, 2015, 4:17 p.m. UTC | #1
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
Bill Pringlemeir Feb. 5, 2015, 4:28 p.m. UTC | #2
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.
Eric Nelson Feb. 5, 2015, 5:16 p.m. UTC | #3
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
Stefano Babic Feb. 5, 2015, 5:19 p.m. UTC | #4
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
Eric Nelson Feb. 5, 2015, 5:22 p.m. UTC | #5
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
Stefano Babic Feb. 5, 2015, 5:52 p.m. UTC | #6
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
Eric Nelson Feb. 5, 2015, 6:22 p.m. UTC | #7
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
Stefano Babic Feb. 5, 2015, 6:49 p.m. UTC | #8
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
Eric Nelson Feb. 5, 2015, 11:01 p.m. UTC | #9
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
Troy Kisky Feb. 5, 2015, 11:17 p.m. UTC | #10
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
Eric Nelson Feb. 10, 2015, 5:19 p.m. UTC | #11
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
Bill Pringlemeir Feb. 10, 2015, 6:08 p.m. UTC | #12
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.
Stefano Babic Feb. 11, 2015, 9:07 a.m. UTC | #13
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
Eric Nelson Feb. 11, 2015, 2:45 p.m. UTC | #14
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
diff mbox

Patch

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: