[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Fabio Estevam]

MX31 Reference Manual states the following possible values for the silicon revision:

.srev = 0x00,
.srev = 0x10,
.srev = 0x11,
.srev = 0x12,
.srev = 0x13,
.srev = 0x14,
.srev = 0x15,
.srev = 0x28,
.srev = 0x29,

However it is possible to find some pre-production silicon on some old hardware, such as MX31ADS
that shows srev = 0x20.

The following message is the currently displayed on such MX31ADS board:

CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG

With this patch we see a better message like:

CPU:   Freescale i.MX31 rev 0.0 (unknown revision) at 531 MHz.Reset cause: WDOG

Reported-by: Felix Radensky <felix@embedded-sol.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/cpu/arm1136/mx31/generic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/arm1136/mx31/generic.c b/arch/arm/cpu/arm1136/mx31/generic.c
index 4ebf38d..770d359 100644
--- a/arch/arm/cpu/arm1136/mx31/generic.c
+++ b/arch/arm/cpu/arm1136/mx31/generic.c
@@ -130,7 +130,7 @@ u32 get_cpu_rev(void)
 		if (srev == mx31_cpu_type[i].srev)
 			return mx31_cpu_type[i].v;
 
-	return srev | 0x8000;
+	return 0x8000;
 }
 
 static char *get_reset_cause(void)
@@ -162,7 +162,7 @@ int print_cpuinfo (void)
 
 	printf("CPU:   Freescale i.MX31 rev %d.%d%s at %d MHz.",
 			(srev & 0xF0) >> 4, (srev & 0x0F),
-			((srev & 0x8000) ? " unknown" : ""),
+			((srev & 0x8000) ? " (unknown revision)" : ""),
 			mx31_get_mcu_main_clk() / 1000000);
 	printf("Reset cause: %s\n", get_reset_cause());
 	return 0;
-- 
1.6.0.4

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Graeme Russ]

On Wed, Jun 15, 2011 at 2:31 AM, Fabio Estevam
<fabio.estevam@freescale.com> wrote:
> MX31 Reference Manual states the following possible values for the silicon revision:
>
> .srev = 0x00,
> .srev = 0x10,
> .srev = 0x11,
> .srev = 0x12,
> .srev = 0x13,
> .srev = 0x14,
> .srev = 0x15,
> .srev = 0x28,
> .srev = 0x29,
>
> However it is possible to find some pre-production silicon on some old hardware, such as MX31ADS
> that shows srev = 0x20.
>
> The following message is the currently displayed on such MX31ADS board:
>
> CPU: ? Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
>
> With this patch we see a better message like:
>
> CPU: ? Freescale i.MX31 rev 0.0 (unknown revision) at 531 MHz.Reset cause: WDOG
>
> Reported-by: Felix Radensky <felix@embedded-sol.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> ?arch/arm/cpu/arm1136/mx31/generic.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/cpu/arm1136/mx31/generic.c b/arch/arm/cpu/arm1136/mx31/generic.c
> index 4ebf38d..770d359 100644
> --- a/arch/arm/cpu/arm1136/mx31/generic.c
> +++ b/arch/arm/cpu/arm1136/mx31/generic.c
> @@ -130,7 +130,7 @@ u32 get_cpu_rev(void)
> ? ? ? ? ? ? ? ?if (srev == mx31_cpu_type[i].srev)
> ? ? ? ? ? ? ? ? ? ? ? ?return mx31_cpu_type[i].v;
>
> - ? ? ? return srev | 0x8000;
> + ? ? ? return 0x8000;
> ?}

Why clobber possibly useful information?

>
> ?static char *get_reset_cause(void)
> @@ -162,7 +162,7 @@ int print_cpuinfo (void)
>
> ? ? ? ?printf("CPU: ? Freescale i.MX31 rev %d.%d%s at %d MHz.",
> ? ? ? ? ? ? ? ? ? ? ? ?(srev & 0xF0) >> 4, (srev & 0x0F),
> - ? ? ? ? ? ? ? ? ? ? ? ((srev & 0x8000) ? " unknown" : ""),
> + ? ? ? ? ? ? ? ? ? ? ? ((srev & 0x8000) ? " (unknown revision)" : ""),

I would just leave the patch at this

> ? ? ? ? ? ? ? ? ? ? ? ?mx31_get_mcu_main_clk() / 1000000);
> ? ? ? ?printf("Reset cause: %s\n", get_reset_cause());
> ? ? ? ?return 0;

