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 <nsek...@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: d...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
