From patchwork Sat Dec 24 02:18:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Heiko_St=C3=BCbner?= X-Patchwork-Id: 9487791 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 005EC62AAE for ; Sat, 24 Dec 2016 02:22:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E2E3620008 for ; Sat, 24 Dec 2016 02:22:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D64B526212; Sat, 24 Dec 2016 02:22:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5579920008 for ; Sat, 24 Dec 2016 02:22:40 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1cKbvO-0008FT-G5; Sat, 24 Dec 2016 02:19:06 +0000 Received: from gloria.sntech.de ([95.129.55.99]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1cKbvJ-0008EJ-Ba; Sat, 24 Dec 2016 02:19:03 +0000 Received: from p57b77bec.dip0.t-ipconnect.de ([87.183.123.236] helo=phil.localnet) by gloria.sntech.de with esmtpsa (TLS1.1:DHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.80) (envelope-from ) id 1cKbuo-0008GR-1J; Sat, 24 Dec 2016 03:18:30 +0100 From: Heiko Stuebner To: Elaine Zhang Subject: Re: [PATCH v2 4/4] clk: rockchip: add new pll-type for rk3328 Date: Sat, 24 Dec 2016 03:18:29 +0100 Message-ID: <1864774.VdbssDBMbY@phil> User-Agent: KMail/5.2.3 (Linux/4.8.0-1-amd64; KDE/5.27.0; x86_64; ; ) In-Reply-To: <1482112573-11613-5-git-send-email-zhangqing@rock-chips.com> References: <1482112573-11613-1-git-send-email-zhangqing@rock-chips.com> <1482112573-11613-5-git-send-email-zhangqing@rock-chips.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20161223_181901_622172_56E9C06A X-CRM114-Status: GOOD ( 20.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, huangtao@rock-chips.com, xf@rock-chips.com, mturquette@baylibre.com, sboyd@codeaurora.org, xxx@rock-chips.com, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, robh+dt@kernel.org, cl@rock-chips.com, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Elaine, Am Montag, 19. Dezember 2016, 09:56:13 CET schrieb Elaine Zhang: > The rk3328's pll and clock are similar with rk3036's, > it different with pll_mode_mask,there are different > control registers bit, > so these should be independent and separate from > the series of rk3328s. not sure I understand this description. In the patch (and TRM excerpt) I see that the number of parents is down to only xin24m but the general handling is similar to the rk3036 and thus reuses its operations. The description makes it sound like there are operational differences (the "different control register bit" part), so could you clarify this a bit please? Also, please move the pll addition before the addition of the clock-controller in the series and move the pll-type addition also here from the controller patch. Some more below: > > Signed-off-by: Elaine Zhang > --- > drivers/clk/rockchip/clk-pll.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c > index 6ed605776abd..9650c75f61d1 100644 > --- a/drivers/clk/rockchip/clk-pll.c > +++ b/drivers/clk/rockchip/clk-pll.c > @@ -29,6 +29,7 @@ > #define PLL_MODE_SLOW 0x0 > #define PLL_MODE_NORM 0x1 > #define PLL_MODE_DEEP 0x2 > +#define PLL_RK3328_MODE_MASK 0x1 > > struct rockchip_clk_pll { > struct clk_hw hw; > @@ -865,13 +866,17 @@ struct clk *rockchip_clk_register_pll(struct > rockchip_clk_provider *ctx, pll_mux = &pll->pll_mux; > pll_mux->reg = ctx->reg_base + mode_offset; > pll_mux->shift = mode_shift; > - pll_mux->mask = PLL_MODE_MASK; > + if (pll_type == pll_rk3328) > + pll_mux->mask = PLL_RK3328_MODE_MASK; > + else > + pll_mux->mask = PLL_MODE_MASK; you're missing the other parts handling parents, like num_parents check and the init.num_parents parameter. The pll really has only one parent, xin24m, so we should handle this correctly in the code, instead of having 2 times xin24m in the parent array coming from the clock controller. > pll_mux->flags = 0; > pll_mux->lock = &ctx->lock; > pll_mux->hw.init = &init; > > if (pll_type == pll_rk3036 || > pll_type == pll_rk3066 || > + pll_type == pll_rk3328 || > pll_type == pll_rk3399) > pll_mux->flags |= CLK_MUX_HIWORD_MASK; > > @@ -929,6 +934,12 @@ struct clk *rockchip_clk_register_pll(struct > rockchip_clk_provider *ctx, else > init.ops = &rockchip_rk3066_pll_clk_ops; > break; > + case pll_rk3328: > + if (!pll->rate_table || IS_ERR(ctx->grf)) > + init.ops = &rockchip_rk3036_pll_clk_norate_ops; > + else > + init.ops = &rockchip_rk3036_pll_clk_ops; > + break; please don't duplicate the rk3036-ops assignment, when adding the pll_rk3328 option to the rk3036 part suffices. I'd think the pll-patch should look something like the following (untested, so please test): --------------- 8< ----------------- Subject: [PATCH] clk: rockchip: add new pll-type for rk3328 The rk3328's pll and clock are similar with rk3036's, it different with pll_mode_mask,there are different control registers bit, so these should be independent and separate from the series of rk3328s. Signed-off-by: Elaine Zhang Signed-off-by: Heiko Stuebner --- drivers/clk/rockchip/clk-pll.c | 18 ++++++++++++++---- drivers/clk/rockchip/clk.h | 1 + 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c index 6ed6057..99ce483 100644 --- a/drivers/clk/rockchip/clk-pll.c +++ b/drivers/clk/rockchip/clk-pll.c @@ -29,6 +29,7 @@ #define PLL_MODE_SLOW 0x0 #define PLL_MODE_NORM 0x1 #define PLL_MODE_DEEP 0x2 +#define PLL_RK3328_MODE_MASK 0x1 struct rockchip_clk_pll { struct clk_hw hw; @@ -848,8 +849,9 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx, struct clk *pll_clk, *mux_clk; char pll_name[20]; - if (num_parents != 2) { - pr_err("%s: needs two parent clocks\n", __func__); + if ((pll_type != pll_rk3328 && num_parents != 2) || + (pll_type == pll_rk3328 && num_parents != 1)) { + pr_err("%s: missing parent clocks\n", __func__); return ERR_PTR(-EINVAL); } @@ -865,13 +867,17 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx, pll_mux = &pll->pll_mux; pll_mux->reg = ctx->reg_base + mode_offset; pll_mux->shift = mode_shift; - pll_mux->mask = PLL_MODE_MASK; + if (pll_type == pll_rk3328) + pll_mux->mask = PLL_RK3328_MODE_MASK; + else + pll_mux->mask = PLL_MODE_MASK; pll_mux->flags = 0; pll_mux->lock = &ctx->lock; pll_mux->hw.init = &init; if (pll_type == pll_rk3036 || pll_type == pll_rk3066 || + pll_type == pll_rk3328 || pll_type == pll_rk3399) pll_mux->flags |= CLK_MUX_HIWORD_MASK; @@ -884,7 +890,10 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx, init.flags = CLK_SET_RATE_PARENT; init.ops = pll->pll_mux_ops; init.parent_names = pll_parents; - init.num_parents = ARRAY_SIZE(pll_parents); + if (pll_type == pll_rk3328) + init.num_parents = 2; + else + init.num_parents = ARRAY_SIZE(pll_parents); mux_clk = clk_register(NULL, &pll_mux->hw); if (IS_ERR(mux_clk)) @@ -918,6 +927,7 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx, switch (pll_type) { case pll_rk3036: + case pll_rk3328: if (!pll->rate_table || IS_ERR(ctx->grf)) init.ops = &rockchip_rk3036_pll_clk_norate_ops; else diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h index d67eecc..06acb7e 100644 --- a/drivers/clk/rockchip/clk.h +++ b/drivers/clk/rockchip/clk.h @@ -130,6 +130,7 @@ struct clk; enum rockchip_pll_type { pll_rk3036, pll_rk3066, + pll_rk3328, pll_rk3399, };