Message-ID: <574F8CBA.2040203@rock-chips.com>
Date:	Thu, 2 Jun 2016 09:32:42 +0800
From:	hl <hl@...k-chips.com>
To:	Heiko Stübner <heiko@...ech.de>,
	Doug Anderson <dianders@...omium.org>
Cc:	David Airlie <airlied@...ux.ie>,
	Michael Turquette <mturquette@...libre.com>,
	Derek Basehore <dbasehore@...omium.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	myungjoo.ham@...sung.com, linux-clk <linux-clk@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	姚智情 <mark.yao@...k-chips.com>
Subject: Re: [RFC PATCH 2/4] clk: rockchip: rk3399: add ddrc clock support

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 <hl@...k-chips.com> wrote:
>>> add ddrc clock setting, so we can do ddr frequency
>>> scaling on rk3399 platform in future.
>>>
>>> Signed-off-by: Lin Huang <hl@...k-chips.com>
>>> ---
>>>
>>>   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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

-- 
Lin Huang

Powered by blists - more mailing lists