[PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[pi-cheng.chen]

This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
for intermediate clock source switching. This patch is based on Mediatek
clock driver patches[1].

[1] http://thread.gmane.org/gmane.linux.kernel/1892436

Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
---
 drivers/clk/mediatek/Makefile          |   2 +-
 drivers/clk/mediatek/clk-cpumux.c      | 119 +++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-cpumux.h      |  32 +++++++++
 drivers/clk/mediatek/clk-mt8173.c      |  23 +++++++
 drivers/clk/mediatek/clk-mtk.c         |  39 +++++++++++
 drivers/clk/mediatek/clk-mtk.h         |   4 ++
 include/dt-bindings/clock/mt8173-clk.h |   4 +-
 7 files changed, 221 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/mediatek/clk-cpumux.c
 create mode 100644 drivers/clk/mediatek/clk-cpumux.h

diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
index 8e4b2a4..299917a 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -1,4 +1,4 @@
-obj-y += clk-mtk.o clk-pll.o clk-gate.o
+obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o
 obj-$(CONFIG_RESET_CONTROLLER) += reset.o
 obj-y += clk-mt8135.o
 obj-y += clk-mt8173.o
diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
new file mode 100644
index 0000000..2026d56
--- /dev/null
+++ b/drivers/clk/mediatek/clk-cpumux.c
@@ -0,0 +1,119 @@
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
+ *
+ * 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 program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "clk-cpumux.h"
+
+#define ARMPLL_INDEX	1
+#define MAINPLL_INDEX	2
+
+static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw)
+{
+	return container_of(_hw, struct mtk_clk_cpumux, hw);
+}
+
+static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
+				      unsigned long min_rate,
+				      unsigned long max_rate,
+				      unsigned long *best_parent_rate,
+				      struct clk_hw **best_parent_p)
+{
+	struct clk *clk = hw->clk, *parent;
+	unsigned long parent_rate;
+	int i;
+
+	for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
+		parent = clk_get_parent_by_index(clk, i);
+		if (!parent)
+			return 0;
+
+		if (i == MAINPLL_INDEX) {
+			parent_rate = __clk_get_rate(parent);
+			if (parent_rate == rate)
+				break;
+		}
+
+		parent_rate = __clk_round_rate(parent, rate);
+	}
+
+	*best_parent_rate = parent_rate;
+	*best_parent_p = __clk_get_hw(parent);
+	return parent_rate;
+}
+
+static u8 clk_cpumux_get_parent(struct clk_hw *hw)
+{
+	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
+	int num_parents = __clk_get_num_parents(hw->clk);
+	u32 val;
+
+	regmap_read(mux->regmap, mux->reg, &val);
+	val = (val & mux->mask) >> mux->shift;
+
+	if (val >= num_parents)
+		return -EINVAL;
+
+	return val;
+}
+
+static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
+
+	return regmap_update_bits(mux->regmap, mux->reg, mux->mask,
+				  index << mux->shift);
+}
+
+static struct clk_ops clk_cpumux_ops = {
+	.get_parent = clk_cpumux_get_parent,
+	.set_parent = clk_cpumux_set_parent,
+	.determine_rate = clk_cpumux_determine_rate,
+};
+
+struct clk *mtk_clk_register_cpumux(const char *name, const char **parent_names,
+				    u8 num_parents, struct regmap *regmap,
+				    u32 reg, u8 shift, u8 width)
+{
+	struct mtk_clk_cpumux *mux;
+	struct clk *clk;
+	struct clk_init_data init;
+	u32 mask = (BIT(width) - 1) << shift;
+
+	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_cpumux_ops;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+	init.flags = CLK_SET_RATE_PARENT;
+
+	mux->regmap = regmap;
+	mux->reg = reg;
+	mux->shift = shift;
+	mux->mask = mask;
+	mux->hw.init = &init;
+
+	clk = clk_register(NULL, &mux->hw);
+	if (IS_ERR(clk))
+		kfree(mux);
+
+	return clk;
+}
+
diff --git a/drivers/clk/mediatek/clk-cpumux.h b/drivers/clk/mediatek/clk-cpumux.h
new file mode 100644
index 0000000..e1c8369
--- /dev/null
+++ b/drivers/clk/mediatek/clk-cpumux.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
+ *
+ * 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 program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __DRV_CLK_CPUMUX_H
+#define __DRV_CLK_CPUMUX_H
+
+#include <linux/regmap.h>
+
+struct mtk_clk_cpumux {
+	struct clk_hw	hw;
+	struct regmap	*regmap;
+	u32		reg;
+	u32		mask;
+	u8		shift;
+};
+
+struct clk *mtk_clk_register_cpumux(const char *name, const char **parent_names,
+				    u8 num_parents, struct regmap *regmap,
+				    u32 reg, u8 shift, u8 width);
+
+#endif /* __DRV_CLK_CPUMUX_H */
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 014e552..124c7da 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -19,6 +19,7 @@
 
 #include "clk-mtk.h"
 #include "clk-gate.h"
+#include "clk-cpumux.h"
 
 #include <dt-bindings/clock/mt8173-clk.h>
 
@@ -517,6 +518,25 @@ static const char *i2s3_b_ck_parents[] __initconst = {
 	"apll2_div5"
 };
 
+static const char *ca53_parents[] __initconst = {
+	"clk26m",
+	"armca7pll",
+	"mainpll",
+	"univpll"
+};
+
+static const char *ca57_parents[] __initconst = {
+	"clk26m",
+	"armca15pll",
+	"mainpll",
+	"univpll"
+};
+
+static struct mtk_composite cpu_muxes[] __initdata = {
+	MUX(INFRA_CA53SEL, "infra_ca53_sel", ca53_parents, 0x0000, 0, 2),
+	MUX(INFRA_CA57SEL, "infra_ca57_sel", ca57_parents, 0x0000, 2, 2),
+};
+
 static struct mtk_composite top_muxes[] __initdata = {
 	/* CLK_CFG_0 */
 	MUX(TOP_AXI_SEL, "axi_sel", axi_parents, 0x0040, 0, 3),
@@ -745,6 +765,9 @@ static void __init mtk_infrasys_init(struct device_node *node)
 	mtk_clk_register_gates(node, infra_clks, ARRAY_SIZE(infra_clks),
 						clk_data);
 
+	mtk_clk_register_cpumuxes(node, cpu_muxes, ARRAY_SIZE(cpu_muxes),
+				  clk_data);
+
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 	if (r)
 		pr_err("%s(): could not register clock provider: %d\n",
diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
index 2bcade0..5c65cc4 100644
--- a/drivers/clk/mediatek/clk-mtk.c
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -23,6 +23,7 @@
 
 #include "clk-mtk.h"
 #include "clk-gate.h"
+#include "clk-cpumux.h"
 
 struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num)
 {
@@ -193,3 +194,41 @@ err_out:
 
 	return ERR_PTR(ret);
 }
+
+int mtk_clk_register_cpumuxes(struct device_node *node,
+			      struct mtk_composite *clks, int num,
+			      struct clk_onecell_data *clk_data)
+{
+	int i;
+	struct clk *clk;
+	struct regmap *regmap;
+
+	if (!clk_data)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(node);
+	if (IS_ERR(regmap)) {
+		pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
+		       PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	for (i = 0; i < num; i++) {
+		struct mtk_composite *mux = &clks[i];
+
+		clk = mtk_clk_register_cpumux(mux->name, mux->parent_names,
+					      mux->num_parents, regmap,
+					      mux->mux_reg, mux->mux_shift,
+					      mux->mux_width);
+
+		if (IS_ERR(clk)) {
+			pr_err("Failed to register clk %s: %ld\n",
+			       mux->name, PTR_ERR(clk));
+			continue;
+		}
+
+		clk_data->clks[mux->id] = clk;
+	}
+
+	return 0;
+}
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index 8f5190c..f3b1840 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -126,6 +126,10 @@ struct mtk_gate {
 int mtk_clk_register_gates(struct device_node *node, struct mtk_gate *clks, int num,
 		struct clk_onecell_data *clk_data);
 
+int mtk_clk_register_cpumuxes(struct device_node *node,
+			      struct mtk_composite *clks, int num,
+			      struct clk_onecell_data *clk_data);
+
 struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num);
 
 #define HAVE_RST_BAR	BIT(0)
diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
index e648b28..2503b03 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -187,7 +187,9 @@
 #define INFRA_CEC		9
 #define INFRA_PMICSPI		10
 #define INFRA_PMICWRAP		11
-#define INFRA_NR_CLK		12
+#define INFRA_CA53SEL		12
+#define INFRA_CA57SEL		13
+#define INFRA_NR_CLK		14
 
 /* PERI_SYS */
 
-- 
1.9.1

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
> for intermediate clock source switching. This patch is based on Mediatek
> clock driver patches[1].
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
> 
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
> +				      unsigned long min_rate,
> +				      unsigned long max_rate,
> +				      unsigned long *best_parent_rate,
> +				      struct clk_hw **best_parent_p)
> +{
> +	struct clk *clk = hw->clk, *parent;
> +	unsigned long parent_rate;
> +	int i;
> +
> +	for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
> +		parent = clk_get_parent_by_index(clk, i);
> +		if (!parent)
> +			return 0;
> +
> +		if (i == MAINPLL_INDEX) {
> +			parent_rate = __clk_get_rate(parent);
> +			if (parent_rate == rate)
> +				break;
> +		}
> +
> +		parent_rate = __clk_round_rate(parent, rate);
> +	}
> +
> +	*best_parent_rate = parent_rate;
> +	*best_parent_p = __clk_get_hw(parent);
> +	return parent_rate;
> +}

