cpufreq: use generic cpufreq drivers for exynos platforms

drivers/clk/samsung/clk-cpu.c new file +0 -0

new file mode 100644
index 0000000..0d62968

drivers/clk/samsung/clk-cpu.c new file +9 -0

+*/
+
+#include <linux/errno.h>
+#include "clk.h"
+
+#define E4210_SRC_CPU		0x0
+#define E4210_STAT_CPU		0x200
+#define E4210_DIV_CPU0		0x300
+#define E4210_DIV_CPU1		0x304

drivers/clk/samsung/clk-cpu.c new file +11 -0

+#define E4210_MUX_HPM_MASK	(1 << 20)
+#define E4210_DIV0_ATB_SHIFT	16
+#define E4210_DIV0_ATB_MASK	(DIV_MASK << E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0)	\
+		(((apll) << 24) | ((pclk_dbg) << 20) | ((atb) << 16) |	\
+		((periph) << 12) | ((corem1) << 8) | ((corem0) <<  4))
+#define E4210_CPU_DIV1(hpm, copy)					\
+		(((hpm) << 4) | ((copy) << 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)		\

drivers/clk/samsung/clk-cpu.c new file +27 -0

+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the CPU
+ * clock domain. The parent frequency at which these divider values are valid is
+ * specified in @prate. The @prate is the frequency of the primary parent clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_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.
+ * @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
+ *	registers can be accessed.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock

drivers/clk/samsung/clk-cpu.c new file +4 -0

+ * @type: type of the CPU clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ *	of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification

drivers/clk/samsung/clk-cpu.c new file +11 -0

+static void wait_until_divider_stable(void __iomem *div_reg, unsigned long mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+
+	do {
+		if (!(readl(div_reg) & mask))
+			return;
+	} while (time_before(jiffies, timeout));
+
+	pr_err("%s: timeout in divider stablization\n", __func__);
+}

drivers/clk/samsung/clk-cpu.c new file +9 -0

+	return parent_rate;
+}
+
+static const struct clk_ops exynos_cpuclk_clk_ops = {
+	.recalc_rate = exynos_cpuclk_recalc_rate,
+	.round_rate = exynos_cpuclk_round_rate,
+};
+
+/*

+

drivers/clk/samsung/clk-cpu.c new file +7 -0

+/* helper function to register a cpu clock */
+static int __init exynos_cpuclk_register(struct samsung_clk_provider *ctx,
+		unsigned int lookup_id, const char *name, const char *parent,
+		const char *alt_parent, struct device_node *np,
+		const struct exynos_cpuclk_soc_data *soc_data)
+{
+	struct exynos_cpuclk *cpuclk;

drivers/clk/samsung/clk-cpu.c new file +13 -0

+	if (!data) {
+		ret = -ENOMEM;
+		goto free_cpuclk;
+	}
+
+	init.name = name;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = &parent;
+	init.num_parents = 1;
+	init.ops = soc_data->ops;
+
+	cpuclk->hw.init = &init;
+	cpuclk->ctrl_base = ctx->reg_base;

drivers/clk/samsung/clk-cpu.c new file +6 -0

+	if (ret) {
+		pr_err("%s: failed to register clock notifier for %s\n",
+				__func__, name);
+		goto free_cpuclk_data;
+	}
+

drivers/clk/samsung/clk-cpu.c new file +16 -0

+	writel(div0, base + E4210_DIV_CPU0);
+	wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, mask);
+}
+
+/* handler for pre-rate change notification from parent clock */
+static int exynos4210_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
+			struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+	const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
+	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
+	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
+	unsigned long div0, div1 = 0, mux_reg;
+
+	/* find out the divider values to use for clock data */
+	while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
+		if (cpuclk_data->prate == 0)

drivers/clk/samsung/clk-cpu.c new file +9 -0

+		div1 = cpuclk_data->div1;
+		if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK) {
+			div1 = readl(base + E4210_DIV_CPU1) &
+					E4210_DIV1_HPM_MASK;
+			div1 |= ((cpuclk_data->div1) & ~E4210_DIV1_HPM_MASK);
+		}
+	}
+
+	spin_lock(cpuclk->lock);

drivers/clk/samsung/clk-cpu.c new file +11 -0

+			/*
+			 * In Exynos4210, ATB clock parent is also mout_core. So
+			 * ATB clock also needs to be mantained at safe speed.
+			 */
+			alt_div |= E4210_DIV0_ATB_MASK;
+			alt_div_mask |= E4210_DIV0_ATB_MASK;
+		}
+		exynos4210_set_safe_div(base, alt_div, alt_div_mask);
+		div0 |= alt_div;
+	}
+

drivers/clk/samsung/clk-cpu.c new file +15 -0

+		writel(div1, base + E4210_DIV_CPU1);
+		wait_until_divider_stable(base + E4210_DIV_STAT_CPU1,
+				DIV_MASK_ALL);
+	}
+
+	spin_unlock(cpuclk->lock);
+	return 0;
+}
+
+/* handler for post-rate change notification from parent clock */
+static int exynos4210_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
+			struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+	const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
+	unsigned long div = 0, div_mask = DIV_MASK;

drivers/clk/samsung/clk-cpu.c new file +25 -0

