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

login
register
mail settings
Submitter Lokesh Vutla
Date June 24, 2013, 1:15 p.m.
Message ID <1372079722-19486-2-git-send-email-lokeshvutla@ti.com>
Download mbox | patch
Permalink /patch/253831/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Lokesh Vutla - June 24, 2013, 1:15 p.m.
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
Heiko Schocher - June 24, 2013, 7:12 p.m.
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 ...

[...]
> diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
> index 9c4d0b4..e878b25 100644
> --- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
> +++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
> @@ -26,56 +26,53 @@
>  #define PRCM_FORCE_WAKEUP	0x2
>  #define PRCM_FUNCTL		0x0
>  
> -#define PRCM_EMIF_CLK_ACTIVITY	BIT(2)
> -#define PRCM_L3_GCLK_ACTIVITY	BIT(4)
> -
> -#define PLL_BYPASS_MODE		0x4
> -#define ST_MN_BYPASS		0x00000100
> -#define ST_DPLL_CLK		0x00000001
> -#define CLK_SEL_MASK		0x7ffff
> -#define CLK_DIV_MASK		0x1f
> -#define CLK_DIV2_MASK		0x7f
> -#define CLK_SEL_SHIFT		0x8
> -#define CLK_MODE_SEL		0x7
> -#define CLK_MODE_MASK		0xfffffff8
> -#define CLK_DIV_SEL		0xFFFFFFE0
>  #define CPGMAC0_IDLE		0x30000
> -#define DPLL_CLKDCOLDO_GATE_CTRL        0x300
> -
>  #define OSC	(V_OSCK/1000000)

and could we move this define then to
arch/arm/include/asm/arch-am33xx/clock.h
too?

Thnaks!

bye,
Heiko
Lokesh Vutla - June 25, 2013, 3:48 a.m.
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.

Thanks and regards,
Lokesh
>
> [...]
>> diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
>> index 9c4d0b4..e878b25 100644
>> --- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
>> +++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
>> @@ -26,56 +26,53 @@
>>   #define PRCM_FORCE_WAKEUP	0x2
>>   #define PRCM_FUNCTL		0x0
>>
>> -#define PRCM_EMIF_CLK_ACTIVITY	BIT(2)
>> -#define PRCM_L3_GCLK_ACTIVITY	BIT(4)
>> -
>> -#define PLL_BYPASS_MODE		0x4
>> -#define ST_MN_BYPASS		0x00000100
>> -#define ST_DPLL_CLK		0x00000001
>> -#define CLK_SEL_MASK		0x7ffff
>> -#define CLK_DIV_MASK		0x1f
>> -#define CLK_DIV2_MASK		0x7f
>> -#define CLK_SEL_SHIFT		0x8
>> -#define CLK_MODE_SEL		0x7
>> -#define CLK_MODE_MASK		0xfffffff8
>> -#define CLK_DIV_SEL		0xFFFFFFE0
>>   #define CPGMAC0_IDLE		0x30000
>> -#define DPLL_CLKDCOLDO_GATE_CTRL        0x300
>> -
>>   #define OSC	(V_OSCK/1000000)
>
> and could we move this define then to
> arch/arm/include/asm/arch-am33xx/clock.h
> too?
>
> Thnaks!
>
> bye,
> Heiko
>
Heiko Schocher - June 25, 2013, 4:54 a.m.
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 ...

bye,
Heiko
Lokesh Vutla - June 25, 2013, 5:39 a.m.
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 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

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..:)

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

Thanks and regards,
Lokesh

>
> bye,
> Heiko
>
Heiko Schocher - June 25, 2013, 7:05 a.m.
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
Lokesh Vutla - June 25, 2013, 8:17 a.m.
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@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
>
Heiko Schocher - June 25, 2013, 2:09 p.m.
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@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
Tom Rini - June 25, 2013, 2:53 p.m.
On Tue, Jun 25, 2013 at 11:09:41AM +0530, Lokesh Vutla wrote:
> 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 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();
> }

