[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 16:09:41, Detlev Zundel wrote:
> Hi Nori,
>
> >> 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.
>
> Well yes, you _can_ treat that as a "revision", but certainly I would
> not understand what you mean.  A CPU revision for me is connected to the
> physical chip mask on the CPU (i.e. what goes into the manufacturing
> process) and the maximum allowed operating frequency is a property of an
> individual chip possibly only detected by actual measurements (what
> comes out of manufacturing).  I never heard people talk about
> _functionally equivalent_ CPUs with different graded operating
> frequencies as "different revisions", but YMMV.

It would have been nice if hardware provided a way to detect
this difference between chips, but unfortunately it does not.

>
> > 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
>
> They surely have differnet part numbers and this is perfectly fine.
>
> But let's take a look at your first link.  I see two parts here with
> different allowed frequencies: AM1806-375 and AM1806-456.  _Both_ links
> lead to the same page with these datasheets:

This is just a documentation trick to make sure common aspects don't have
to be maintained separately.

> > Yes, but I am still unconvinced ATAG_REVISION is not suitable for this
> > purpose.
>
> When writing code which should also be maintainable by other people it
> is a good idea to consider common expectations also of other people.

Agreed. And I am open to concrete suggestions on how this could be
better done.

>
> >> >> > +
> >> >> > +/*
> >> >> > + * 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)
> >  */
>
> Let me try to reformulate the ambiguity that I see here - when a
> documentation tells that "bit[0-3]" is used, then I would really expect
> something like "0001 - xx  0010 - yy ; 0100 - zz".  If I don't see
> this and read "0 - xx; 1 - yy; 2 - zz; 3 - aa" on first read I would
> interpret this as "bit 0 - xx; bit 1 - yy; bit 2 - zz; bit 3 - aa" which
> is obviously _not_ what you do.

Okay. I can document the values in binary instead of decimal if that helps
readability. Does the following look good?

/*
  * get_board_rev() - setup to pass kernel board revision information
  * Returns:
  * bit[0-3]             Maximum speed supported by the DA850/OMAP-L138/AM18x part
  *                              0000b - 300 MHz
  *                              0001b - 372 MHz
  *                              0010b - 408 MHz
  *                              0011b - 456 MHz
  * bit[4-31]    Reserved (unused for now)
  */

> Using HZ would probably settle the "which unit is used" question, but
> the code would overflow at ~4.2 GHz and the numbers will be large and
> entry errors have to be expected.  As Hz is too fine for CPU frequencies
> this would lead me to use either kilo or megaherz but I would express it
> in the variable name.

I don't have a very strong inclination on this so I will go with
your suggestion.

Thanks,
Sekhar