Re: [PATCH v4 3/8] clk: samsung: add infrastructure to register cpu clocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tomasz,

On Fri, May 16, 2014 at 10:47 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> Hi Thomas,
>
> On 14.05.2014 03:11, Thomas Abraham wrote:
>> From: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
>>
>> The CPU clock provider supplies the clock to the CPU clock domain. The
>> composition and organization of the CPU clock provider could vary among
>> Exynos SoCs. A CPU clock provider can be composed of clock mux, dividers
>> and gates. This patch defines a new clock type for CPU clock provider and
>> adds infrastructure to register the CPU clock providers for Samsung
>> platforms.
>>
>> Cc: Tomasz Figa <t.figa@xxxxxxxxxxx>
>> Signed-off-by: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
>> ---
>>  drivers/clk/samsung/Makefile  |    2 +-
>>  drivers/clk/samsung/clk-cpu.c |  458 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/samsung/clk.h     |    5 +
>>  3 files changed, 464 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/clk/samsung/clk-cpu.c
>>
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> index 8eb4799..e2b453f 100644
>> --- a/drivers/clk/samsung/Makefile
>> +++ b/drivers/clk/samsung/Makefile
>> @@ -2,7 +2,7 @@
>>  # Samsung Clock specific Makefile
>>  #
>>
>> -obj-$(CONFIG_COMMON_CLK)     += clk.o clk-pll.o
>> +obj-$(CONFIG_COMMON_CLK)     += clk.o clk-pll.o clk-cpu.o
>>  obj-$(CONFIG_ARCH_EXYNOS4)   += clk-exynos4.o
>>  obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
>>  obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
>> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
>> new file mode 100644
>> index 0000000..6a40862
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-cpu.c
>> @@ -0,0 +1,458 @@
>> +/*
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
>> + *
>> + * 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 functions to register the cpu clocks
>> + * for samsung platforms.
>
> s/cpu/CPU/
> s/samsung/Samsung/
>
>> +*/
>> +
>> +#include <linux/errno.h>
>> +#include "clk.h"
>> +
>> +#define SRC_CPU                      0x0
>> +#define STAT_CPU             0x200
>> +#define DIV_CPU0             0x300
>> +#define DIV_CPU1             0x304
>> +#define DIV_STAT_CPU0                0x400
>> +#define DIV_STAT_CPU1                0x404
>> +
>> +#define MAX_DIV                      8
>> +
>> +#define EXYNOS4210_ARM_DIV1(div) ((div & 0x7) + 1)
>> +#define EXYNOS4210_ARM_DIV2(div) (((div >> 28) & 0x7) + 1)
>> +
>> +#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0)                  \
>> +             ((d5 << 24) | (d4 << 20) | (d3 << 16) | (d2 << 12) |    \
>> +              (d1 << 8) | (d0 <<  4))
>> +#define EXYNOS4210_DIV_CPU1(d2, d1, d0)                                      \
>> +             ((d2 << 8) | (d1 << 4) | (d0 << 0))
>
> Macro arguments should be put into parentheses to make sure that whole
> argument is subject to further arithmetic operations.
>
>> +
>> +#define EXYNOS4210_DIV1_HPM_MASK     ((0x7 << 0) | (0x7 << 4))
>> +#define EXYNOS4210_MUX_HPM_MASK              (1 << 20)
>> +
>> +/**
>> + * struct exynos4210_armclk_data: config data to setup exynos4210 cpu clocks.
>> + * @prate:   frequency of the parent clock.
>> + * @div0:    value to be programmed in the div_cpu0 register.
>> + * @div1:    value to be programmed in the div_cpu1 register.
>> + *
>> + * This structure holds the divider configuration data for divider clocks
>> + * belonging to the CMU_CPU clock domain. The parent frequency at which these
>> + * divider values are vaild is specified in @prate.
>
> s/vaild/valid/
>
>> + */
>> +struct exynos4210_armclk_data {
>> +     unsigned long   prate;
>> +     unsigned int    div0;
>> +     unsigned int    div1;
>> +};
>> +
>> +/**
>> + * struct exynos_cpuclk: information about clock supplied to a CPU core.
>> + * @hw:              handle between ccf and cpu clock.
>
> s/ccf/CCF/
> s/cpu/CPU/
>
>> + * @alt_parent:      alternate parent clock to use when switching the speed
>> + *           of the primary parent clock.
>> + * @ctrl_base:       base address of the clock controller.
>> + * @offset:  offset from the ctrl_base address where the cpu clock div/mux
>
> s/cpu/CPU/
>
>> + *           registers can be accessed.
>> + * @clk_nb:  clock notifier registered for changes in clock speed of the
>> + *           primary parent clock.
>> + * @lock:    register access lock.
>> + * @data:    optional data which the acutal instantiation of this clock
>> + *           can use.
>
> s/acutal/actual/
>
>> + */
>> +struct exynos_cpuclk {
>> +     struct clk_hw           hw;
>> +     struct clk              *alt_parent;
>> +     void __iomem            *ctrl_base;
>> +     unsigned long           offset;
>> +     struct notifier_block   clk_nb;
>> +     spinlock_t              *lock;
>> +     void                    *data;
>> +};
>> +
>> +#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
>> +#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)
>> +
>> +/**
>> + * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
>> + * @parser:  pointer to a function that can parse SoC specific data.
>> + * @ops:     clock operations to be used for this clock.
>> + * @offset:  optional offset from base of clock controller register base, to
>> + *           be used when accessing clock controller registers related to the
>> + *           cpu clock.
>
> s/cpu/CPU/
>
>> + * @clk_cb:  the clock notifier callback to be called for changes in the
>> + *           clock rate of the primary parent clock.
>> + *
>> + * This structure provides SoC specific data for ARM clocks. Based on
>> + * the compatible value of the clock controller node, the value of the
>> + * fields in this structure can be populated.
>> + */
>> +struct exynos_cpuclk_soc_data {
>> +     int (*parser)(struct device_node *, void **);
>
> Here you don't have argument names, but...
>
>> +     const struct clk_ops *ops;
>> +     unsigned int offset;
>> +     int (*clk_cb)(struct notifier_block *nb, unsigned long evt, void *data);
>
> ...here you have. Please keep some consistency.
>
>> +};
>> +
>> +/* common round rate callback useable for all types of cpu clocks */
>
> s/cpu/CPU/
>
>> +static long exynos_cpuclk_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate)
>
> Hmm, the long return type will overflow with *prate > INT_MAX and
> best_div == 1, I wonder why it is defined so in CCF, even though it
> shouldn't return error codes...
>
>> +{
>> +     struct clk *parent = __clk_get_parent(hw->clk);
>> +     unsigned long max_prate = __clk_round_rate(parent, UINT_MAX);
>> +     unsigned long t_prate, div = 1, best_div = 1;
>> +     unsigned long delta, min_delta = UINT_MAX;
>
> By the way, shouldn't this function take into account the list of
> available CPU rates and round drate to a less or equal supported one?
> Otherwise, in further code you might hit cases where an unsupported rate
> is requested, which is against the CCF semantics, if .round_rate()
> operation is provided.
>
>> +
>> +     do {
>> +             t_prate = __clk_round_rate(parent, drate * div);
>> +             delta = drate - (t_prate / div);
>> +             if (delta < min_delta) {
>> +                     *prate = t_prate;
>> +                     best_div = div;
>> +                     min_delta = delta;
>> +             }
>> +             if (!delta)
>> +                     break;
>> +             div++;
>> +     } while ((drate * div) < max_prate && div <= MAX_DIV);
>> +
>> +     return *prate / best_div;
>> +}
>> +
>> +static unsigned long _calc_div(unsigned long prate, unsigned long drate)
>> +{
>> +     unsigned long div = prate / drate;
>> +
>> +     WARN_ON(div >= MAX_DIV);
>> +     return (!(prate % drate)) ? div-- : div;
>
> Could you explain what is the purpose of this check and adjustment?
>
> If my assumption that this is essentially DIV_ROUND_UP(prate, drate) - 1
> is true then this probably used to obtain a divisor value to get less or
> equal rate than drate. Is it right?
>
>> +}
>> +
>> +/* helper function to register a cpu clock */
>> +static int __init exynos_cpuclk_register(unsigned int lookup_id,
>> +             const char *name, const char **parents,
>> +             unsigned int num_parents, void __iomem *base,
>
> The num_parents argument doesn't seem to be used in the code. Maybe
> instead you should simply replace it and parents arguments with (const
> char *parent) and (const char *alt_parent)?
>
>> +             const struct exynos_cpuclk_soc_data *soc_data,
>> +             struct device_node *np, const struct clk_ops *ops,
>> +             spinlock_t *lock)
>> +{
>> +     struct exynos_cpuclk *cpuclk;
>> +     struct clk_init_data init;
>> +     struct clk *clk;
>> +     int ret;
>> +
>> +     cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
>> +     if (!cpuclk) {
>> +             pr_err("%s: could not allocate memory for %s clock\n",
>> +                                     __func__, name);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     init.name = name;
>> +     init.flags = CLK_SET_RATE_PARENT;
>> +     init.parent_names = parents;
>> +     init.num_parents = 1;
>> +     init.ops = ops;
>> +
>> +     cpuclk->hw.init = &init;
>> +     cpuclk->ctrl_base = base;
>> +     cpuclk->lock = lock;
>> +
>> +     ret = soc_data->parser(np, &cpuclk->data);
>> +     if (ret) {
>> +             pr_err("%s: error %d in parsing %s clock data",
>> +                             __func__, ret, name);
>> +             ret = -EINVAL;
>> +             goto free_cpuclk;
>> +     }
>> +     cpuclk->offset = soc_data->offset;
>> +     init.ops = soc_data->ops;
>> +
>> +     cpuclk->clk_nb.notifier_call = soc_data->clk_cb;
>> +     if (clk_notifier_register(__clk_lookup(parents[0]), &cpuclk->clk_nb)) {
>> +             pr_err("%s: failed to register clock notifier for %s\n",
>> +                             __func__, name);
>> +             goto free_cpuclk_data;
>> +     }
>> +
>> +     cpuclk->alt_parent = __clk_lookup(parents[1]);
>> +     if (!cpuclk->alt_parent) {
>> +             pr_err("%s: could not lookup alternate parent %s\n",
>> +                             __func__, parents[1]);
>> +             ret = -EINVAL;
>> +             goto free_cpuclk_data;
>
> Shouldn't it be goto unregister_notifier and the notifier unregistered
> there?
>
>> +     }
>> +
>> +     clk = clk_register(NULL, &cpuclk->hw);
>> +     if (IS_ERR(clk)) {
>> +             pr_err("%s: could not register cpuclk %s\n", __func__,  name);
>> +             ret = PTR_ERR(clk);
>> +             goto free_cpuclk_data;
>
> Ditto.
>
>> +     }
>> +
>> +     samsung_clk_add_lookup(clk, lookup_id);
>> +     return 0;
>> +
>> +free_cpuclk_data:
>> +     kfree(cpuclk->data);
>> +free_cpuclk:
>> +     kfree(cpuclk);
>> +     return ret;
>> +}
>> +
>> +static void _exynos4210_set_armclk_div(void __iomem *base, unsigned long div)
>> +{
>> +     unsigned long timeout = jiffies + msecs_to_jiffies(10);
>> +
>> +     writel((readl(base + DIV_CPU0) & ~0x7) | div, base + DIV_CPU0);
>
> The 0x7 could be defined as a preprocessor macro. Also for increased
> readability, this could be split into separate read, modify and write.
>
>> +     while (time_before(jiffies, timeout))
>> +             if (!readl(base + DIV_STAT_CPU0))
>> +                     return;
>> +     pr_err("%s: timeout in divider stablization\n", __func__);
>> +}
>> +
>> +static unsigned long exynos4210_armclk_recalc_rate(struct clk_hw *hw,
>> +                             unsigned long parent_rate)
>> +{
>> +     struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
>> +     void __iomem *base = armclk->ctrl_base + armclk->offset;
>> +     unsigned long div0 = readl(base + DIV_CPU0);
>> +
>> +     return parent_rate / EXYNOS4210_ARM_DIV1(div0) /
>> +                             EXYNOS4210_ARM_DIV2(div0);
>> +}
>> +
>> +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data *ndata,
>> +                     struct exynos_cpuclk *armclk, void __iomem *base)
>> +{
>> +     struct exynos4210_armclk_data *armclk_data = armclk->data;
>> +     unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
>> +     unsigned long alt_div, div0, div1, tdiv0, mux_reg;
>> +     unsigned long cur_armclk_rate, timeout;
>> +     unsigned long flags;
>> +
>> +     /* find out the divider values to use for clock data */
>> +     while (armclk_data->prate != ndata->new_rate) {
>
> I assume this code relies on the assumption that target DIV_CORE and
> DIV_CORE2 are always 0 (divide by 1)? Otherwise it should compare
> armclk_data->prate with new parent rate, not new target armclk rate,
> which would be parent rate divided by DIV_CORE and DIV_CORE2.
>
>> +             if (armclk_data->prate == 0)
>> +                     return -EINVAL;
>> +             armclk_data++;
>> +     }
>> +
>> +     div0 = armclk_data->div0;
>> +     div1 = armclk_data->div1;
>
> A comment about the following if would be nice.
>
>> +     if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
>> +             div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
>> +             div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
>> +     }
>> +
>> +     /*
>> +      * if the new and 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_prate until the dividers are
>> +      * set.
>> +      */
>> +     tdiv0 = readl(base + DIV_CPU0);
>> +     cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
>> +                             EXYNOS4210_ARM_DIV2(tdiv0);
>> +     if (alt_prate > cur_armclk_rate) {
>
> Shouldn't you compare two parent rates here, not alt parent rate with
> current armclk rate?
>
> Also, this condition compares only alt rate with current rate. Let's see:
>
> 1) old >= alt && new >= alt => alt < old X new
>
> The voltage will be always enough to handle the switch, so no division
> is needed.
>
> 2) old < alt && new >= alt => old < alt <= new
>
> The voltage will be switched to higher or equal necessary one for alt
> rate, so no division is needed.
>
> 3) old < alt && new < alt => old X new < alt
>
> The voltage won't be enough for alt rate so division is needed.
>
> 4) old >= alt && new < alt => new < alt <= old
>
> Current voltage is enough for alt rate and it will be lowered only after
> the switching finishes, so division is not needed.
>
> This means that division is necessary only if both new and old rates are
> lower than alt and this is what the comment above says, but not what the
> code does, which is slightly inefficient.
>
>> +             alt_div = _calc_div(alt_prate, cur_armclk_rate);
>> +             _exynos4210_set_armclk_div(base, alt_div);
>> +             div0 |= alt_div;
>
> Hmm, this code is barely readable. It is not clear whether _calc_div()
> is returning a value ready to be written to the register or real divisor
> value. I'd make _calc_div() to simply return raw divisor value and then
> use a macro that calculates required bitfield value.
>
> Another thing is whether 8 is big enough maximum divisor. If not, both
> DIV_CORE and DIV_CORE2 should be used together to form a 6-bit divisor,
> which lets you divide by up to 64.
>
>> +     }
>> +
>> +     /* select sclk_mpll as the alternate parent */
>> +     spin_lock_irqsave(armclk->lock, flags);
>
> Hmm, is the start of critical section really here? The big
> read-modify-write section seems to begin at
>
>         if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
>                 div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
>
>> +     mux_reg = readl(base + SRC_CPU);
>> +     writel(mux_reg | (1 << 16), base + SRC_CPU);
>> +
>> +     timeout = jiffies + msecs_to_jiffies(10);
>> +     while (time_before(jiffies, timeout))
>> +             if (((readl(base + STAT_CPU) >> 16) & 0x7) == 2)
>> +                     break;
>> +     spin_unlock_irqrestore(armclk->lock, flags);
>
> Is this really end of critical secion? More writes to registers are
> happening below. Keep in mind that APLL_RATIO field of CLK_DIV_CPU0
> register is used by generic divider clock - "sclk_apll".
>
>> +
>> +     if (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>> +             pr_err("%s: re-parenting to sclk_mpll failed\n", __func__);
>> +
>> +     /* alternate parent is active now. set the dividers */
>> +     writel(div0, base + DIV_CPU0);
>> +     timeout = jiffies + msecs_to_jiffies(10);
>> +     while (time_before(jiffies, timeout))
>> +             if (!readl(base + DIV_STAT_CPU0))
>> +                     break;
>> +
>> +     if (readl(base + DIV_STAT_CPU0))
>> +             pr_err("%s: timeout in divider0 stablization\n", __func__);
>> +
>> +     writel(div1, base + DIV_CPU1);
>> +     timeout = jiffies + msecs_to_jiffies(10);
>> +     while (time_before(jiffies, timeout))
>> +             if (!readl(base + DIV_STAT_CPU1))
>> +                     break;
>> +     if (readl(base + DIV_STAT_CPU1))
>> +             pr_err("%s: timeout in divider1 stablization\n", __func__);
>
> IMHO to be safe, the spin_unlock_irqrestore() should be called here.
>
>> +
>> +     return 0;
>> +}
>> +
>> +static int exynos4210_armclk_post_rate_change(struct exynos_cpuclk *armclk,
>> +                     void __iomem *base)
>> +{
>> +     unsigned long mux_reg, flags;
>> +     unsigned long timeout = jiffies + msecs_to_jiffies(10);
>> +
>> +     spin_lock_irqsave(armclk->lock, flags);
>> +     mux_reg = readl(base + SRC_CPU);
>> +     writel(mux_reg & ~(1 << 16), base + SRC_CPU);
>
> Please replace (1 << 16) with appropriate macro.
>
>> +     while (time_before(jiffies, timeout))
>> +             if (((readl(base + STAT_CPU) >> 16) & 0x7) == 1)
>> +                     break;
>> +     spin_unlock_irqrestore(armclk->lock, flags);
>> +
>> +     if (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
>> +             pr_err("%s: re-parenting to mout_apll failed\n", __func__);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * This clock notifier is called when the frequency of the parent clock
>> + * of armclk 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 exynos4210_armclk_notifier_cb(struct notifier_block *nb,
>> +                             unsigned long event, void *data)
>> +{
>> +     struct clk_notifier_data *ndata = data;
>> +     struct exynos_cpuclk *armclk = to_exynos_cpuclk_nb(nb);
>> +     void __iomem *base =  armclk->ctrl_base + armclk->offset;
>> +     int err = 0;
>> +
>> +     if (event == PRE_RATE_CHANGE)
>> +             err = exynos4210_armclk_pre_rate_change(ndata, armclk, base);
>> +     else if (event == POST_RATE_CHANGE)
>> +             err = exynos4210_armclk_post_rate_change(armclk, base);
>> +
>> +     return notifier_from_errno(err);
>> +}
>> +
>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned long drate,
>> +                                     unsigned long prate)
>> +{
>> +     struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
>> +     void __iomem *base = armclk->ctrl_base + armclk->offset;
>> +     unsigned long div;
>> +
>> +     div = drate < prate ? _calc_div(prate, drate) : 0;
>> +     _exynos4210_set_armclk_div(base, div);
>
> Hmm, the code above in pre_rate_change() assumed that both DIV_CORE and
> DIV_CORE2 are 0, but here it sets DIV_CORE to a potentially non-zero
> value. It doesn't look correct.
>
>> +     return 0;
>> +}
>> +
>> +static const struct clk_ops exynos4210_armclk_clk_ops = {
>> +     .recalc_rate = exynos4210_armclk_recalc_rate,
>> +     .round_rate = exynos_cpuclk_round_rate,
>> +     .set_rate = exynos4210_armclk_set_rate,
>> +};
>> +
>> +/*
>> + * parse divider configuration data from dt for all the cpu clock domain
>> + * clocks in exynos4210 and compatible SoC's.
>> + */
>> +static int __init exynos4210_armclk_parser(struct device_node *np, void **data)
>> +{
>> +     struct exynos4210_armclk_data *tdata;
>> +     u32 cfg[10], num_rows, row, col;
>> +     struct property *prop;
>> +     const __be32 *ptr = NULL;
>> +     u32 cells;
>> +     int ret;
>> +
>> +     if (of_property_read_u32(np, "samsung,armclk-cells", &cells))
>> +             return -EINVAL;
>> +     prop = of_find_property(np, "samsung,armclk-divider-table", NULL);
>
> You should rather use the *lenp argument of of_find_property(), instead
> of dereferencing the struct.
>
>> +     if (!prop)
>> +             return -EINVAL;
>> +     if (!prop->value)
>> +             return -EINVAL;
>
> You can skip the check above, as the calculation below will give you
> num_rows equal 0 in this case.
>
>> +     if ((prop->length / sizeof(u32)) % cells)
>> +             return -EINVAL;
>> +     num_rows = (prop->length / sizeof(u32)) / cells;
>> +
>> +     /* allocate a zero terminated table */
>> +     *data = kzalloc(sizeof(*tdata) * (num_rows + 1), GFP_KERNEL);
>> +     if (!*data)
>> +             ret = -ENOMEM;
>
> Shouldn't you just return -ENOMEM here?
>
> Best regards,
> Tomasz

Thanks for your detailed review. I have made all the changes that you
have suggested.

Regards,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Devel]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Forum]     [Linux SCSI]

  Powered by Linux