From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lokesh Vutla Date: Tue, 25 Jun 2013 13:47:43 +0530 Subject: [U-Boot] [PATCH 1/4] ARM: AM33xx: Cleanup dplls data In-Reply-To: <51C9412B.2000203@denx.de> References: <1372079722-19486-1-git-send-email-lokeshvutla@ti.com> <1372079722-19486-2-git-send-email-lokeshvutla@ti.com> <51C89A2F.80303@denx.de> <51C91323.9050708@ti.com> <51C92294.7050501@denx.de> <51C92D1D.3070908@ti.com> <51C9412B.2000203@denx.de> Message-ID: <51C95227.5060307@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>> --- >>>>>> 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_ >> 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 >