Regards,

Graeme

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Wolfgang Denk]

Dear Fabio Estevam,

In message <1308069108-5438-1-git-send-email-fabio.estevam@freescale.com> you wrote:
...
> However it is possible to find some pre-production silicon on some old hardware, such as MX31ADS
> that shows srev = 0x20.
> 
> The following message is the currently displayed on such MX31ADS board:
> 
> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
> 
> With this patch we see a better message like:
> 
> CPU:   Freescale i.MX31 rev 0.0 (unknown revision) at 531 MHz.Reset cause: WDOG

The old information is way more useful, and shorter, than the new one.
And the code is smaller and without any magic, too.


I agree with Graeme: we should rather leave the code as is.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Unix is simple, but it takes a genius to understand the simplicity."
					             - Dennis Ritchie

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Stefano Babic]

On 06/14/2011 06:31 PM, Fabio Estevam wrote:
> MX31 Reference Manual states the following possible values for the silicon revision:
> 

Hi Fabio,

> However it is possible to find some pre-production silicon on some old hardware, such as MX31ADS
> that shows srev = 0x20.
> 
> The following message is the currently displayed on such MX31ADS board:
> 
> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
> 
> With this patch we see a better message like:
> 
> CPU:   Freescale i.MX31 rev 0.0 (unknown revision) at 531 MHz.Reset cause: WDOG

Why is the new output better as we have now ? You drop the output of
the srev register, and then we cannot get which strange silicon version
is running without patching the code.

>  		if (srev == mx31_cpu_type[i].srev)
>  			return mx31_cpu_type[i].v;
>  
> -	return srev | 0x8000;
> +	return 0x8000;

IMHO in the case the revision is not recognized, it is better to print
the value of the srev register, as it is done now. This can be a useful
information and printing a fixed "0.0" version does not say anything
about the processor.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Fabio Estevam]

Hi Stefano,

On 6/15/2011 2:29 AM, Stefano Babic wrote:
...
> 
> Why is the new output better as we have now ? You drop the output of
> the srev register, and then we cannot get which strange silicon version
> is running without patching the code.

Let me try to explain the problem I see with the current silicon detection mechanism:

On my board (srev=0x28), which is a TO2.0 silicon I get:

CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG

On Felix?s MX31ADS board (srev=0x20) (unknown chip version) he gets:

CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG

Reading rev 2.0 on Felix?s case is misleading IMHO as we tend to think that we have a TO2.0 silicon on his board even though we get a "unknown" string.

The reason of this is that we currently return the .v struct when the silicon version is detected and the srev version when it is not recognized.


>>  		if (srev == mx31_cpu_type[i].srev)
>>  			return mx31_cpu_type[i].v;
>>  
>> -	return srev | 0x8000;
>> +	return 0x8000;
> 
> IMHO in the case the revision is not recognized, it is better to print
> the value of the srev register, as it is done now. 

Yes, agreed, but we should print srev as an hex number instead of a string as done today

If you agree I can implement the following logic:

When the chip version is detected let?s just leave as it is today:

CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG

When the chip version is not valid, then we print:

CPU:   Freescale i.MX31 (unknown rev, srev=0x20) at 531 MHz.Reset cause: WDOG

Please let me know what you think about this proposal.

Thanks,

Fabio Estevam

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Graeme Russ]

Hi Fabio,

On 15/06/11 21:50, Fabio Estevam wrote:
> Hi Stefano,
> 
> On 6/15/2011 2:29 AM, Stefano Babic wrote:
> ...
>>
>> Why is the new output better as we have now ? You drop the output of
>> the srev register, and then we cannot get which strange silicon version
>> is running without patching the code.
> 
> Let me try to explain the problem I see with the current silicon detection mechanism:
> 
> On my board (srev=0x28), which is a TO2.0 silicon I get:
> 
> CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG
> 
> On Felix?s MX31ADS board (srev=0x20) (unknown chip version) he gets:
> 
> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
> 
> Reading rev 2.0 on Felix?s case is misleading IMHO as we tend to think that we have a TO2.0 silicon on his board even though we get a "unknown" string.
> 
> The reason of this is that we currently return the .v struct when the silicon version is detected and the srev version when it is not recognized.

