[U-Boot] [PATCH v2 2/2] davinci: add support for printing clock frequency

[Laurence Withers <lwithers@guralp.com>]

On Mon, Feb 06, 2012 at 04:00:44PM +0530, Manjunath Hadli wrote:
> add support for printing various clock frequency info found
> in SOC such as ARM core frequency, DSP core frequency and DDR
> frequency as part of bdinfo command.
> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Cc: Tom Rini <trini@ti.com>

Sorry I am late to the party, especially as this already seems to have been
applied to u-boot master, but I have just tried out this bit of code and have
some feedback having looked at it.

> ---
>  arch/arm/cpu/arm926ejs/davinci/cpu.c   |   32 ++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/u-boot.h          |    3 +++
>  arch/arm/lib/board.c                   |   10 +++++++++-
>  common/cmd_bdinfo.c                    |    9 +++++++++
>  include/common.h                       |    1 +
>  include/configs/cam_enc_4xx.h          |    4 ++++
>  include/configs/da830evm.h             |    4 ++++
>  include/configs/da850evm.h             |    4 ++++
>  include/configs/davinci_dm355evm.h     |    4 ++++
>  include/configs/davinci_dm355leopard.h |    4 ++++
>  include/configs/davinci_dm365evm.h     |    4 ++++
>  include/configs/davinci_dm6467Tevm.h   |    4 ++++
>  include/configs/davinci_dm6467evm.h    |    4 ++++
>  include/configs/davinci_dvevm.h        |    5 +++++
>  include/configs/davinci_schmoogie.h    |    4 ++++
>  include/configs/davinci_sffsdr.h       |    4 ++++
>  include/configs/davinci_sonata.h       |    4 ++++
>  include/configs/ea20.h                 |    4 ++++
>  include/configs/enbw_cmc.h             |    4 ++++
>  include/configs/hawkboard.h            |    4 ++++
>  20 files changed, 115 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/davinci/cpu.c b/arch/arm/cpu/arm926ejs/davinci/cpu.c
> index 1735555..b3c9fb7 100644
> --- a/arch/arm/cpu/arm926ejs/davinci/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/davinci/cpu.c
> @@ -25,6 +25,8 @@
>  #include <asm/arch/hardware.h>
>  #include <asm/io.h>
>  
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  /* offsets from PLL controller base */
>  #define PLLC_PLLCTL	0x100
>  #define PLLC_PLLM	0x110
> @@ -187,6 +189,36 @@ unsigned int davinci_clk_get(unsigned int div)
>  #endif
>  #endif /* !CONFIG_SOC_DA8XX */
>  
> +int set_cpu_clk_info(void)
> +{
> +#ifdef CONFIG_SOC_DA8XX
> +	gd->bd->bi_arm_freq = clk_get(DAVINCI_ARM_CLKID) / 1000000;
> +	/* DDR PHY uses an x2 input clock */
> +	gd->bd->bi_ddr_freq = clk_get(0x10001) / 1000000;

0x10001 is a magic number... add a define for it?

> +#else
> +
> +	unsigned int pllbase = DAVINCI_PLL_CNTRL0_BASE;
> +#if defined(CONFIG_SOC_DM365)
> +	pllbase = DAVINCI_PLL_CNTRL1_BASE;
> +#endif
> +	gd->bd->bi_arm_freq = pll_sysclk_mhz(pllbase, ARM_PLLDIV);
> +
> +#ifdef DSP_PLLDIV
> +	gd->bd->bi_dsp_freq =
> +		pll_sysclk_mhz(DAVINCI_PLL_CNTRL0_BASE, DSP_PLLDIV);
> +#else
> +	gd->bd->bi_dsp_freq = 0;
> +#endif
> +
> +	pllbase = DAVINCI_PLL_CNTRL1_BASE;
> +#if defined(CONFIG_SOC_DM365)
> +	pllbase = DAVINCI_PLL_CNTRL0_BASE;
> +#endif
> +	gd->bd->bi_ddr_freq = pll_sysclk_mhz(pllbase, DDR_PLLDIV) / 2;
> +#endif
> +	return 0;
> +}
> +

