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

[Detlev Zundel]

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.

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

AM1806 ARM Microprocessor (Rev. B) (PDF 1431 KB)  03 May 2010
AM1806 Silicon Errata Silicon Revision 2.0 (PDF 66 KB)  12 Mar 2010

So I cannot follow your "differnet datasheets" here.  Also the usage of
"revision" here is not coupled to any operating frequency.

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

When writing code which should also be maintainable by other people it
is a good idea to consider common expectations also of other people.

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

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

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.  

Think about it this way - how often will the user actually type the name
of the variable?  Is saving 4 characters worth making him look up the
documentation?

Cheers
  Detlev

-- 
PUBLIC NOTICE AS REQUIRED BY LAW:    Any Use of  This Product,  in Any Manner
Whatsoever, Will Increase the Amount of Disorder in the Universe. Although No
Liability Is  Implied Herein,  the Consumer Is Warned  That This Process Will
Ultimately Lead to the Heat Death of the Universe.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de