Hi Detlev,
On Fri, Aug 13, 2010 at 14:00:21, Detlev Zundel wrote:
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. 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...
and the 300 MHz OMAP-L1x parts:
http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dsp%C2%A7io...
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.
Note that U-Boot itself does not set the CPU rate. The CPU speed is setup by a primary bootloader ("UBL"). The rate setup by UBL could be different from the maximum speed grade of the device.
I do not understand how the UBL gets to set the _U-Boot_ environment variable "maxspeed". Can you please explain how this is done?
UBL does not (cannot) set the maxspeed variable. The user of U-Boot is expected to set it based on what he sees on the packaging. Alternately he can also change the U-Boot configuration file and re-build U-Boot. UBL will setup the board to work at the common frequency of 300 MHz.
I see, so please write in the documentation that the user is supposed to set this variable correctly for his chip. I did not read this from the text.
Sure, I can include this in the commit text and in the README I promised.
+/*
- 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 MHz * 1 - 372 MHz * 2 - 408 MHz * 3 - 456 MHz * bit[4-31] Reserved (unused for now) */
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?
Thanks, Sekhar