Why this determine_rate hook? If you want to switch the clock to some
intermediate parent I would assume you do this explicitly by setting the
parent and not implicitly by setting a rate.

> +int mtk_clk_register_cpumuxes(struct device_node *node,
> +			      struct mtk_composite *clks, int num,
> +			      struct clk_onecell_data *clk_data)
> +{
> +	int i;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	regmap = syscon_node_to_regmap(node);
> +	if (IS_ERR(regmap)) {
> +		pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
> +		       PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		struct mtk_composite *mux = &clks[i];
> +
> +		clk = mtk_clk_register_cpumux(mux->name, mux->parent_names,
> +					      mux->num_parents, regmap,
> +					      mux->mux_reg, mux->mux_shift,
> +					      mux->mux_width);

Pass 'mux' directly instead of dispatching this struct into the
individual fields.
Also, probably better to move this function to
drivers/clk/mediatek/clk-cpumux.c

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

Hi Sascha,

On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
>> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
>> for intermediate clock source switching. This patch is based on Mediatek
>> clock driver patches[1].
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
>> +                                   unsigned long min_rate,
>> +                                   unsigned long max_rate,
>> +                                   unsigned long *best_parent_rate,
>> +                                   struct clk_hw **best_parent_p)
>> +{
>> +     struct clk *clk = hw->clk, *parent;
>> +     unsigned long parent_rate;
>> +     int i;
>> +
>> +     for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
>> +             parent = clk_get_parent_by_index(clk, i);
>> +             if (!parent)
>> +                     return 0;
>> +
>> +             if (i == MAINPLL_INDEX) {
>> +                     parent_rate = __clk_get_rate(parent);
>> +                     if (parent_rate == rate)
>> +                             break;
>> +             }
>> +
>> +             parent_rate = __clk_round_rate(parent, rate);
>> +     }
>> +
>> +     *best_parent_rate = parent_rate;
>> +     *best_parent_p = __clk_get_hw(parent);
>> +     return parent_rate;
>> +}
>
> Why this determine_rate hook? If you want to switch the clock to some
> intermediate parent I would assume you do this explicitly by setting the
> parent and not implicitly by setting a rate.
>

I use determine_rate hook here because I am using generic cpufreq-dt
driver and I want to make clock switching transparent to cpufreq-dt.
I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I
call clk_set_rate() with the rate of MAINPLL, and determine_rate will
select MAINPLL as the new parent.

>> +int mtk_clk_register_cpumuxes(struct device_node *node,
>> +                           struct mtk_composite *clks, int num,
>> +                           struct clk_onecell_data *clk_data)
>> +{
>> +     int i;
>> +     struct clk *clk;
>> +     struct regmap *regmap;
>> +
>> +     if (!clk_data)
>> +             return -ENOMEM;
>> +
>> +     regmap = syscon_node_to_regmap(node);
>> +     if (IS_ERR(regmap)) {
>> +             pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
>> +                    PTR_ERR(regmap));
>> +             return PTR_ERR(regmap);
>> +     }
>> +
>> +     for (i = 0; i < num; i++) {
>> +             struct mtk_composite *mux = &clks[i];
>> +
>> +             clk = mtk_clk_register_cpumux(mux->name, mux->parent_names,
>> +                                           mux->num_parents, regmap,
>> +                                           mux->mux_reg, mux->mux_shift,
>> +                                           mux->mux_width);
>
> Pass 'mux' directly instead of dispatching this struct into the
> individual fields.
> Also, probably better to move this function to
> drivers/clk/mediatek/clk-cpumux.c

Will do it. Thanks for reviewing.

Best Regards,
Pi-Cheng

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

+Cc Viresh Kumar

Viresh, this is the patch for the underlying clocks for the Mediatek
cpufreq driver.

On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
> Hi Sascha,
> 
> On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
> >> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
> >> for intermediate clock source switching. This patch is based on Mediatek
> >> clock driver patches[1].
> >>
> >> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
> >>
> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> >> ---
> >> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
> >> +                                   unsigned long min_rate,
> >> +                                   unsigned long max_rate,
> >> +                                   unsigned long *best_parent_rate,
> >> +                                   struct clk_hw **best_parent_p)
> >> +{
> >> +     struct clk *clk = hw->clk, *parent;
> >> +     unsigned long parent_rate;
> >> +     int i;
> >> +
> >> +     for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
> >> +             parent = clk_get_parent_by_index(clk, i);
> >> +             if (!parent)
> >> +                     return 0;
> >> +
> >> +             if (i == MAINPLL_INDEX) {
> >> +                     parent_rate = __clk_get_rate(parent);
> >> +                     if (parent_rate == rate)
> >> +                             break;
> >> +             }
> >> +
> >> +             parent_rate = __clk_round_rate(parent, rate);
> >> +     }
> >> +
> >> +     *best_parent_rate = parent_rate;
> >> +     *best_parent_p = __clk_get_hw(parent);
> >> +     return parent_rate;
> >> +}
> >
> > Why this determine_rate hook? If you want to switch the clock to some
> > intermediate parent I would assume you do this explicitly by setting the
> > parent and not implicitly by setting a rate.
> >
> 
> I use determine_rate hook here because I am using generic cpufreq-dt
> driver and I want to make clock switching transparent to cpufreq-dt.
> I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I
> call clk_set_rate() with the rate of MAINPLL, and determine_rate will
> select MAINPLL as the new parent.

We have clk_set_parent for changing the parent and clk_set_rate to
change the rate. Use the former for changing the parent and the latter
for changing the rate. What you are interested in is changing the
parent, so use clk_set_parent for this and not abuse a side effect
of clk_set_rate.

My suggestion is to take another approach. Implement clk_set_rate for
these muxes and in the set_rate hook:

- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL

