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

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