Keep in mind the OPP50 advisory (errata 1.0.24) as well.  The first
fix/work-around for this I've seen drops us down to start with, and then
raises things up.
Heiko Schocher - June 26, 2013, 4:39 a.m.
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


Acked-by: Heiko Schocher <hs@denx.de>

Tested on 3 am335x boards (no need anymore to set mpu_pll twice
on this boards :-), so:

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
Heiko Schocher - June 26, 2013, 4:40 a.m.
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

Acked-by: Heiko Schocher <hs@denx.de>

Tested on 3 am335x boards so:

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko

Patch

diff --git a/arch/arm/cpu/armv7/am33xx/Makefile b/arch/arm/cpu/armv7/am33xx/Makefile
index c97e30d..f4ccd2a 100644
--- a/arch/arm/cpu/armv7/am33xx/Makefile
+++ b/arch/arm/cpu/armv7/am33xx/Makefile
@@ -18,6 +18,7 @@  LIB	= $(obj)lib$(SOC).o
 
 COBJS-$(CONFIG_AM33XX)	+= clock_am33xx.o
 COBJS-$(CONFIG_TI814X)	+= clock_ti814x.o
+COBJS-$(CONFIG_AM33XX)	+= clock.o
 COBJS	+= sys_info.o
 COBJS	+= mem.o
 COBJS	+= ddr.o
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 @@ 
+/*
+ * clock.c
+ *
+ * Clock initialization for AM33XX boards.
+ * Derived from OMAP4 boards
+ *
+ * Copyright (C) 2013, Texas Instruments, Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR /PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <common.h>
+#include <asm/arch/cpu.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/hardware.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/io.h>
+
+static void setup_post_dividers(const struct dpll_regs *dpll_regs,
+			 const struct dpll_params *params)
+{
+	/* Setup post-dividers */
+	if (params->m2 >= 0)
+		writel(params->m2, dpll_regs->cm_div_m2_dpll);
+	if (params->m3 >= 0)
+		writel(params->m3, dpll_regs->cm_div_m3_dpll);
+	if (params->m4 >= 0)
+		writel(params->m4, dpll_regs->cm_div_m4_dpll);
+	if (params->m5 >= 0)
+		writel(params->m5, dpll_regs->cm_div_m5_dpll);
+	if (params->m6 >= 0)
+		writel(params->m6, dpll_regs->cm_div_m6_dpll);
+}
+
+static inline void do_lock_dpll(const struct dpll_regs *dpll_regs)
+{
+	clrsetbits_le32(dpll_regs->cm_clkmode_dpll,
+			CM_CLKMODE_DPLL_DPLL_EN_MASK,
+			DPLL_EN_LOCK << CM_CLKMODE_DPLL_EN_SHIFT);
+}
+
+static inline void wait_for_lock(const struct dpll_regs *dpll_regs)
+{
+	if (!wait_on_value(ST_DPLL_CLK_MASK, ST_DPLL_CLK_MASK,
+			   (void *)dpll_regs->cm_idlest_dpll, LDELAY)) {
+		printf("DPLL locking failed for 0x%x\n",
+		       dpll_regs->cm_clkmode_dpll);
+		hang();
+	}
+}
+
+static inline void do_bypass_dpll(const struct dpll_regs *dpll_regs)
+{
+	clrsetbits_le32(dpll_regs->cm_clkmode_dpll,
+			CM_CLKMODE_DPLL_DPLL_EN_MASK,
+			DPLL_EN_MN_BYPASS << CM_CLKMODE_DPLL_EN_SHIFT);
+}
+
+static inline void wait_for_bypass(const struct dpll_regs *dpll_regs)
+{
+	if (!wait_on_value(ST_DPLL_CLK_MASK, 0,
+			   (void *)dpll_regs->cm_idlest_dpll, LDELAY)) {
+		printf("Bypassing DPLL failed 0x%x\n",
+		       dpll_regs->cm_clkmode_dpll);
+	}
+}
+
+static void bypass_dpll(const struct dpll_regs *dpll_regs)
+{
+	do_bypass_dpll(dpll_regs);
+	wait_for_bypass(dpll_regs);
+}
+
+static void do_setup_dpll(const struct dpll_regs *dpll_regs,
+			  const struct dpll_params *params)
+{
+	u32 temp;
+
+	if (!params)
+		return;
+
+	temp = readl(dpll_regs->cm_clksel_dpll);
+
+	bypass_dpll(dpll_regs);
+
+	/* Set M & N */
+	temp &= ~CM_CLKSEL_DPLL_M_MASK;
+	temp |= (params->m << CM_CLKSEL_DPLL_M_SHIFT) & CM_CLKSEL_DPLL_M_MASK;
+
+	temp &= ~CM_CLKSEL_DPLL_N_MASK;
+	temp |= (params->n << CM_CLKSEL_DPLL_N_SHIFT) & CM_CLKSEL_DPLL_N_MASK;
+
+	writel(temp, dpll_regs->cm_clksel_dpll);
+
+	setup_post_dividers(dpll_regs, params);
+
+	/* Wait till the DPLL locks */
+	do_lock_dpll(dpll_regs);
+	wait_for_lock(dpll_regs);
+}
+
+void setup_dplls(void)
+{
+	do_setup_dpll(&dpll_core_regs, &dpll_core);
+	do_setup_dpll(&dpll_mpu_regs, &dpll_mpu);
+	do_setup_dpll(&dpll_per_regs, &dpll_per);
+	writel(0x300, &cmwkup->clkdcoldodpllper);
+	do_setup_dpll(&dpll_ddr_regs, &dpll_ddr);
+}
diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
index 9c4d0b4..e878b25 100644
--- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
+++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c
@@ -26,56 +26,53 @@ 
 #define PRCM_FORCE_WAKEUP	0x2
 #define PRCM_FUNCTL		0x0
 