This way the things happening behind the scenes are completely transparent
to the cpufreq driver and you can use cpufreq-dt as is without changes.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On 5 March 2015 at 15:42, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> +Cc Viresh Kumar
>
> Viresh, this is the patch for the underlying clocks for the Mediatek
> cpufreq driver.
>
> On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
>> Hi Sascha,
>>
>> On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
>> >> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
>> >> for intermediate clock source switching. This patch is based on Mediatek
>> >> clock driver patches[1].
>> >>
>> >> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
>> >>
>> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> >> ---
>> >> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
>> >> +                                   unsigned long min_rate,
>> >> +                                   unsigned long max_rate,
>> >> +                                   unsigned long *best_parent_rate,
>> >> +                                   struct clk_hw **best_parent_p)
>> >> +{
>> >> +     struct clk *clk = hw->clk, *parent;
>> >> +     unsigned long parent_rate;
>> >> +     int i;
>> >> +
>> >> +     for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
>> >> +             parent = clk_get_parent_by_index(clk, i);
>> >> +             if (!parent)
>> >> +                     return 0;
>> >> +
>> >> +             if (i == MAINPLL_INDEX) {
>> >> +                     parent_rate = __clk_get_rate(parent);
>> >> +                     if (parent_rate == rate)
>> >> +                             break;
>> >> +             }
>> >> +
>> >> +             parent_rate = __clk_round_rate(parent, rate);
>> >> +     }
>> >> +
>> >> +     *best_parent_rate = parent_rate;
>> >> +     *best_parent_p = __clk_get_hw(parent);
>> >> +     return parent_rate;
>> >> +}
>> >
>> > Why this determine_rate hook? If you want to switch the clock to some
>> > intermediate parent I would assume you do this explicitly by setting the
>> > parent and not implicitly by setting a rate.
>> >
>>
>> I use determine_rate hook here because I am using generic cpufreq-dt
>> driver and I want to make clock switching transparent to cpufreq-dt.
>> I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I
>> call clk_set_rate() with the rate of MAINPLL, and determine_rate will
>> select MAINPLL as the new parent.
>
> We have clk_set_parent for changing the parent and clk_set_rate to
> change the rate. Use the former for changing the parent and the latter
> for changing the rate. What you are interested in is changing the
> parent, so use clk_set_parent for this and not abuse a side effect
> of clk_set_rate.
>
> My suggestion is to take another approach. Implement clk_set_rate for
> these muxes and in the set_rate hook:
>
> - switch mux to intermediate PLL parent
> - call clk_set_rate() for the real parent PLL
> - switch mux back to real parent PLL
>
> This way the things happening behind the scenes are completely transparent
> to the cpufreq driver and you can use cpufreq-dt as is without changes.

Hi,

Yes. But the frequency of intermediate PLL parent is different from the original
frequency and the frequency after changed of original PLL clock in most cases.
If the frequency of intermediate PLL parent is higher than the
original frequency,
then we have to scale up the voltage first before switch mux to intermediate PLL
parent. I am not sure this could be properly handled if I implement it
in the way
you suggested. For now, I put the mux switching logic in the determine_rate
hook so that the voltage scaling could be handled by target_intermediate hook
of cpufreq-dt which I am also working on[1].

[1] http://marc.info/?l=linux-kernel&m=142545898313351&w=2

Best Regards,
Pi-Cheng

>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Viresh Kumar]

On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> We have clk_set_parent for changing the parent and clk_set_rate to
> change the rate. Use the former for changing the parent and the latter
> for changing the rate. What you are interested in is changing the
> parent, so use clk_set_parent for this and not abuse a side effect
> of clk_set_rate.

clk_set_rate() for CPUs clock is responsible to change clock rate
of the CPU. Whether it plays with PLLs or muxes, its not that relevant.

> My suggestion is to take another approach. Implement clk_set_rate for
> these muxes and in the set_rate hook:
>
> - switch mux to intermediate PLL parent
> - call clk_set_rate() for the real parent PLL
> - switch mux back to real parent PLL
>
> This way the things happening behind the scenes are completely transparent
> to the cpufreq driver and you can use cpufreq-dt as is without changes.

CPUFreq wants to change to intermediate frequency by itself against
some magic change behind the scene. The major requirement for that
comes from the fact that we want to send PRE/POST freq notifiers on
which loops-per-jiffie depends.

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > We have clk_set_parent for changing the parent and clk_set_rate to
> > change the rate. Use the former for changing the parent and the latter
> > for changing the rate. What you are interested in is changing the
> > parent, so use clk_set_parent for this and not abuse a side effect
> > of clk_set_rate.
> 
> clk_set_rate() for CPUs clock is responsible to change clock rate
> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.

The sequence to change the CPU frequency on the Mediatek SoCs is like this:

- Change CPU from CPU PLL to another clock source (intermediate source)
- Change CPU PLL frequency
- wait until PLL has settled
- switch back to CPU PLL

The frequency of the intermediate source is irrelevant, the important
thing is that the CPU is switched to this source while the CPU PLL is
reconfigured.

In Pi-Chengs patches the switch to th eintermediate clock is done like:

	rate = clk_get_rate(intermediate_clk);
	clk_set_rate(cpu_clk, rate);

Now the clk framework does the switch not because it's told to switch
to another parent, but only because the other parent happens to be the
only provider for that rate. That's rubbish, when the parent must be
changed, then it should be done explicitly.
What if the CPU PLL and the intermediate clk happen to have the same
rate? Then the clk_set_rate above simply does nothing, no parent is
changed and the following rate change of the CPU PLL just crashes the
system.

> 
> > My suggestion is to take another approach. Implement clk_set_rate for
> > these muxes and in the set_rate hook:
> >
> > - switch mux to intermediate PLL parent
> > - call clk_set_rate() for the real parent PLL
> > - switch mux back to real parent PLL
> >
> > This way the things happening behind the scenes are completely transparent
> > to the cpufreq driver and you can use cpufreq-dt as is without changes.
> 
> CPUFreq wants to change to intermediate frequency by itself against
> some magic change behind the scene. The major requirement for that
> comes from the fact that we want to send PRE/POST freq notifiers on
> which loops-per-jiffie depends.

Maybe, I don't know the internals of CPUFreq. But here the important
thing is the rate change, not the parent change.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On 5 March 2015 at 17:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
>> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > We have clk_set_parent for changing the parent and clk_set_rate to
>> > change the rate. Use the former for changing the parent and the latter
>> > for changing the rate. What you are interested in is changing the
>> > parent, so use clk_set_parent for this and not abuse a side effect
>> > of clk_set_rate.
>>
>> clk_set_rate() for CPUs clock is responsible to change clock rate
>> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
>
> The sequence to change the CPU frequency on the Mediatek SoCs is like this:
>
> - Change CPU from CPU PLL to another clock source (intermediate source)
> - Change CPU PLL frequency
> - wait until PLL has settled
> - switch back to CPU PLL
>
> The frequency of the intermediate source is irrelevant, the important
> thing is that the CPU is switched to this source while the CPU PLL is
> reconfigured.
>
> In Pi-Chengs patches the switch to th eintermediate clock is done like:
>
>         rate = clk_get_rate(intermediate_clk);
>         clk_set_rate(cpu_clk, rate);
>
> Now the clk framework does the switch not because it's told to switch
> to another parent, but only because the other parent happens to be the
> only provider for that rate. That's rubbish, when the parent must be
> changed, then it should be done explicitly.

Hi,

Please correct me if I was wrong. But I think that's the reason why we have
a determine_rate hook here, isn't it?

> What if the CPU PLL and the intermediate clk happen to have the same
> rate? Then the clk_set_rate above simply does nothing, no parent is
> changed and the following rate change of the CPU PLL just crashes the
> system.

I think what I am trying to do in the determine_rate hook of cpumux is:

* if the target rate of clk_set_rate is equal to the rate of MAINPLL,
then switch
  the mux to MAINPLL
* otherwise, set the rate directly on ARMPLL and switch the mux back to
  ARMPLL if the current parent of mux is not ARMPLL

And if the CPU PLL and the intermediate clk happen to have the same rate,
I think the cpufreq-dt driver will change nothing on the clk framework. Or do
I misunderstand your point?

Thanks.

Best Regards,
Pi-Cheng

>
>>
>> > My suggestion is to take another approach. Implement clk_set_rate for
>> > these muxes and in the set_rate hook:
>> >
>> > - switch mux to intermediate PLL parent
>> > - call clk_set_rate() for the real parent PLL
>> > - switch mux back to real parent PLL
>> >
>> > This way the things happening behind the scenes are completely transparent
>> > to the cpufreq driver and you can use cpufreq-dt as is without changes.
>>
>> CPUFreq wants to change to intermediate frequency by itself against
>> some magic change behind the scene. The major requirement for that
>> comes from the fact that we want to send PRE/POST freq notifiers on
>> which loops-per-jiffie depends.
>
> Maybe, I don't know the internals of CPUFreq. But here the important
> thing is the rate change, not the parent change.
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Viresh Kumar]

