[RFC] clk: imx8mm: Add dram freq switch support

Commit Message

Leonard Crestez May 30, 2019, 7:13 a.m. UTC

Add a wrapper clock encapsulating dram frequency switch support for
imx8m chips. This allows higher-level DVFS code to manipulate dram
frequency using standard clock framework APIs.

Linux-side implementation is similar in principle to imx_clk_cpu or a
composite clock. Only some preparation is done inside the kernel, the
actual freq switch is performed from TF-A code which runs from an SRAM
area. Cores other than the one performing the switch are also made to
spin inside TF-A by sending each an IRQ.

This is an early proof-of-concept which only support low/high mode on
imx8mm but NXP has secure-world dram freq switching implementations for
multiple other chips and this approach can be extended.

This was tested using a large pile of NXP out-of-tree patches. Code for
the "busfreq core" from last release can be seen here:
https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/busfreq-imx8mq.c?h=imx_4.14.98_2.0.0_ga

It can be likely made to work with interconnect RFC:
https://patchwork.kernel.org/cover/10851705/

This RFC effectively refactors a common part between them.

Among the possible cleanups:
 * Handle errors in low/high busfreq code and back off
 * Move irq to secure world
 * Try to use fewer clk parameters
 * More chips and frequencies

Many platforms handle this kind of stuff externally but cpufreq is quite
insistent that actual rates are set by clk code and that new platforms
use cpufreq-dt.

Let me know if there are objections to handling dram freq via clk.

Signed-off-by: Leonard Crestez <[e-mail]>
---
 drivers/clk/imx/Makefile                 |   2 +-
 drivers/clk/imx/clk-imx8m-dram.c         | 256 +++++++++++++++++++++++
 drivers/clk/imx/clk-imx8mm.c             |  12 ++
 drivers/clk/imx/clk.h                    |  13 ++
 include/dt-bindings/clock/imx8mm-clock.h |   4 +-
 5 files changed, 285 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/imx/clk-imx8m-dram.c

Comments

Stephen Boyd June 27, 2019, 9:14 p.m. UTC | #1

Quoting Leonard Crestez (2019-05-30 00:13:51)
> Add a wrapper clock encapsulating dram frequency switch support for
> imx8m chips. This allows higher-level DVFS code to manipulate dram
> frequency using standard clock framework APIs.
> 
> Linux-side implementation is similar in principle to imx_clk_cpu or a
> composite clock. Only some preparation is done inside the kernel, the
> actual freq switch is performed from TF-A code which runs from an SRAM
> area. Cores other than the one performing the switch are also made to
> spin inside TF-A by sending each an IRQ.
> 
> This is an early proof-of-concept which only support low/high mode on
> imx8mm but NXP has secure-world dram freq switching implementations for
> multiple other chips and this approach can be extended.
> 
> This was tested using a large pile of NXP out-of-tree patches. Code for
> the "busfreq core" from last release can be seen here:
> https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/busfreq-imx8mq.c?h=imx_4.14.98_2.0.0_ga
> 
> It can be likely made to work with interconnect RFC:
> https://patchwork.kernel.org/cover/10851705/
> 
> This RFC effectively refactors a common part between them.
> 
> Among the possible cleanups:
>  * Handle errors in low/high busfreq code and back off
>  * Move irq to secure world
>  * Try to use fewer clk parameters
>  * More chips and frequencies
> 
> Many platforms handle this kind of stuff externally but cpufreq is quite
> insistent that actual rates are set by clk code and that new platforms
> use cpufreq-dt.
> 
> Let me know if there are objections to handling dram freq via clk.

Can it be an interconnect driver instead? I don't see how this is a clk
driver. It looks more like a driver that itself manages a collection of
clks, and you've put the coordination of those clks behind the clk_ops
interface. We don't want to have clk_ops calling clk consumer APIs in
general, so the whole approach doesn't seem correct. Hopefully this can
work out as some other sort of driver that is used directly from devfreq
or interconnect core instead and then have a different consumer driver
of devfreq or interconnect core that knows how to drive the clk tree.

Leonard Crestez June 28, 2019, 8:30 a.m. UTC | #2

On 28.06.2019 00:15, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-05-30 00:13:51)