-#define PRCM_EMIF_CLK_ACTIVITY	BIT(2)
-#define PRCM_L3_GCLK_ACTIVITY	BIT(4)
-
-#define PLL_BYPASS_MODE		0x4
-#define ST_MN_BYPASS		0x00000100
-#define ST_DPLL_CLK		0x00000001
-#define CLK_SEL_MASK		0x7ffff
-#define CLK_DIV_MASK		0x1f
-#define CLK_DIV2_MASK		0x7f
-#define CLK_SEL_SHIFT		0x8
-#define CLK_MODE_SEL		0x7
-#define CLK_MODE_MASK		0xfffffff8
-#define CLK_DIV_SEL		0xFFFFFFE0
 #define CPGMAC0_IDLE		0x30000
-#define DPLL_CLKDCOLDO_GATE_CTRL        0x300
-
 #define OSC	(V_OSCK/1000000)
 
-#define MPUPLL_M	CONFIG_SYS_MPUCLK
-#define MPUPLL_N	(OSC-1)
-#define MPUPLL_M2	1
-
-/* Core PLL Fdll = 1 GHZ, */
-#define COREPLL_M	1000
-#define COREPLL_N	(OSC-1)
-
-#define COREPLL_M4	10	/* CORE_CLKOUTM4 = 200 MHZ */
-#define COREPLL_M5	8	/* CORE_CLKOUTM5 = 250 MHZ */
-#define COREPLL_M6	4	/* CORE_CLKOUTM6 = 500 MHZ */
-
-/*
- * USB PHY clock is 960 MHZ. Since, this comes directly from Fdll, Fdll
- * frequency needs to be set to 960 MHZ. Hence,
- * For clkout = 192 MHZ, Fdll = 960 MHZ, divider values are given below
- */
-#define PERPLL_M	960
-#define PERPLL_N	(OSC-1)
-#define PERPLL_M2	5
-
-/* DDR Freq is 266 MHZ for now */
-/* Set Fdll = 400 MHZ , Fdll = M * 2 * CLKINP/ N + 1; clkout = Fdll /(2 * M2) */
-#define DDRPLL_M	266
-#define DDRPLL_N	(OSC-1)
-#define DDRPLL_M2	1
-
 const struct cm_perpll *cmper = (struct cm_perpll *)CM_PER;
 const struct cm_wkuppll *cmwkup = (struct cm_wkuppll *)CM_WKUP;
 const struct cm_dpll *cmdpll = (struct cm_dpll *)CM_DPLL;
 const struct cm_rtc *cmrtc = (struct cm_rtc *)CM_RTC;
 
