Re: [PATCH v2] clk: rockchip: Add support for multiple clock providers
From: Heiko Stuebner
Date: Wed Jan 20 2016 - 19:06:18 EST
Hi,
Am Mittwoch, 20. Januar 2016, 17:06:49 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 <zhengxing@xxxxxxxxxxxxxx>
overall this looks really nice. Thanks for following up on our talk so
quickly :-) .
> ---
>
> Changes in v2:
> - Fix missed to call rockchip_clk_common_cru_init when SoCs clock init.
>
> drivers/clk/rockchip/clk-rk3036.c | 15 +++--
> drivers/clk/rockchip/clk-rk3188.c | 40 ++++++++-----
> drivers/clk/rockchip/clk-rk3228.c | 15 +++--
> drivers/clk/rockchip/clk-rk3288.c | 17 ++++--
> drivers/clk/rockchip/clk-rk3368.c | 19 +++---
> drivers/clk/rockchip/clk.c | 120
> +++++++++++++++++++++++-------------- drivers/clk/rockchip/clk.h |
> 35 ++++++++---
> 7 files changed, 170 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk-rk3036.c
> b/drivers/clk/rockchip/clk-rk3036.c index 483913b..050ad13 100644
> --- a/drivers/clk/rockchip/clk-rk3036.c
> +++ b/drivers/clk/rockchip/clk-rk3036.c
> @@ -434,6 +434,7 @@ static const char *const rk3036_critical_clocks[]
> __initconst = {
>
> static void __init rk3036_clk_init(struct device_node *np)
> {
> + struct rockchip_clk_provider *ctx;
> void __iomem *reg_base;
> struct clk *clk;
>
> @@ -443,7 +444,9 @@ static void __init rk3036_clk_init(struct device_node
> *np) return;
> }
>
> - rockchip_clk_init(np, reg_base, CLK_NR_CLKS);
> + ctx = rockchip_clk_init(reg_base, CLK_NR_CLKS);
> +
> + rockchip_clk_common_cru_init(np);
>
> /* xin12m is created by an cru-internal divider */
> clk = clk_register_fixed_factor(NULL, "xin12m", "xin24m", 0, 1, 2);
> @@ -473,15 +476,15 @@ static void __init rk3036_clk_init(struct
> device_node *np) pr_warn("%s: could not register clock sclk_macref_out:
> %ld\n", __func__, PTR_ERR(clk));
>
> - rockchip_clk_register_plls(rk3036_pll_clks,
> + rockchip_clk_register_plls(ctx, rk3036_pll_clks,
> ARRAY_SIZE(rk3036_pll_clks),
> RK3036_GRF_SOC_STATUS0);
> - rockchip_clk_register_branches(rk3036_clk_branches,
> + rockchip_clk_register_branches(ctx, rk3036_clk_branches,
> ARRAY_SIZE(rk3036_clk_branches));
> rockchip_clk_protect_critical(rk3036_critical_clocks,
> ARRAY_SIZE(rk3036_critical_clocks));
>
> - rockchip_clk_register_armclk(ARMCLK, "armclk",
> + rockchip_clk_register_armclk(ctx, ARMCLK, "armclk",
> mux_armclk_p, ARRAY_SIZE(mux_armclk_p),
> &rk3036_cpuclk_data, rk3036_cpuclk_rates,
> ARRAY_SIZE(rk3036_cpuclk_rates));
> @@ -489,6 +492,8 @@ static void __init rk3036_clk_init(struct device_node
> *np) 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);
> +
> + rockchip_clk_of_add_provider(np, ctx);
> }
> CLK_OF_DECLARE(rk3036_cru, "rockchip,rk3036-cru", rk3036_clk_init);
> diff --git a/drivers/clk/rockchip/clk-rk3188.c
> b/drivers/clk/rockchip/clk-rk3188.c index 7f7444c..338a22e 100644
> --- a/drivers/clk/rockchip/clk-rk3188.c
> +++ b/drivers/clk/rockchip/clk-rk3188.c
> @@ -750,18 +750,19 @@ static const char *const rk3188_critical_clocks[]
> __initconst = { "pclk_peri",
> };
>
> -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;
> struct clk *clk;
>
> reg_base = of_iomap(np, 0);
> - if (!reg_base) {
> - pr_err("%s: could not map cru region\n", __func__);
> - return;
> - }
> + if (!reg_base)
> + panic("%s: could not map cru region\n", __func__);
I don't believe in doing panics everywhere :-), please do
return ERR_PTR(-ENOMEM);
here and in similar locations and let the top-most caller handle errors.
> @@ -260,27 +261,49 @@ static struct clk
> *rockchip_clk_register_frac_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;
please also include the devicetree node into the ctx struct. That way we can
keep track of all of them (when there are multiple).
> 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(
> + void __iomem *base, unsigned long nr_clks)
> {
> - reg_base = base;
> - cru_node = np;
> - grf = ERR_PTR(-EPROBE_DEFER);
> + struct rockchip_clk_provider *ctx;
> + struct clk **clk_table;
> + int i;
> +
> + ctx = kzalloc(sizeof(struct rockchip_clk_provider), GFP_KERNEL);
> + if (!ctx)
> + panic("could not allocate clock provider context.\n");
as I wrote above, please return a real error here. Especially when there are
multiple clock controllers, it might actually get a bit farther in cases
when only one of them fails, which might help in debugging underlying issues
in such cases.
>
> clk_table = kcalloc(nr_clks, sizeof(struct clk *), GFP_KERNEL);
> if (!clk_table)
> - pr_err("%s: could not allocate clock lookup table\n", __func__);
> + panic("could not allocate clock lookup table\n");
same here (and kfree the ctx from above before returning)
> +
> + for (i = 0; i < nr_clks; ++i)
> + clk_table[i] = ERR_PTR(-ENOENT);
> +
> + ctx->reg_base = base;
> + ctx->clk_data.clks = clk_table;
> + ctx->clk_data.clk_num = nr_clks;
> + spin_lock_init(&ctx->lock);
> +
> + return ctx;
> +}
>
> - clk_data.clks = clk_table;
> - clk_data.clk_num = nr_clks;
> - of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +void __init rockchip_clk_common_cru_init(struct device_node *np)
> +{
> + cru_node = np;
> + grf = ERR_PTR(-EPROBE_DEFER);
I'm not sure why this is a separate function. Setting the node pointer (into
the new struct as written above) can still happen in rockchip_clk_init, and
initializing the global grf pointer as well.
Heiko