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

[Detlev Zundel <dzu@denx.de>]

Hi Nori,

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

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.  The maximum speed of a CPU is orthogonal for
me, i.e. there can still be a fast and a slow rev 1 CPU but still we
cannot enable cache.  Do you see my point?  If you want to express a
maximum speed, then use an ATAG which supports such a usage.  Reading
the name ATAG_REVISION in code I would _never_ think that this
transports the maximum clock frequency.

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

I hope I explained my point better this time.

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

>> > Signed-off-by: Sekhar Nori <nsekhar@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?

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.

>> 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 still remember old times when the Linux kernel for PowerPC
switched from its interpretation of clock frequencies from hz to mhz and
the many support questions it generated....

Cheers
  Detlev

-- 
Of course my password is the same as my pet's name
My macaw's name was Q47pY!3 and I change it every 90 days
                        -- Trevor Linton
--
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