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

[Nori, Sekhar]

Hi Detlev,

On Thu, Aug 12, 2010 at 18:23:42, Detlev Zundel wrote:
> 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?

The EVM can be populated with devices of different speed grades and this
patch treats those as different revisions of the EVM. Why would this be
treated as a misuse of ATAG_REVISION?

>  Is this really a good idea?  I can easily imagine different
> CPU revisions with different maximum clock frequencies.  How will you
> handle that?

I don't think I understood you. This patch _is_ meant to handle
different revisions of DA850 EVMs populated with DA850 devices
of differing speed grades.

>
> Is the counterpart in the Linux kernel already implemented?

It is implemented, but its submission upstream depends on this patch.

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

UBL does not (cannot) set the maxspeed variable. The user of U-Boot is
expected to set it based on what he sees on the packaging. Alternately
he can also change the U-Boot configuration file and re-build U-Boot.
UBL will setup the board to work at the common frequency of 300 MHz.

>
> > Signed-off-by: Sekhar Nori <nsekhar at 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.

[0-3] usually indicates the range the range 0 to 3. Do you have
suggestions on how else this might be documented?

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

Sure. Will do.

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

I am hoping the documentation promised in the response above
will help clarify its usage. I wanted to keep the variable name
short.

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

Can do and will change.

Thanks for the feedback!

Thanks,
Sekhar