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

[Nishanth Menon <menon.nishanth@gmail.com>]

On 08/11/2010 10:37 AM, Nori, Sekhar wrote:
> Hi Nishanth,
>
> On Wed, Aug 11, 2010 at 09:33:29, Nishanth Menon wrote:
>> On 08/10/2010 06:39 AM, Sekhar Nori wrote:
>
>>> diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c
>>> index 959b2c6..6a6d4fb 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
>>> + */
>>> +u32 get_board_rev(void)
>>> +{
>>> +   char *s;
>>> +   u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED;
>>> +   u32 rev = 0;
>>> +
>>> +   s = getenv("maxspeed");
>>> +   if (s)
>>> +           maxspeed = simple_strtoul(s, NULL, 10);
>>> +
>>> +   switch (maxspeed) {
>>> +   case 456000:
>>> +           rev |= 3;
>>> +           break;
>>> +   case 408000:
>>> +           rev |= 2;
>>> +           break;
>>> +   case 372000:
>>> +           rev |= 1;
>> wondering if the |= makes any sense...
>
> Yeah, I put it in there in case additional fields pop-up
> in board revision. It doesn't make a lot of sense now so
> I will remove it.
thx.

>
>>> +           break;
>>> +   }
>>> +
>>> +   return rev;
>>
>> IMHO, the logic could be simplified?
>>
>> option 1:
>> u8 rev=0;
>> s = simple_strtoul(s, NULL, 10);
aarrg.. emailing with eyes half shut mistake
should have been:
s = getenv("maxspeed");
>> if (s) {
>>        switch (simple_strtoul(s, NULL, 10)) {
>>        case 456000:
>>                rev = 3;
>>                break;
>>        case 408000:
>>                rev = 2;
>>                break;
>>        case 372000:
>>                rev = 1;
>>                break;
>>        }
>> }
>
> Not sure how this simplifies the logic. I'd argue multiple strtoul
> calls are actually better avoided. How does it handle the case where
> max speed is to be set using board configuration?
>
my bad, the above should explain..

>>
>> option 2:
>> if you think that the speeds could get added in the future, that switch
>> is going to look pretty ugly.. use a lookup instead..
>
> Right now there is no known plan to add more speeds so I will
> stick with the switch. You are right, option of using a lookup
> deserves a look-in if there were a lot more speed steps.
ok.
Regards,
Nishanth Menon