+const struct dpll_regs dpll_mpu_regs = {
+	.cm_clkmode_dpll	= CM_WKUP + 0x88,
+	.cm_idlest_dpll		= CM_WKUP + 0x20,
+	.cm_clksel_dpll		= CM_WKUP + 0x2C,
+	.cm_div_m2_dpll		= CM_WKUP + 0xA8,
+};
+
+const struct dpll_regs dpll_core_regs = {
+	.cm_clkmode_dpll	= CM_WKUP + 0x90,
+	.cm_idlest_dpll		= CM_WKUP + 0x5C,
+	.cm_clksel_dpll		= CM_WKUP + 0x68,
+	.cm_div_m4_dpll		= CM_WKUP + 0x80,
+	.cm_div_m5_dpll		= CM_WKUP + 0x84,
+	.cm_div_m6_dpll		= CM_WKUP + 0xD8,
+};
+
+const struct dpll_regs dpll_per_regs = {
+	.cm_clkmode_dpll	= CM_WKUP + 0x8C,
+	.cm_idlest_dpll		= CM_WKUP + 0x70,
+	.cm_clksel_dpll		= CM_WKUP + 0x9C,
+	.cm_div_m2_dpll		= CM_WKUP + 0xAC,
+};
+
+const struct dpll_regs dpll_ddr_regs = {
+	.cm_clkmode_dpll	= CM_WKUP + 0x94,
+	.cm_idlest_dpll		= CM_WKUP + 0x34,
+	.cm_clksel_dpll		= CM_WKUP + 0x40,
+	.cm_div_m2_dpll		= CM_WKUP + 0xA0,
+};
+
+const struct dpll_params dpll_mpu = {
+		CONFIG_SYS_MPUCLK, OSC-1, 1, -1, -1, -1, -1};
+const struct dpll_params dpll_core = {
+		1000, OSC-1, -1, -1, 10, 8, 4};
+const struct dpll_params dpll_per = {
+		960, OSC-1, 5, -1, -1, -1, -1};
+const struct dpll_params dpll_ddr = {
+		266, OSC-1, 1, -1, -1, -1, -1};
+
 static void enable_interface_clocks(void)
 {
 	/* Enable all the Interconnect Modules */
@@ -246,142 +243,6 @@  static void enable_per_clocks(void)
 		;
 }
 
