[U-Boot] [PATCH 1/4] ARM: AM33xx: Cleanup dplls data

[Heiko Schocher <hs@denx.de>]

Hello Lokesh,

Am 25.06.2013 07:39, schrieb Lokesh Vutla:
> Hi Heiko,
> On Tuesday 25 June 2013 10:24 AM, Heiko Schocher wrote:
>> Hello Lokesh,
>>
>> Am 25.06.2013 05:48, schrieb Lokesh Vutla:
>>> Hi Heiko,
>>> On Tuesday 25 June 2013 12:42 AM, Heiko Schocher wrote:
>>>> Hello Lokesh,
>>>>
>>>> Am 24.06.2013 15:15, schrieb Lokesh Vutla:
>>>>> Locking sequence for all the dplls is same.
>>>>> In the current code same sequence is done repeatedly
>>>>> for each dpll. Instead have a generic function
>>>>> for locking dplls and pass dpll data to that function.
>>>>>
>>>>> This is derived from OMAP4 boards.
>>>>>
>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>>> ---
>>>>>    arch/arm/cpu/armv7/am33xx/Makefile           |    1 +
>>>>>    arch/arm/cpu/armv7/am33xx/clock.c            |  116 ++++++++++++++
>>>>>    arch/arm/cpu/armv7/am33xx/clock_am33xx.c     |  222 +++++---------------------
>>>>>    arch/arm/cpu/armv7/am33xx/emif4.c            |    4 +
>>>>>    arch/arm/include/asm/arch-am33xx/clock.h     |   68 ++++++++
>>>>>    arch/arm/include/asm/arch-am33xx/ddr_defs.h  |    2 +
>>>>>    arch/arm/include/asm/arch-am33xx/sys_proto.h |    1 +
>>>>>    7 files changed, 232 insertions(+), 182 deletions(-)
>>>>>    create mode 100644 arch/arm/cpu/armv7/am33xx/clock.c
>>>>>
>>>> [...]
>>>>> diff --git a/arch/arm/cpu/armv7/am33xx/clock.c b/arch/arm/cpu/armv7/am33xx/clock.c
>>>>> new file mode 100644
>>>>> index 0000000..a7f1d83
>>>>> --- /dev/null
>>>>> +++ b/arch/arm/cpu/armv7/am33xx/clock.c
>>>>> @@ -0,0 +1,116 @@
>>>> [...]
>>>>> +static void do_setup_dpll(const struct dpll_regs *dpll_regs,
>>>>> +			  const struct dpll_params *params)
>>>>> +{
>>>>
>>>> Could we have this function not only static? I posted a patch:
>>>>
>>>> [U-Boot] arm, am335x: make mpu pll config configurable
>>>> http://patchwork.ozlabs.org/patch/248509/
>>>>
>>>> which uses mpu_pll_config() for switching mpu pll clock
>>>> from board code ... you delete this function later in this patch,
>>>> so I think, I can switch to do_setup_pll() ... if this is not
>>>> static code ...
>>> Yes I saw that patch. No need to make this non-static.
>>> Please have your own struct "const struct dpll_params dpll_mpu"
>>> and update your values accordingly.
>>
>> Hmm.. maybe I miss something here. You call setup_dplls()
>> in arch/arm/cpu/armv7/am33xx/clock.c using &dpll_mpu defined
>> in arch/arm/cpu/armv7/am33xx/clock_am33xx.c ... so how to
>> make here a board specific struct?
>>
>> The MPUCLK is configurable through the define CONFIG_SYS_MPUCLK
>> which is good, but I have on this board a PMIC, which in board SPL
>> code change MPU and core voltage ... and after that I change
>> the MPU clock again ...
> Ohk.
> Can't we scale the voltages before calling setup_dplls()
> (Why do you want to configure the MPU clocks twice?

I speak with the customer ...

> I don't know much about your board, so I am just asking..:) )
> What I meant is something like below:
> void __weak scale_vcores(void)
> {}
> 
> void prcm_init()
> {
> 	enable_basic_clocks();
> 	scale_vcores();
> 	setup_dplls();
> }
> 
> have your own scale_vcores in your board file.
> and for dpll_mpu have something like this:
> #ifdef CONFIG_<BOARD>
> const struct dpll_params dpll_mpu = {
> 		M, N, 1, -1, -1, -1, -1};
> #else
> const struct dpll_params dpll_mpu = {
> 		CONFIG_SYS_MPUCLK, OSC-1, 1, -1, -1, -1, -1};
> #endif

No, that is not good. We should prevent such board specific
defines in common code. I think this define is not necessary,
as, if we have a scale_vcore() function, I can set
CONFIG_SYS_MPUCLK to the end value ! I try this out! Thanks!

> I hope this should be possible on your board.
> I am telling this because it will be easy for me during my next cleanup 
> during
> which I planned to combine omap-common and am33xx code..:)

Ok, i try it ...

> This is the exactly what is done for omap( program voltages and then 
> setup dplls)
> You can refer to arch/arm/cpu/armv7/omap-common/clocks-common.c
> prcm_init() function.
> 
> Please correct me if I am wrong..

Yes, that looks good. Hmm... have we access to an pmic connected
over i2c at this time?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany