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

[Detlev Zundel]

Hi Nori,

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

Very likely because of what I wrote above - if this is a qualification
_after_ the manufacturing.

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

So you call labeling the parts with the same revision a "documentation
trick"?  Is that easier than to accept the fact that the maximum speed
rating of a CPU is not a revision?

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

Let's take a step back.  What information do you want to pass to the
Linux kernel?  What does the Linux kernel do with it?

As far as I understand you, it is a maximum frequency, correct?  The
absolute limit is given by the labeling of individual parts - but for
whatever reason - maybe the user also wants to influence that.  So why
not call it "maximum operating frequency"?

If you really do have to pass the informatino to the kernel (why does no
other SoC in ARM use such a aconcept yet?  How do they handle multiple
frequencies?) and because I'm not too familiar with the ATAGs (flat
device trees are far superior), let's see what the current Linux kernel
declares.  [Studying the code] Ah, in arch/avr32/include/asm/setup.h
someone came up with the idea to create a generic ATAG_CLOCK tag.  That
does look promising - it scales, i.e. one can specify multiple "clocks"
(i.e. core, bus, whatever) and it uses long long so it will not overflow
in the near future.

Why not reuse such an existing concept which matches your usage much
better (arch/arm/include/asm/setup.h comments ATAG_REVISION as "board
revision")?

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

Yes, thanks - we spent already way too much time discussing this little
detail.

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

Did you check if we can learn from other code already present in U-Boot?

Let's see - in arch/mips/cpu/incaip_clock.c there is an environment
variable "cpuclk" which is in MHz.  Aha, the 8xx parts also use a
"cpuclk" environment variable which is even documented in
doc/README.MPC866.  

Ah, now I'm in a tight spot - contrary to my previous writings when I
belived we did not have a comparably construct - I would now vote to use
exactly the same name and thus unfortunately not use a "_mhz" suffix but
still specify the clock in mhz.

Thanks
  Detlev

-- 
(7)  It is always something
(7a) (corollary). Good, Fast, Cheap: Pick any two (you can't have all three).
                                   -- The Twelve Networking Truths (RFC 1925)
--
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