From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xing Zheng Subject: Re: [PATCH v3 4/7] clk: rockchip: Add support for multiple clock providers Date: Thu, 10 Mar 2016 10:29:50 +0800 Message-ID: <56E0DC1E.9090004@rock-chips.com> References: <1457491027-30936-1-git-send-email-zhengxing@rock-chips.com> <1457491027-30936-5-git-send-email-zhengxing@rock-chips.com> <1978482.7WNnERYOPX@diego> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1978482.7WNnERYOPX@diego> Sender: linux-clk-owner@vger.kernel.org To: =?UTF-8?Q?Heiko_St=c3=bcbner?= Cc: linux-rockchip@lists.infradead.org, huangtao@rock-chips.com, jay.xu@rock-chips.com, elaine.zhang@rock-chips.com, dianders@chromium.org, Michael Turquette , Stephen Boyd , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org List-Id: linux-rockchip.vger.kernel.org Hi Heiko, Thank you for helping me to optimize these details. :-) On 2016=E5=B9=B403=E6=9C=8810=E6=97=A5 06:25, Heiko St=C3=BCbner wrote: > Hi Xing, > > Am Mittwoch, 9. M=C3=A4rz 2016, 10:37:04 schrieb Xing Zheng: >> There are need to support Multi-CRUs probability in future, but >> it is not supported on the current Rockchip Clock Framework. >> >> Therefore, this patch add support a provider as the parameter >> handler when we call the clock register functions for per CRU. >> >> Signed-off-by: Xing Zheng > I've applied that in a clk branch for 4.7 [0] with some changes detai= led > below. If you can, please check that I didn't mess anything up :-) > > I've sucessfully booted that on both a rk3036 and rk3288 as well. > > > Heiko > > [0] https://git.kernel.org/cgit/linux/kernel/git/mmind/linux-rockchip= =2Egit/commit/?h=3Dv4.7-clk/next&id=3Dd509ddf2e57c99ae760d1a289b85f1e0d= 729f864 > > >> --- >> >> Changes in v3: None >> Changes in v2: None >> >> drivers/clk/rockchip/clk-pll.c | 30 ++++---- >> drivers/clk/rockchip/clk-rk3036.c | 17 +++-- >> drivers/clk/rockchip/clk-rk3188.c | 48 ++++++++---- >> drivers/clk/rockchip/clk-rk3228.c | 17 +++-- >> drivers/clk/rockchip/clk-rk3288.c | 19 +++-- >> drivers/clk/rockchip/clk-rk3368.c | 21 ++++-- >> drivers/clk/rockchip/clk.c | 148 >> +++++++++++++++++++++++-------------- drivers/clk/rockchip/clk.h = | >> 49 ++++++++---- >> 8 files changed, 231 insertions(+), 118 deletions(-) > [...] > >> diff --git a/drivers/clk/rockchip/clk-rk3188.c >> b/drivers/clk/rockchip/clk-rk3188.c index e832403..7c73c51 100644 >> --- a/drivers/clk/rockchip/clk-rk3188.c >> @@ -759,57 +759,78 @@ static const char *const rk3188_critical_clock= s[] >> __initconst =3D { "hclk_cpubus" >> }; >> >> -static void __init rk3188_common_clk_init(struct device_node *np) >> +static struct rockchip_clk_provider *__init rk3188_common_clk_init(= struct >> device_node *np) { >> + struct rockchip_clk_provider *ctx; >> void __iomem *reg_base; >> >> reg_base =3D of_iomap(np, 0); >> if (!reg_base) { >> pr_err("%s: could not map cru region\n", __func__); >> - return; >> + return ERR_PTR(-ENOMEM); >> } >> >> - rockchip_clk_init(np, reg_base, CLK_NR_CLKS); >> + ctx =3D rockchip_clk_init(np, reg_base, CLK_NR_CLKS); >> + if (IS_ERR(ctx)) { >> + pr_err("%s: rockchip clk init failed\n", __func__); >> + return ERR_PTR(-ENOMEM); >> + } >> >> - rockchip_clk_register_branches(common_clk_branches, >> + rockchip_clk_register_branches(ctx, common_clk_branches, >> ARRAY_SIZE(common_clk_branches)); >> >> rockchip_register_softrst(np, 9, reg_base + RK2928_SOFTRST_CON(0)= , >> ROCKCHIP_SOFTRST_HIWORD_MASK); >> >> - rockchip_register_restart_notifier(RK2928_GLB_SRST_FST, NULL); >> + rockchip_register_restart_notifier(ctx, RK2928_GLB_SRST_FST, NULL)= ; >> + >> + return ctx; >> } >> >> static void __init rk3066a_clk_init(struct device_node *np) >> { >> - rk3188_common_clk_init(np); >> - rockchip_clk_register_plls(rk3066_pll_clks, >> + struct rockchip_clk_provider *ctx; >> + >> + ctx =3D rk3188_common_clk_init(np); >> + if (IS_ERR(ctx)) { >> + pr_err("%s: common clk init failed\n", __func__); >> + return; >> + } > I've dropped the pr_err + parentheses, as rk3188_common_clk_init > will already output a suitable error. > > >> + >> + rockchip_clk_register_plls(ctx, rk3066_pll_clks, >> ARRAY_SIZE(rk3066_pll_clks), >> RK3066_GRF_SOC_STATUS); >> - rockchip_clk_register_branches(rk3066a_clk_branches, >> + rockchip_clk_register_branches(ctx, rk3066a_clk_branches, >> ARRAY_SIZE(rk3066a_clk_branches)); >> - rockchip_clk_register_armclk(ARMCLK, "armclk", >> + rockchip_clk_register_armclk(ctx, ARMCLK, "armclk", >> mux_armclk_p, ARRAY_SIZE(mux_armclk_p), >> &rk3066_cpuclk_data, rk3066_cpuclk_rates, >> ARRAY_SIZE(rk3066_cpuclk_rates)); >> rockchip_clk_protect_critical(rk3188_critical_clocks, >> ARRAY_SIZE(rk3188_critical_clocks)); >> + rockchip_clk_of_add_provider(np, ctx); >> } >> CLK_OF_DECLARE(rk3066a_cru, "rockchip,rk3066a-cru", rk3066a_clk_in= it); >> >> static void __init rk3188a_clk_init(struct device_node *np) >> { >> + struct rockchip_clk_provider *ctx; >> struct clk *clk1, *clk2; >> unsigned long rate; >> int ret; >> >> - rk3188_common_clk_init(np); >> - rockchip_clk_register_plls(rk3188_pll_clks, >> + ctx =3D rk3188_common_clk_init(np); >> + if (IS_ERR(ctx)) { >> + pr_err("%s: common clk init failed\n", __func__); >> + return; >> + } > same as above > > >> + >> + rockchip_clk_register_plls(ctx, rk3188_pll_clks, >> ARRAY_SIZE(rk3188_pll_clks), >> RK3188_GRF_SOC_STATUS); >> - rockchip_clk_register_branches(rk3188_clk_branches, >> + rockchip_clk_register_branches(ctx, rk3188_clk_branches, >> ARRAY_SIZE(rk3188_clk_branches)); >> - rockchip_clk_register_armclk(ARMCLK, "armclk", >> + rockchip_clk_register_armclk(ctx, ARMCLK, "armclk", >> mux_armclk_p, ARRAY_SIZE(mux_armclk_p), >> &rk3188_cpuclk_data, rk3188_cpuclk_rates, >> ARRAY_SIZE(rk3188_cpuclk_rates)); >> @@ -833,6 +854,7 @@ static void __init rk3188a_clk_init(struct devic= e_node >> *np) >> >> rockchip_clk_protect_critical(rk3188_critical_clocks, >> ARRAY_SIZE(rk3188_critical_clocks)); >> + rockchip_clk_of_add_provider(np, ctx); >> } >> CLK_OF_DECLARE(rk3188a_cru, "rockchip,rk3188a-cru", rk3188a_clk_in= it); > [...] > >> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c >> index ab50524..54e6b74 100644 >> --- a/drivers/clk/rockchip/clk.c >> +++ b/drivers/clk/rockchip/clk.c >> @@ -312,66 +316,94 @@ static struct clk >> *rockchip_clk_register_factor_branch(const char *name, return clk; >> } >> >> -static DEFINE_SPINLOCK(clk_lock); >> -static struct clk **clk_table; >> -static void __iomem *reg_base; >> -static struct clk_onecell_data clk_data; >> -static struct device_node *cru_node; >> -static struct regmap *grf; >> - >> -void __init rockchip_clk_init(struct device_node *np, void __iomem = *base, >> - unsigned long nr_clks) >> +struct rockchip_clk_provider *__init rockchip_clk_init(struct devic= e_node > I've added a space between the asterisk and __init flag > > >> *np, + void __iomem *base, unsigned long nr_clks) >> { >> - reg_base =3D base; >> - cru_node =3D np; >> - grf =3D ERR_PTR(-EPROBE_DEFER); >> + struct rockchip_clk_provider *ctx; >> + struct clk **clk_table; >> + int i; >> + >> + ctx =3D kzalloc(sizeof(struct rockchip_clk_provider), GFP_KERNEL); >> + if (!ctx) { >> + pr_err("%s: Could not allocate clock provider context\n", >> + __func__); >> + return ERR_PTR(-ENOMEM); >> + } >> >> clk_table =3D kcalloc(nr_clks, sizeof(struct clk *), GFP_KERNEL); >> - if (!clk_table) >> - pr_err("%s: could not allocate clock lookup table\n", __func__); >> + if (!clk_table) { >> + pr_err("%s: Could not allocate clock lookup table\n", >> + __func__); >> + goto err_free; >> + } >> + >> + for (i =3D 0; i < nr_clks; ++i) >> + clk_table[i] =3D ERR_PTR(-ENOENT); >> >> - clk_data.clks =3D clk_table; >> - clk_data.clk_num =3D nr_clks; >> - of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data); >> + ctx->reg_base =3D base; >> + ctx->clk_data.clks =3D clk_table; >> + ctx->clk_data.clk_num =3D nr_clks; >> + ctx->cru_node =3D np; >> + ctx->grf =3D ERR_PTR(-EPROBE_DEFER); >> + spin_lock_init(&ctx->lock); >> + >> + return ctx; >> + >> +err_free: >> + kfree(ctx); >> + return ERR_PTR(-ENOMEM); >> +} >> + >> +void __init rockchip_clk_of_add_provider(struct device_node *np, >> + struct rockchip_clk_provider *ctx) >> +{ >> + if (np) { >> + if (of_clk_add_provider(np, of_clk_src_onecell_get, >> + &ctx->clk_data)) >> + panic("could not register clk provider\n"); > I've changed that to a pr_err, again no need to panic on this, as let= ting > the kernel run may give the affected developer more hints what may be= wrong. > > >> + } >> } >> >> diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h >> index 7aafe18..b7affb6 100644 >> --- a/drivers/clk/rockchip/clk.h >> +++ b/drivers/clk/rockchip/clk.h >> @@ -127,6 +128,20 @@ enum rockchip_pll_type { >> .nb =3D _nb, \ >> } >> >> +/** >> + * struct rockchip_clk_provider: information about clock provider >> + * @reg_base: virtual address for the register base. >> + * @clk_data: holds clock related data like clk* and number of cloc= ks. >> + * @lock: maintains exclusion between callbacks for a given clock-p= rovider. > I've added the missing kerneldoc entries here > > >> + */ >> +struct rockchip_clk_provider { >> + void __iomem *reg_base; >> + struct clk_onecell_data clk_data; >> + struct device_node *cru_node; >> + struct regmap *grf; >> + spinlock_t lock; >> +}; >> + >> struct rockchip_pll_rate_table { >> unsigned long rate; >> unsigned int nr; > > > --=20 - Xing Zheng