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

Tom Rini
2012-02-17T17:26:56Z

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