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

[Heiko Schocher]

Hello Lokesh,

Am 25.06.2013 10:17, schrieb Lokesh Vutla:
> Hi Heiko,
> On Tuesday 25 June 2013 12:35 PM, Heiko Schocher wrote:
>> 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 at 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 ...

It seems, we can make this static, no need for
doing this dynamically ... I try it ...

>>> 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!
> My idea here is to populate data according to the board.
> Its good if you use the same value.
>>
>>> 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?
> you can do an i2c_init() here.
> Thanks and regards,
> Lokesh

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