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? Is this really a good idea? I can easily imagine different
CPU revisions with different maximum clock frequencies. How will you
handle that?
Is the counterpart in the Linux kernel already implemented?
> 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?
> 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.
> + */
> +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.
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"?
> + 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?
Cheers
Detlev
--
Warning: this comic occasionally contains strong language (which may be unsuit-
able for children), unusual humor (which may be unsuitable for adults), and ad-
vanced mathematics (which may be unsuitable for liberal-arts majors). /xkcd.org
--
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
