From: Heiko Stuebner <heiko@sntech.de> To: Elaine Zhang <zhangqing@rock-chips.com> Cc: mturquette@baylibre.com, sboyd@codeaurora.org, xf@rock-chips.com, robh+dt@kernel.org, mark.rutland@arm.com, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, huangtao@rock-chips.com, xxx@rock-chips.com, cl@rock-chips.com, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/4] clk: rockchip: add new pll-type for rk3328 Date: Sat, 24 Dec 2016 03:18:29 +0100 [thread overview] Message-ID: <1864774.VdbssDBMbY@phil> (raw) In-Reply-To: <1482112573-11613-5-git-send-email-zhangqing@rock-chips.com> 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 <zhangqing@rock-chips.com> > --- > 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 <zhangqing@rock-chips.com> Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- 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, }; -- 2.10.2 --------------- 8< -----------------
WARNING: multiple messages have this Message-ID (diff)
From: heiko@sntech.de (Heiko Stuebner) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 4/4] clk: rockchip: add new pll-type for rk3328 Date: Sat, 24 Dec 2016 03:18:29 +0100 [thread overview] Message-ID: <1864774.VdbssDBMbY@phil> (raw) In-Reply-To: <1482112573-11613-5-git-send-email-zhangqing@rock-chips.com> 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 <zhangqing@rock-chips.com> > --- > 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 <zhangqing@rock-chips.com> Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- 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, }; -- 2.10.2 --------------- 8< -----------------
next prev parent reply other threads:[~2016-12-24 2:18 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-19 1:56 [PATCH v2 0/4] clk: rockchip: support clk controller for rk3328 SoC Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-19 1:56 ` [PATCH v2 1/4] clk: rockchip: add dt-binding header for rk3328 Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-19 1:56 ` [PATCH v2 2/4] dt-bindings: add bindings for rk3328 clock controller Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-22 18:39 ` Rob Herring 2016-12-22 18:39 ` Rob Herring 2016-12-19 1:56 ` [PATCH v2 3/4] clk: rockchip: add clock controller for rk3328 Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-19 1:56 ` [PATCH v2 4/4] clk: rockchip: add new pll-type " Elaine Zhang 2016-12-19 1:56 ` Elaine Zhang 2016-12-24 2:18 ` Heiko Stuebner [this message] 2016-12-24 2:18 ` Heiko Stuebner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1864774.VdbssDBMbY@phil \ --to=heiko@sntech.de \ --cc=cl@rock-chips.com \ --cc=devicetree@vger.kernel.org \ --cc=huangtao@rock-chips.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=mark.rutland@arm.com \ --cc=mturquette@baylibre.com \ --cc=robh+dt@kernel.org \ --cc=sboyd@codeaurora.org \ --cc=xf@rock-chips.com \ --cc=xxx@rock-chips.com \ --cc=zhangqing@rock-chips.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.