Re: [U-Boot] [PATCH v2] TI: DaVinci DA850 EVM: support passing device speed grade information to kernel

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? Is this really a good idea? I can easily imagine different CPU revisions with different maximum clock frequencies. How will you handle that?

Is the counterpart in the Linux kernel already implemented?

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?

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 MHz
    

The 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.

• */

+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.

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"?

• 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?

Cheers Detlev