Hi Nori,
A revision for me is attached to certain bugs/problems which we may need to work around in software. Think about something like "we can enable caching only on rev2 CPUs". For all I know, the ATAG_REVISION tag seems to capture this aspect.
We will most likely end up using this aspect of ATAG_REVISION as further revisions of the EVM appear.
The maximum speed of a CPU is orthogonal for me, i.e. there can still be a fast and a slow rev 1 CPU
Note that we are not taking about CPU being fast or slow at a given point of time. We are talking about whether the cpu on the board can support a given rate. It means that the silicon has been characterized (and probably priced) differently. So you can actually treat it as a different CPU revision.
Well yes, you _can_ treat that as a "revision", but certainly I would not understand what you mean. A CPU revision for me is connected to the physical chip mask on the CPU (i.e. what goes into the manufacturing process) and the maximum allowed operating frequency is a property of an individual chip possibly only detected by actual measurements (what comes out of manufacturing). I never heard people talk about _functionally equivalent_ CPUs with different graded operating frequencies as "different revisions", but YMMV.
In fact, all of these speed graded parts are sold separately with different datasheets. Please see the 375 and 456 AM1x parts:
http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dsp%C2%A7io...
They surely have differnet part numbers and this is perfectly fine.
But let's take a look at your first link. I see two parts here with different allowed frequencies: AM1806-375 and AM1806-456. _Both_ links lead to the same page with these datasheets:
AM1806 ARM Microprocessor (Rev. B) (PDF 1431 KB) 03 May 2010 AM1806 Silicon Errata Silicon Revision 2.0 (PDF 66 KB) 12 Mar 2010
So I cannot follow your "differnet datasheets" here. Also the usage of "revision" here is not coupled to any operating frequency.
Moreover, CPU revision only occupies bits 0-3 of the ATAG_REVISION. As you mention the rest of the bits can be used to mark other changes in the EVMs (bug fixes etc).
but still we cannot enable cache. Do you see my point?
No, let me know if the above explanation clarifies the topic for you.
I hope I explained my point better this time.
Yes, but I am still unconvinced ATAG_REVISION is not suitable for this purpose.
When writing code which should also be maintainable by other people it is a good idea to consider common expectations also of other people.
+/*
- get_board_rev() - setup to pass kernel board revision information
- Returns:
- bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part
0 - 300 MHz
1 - 372 MHz
2 - 408 MHz
3 - 456 MHzThe description says it returns "bit[0-3]" which may seem that those canstants are encoded by a single bit each, whereas the code uses integer values. Change either the comment or the code.
[0-3] usually indicates the range the range 0 to 3. Do you have suggestions on how else this might be documented?
As I said, "bit[0-3]" denotes the 4 bits 0-3, i.e. a range from 0-15. (In this context the numbers below would actually translate into individual bits.) Just drop this "bit[0-3]" and it is clear.
Why would dropping "bit[0-3]" make anything clear? As I mentioned above other bits can be used in future to determine other aspects of board revision. May be I can add that information. Is the following more clear?
/*
- get_board_rev() - setup to pass kernel board revision information
- Returns:
- bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part
0 - 300 MHz1 - 372 MHz2 - 408 MHz3 - 456 MHz- bit[4-31] Reserved (unused for now)
*/
Let me try to reformulate the ambiguity that I see here - when a documentation tells that "bit[0-3]" is used, then I would really expect something like "0001 - xx 0010 - yy ; 0100 - zz". If I don't see this and read "0 - xx; 1 - yy; 2 - zz; 3 - aa" on first read I would interpret this as "bit 0 - xx; bit 1 - yy; bit 2 - zz; bit 3 - aa" which is obviously _not_ what you do.
Moreover I do not like that you call the variable "maxpseed" but interpret the value in kHz. Maybe the variable should be called "maxspeed_khz"?
I am hoping the documentation promised in the response above will help clarify its usage. I wanted to keep the variable name short.
Shortness is a worthwhile goal but clearness even more so. Those 4 characters would prevent anyone from ever looking into the documentation on deciding what unit the value is in. Personally I believe this is worth it.
I see. Let me toss between this and specifying the speed in HZ and leaving the variable as is. I guess you would be OK both ways?
Using HZ would probably settle the "which unit is used" question, but the code would overflow at ~4.2 GHz and the numbers will be large and entry errors have to be expected. As Hz is too fine for CPU frequencies this would lead me to use either kilo or megaherz but I would express it in the variable name.
Think about it this way - how often will the user actually type the name of the variable? Is saving 4 characters worth making him look up the documentation?
Cheers Detlev