| Message ID | 1308069108-5438-1-git-send-email-fabio.estevam@freescale.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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
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;
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(-)