mx31: Improve the handling of unidentified silicon version

PATCH New Fabio Estevam 4 participants 14 messages Jun 14, 2011 – Jun 15, 2011 Arm (32-bit)

lore

Share & tools

lore

Fabio Estevam

author

2011-06-14 16:31 UTC #0 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

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 <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 arch/arm/cpu/arm1136/mx31/generic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/cpu/arm1136/mx31/generic.c +2 -2

index 4ebf38d..770d359 100644
@@ -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;

Signature · 1 line

1.6.0.4

Wolfgang Denk

2011-06-15 05:28 UTC #2 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

In reply to Fabio Estevam

Dear Fabio Estevam,

In message <[email protected]> you wrote:
...

Quoted 10 lines

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

Signature · 5 lines

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

Stefano Babic

2011-06-15 05:29 UTC #3 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

In reply to Fabio Estevam

Quoted 2 lines · Fabio Estevam

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

Hi Fabio,

Quoted 10 lines

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.

arch/arm/cpu/arm1136/mx31/generic.c u32 get_cpu_rev(void) · line 133

Quoted 5 lines

arch/arm/cpu/arm1136/mx31/generic.c +1 -1

 		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

Signature · 5 lines

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

Fabio Estevam

author

2011-06-15 11:50 UTC #4 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Hi Stefano,

On 6/15/2011 2:29 AM, Stefano Babic wrote:
...

Quoted 4 lines

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.

Quoted 8 lines

arch/arm/cpu/arm1136/mx31/generic.c u32 get_cpu_rev(void) · line 133
Quoted 5 lines
arch/arm/cpu/arm1136/mx31/generic.c +1 -1
 		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

Graeme Russ

2011-06-15 12:08 UTC #5 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Hi Fabio,

Quoted 15 lines · Fabio Estevam

Hi Stefano,
On 6/15/2011 2:29 AM, Stefano Babic wrote:
...
Quoted 4 lines
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

Quoted 7 lines

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

Quoted 15 lines

Quoted 8 lines
arch/arm/cpu/arm1136/mx31/generic.c u32 get_cpu_rev(void) · line 133
Quoted 5 lines
arch/arm/cpu/arm1136/mx31/generic.c +1 -1
 		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:

Quoted 7 lines

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

Fabio Estevam

author

2011-06-15 12:17 UTC #6 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Hi Graeme,

On 6/15/2011 9:08 AM, Graeme Russ wrote:
...

Quoted 2 lines

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

Wolfgang Denk

2011-06-15 12:12 UTC #7 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

In reply to Fabio Estevam

Dear Fabio Estevam,

In message <[email protected]> you wrote:
...

Quoted 1 line

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

...

Quoted 3 lines

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.

Quoted 1 line

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

Signature · 5 lines

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

Graeme Russ

2011-06-15 12:33 UTC #8 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Hi Wolfgang,

Quoted 14 lines · Wolfgang Denk

Dear Fabio Estevam,
In message <[email protected]> you wrote:
...
Quoted 1 line
CPU: Freescale i.MX31 rev 2.0 unknown at 531 MHz.Reset cause: WDOG
...
Quoted 3 lines
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.

Quoted 5 lines

Quoted 1 line
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

Wolfgang Denk

2011-06-15 12:49 UTC #9 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Dear Graeme Russ,

Quoted 3 lines · In message <[email protected]> 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

Signature · 4 lines

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.

Graeme Russ

2011-06-15 13:00 UTC #10 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Quoted 9 lines · Wolfgang Denk

Dear Graeme Russ,
Quoted 3 lines · In message <[email protected]> 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'

Quoted 2 lines

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

Stefano Babic

2011-06-15 12:17 UTC #11 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

In reply to Fabio Estevam

Quoted 2 lines · Fabio Estevam

Hi Stefano,

Hi Fabio,

Quoted 7 lines

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

Quoted 7 lines

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.

Quoted 12 lines

Quoted 6 lines
Review
Quoted 3 lines
if (srev == mx31_cpu_type[i].srev) return mx31_cpu_type[i].v;
(quoted diff) +0 -1
-	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:

Quoted 8 lines

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

Signature · 5 lines

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

Wolfgang Denk

2011-06-15 12:47 UTC #12 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Dear Stefano Babic,

Quoted 8 lines · In message <[email protected]> you wrote:

Quoted 4 lines
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.

Quoted 1 line

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

Signature · 5 lines

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

Stefano Babic

2011-06-15 13:02 UTC #13 lore raw

PATCH 1/1 mx31: Improve the handling of unidentified silicon version

Quoted 2 lines · Wolfgang Denk

Dear Stefano Babic,

Hi Wolfgang,

Quoted 3 lines

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.

Quoted 2 lines

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

Signature · 5 lines

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