Hi Detlev,
On Thu, Aug 12, 2010 at 18:23:42, Detlev Zundel wrote:
> Hi Sekhar,
>
> > 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?
> 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.
>
> Is the counterpart in the Linux kernel already implemented?
It is implemented, but its submission upstream depends on this patch.
>
> > 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.
>
> > 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?
>
> > + */
> > +u32 get_board_rev(void)
> > +{
> > + char *s;
> > + u32 maxspeed = CONFIG_DA850_EVM_MAX_SPEED;
> > + u32 rev = 0;
> > +
> > + s = getenv("maxspeed");
>
> You introduce a new "magic" environment variable, so it should be
> documented at least in a board specific readme file.
Sure. Will 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.
>
> > + if (s)
> > + maxspeed = simple_strtoul(s, NULL, 10);
> > +
> > + switch (maxspeed) {
> > + case 456000:
> > + rev = 3;
> > + break;
> > + case 408000:
> > + rev = 2;
> > + break;
> > + case 372000:
> > + rev = 1;
> > + break;
> > + }
>
> Although the speeds are maximum values you check for _exact_ matches.
> Does this make sense? Why not use increasing "less than" compares?
Can do and will change.
Thanks for the feedback!
Thanks,
Sekhar
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
