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

[Nori, Sekhar]

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.

> > +           break;
> > +   }
> > +
> > +   return rev;
>
> IMHO, the logic could be simplified?
>
> option 1:
> u8 rev=0;
> s = simple_strtoul(s, NULL, 10);
> 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?

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

Thanks,
Sekhar