On 5 March 2015 at 14:49, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The sequence to change the CPU frequency on the Mediatek SoCs is like this:
>
> - Change CPU from CPU PLL to another clock source (intermediate source)
> - Change CPU PLL frequency
> - wait until PLL has settled
> - switch back to CPU PLL

This should be the case for most of the intermediate-freq users..

> The frequency of the intermediate source is irrelevant, the important
> thing is that the CPU is switched to this source while the CPU PLL is
> reconfigured.

Right.

> In Pi-Chengs patches the switch to th eintermediate clock is done like:
>
>         rate = clk_get_rate(intermediate_clk);
>         clk_set_rate(cpu_clk, rate);
>
> Now the clk framework does the switch not because it's told to switch
> to another parent, but only because the other parent happens to be the
> only provider for that rate. That's rubbish, when the parent must be
> changed, then it should be done explicitly.
> What if the CPU PLL and the intermediate clk happen to have the same
> rate? Then the clk_set_rate above simply does nothing, no parent is
> changed and the following rate change of the CPU PLL just crashes the
> system.

The problem is that the code is common across platforms that need to
reparent or just change rate for intermediate clocks. And the best we
can do is clk_set_rate() and so probably the clk driver need to take care
of this somehow and make sure we don't result in a crash like you just
demonstrated.

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

On Thu, Mar 05, 2015 at 05:39:12PM +0800, Pi-Cheng Chen wrote:
> On 5 March 2015 at 17:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
> >> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > We have clk_set_parent for changing the parent and clk_set_rate to
> >> > change the rate. Use the former for changing the parent and the latter
> >> > for changing the rate. What you are interested in is changing the
> >> > parent, so use clk_set_parent for this and not abuse a side effect
> >> > of clk_set_rate.
> >>
> >> clk_set_rate() for CPUs clock is responsible to change clock rate
> >> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
> >
> > The sequence to change the CPU frequency on the Mediatek SoCs is like this:
> >
> > - Change CPU from CPU PLL to another clock source (intermediate source)
> > - Change CPU PLL frequency
> > - wait until PLL has settled
> > - switch back to CPU PLL
> >
> > The frequency of the intermediate source is irrelevant, the important
> > thing is that the CPU is switched to this source while the CPU PLL is
> > reconfigured.
> >
> > In Pi-Chengs patches the switch to th eintermediate clock is done like:
> >
> >         rate = clk_get_rate(intermediate_clk);
> >         clk_set_rate(cpu_clk, rate);
> >
> > Now the clk framework does the switch not because it's told to switch
> > to another parent, but only because the other parent happens to be the
> > only provider for that rate. That's rubbish, when the parent must be
> > changed, then it should be done explicitly.
> 
> Hi,
> 
> Please correct me if I was wrong. But I think that's the reason why we have
> a determine_rate hook here, isn't it?
> 
> > What if the CPU PLL and the intermediate clk happen to have the same
> > rate? Then the clk_set_rate above simply does nothing, no parent is
> > changed and the following rate change of the CPU PLL just crashes the
> > system.
> 
> I think what I am trying to do in the determine_rate hook of cpumux is:
> 
> * if the target rate of clk_set_rate is equal to the rate of MAINPLL,
> then switch
>   the mux to MAINPLL
> * otherwise, set the rate directly on ARMPLL and switch the mux back to
>   ARMPLL if the current parent of mux is not ARMPLL
> 
> And if the CPU PLL and the intermediate clk happen to have the same rate,
> I think the cpufreq-dt driver will change nothing on the clk framework. Or do
> I misunderstand your point?

Imagine the board starts with both the CPU PLL and Intermediate PLL
running with the same frequency with the CPU driven from the CPU PLL.
Now the cpufreq driver does it's very first frequency transition. Now
when clk_set_rate is used with the intention to switch to the
intermediate PLL nothing will happen because the CPU PLL already runs
at the desired frequency. Reconfiguring the CPU PLL then crashes your
system.

Another reason why abusing clk_set_rate to change the parent is this:
You have this in your SoC:

            ---
CPU_PLL ---|   |
           |   |
           |   |----- CPU
           |   |
IM_PLL ----|   |
            ---

Now you do a clk_set_rate(CPU, clk_get_rate(IM_PLL)) which (often) works
in your case. Many SoCs have an additional divider though, like this:

            ---
CPU_PLL ---|   |
           |   |   ----
           |   |--| :x | --- CPU
           |   |   ----
IM_PLL ----|   |
            ---

Now clk_set_rate(CPU, clk_get_rate(IM_PLL)) doesn't give anything
sensible anymore.

Given the variance of different SoCs I don't think it makes sense
to try to handle all these cases. Instead the cpufreq-dt driver
should just call clk_set_rate() on the CPU clock with the desired
target frequency. Everything else should be handled in the clock
driver which has intimate knowledge about the SoC anyway.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > We have clk_set_parent for changing the parent and clk_set_rate to
> > change the rate. Use the former for changing the parent and the latter
> > for changing the rate. What you are interested in is changing the
> > parent, so use clk_set_parent for this and not abuse a side effect
> > of clk_set_rate.
> 
> clk_set_rate() for CPUs clock is responsible to change clock rate
> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
> 
> > My suggestion is to take another approach. Implement clk_set_rate for
> > these muxes and in the set_rate hook:
> >
> > - switch mux to intermediate PLL parent
> > - call clk_set_rate() for the real parent PLL
> > - switch mux back to real parent PLL
> >
> > This way the things happening behind the scenes are completely transparent
> > to the cpufreq driver and you can use cpufreq-dt as is without changes.
> 
> CPUFreq wants to change to intermediate frequency by itself against
> some magic change behind the scene. The major requirement for that
> comes from the fact that we want to send PRE/POST freq notifiers on
> which loops-per-jiffie depends.

You could register a clk notifier using clk_notifier_register(). The
notifer callback can then call the cpufreq notifiers. The clk notifiers
can also be used to change the CPU voltage.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Viresh Kumar]

On 5 March 2015 at 16:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Given the variance of different SoCs I don't think it makes sense
> to try to handle all these cases. Instead the cpufreq-dt driver
> should just call clk_set_rate() on the CPU clock with the desired
> target frequency. Everything else should be handled in the clock
> driver which has intimate knowledge about the SoC anyway.

I agree..

@Russell: I wanted to ask you this since sometime..

On CPU-freq changes we fire PRE/POST notifiers and they are
used for updating loops_per_jiffies which then controls delays.

Now, it is fine to do that for normal frequencies, but what should be
the approach for intermediate frequencies ?

Intermediate freqs: On some platforms changing PLL's straight away
isn't considered safe and so we switch parent to another stable clock,
change PLL rate and switch back.

The *wild* thought I earlier had was to fire these notifiers for even these
intermediate frequencies, otherwise some of the delays will end before
they should have and that *might* cause other problems.

I wanted to know what do you (and other champs) think about this..

