[PATCH v3 6/8] clk: rockchip: add new clock-type for the cpuclk
Doug Anderson
dianders at chromium.org
Mon Sep 22 12:33:38 PDT 2014
Heiko,
On Mon, Sep 22, 2014 at 12:21 PM, Heiko Stübner <heiko at sntech.de> wrote:
> Am Montag, 22. September 2014, 10:47:25 schrieb Doug Anderson:
>> Heiko,
>>
>> On Wed, Sep 17, 2014 at 3:12 PM, Heiko Stuebner <heiko at sntech.de> wrote:
>> > When changing the armclk on Rockchip SoCs it is supposed to be reparented
>> > to an alternate parent before changing the underlying pll and back after
>> > the change. Additionally there exist clocks that are very tightly bound to
>> > the armclk whose divider values are set according to the armclk rate.
>> >
>> > Add a special clock-type to handle all that. The rate table and divider
>> > values will be supplied from the soc-specific clock controllers.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>> > ---
>> >
>> > drivers/clk/rockchip/Makefile | 1 +
>> > drivers/clk/rockchip/clk-cpu.c | 330
>> > +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c
>> > | 20 +++
>> > drivers/clk/rockchip/clk.h | 36 +++++
>> > 4 files changed, 387 insertions(+)
>> > create mode 100644 drivers/clk/rockchip/clk-cpu.c
>> >
>> > diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
>> > index ee6b077..bd8514d 100644
>> > --- a/drivers/clk/rockchip/Makefile
>> > +++ b/drivers/clk/rockchip/Makefile
>> > @@ -5,6 +5,7 @@
>> >
>> > obj-y += clk-rockchip.o
>> > obj-y += clk.o
>> > obj-y += clk-pll.o
>> >
>> > +obj-y += clk-cpu.o
>> >
>> > obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
>> >
>> > obj-y += clk-rk3188.o
>> >
>> > diff --git a/drivers/clk/rockchip/clk-cpu.c
>> > b/drivers/clk/rockchip/clk-cpu.c new file mode 100644
>> > index 0000000..8f0aaeb
>> > --- /dev/null
>> > +++ b/drivers/clk/rockchip/clk-cpu.c
>> > @@ -0,0 +1,330 @@
>> > +/*
>> > + * Copyright (c) 2014 MundoReader S.L.
>> > + * Author: Heiko Stuebner <heiko at sntech.de>
>> > + *
>> > + * based on clk/samsung/clk-cpu.c
>> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> > + * Author: Thomas Abraham <thomas.ab at samsung.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + *
>> > + * This file contains the utility function to register CPU clock for
>> > Samsung
>> I think on the last review someone pointed out that this part of the
>> comment is wrong. Perhaps you could remove it or fix it?
>> Specifically, this is not for Samsung Exynos...
>>
>> > + * Exynos platforms. A CPU clock is defined as a clock supplied to a CPU
>> > or a + * group of CPUs. The CPU clock is typically derived from a
>> > hierarchy of clock + * blocks which includes mux and divider blocks.
>> > There are a number of other + * auxiliary clocks supplied to the CPU
>> > domain such as the debug blocks and AXI + * clock for CPU domain. The
>> > rates of these auxiliary clocks are related to the + * CPU clock rate and
>> > this relation is usually specified in the hardware manual + * of the SoC
>> > or supplied after the SoC characterization.
>> > + *
>> > + * The below implementation of the CPU clock allows the rate changes of
>> > the CPU + * clock and the corresponding rate changes of the auxillary
>> > clocks of the CPU + * domain. The platform clock driver provides a clock
>> > register configuration + * for each configurable rate which is then used
>> > to program the clock hardware + * registers to acheive a fast
>> > co-oridinated rate change for all the CPU domain + * clocks.
>> > + *
>> > + * On a rate change request for the CPU clock, the rate change is
>> > propagated + * upto the PLL supplying the clock to the CPU domain clock
>> > blocks. While the + * CPU domain PLL is reconfigured, the CPU domain
>> > clocks are driven using an + * alternate clock source. If required, the
>> > alternate clock source is divided + * down in order to keep the output
>> > clock rate within the previous OPP limits. + */
>> > +
>> > +#include <linux/of.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/io.h>
>> > +#include <linux/clk-provider.h>
>> > +#include "clk.h"
>> > +
>> > +/**
>> > + * struct rockchip_cpuclk: information about clock supplied to a CPU
>> > core.
>> > + * @hw: handle between ccf and cpu clock.
>> > + * @alt_parent: alternate parent clock to use when switching the
>> > speed + * of the primary parent clock.
>> > + * @reg_base: base register for cpu-clock values.
>> > + * @clk_nb: clock notifier registered for changes in clock speed of
>> > the
>> > + * primary parent clock.
>> > + * @rate_count: number of rates in the rate_table
>> > + * @rate_table: pll-rates and their associated dividers
>> > + * @reg_data: cpu-specific register settings
>> > + * @lock: clock lock
>> > + */
>> > +struct rockchip_cpuclk {
>> > + struct clk_hw hw;
>> > +
>> > + struct clk_mux cpu_mux;
>> > + const struct clk_ops *cpu_mux_ops;
>> > +
>> > + struct clk *alt_parent;
>> > + void __iomem *reg_base;
>> > + struct notifier_block clk_nb;
>> > + unsigned int rate_count;
>> > + struct rockchip_cpuclk_rate_table *rate_table;
>> > + const struct rockchip_cpuclk_reg_data *reg_data;
>> > + spinlock_t *lock;
>> > +};
>> > +
>> > +#define to_rockchip_cpuclk_hw(hw) container_of(hw, struct
>> > rockchip_cpuclk, hw) +#define to_rockchip_cpuclk_nb(nb) \
>> > + container_of(nb, struct rockchip_cpuclk, clk_nb)
>> > +
>> > +static const struct rockchip_cpuclk_rate_table
>> > *rockchip_get_cpuclk_settings( + struct
>> > rockchip_cpuclk *cpuclk, unsigned long rate) +{
>> > + const struct rockchip_cpuclk_rate_table *rate_table =
>> > +
>> > cpuclk->rate_table;
>> > + int i;
>> > +
>> > + for (i = 0; i < cpuclk->rate_count; i++) {
>> > + if (rate == rate_table[i].prate)
>> > + return &rate_table[i];
>> > + }
>> > +
>> > + return NULL;
>> > +}
>> > +
>> > +static unsigned long rockchip_cpuclk_recalc_rate(struct clk_hw *hw,
>> > + unsigned long parent_rate)
>> > +{
>> > + struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_hw(hw);
>> > + const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > + u32 clksel0 = readl_relaxed(cpuclk->reg_base +
>> > reg_data->core_reg);
>> > +
>> > + clksel0 >>= reg_data->div_core_shift;
>> > + clksel0 &= reg_data->div_core_mask;
>> > + return parent_rate / (clksel0 + 1);
>> > +}
>> > +
>> > +static const struct clk_ops rockchip_cpuclk_ops = {
>> > + .recalc_rate = rockchip_cpuclk_recalc_rate,
>> > +};
>> > +
>> > +static int rockchip_cpuclk_pre_rate_change(struct rockchip_cpuclk
>> > *cpuclk,
>> > + struct clk_notifier_data
>> > *ndata)
>> > +{
>> > + const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > + const struct rockchip_cpuclk_rate_table *rate;
>> > + unsigned long alt_prate, alt_div;
>> > + int i;
>> > +
>> > + rate = rockchip_get_cpuclk_settings(cpuclk, ndata->new_rate);
>> > + if (!rate) {
>> > + pr_err("%s: Invalid rate : %lu for cpuclk\n",
>> > + __func__, ndata->new_rate);
>> > + return -EINVAL;
>> > + }
>> > +
>> > + alt_prate = clk_get_rate(cpuclk->alt_parent);
>> > +
>> > + spin_lock(cpuclk->lock);
>> > +
>> > + /*
>> > + * If the old parent clock speed is less than the clock speed
>> > + * of the alternate parent, then it should be ensured that at no
>> > point + * the armclk speed is more than the old_rate until the
>> > dividers are + * set.
>> > + */
>> > + if (alt_prate > ndata->old_rate) {
>> > + /* calculate dividers */
>> > + alt_div = DIV_ROUND_UP(alt_prate, ndata->old_rate) - 1;
>> > + if (alt_div > reg_data->div_core_mask) {
>> > + pr_warn("%s: limiting alt-divider %lu to %d\n",
>> > + __func__, alt_div,
>> > reg_data->div_core_mask); + alt_div =
>> > reg_data->div_core_mask;
>> > + }
>> > +
>> > + /*
>> > + * Change parents and add dividers in a single
>> > transaction.
>> > + *
>> > + * NOTE: we do this in a single transaction so we're never
>> > + * dividing the primary parent by the extra dividers that
>> > were + * needed for the alt.
>> > + */
>> > + pr_debug("%s: setting div %lu as alt-rate %lu > old-rate
>> > %lu\n", + __func__, alt_div, alt_prate,
>> > ndata->old_rate); +
>> > + writel(HIWORD_UPDATE(alt_div, reg_data->div_core_mask,
>> > + reg_data->div_core_shift) |
>> > + HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > + cpuclk->reg_base + reg_data->core_reg);
>> > + } else {
>> > + /* select alternate parent */
>> > + writel(HIWORD_UPDATE(1, 1, reg_data->mux_core_shift),
>> > + cpuclk->reg_base + reg_data->core_reg);
>> > + }
>> > +
>> > + /* alternate parent is active now. set the dividers */
>> > + for (i = 0; i < ARRAY_SIZE(rate->divs); i++) {
>> > + const struct rockchip_cpuclk_clksel *clksel =
>> > &rate->divs[i]; +
>> > + if (!clksel->reg)
>> > + continue;
>> > +
>> > + pr_debug("%s: setting reg 0x%x to 0x%x\n",
>> > + __func__, clksel->reg, clksel->val);
>> > + writel(clksel->val , cpuclk->reg_base + clksel->reg);
>> > + }
>>
>> I'm not 100% certain that you can always set the dividers here and
>> still be safe.
>>
>> Let's imagine that we're going 800MHz => 300MHz. Let's say that we
>> have "divide by 3" for 800Mhz and "divide by 1" for 300Mhz. Let's say
>> that the alternate PLL is 500Mhz.
>>
>> ...so at this point in the transition we've switched to the alternate
>> PLL (500Mhz) but we're now setting the dividers for 300MHz. That
>> doesn't seem quite right.
>
> So setting dividers here when increasing the rate and in the post_rate notifier
> when lowering the rate?
Yes.
> I think the samsung cpuclk had the same discussion at some point, but am not
> sure what the outcome was.
OK. I didn't go try to read all of the exynos upstream discussion. I
mostly just reviewed your patch as-is and also did some diffs to the
newest version of the exynos patch I could find.
It appears that the exynos patch does set some dividers in the post change.
>> I also _think_ that these dividers are based on the PLL, not based on
>> the armclock, right. In other words:
>>
>> 200MHz -> 300MHz with 500Mhz alternate PLL will do:
>>
>> 1. Switch core PLL to 500MHz alt with "div by 3", so 166MHz arm clock. OK.
>>
>> 2. ...but the "div by 3" doesn't apply to MP AXI clock divider, M0 AXI
>> clock divider, etc. That means _those_ are now based on 500MHz and
>> are now running faster than they were.
>>
>
> Nope, the dividers are clocks that are really children of the armclk. In the
> rk3288 trm on page 44 you can see the mux from apll and gpll, followed by the
> divider we use to keep the temporary rate below the old apll rate.
Ah, perfect. Yes, that diagram clears things up. I was using the
register descriptions only where it was unclear.
> The result of this is then supplied to the armclk [*] and the tightly bound
> clocks. So the "div by 3" when using the alternate parent will be used by both
> the arm-cores as well as the dependent clocks.
>
>
>
> [*] There is a slight variance between rk3288 and the Cortex-A9 SoCs. On
> rk3188 and before the result of the mux+divider described above are fed
> directly and unmodified to the arm cores, while the core clocks on rk3288 could
> use another divider on a per-core basis (see clk_core0..clk_core3 on the page
> 44 cited above).
>
> But it also doesn't matter much for this, as the coupled clocks are dependent
> on the shared parent of clk_coreX.
>
>
>>
>> None of that terribly matters for rk3288 which (currently) has the
>> same dividers for every rate point, but it seems like it might be
>> relevant for 3066 and 3188. ...or if they ever need to change
>> dividers for rk3288.
>>
>> > +
>> > + spin_unlock(cpuclk->lock);
>> > + return 0;
>> > +}
>> > +
>> > +static int rockchip_cpuclk_post_rate_change(struct rockchip_cpuclk
>> > *cpuclk, + struct
>> > clk_notifier_data *ndata) +{
>> > + const struct rockchip_cpuclk_reg_data *reg_data =
>> > cpuclk->reg_data;
>> > +
>> > + spin_lock(cpuclk->lock);
>> > +
>> > + /*
>> > + * post-rate change event, re-mux to primary parent and remove
>> > dividers. + *
>> > + * NOTE: we do this in a single transaction so we're never
>> > dividing the + * primary parent by the extra dividers that were
>> > needed for the alt. + */
>> > +
>> > + writel(HIWORD_UPDATE(0, reg_data->div_core_mask,
>> > + reg_data->div_core_shift) |
>> > + HIWORD_UPDATE(0, 1, reg_data->mux_core_shift),
>> > + cpuclk->reg_base + reg_data->core_reg);
>> > +
>> > + spin_unlock(cpuclk->lock);
>> > + return 0;
>> > +}
>> > +
>> > +/*
>> > + * This clock notifier is called when the frequency of the parent clock
>> > + * of cpuclk is to be changed. This notifier handles the setting up all
>> > + * the divider clocks, remux to temporary parent and handling the safe
>> > + * frequency levels when using temporary parent.
>> > + */
>> > +static int rockchip_cpuclk_notifier_cb(struct notifier_block *nb,
>> > + unsigned long event, void *data)
>> > +{
>> > + struct clk_notifier_data *ndata = data;
>> > + struct rockchip_cpuclk *cpuclk = to_rockchip_cpuclk_nb(nb);
>> > + int ret = 0;
>> > +
>> > + pr_debug("%s: event %lu, old_rate %lu, new_rate: %lu\n",
>> > + __func__, event, ndata->old_rate, ndata->new_rate);
>> > + if (event == PRE_RATE_CHANGE)
>> > + ret = rockchip_cpuclk_pre_rate_change(cpuclk, ndata);
>> > + else if (event == POST_RATE_CHANGE)
>> > + ret = rockchip_cpuclk_post_rate_change(cpuclk, ndata);
>> > +
>> > + return notifier_from_errno(ret);
>> > +}
>> > +
>> > +struct clk *rockchip_clk_register_cpuclk(const char *name,
>> > + const char **parent_names, u8 num_parents,
>> > + const struct rockchip_cpuclk_reg_data *reg_data,
>> > + struct rockchip_cpuclk_rate_table *rate_table,
>> > + void __iomem *reg_base, spinlock_t *lock)
>> > +{
>> > + struct rockchip_cpuclk *cpuclk;
>> > + struct clk_init_data init;
>> > + struct clk *clk, *cclk;
>> > + int ret;
>> > +
>> > + if (!reg_data) {
>> > + pr_err("%s: no soc register information\n", __func__);
>> > + return ERR_PTR(-EINVAL);
>> > + }
>>
>> Do we really think it likely that we'll get passed a NULL reg_data?
>>
>> > +
>> > + if (num_parents != 2) {
>> > + pr_err("%s: needs two parent clocks\n", __func__);
>> > + return ERR_PTR(-EINVAL);
>> > + }
>>
>> I guess this is just future proofing things? I notice that the most
>> recently posted exynos driver just takes two strings, the main parent
>> and an alternate parent. That does seem slightly cleaner.
>
> As the regular clock register functions use the parent_names, num_parents form
> it somehow seemed cleaner to me to continue this, but I don't have a hard
> preference here.
I won't force the issue, so do what you feel is cleanest.
>> > + cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
>> > + if (!cpuclk)
>> > + return ERR_PTR(-ENOMEM);
>> > +
>> > + init.name = name;
>> > + init.parent_names = &parent_names[0];
>> > + init.num_parents = 1;
>> > + init.ops = &rockchip_cpuclk_ops;
>> > +
>> > + /* only allow rate changes when we have a rate table */
>> > + init.flags = rate_table ? CLK_SET_RATE_PARENT : 0;
>> > +
>> > + /* disallow automatic parent changes by ccf */
>> > + init.flags |= CLK_SET_RATE_NO_REPARENT;
>> > +
>> > + init.flags |= CLK_GET_RATE_NOCACHE;
>> > +
>> > + cpuclk->reg_base = reg_base;
>> > + cpuclk->lock = lock;
>> > + cpuclk->reg_data = reg_data;
>> > + cpuclk->clk_nb.notifier_call = rockchip_cpuclk_notifier_cb;
>> > + cpuclk->hw.init = &init;
>> > +
>> > + cpuclk->alt_parent = __clk_lookup(parent_names[1]);
>> > + if (!cpuclk->alt_parent) {
>> > + pr_err("%s: could not lookup alternate parent\n",
>> > + __func__);
>> > + ret = -EINVAL;
>> > + goto free_cpuclk;
>> > + }
>> > +
>> > + ret = clk_prepare_enable(cpuclk->alt_parent);
>> > + if (ret) {
>> > + pr_err("%s: could not enable alternate parent\n",
>> > + __func__);
>> > + goto free_cpuclk;
>> > + }
>> > +
>> > + clk = __clk_lookup(parent_names[0]);
>> > + if (!clk) {
>> > + pr_err("%s: could not lookup parent clock %s\n",
>> > + __func__, parent_names[0]);
>> > + ret = -EINVAL;
>> > + goto free_cpuclk;
>> > + }
>> > +
>> > + ret = clk_notifier_register(clk, &cpuclk->clk_nb);
>> > + if (ret) {
>> > + pr_err("%s: failed to register clock notifier for %s\n",
>> > + __func__, name);
>> > + goto free_cpuclk;
>> > + }
>> > +
>> > + if (rate_table) {
>> > + int nrates;
>> > +
>> > + /* find count of rates in rate_table */
>> > + for (nrates = 0; rate_table[nrates].prate != 0; )
>> > + nrates++;
>> > +
>> > + cpuclk->rate_count = nrates;
>> > + cpuclk->rate_table = kmemdup(rate_table,
>> > + sizeof(*rate_table) * nrates,
>> > + GFP_KERNEL);
>>
>> Ah, I think I see what Derek was saying earlier. It's a little
>> awkward that you pass this in as a table with a sentinel at the end
>> and then at runtime convert it to an array with a stored length.
>> Given that all of these are compiled-in tables, it wouldn't be crazy
>> to just pass in nrates (which is just ARRAY_SIZE in all callers) and
>> get rid of the sentinel. ...but I won't insist on it.
>
> This would also be true for the pll rate tables then ... mixing concepts
> between such tables in clock drivers would be strange somehow.
>
> I guess this was originally simply getting a lot of inpsiration from the
> samsung pll-clock driver, which does use the same style (including the
> copying). Not sure if there was a rationale behind the original.
>
> Interestingly (I just looked it up) the new samsung cpuclk uses the rates +
> nrates approach.
>
> So I guess I would be ok with using the other approach as well :-)
Again, no hard requirement from me, so do as you see fit. It does
seem nice to get rid of a little extra code though...
-Doug
More information about the linux-arm-kernel
mailing list