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

[Detlev Zundel]

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 <nsekhar at 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: dzu at denx.de