>> Add a wrapper clock encapsulating dram frequency switch support for
>> imx8m chips. This allows higher-level DVFS code to manipulate dram
>> frequency using standard clock framework APIs.
>>
>> Linux-side implementation is similar in principle to imx_clk_cpu or a
>> composite clock. Only some preparation is done inside the kernel, the
>> actual freq switch is performed from TF-A code which runs from an SRAM
>> area. Cores other than the one performing the switch are also made to
>> spin inside TF-A by sending each an IRQ.
>>
>> This is an early proof-of-concept which only support low/high mode on
>> imx8mm but NXP has secure-world dram freq switching implementations for
>> multiple other chips and this approach can be extended.
>>
>> Many platforms handle this kind of stuff externally but cpufreq is quite
>> insistent that actual rates are set by clk code and that new platforms
>> use cpufreq-dt.
>>
>> Let me know if there are objections to handling dram freq via clk.
> 
> Can it be an interconnect driver instead? I don't see how this is a clk
> driver. It looks more like a driver that itself manages a collection of
> clks, and you've put the coordination of those clks behind the clk_ops
> interface. We don't want to have clk_ops calling clk consumer APIs in
> general, so the whole approach doesn't seem correct.

The imx8m dram clk structure is only slightly more complicated than that 
for the cpu. It's not clear why mux manipulation should be pushed away 
from clk and onto consumers. Isn't it desirable for clk_set_rate to 
"just work"?

Implementation uses consumer APIs because the constructor takes struct 
clk*. It could be modifed to take struct clk_hw*, but probably only with 
larger changes to clk-imx8m.

The interrupt handling should be moved to secure world.

> Hopefully this can
> work out as some other sort of driver that is used directly from devfreq
> or interconnect core instead and then have a different consumer driver
> of devfreq or interconnect core that knows how to drive the clk tree.

Hiding dram rate setting behind a clk rate makes it much easier to 
implement devfreq or interconnect, as in this series:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=139367

I sent than RFC after you replied to this email, mostly because it's 
been pending for a while.

--
Regards,
Leonard

Patch

diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 05641c64b317..0588070a9a1f 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -23,11 +23,11 @@  obj-$(CONFIG_MXC_CLK) += \
 
 obj-$(CONFIG_MXC_CLK_SCU) += \
 	clk-scu.o \
 	clk-lpcg-scu.o
 
-obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
+obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o clk-imx8m-dram.o
 obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
 obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
 
 obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
 obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
