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

[Nori, Sekhar]

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.

We haven't gotten rid of any constructs either so not sure what the
simplification here is.

Thanks,
Sekhar