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

[Nori, Sekhar]

Hi Detlev,

On Fri, Aug 13, 2010 at 14:00:21, Detlev Zundel wrote:

>
> A revision for me is attached to certain bugs/problems which we may need
> to work around in software.  Think about something like "we can enable
> caching only on rev2 CPUs".  For all I know, the ATAG_REVISION tag seems
> to capture this aspect.

We will most likely end up using this aspect of ATAG_REVISION as further
revisions of the EVM appear.

>  The maximum speed of a CPU is orthogonal for
> me, i.e. there can still be a fast and a slow rev 1 CPU

Note that we are not taking about CPU being fast or slow at a given point
of time. We are talking about whether the cpu on the board can support a
given rate. It means that the silicon has been characterized (and probably
priced) differently. So you can actually treat it as a different CPU revision.
In fact, all of these speed graded parts are sold separately with different
datasheets. Please see the 375 and 456 AM1x parts:

http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dsp&sectionId=2&tabId=2641&familyId=1877&paramCriteria=no

and the 300 MHz OMAP-L1x parts:

http://focus.ti.com/paramsearch/docs/parametricsearch.tsp?family=dsp&sectionId=2&tabId=2231&familyId=1621&paramCriteria=no

Moreover, CPU revision only occupies bits 0-3 of the ATAG_REVISION.
As you mention the rest of the bits can be used to mark other changes in the
EVMs (bug fixes etc).

> but still we
> cannot enable cache.  Do you see my point?

No, let me know if the above explanation clarifies the topic for you.

> I hope I explained my point better this time.

Yes, but I am still unconvinced ATAG_REVISION is not suitable for this
purpose.

>
> >> > 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.
>
> I see, so please write in the documentation that the user is supposed to
> set this variable correctly for his chip.  I did not read this from the
> text.

Sure, I can include this in the commit text and in the README I promised.

> >> > +
> >> > +/*
> >> > + * 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?
>
> As I said, "bit[0-3]" denotes the 4 bits 0-3, i.e. a range from 0-15.
> (In this context the numbers below would actually translate into
> individual bits.)  Just drop this "bit[0-3]" and it is clear.

Why would dropping "bit[0-3]" make anything clear? As I mentioned
above other bits can be used in future to determine other aspects
of board revision. May be I can add that information. Is the following
more clear?

/*
 * 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
 * bit[4-31]    Reserved (unused for now)
 */

> >> 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.
>
> Shortness is a worthwhile goal but clearness even more so.  Those 4
> characters would prevent anyone from ever looking into the documentation
> on deciding what unit the value is in.  Personally I believe this is
> worth it.

I see. Let me toss between this and specifying the speed in HZ and leaving
the variable as is. I guess you would be OK both ways?

Thanks,
Sekhar