-void mpu_pll_config_val(int mpull_m)
-{
-	u32 clkmode, clksel, div_m2;
-
-	clkmode = readl(&cmwkup->clkmoddpllmpu);
-	clksel = readl(&cmwkup->clkseldpllmpu);
-	div_m2 = readl(&cmwkup->divm2dpllmpu);
-
-	/* Set the PLL to bypass Mode */
-	writel(PLL_BYPASS_MODE, &cmwkup->clkmoddpllmpu);
-	while (readl(&cmwkup->idlestdpllmpu) != ST_MN_BYPASS)
-		;
-
-	clksel = clksel & (~CLK_SEL_MASK);
-	clksel = clksel | ((mpull_m << CLK_SEL_SHIFT) | MPUPLL_N);
-	writel(clksel, &cmwkup->clkseldpllmpu);
-
-	div_m2 = div_m2 & ~CLK_DIV_MASK;
-	div_m2 = div_m2 | MPUPLL_M2;
-	writel(div_m2, &cmwkup->divm2dpllmpu);
-
-	clkmode = clkmode | CLK_MODE_SEL;
-	writel(clkmode, &cmwkup->clkmoddpllmpu);
-
-	while (readl(&cmwkup->idlestdpllmpu) != ST_DPLL_CLK)
-		;
-}
-
-static void mpu_pll_config(void)
-{
-	mpu_pll_config_val(CONFIG_SYS_MPUCLK);
-}
-
-static void core_pll_config(void)
-{
-	u32 clkmode, clksel, div_m4, div_m5, div_m6;
-
-	clkmode = readl(&cmwkup->clkmoddpllcore);
-	clksel = readl(&cmwkup->clkseldpllcore);
-	div_m4 = readl(&cmwkup->divm4dpllcore);
-	div_m5 = readl(&cmwkup->divm5dpllcore);
-	div_m6 = readl(&cmwkup->divm6dpllcore);
-
-	/* Set the PLL to bypass Mode */
-	writel(PLL_BYPASS_MODE, &cmwkup->clkmoddpllcore);
-
-	while (readl(&cmwkup->idlestdpllcore) != ST_MN_BYPASS)
-		;
-
-	clksel = clksel & (~CLK_SEL_MASK);
-	clksel = clksel | ((COREPLL_M << CLK_SEL_SHIFT) | COREPLL_N);
-	writel(clksel, &cmwkup->clkseldpllcore);
-
-	div_m4 = div_m4 & ~CLK_DIV_MASK;
-	div_m4 = div_m4 | COREPLL_M4;
-	writel(div_m4, &cmwkup->divm4dpllcore);
-
-	div_m5 = div_m5 & ~CLK_DIV_MASK;
-	div_m5 = div_m5 | COREPLL_M5;
-	writel(div_m5, &cmwkup->divm5dpllcore);
-
-	div_m6 = div_m6 & ~CLK_DIV_MASK;
-	div_m6 = div_m6 | COREPLL_M6;
-	writel(div_m6, &cmwkup->divm6dpllcore);
-
-	clkmode = clkmode | CLK_MODE_SEL;
-	writel(clkmode, &cmwkup->clkmoddpllcore);
-
-	while (readl(&cmwkup->idlestdpllcore) != ST_DPLL_CLK)
-		;
-}
-
-static void per_pll_config(void)
-{
-	u32 clkmode, clksel, div_m2;
-
-	clkmode = readl(&cmwkup->clkmoddpllper);
-	clksel = readl(&cmwkup->clkseldpllper);
-	div_m2 = readl(&cmwkup->divm2dpllper);
-
-	/* Set the PLL to bypass Mode */
-	writel(PLL_BYPASS_MODE, &cmwkup->clkmoddpllper);
-
-	while (readl(&cmwkup->idlestdpllper) != ST_MN_BYPASS)
-		;
-
-	clksel = clksel & (~CLK_SEL_MASK);
-	clksel = clksel | ((PERPLL_M << CLK_SEL_SHIFT) | PERPLL_N);
-	writel(clksel, &cmwkup->clkseldpllper);
-
-	div_m2 = div_m2 & ~CLK_DIV2_MASK;
-	div_m2 = div_m2 | PERPLL_M2;
-	writel(div_m2, &cmwkup->divm2dpllper);
-
-	clkmode = clkmode | CLK_MODE_SEL;
-	writel(clkmode, &cmwkup->clkmoddpllper);
-
-	while (readl(&cmwkup->idlestdpllper) != ST_DPLL_CLK)
-		;
-
-	writel(DPLL_CLKDCOLDO_GATE_CTRL, &cmwkup->clkdcoldodpllper);
-}
-
-void ddr_pll_config(unsigned int ddrpll_m)
-{
-	u32 clkmode, clksel, div_m2;
-
-	clkmode = readl(&cmwkup->clkmoddpllddr);
-	clksel = readl(&cmwkup->clkseldpllddr);
-	div_m2 = readl(&cmwkup->divm2dpllddr);
-
-	/* Set the PLL to bypass Mode */
-	clkmode = (clkmode & CLK_MODE_MASK) | PLL_BYPASS_MODE;
-	writel(clkmode, &cmwkup->clkmoddpllddr);
-
-	/* Wait till bypass mode is enabled */
-	while ((readl(&cmwkup->idlestdpllddr) & ST_MN_BYPASS)
-				!= ST_MN_BYPASS)
-		;
-
-	clksel = clksel & (~CLK_SEL_MASK);
-	clksel = clksel | ((ddrpll_m << CLK_SEL_SHIFT) | DDRPLL_N);
-	writel(clksel, &cmwkup->clkseldpllddr);
-
-	div_m2 = div_m2 & CLK_DIV_SEL;
-	div_m2 = div_m2 | DDRPLL_M2;
-	writel(div_m2, &cmwkup->divm2dpllddr);
-
-	clkmode = (clkmode & CLK_MODE_MASK) | CLK_MODE_SEL;
-	writel(clkmode, &cmwkup->clkmoddpllddr);
-
-	/* Wait till dpll is locked */
-	while ((readl(&cmwkup->idlestdpllddr) & ST_DPLL_CLK) != ST_DPLL_CLK)
-		;
-}
-
 void enable_emif_clocks(void)
 {
 	/* Enable the  EMIF_FW Functional clock */
@@ -398,10 +259,7 @@  void enable_emif_clocks(void)
  */
 void pll_init()
 {
-	mpu_pll_config();
-	core_pll_config();
-	per_pll_config();
-
+	setup_dplls();
 	/* Enable the required interconnect clocks */
 	enable_interface_clocks();
 
diff --git a/arch/arm/cpu/armv7/am33xx/emif4.c b/arch/arm/cpu/armv7/am33xx/emif4.c
index aa84e96..38f1b4d 100644
--- a/arch/arm/cpu/armv7/am33xx/emif4.c
+++ b/arch/arm/cpu/armv7/am33xx/emif4.c
@@ -83,6 +83,10 @@  static void config_vtp(int nr)
 		;
 }
 
+void __weak ddr_pll_config(unsigned int ddrpll_m)
+{
+}
+
 void config_ddr(unsigned int pll, unsigned int ioctrl,
 		const struct ddr_data *data, const struct cmd_control *ctrl,
 		const struct emif_regs *regs, int nr)
diff --git a/arch/arm/include/asm/arch-am33xx/clock.h b/arch/arm/include/asm/arch-am33xx/clock.h
index ecb5901..b2a0a5b 100644
--- a/arch/arm/include/asm/arch-am33xx/clock.h
+++ b/arch/arm/include/asm/arch-am33xx/clock.h
@@ -21,4 +21,72 @@ 
 
 #include <asm/arch/clocks_am33xx.h>
 
+#define LDELAY 1000000
+
+/* CM_CLKMODE_DPLL */
+#define CM_CLKMODE_DPLL_REGM4XEN_SHIFT		11
+#define CM_CLKMODE_DPLL_REGM4XEN_MASK		(1 << 11)
+#define CM_CLKMODE_DPLL_LPMODE_EN_SHIFT		10
+#define CM_CLKMODE_DPLL_LPMODE_EN_MASK		(1 << 10)
+#define CM_CLKMODE_DPLL_RELOCK_RAMP_EN_SHIFT	9
+#define CM_CLKMODE_DPLL_RELOCK_RAMP_EN_MASK	(1 << 9)
+#define CM_CLKMODE_DPLL_DRIFTGUARD_EN_SHIFT	8
+#define CM_CLKMODE_DPLL_DRIFTGUARD_EN_MASK	(1 << 8)
+#define CM_CLKMODE_DPLL_RAMP_RATE_SHIFT		5
+#define CM_CLKMODE_DPLL_RAMP_RATE_MASK		(0x7 << 5)
+#define CM_CLKMODE_DPLL_EN_SHIFT		0
+#define CM_CLKMODE_DPLL_EN_MASK			(0x7 << 0)
+
+#define CM_CLKMODE_DPLL_DPLL_EN_SHIFT		0
+#define CM_CLKMODE_DPLL_DPLL_EN_MASK		7
+
+#define DPLL_EN_STOP			1
+#define DPLL_EN_MN_BYPASS		4
+#define DPLL_EN_LOW_POWER_BYPASS	5
+#define DPLL_EN_LOCK			7
+
+/* CM_IDLEST_DPLL fields */
+#define ST_DPLL_CLK_MASK		1
+
+/* CM_CLKSEL_DPLL */
+#define CM_CLKSEL_DPLL_M_SHIFT			8
+#define CM_CLKSEL_DPLL_M_MASK			(0x7FF << 8)
+#define CM_CLKSEL_DPLL_N_SHIFT			0
+#define CM_CLKSEL_DPLL_N_MASK			0x7F
+
+struct dpll_params {
+	u32 m;
+	u32 n;
+	s8 m2;
+	s8 m3;
+	s8 m4;
+	s8 m5;
+	s8 m6;
+};
+
+struct dpll_regs {
+	u32 cm_clkmode_dpll;
+	u32 cm_idlest_dpll;
+	u32 cm_autoidle_dpll;
+	u32 cm_clksel_dpll;
+	u32 cm_div_m2_dpll;
+	u32 cm_div_m3_dpll;
+	u32 cm_div_m4_dpll;
+	u32 cm_div_m5_dpll;
+	u32 cm_div_m6_dpll;
+};
+
+extern const struct dpll_regs dpll_mpu_regs;
+extern const struct dpll_regs dpll_core_regs;
+extern const struct dpll_regs dpll_per_regs;
+extern const struct dpll_regs dpll_ddr_regs;
+extern const struct dpll_params dpll_mpu;
+extern const struct dpll_params dpll_core;
+extern const struct dpll_params dpll_per;
+extern const struct dpll_params dpll_ddr;
+
+extern const struct cm_wkuppll *cmwkup;
+
+void setup_dplls(void);
+
 #endif
diff --git a/arch/arm/include/asm/arch-am33xx/ddr_defs.h b/arch/arm/include/asm/arch-am33xx/ddr_defs.h
index bb53a6a..c7048d1 100644
--- a/arch/arm/include/asm/arch-am33xx/ddr_defs.h
+++ b/arch/arm/include/asm/arch-am33xx/ddr_defs.h
@@ -154,6 +154,8 @@  void set_sdram_timings(const struct emif_regs *regs, int nr);
  */
 void config_ddr_phy(const struct emif_regs *regs, int nr);
 
+void ddr_pll_config(unsigned int ddrpll_m);
+
 struct ddr_cmd_regs {
 	unsigned int resv0[7];
 	unsigned int cm0csratio;	/* offset 0x01C */
diff --git a/arch/arm/include/asm/arch-am33xx/sys_proto.h b/arch/arm/include/asm/arch-am33xx/sys_proto.h
index 307ac28..04b8561 100644
--- a/arch/arm/include/asm/arch-am33xx/sys_proto.h
+++ b/arch/arm/include/asm/arch-am33xx/sys_proto.h
@@ -45,4 +45,5 @@  void omap_nand_switch_ecc(uint32_t, uint32_t);
 
 void rtc32k_enable(void);
 void uart_soft_reset(void);
+u32 wait_on_value(u32, u32, void *, u32);
 #endif