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

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

On 08/12/2010 01:14 AM, Nori, Sekhar wrote:
>
> Hi Nishanth,
>
> On Thu, Aug 12, 2010 at 11:13:10, Nishanth Menon wrote:
>> 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;
>
> [...]
>
>>>
>>>>> +           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..
>
> I still don't see how this handles the case when maxspeed env variable
> is not set. The above code will just default to rev = 0 in that case.
it does the same here as well (default rev = 0). and the fact that 
compared to v1, |= was avoided (this is part of v2 of the patch).

>
> We haven't gotten rid of any constructs either so not sure what the
> simplification here is.
got rid of a var maxspeed - but I doubt it will have much benefit, 
please consider my objection withdrawn.. just to point to the note that 
the var has not much of a lifetime or a function beyond providing data 
to the switch..

Regards,
Nishanth Mnon