Ouch

>>>  		if (srev == mx31_cpu_type[i].srev)
>>>  			return mx31_cpu_type[i].v;
>>>  
>>> -	return srev | 0x8000;
>>> +	return 0x8000;
>>
>> IMHO in the case the revision is not recognized, it is better to print
>> the value of the srev register, as it is done now. 
> 
> Yes, agreed, but we should print srev as an hex number instead of a string as done today
> 
> If you agree I can implement the following logic:
> 
> When the chip version is detected let?s just leave as it is today:
> 
> CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG
> 
> When the chip version is not valid, then we print:
> 
> CPU:   Freescale i.MX31 (unknown rev, srev=0x20) at 531 MHz.Reset cause: WDOG
> 
> Please let me know what you think about this proposal.

Does 'srev' have a more plain language description? or would somebody
reading that instantly know what 'srev' meant?

If the later, then this looks good. If the former, then you really should
be saying "(unknown rev, SOC Silicon ID=0x20)" where 'SOC silicon ID' is a
string taken directly from a data sheet or manual. There is nothing worse
than trying to do a search on a debug message in a manual where the string
does not even exist

Regards,

Graeme

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Fabio Estevam]

Hi Graeme,

On 6/15/2011 9:08 AM, Graeme Russ wrote:
...
> Does 'srev' have a more plain language description? or would somebody
> reading that instantly know what 'srev' meant?

Yes, srev is the exact term that is found on the MX31 Reference Manual - Table 13-2

Reading Wolfgang?s message he thinks it is better to discard this patch, so I am not going to send a v2.

Regards,

Fabio Estevam

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Wolfgang Denk]

Dear Fabio Estevam,

In message <4DF89C84.7090605@freescale.com> you wrote:
...
> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
...
> When the chip version is not valid, then we print:
>
> CPU:   Freescale i.MX31 (unknown rev, srev=0x20) at 531 MHz.Reset cause: WDOG

Compare to the current line above - which new information do you
print?  None.  You just print it in a new format, and with more
characters, i. e. with a worse SNR.

> Please let me know what you think about this proposal.

You received 3 messages from 3 different guys who told you all the
same: just leave the code as is.  Your proposed changes are not
considered an improvement.

Thanks.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"How is this place run - is it an anarchy?"
"No, I wouldn't say so; it is not that well organised..."

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Graeme Russ]

Hi Wolfgang,

On 15/06/11 22:12, Wolfgang Denk wrote:
> Dear Fabio Estevam,
> 
> In message <4DF89C84.7090605@freescale.com> you wrote:
> ...
>> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
> ...
>> When the chip version is not valid, then we print:
>>
>> CPU:   Freescale i.MX31 (unknown rev, srev=0x20) at 531 MHz.Reset cause: WDOG
> 
> Compare to the current line above - which new information do you
> print?  None.  You just print it in a new format, and with more
> characters, i. e. with a worse SNR.
> 
>> Please let me know what you think about this proposal.
> 
> You received 3 messages from 3 different guys who told you all the
> same: just leave the code as is.  Your proposed changes are not
> considered an improvement.

But as Fabio has pointed out, the '2.0' in 'rev 2.0' is not srev - This
highlights the root of the problem - (srev == 0x20) != (rev 2.0)

Regards,

Graeme

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Wolfgang Denk]

Dear Graeme Russ,

In message <4DF8A694.3030208@gmail.com> you wrote:
> 
> But as Fabio has pointed out, the '2.0' in 'rev 2.0' is not srev - This
> highlights the root of the problem - (srev == 0x20) != (rev 2.0)

But everybody who spends half a minute on the problem can easily
determine this, without adding code for an exotic error case.

It's not any new information that gets printed, it's just minimally
differently formatted.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Of course there's no reason for it, it's just our policy.

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Graeme Russ]