+		/* find out the divider values to use for clock data */
+		while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
+			if (cpuclk_data->prate == 0)
+				return -EINVAL;
+			cpuclk_data++;
+		}
+
+		div |= (cpuclk_data->div0 & E4210_DIV0_ATB_MASK);
+		div_mask |= E4210_DIV0_ATB_MASK;
+	}
+
+	exynos4210_set_safe_div(base, div, div_mask);
+	spin_unlock(cpuclk->lock);
+	return 0;
+}
+
+static const struct exynos4210_cpuclk_data e4210_armclk_d[] __initconst = {
+	{ 1200000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 5), },
+	{ 1000000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 4), },
+	{  800000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+	{  500000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+	{  400000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+	{  200000, E4210_CPU_DIV0(0, 1, 1, 1, 3, 1), E4210_CPU_DIV1(0, 3), },
+	{  0 },
+};

drivers/clk/samsung/clk-cpu.c new file +9 -0

+static const struct exynos_cpuclk_soc_data e4210_clk_soc_data __initconst = {
+	.ops = &exynos_cpuclk_clk_ops,
+	.offset = 0x14200,
+	.data = e4210_armclk_d,
+	.data_size = sizeof(e4210_armclk_d),
+	.type = EXYNOS4210,
+	.pre_rate_cb = exynos4210_cpuclk_pre_rate_change,
+	.post_rate_cb = exynos4210_cpuclk_post_rate_change,
+};

drivers/clk/samsung/clk-cpu.c new file +20 -0

+/**
+ * exynos_register_cpu_clock: register cpu clock with ccf.
+ * @ctx: driver context.
+ * @cluster_id: cpu cluster number to which this clock is connected.
+ * @lookup_id: cpuclk clock output id for the clock controller.
+ * @name: the name of the cpu clock.
+ * @parent: name of the parent clock for cpuclk.
+ * @alt_parent: name of the alternate clock parent.
+ * @np: device tree node pointer of the clock controller.
+ */
+int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
+		unsigned int cluster_id, unsigned int lookup_id,
+		const char *name, const char *parent,
+		const char *alt_parent, struct device_node *np)
+{
+	const struct of_device_id *match;
+	const struct exynos_cpuclk_soc_data *data = NULL;
+
+	if (!np)
+		return -EINVAL;

Hi Thomas,

Please see my comments inline.

On 14.07.2014 15:38, Thomas Abraham wrote:

[snip]

Quoted 6 lines

drivers/clk/samsung/clk-cpu.c new file +0 -0

new file mode 100644
index 0000000..0d62968

drivers/clk/samsung/clk-cpu.c line 10

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

@@ -0,0 +1,576 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Thomas Abraham <[email protected]>
+ *
+ * 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.

I'd expect few words here or in separate comment on how the code works, i.e. assumptions made, most important procedures, etc. This is a complex piece of code for quite complex hardware, so proper documentation is essential.

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+*/
+
+#include <linux/errno.h>
+#include "clk.h"
+
+#define E4210_SRC_CPU                0x0
+#define E4210_STAT_CPU               0x200
+#define E4210_DIV_CPU0               0x300
+#define E4210_DIV_CPU1               0x304

drivers/clk/samsung/clk-cpu.c line 29

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+#define E4210_DIV_STAT_CPU0  0x400
+#define E4210_DIV_STAT_CPU1  0x404
+
+#define MAX_DIV                      8
+#define DIV_MASK             7
+#define DIV_MASK_ALL         0xffffffff
+#define MUX_MASK             7
+
+#define E4210_DIV0_RATIO0_MASK       0x7
+#define E4210_DIV1_HPM_MASK  ((0x7 << 4) | (0x7 << 0))

This mask contains two fields, doesn't it? I'd say it would be better for readability if you split it.

Quoted 11 lines

drivers/clk/samsung/clk-cpu.c new file +11 -0

+#define E4210_MUX_HPM_MASK   (1 << 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK  (DIV_MASK << E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0)  \
+             (((apll) << 24) | ((pclk_dbg) << 20) | ((atb) << 16) |  \
+             ((periph) << 12) | ((corem1) << 8) | ((corem0) <<  4))
+#define E4210_CPU_DIV1(hpm, copy)                                    \
+             (((hpm) << 4) | ((copy) << 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)               \

drivers/clk/samsung/clk-cpu.c line 50

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+             (((apll << 24) | (pclk_dbg << 20) | (atb << 16) |       \
+              (periph << 12) | (acp << 8) | (cpud << 4)))
+#define E5250_CPU_DIV1(hpm, copy)                                    \
+             (((hpm) << 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud)                    \
+             (((apll << 24) | (pclk_dbg << 20) | (atb << 16) |       \
+              (cpud << 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)                                      \
+             (((kpll << 24) | (pclk << 20) | (aclk << 4)))

Again, used macro arguments should always be surrounded with parentheses.

drivers/clk/samsung/clk-cpu.c line 59

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+
+enum cpuclk_type {
+     EXYNOS4210,
+     EXYNOS5250,
+     EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.

It seems like this could be used for all Exynos SoCs, so probably should be called exynos_cpuclk_data.

Quoted 27 lines

drivers/clk/samsung/clk-cpu.c new file +27 -0

+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the CPU
+ * clock domain. The parent frequency at which these divider values are valid is
+ * specified in @prate. The @prate is the frequency of the primary parent clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_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.
+ * @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
+ *   registers can be accessed.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock

drivers/clk/samsung/clk-cpu.c line 96

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+ *   can use.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ *   primary parent clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ *   of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
+ *   of the primary parent clock.
+ *
+ * This structure holds information required for programming the cpu clock for
+ * various clock speeds.

nit: s/cpu/CPU/

drivers/clk/samsung/clk-cpu.c line 105

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+ */
+struct exynos_cpuclk {
+     struct clk_hw           hw;
+     struct clk              *alt_parent;
+     void __iomem            *ctrl_base;
+     unsigned long           offset;
+     spinlock_t              *lock;
+     enum cpuclk_type        type;
+     const void              *data;

The code always expects this to be const struct exynos4210_cpuclk_data. Why not make this field so?

Also this is not some plain data, but an array of operating points, so probably a name like "rates" would be better.

drivers/clk/samsung/clk-cpu.c line 112

Quoted 7 lines

drivers/clk/samsung/clk-cpu.c new file +7 -0

+     struct notifier_block   clk_nb;
+     int                     (*pre_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);
+     int                     (*post_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);

All the Exynos SoCs supported by this patch seem to be using exactly the same notifiers. We don't know what changes in further SoCs, so there is no guarantee that having these as pointer here will give us any benefits. I'd recommend just getting rid of this indirection for now. If it turns out to be needed, it will be trivial to add it back.

drivers/clk/samsung/clk-cpu.c line 116

Quoted 4 lines

drivers/clk/samsung/clk-cpu.c new file +4 -0

+};
+
+#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)

Please make these static inlines for type safety.

drivers/clk/samsung/clk-cpu.c line 124

Quoted 8 lines

drivers/clk/samsung/clk-cpu.c new file +8 -0

+
+/**
+ * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
+ * @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.
+ * @data: SoC specific data for cpuclk configuration (optional).

How is this optional? Can this code work without a list of operating points?

drivers/clk/samsung/clk-cpu.c line 125

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+ * @data_size: size of the data contained in @data member.

Both fields could be probably named "rates" and "num_rates", with the meaning of the latter changed to mean the number of entries, not size in bytes.

Quoted 4 lines

drivers/clk/samsung/clk-cpu.c new file +4 -0

+ * @type: type of the CPU clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ *   of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification

drivers/clk/samsung/clk-cpu.c line 139

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+ *   of the primary parent clock.
+ *
+ * This structure provides SoC specific data for CPU 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 {
+     const struct clk_ops    *ops;
+     unsigned int            offset;
+     const void              *data;

Same comments as for the data field above.

drivers/clk/samsung/clk-cpu.c line 140

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+     const unsigned int      data_size;

If the same struct is always used it would be clearer to

drivers/clk/samsung/clk-cpu.c line 112

Quoted 7 lines

drivers/clk/samsung/clk-cpu.c new file +7 -0

+     enum cpuclk_type        type;
+     int                     (*pre_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);
+     int                     (*post_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);

Same comment as above.

drivers/clk/samsung/clk-cpu.c line 148

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+};

It looks like instead of duplicating most of the fields of this struct in exynos_cpuclk struct the latter could simply have a pointer to an instance of the former.

drivers/clk/samsung/clk-cpu.c line 153

Quoted 5 lines

drivers/clk/samsung/clk-cpu.c new file +5 -0

+
+/*
+ * Helper function to wait until divider(s) have stabilized after the divider
+ * value has changed.
+ */

How about a kernel doc like comments for functions as well? (same comment for remaining functions)

Quoted 12 lines

drivers/clk/samsung/clk-cpu.c new file +11 -0

+static void wait_until_divider_stable(void __iomem *div_reg, unsigned long mask)
+{
+     unsigned long timeout = jiffies + msecs_to_jiffies(10);
+
+     do {
+             if (!(readl(div_reg) & mask))
+                     return;
+     } while (time_before(jiffies, timeout));
+
+     pr_err("%s: timeout in divider stablization\n", __func__);
+}

[snip]

drivers/clk/samsung/clk-cpu.c line 195

Quoted 4 lines

drivers/clk/samsung/clk-cpu.c new file +4 -0

+/* common recalc rate callback useable for all types of CPU clocks */
+static unsigned long exynos_cpuclk_recalc_rate(struct clk_hw *hw,
+                     unsigned long parent_rate)
+{

This function is so trivial that it might be reasonable to explain why nothing else is needed, e.g.

/*
* The CPU clock output (armclk) rate is the same as its parent
* rate. Although there exist certain dividers inside the CPU
* clock block that could be used to divide the parent clock,
* the driver does not make use of them currently, except during
* frequency transitions.
*/

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+     return parent_rate;
+}
+
+static const struct clk_ops exynos_cpuclk_clk_ops = {
+     .recalc_rate = exynos_cpuclk_recalc_rate,
+     .round_rate = exynos_cpuclk_round_rate,
+};
+
+/*

drivers/clk/samsung/clk-cpu.c line 214

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+ * Calculates the divider value to be set for deriving drate from prate.
+ * Divider value is actual divider value - 1.
+ */
+static unsigned long _calc_div(unsigned long prate, unsigned long drate)
+{
+     unsigned long div = DIV_ROUND_UP(prate, drate) - 1;
+
+     WARN_ON(div >= MAX_DIV);
+     return div;
+}

This function seems to be used just once. Not even saying about its strange semantics - the name would suggest a real divider being returned, but it's not, it's divider minus one.

So I'd suggest to completely remove this function and simply paste its contents instead, since it's only used once.

Quoted 1 line

+

[snip]

Quoted 7 lines

drivers/clk/samsung/clk-cpu.c new file +7 -0

+/* helper function to register a cpu clock */
+static int __init exynos_cpuclk_register(struct samsung_clk_provider *ctx,
+             unsigned int lookup_id, const char *name, const char *parent,
+             const char *alt_parent, struct device_node *np,
+             const struct exynos_cpuclk_soc_data *soc_data)
+{
+     struct exynos_cpuclk *cpuclk;

drivers/clk/samsung/clk-cpu.c line 252

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+     struct clk_init_data init;
+     struct clk *clk;
+     void *data;
+     int ret = 0;
+
+     cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
+     if (!cpuclk)
+             return -ENOMEM;
+
+     data = kmalloc(soc_data->data_size, GFP_KERNEL);

You could simply use kmemdup() here to duplicate the source array. Also the return value could be already saved in cpuclk->data without the need for a local variable.

Quoted 13 lines

drivers/clk/samsung/clk-cpu.c new file +13 -0

+     if (!data) {
+             ret = -ENOMEM;
+             goto free_cpuclk;
+     }
+
+     init.name = name;
+     init.flags = CLK_SET_RATE_PARENT;
+     init.parent_names = &parent;
+     init.num_parents = 1;
+     init.ops = soc_data->ops;
+
+     cpuclk->hw.init = &init;
+     cpuclk->ctrl_base = ctx->reg_base;

drivers/clk/samsung/clk-cpu.c line 275

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+     cpuclk->lock = &ctx->lock;
+     cpuclk->offset = soc_data->offset;
+     cpuclk->type = soc_data->type;
+     cpuclk->pre_rate_cb = soc_data->pre_rate_cb;
+     cpuclk->post_rate_cb = soc_data->post_rate_cb;
+     memcpy(data, soc_data->data, soc_data->data_size);
+     cpuclk->data = data;
+
+     cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
+     ret = clk_notifier_register(__clk_lookup(parent), &cpuclk->clk_nb);

It would be a good idea to check if __clk_lookup() succeeded and error out otherwise.

Quoted 6 lines

drivers/clk/samsung/clk-cpu.c new file +6 -0

+     if (ret) {
+             pr_err("%s: failed to register clock notifier for %s\n",
+                             __func__, name);
+             goto free_cpuclk_data;
+     }
+

[snip]

drivers/clk/samsung/clk-cpu.c line 320

Quoted 12 lines

drivers/clk/samsung/clk-cpu.c new file +12 -0

+/*
+ * Helper function to set the 'safe' dividers for the CPU clock. The parameters
+ * div and mask contain the divider value and the register bit mask of the
+ * dividers to be programmed.
+ */
+static void exynos4210_set_safe_div(void __iomem *base, unsigned long div,
+                                     unsigned long mask)
+{
+     unsigned long div0;
+
+     div0 = readl(base + E4210_DIV_CPU0);
+     div0 = (div0 & ~mask) | div;

There is nothing said in the comment above about the assumption that div has bits not indicated by mask cleared, so to be safe it might be a good idea to make this

        div0 = (div0 & ~mask) | (div & mask);

instead.

Quoted 16 lines

drivers/clk/samsung/clk-cpu.c new file +16 -0

+     writel(div0, base + E4210_DIV_CPU0);
+     wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, mask);
+}
+
+/* handler for pre-rate change notification from parent clock */
+static int exynos4210_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
+                     struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+     const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
+     unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
+     unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
+     unsigned long div0, div1 = 0, mux_reg;
+
+     /* find out the divider values to use for clock data */
+     while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
+             if (cpuclk_data->prate == 0)

drivers/clk/samsung/clk-cpu.c line 346

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+                     return -EINVAL;
+             cpuclk_data++;
+     }
+
+     /* For the selected PLL clock frequency, get the pre-defined divider
+      * values. If the clock for sclk_hpm is not sourced from apll, then
+      * the values for DIV_COPY and DIV_HPM dividers need not be set.
+      */
+     div0 = cpuclk_data->div0;
+     if (cpuclk->type != EXYNOS5420) {

Rather than checking for Exynos5420 explicitly, it would be better to add a boolean "has_mux_hpm" flag to cpuclk.

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+             div1 = cpuclk_data->div1;
+             if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK) {
+                     div1 = readl(base + E4210_DIV_CPU1) &
+                                     E4210_DIV1_HPM_MASK;
+                     div1 |= ((cpuclk_data->div1) & ~E4210_DIV1_HPM_MASK);
+             }
+     }
+
+     spin_lock(cpuclk->lock);

drivers/clk/samsung/clk-cpu.c line 365

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+
+     /*
+      * 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.
+      */
+     if (alt_prate > ndata->old_rate) {
+             alt_div = _calc_div(alt_prate, ndata->old_rate);
+             if (cpuclk->type == EXYNOS4210) {

Again, this could be handled by a boolean "needs_atb_alt_div" flag, instead of checking for Exynos4210 explicitly.

Quoted 11 lines

drivers/clk/samsung/clk-cpu.c new file +11 -0

+                     /*
+                      * In Exynos4210, ATB clock parent is also mout_core. So
+                      * ATB clock also needs to be mantained at safe speed.
+                      */
+                     alt_div |= E4210_DIV0_ATB_MASK;
+                     alt_div_mask |= E4210_DIV0_ATB_MASK;
+             }
+             exynos4210_set_safe_div(base, alt_div, alt_div_mask);
+             div0 |= alt_div;
+     }
+

drivers/clk/samsung/clk-cpu.c line 386

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+     /* select sclk_mpll as the alternate parent */
+     mux_reg = readl(base + E4210_SRC_CPU);
+     writel(mux_reg | (1 << 16), base + E4210_SRC_CPU);
+     wait_until_mux_stable(base + E4210_STAT_CPU, 16, 2);
+
+     /* alternate parent is active now. set the dividers */
+     writel(div0, base + E4210_DIV_CPU0);
+     wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, DIV_MASK_ALL);
+
+     if (cpuclk->type != EXYNOS5420) {

This could be handled by "has_div_cpu1" boolean flag.

Quoted 15 lines

drivers/clk/samsung/clk-cpu.c new file +15 -0

+             writel(div1, base + E4210_DIV_CPU1);
+             wait_until_divider_stable(base + E4210_DIV_STAT_CPU1,
+                             DIV_MASK_ALL);
+     }
+
+     spin_unlock(cpuclk->lock);
+     return 0;
+}
+
+/* handler for post-rate change notification from parent clock */
+static int exynos4210_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
+                     struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+     const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
+     unsigned long div = 0, div_mask = DIV_MASK;

drivers/clk/samsung/clk-cpu.c line 411

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+     unsigned long mux_reg;
+
+     spin_lock(cpuclk->lock);
+
+     /* select mout_apll as the alternate parent */
+     mux_reg = readl(base + E4210_SRC_CPU);
+     writel(mux_reg & ~(1 << 16), base + E4210_SRC_CPU);
+     wait_until_mux_stable(base + E4210_STAT_CPU, 16, 1);
+
+     if (cpuclk->type == EXYNOS4210) {

Here the "needs_atb_alt_div" flag could be used again.

Quoted 25 lines

drivers/clk/samsung/clk-cpu.c new file +25 -0

+             /* find out the divider values to use for clock data */
+             while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
+                     if (cpuclk_data->prate == 0)
+                             return -EINVAL;
+                     cpuclk_data++;
+             }
+
+             div |= (cpuclk_data->div0 & E4210_DIV0_ATB_MASK);
+             div_mask |= E4210_DIV0_ATB_MASK;
+     }
+
+     exynos4210_set_safe_div(base, div, div_mask);
+     spin_unlock(cpuclk->lock);
+     return 0;
+}
+
+static const struct exynos4210_cpuclk_data e4210_armclk_d[] __initconst = {
+     { 1200000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 5), },
+     { 1000000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 4), },
+     {  800000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+     {  500000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+     {  400000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
+     {  200000, E4210_CPU_DIV0(0, 1, 1, 1, 3, 1), E4210_CPU_DIV1(0, 3), },
+     {  0 },
+};

[snip]

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+static const struct exynos_cpuclk_soc_data e4210_clk_soc_data __initconst = {
+     .ops = &exynos_cpuclk_clk_ops,
+     .offset = 0x14200,
+     .data = e4210_armclk_d,
+     .data_size = sizeof(e4210_armclk_d),
+     .type = EXYNOS4210,
+     .pre_rate_cb = exynos4210_cpuclk_pre_rate_change,
+     .post_rate_cb = exynos4210_cpuclk_post_rate_change,
+};

[snip]

Quoted 20 lines

drivers/clk/samsung/clk-cpu.c new file +20 -0

+/**
+ * exynos_register_cpu_clock: register cpu clock with ccf.
+ * @ctx: driver context.
+ * @cluster_id: cpu cluster number to which this clock is connected.
+ * @lookup_id: cpuclk clock output id for the clock controller.
+ * @name: the name of the cpu clock.
+ * @parent: name of the parent clock for cpuclk.
+ * @alt_parent: name of the alternate clock parent.
+ * @np: device tree node pointer of the clock controller.
+ */
+int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
+             unsigned int cluster_id, unsigned int lookup_id,
+             const char *name, const char *parent,
+             const char *alt_parent, struct device_node *np)
+{
+     const struct of_device_id *match;
+     const struct exynos_cpuclk_soc_data *data = NULL;
+
+     if (!np)
+             return -EINVAL;

drivers/clk/samsung/clk-cpu.c line 576

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+
+     match = of_match_node(exynos_cpuclk_ids, np);
+     if (!match)
+             return -EINVAL;
+
+     data = match->data;
+     data += cluster_id;
+     return exynos_cpuclk_register(ctx, lookup_id, name, parent,
+                     alt_parent, np, data);
+}

We now have SoC-specific data hardcoded here, so (as opposed to my earlier comments when we did not have such) it's now reasonable to move such data to SoC-specific source files and then just call exynos_register_cpu_clock() with a pointer to such data. This would also eliminate the not so good idea of indexing internal data array with cluster_id passed as an argument from external code.

Best regards,
Tomasz

To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tomasz,

Thanks for your review comments. I have made most of the changes you have suggested. The suggested modifications which I did not include is marked below.

Quoted 223 lines · Tomasz Figa

Hi Thomas,

Please see my comments inline.

On 14.07.2014 15:38, Thomas Abraham wrote:

[snip]

Quoted 6 lines

drivers/clk/samsung/clk-cpu.c new file +0 -0

new file mode 100644
index 0000000..0d62968

drivers/clk/samsung/clk-cpu.c line 10

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

@@ -0,0 +1,576 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Thomas Abraham <[email protected]>
+ *
+ * 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.

I'd expect few words here or in separate comment on how the code works, i.e. assumptions made, most important procedures, etc. This is a complex piece of code for quite complex hardware, so proper documentation is essential.

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+*/
+
+#include <linux/errno.h>
+#include "clk.h"
+
+#define E4210_SRC_CPU                0x0
+#define E4210_STAT_CPU               0x200
+#define E4210_DIV_CPU0               0x300
+#define E4210_DIV_CPU1               0x304

drivers/clk/samsung/clk-cpu.c line 29

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+#define E4210_DIV_STAT_CPU0  0x400
+#define E4210_DIV_STAT_CPU1  0x404
+
+#define MAX_DIV                      8
+#define DIV_MASK             7
+#define DIV_MASK_ALL         0xffffffff
+#define MUX_MASK             7
+
+#define E4210_DIV0_RATIO0_MASK       0x7
+#define E4210_DIV1_HPM_MASK  ((0x7 << 4) | (0x7 << 0))

This mask contains two fields, doesn't it? I'd say it would be better for readability if you split it.

Quoted 11 lines

drivers/clk/samsung/clk-cpu.c new file +11 -0

+#define E4210_MUX_HPM_MASK   (1 << 20)
+#define E4210_DIV0_ATB_SHIFT 16
+#define E4210_DIV0_ATB_MASK  (DIV_MASK << E4210_DIV0_ATB_SHIFT)
+
+#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0)  \
+             (((apll) << 24) | ((pclk_dbg) << 20) | ((atb) << 16) |  \
+             ((periph) << 12) | ((corem1) << 8) | ((corem0) <<  4))
+#define E4210_CPU_DIV1(hpm, copy)                                    \
+             (((hpm) << 4) | ((copy) << 0))
+
+#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud)               \

drivers/clk/samsung/clk-cpu.c line 50

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+             (((apll << 24) | (pclk_dbg << 20) | (atb << 16) |       \
+              (periph << 12) | (acp << 8) | (cpud << 4)))
+#define E5250_CPU_DIV1(hpm, copy)                                    \
+             (((hpm) << 4) | (copy))
+
+#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud)                    \
+             (((apll << 24) | (pclk_dbg << 20) | (atb << 16) |       \
+              (cpud << 4)))
+#define E5420_KFC_DIV(kpll, pclk, aclk)                                      \
+             (((kpll << 24) | (pclk << 20) | (aclk << 4)))

Again, used macro arguments should always be surrounded with parentheses.

drivers/clk/samsung/clk-cpu.c line 59

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+
+enum cpuclk_type {
+     EXYNOS4210,
+     EXYNOS5250,
+     EXYNOS5420,
+};
+
+/**
+ * struct exynos4210_cpuclk_data: config data to setup cpu clocks.

It seems like this could be used for all Exynos SoCs, so probably should be called exynos_cpuclk_data.

Quoted 27 lines

drivers/clk/samsung/clk-cpu.c new file +27 -0

+ * @prate: frequency of the primary parent clock (in KHz).
+ * @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 dividers in the CPU
+ * clock domain. The parent frequency at which these divider values are valid is
+ * specified in @prate. The @prate is the frequency of the primary parent clock.
+ * For CPU clock domains that do not have a DIV1 register, the @div1 member
+ * is optional.
+ */
+struct exynos4210_cpuclk_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.
+ * @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
+ *   registers can be accessed.
+ * @lock: cpu clock domain register access lock.
+ * @type: type of the CPU clock.
+ * @data: optional data which the actual instantiation of this clock

drivers/clk/samsung/clk-cpu.c line 96

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+ *   can use.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ *   primary parent clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ *   of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
+ *   of the primary parent clock.
+ *
+ * This structure holds information required for programming the cpu clock for
+ * various clock speeds.

nit: s/cpu/CPU/

drivers/clk/samsung/clk-cpu.c line 105

Quoted 9 lines

drivers/clk/samsung/clk-cpu.c new file +9 -0

+ */
+struct exynos_cpuclk {
+     struct clk_hw           hw;
+     struct clk              *alt_parent;
+     void __iomem            *ctrl_base;
+     unsigned long           offset;
+     spinlock_t              *lock;
+     enum cpuclk_type        type;
+     const void              *data;

The code always expects this to be const struct exynos4210_cpuclk_data. Why not make this field so?

Also this is not some plain data, but an array of operating points, so probably a name like "rates" would be better.

drivers/clk/samsung/clk-cpu.c line 112

Quoted 7 lines

drivers/clk/samsung/clk-cpu.c new file +7 -0

+     struct notifier_block   clk_nb;
+     int                     (*pre_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);
+     int                     (*post_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);

All the Exynos SoCs supported by this patch seem to be using exactly the same notifiers. We don't know what changes in further SoCs, so there is no guarantee that having these as pointer here will give us any benefits. I'd recommend just getting rid of this indirection for now. If it turns out to be needed, it will be trivial to add it back.

drivers/clk/samsung/clk-cpu.c line 116

Quoted 4 lines

drivers/clk/samsung/clk-cpu.c new file +4 -0

+};
+
+#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)

Please make these static inlines for type safety.

drivers/clk/samsung/clk-cpu.c line 124

Quoted 8 lines

drivers/clk/samsung/clk-cpu.c new file +8 -0

+
+/**
+ * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
+ * @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.
+ * @data: SoC specific data for cpuclk configuration (optional).

How is this optional? Can this code work without a list of operating points?

drivers/clk/samsung/clk-cpu.c line 125

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+ * @data_size: size of the data contained in @data member.

Both fields could be probably named "rates" and "num_rates", with the meaning of the latter changed to mean the number of entries, not size in bytes.

Quoted 4 lines

drivers/clk/samsung/clk-cpu.c new file +4 -0

+ * @type: type of the CPU clock.
+ * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
+ *   of the primary parent clock.
+ * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification

drivers/clk/samsung/clk-cpu.c line 139

Quoted 10 lines

drivers/clk/samsung/clk-cpu.c new file +10 -0

+ *   of the primary parent clock.
+ *
+ * This structure provides SoC specific data for CPU 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 {
+     const struct clk_ops    *ops;
+     unsigned int            offset;
+     const void              *data;

Same comments as for the data field above.

drivers/clk/samsung/clk-cpu.c line 140

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+     const unsigned int      data_size;

If the same struct is always used it would be clearer to

drivers/clk/samsung/clk-cpu.c line 112

Quoted 7 lines

drivers/clk/samsung/clk-cpu.c new file +7 -0

+     enum cpuclk_type        type;
+     int                     (*pre_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);
+     int                     (*post_rate_cb)(struct clk_notifier_data *,
+                                     struct exynos_cpuclk *,
+                                     void __iomem *base);

Same comment as above.

drivers/clk/samsung/clk-cpu.c line 148

Quoted 1 line

drivers/clk/samsung/clk-cpu.c new file +1 -0

+};

It looks like instead of duplicating most of the fields of this struct in exynos_cpuclk struct the latter could simply have a pointer to an instance of the former.

Quoted 8 lines

drivers/clk/samsung/clk-cpu.c line 153

Quoted 5 lines

drivers/clk/samsung/clk-cpu.c new file +5 -0

+
+/*
+ * Helper function to wait until divider(s) have stabilized after the divider
+ * value has changed.
+ */

How about a kernel doc like comments for functions as well? (same comment for remaining functions)

Since most of these functions are static and very simple, I have not added kernel-doc like comments.

From: Thomas Abraham <[email protected]>

Register the PLL configuration data for APLL and KPLL on Exynos5420. This configuration data table specifies PLL coefficients for supported PLL clock speeds when a 24MHz clock is supplied as the input clock source for these PLLs.

Cc Tomasz Figa <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

---
 drivers/clk/samsung/clk-exynos5420.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

From: Thomas Abraham <[email protected]>

Register cpu clocks using the new cpu-clock provider type for exynos platforms. The differnt clock blocks that are now encapsulated within the cpu-clock can be marked with read-only attribute.

Cc Tomasz Figa <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

---
 drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++++++++++--------
 drivers/clk/samsung/clk-exynos5250.c   |   16 +++++++++++-----
 drivers/clk/samsung/clk-exynos5420.c   |   32 +++++++++++++++++++++++---------
 include/dt-bindings/clock/exynos5250.h |    1 +
 include/dt-bindings/clock/exynos5420.h |    2 ++
 5 files changed, 54 insertions(+), 22 deletions(-)

drivers/clk/samsung/clk-exynos4.c +12 -6

index 75573a4..10f1818 100644
@@ -471,7 +471,8 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
 	MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
 	MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
 	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
-	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
+	MUX_F(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1, 0,
+			CLK_MUX_READ_ONLY),
 	MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
 	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
 	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),