Thanks in advance for your advice.

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On 5 March 2015 at 18:51, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Mar 05, 2015 at 05:39:12PM +0800, Pi-Cheng Chen wrote:
>> On 5 March 2015 at 17:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Thu, Mar 05, 2015 at 02:29:50PM +0530, Viresh Kumar wrote:
>> >> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> > We have clk_set_parent for changing the parent and clk_set_rate to
>> >> > change the rate. Use the former for changing the parent and the latter
>> >> > for changing the rate. What you are interested in is changing the
>> >> > parent, so use clk_set_parent for this and not abuse a side effect
>> >> > of clk_set_rate.
>> >>
>> >> clk_set_rate() for CPUs clock is responsible to change clock rate
>> >> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.
>> >
>> > The sequence to change the CPU frequency on the Mediatek SoCs is like this:
>> >
>> > - Change CPU from CPU PLL to another clock source (intermediate source)
>> > - Change CPU PLL frequency
>> > - wait until PLL has settled
>> > - switch back to CPU PLL
>> >
>> > The frequency of the intermediate source is irrelevant, the important
>> > thing is that the CPU is switched to this source while the CPU PLL is
>> > reconfigured.
>> >
>> > In Pi-Chengs patches the switch to th eintermediate clock is done like:
>> >
>> >         rate = clk_get_rate(intermediate_clk);
>> >         clk_set_rate(cpu_clk, rate);
>> >
>> > Now the clk framework does the switch not because it's told to switch
>> > to another parent, but only because the other parent happens to be the
>> > only provider for that rate. That's rubbish, when the parent must be
>> > changed, then it should be done explicitly.
>>
>> Hi,
>>
>> Please correct me if I was wrong. But I think that's the reason why we have
>> a determine_rate hook here, isn't it?
>>
>> > What if the CPU PLL and the intermediate clk happen to have the same
>> > rate? Then the clk_set_rate above simply does nothing, no parent is
>> > changed and the following rate change of the CPU PLL just crashes the
>> > system.
>>
>> I think what I am trying to do in the determine_rate hook of cpumux is:
>>
>> * if the target rate of clk_set_rate is equal to the rate of MAINPLL,
>> then switch
>>   the mux to MAINPLL
>> * otherwise, set the rate directly on ARMPLL and switch the mux back to
>>   ARMPLL if the current parent of mux is not ARMPLL
>>
>> And if the CPU PLL and the intermediate clk happen to have the same rate,
>> I think the cpufreq-dt driver will change nothing on the clk framework. Or do
>> I misunderstand your point?
>
> Imagine the board starts with both the CPU PLL and Intermediate PLL
> running with the same frequency with the CPU driven from the CPU PLL.
> Now the cpufreq driver does it's very first frequency transition. Now
> when clk_set_rate is used with the intention to switch to the
> intermediate PLL nothing will happen because the CPU PLL already runs
> at the desired frequency. Reconfiguring the CPU PLL then crashes your
> system.
>
> Another reason why abusing clk_set_rate to change the parent is this:
> You have this in your SoC:
>
>             ---
> CPU_PLL ---|   |
>            |   |
>            |   |----- CPU
>            |   |
> IM_PLL ----|   |
>             ---
>
> Now you do a clk_set_rate(CPU, clk_get_rate(IM_PLL)) which (often) works
> in your case. Many SoCs have an additional divider though, like this:
>
>             ---
> CPU_PLL ---|   |
>            |   |   ----
>            |   |--| :x | --- CPU
>            |   |   ----
> IM_PLL ----|   |
>             ---
>
> Now clk_set_rate(CPU, clk_get_rate(IM_PLL)) doesn't give anything
> sensible anymore.
>
> Given the variance of different SoCs I don't think it makes sense
> to try to handle all these cases. Instead the cpufreq-dt driver
> should just call clk_set_rate() on the CPU clock with the desired
> target frequency. Everything else should be handled in the clock
> driver which has intimate knowledge about the SoC anyway.

Now I got it. Thanks for your elaboration. I will investigate the way
you suggested to implement intermediate clock switching.

Best Regards,
Pi-Cheng

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On 5 March 2015 at 15:42, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> +Cc Viresh Kumar
>
> Viresh, this is the patch for the underlying clocks for the Mediatek
> cpufreq driver.
>
> On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
>> Hi Sascha,
>>
>> On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
>> >> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
>> >> for intermediate clock source switching. This patch is based on Mediatek
>> >> clock driver patches[1].
>> >>
>> >> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
>> >>
>> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> >> ---
>> >> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
>> >> +                                   unsigned long min_rate,
>> >> +                                   unsigned long max_rate,
>> >> +                                   unsigned long *best_parent_rate,
>> >> +                                   struct clk_hw **best_parent_p)
>> >> +{
>> >> +     struct clk *clk = hw->clk, *parent;
>> >> +     unsigned long parent_rate;
>> >> +     int i;
>> >> +
>> >> +     for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
>> >> +             parent = clk_get_parent_by_index(clk, i);
>> >> +             if (!parent)
>> >> +                     return 0;
>> >> +
>> >> +             if (i == MAINPLL_INDEX) {
>> >> +                     parent_rate = __clk_get_rate(parent);
>> >> +                     if (parent_rate == rate)
>> >> +                             break;
>> >> +             }
>> >> +
>> >> +             parent_rate = __clk_round_rate(parent, rate);
>> >> +     }
>> >> +
>> >> +     *best_parent_rate = parent_rate;
>> >> +     *best_parent_p = __clk_get_hw(parent);
>> >> +     return parent_rate;
>> >> +}
>> >
>> > Why this determine_rate hook? If you want to switch the clock to some
>> > intermediate parent I would assume you do this explicitly by setting the
>> > parent and not implicitly by setting a rate.
>> >
>>
>> I use determine_rate hook here because I am using generic cpufreq-dt
>> driver and I want to make clock switching transparent to cpufreq-dt.
>> I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I
>> call clk_set_rate() with the rate of MAINPLL, and determine_rate will
>> select MAINPLL as the new parent.
>
> We have clk_set_parent for changing the parent and clk_set_rate to
> change the rate. Use the former for changing the parent and the latter
> for changing the rate. What you are interested in is changing the
> parent, so use clk_set_parent for this and not abuse a side effect
> of clk_set_rate.
>
> My suggestion is to take another approach. Implement clk_set_rate for
> these muxes and in the set_rate hook:
>
> - switch mux to intermediate PLL parent
> - call clk_set_rate() for the real parent PLL
> - switch mux back to real parent PLL

Hi Sascha,

Thanks for your suggestion. I've tried to take this approach, but there's some
issues here.

Calling clk_set_rate() inside the set_rate callback of cpumux will cause
an infinite recursive calling in the clock framework:
mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...

I've also tries to update pll register settings in the set_rate()
callback of cpumux,
but the PLL clock information will not be correctly updated in this case.

How do you think to create a new "CPU PLL" type of clock and do underlying
mux switching in the set_rate callback of "CPU PLL"?

Best Regards,
Pi-Cheng

>
> This way the things happening behind the scenes are completely transparent
> to the cpufreq driver and you can use cpufreq-dt as is without changes.
>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer]

On Tue, Mar 10, 2015 at 09:53:19AM +0800, Pi-Cheng Chen wrote:
> On 5 March 2015 at 15:42, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > My suggestion is to take another approach. Implement clk_set_rate for
> > these muxes and in the set_rate hook:
> >
> > - switch mux to intermediate PLL parent
> > - call clk_set_rate() for the real parent PLL
> > - switch mux back to real parent PLL
> 
> Hi Sascha,
> 
> Thanks for your suggestion. I've tried to take this approach, but there's some
> issues here.
> 
> Calling clk_set_rate() inside the set_rate callback of cpumux will cause
> an infinite recursive calling in the clock framework:
> mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...

I don't understand why setting the PLL rate should call into the mux
set_rate. Are you sure you call clk_set_rate for the mux parent clk?
I think the general approach should work, drivers/clk/sirf/clk-common.c
does something similar in cpu_clk_set_rate(). If you like you can send
me your work in progress state privatly, I'll have a look then.

> 
> I've also tries to update pll register settings in the set_rate()
> callback of cpumux,
> but the PLL clock information will not be correctly updated in this case.

No, that won't work.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Mike Turquette]

Quoting Viresh Kumar (2015-03-05 00:59:50)
> On 5 March 2015 at 13:12, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > We have clk_set_parent for changing the parent and clk_set_rate to
> > change the rate. Use the former for changing the parent and the latter
> > for changing the rate. What you are interested in is changing the
> > parent, so use clk_set_parent for this and not abuse a side effect
> > of clk_set_rate.
> 
> clk_set_rate() for CPUs clock is responsible to change clock rate
> of the CPU. Whether it plays with PLLs or muxes, its not that relevant.

Agreed.

