Hi Detlev,
On Thu, Aug 12, 2010 at 18:23:42, Detlev Zundel wrote:
Hi Sekhar,
The TI DA850/OMAP-L138/AM18x EVM can be populated with devices of different speed grades.
The maximum speed the chip can support can only be determined from the label on the package (not software readable).
Introduce a method to pass the speed grade information to kernel using ATAG_REVISION. The kernel uses this information to determine the maximum speed reachable using cpufreq.
Do I understand you correctly that you _misuses_ an atag defined to carry the revision of a CPU to carry the maximum allowed clock frequency?
The EVM can be populated with devices of different speed grades and this patch treats those as different revisions of the EVM. Why would this be treated as a misuse of ATAG_REVISION?
Is this really a good idea? I can easily imagine different CPU revisions with different maximum clock frequencies. How will you handle that?
I don't think I understood you. This patch _is_ meant to handle different revisions of DA850 EVMs populated with DA850 devices of differing speed grades.
Is the counterpart in the Linux kernel already implemented?
It is implemented, but its submission upstream depends on this patch.
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.
Signed-off-by: Sekhar Nori nsekhar@ti.com
v2: removed unnecessary logical OR while constructing revision value
board/davinci/da8xxevm/da850evm.c | 38 +++++++++++++++++++++++++++++++++++++ include/configs/da850evm.h | 1 + 2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c index eeb456c..0eb3608 100644 --- a/board/davinci/da8xxevm/da850evm.c +++ b/board/davinci/da8xxevm/da850evm.c @@ -70,6 +70,44 @@ static const struct lpsc_resource lpsc[] = { { DAVINCI_LPSC_GPIO }, };
+#ifndef CONFIG_DA850_EVM_MAX_SPEED +#define CONFIG_DA850_EVM_MAX_SPEED 300000 +#endif
+/*
- 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?
- */
+u32 get_board_rev(void) +{
- char *s;
- u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED;
- u32 rev = 0;
- s = getenv("maxspeed");
You introduce a new "magic" environment variable, so it should be documented at least in a board specific readme file.
Sure. Will 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.
- if (s)
maxspeed = simple_strtoul(s, NULL, 10);- switch (maxspeed) {
- case 456000:
rev = 3;break;- case 408000:
rev = 2;break;- case 372000:
rev = 1;break;- }
Although the speeds are maximum values you check for _exact_ matches. Does this make sense? Why not use increasing "less than" compares?
Can do and will change.
Thanks for the feedback!
Thanks, Sekhar