@@ -530,7 +531,8 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
 	MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
 	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
 	MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
-	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
+	MUX_F(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1, 0,
+			CLK_MUX_READ_ONLY),
 	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
 	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
 	MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),

@@ -572,8 +574,10 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
 
 /* list of divider clocks supported in all exynos4 soc's */
 static struct samsung_div_clock exynos4_div_clks[] __initdata = {
-	DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
-	DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
+	DIV_F(0, "div_core", "mout_core", DIV_CPU0, 0, 3,
+			CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
+	DIV_F(0, "div_core2", "div_core", DIV_CPU0, 28, 3,
+			CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
 	DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
 	DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
 	DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),

@@ -619,8 +623,10 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
 	DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
 	DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
 	DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
-	DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
-	DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
+	DIV_F(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3,
+			CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
+	DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
+			CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
 	DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
 			CLK_SET_RATE_PARENT, 0),
 	DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,

drivers/clk/samsung/clk-exynos4.c +5 -1

@@ -1255,6 +1260,8 @@ static void __init exynos4_clk_init(struct device_node *np,
 			ARRAY_SIZE(exynos4210_gate_clks));
 		samsung_clk_register_alias(ctx, exynos4210_aliases,
 			ARRAY_SIZE(exynos4210_aliases));
+		exynos_register_cpu_clock(ctx, 0, CLK_ARM_CLK, "armclk",
+			mout_core_p4210[0], mout_core_p4210[1], np);
 	} else {
 		samsung_clk_register_mux(ctx, exynos4x12_mux_clks,
 			ARRAY_SIZE(exynos4x12_mux_clks));

@@ -1264,6 +1271,8 @@ static void __init exynos4_clk_init(struct device_node *np,
 			ARRAY_SIZE(exynos4x12_gate_clks));
 		samsung_clk_register_alias(ctx, exynos4x12_aliases,
 			ARRAY_SIZE(exynos4x12_aliases));
+		exynos_register_cpu_clock(ctx, 0, CLK_ARM_CLK, "armclk",
+			mout_core_p4x12[0], mout_core_p4x12[1], np);
 	}
 
 	samsung_clk_register_alias(ctx, exynos4_aliases,

@@ -1278,7 +1287,7 @@ static void __init exynos4_clk_init(struct device_node *np,
 		exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
 		_get_rate("sclk_apll"),	_get_rate("sclk_mpll"),
 		_get_rate("sclk_epll"), _get_rate("sclk_vpll"),
-		_get_rate("arm_clk"));
+		_get_rate("armclk"));
 }
 
 