> 
> > My suggestion is to take another approach. Implement clk_set_rate for
> > these muxes and in the set_rate hook:
> >
> > - switch mux to intermediate PLL parent
> > - call clk_set_rate() for the real parent PLL
> > - switch mux back to real parent PLL
> >
> > This way the things happening behind the scenes are completely transparent
> > to the cpufreq driver and you can use cpufreq-dt as is without changes.
> 
> CPUFreq wants to change to intermediate frequency by itself against
> some magic change behind the scene. The major requirement for that
> comes from the fact that we want to send PRE/POST freq notifiers on
> which loops-per-jiffie depends.

I assume you are saying that you want to update loops-per-jiffie while
at an intermediate frequency. Why? This operation should not take very
long.

Imagine a (hypothetical?) processor that changes frequency in many small
steps until it converges to the target rate. Would you want to update
lpj for every step?

Regards,
Mike

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Mike Turquette]

Quoting Viresh Kumar (2015-03-05 03:02:06)
> On 5 March 2015 at 16:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Given the variance of different SoCs I don't think it makes sense
> > to try to handle all these cases. Instead the cpufreq-dt driver
> > should just call clk_set_rate() on the CPU clock with the desired
> > target frequency. Everything else should be handled in the clock
> > driver which has intimate knowledge about the SoC anyway.
> 
> I agree..
> 
> @Russell: I wanted to ask you this since sometime..
> 
> On CPU-freq changes we fire PRE/POST notifiers and they are
> used for updating loops_per_jiffies which then controls delays.
> 
> Now, it is fine to do that for normal frequencies, but what should be
> the approach for intermediate frequencies ?
> 
> Intermediate freqs: On some platforms changing PLL's straight away
> isn't considered safe and so we switch parent to another stable clock,
> change PLL rate and switch back.
> 
> The *wild* thought I earlier had was to fire these notifiers for even these
> intermediate frequencies, otherwise some of the delays will end before
> they should have and that *might* cause other problems.
> 
> I wanted to know what do you (and other champs) think about this..
> 
> Thanks in advance for your advice.

Sorry, I am not who you asked for advice but I will chime in anyways ;-)

I really hate this intermediate frequency stuff in cpufreq. As we
discussed over lunch in Hong Kong, it does not scale. What if there two
intermediate frequencies? What if a processor steps frequency up in 5KHz
increments (albeit very quickly) until it converges to the target rate?

Or what if we have more complex levels of intermediate frequencies?
Stealing Sascha's excellent ascii art:

            ---
CPU_PLL ---|   |
           |   |
           |   |----- CPU
           |   |
IM_PLL ----|   |
            ---

Where the sequence is as follows:

1) set IM_PLL to a new freq
2) reparent CPU to IM_PLL
3) configure CPU_PLL
4) reparent CPU to CPU_PLL
5) restore IM_PLL to original frequency

Steps #1 and #5 are new in this example.

I don't see how the concept of an intermediate frequency in the cpufreq
core could ever scale gracefully to handle corner cases. They may be
hypothetical now but I'd rather see us dodge this mistake.

Furthermore any intermediate-frequency property in a Devicetree binding
would suffer the same fate. Trying to neatly encode some weird sequence
into this generic thing will get very ugly very fast.

For proof please look at clk-divider.c, clk-gate.c, clk-mux.c or
clk-composite.c and you'll see the result of the slow accumulation of
lots and lots of hardware corner cases onto generic code. If I had known
then what I know now I would not have created those generic clock types
and I would have tried for an abstraction layer between generic stuff
(e.g. find the best divider) and the real hardware stuff (write to the
register). Instead I kept all of it together and now things are super
ugly.

Regards,
Mike

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On Tue, Mar 10, 2015 at 3:55 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Mar 10, 2015 at 09:53:19AM +0800, Pi-Cheng Chen wrote:
>> On 5 March 2015 at 15:42, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >
>> > My suggestion is to take another approach. Implement clk_set_rate for
>> > these muxes and in the set_rate hook:
>> >
>> > - switch mux to intermediate PLL parent
>> > - call clk_set_rate() for the real parent PLL
>> > - switch mux back to real parent PLL
>>
>> Hi Sascha,
>>
>> Thanks for your suggestion. I've tried to take this approach, but there's some
>> issues here.
>>
>> Calling clk_set_rate() inside the set_rate callback of cpumux will cause
>> an infinite recursive calling in the clock framework:
>> mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ...
>
> I don't understand why setting the PLL rate should call into the mux
> set_rate. Are you sure you call clk_set_rate for the mux parent clk?
> I think the general approach should work, drivers/clk/sirf/clk-common.c
> does something similar in cpu_clk_set_rate(). If you like you can send
> me your work in progress state privatly, I'll have a look then.

Thanks for pointing me out the reference. I think I misunderstood the way you
suggested to do it. I'll post the new version once the design including cpufreq
part is finalized.

Best Regards,
Pi-Cheng
>
>>
>> I've also tries to update pll register settings in the set_rate()
>> callback of cpumux,
>> but the PLL clock information will not be correctly updated in this case.
>
> No, that won't work.
>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Viresh Kumar]

On 11 March 2015 at 05:43, Mike Turquette <mturquette@linaro.org> wrote:
> Sorry, I am not who you asked for advice but I will chime in anyways ;-)

Always welcome :)

> I really hate this intermediate frequency stuff in cpufreq. As we

I am starting to :)

> Furthermore any intermediate-frequency property in a Devicetree binding
> would suffer the same fate. Trying to neatly encode some weird sequence
> into this generic thing will get very ugly very fast.

Hmm..

> For proof please look at clk-divider.c, clk-gate.c, clk-mux.c or
> clk-composite.c and you'll see the result of the slow accumulation of
> lots and lots of hardware corner cases onto generic code. If I had known
> then what I know now I would not have created those generic clock types
> and I would have tried for an abstraction layer between generic stuff
> (e.g. find the best divider) and the real hardware stuff (write to the
> register). Instead I kept all of it together and now things are super
> ugly.

Yeah.

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Viresh Kumar]

On 11 March 2015 at 05:29, Mike Turquette <mturquette@linaro.org> wrote:
> I assume you are saying that you want to update loops-per-jiffie while
> at an intermediate frequency. Why? This operation should not take very
> long.
>
> Imagine a (hypothetical?) processor that changes frequency in many small
> steps until it converges to the target rate. Would you want to update
> lpj for every step?

That's why I have asked Russell specifically about this but haven't got a reply
yet. But it looks wrong to me as well now.. Probably we don't need to care
about it at all..

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Russell King - ARM Linux]

On Thu, Mar 05, 2015 at 04:32:06PM +0530, Viresh Kumar wrote:
> On 5 March 2015 at 16:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Given the variance of different SoCs I don't think it makes sense
> > to try to handle all these cases. Instead the cpufreq-dt driver
> > should just call clk_set_rate() on the CPU clock with the desired
> > target frequency. Everything else should be handled in the clock
> > driver which has intimate knowledge about the SoC anyway.
> 
> I agree..
> 
> @Russell: I wanted to ask you this since sometime..

Remember that I've been "away" from mail for several days last week, as
I had widely published on google+ and on the lists.

> On CPU-freq changes we fire PRE/POST notifiers and they are
> used for updating loops_per_jiffies which then controls delays.

Their main purpose is to allow drivers to shut down their peripherals
before the change and bring them back up after the change on SoCs where
the peripherals are clocked off the CPU clock or a derivative of it.

For example, SA11x0 SoCs were never actually designed for CPU frequency
scaling, but we made it work - and in doing so, you have to reprogram a
bunch of peripherals such as PCMCIA timings, LCD controller, and SDRAM
controller - and because of the need to quiesce the SDRAM controller
over the transition, we need to shut down anything which may cause RAM
to be accessed (such as DMA.)

> Now, it is fine to do that for normal frequencies, but what should be
> the approach for intermediate frequencies ?

I don't think so - calling the notifiers can be expensive where peripheral
drivers are involved - peripherals may need to wait for the hardware to
quiesce before they can allow the notifier to continue.  You could make
it a different reason code (so that drivers such as the SA11x0 stuff can
ignore these intermediate steps.)

> The *wild* thought I earlier had was to fire these notifiers for even
> these intermediate frequencies, otherwise some of the delays will end
> before they should have and that *might* cause other problems.

