Hi Detlev,
On Fri, Aug 13, 2010 at 16:09:41, Detlev Zundel wrote:
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.
It would have been nice if hardware provided a way to detect this difference between chips, but unfortunately it does not.
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:
This is just a documentation trick to make sure common aspects don't have to be maintained separately.
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.
Agreed. And I am open to concrete suggestions on how this could be better done.
+/*
- 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.
Okay. I can document the values in binary instead of decimal if that helps readability. Does the following look good?
/* * get_board_rev() - setup to pass kernel board revision information * Returns: * bit[0-3] Maximum speed supported by the DA850/OMAP-L138/AM18x part * 0000b - 300 MHz * 0001b - 372 MHz * 0010b - 408 MHz * 0011b - 456 MHz * bit[4-31] Reserved (unused for now) */
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.
I don't have a very strong inclination on this so I will go with your suggestion.
Thanks, Sekhar