drivers/clk/samsung/clk-exynos5250.c +0 -0

index 5861183..2f78bb0 100644

From: Thomas Abraham <[email protected]>

For Exynos 4210/5250/5420 based platforms, add CPU nodes, operating points and cpu clock data for migrating from Exynos specific cpufreq driver to using generic cpufreq drivers.

arch/arm/boot/dts/exynos4210.dtsi +4 -0

+			clocks = <&clock CLK_ARM_CLK>;
+			clock-names = "cpu";
+
+			operating-points = <

arch/arm/boot/dts/exynos4210.dtsi +2 -0

+	};
+
 	sysram at 02020000 {
 		compatible = "mmio-sram";
 		reg = <0x02020000 0x20000>;

arch/arm/boot/dts/exynos5250.dtsi +13 -0

index 834fb5a..66b0f98 100644
@@ -63,6 +63,29 @@
 			compatible = "arm,cortex-a15";
 			reg = <0>;
 			clock-frequency = <1700000000>;
+
+			clocks = <&clock CLK_ARM_CLK>;
+			clock-names = "cpu";
+
+			operating-points = <
+				1700000 1300000
+				1600000 1250000
+				1500000 1225000
+				1400000 1200000
+				1300000 1150000
+				1200000 1125000
+				1100000 1100000
+				1000000 1075000

Hi Thomas,

Please see my comments inline.

Quoted 5 lines · Thomas Abraham

From: Thomas Abraham <[email protected]>

For Exynos 4210/5250/5420 based platforms, add CPU nodes, operating points and cpu clock data for migrating from Exynos specific cpufreq driver to using generic cpufreq drivers.

[snip]

arch/arm/boot/dts/exynos4210.dtsi line 37

Quoted 12 lines

arch/arm/boot/dts/exynos4210.dtsi +4 -0

index ee3001f..c3a73bf 100644
@@ -31,6 +31,33 @@
              pinctrl2 = &pinctrl_2;
      };
 