The real answer is not to use the software delay at all when cpufreq is
enabled, and use a timer based delay which is then independent of the
CPU clock rate.

The only case where a software delay is acceptable is as a last resort
fallback if the platform has no other option, or the CPU clock is
stable.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

On Wed, Jul 15, 2015 at 02:38:46PM +0800, Pi-Cheng Chen wrote:
> From: "pi-cheng.chen" <pi-cheng.chen@linaro.org>
> 
> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
> for intermediate clock source switching.

Hi Mike and Stephen,

since the MT8173 cpufreq driver is likely going to be merged[1] and we still
need the CPU MUX clocks consumed by MT8173 cpufreq driver, may I have some
comments from you to get this patch moving forwards so that MT8173 cpufreq
driver could work completely?

Or the regmap supported basic clock type[2] is the prefered way?
Thanks.

Best Regards,
Pi-Cheng

[1] http://marc.info/?l=linux-pm&m=143850044118837&w=2
[2] http://marc.info/?l=linux-kernel&m=143835585905028&w=2

> 
> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
> Changes in v4:
> - Address comments for v3
> - Rebase to the patch that adds 13mhz clock for MT8173[1]
> 
> Changes in v3:
> - Rebase to 4.2-rc1
> - Fix some issues of v2
> 
> Changes in v2:
> - Remove use of .determine_rate callback
> 
> [1] http://patchwork.kernerl.xyz/patch/6777041/
> ---
>  drivers/clk/mediatek/Makefile          |   2 +-
>  drivers/clk/mediatek/clk-cpumux.c      | 127 +++++++++++++++++++++++++++++++++
>  drivers/clk/mediatek/clk-cpumux.h      |  22 ++++++
>  drivers/clk/mediatek/clk-mt8173.c      |  23 ++++++
>  include/dt-bindings/clock/mt8173-clk.h |   4 +-
>  5 files changed, 176 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clk/mediatek/clk-cpumux.c
>  create mode 100644 drivers/clk/mediatek/clk-cpumux.h
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 8e4b2a4..299917a 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -1,4 +1,4 @@
> -obj-y += clk-mtk.o clk-pll.o clk-gate.o
> +obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o
>  obj-$(CONFIG_RESET_CONTROLLER) += reset.o
>  obj-y += clk-mt8135.o
>  obj-y += clk-mt8173.o
> diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
> new file mode 100644
> index 0000000..fb04fe1
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-cpumux.c
> @@ -0,0 +1,127 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> + *
> + * 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 program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/slab.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-cpumux.h"
> +
> +struct mtk_clk_cpumux {
> +	struct clk_hw	hw;
> +	struct regmap	*regmap;
> +	u32		reg;
> +	u32		mask;
> +	u8		shift;
> +};
> +
> +static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw)
> +{
> +	return container_of(_hw, struct mtk_clk_cpumux, hw);
> +}
> +
> +static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> +{
> +	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
> +	int num_parents = __clk_get_num_parents(hw->clk);
> +	unsigned int val;
> +
> +	regmap_read(mux->regmap, mux->reg, &val);
> +
> +	val >>= mux->shift;
> +	val &= mux->mask;
> +
> +	if (val >= num_parents)
> +		return -EINVAL;
> +
> +	return val;
> +}
> +
> +static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
> +	u32 mask, val;
> +
> +	val = index << mux->shift;
> +	mask = mux->mask << mux->shift;
> +
> +	return regmap_update_bits(mux->regmap, mux->reg, mask, val);
> +}
> +
> +static const struct clk_ops clk_cpumux_ops = {
> +	.get_parent = clk_cpumux_get_parent,
> +	.set_parent = clk_cpumux_set_parent,
> +};
> +
> +static struct clk __init *mtk_clk_register_cpumux(const struct mtk_composite *mux,
> +					   struct regmap *regmap)
> +{
> +	struct mtk_clk_cpumux *cpumux;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	cpumux = kzalloc(sizeof(*cpumux), GFP_KERNEL);
> +	if (!cpumux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = mux->name;
> +	init.ops = &clk_cpumux_ops;
> +	init.parent_names = mux->parent_names;
> +	init.num_parents = mux->num_parents;
> +	init.flags = mux->flags;
> +
> +	cpumux->reg = mux->mux_reg;
> +	cpumux->shift = mux->mux_shift;
> +	cpumux->mask = BIT(mux->mux_width) - 1;
> +	cpumux->regmap = regmap;
> +	cpumux->hw.init = &init;
> +
> +	clk = clk_register(NULL, &cpumux->hw);
> +	if (IS_ERR(clk))
> +		kfree(cpumux);
> +
> +	return clk;
> +}
> +
> +int __init mtk_clk_register_cpumuxes(struct device_node *node,
> +			      const struct mtk_composite *clks, int num,
> +			      struct clk_onecell_data *clk_data)
> +{
> +	int i;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +
> +	regmap = syscon_node_to_regmap(node);
> +	if (IS_ERR(regmap)) {
> +		pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
> +		       PTR_ERR(regmap));
> +		return PTR_ERR(regmap);
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		const struct mtk_composite *mux = &clks[i];
> +
> +		clk = mtk_clk_register_cpumux(mux, regmap);
> +		if (IS_ERR(clk)) {
> +			pr_err("Failed to register clk %s: %ld\n",
> +			       mux->name, PTR_ERR(clk));
> +			continue;
> +		}
> +
> +		clk_data->clks[mux->id] = clk;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/clk/mediatek/clk-cpumux.h b/drivers/clk/mediatek/clk-cpumux.h
> new file mode 100644
> index 0000000..52c769f
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-cpumux.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> + *
> + * 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 program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __DRV_CLK_CPUMUX_H
> +#define __DRV_CLK_CPUMUX_H
> +
> +int mtk_clk_register_cpumuxes(struct device_node *node,
> +			      const struct mtk_composite *clks, int num,
> +			      struct clk_onecell_data *clk_data);
> +
> +#endif /* __DRV_CLK_CPUMUX_H */
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> index 540c5c3..053ca27 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -19,6 +19,7 @@
>  
>  #include "clk-mtk.h"
>  #include "clk-gate.h"
> +#include "clk-cpumux.h"
>  
>  #include <dt-bindings/clock/mt8173-clk.h>
>  
> @@ -517,6 +518,25 @@ static const char * const i2s3_b_ck_parents[] __initconst = {
>  	"apll2_div5"
>  };
>  
> +static const char * const ca53_parents[] __initconst = {
> +	"clk26m",
> +	"armca7pll",
> +	"mainpll",
> +	"univpll"
> +};
> +
> +static const char * const ca57_parents[] __initconst = {
> +	"clk26m",
> +	"armca15pll",
> +	"mainpll",
> +	"univpll"
> +};
> +
> +static const struct mtk_composite cpu_muxes[] __initconst = {
> +	MUX(CLK_INFRA_CA53SEL, "infra_ca53_sel", ca53_parents, 0x0000, 0, 2),
> +	MUX(CLK_INFRA_CA57SEL, "infra_ca57_sel", ca57_parents, 0x0000, 2, 2),
> +};
> +
>  static const struct mtk_composite top_muxes[] __initconst = {
>  	/* CLK_CFG_0 */
>  	MUX(CLK_TOP_AXI_SEL, "axi_sel", axi_parents, 0x0040, 0, 3),
> @@ -743,6 +763,9 @@ static void __init mtk_infrasys_init(struct device_node *node)
>  						clk_data);
>  	mtk_clk_register_factors(infra_divs, ARRAY_SIZE(infra_divs), clk_data);
>  
> +	mtk_clk_register_cpumuxes(node, cpu_muxes, ARRAY_SIZE(cpu_muxes),
> +						clk_data);
> +
>  	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
>  	if (r)
>  		pr_err("%s(): could not register clock provider: %d\n",
> diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
> index fa2a2bb..a17cbaa 100644
> --- a/include/dt-bindings/clock/mt8173-clk.h
> +++ b/include/dt-bindings/clock/mt8173-clk.h
> @@ -188,7 +188,9 @@
>  #define CLK_INFRA_PMICSPI		10
>  #define CLK_INFRA_PMICWRAP		11
>  #define CLK_INFRA_CLK_13M		12
> -#define CLK_INFRA_NR_CLK		13
> +#define CLK_INFRA_CA53SEL		13
> +#define CLK_INFRA_CA57SEL		14
> +#define CLK_INFRA_NR_CLK		15
>  
>  /* PERI_SYS */
>  
> -- 
> 1.9.1
> 

[PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Pi-Cheng Chen]

From: "pi-cheng.chen" <pi-cheng.chen@linaro.org>

This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
for intermediate clock source switching.

Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
---
Changes in v4:
- Address comments for v3
- Rebase to the patch that adds 13mhz clock for MT8173[1]

Changes in v3:
- Rebase to 4.2-rc1
- Fix some issues of v2

Changes in v2:
- Remove use of .determine_rate callback

[1] http://patchwork.kernerl.xyz/patch/6777041/
---
 drivers/clk/mediatek/Makefile          |   2 +-
 drivers/clk/mediatek/clk-cpumux.c      | 127 +++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-cpumux.h      |  22 ++++++
 drivers/clk/mediatek/clk-mt8173.c      |  23 ++++++
 include/dt-bindings/clock/mt8173-clk.h |   4 +-
 5 files changed, 176 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/mediatek/clk-cpumux.c
 create mode 100644 drivers/clk/mediatek/clk-cpumux.h

diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
index 8e4b2a4..299917a 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -1,4 +1,4 @@
-obj-y += clk-mtk.o clk-pll.o clk-gate.o
+obj-y += clk-mtk.o clk-pll.o clk-gate.o clk-cpumux.o
 obj-$(CONFIG_RESET_CONTROLLER) += reset.o
 obj-y += clk-mt8135.o
 obj-y += clk-mt8173.o
diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
new file mode 100644
index 0000000..fb04fe1
--- /dev/null
+++ b/drivers/clk/mediatek/clk-cpumux.c
@@ -0,0 +1,127 @@
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
+ *
+ * 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 program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/slab.h>
+
+#include "clk-mtk.h"
+#include "clk-cpumux.h"
+
+struct mtk_clk_cpumux {
+	struct clk_hw	hw;
+	struct regmap	*regmap;
+	u32		reg;
+	u32		mask;
+	u8		shift;
+};
+
+static inline struct mtk_clk_cpumux *to_clk_mux(struct clk_hw *_hw)
+{
+	return container_of(_hw, struct mtk_clk_cpumux, hw);
+}
+
+static u8 clk_cpumux_get_parent(struct clk_hw *hw)
+{
+	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
+	int num_parents = __clk_get_num_parents(hw->clk);
+	unsigned int val;
+
+	regmap_read(mux->regmap, mux->reg, &val);
+
+	val >>= mux->shift;
+	val &= mux->mask;
+
+	if (val >= num_parents)
+		return -EINVAL;
+
+	return val;
+}
+
+static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct mtk_clk_cpumux *mux = to_clk_mux(hw);
+	u32 mask, val;
+
+	val = index << mux->shift;
+	mask = mux->mask << mux->shift;
+
+	return regmap_update_bits(mux->regmap, mux->reg, mask, val);
+}
+
+static const struct clk_ops clk_cpumux_ops = {
+	.get_parent = clk_cpumux_get_parent,
+	.set_parent = clk_cpumux_set_parent,
+};
+
+static struct clk __init *mtk_clk_register_cpumux(const struct mtk_composite *mux,
+					   struct regmap *regmap)
+{
+	struct mtk_clk_cpumux *cpumux;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	cpumux = kzalloc(sizeof(*cpumux), GFP_KERNEL);
+	if (!cpumux)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = mux->name;
+	init.ops = &clk_cpumux_ops;
+	init.parent_names = mux->parent_names;
+	init.num_parents = mux->num_parents;
+	init.flags = mux->flags;
+
+	cpumux->reg = mux->mux_reg;
+	cpumux->shift = mux->mux_shift;
+	cpumux->mask = BIT(mux->mux_width) - 1;
+	cpumux->regmap = regmap;
+	cpumux->hw.init = &init;
+
+	clk = clk_register(NULL, &cpumux->hw);
+	if (IS_ERR(clk))
+		kfree(cpumux);
+
+	return clk;
+}
+
+int __init mtk_clk_register_cpumuxes(struct device_node *node,
+			      const struct mtk_composite *clks, int num,
+			      struct clk_onecell_data *clk_data)
+{
+	int i;
+	struct clk *clk;
+	struct regmap *regmap;
+
+	regmap = syscon_node_to_regmap(node);
+	if (IS_ERR(regmap)) {
+		pr_err("Cannot find regmap for %s: %ld\n", node->full_name,
+		       PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
+
+	for (i = 0; i < num; i++) {
+		const struct mtk_composite *mux = &clks[i];
+
+		clk = mtk_clk_register_cpumux(mux, regmap);
+		if (IS_ERR(clk)) {
+			pr_err("Failed to register clk %s: %ld\n",
+			       mux->name, PTR_ERR(clk));
+			continue;
+		}
+
+		clk_data->clks[mux->id] = clk;
+	}
+
+	return 0;
+}
diff --git a/drivers/clk/mediatek/clk-cpumux.h b/drivers/clk/mediatek/clk-cpumux.h
new file mode 100644
index 0000000..52c769f
--- /dev/null
+++ b/drivers/clk/mediatek/clk-cpumux.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
+ *
+ * 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 program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __DRV_CLK_CPUMUX_H
+#define __DRV_CLK_CPUMUX_H
+
+int mtk_clk_register_cpumuxes(struct device_node *node,
+			      const struct mtk_composite *clks, int num,
+			      struct clk_onecell_data *clk_data);
+
+#endif /* __DRV_CLK_CPUMUX_H */
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 540c5c3..053ca27 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -19,6 +19,7 @@
 
 #include "clk-mtk.h"
 #include "clk-gate.h"
+#include "clk-cpumux.h"
 
 #include <dt-bindings/clock/mt8173-clk.h>
 
@@ -517,6 +518,25 @@ static const char * const i2s3_b_ck_parents[] __initconst = {
 	"apll2_div5"
 };
 
+static const char * const ca53_parents[] __initconst = {
+	"clk26m",
+	"armca7pll",
+	"mainpll",
+	"univpll"
+};
+
+static const char * const ca57_parents[] __initconst = {
+	"clk26m",
+	"armca15pll",
+	"mainpll",
+	"univpll"
+};
+
+static const struct mtk_composite cpu_muxes[] __initconst = {
+	MUX(CLK_INFRA_CA53SEL, "infra_ca53_sel", ca53_parents, 0x0000, 0, 2),
+	MUX(CLK_INFRA_CA57SEL, "infra_ca57_sel", ca57_parents, 0x0000, 2, 2),
+};
+
 static const struct mtk_composite top_muxes[] __initconst = {
 	/* CLK_CFG_0 */
 	MUX(CLK_TOP_AXI_SEL, "axi_sel", axi_parents, 0x0040, 0, 3),
@@ -743,6 +763,9 @@ static void __init mtk_infrasys_init(struct device_node *node)
 						clk_data);
 	mtk_clk_register_factors(infra_divs, ARRAY_SIZE(infra_divs), clk_data);
 
+	mtk_clk_register_cpumuxes(node, cpu_muxes, ARRAY_SIZE(cpu_muxes),
+						clk_data);
+
 	r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 	if (r)
 		pr_err("%s(): could not register clock provider: %d\n",
diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
index fa2a2bb..a17cbaa 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -188,7 +188,9 @@
 #define CLK_INFRA_PMICSPI		10
 #define CLK_INFRA_PMICWRAP		11
 #define CLK_INFRA_CLK_13M		12
-#define CLK_INFRA_NR_CLK		13
+#define CLK_INFRA_CA53SEL		13
+#define CLK_INFRA_CA57SEL		14
+#define CLK_INFRA_NR_CLK		15
 
 /* PERI_SYS */
 
-- 
1.9.1