This function seems to be wrong. If CONFIG_SOC_DA8XX, then gd->bd->bi_dsp_freq
never gets initialised, as evidenced by the following result:

gsl-diamond > bdinfo
arch_number = 0x00000D27
(...snip...)
ARM frequency = 300 MHz
DSP frequency = -17 MHz
DDR frequency = 100 MHz

I suppose it could be resolved by moving the section starting at #ifdef
DSP_PLLDIV to just above the return statement, but to be honest I found
this function rather difficult to follow. Perhaps it could be reworked
to be a bit cleaner?

>  /*
>   * Initializes on-chip ethernet controllers.
>   * to override, implement board_eth_init()
> diff --git a/arch/arm/include/asm/u-boot.h b/arch/arm/include/asm/u-boot.h
> index f30b9fc..20e1653 100644
> --- a/arch/arm/include/asm/u-boot.h
> +++ b/arch/arm/include/asm/u-boot.h
> @@ -41,6 +41,9 @@ typedef struct bd_info {
>      unsigned long	bi_ip_addr;	/* IP Address */
>      ulong	        bi_arch_number;	/* unique id for this board */
>      ulong	        bi_boot_params;	/* where this board expects params */
> +	unsigned long	bi_arm_freq; /* arm frequency */
> +	unsigned long	bi_dsp_freq; /* dsp core frequency */
> +	unsigned long	bi_ddr_freq; /* ddr frequency */
>      struct				/* RAM configuration */
>      {
>  	ulong start;

I'm not sure that this really belongs in every ARM board's struct bd_info, as
it implies the only three clocks we'll care about are ARM, DSP and DDR. I
doubt that maps nicely onto most of the SoCs out there.

Whitespace doesn't match the rest of the file (even if the rest of the file
is wrong, and this patch has used tabs as it should have).

> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 3d78274..500e216 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -463,7 +463,15 @@ void board_init_r(gd_t *id, ulong dest_addr)
>  
>  	debug("monitor flash len: %08lX\n", monitor_flash_len);
>  	board_init();	/* Setup chipselects */
> -
> +	/*
> +	 * TODO: printing of the clock inforamtion of the board is now
> +	 * implemented as part of bdinfo command. Currently only support for
> +	 * davinci SOC's is added. Remove this check once all the board
> +	 * implement this.
> +	 */
> +#ifdef CONFIG_CLOCKS
> +	set_cpu_clk_info(); /* Setup clock information */
> +#endif
>  #ifdef CONFIG_SERIAL_MULTI
>  	serial_initialize();
>  #endif
> diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
> index 97f2945..5359a47 100644
> --- a/common/cmd_bdinfo.c
> +++ b/common/cmd_bdinfo.c
> @@ -370,6 +370,15 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	print_num("irq_sp", gd->irq_sp);	/* irq stack pointer */
>  	print_num("sp start ", gd->start_addr_sp);
>  	print_num("FB base  ", gd->fb_base);
> +	/*
> +	 * TODO: Currently only support for davinci SOC's is added.
> +	 * Remove this check once all the board implement this.
> +	 */
> +#ifdef CONFIG_CLOCKS
> +	printf("ARM frequency = %ld MHz\n", gd->bd->bi_arm_freq);
> +	printf("DSP frequency = %ld MHz\n", gd->bd->bi_dsp_freq);
> +	printf("DDR frequency = %ld MHz\n", gd->bd->bi_ddr_freq);
> +#endif
>  	return 0;
>  }
>  

Again, here it is implied that the only clocks we'll ever want to print are
ARM, DSP and DDR. Seems way too specific for such common code.

Bye for now,
-- 
Laurence Withers, <lwithers@guralp.com>                http://www.guralp.com/
Direct tel:+447753988197 or tel:+443333408643               Software Engineer
General support queries: <support@guralp.com>         CMG-DCM CMG-EAM CMG-NAM