From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752007AbcFBBcx (ORCPT ); Wed, 1 Jun 2016 21:32:53 -0400 Received: from lucky1.263xmail.com ([211.157.147.133]:60467 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751568AbcFBBcv (ORCPT ); Wed, 1 Jun 2016 21:32:51 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: hl@rock-chips.com X-FST-TO: mark.yao@rock-chips.com X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: hl@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [RFC PATCH 2/4] clk: rockchip: rk3399: add ddrc clock support To: =?UTF-8?Q?Heiko_St=c3=bcbner?= , Doug Anderson References: <1464773739-18152-1-git-send-email-hl@rock-chips.com> <1464773739-18152-3-git-send-email-hl@rock-chips.com> <7336973.nHpy8GhqcO@diego> Cc: David Airlie , Michael Turquette , Derek Basehore , Stephen Boyd , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "open list:ARM/Rockchip SoC..." , Kyungmin Park , myungjoo.ham@samsung.com, linux-clk , "linux-arm-kernel@lists.infradead.org" , =?UTF-8?B?5aea5pm65oOF?= From: hl Message-ID: <574F8CBA.2040203@rock-chips.com> Date: Thu, 2 Jun 2016 09:32:42 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <7336973.nHpy8GhqcO@diego> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug&Heiko, On 2016年06月01日 23:46, Heiko Stübner wrote: > Am Mittwoch, 1. Juni 2016, 08:24:48 schrieb Doug Anderson: >> Lin Huang, >> >> On Wed, Jun 1, 2016 at 2:35 AM, Lin Huang wrote: >>> add ddrc clock setting, so we can do ddr frequency >>> scaling on rk3399 platform in future. >>> >>> Signed-off-by: Lin Huang >>> --- >>> >>> drivers/clk/rockchip/clk-rk3399.c | 16 ++++++++++++++++ >>> include/dt-bindings/clock/rk3399-cru.h | 1 + >>> 2 files changed, 17 insertions(+) >>> >>> diff --git a/drivers/clk/rockchip/clk-rk3399.c >>> b/drivers/clk/rockchip/clk-rk3399.c index f1d8e44..749ea59 100644 >>> --- a/drivers/clk/rockchip/clk-rk3399.c >>> +++ b/drivers/clk/rockchip/clk-rk3399.c >>> @@ -118,6 +118,10 @@ PNAME(mux_armclkb_p) = >>> { "clk_core_b_lpll_src",> >>> "clk_core_b_bpll_src", >>> "clk_core_b_dpll_src", >>> "clk_core_b_gpll_src" >>> }; >>> >>> +PNAME(mux_ddrclk_p) = { "clk_ddrc_lpll_src", >>> + "clk_ddrc_bpll_src", >>> + "clk_ddrc_dpll_src", >>> + "clk_ddrc_gpll_src" }; >>> >>> PNAME(mux_aclk_cci_p) = { "cpll_aclk_cci_src", >>> >>> "gpll_aclk_cci_src", >>> "npll_aclk_cci_src", >>> >>> @@ -1377,6 +1381,18 @@ static struct rockchip_clk_branch >>> rk3399_clk_branches[] __initdata = {> >>> COMPOSITE_NOMUX(0, "clk_test", "clk_test_pre", CLK_IGNORE_UNUSED, >>> >>> RK3368_CLKSEL_CON(58), 0, 5, DFLAGS, >>> RK3368_CLKGATE_CON(13), 11, GFLAGS), >>> >>> + >>> + /* ddrc */ >>> + GATE(0, "clk_ddrc_lpll_src", "lpll", CLK_IGNORE_UNUSED, >>> + RK3399_CLKGATE_CON(3), 0, GFLAGS), >>> + GATE(0, "clk_ddrc_bpll_src", "bpll", CLK_IGNORE_UNUSED, >>> + RK3399_CLKGATE_CON(3), 1, GFLAGS), >>> + GATE(0, "clk_ddrc_dpll_src", "dpll", CLK_IGNORE_UNUSED, >>> + RK3399_CLKGATE_CON(3), 2, GFLAGS), >>> + GATE(0, "clk_ddrc_gpll_src", "gpll", CLK_IGNORE_UNUSED, >>> + RK3399_CLKGATE_CON(3), 3, GFLAGS), >>> + COMPOSITE_DDRC(SCLK_DDRCLK, "clk_ddrc", mux_ddrclk_p, >>> CLK_IGNORE_UNUSED, + RK3399_CLKSEL_CON(6), 4, 2, >>> MFLAGS, 0, 3, DFLAGS), >> It seems slightly unfortunate that we need CLK_IGNORE_UNUSED on these. >> Only one of these will ever be used at once and it would be awfully >> nice if the others could get gated, right? >> >> ...presumably this is needed because we might not have an actual >> driver for DDR Freq and we definitely want to make sure that clk_ddrc >> is enabled in that case. I guess what we really want is something >> like CLK_ENABLE_HAND_OFF eventually, but until then I think you might >> get slightly better behavior by getting rid of all of these >> CLK_IGNORE_UNUSED and setting "clk_ddrc" as a critical clock, either >> using the table in this file or the new flag. > My current feeling is that staying with the homegrown solution for critical > clocks might be preferable until the clk-handoff mechanism lands as well, as > our critical clocks fall into both categories and I'd like to not touch > everything twice. > > The clock above should be controlled by the dcf, so falls into the handoff > category (critical until a driver picks up the clock). > > Also mixing both new and old approach might get confusing. > > But I'm definitly open to counter-arguments :-) I will remove CLK_IGNORE_UNUSED flag and set "clk_ddrc" as critical clock in next version. Besides, i think we should also set "clk_ddrc_dpll_src" as critical clock, since we always use dpll as ddr clock source, and it should always on. > >>> }; >>> >>> static struct rockchip_clk_branch rk3399_clk_pmu_branches[] __initdata = >>> { >>> >>> diff --git a/include/dt-bindings/clock/rk3399-cru.h >>> b/include/dt-bindings/clock/rk3399-cru.h index 50a44cf..8a0f0442 100644 >>> --- a/include/dt-bindings/clock/rk3399-cru.h >>> +++ b/include/dt-bindings/clock/rk3399-cru.h >>> @@ -131,6 +131,7 @@ >>> >>> #define SCLK_DPHY_RX0_CFG 165 >>> #define SCLK_RMII_SRC 166 >>> #define SCLK_PCIEPHY_REF100M 167 >>> >>> +#define SCLK_DDRCLK 168 >> Almost certainly you'll want to create a separate patch for the >> dt-bindings change since it will need to land in a different tree so >> it can be pulled into both Heiko's clock topic branch and dts64 topic >> branch. > correct. will fix next version, thanks. > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > > -- Lin Huang