+     cpus {
+             #address-cells = <1>;
+             #size-cells = <0>;
+             cpu at 0 {

nit: Missing blank line after last property.

The cluster ID field of MPIDR on Exynos4210 is 0x9 not zero, which means that this should be cpu at 900.

arch/arm/boot/dts/exynos4210.dtsi line 40

Quoted 3 lines

arch/arm/boot/dts/exynos4210.dtsi +3 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a9";
+                     reg = <0>;

                        reg = <0x900>;

Quoted 4 lines

arch/arm/boot/dts/exynos4210.dtsi +4 -0

+                     clocks = <&clock CLK_ARM_CLK>;
+                     clock-names = "cpu";
+
+                     operating-points = <

arch/arm/boot/dts/exynos4210.dtsi line 54

Quoted 10 lines

arch/arm/boot/dts/exynos4210.dtsi +10 -0

+                             1200000 1250000
+                             1000000 1150000
+                             800000  1075000
+                             500000  975000
+                             400000  975000
+                             200000  950000
+                     >;
+             };
+
+             cpu at 1 {

                cpu at 901

arch/arm/boot/dts/exynos4210.dtsi line 57

Quoted 3 lines

arch/arm/boot/dts/exynos4210.dtsi +3 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a9";
+                     reg = <1>;

                        reg = <0x901>;

arch/arm/boot/dts/exynos4210.dtsi line 58

Quoted 1 line

arch/arm/boot/dts/exynos4210.dtsi +1 -0

+             };

In general this wouldn't have even booted, because there were several places where code relied on CPUs being 0, 1, 2... However I have sent necessary fixes and they should hit linux-next in few days.

Quoted 5 lines

arch/arm/boot/dts/exynos4210.dtsi +2 -0

+     };
+
      sysram at 02020000 {
              compatible = "mmio-sram";
              reg = <0x02020000 0x20000>;

[snip]

Quoted 26 lines

arch/arm/boot/dts/exynos5250.dtsi +18 -0

index 834fb5a..66b0f98 100644
@@ -63,6 +63,29 @@
                      compatible = "arm,cortex-a15";
                      reg = <0>;
                      clock-frequency = <1700000000>;
+
+                     clocks = <&clock CLK_ARM_CLK>;
+                     clock-names = "cpu";
+
+                     operating-points = <
+                             1700000 1300000
+                             1600000 1250000
+                             1500000 1225000
+                             1400000 1200000
+                             1300000 1150000
+                             1200000 1125000
+                             1100000 1100000
+                             1000000 1075000
+                              900000 1050000
+                              800000 1025000
+                              700000 1012500
+                              600000 1000000
+                              500000  975000

arch/arm/boot/dts/exynos5250.dtsi line 88

Quoted 5 lines

arch/arm/boot/dts/exynos5250.dtsi +5 -0

+                              400000  950000
+                              300000  937500
+                              200000  925000
+                     >;
+                     clock-latency = <200000>;

I don't see this property specified for Exynos4210. Have you missed it there?

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Thomas Abraham <[email protected]>

Remove the platform device instantiation for exynos cpufreq driver and add the platform device for generic cpufreq drivers.

Cc Kukjin Kim <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

---
 arch/arm/mach-exynos/exynos.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

arch/arm/mach-exynos/exynos.c +0 -0

index 2a43a17..5028b35 100644

Quoted 19 lines · Thomas Abraham

From: Thomas Abraham <[email protected]>

Remove the platform device instantiation for exynos cpufreq driver and add the platform device for generic cpufreq drivers.

Cc Kukjin Kim <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

---
 arch/arm/mach-exynos/exynos.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

arch/arm/mach-exynos/exynos.c +0 -0

index 2a43a17..5028b35 100644

arch/arm/mach-exynos/exynos.c void __init exynos_cpuidle_init(void) · line 189

Quoted 8 lines

arch/arm/mach-exynos/exynos.c +4 -1

@@ -183,7 +183,20 @@ void __init exynos_cpuidle_init(void)
 
 void __init exynos_cpufreq_init(void)
 {
-     platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
+     char *dev_name;
+
+     if (of_machine_is_compatible("samsung,exynos5440"))
+             return;

The original code registers the device unconditionally. Why Exynos5440 is excluded now?

arch/arm/mach-exynos/exynos.c void __init exynos_cpuidle_init(void) · line 200

Quoted 11 lines

arch/arm/mach-exynos/exynos.c +10 -0

+     if (of_machine_is_compatible("samsung,exynos5420"))
+             dev_name = "arm-bL-cpufreq-dt";
+     else
+             if (of_machine_is_compatible("samsung,exynos4412") ||
+                     of_machine_is_compatible("samsung,exynos4212"))
+                     dev_name = "exynos-cpufreq";
+             else
+                     dev_name = "cpufreq-cpu0";
+
+     platform_device_register_simple(dev_name, -1, NULL, 0);
 }

How about rewriting this to:

static const struct of_device_id exynos_cpufreq_matches[] = {
        { .compatible = "samsung,exynos5420",
                        .data = "arm-bL-cpufreq-dt" },
        { .compatible = "samsung,exynos5250",
                        .data = "cpufreq-cpu0" },
        { .compatible = "samsung,exynos4210",
                        .data = "cpufreq-cpu0" },
        { /* sentinel */ }
};

void __init exynos_cpufreq_init(void)

{
        struct device_node *root = of_find_node_by_path("/");
        const struct of_device_id *match;

        match = of_match_node(exynos_cpufreq_matches, root);
        if (!match) {
                platform_device_register_simple("exynos-cpufreq", -1,
                                                NULL, 0);
                return;
        }

        platform_device_register_simple(match->data, -1, NULL, 0);
}

This way it is much more readable and original behavior is preserved for any SoCs not supported by new drivers.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Thomas Abraham <[email protected]>

Exynos4210 and Exynos5250 based platforms have switched over to use generic cpufreq drivers for cpufreq functionality. So the Exynos specific cpufreq drivers for these platforms can be removed.

Signed-off-by Thomas Abraham <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

---
 drivers/cpufreq/Kconfig.arm          |   22 ----
 drivers/cpufreq/Makefile             |    2 -
 drivers/cpufreq/exynos4210-cpufreq.c |  184 -----------------------------
 drivers/cpufreq/exynos5250-cpufreq.c |  210 ----------------------------------
 4 files changed, 418 deletions(-)
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
 delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c

Hi,

Quoted 10 lines · Thomas Abraham

From: Thomas Abraham <[email protected]>

Exynos4210 and Exynos5250 based platforms have switched over to use generic cpufreq drivers for cpufreq functionality. So the Exynos specific cpufreq drivers for these platforms can be removed.

Signed-off-by Thomas Abraham <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Reviewed-by Amit Daniel Kachhap <[email protected]>

Tested-by Arjun K.V <[email protected]>

Quoted 9 lines

---
 drivers/cpufreq/Kconfig.arm          |   22 ----
 drivers/cpufreq/Makefile             |    2 -
 drivers/cpufreq/exynos4210-cpufreq.c |  184 -----------------------------
 drivers/cpufreq/exynos5250-cpufreq.c |  210 ----------------------------------
 4 files changed, 418 deletions(-)
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
 delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c

I would hesitate with removing this driver straightaway and replacing it with a new one that isn't yet well tested. For at least one release I would keep the old code in place with the possibility of switching between old and new one in Kconfig.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Changes since v6:
- Fixes suggested by Amit Daniel <[email protected]>.

This patch series removes the use of Exynos4210 and Exynos5250 specific cpufreq drivers and enables the use of cpufreq-cpu0 driver for these platforms. This series also enabled cpufreq support for Exynos5420 using arm_big_little cpufreq driver.

Thomas Abraham (6):
clk: samsung: add infrastructure to register cpu clocks
clk: samsung: register exynos5420 apll/kpll configuration data
clk: exynos: use cpu-clock provider type to represent arm clock
ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
ARM: Exynos: switch to using generic cpufreq driver for exynos4210/5250
cpufreq: exynos: remove exynos4210/5250 specific cpufreq driver support

 arch/arm/boot/dts/exynos4210-origen.dts         |    6 +
 arch/arm/boot/dts/exynos4210-trats.dts          |    6 +
 arch/arm/boot/dts/exynos4210-universal_c210.dts |    6 +
 arch/arm/boot/dts/exynos4210.dtsi               |   27 ++
 arch/arm/boot/dts/exynos5250-arndale.dts        |    6 +
 arch/arm/boot/dts/exynos5250-cros-common.dtsi   |    6 +
 arch/arm/boot/dts/exynos5250-smdk5250.dts       |    6 +
 arch/arm/boot/dts/exynos5250.dtsi               |   23 +
 arch/arm/boot/dts/exynos5420-smdk5420.dts       |    6 +
 arch/arm/boot/dts/exynos5420.dtsi               |   32 ++
 arch/arm/mach-exynos/exynos.c                   |   15 +-
 drivers/clk/samsung/Makefile                    |    2 +-
 drivers/clk/samsung/clk-cpu.c                   |  576 +++++++++++++++++++++++
 drivers/clk/samsung/clk-exynos4.c               |   25 +-
 drivers/clk/samsung/clk-exynos5250.c            |   16 +-
 drivers/clk/samsung/clk-exynos5420.c            |   60 ++-
 drivers/clk/samsung/clk.h                       |    5 +
 drivers/cpufreq/Kconfig.arm                     |   22 -
 drivers/cpufreq/Makefile                        |    2 -
 drivers/cpufreq/exynos4210-cpufreq.c            |  184 --------
 drivers/cpufreq/exynos5250-cpufreq.c            |  210 ---------
 include/dt-bindings/clock/exynos5250.h          |    1 +
 include/dt-bindings/clock/exynos5420.h          |    2 +
 23 files changed, 802 insertions(+), 442 deletions(-)
 create mode 100644 drivers/clk/samsung/clk-cpu.c
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
 delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c

--
1.7.9.5

Hi Tomasz,

Quoted 8 lines · Thomas Abraham

Changes since v6:
- Fixes suggested by Amit Daniel <[email protected]>.

This patch series removes the use of Exynos4210 and Exynos5250 specific cpufreq drivers and enables the use of cpufreq-cpu0 driver for these platforms. This series also enabled cpufreq support for Exynos5420 using arm_big_little cpufreq driver.

Quoted 7 lines

Thomas Abraham (6):
clk: samsung: add infrastructure to register cpu clocks
clk: samsung: register exynos5420 apll/kpll configuration data
clk: exynos: use cpu-clock provider type to represent arm clock
ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
ARM: Exynos: switch to using generic cpufreq driver for exynos4210/5250
cpufreq: exynos: remove exynos4210/5250 specific cpufreq driver support

In this series, support for Exynos5420 has been included. If there are any more changes required, could you please let me know.

Thanks,
Thomas.

Quoted 32 lines

 arch/arm/boot/dts/exynos4210-origen.dts         |    6 +
 arch/arm/boot/dts/exynos4210-trats.dts          |    6 +
 arch/arm/boot/dts/exynos4210-universal_c210.dts |    6 +
 arch/arm/boot/dts/exynos4210.dtsi               |   27 ++
 arch/arm/boot/dts/exynos5250-arndale.dts        |    6 +
 arch/arm/boot/dts/exynos5250-cros-common.dtsi   |    6 +
 arch/arm/boot/dts/exynos5250-smdk5250.dts       |    6 +
 arch/arm/boot/dts/exynos5250.dtsi               |   23 +
 arch/arm/boot/dts/exynos5420-smdk5420.dts       |    6 +
 arch/arm/boot/dts/exynos5420.dtsi               |   32 ++
 arch/arm/mach-exynos/exynos.c                   |   15 +-
 drivers/clk/samsung/Makefile                    |    2 +-
 drivers/clk/samsung/clk-cpu.c                   |  576 +++++++++++++++++++++++
 drivers/clk/samsung/clk-exynos4.c               |   25 +-
 drivers/clk/samsung/clk-exynos5250.c            |   16 +-
 drivers/clk/samsung/clk-exynos5420.c            |   60 ++-
 drivers/clk/samsung/clk.h                       |    5 +
 drivers/cpufreq/Kconfig.arm                     |   22 -
 drivers/cpufreq/Makefile                        |    2 -
 drivers/cpufreq/exynos4210-cpufreq.c            |  184 --------
 drivers/cpufreq/exynos5250-cpufreq.c            |  210 ---------
 include/dt-bindings/clock/exynos5250.h          |    1 +
 include/dt-bindings/clock/exynos5420.h          |    2 +
 23 files changed, 802 insertions(+), 442 deletions(-)
 create mode 100644 drivers/clk/samsung/clk-cpu.c
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
 delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c

--
1.7.9.5

Hi Mike,

Quoted 19 lines · Thomas Abraham

Hi Tomasz,

Quoted 8 lines · Thomas Abraham

Changes since v6:
- Fixes suggested by Amit Daniel <[email protected]>.

This patch series removes the use of Exynos4210 and Exynos5250 specific cpufreq drivers and enables the use of cpufreq-cpu0 driver for these platforms. This series also enabled cpufreq support for Exynos5420 using arm_big_little cpufreq driver.

Quoted 7 lines

Thomas Abraham (6):
clk: samsung: add infrastructure to register cpu clocks
clk: samsung: register exynos5420 apll/kpll configuration data
clk: exynos: use cpu-clock provider type to represent arm clock
ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
ARM: Exynos: switch to using generic cpufreq driver for exynos4210/5250
cpufreq: exynos: remove exynos4210/5250 specific cpufreq driver support

Quoted 2 lines

In this series, support for Exynos5420 has been included. If there are any more changes required, could you please let me know.

This patch series migrates exynos cpufreq support to use generic cpufreq drivers. For this, cpu clock blocks are encapsulated into a cpu clock type and cpufreq driver operates the cpu clock type.

We had discussed in the [1] about using coordinated clocks. This series is not using the concept of coordinated clocks but the code in this series can be migrated to use coordinated clocks when it is available.

Is it okay to consider this series for v3.17-rc1. I will update this code to use coordinated clocks when it is available. This series helps with cpufreq support for newer Exynos SoCs such as Exynos5420/3250.

[1] http://www.spinics.net/lists/cpufreq/msg10042.html

Hi Thomas,

Quoted 38 lines · Thomas Abraham

Hi Mike,

Quoted 19 lines · Thomas Abraham

Hi Tomasz,

Quoted 8 lines · Thomas Abraham

Changes since v6:
- Fixes suggested by Amit Daniel <[email protected]>.

This patch series removes the use of Exynos4210 and Exynos5250 specific cpufreq drivers and enables the use of cpufreq-cpu0 driver for these platforms. This series also enabled cpufreq support for Exynos5420 using arm_big_little cpufreq driver.

Quoted 7 lines

Thomas Abraham (6):
clk: samsung: add infrastructure to register cpu clocks
clk: samsung: register exynos5420 apll/kpll configuration data
clk: exynos: use cpu-clock provider type to represent arm clock
ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
ARM: Exynos: switch to using generic cpufreq driver for exynos4210/5250
cpufreq: exynos: remove exynos4210/5250 specific cpufreq driver support

Quoted 2 lines

In this series, support for Exynos5420 has been included. If there are any more changes required, could you please let me know.

This patch series migrates exynos cpufreq support to use generic cpufreq drivers. For this, cpu clock blocks are encapsulated into a cpu clock type and cpufreq driver operates the cpu clock type.

We had discussed in the [1] about using coordinated clocks. This series is not using the concept of coordinated clocks but the code in this series can be migrated to use coordinated clocks when it is available.

Is it okay to consider this series for v3.17-rc1. I will update this code to use coordinated clocks when it is available. This series helps with cpufreq support for newer Exynos SoCs such as Exynos5420/3250.

Quoted 2 lines

[1] http://www.spinics.net/lists/cpufreq/msg10042.html

I tested this patchset with Exynos3250 cpufreq patchset[1] on Exynos3250 SoC.
[1] https://lkml.org/lkml/2014/6/18/126

Tested-by Chanwoo Choi <[email protected]>

Best Regards,
Chanwoo Choi

Hello,

I have a question concerning an older version of this patchset, the one which still included exynos4x12 support and where the clocking data was provided through DT.

I tried to get this working for my ODROID-X2, which does work well, including the usage of the boost frequences. Since the X2 has an Exynos4412 Prime I added additional clocking data and operating points.

clocking data (comes from the vendor kernel):
<1704000 3 7 0 6 1 2 7 0 7>
<1600000 3 7 0 6 1 2 6 0 7>

operating points (again from vendor kernel):
1704000 1375000
1600000 1350000

Doing this I get correct output from scaling_available_frequencies and scaling_boost_frequencies and the driver seems to select these frequences. However when reading from cpufreq_cur_freq, which I think returns the frequency of the armclk, the maximum freq never exceeds 1.5GHz. So effectively the additional clocking data and opps are not used.

I wonder this happens? Is there some kind of limit enforced?

Tobias,

Quoted 24 lines · Tobias Jakobi

Hello,

I have a question concerning an older version of this patchset, the one which still included exynos4x12 support and where the clocking data was provided through DT.

I tried to get this working for my ODROID-X2, which does work well, including the usage of the boost frequences. Since the X2 has an Exynos4412 Prime I added additional clocking data and operating points.

clocking data (comes from the vendor kernel):
<1704000 3 7 0 6 1 2 7 0 7>
<1600000 3 7 0 6 1 2 6 0 7>

operating points (again from vendor kernel):
1704000 1375000
1600000 1350000

Doing this I get correct output from scaling_available_frequencies and scaling_boost_frequencies and the driver seems to select these frequences. However when reading from cpufreq_cur_freq, which I think returns the frequency of the armclk, the maximum freq never exceeds 1.5GHz. So effectively the additional clocking data and opps are not used.

Quoted 1 line

I wonder this happens? Is there some kind of limit enforced?

Have you also extended the array of allowed APLL settings in drivers/clk/clk-exynos4.c?

Best regards,
Tomasz