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

[Tom Rini <trini@ti.com>]

On 02/17/2012 06:22 AM, Laurence Withers wrote:
> 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.

Review is always welcome, thanks.

[snip]
>> 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.

Checking powerpc there is precedent for #if'ing this structure, so that 
should be done.  As for making it more generic, yes, my hope is that 
once the next SoC says "I want to print clocks" we can start mapping 
things out more.  And as you noted above (and I snipped) we do need to 
clean up and fix the current init sequence.

> 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).

I was hoping checkpatch warned on that type of mismatch so I need to 
rework my workflow a bit.

[snip]
>> 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.

Note that do_bdinfo is already per-arch.  This is intended as a starting 
point for generic work and the first step away from doing this in a 
per-board fashion.  Thanks!

-- 
Tom