[U-Boot] [PATCH v2 13/22] omap4: add clock support

[Aneesh V]

Hi Wolfgang,

On Monday 16 May 2011 12:30 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<1305472900-4004-14-git-send-email-aneesh at ti.com>  you wrote:
>> Add support for:
>> 1. DPLL locking
>> 2. Initialization of clock domains and clock modules
>>
>> This work draws upon previous work done for x-loader mainly by:
>> 	Santosh Shilimkar<santosh.shilimkar at ti.com>
>> 	Rajendra Nayak<rnayak at ti.com>
>>
>> Signed-off-by: Aneesh V<aneesh at ti.com>
>> ---
>> V2:
>> * Use pre-calculated M&  N values instead of calculated ones
>> * Changes due to make file changes
>> * Some corrections
>> * Do all clock initialization in SPL itself instead of differing some
>>    work to u-boot
>> ---
>>   arch/arm/cpu/armv7/omap4/Makefile           |    1 +
>>   arch/arm/cpu/armv7/omap4/board.c            |    1 +
>>   arch/arm/cpu/armv7/omap4/clocks.c           |  731 +++++++++++++++++++++++++++
>>   arch/arm/cpu/armv7/omap4/clocks_get_m_n.c   |  154 ++++++
>>   arch/arm/include/asm/arch-omap4/clocks.h    |  506 ++++++++++++++++++
>>   arch/arm/include/asm/arch-omap4/sys_proto.h |    6 +
>>   arch/arm/include/asm/omap_common.h          |    3 +
>>   spl/board/ti/omap4.mk                       |    7 +-
>>   8 files changed, 1408 insertions(+), 1 deletions(-)
>>   create mode 100644 arch/arm/cpu/armv7/omap4/clocks.c
>>   create mode 100644 arch/arm/cpu/armv7/omap4/clocks_get_m_n.c
>>   create mode 100644 arch/arm/include/asm/arch-omap4/clocks.h
>
> It appears this might be part of or taken from some bigger scope
> clocks framework.  Otherwise it's diffcult for me to understand why
> OMAP4 needs 1400+ lines of code, when other SoCs appear to do with
> considerably less. Please comment.
>

No. This code was written for SPL. Please note that a lot of it is
tables used for PLL locking, clock enabling etc. OMAP4 supports
different crystal frequencies. So, more entries in each table. Also,
there are some special handling based on the OMAP4 revisions because of
some frequency limitations. So, more number of tables.

>> diff --git a/arch/arm/cpu/armv7/omap4/Makefile b/arch/arm/cpu/armv7/omap4/Makefile
>> index 987dc9d..6154e86 100644
>> --- a/arch/arm/cpu/armv7/omap4/Makefile
>> +++ b/arch/arm/cpu/armv7/omap4/Makefile
>> @@ -30,6 +30,7 @@ SOBJS	+= lowlevel_init.o
>>   COBJS	+= board.o
>>   COBJS	+= mem.o
>>   COBJS	+= sys_info.o
>> +COBJS	+= clocks.o
>
> Please keep lists sorted.

ok.

>
> ...
>> +static inline void do_bypass_dpll(u32 base)
>> +{
>> +	struct dpll_regs *dpll_regs = (struct dpll_regs *)base;
>> +
>> +	modify_reg_32(&dpll_regs->cm_clkmode_dpll,
>> +		      CM_CLKMODE_DPLL_DPLL_EN_SHIFT,
>> +		      CM_CLKMODE_DPLL_DPLL_EN_MASK, DPLL_EN_FAST_RELOCK_BYPASS);
>
> NAK, please use standard macros (see previous messages).
>
>> +static void do_setup_dpll(u32 base, const struct dpll_params *params, u8 lock)
>> +{
>> +	u32 temp;
>> +	struct dpll_regs *dpll_regs = (struct dpll_regs *)base;
>> +
>> +	bypass_dpll(base);
>> +
>> +	/* Set M&  N */
>> +	temp = readl(&dpll_regs->cm_clksel_dpll);
>> +	set_bit_field(temp, CM_CLKSEL_DPLL_M_SHIFT, CM_CLKSEL_DPLL_M_MASK,
>> +			params->m);
>> +	set_bit_field(temp, CM_CLKSEL_DPLL_N_SHIFT, CM_CLKSEL_DPLL_N_MASK,
>> +			params->n);
>> +	writel(temp,&dpll_regs->cm_clksel_dpll);
>
> NAK, please use standard macros (see previous messages).
>
>
> ...
>> diff --git a/arch/arm/cpu/armv7/omap4/clocks_get_m_n.c b/arch/arm/cpu/armv7/omap4/clocks_get_m_n.c
>> new file mode 100644
>> index 0000000..777ec11
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/omap4/clocks_get_m_n.c
>> @@ -0,0 +1,154 @@
> ...
>> +void main(void)
>
> It appears this is a host program, not part of U-Boot.  I don't think
> that arch/arm/cpu/armv7/omap4/ is the right place for this program.
>
> ...
...
>> +#define CM_CLKMODE_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4220)
>> +#define CM_IDLEST_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4224)
>> +#define CM_AUTOIDLE_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4228)
>> +#define CM_CLKSEL_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x422c)
>> +#define CM_DIV_M2_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4230)
>> +#define CM_DIV_M4_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4238)
>> +#define CM_DIV_M5_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x423c)
>> +#define CM_DIV_M6_DPLL_DDRPHY		(OMAP44XX_L4_CORE_BASE + 0x4240)
>> +#define CM_SSC_DELTAMSTEP_DPLL_DDRPHY	(OMAP44XX_L4_CORE_BASE + 0x4248)
>> +#define CM_SHADOW_FREQ_CONFIG1		(OMAP44XX_L4_CORE_BASE + 0x4260)
>
> NAK.  We do not accept base address plus offset notation.  Please
> declare C structs instead.
>

Ok. will do.

Again just curious, what's the reasoning behind this policy? Is it just
aesthetics or something more?

best regards,
Aneesh