diff --git a/drivers/clk/imx/clk-imx8m-dram.c b/drivers/clk/imx/clk-imx8m-dram.c
new file mode 100644
index 000000000000..18a1411d154e
--- /dev/null
+++ b/drivers/clk/imx/clk-imx8m-dram.c
@@ -0,0 +1,256 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 NXP
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/of_irq.h>
+#include "clk.h"
+
+#define FSL_SIP_DDR_DVFS                0xc2000004
+
+#define FSL_SIP_DDR_FREQ_SET_HIGH	0x0
+#define FSL_SIP_DDR_FREQ_SET_100MTS	0x2
+#define FSL_SIP_DDR_FREQ_WAIT_DONE	0xf
+
+struct dram_clk {
+	struct clk_hw	hw;
+	struct clk	*dram_core;
+	struct clk	*dram_apb;
+	struct clk	*dram_pll;
+	struct clk	*dram_alt;
+	struct clk	*dram_alt_root;
+	struct clk	*sys1_pll_40m;
+	struct clk	*sys1_pll_100m;
+	struct clk	*sys1_pll_800m;
+	int		irqs[CONFIG_NR_CPUS];
+};
+
+static inline struct dram_clk *to_dram_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct dram_clk, hw);
+}
+
+static irqreturn_t wait_in_wfe_irq(int irq, void *dev_id)
+{
+	struct arm_smccc_res res;
+
+	/* call smc trap to ATF */
+	arm_smccc_smc(FSL_SIP_DDR_DVFS, FSL_SIP_DDR_FREQ_WAIT_DONE, 0,
+		0, 0, 0, 0, 0, &res);
+
+	return IRQ_HANDLED;
+}
+
+static void update_bus_freq(int target_freq)
+{
+	struct arm_smccc_res res;
+	u32 online_cpus = 0;
+	int cpu = 0;
+
+	local_irq_disable();
+
+	for_each_online_cpu(cpu)
+		online_cpus |= (1 << (cpu * 8));
+
+	/* change the ddr freqency */
+	arm_smccc_smc(FSL_SIP_DDR_DVFS, target_freq, online_cpus,
+		0, 0, 0, 0, 0, &res);
+
+	local_irq_enable();
+}
+
+static int dram_clk_ensure_irq_affinity(struct dram_clk* priv)
+{
+	int err, cpu;
+
+	for_each_online_cpu(cpu) {
+		err = irq_set_affinity(priv->irqs[cpu], cpumask_of(cpu));
+		if (err) {
+			pr_err("imx8m_dram_clk set irqs[%d] affinity failed: %d\n",
+				cpu, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int dram_clk_set_low(struct dram_clk *priv)
+{
+	// FIXME: error handling
+	clk_prepare_enable(priv->sys1_pll_40m);
+	clk_prepare_enable(priv->dram_alt_root);
+	clk_prepare_enable(priv->sys1_pll_100m);
+
+	/* switch the DDR freqeuncy */
+	update_bus_freq(FSL_SIP_DDR_FREQ_SET_100MTS);
+
+	/* correct the clock tree info */
+	clk_set_parent(priv->dram_alt, priv->sys1_pll_100m);
+	clk_set_parent(priv->dram_core, priv->dram_alt_root);
+	clk_set_parent(priv->dram_apb, priv->sys1_pll_40m);
+	clk_set_rate(priv->dram_apb, 20000000);
+	clk_disable_unprepare(priv->sys1_pll_100m);
+	clk_disable_unprepare(priv->sys1_pll_40m);
+	clk_disable_unprepare(priv->dram_alt_root);
+	return 0;
+}
+
+static int dram_clk_set_high(struct dram_clk *priv)
+{
+	// FIXME: error handling
+	clk_prepare_enable(priv->sys1_pll_800m);
+	clk_prepare_enable(priv->dram_pll);
+
+	/* switch the DDR freqeuncy */
+	update_bus_freq(FSL_SIP_DDR_FREQ_SET_HIGH);
+
+	/* correct the clock tree info */
+	clk_set_parent(priv->dram_apb, priv->sys1_pll_800m);
+	clk_set_rate(priv->dram_apb, 160000000);
+	clk_set_parent(priv->dram_core, priv->dram_pll);
+	clk_disable_unprepare(priv->sys1_pll_800m);
+	clk_disable_unprepare(priv->dram_pll);
+
+	return 0;
+}
+
+static long dram_clk_round_rate(struct clk_hw *hw,
+		unsigned long rate,
+		unsigned long *parent_rate)
+{
+	// FIXME: table?
+	if (rate < 387500000)
+		return 250000000;
+	else
+		return 75000000000;
+}
+
+static int dram_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct dram_clk *priv = to_dram_clk(hw);
+	int ret;
+
+	ret = dram_clk_ensure_irq_affinity(priv);
+	if (ret)
+		return ret;
+
+	if (rate == 250000000) {
+		return dram_clk_set_low(priv);
+	} else if (rate == 75000000000) {
+		return dram_clk_set_high(priv);
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static unsigned long dram_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct dram_clk *priv = to_dram_clk(hw);
+
+	return clk_get_rate(priv->dram_core);
+}
+
+static const struct clk_ops dram_clk_ops = {
+	.recalc_rate	= dram_clk_recalc_rate,
+	.round_rate	= dram_clk_round_rate,
+	.set_rate	= dram_clk_set_rate,
+};
+
+static int dram_clk_init_irqs(struct dram_clk* priv, struct device_node *np)
+{
+	int err, irq, cpu;
+
+	for_each_possible_cpu(cpu) {
+		irq = of_irq_get(np, cpu);
+		if (irq < 0) {
+			pr_err("imx8m_dram_clk fail of_irq_get %d\n", irq);
+			return irq;
+		}
+
+		err = request_irq(irq, wait_in_wfe_irq,
+				IRQF_PERCPU, "ddrc", NULL);
+		if (err) {
+			pr_err("imx8m_dram_clk request irq %d failed: %d\n",
+				irq, err);
+			return err;
+		}
+		priv->irqs[cpu] = irq;
+	}
+
+	return 0;
+}
+
+static void dram_clk_free_irqs(struct dram_clk* priv)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		free_irq(priv->irqs[cpu], NULL);
+		priv->irqs[cpu] = 0;
+	}
+}
+
+struct clk* imx8m_dram_clk(
+		struct device_node* np,
+		const char *name, const char* parent_name,
+		struct clk* dram_core,
+		struct clk* dram_apb,
+		struct clk* dram_pll,
+		struct clk* dram_alt,
+		struct clk* dram_alt_root,
+		struct clk* sys1_pll_40m,
+		struct clk* sys1_pll_100m,
+		struct clk* sys1_pll_800m)
+{
+	struct dram_clk *priv;
+	struct clk *clk;
+	struct clk_init_data init;
+	int err;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	priv->dram_apb = dram_apb;
+	priv->dram_core = dram_core;
+	priv->dram_pll = dram_pll;
+	priv->dram_alt = dram_alt;
+	priv->dram_alt_root = dram_alt_root;
+	priv->sys1_pll_40m = sys1_pll_40m;
+	priv->sys1_pll_100m = sys1_pll_100m;
+	priv->sys1_pll_800m = sys1_pll_800m;
+
+	init.name = name;
+	init.ops = &dram_clk_ops;
+	init.flags = CLK_IS_CRITICAL;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	err = dram_clk_init_irqs(priv, np);
+	if (err)
+		goto err_free_priv;
+
+	priv->hw.init = &init;
+	clk = clk_register(NULL, &priv->hw);
+	if (IS_ERR(clk)) {
+		err = PTR_ERR(clk);
+		goto err_free_irqs;
+	}
+	return clk;
+
+err_free_irqs:
+	dram_clk_free_irqs(priv);
+err_free_priv:
+	kfree(priv);
+	return ERR_PTR(err);
+}
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index eb9fcf0df0b2..2edb62b10572 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -660,10 +660,22 @@  static int __init imx8mm_clocks_init(struct device_node *ccm_node)
 	clks[IMX8MM_CLK_GPT_3M] = imx_clk_fixed_factor("gpt_3m", "osc_24m", 1, 8);
 
 	clks[IMX8MM_CLK_DRAM_ALT_ROOT] = imx_clk_fixed_factor("dram_alt_root", "dram_alt", 1, 4);
 	clks[IMX8MM_CLK_DRAM_CORE] = imx_clk_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mm_dram_core_sels, ARRAY_SIZE(imx8mm_dram_core_sels), CLK_IS_CRITICAL);
 
+	clks[IMX8MM_CLK_DRAM] = imx8m_dram_clk(
+			ccm_node,
+			"dram", "dram_core_clk",
+			clks[IMX8MM_CLK_DRAM_CORE],
+			clks[IMX8MM_CLK_DRAM_APB],
+			clks[IMX8MM_DRAM_PLL_OUT],
+			clks[IMX8MM_CLK_DRAM_ALT],
+			clks[IMX8MM_CLK_DRAM_ALT_ROOT],
+			clks[IMX8MM_SYS_PLL1_40M],
+			clks[IMX8MM_SYS_PLL1_100M],
+			clks[IMX8MM_SYS_PLL1_800M]);
+
 	clks[IMX8MM_CLK_ARM] = imx_clk_cpu("arm", "arm_a53_div",
 					   clks[IMX8MM_CLK_A53_DIV],
 					   clks[IMX8MM_CLK_A53_SRC],
 					   clks[IMX8MM_ARM_PLL_OUT],
 					   clks[IMX8MM_CLK_24M]);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 6dcdc91cbba8..f41e27d3132c 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -396,6 +396,19 @@  struct clk *imx8m_clk_composite_flags(const char *name,
 
 struct clk_hw *imx_clk_divider_gate(const char *name, const char *parent_name,
 		unsigned long flags, void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,
 		spinlock_t *lock);
+
+struct clk* imx8m_dram_clk(
+		struct device_node *np,
+		const char *name, const char* parent_name,
+		struct clk* dram_core,
+		struct clk* dram_apb,
+		struct clk* dram_pll,
+		struct clk* dram_alt,
+		struct clk* dram_alt_root,
+		struct clk* sys1_pll_40m,
+		struct clk* sys1_pll_100m,
+		struct clk* sys1_pll_800m);
+
 #endif
diff --git a/include/dt-bindings/clock/imx8mm-clock.h b/include/dt-bindings/clock/imx8mm-clock.h
index 07e6c686f3ef..dde146b923a8 100644
--- a/include/dt-bindings/clock/imx8mm-clock.h
+++ b/include/dt-bindings/clock/imx8mm-clock.h
@@ -246,8 +246,10 @@ 
 #define IMX8MM_CLK_GPIO5_ROOT			227
 
 #define IMX8MM_CLK_SNVS_ROOT			228
 #define IMX8MM_CLK_GIC				229
 
-#define IMX8MM_CLK_END				230
+#define IMX8MM_CLK_DRAM				230
+
+#define IMX8MM_CLK_END				231
 
 #endif