On 15/06/11 22:49, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4DF8A694.3030208@gmail.com> you wrote:
>>
>> But as Fabio has pointed out, the '2.0' in 'rev 2.0' is not srev - This
>> highlights the root of the problem - (srev == 0x20) != (rev 2.0)
> 
> But everybody who spends half a minute on the problem can easily
> determine this, without adding code for an exotic error case.

Only if they know '(unknown)' changes the meaning of 'rev' to 'srev' - And
that's the problem. If you don't have access to the code (or some fine
documentation) how will you know that 'rev 2.0 (unknown)' means you need to
search the SOC manual for 'srev == 0x20'

> It's not any new information that gets printed, it's just minimally
> differently formatted.

It is more information - In the known case, you are printing
mx31_cpu_type[i].v in the unknown case, you are printing srev

Now I personally think that in itself is a no-no, but fixing it would mean
doing something like:

CPU:   Freescale i.MX31 rev 2.0 (srev = 0x32) at 531 MHz.Reset cause: WDOG
for the known case

and

CPU:   Freescale i.MX31 unknown rev 0.0 (srev = 0x20) at 531 MHz.Reset
cause: WDOG

for the unknown case - Oops, we hit 80 characters :(

Regards,

Graeme

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Stefano Babic]

On 06/15/2011 01:50 PM, Fabio Estevam wrote:
> Hi Stefano,
> 

Hi Fabio,

> Let me try to explain the problem I see with the current silicon
> detection mechanism:
> 
> On my board (srev=0x28), which is a TO2.0 silicon I get:
> 
> CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG
> 
> On Felix?s MX31ADS board (srev=0x20) (unknown chip version) he gets:
> 
> CPU:   Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
> 
> Reading rev 2.0 on Felix?s case is misleading IMHO as we tend to
> think that we have a TO2.0 silicon on his board even though we get a
> "unknown" string.

Ok, so only reading the code we can know that version number + "unknown"
give us the value of the srev register. We can explicitely expand the
output to make things clearer.

>>> if (srev == mx31_cpu_type[i].srev) return mx31_cpu_type[i].v;
>>> 
>>> -	return srev | 0x8000; +	return 0x8000;
>> 
>> IMHO in the case the revision is not recognized, it is better to
>> print the value of the srev register, as it is done now.
> 
> Yes, agreed, but we should print srev as an hex number instead of a
> string as done today
> 
> If you agree I can implement the following logic:
> 
> When the chip version is detected let?s just leave as it is today:
> 
> CPU:   Freescale i.MX31 rev 2.0 at 531 MHz.Reset cause: WDOG
> 
> When the chip version is not valid, then we print:
> 
> CPU:   Freescale i.MX31 (unknown rev, srev=0x20) at 531 MHz.Reset
> cause: WDOG

Agree with your proposal.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Wolfgang Denk]

Dear Stefano Babic,

In message <4DF8A2DE.3040501@denx.de> you wrote:
>
> > Reading rev 2.0 on Felix=B4s case is misleading IMHO as we tend to
> > think that we have a TO2.0 silicon on his board even though we get a
> > "unknown" string.
> 
> Ok, so only reading the code we can know that version number + "unknown"
> give us the value of the srev register. We can explicitely expand the
> output to make things clearer.

Given that this is an exotic error case that happens only on a very
small number of chips that are actually not supposed to exist at all,
I think this is not worth the effort.

The "unknown" is warning enough, and anybody who needs to know more
details can see theremaining details easily from the code.

> Agree with your proposal.

No.  We should not add code that is almost never ever useful anywhere,
just to make this message more verbose.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Was hei?t Windows auf Indianisch? - "Wei?er  Mann,  der  auf  Sanduhr
wartet!"

[U-Boot] [PATCH] mx31: Improve the handling of unidentified silicon version

[Stefano Babic]

On 06/15/2011 02:47 PM, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 

Hi Wolfgang,

> 
> Given that this is an exotic error case that happens only on a very
> small number of chips that are actually not supposed to exist at all,

Right, it can happen with some pre-production chips, as I understood
from previous mails.

> No.  We should not add code that is almost never ever useful anywhere,
> just to make this message more verbose.

Ok. Let's the code as it is now.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================