* [PATCH 0/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-09 9:54 ` pi-cheng.chen
0 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger, Thomas Petazzoni
Cc: pi-cheng.chen, linux-kernel, linux-pm, linux-arm-kernel,
Mike Turquette, linaro-kernel, Eddie Huang, Yingjoe Chen
MT8173 is a ARMv8 based SoC with 2 CA53 + 2 CA57 cores and 2 clusters.
All CPUs in a single cluster share the same power and clock domain. This
series tries to add cpufreq driver support for MT8173 SoC by using DT
based cpufreq driver.
Currently the DT based cpufreq driver is missing some way to check which
CPUs share clocks. In the 1st patch, CPU clock/power domain information is
added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
check which CPUs share clock/power.
When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
SoC works properly:
1. When doing frequency scaling of CPUs, the clock source should be switched
to another PLL, then switched back to the orignal until it's stable.
2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
controlled concurrently and 2 limitations should be followed:
a. Vsram > Vproc
b. Vsram - Vproc < 200 mV
To address these needs, we use cpufreq-dt but do voltage scaling in the
cpufreq notifier.
pi-cheng.chen (2):
cpufreq-dt: check CPU clock/power domain during initializing
cpufreq: add cpufreq driver for Mediatek MT8173 SoC
drivers/cpufreq/Kconfig.arm | 6 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/cpufreq-dt.c | 15 ++
drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
include/linux/cpufreq-dt.h | 6 +
5 files changed, 487 insertions(+)
create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
--
1.9.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 0/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-09 9:54 ` pi-cheng.chen
0 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: linux-arm-kernel
MT8173 is a ARMv8 based SoC with 2 CA53 + 2 CA57 cores and 2 clusters.
All CPUs in a single cluster share the same power and clock domain. This
series tries to add cpufreq driver support for MT8173 SoC by using DT
based cpufreq driver.
Currently the DT based cpufreq driver is missing some way to check which
CPUs share clocks. In the 1st patch, CPU clock/power domain information is
added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
check which CPUs share clock/power.
When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
SoC works properly:
1. When doing frequency scaling of CPUs, the clock source should be switched
to another PLL, then switched back to the orignal until it's stable.
2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
controlled concurrently and 2 limitations should be followed:
a. Vsram > Vproc
b. Vsram - Vproc < 200 mV
To address these needs, we use cpufreq-dt but do voltage scaling in the
cpufreq notifier.
pi-cheng.chen (2):
cpufreq-dt: check CPU clock/power domain during initializing
cpufreq: add cpufreq driver for Mediatek MT8173 SoC
drivers/cpufreq/Kconfig.arm | 6 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/cpufreq-dt.c | 15 ++
drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
include/linux/cpufreq-dt.h | 6 +
5 files changed, 487 insertions(+)
create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
--
1.9.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
2015-01-09 9:54 ` pi-cheng.chen
@ 2015-01-09 9:54 ` pi-cheng.chen
-1 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger, Thomas Petazzoni
Cc: pi-cheng.chen, linux-kernel, linux-pm, linux-arm-kernel,
Mike Turquette, linaro-kernel, Eddie Huang, Yingjoe Chen
Currently the DT based cpufreq driver is missing some way to check which
CPUs share clocks. In the 1st patch, CPU clock/power domain information is
added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
check which CPUs share clock/power.
Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
---
drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
include/linux/cpufreq-dt.h | 6 ++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index fde97d6..ff8c266 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
pd = cpufreq_get_driver_data();
if (!pd || !pd->independent_clocks)
cpumask_setall(policy->cpus);
+ else if (pd && !list_empty(&pd->domain_list)) {
+ struct list_head *domain_node;
+
+ list_for_each(domain_node, &pd->domain_list) {
+ struct cpufreq_cpu_domain *domain;
+
+ domain = container_of(domain_node,
+ struct cpufreq_cpu_domain, node);
+ if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
+ continue;
+
+ cpumask_copy(policy->cpus, &domain->cpus);
+ break;
+ }
+ }
of_node_put(np);
diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
index 0414009..92bffd3 100644
--- a/include/linux/cpufreq-dt.h
+++ b/include/linux/cpufreq-dt.h
@@ -10,6 +10,11 @@
#ifndef __CPUFREQ_DT_H__
#define __CPUFREQ_DT_H__
+struct cpufreq_cpu_domain {
+ struct list_head node;
+ cpumask_t cpus;
+};
+
struct cpufreq_dt_platform_data {
/*
* True when each CPU has its own clock to control its
@@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
* clock.
*/
bool independent_clocks;
+ struct list_head domain_list;
};
#endif /* __CPUFREQ_DT_H__ */
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-09 9:54 ` pi-cheng.chen
0 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: linux-arm-kernel
Currently the DT based cpufreq driver is missing some way to check which
CPUs share clocks. In the 1st patch, CPU clock/power domain information is
added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
check which CPUs share clock/power.
Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
---
drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
include/linux/cpufreq-dt.h | 6 ++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index fde97d6..ff8c266 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
pd = cpufreq_get_driver_data();
if (!pd || !pd->independent_clocks)
cpumask_setall(policy->cpus);
+ else if (pd && !list_empty(&pd->domain_list)) {
+ struct list_head *domain_node;
+
+ list_for_each(domain_node, &pd->domain_list) {
+ struct cpufreq_cpu_domain *domain;
+
+ domain = container_of(domain_node,
+ struct cpufreq_cpu_domain, node);
+ if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
+ continue;
+
+ cpumask_copy(policy->cpus, &domain->cpus);
+ break;
+ }
+ }
of_node_put(np);
diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
index 0414009..92bffd3 100644
--- a/include/linux/cpufreq-dt.h
+++ b/include/linux/cpufreq-dt.h
@@ -10,6 +10,11 @@
#ifndef __CPUFREQ_DT_H__
#define __CPUFREQ_DT_H__
+struct cpufreq_cpu_domain {
+ struct list_head node;
+ cpumask_t cpus;
+};
+
struct cpufreq_dt_platform_data {
/*
* True when each CPU has its own clock to control its
@@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
* clock.
*/
bool independent_clocks;
+ struct list_head domain_list;
};
#endif /* __CPUFREQ_DT_H__ */
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` pi-cheng.chen
@ 2015-01-09 9:54 ` pi-cheng.chen
-1 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger, Thomas Petazzoni
Cc: pi-cheng.chen, linux-kernel, linux-pm, linux-arm-kernel,
Mike Turquette, linaro-kernel, Eddie Huang, Yingjoe Chen
When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
SoC works properly:
1. When doing frequency scaling of CPUs, the clock source should be switched
to another PLL, then switched back to the orignal until it's stable.
2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
controlled concurrently and 2 limitations should be followed:
a. Vsram > Vproc
b. Vsram - Vproc < 200 mV
To address these needs, we reuse cpufreq-dt but do voltage scaling in the
cpufreq notifier.
Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
---
drivers/cpufreq/Kconfig.arm | 6 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
3 files changed, 466 insertions(+)
create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0f9a2c3..97ed7dd 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
This add the CPUFreq driver support for Intel PXA2xx SOCs.
If in doubt, say N.
+
+config ARM_MT8173_CPUFREQ
+ bool "Mediatek MT8173 CPUFreq support"
+ depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
+ help
+ This adds the CPUFreq driver support for Mediatek MT8173 SoC.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index b3ca7b0..67b7f17 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
##################################################################################
# PowerPC platform drivers
diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
new file mode 100644
index 0000000..b578c10
--- /dev/null
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -0,0 +1,459 @@
+/*
+* Copyright (c) 2015 Linaro.
+* 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/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/cpufreq.h>
+#include <linux/cpufreq-dt.h>
+#include <linux/cpumask.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regulator/consumer.h>
+#include <linux/cpu.h>
+#include <linux/pm_opp.h>
+
+#define CPUCLK_MUX_ARMPLL 0x1
+#define CPUCLK_MUX_MAINPLL 0x2
+#define VOLT_SHIFT_LIMIT 150000
+
+enum {
+ LITTLE_CLUSTER = 0,
+ BIG_CLUSTER,
+ NR_CLUSTERS
+};
+
+static struct cluster_dvfs_info {
+ int cpu;
+ struct cpumask cpus;
+ struct device *cpu_dev;
+ struct regulator *proc_reg;
+ struct regulator *sram_reg;
+ int inter_volt;
+ u32 volt_tol;
+} *dvfs_info;
+
+static unsigned long mainpll_freq;
+static void __iomem *clk_mux_regs;
+static spinlock_t lock;
+
+static void cpuclk_mux_set(int cluster, u32 sel)
+{
+ u32 val;
+ u32 mask = 0x3;
+
+ if (cluster == BIG_CLUSTER) {
+ mask <<= 2;
+ sel <<= 2;
+ }
+
+ spin_lock(&lock);
+
+ val = readl(clk_mux_regs);
+ val = (val & ~mask) | sel;
+ writel(val, clk_mux_regs);
+
+ spin_unlock(&lock);
+}
+
+static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
+ unsigned long old_vproc,
+ unsigned long new_vproc)
+{
+ int cur_vsram, cur_vproc, target_volt, tol;
+
+ if (old_vproc > new_vproc) {
+ while (1) {
+ cur_vsram = regulator_get_voltage(dvfs->sram_reg);
+ if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
+ cur_vsram > new_vproc) {
+ tol = new_vproc * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ new_vproc, tol);
+ break;
+ }
+
+ target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
+ tol);
+
+ cur_vproc = regulator_get_voltage(dvfs->proc_reg);
+ target_volt = cur_vproc + 1;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
+ tol);
+ }
+ } else if (old_vproc < new_vproc) {
+ while (1) {
+ cur_vsram = regulator_get_voltage(dvfs->sram_reg);
+ if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
+ cur_vsram > new_vproc) {
+ tol = new_vproc * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ new_vproc, tol);
+ break;
+ }
+
+ cur_vproc = regulator_get_voltage(dvfs->proc_reg);
+ target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
+ tol);
+
+ if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
+ cur_vsram = regulator_get_voltage(
+ dvfs->sram_reg);
+ target_volt = cur_vsram - 1;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ target_volt, tol);
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
+{
+ struct dev_pm_opp *opp;
+ int volt;
+
+ rcu_read_lock();
+ opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
+ if (IS_ERR(opp)) {
+ rcu_read_unlock();
+ pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
+ return PTR_ERR(opp);
+ }
+ volt = dev_pm_opp_get_voltage(opp);
+ rcu_read_unlock();
+
+ return volt;
+}
+
+static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
+{
+ struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
+
+ if (dvfs->inter_volt == 0)
+ dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
+ mainpll_freq);
+
+ return dvfs->inter_volt;
+}
+
+static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
+{
+ struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
+ int ret = 0;
+
+ if (cluster == LITTLE_CLUSTER) {
+ int tol;
+
+ tol = new_volt * dvfs->volt_tol / 100;
+ ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
+ } else {
+ ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
+ }
+
+ if (ret)
+ pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
+ __func__, cluster, old_volt, new_volt);
+}
+
+static int mt8173_cpufreq_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct cpufreq_freqs *freqs = data;
+ struct cluster_dvfs_info *dvfs;
+ unsigned long old_volt, new_volt, inter_volt;
+ int cluster;
+
+ if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
+ cluster = BIG_CLUSTER;
+ else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
+ cluster = LITTLE_CLUSTER;
+ else
+ return NOTIFY_DONE;
+
+ dvfs = &dvfs_info[cluster];
+
+ old_volt = regulator_get_voltage(dvfs->proc_reg);
+
+ new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
+ inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
+
+ if (action == CPUFREQ_PRECHANGE) {
+ if (old_volt < inter_volt || old_volt < new_volt) {
+ new_volt = inter_volt > new_volt ?
+ inter_volt : new_volt;
+ mt8173_cpufreq_set_voltage(cluster, old_volt,
+ new_volt);
+ }
+
+ cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
+ } else if (action == CPUFREQ_POSTCHANGE) {
+ cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
+
+ if (new_volt < old_volt)
+ mt8173_cpufreq_set_voltage(cluster, old_volt,
+ new_volt);
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block mt8173_cpufreq_nb = {
+ .notifier_call = mt8173_cpufreq_notify,
+};
+
+static struct cpufreq_dt_platform_data *pd;
+
+static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
+{
+ struct list_head *node;
+
+ list_for_each(node, domain_list) {
+ struct cpufreq_cpu_domain *domain;
+
+ domain = container_of(node, struct cpufreq_cpu_domain, node);
+ if (cpumask_test_cpu(cpu, &domain->cpus))
+ return 1;
+ }
+
+ return 0;
+}
+
+static void cpufreq_dt_pdata_release(void)
+{
+ if (!pd)
+ return;
+
+ if (!list_empty(&pd->domain_list)) {
+ struct list_head *node, *tmp;
+ struct cpufreq_cpu_domain *domain;
+
+ list_for_each_safe(node, tmp, &pd->domain_list) {
+ list_del(node);
+ domain = container_of(node, struct cpufreq_cpu_domain,
+ node);
+ kfree(domain);
+ }
+ }
+
+ kfree(pd);
+}
+
+static int cpufreq_dt_pdata_init(void)
+{
+ int cpu;
+
+ pd = kmalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ pd->independent_clocks = 1,
+ INIT_LIST_HEAD(&pd->domain_list);
+
+ for_each_possible_cpu(cpu) {
+ struct cpufreq_cpu_domain *new_domain;
+
+ if (cpu_in_domain_list(&pd->domain_list, cpu))
+ continue;
+
+ new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
+ if (!new_domain) {
+ cpufreq_dt_pdata_release();
+ return -ENOMEM;
+ }
+
+ cpumask_copy(&new_domain->cpus,
+ &cpu_topology[cpu].core_sibling);
+ list_add(&new_domain->node, &pd->domain_list);
+ }
+
+ return 0;
+}
+
+static void mt8173_cpufreq_dvfs_info_release(void)
+{
+ int i;
+
+ if (!dvfs_info)
+ return;
+
+ for (i = 0; i < NR_CLUSTERS; ++i) {
+ struct cluster_dvfs_info *dvfs = &dvfs_info[i];
+
+ if (!IS_ERR_OR_NULL(dvfs->proc_reg))
+ regulator_put(dvfs->proc_reg);
+
+ if (!IS_ERR_OR_NULL(dvfs->sram_reg))
+ regulator_put(dvfs->sram_reg);
+ }
+
+ if (clk_mux_regs)
+ iounmap(clk_mux_regs);
+}
+
+static int mt8173_cpufreq_dvfs_info_init(void)
+{
+ struct list_head *node;
+ struct device_node *np;
+ struct clk *mainpll;
+ int i, ret;
+
+ dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
+ if (!dvfs_info)
+ return -ENOMEM;
+
+ list_for_each(node, &pd->domain_list) {
+ struct cpufreq_cpu_domain *domain = container_of(
+ node, struct cpufreq_cpu_domain, node);
+ int cpu = cpumask_next(0, &domain->cpus);
+
+ np = of_get_cpu_node(cpu, NULL);
+
+ if (of_property_match_string(np, "compatible",
+ "arm,cortex-a53") >= 0)
+ cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
+ &domain->cpus);
+ else if (of_property_match_string(np, "compatible",
+ "arm,cortex-a57") >= 0)
+ cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
+ &domain->cpus);
+ else {
+ of_node_put(np);
+ pr_err("%s: unknown CPU core\n", __func__);
+ return -EINVAL;
+ }
+
+ of_node_put(np);
+ }
+
+ for (i = 0; i < NR_CLUSTERS; i++) {
+ struct cluster_dvfs_info *dvfs = &dvfs_info[i];
+ int cpu;
+
+ for_each_cpu_mask(cpu, dvfs->cpus) {
+ struct device_node *np;
+
+ dvfs->cpu_dev = get_cpu_device(cpu);
+ dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
+ if (IS_ERR(dvfs->proc_reg))
+ continue;
+
+ dvfs->cpu = cpu;
+ dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
+
+ np = of_node_get(dvfs->cpu_dev->of_node);
+ of_property_read_u32(np, "voltage-tolerance",
+ &dvfs->volt_tol);
+ of_node_put(np);
+ break;
+ }
+
+ if (IS_ERR(dvfs->proc_reg)) {
+ pr_err("%s: no proc regulator for cluster %d\n",
+ __func__, i);
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+
+ if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
+ pr_err("%s: no sram regulator for cluster %d\n",
+ __func__, i);
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+ }
+
+ mainpll = __clk_lookup("mainpll");
+ if (!mainpll) {
+ pr_err("failed to get mainpll clk\n");
+ ret = -ENOENT;
+ goto dvfs_info_release;
+ }
+ mainpll_freq = clk_get_rate(mainpll);
+
+ np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
+ if (!np) {
+ pr_err("failed to find clock mux registers\n");
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+
+ clk_mux_regs = of_iomap(np, 0);
+ if (!clk_mux_regs) {
+ pr_err("failed to map clock mux registers\n");
+ ret = -EFAULT;
+ goto dvfs_info_release;
+ }
+ of_node_put(np);
+
+ spin_lock_init(&lock);
+
+ return 0;
+
+dvfs_info_release:
+ mt8173_cpufreq_dvfs_info_release();
+ return ret;
+}
+
+static int mt8173_cpufreq_driver_init(void)
+{
+ int ret;
+
+ if (!of_machine_is_compatible("mediatek,mt8173"))
+ return -ENODEV;
+
+ ret = cpufreq_dt_pdata_init();
+ if (ret) {
+ pr_err("failed to initialize platform data: %d\n", ret);
+ return ret;
+ }
+
+ ret = mt8173_cpufreq_dvfs_info_init();
+ if (ret) {
+ pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
+ goto dt_pdata_release;
+ }
+
+ ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ if (ret) {
+ pr_err("failed to register cpufreq notifier: %d\n", ret);
+ goto dt_pdata_release;
+ }
+
+ platform_device_register_data(NULL, "cpufreq-dt", -1,
+ pd, sizeof(*pd));
+
+ return 0;
+
+dt_pdata_release:
+ cpufreq_dt_pdata_release();
+ return ret;
+}
+
+module_init(mt8173_cpufreq_driver_init);
+
+MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
+MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
+MODULE_LICENSE("GPL");
+
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-09 9:54 ` pi-cheng.chen
0 siblings, 0 replies; 34+ messages in thread
From: pi-cheng.chen @ 2015-01-09 9:54 UTC (permalink / raw)
To: linux-arm-kernel
When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
SoC works properly:
1. When doing frequency scaling of CPUs, the clock source should be switched
to another PLL, then switched back to the orignal until it's stable.
2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
controlled concurrently and 2 limitations should be followed:
a. Vsram > Vproc
b. Vsram - Vproc < 200 mV
To address these needs, we reuse cpufreq-dt but do voltage scaling in the
cpufreq notifier.
Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
---
drivers/cpufreq/Kconfig.arm | 6 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
3 files changed, 466 insertions(+)
create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0f9a2c3..97ed7dd 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
This add the CPUFreq driver support for Intel PXA2xx SOCs.
If in doubt, say N.
+
+config ARM_MT8173_CPUFREQ
+ bool "Mediatek MT8173 CPUFreq support"
+ depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
+ help
+ This adds the CPUFreq driver support for Mediatek MT8173 SoC.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index b3ca7b0..67b7f17 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
##################################################################################
# PowerPC platform drivers
diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
new file mode 100644
index 0000000..b578c10
--- /dev/null
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -0,0 +1,459 @@
+/*
+* Copyright (c) 2015 Linaro.
+* 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/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/cpufreq.h>
+#include <linux/cpufreq-dt.h>
+#include <linux/cpumask.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regulator/consumer.h>
+#include <linux/cpu.h>
+#include <linux/pm_opp.h>
+
+#define CPUCLK_MUX_ARMPLL 0x1
+#define CPUCLK_MUX_MAINPLL 0x2
+#define VOLT_SHIFT_LIMIT 150000
+
+enum {
+ LITTLE_CLUSTER = 0,
+ BIG_CLUSTER,
+ NR_CLUSTERS
+};
+
+static struct cluster_dvfs_info {
+ int cpu;
+ struct cpumask cpus;
+ struct device *cpu_dev;
+ struct regulator *proc_reg;
+ struct regulator *sram_reg;
+ int inter_volt;
+ u32 volt_tol;
+} *dvfs_info;
+
+static unsigned long mainpll_freq;
+static void __iomem *clk_mux_regs;
+static spinlock_t lock;
+
+static void cpuclk_mux_set(int cluster, u32 sel)
+{
+ u32 val;
+ u32 mask = 0x3;
+
+ if (cluster == BIG_CLUSTER) {
+ mask <<= 2;
+ sel <<= 2;
+ }
+
+ spin_lock(&lock);
+
+ val = readl(clk_mux_regs);
+ val = (val & ~mask) | sel;
+ writel(val, clk_mux_regs);
+
+ spin_unlock(&lock);
+}
+
+static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
+ unsigned long old_vproc,
+ unsigned long new_vproc)
+{
+ int cur_vsram, cur_vproc, target_volt, tol;
+
+ if (old_vproc > new_vproc) {
+ while (1) {
+ cur_vsram = regulator_get_voltage(dvfs->sram_reg);
+ if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
+ cur_vsram > new_vproc) {
+ tol = new_vproc * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ new_vproc, tol);
+ break;
+ }
+
+ target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
+ tol);
+
+ cur_vproc = regulator_get_voltage(dvfs->proc_reg);
+ target_volt = cur_vproc + 1;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
+ tol);
+ }
+ } else if (old_vproc < new_vproc) {
+ while (1) {
+ cur_vsram = regulator_get_voltage(dvfs->sram_reg);
+ if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
+ cur_vsram > new_vproc) {
+ tol = new_vproc * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ new_vproc, tol);
+ break;
+ }
+
+ cur_vproc = regulator_get_voltage(dvfs->proc_reg);
+ target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
+ tol);
+
+ if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
+ cur_vsram = regulator_get_voltage(
+ dvfs->sram_reg);
+ target_volt = cur_vsram - 1;
+ tol = target_volt * dvfs->volt_tol / 100;
+ regulator_set_voltage_tol(dvfs->proc_reg,
+ target_volt, tol);
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
+{
+ struct dev_pm_opp *opp;
+ int volt;
+
+ rcu_read_lock();
+ opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
+ if (IS_ERR(opp)) {
+ rcu_read_unlock();
+ pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
+ return PTR_ERR(opp);
+ }
+ volt = dev_pm_opp_get_voltage(opp);
+ rcu_read_unlock();
+
+ return volt;
+}
+
+static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
+{
+ struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
+
+ if (dvfs->inter_volt == 0)
+ dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
+ mainpll_freq);
+
+ return dvfs->inter_volt;
+}
+
+static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
+{
+ struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
+ int ret = 0;
+
+ if (cluster == LITTLE_CLUSTER) {
+ int tol;
+
+ tol = new_volt * dvfs->volt_tol / 100;
+ ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
+ } else {
+ ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
+ }
+
+ if (ret)
+ pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
+ __func__, cluster, old_volt, new_volt);
+}
+
+static int mt8173_cpufreq_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct cpufreq_freqs *freqs = data;
+ struct cluster_dvfs_info *dvfs;
+ unsigned long old_volt, new_volt, inter_volt;
+ int cluster;
+
+ if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
+ cluster = BIG_CLUSTER;
+ else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
+ cluster = LITTLE_CLUSTER;
+ else
+ return NOTIFY_DONE;
+
+ dvfs = &dvfs_info[cluster];
+
+ old_volt = regulator_get_voltage(dvfs->proc_reg);
+
+ new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
+ inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
+
+ if (action == CPUFREQ_PRECHANGE) {
+ if (old_volt < inter_volt || old_volt < new_volt) {
+ new_volt = inter_volt > new_volt ?
+ inter_volt : new_volt;
+ mt8173_cpufreq_set_voltage(cluster, old_volt,
+ new_volt);
+ }
+
+ cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
+ } else if (action == CPUFREQ_POSTCHANGE) {
+ cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
+
+ if (new_volt < old_volt)
+ mt8173_cpufreq_set_voltage(cluster, old_volt,
+ new_volt);
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block mt8173_cpufreq_nb = {
+ .notifier_call = mt8173_cpufreq_notify,
+};
+
+static struct cpufreq_dt_platform_data *pd;
+
+static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
+{
+ struct list_head *node;
+
+ list_for_each(node, domain_list) {
+ struct cpufreq_cpu_domain *domain;
+
+ domain = container_of(node, struct cpufreq_cpu_domain, node);
+ if (cpumask_test_cpu(cpu, &domain->cpus))
+ return 1;
+ }
+
+ return 0;
+}
+
+static void cpufreq_dt_pdata_release(void)
+{
+ if (!pd)
+ return;
+
+ if (!list_empty(&pd->domain_list)) {
+ struct list_head *node, *tmp;
+ struct cpufreq_cpu_domain *domain;
+
+ list_for_each_safe(node, tmp, &pd->domain_list) {
+ list_del(node);
+ domain = container_of(node, struct cpufreq_cpu_domain,
+ node);
+ kfree(domain);
+ }
+ }
+
+ kfree(pd);
+}
+
+static int cpufreq_dt_pdata_init(void)
+{
+ int cpu;
+
+ pd = kmalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ pd->independent_clocks = 1,
+ INIT_LIST_HEAD(&pd->domain_list);
+
+ for_each_possible_cpu(cpu) {
+ struct cpufreq_cpu_domain *new_domain;
+
+ if (cpu_in_domain_list(&pd->domain_list, cpu))
+ continue;
+
+ new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
+ if (!new_domain) {
+ cpufreq_dt_pdata_release();
+ return -ENOMEM;
+ }
+
+ cpumask_copy(&new_domain->cpus,
+ &cpu_topology[cpu].core_sibling);
+ list_add(&new_domain->node, &pd->domain_list);
+ }
+
+ return 0;
+}
+
+static void mt8173_cpufreq_dvfs_info_release(void)
+{
+ int i;
+
+ if (!dvfs_info)
+ return;
+
+ for (i = 0; i < NR_CLUSTERS; ++i) {
+ struct cluster_dvfs_info *dvfs = &dvfs_info[i];
+
+ if (!IS_ERR_OR_NULL(dvfs->proc_reg))
+ regulator_put(dvfs->proc_reg);
+
+ if (!IS_ERR_OR_NULL(dvfs->sram_reg))
+ regulator_put(dvfs->sram_reg);
+ }
+
+ if (clk_mux_regs)
+ iounmap(clk_mux_regs);
+}
+
+static int mt8173_cpufreq_dvfs_info_init(void)
+{
+ struct list_head *node;
+ struct device_node *np;
+ struct clk *mainpll;
+ int i, ret;
+
+ dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
+ if (!dvfs_info)
+ return -ENOMEM;
+
+ list_for_each(node, &pd->domain_list) {
+ struct cpufreq_cpu_domain *domain = container_of(
+ node, struct cpufreq_cpu_domain, node);
+ int cpu = cpumask_next(0, &domain->cpus);
+
+ np = of_get_cpu_node(cpu, NULL);
+
+ if (of_property_match_string(np, "compatible",
+ "arm,cortex-a53") >= 0)
+ cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
+ &domain->cpus);
+ else if (of_property_match_string(np, "compatible",
+ "arm,cortex-a57") >= 0)
+ cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
+ &domain->cpus);
+ else {
+ of_node_put(np);
+ pr_err("%s: unknown CPU core\n", __func__);
+ return -EINVAL;
+ }
+
+ of_node_put(np);
+ }
+
+ for (i = 0; i < NR_CLUSTERS; i++) {
+ struct cluster_dvfs_info *dvfs = &dvfs_info[i];
+ int cpu;
+
+ for_each_cpu_mask(cpu, dvfs->cpus) {
+ struct device_node *np;
+
+ dvfs->cpu_dev = get_cpu_device(cpu);
+ dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
+ if (IS_ERR(dvfs->proc_reg))
+ continue;
+
+ dvfs->cpu = cpu;
+ dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
+
+ np = of_node_get(dvfs->cpu_dev->of_node);
+ of_property_read_u32(np, "voltage-tolerance",
+ &dvfs->volt_tol);
+ of_node_put(np);
+ break;
+ }
+
+ if (IS_ERR(dvfs->proc_reg)) {
+ pr_err("%s: no proc regulator for cluster %d\n",
+ __func__, i);
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+
+ if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
+ pr_err("%s: no sram regulator for cluster %d\n",
+ __func__, i);
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+ }
+
+ mainpll = __clk_lookup("mainpll");
+ if (!mainpll) {
+ pr_err("failed to get mainpll clk\n");
+ ret = -ENOENT;
+ goto dvfs_info_release;
+ }
+ mainpll_freq = clk_get_rate(mainpll);
+
+ np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
+ if (!np) {
+ pr_err("failed to find clock mux registers\n");
+ ret = -ENODEV;
+ goto dvfs_info_release;
+ }
+
+ clk_mux_regs = of_iomap(np, 0);
+ if (!clk_mux_regs) {
+ pr_err("failed to map clock mux registers\n");
+ ret = -EFAULT;
+ goto dvfs_info_release;
+ }
+ of_node_put(np);
+
+ spin_lock_init(&lock);
+
+ return 0;
+
+dvfs_info_release:
+ mt8173_cpufreq_dvfs_info_release();
+ return ret;
+}
+
+static int mt8173_cpufreq_driver_init(void)
+{
+ int ret;
+
+ if (!of_machine_is_compatible("mediatek,mt8173"))
+ return -ENODEV;
+
+ ret = cpufreq_dt_pdata_init();
+ if (ret) {
+ pr_err("failed to initialize platform data: %d\n", ret);
+ return ret;
+ }
+
+ ret = mt8173_cpufreq_dvfs_info_init();
+ if (ret) {
+ pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
+ goto dt_pdata_release;
+ }
+
+ ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ if (ret) {
+ pr_err("failed to register cpufreq notifier: %d\n", ret);
+ goto dt_pdata_release;
+ }
+
+ platform_device_register_data(NULL, "cpufreq-dt", -1,
+ pd, sizeof(*pd));
+
+ return 0;
+
+dt_pdata_release:
+ cpufreq_dt_pdata_release();
+ return ret;
+}
+
+module_init(mt8173_cpufreq_driver_init);
+
+MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
+MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
+MODULE_LICENSE("GPL");
+
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` pi-cheng.chen
@ 2015-01-14 7:43 ` Yingjoe Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Yingjoe Chen @ 2015-01-14 7:43 UTC (permalink / raw)
To: pi-cheng.chen
Cc: Thomas Petazzoni, linaro-kernel, Mike Turquette, linux-pm,
Viresh Kumar, Rafael J. Wysocki, linux-kernel, Matthias Brugger,
Eddie Huang, linux-arm-kernel
Hi,
On Fri, 2015-01-09 at 17:54 +0800, pi-cheng.chen wrote:
> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
> SoC works properly:
> 1. When doing frequency scaling of CPUs, the clock source should be switched
> to another PLL, then switched back to the orignal until it's stable.
> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
> controlled concurrently and 2 limitations should be followed:
> a. Vsram > Vproc
> b. Vsram - Vproc < 200 mV
It seems to me this is not much different then other mtk big little
SoCs. Is it possible to aim to support both mt8173 & mt8135 with this
driver?
> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
> cpufreq notifier.
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 6 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0f9a2c3..97ed7dd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_MT8173_CPUFREQ
> + bool "Mediatek MT8173 CPUFreq support"
> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
> + help
> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b3ca7b0..67b7f17 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
> +/*
> +* Copyright (c) 2015 Linaro.
> +* 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/pm_opp.h>
> +
> +#define CPUCLK_MUX_ARMPLL 0x1
> +#define CPUCLK_MUX_MAINPLL 0x2
> +#define VOLT_SHIFT_LIMIT 150000
> +
> +enum {
> + LITTLE_CLUSTER = 0,
> + BIG_CLUSTER,
> + NR_CLUSTERS
> +};
> +
> +static struct cluster_dvfs_info {
> + int cpu;
> + struct cpumask cpus;
> + struct device *cpu_dev;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + int inter_volt;
> + u32 volt_tol;
> +} *dvfs_info;
> +
> +static unsigned long mainpll_freq;
> +static void __iomem *clk_mux_regs;
> +static spinlock_t lock;
> +
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
> +
> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
> + unsigned long old_vproc,
> + unsigned long new_vproc)
> +{
> + int cur_vsram, cur_vproc, target_volt, tol;
> +
> + if (old_vproc > new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
I don't quite get the logic for tol calculation here. What's the
expected value for volt_tol? Care to explain?
> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
> + tol);
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> + }
> + } else if (old_vproc < new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> +
> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
> + cur_vsram = regulator_get_voltage(
> + dvfs->sram_reg);
> + target_volt = cur_vsram - 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + target_volt, tol);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
> +{
> + struct dev_pm_opp *opp;
> + int volt;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + return volt;
> +}
> +
> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> +
> + if (dvfs->inter_volt == 0)
> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
> + mainpll_freq);
> +
> + return dvfs->inter_volt;
> +}
> +
> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> + int ret = 0;
> +
> + if (cluster == LITTLE_CLUSTER) {
> + int tol;
> +
> + tol = new_volt * dvfs->volt_tol / 100;
> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
> + } else {
> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
> + }
> +
> + if (ret)
> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
> + __func__, cluster, old_volt, new_volt);
> +}
It seems the only reason to check if it is a BIG or LITTLE cluster is to
control vsram correctly. If we assume we need to do the control whenever
sram_reg exists, we can drop all the BIG/LITTLE cluster check. I think
this will make code looks more compact and generic.
> +
> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct cpufreq_freqs *freqs = data;
> + struct cluster_dvfs_info *dvfs;
> + unsigned long old_volt, new_volt, inter_volt;
> + int cluster;
> +
> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
> + cluster = BIG_CLUSTER;
> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
> + cluster = LITTLE_CLUSTER;
> + else
> + return NOTIFY_DONE;
> +
> + dvfs = &dvfs_info[cluster];
> +
> + old_volt = regulator_get_voltage(dvfs->proc_reg);
> +
> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
This is only used in PRECHANGE, please move this inside the if.
> +
> + if (action == CPUFREQ_PRECHANGE) {
> + if (old_volt < inter_volt || old_volt < new_volt) {
> + new_volt = inter_volt > new_volt ?
> + inter_volt : new_volt;
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
> + } else if (action == CPUFREQ_POSTCHANGE) {
> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
> +
> + if (new_volt < old_volt)
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mt8173_cpufreq_nb = {
> + .notifier_call = mt8173_cpufreq_notify,
> +};
> +
> +static struct cpufreq_dt_platform_data *pd;
Please drop this global variable. You can return it from
cpufreq_dt_pdata_init and pass to all the function need it.
> +
> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
> +{
> + struct list_head *node;
> +
> + list_for_each(node, domain_list) {
> + struct cpufreq_cpu_domain *domain;
> +
> + domain = container_of(node, struct cpufreq_cpu_domain, node);
> + if (cpumask_test_cpu(cpu, &domain->cpus))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_dt_pdata_release(void)
> +{
> + if (!pd)
> + return;
> +
> + if (!list_empty(&pd->domain_list)) {
> + struct list_head *node, *tmp;
> + struct cpufreq_cpu_domain *domain;
> +
> + list_for_each_safe(node, tmp, &pd->domain_list) {
> + list_del(node);
> + domain = container_of(node, struct cpufreq_cpu_domain,
> + node);
> + kfree(domain);
> + }
> + }
> +
> + kfree(pd);
> +}
> +
> +static int cpufreq_dt_pdata_init(void)
> +{
> + int cpu;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + pd->independent_clocks = 1,
> + INIT_LIST_HEAD(&pd->domain_list);
> +
> + for_each_possible_cpu(cpu) {
> + struct cpufreq_cpu_domain *new_domain;
> +
> + if (cpu_in_domain_list(&pd->domain_list, cpu))
> + continue;
> +
> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
> + if (!new_domain) {
> + cpufreq_dt_pdata_release();
> + return -ENOMEM;
> + }
> +
> + cpumask_copy(&new_domain->cpus,
> + &cpu_topology[cpu].core_sibling);
> + list_add(&new_domain->node, &pd->domain_list);
> + }
> +
> + return 0;
> +}
> +
> +static void mt8173_cpufreq_dvfs_info_release(void)
> +{
> + int i;
> +
> + if (!dvfs_info)
> + return;
> +
> + for (i = 0; i < NR_CLUSTERS; ++i) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> +
> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
> + regulator_put(dvfs->proc_reg);
> +
> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
> + regulator_put(dvfs->sram_reg);
> + }
> +
> + if (clk_mux_regs)
> + iounmap(clk_mux_regs);
> +}
> +
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
> + struct list_head *node;
> + struct device_node *np;
> + struct clk *mainpll;
> + int i, ret;
> +
> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
> + if (!dvfs_info)
> + return -ENOMEM;
> +
> + list_for_each(node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain = container_of(
> + node, struct cpufreq_cpu_domain, node);
> + int cpu = cpumask_next(0, &domain->cpus);
> +
> + np = of_get_cpu_node(cpu, NULL);
> +
> + if (of_property_match_string(np, "compatible",
> + "arm,cortex-a53") >= 0)
> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
> + &domain->cpus);
> + else if (of_property_match_string(np, "compatible",
> + "arm,cortex-a57") >= 0)
> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
> + &domain->cpus);
> + else {
> + of_node_put(np);
> + pr_err("%s: unknown CPU core\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> + }
It seems you hardcode the match CPU here to know which is big/little
cluster. If we still need this information, should we add this to
topology, maybe something similar to capacity in arm topology, instead
of coding here?
> +
> + for (i = 0; i < NR_CLUSTERS; i++) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> + int cpu;
> +
> + for_each_cpu_mask(cpu, dvfs->cpus) {
> + struct device_node *np;
> +
> + dvfs->cpu_dev = get_cpu_device(cpu);
> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
> + if (IS_ERR(dvfs->proc_reg))
> + continue;
> +
> + dvfs->cpu = cpu;
> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
> +
> + np = of_node_get(dvfs->cpu_dev->of_node);
> + of_property_read_u32(np, "voltage-tolerance",
> + &dvfs->volt_tol);
> + of_node_put(np);
> + break;
> + }
> +
> + if (IS_ERR(dvfs->proc_reg)) {
> + pr_err("%s: no proc regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
> + pr_err("%s: no sram regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> + }
> +
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
> +
> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
> + if (!np) {
> + pr_err("failed to find clock mux registers\n");
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + clk_mux_regs = of_iomap(np, 0);
> + if (!clk_mux_regs) {
> + pr_err("failed to map clock mux registers\n");
> + ret = -EFAULT;
> + goto dvfs_info_release;
> + }
> + of_node_put(np);
This is already used by mt8173 clock driver, and you are directly
accessing it registers to control clock mux. I think you should add
these 2 clk to clock driver and use clk API to control it. Also you can
drop the lock init below if you do this.
Joe.C
> +
> + spin_lock_init(&lock);
> +
> + return 0;
> +
> +dvfs_info_release:
> + mt8173_cpufreq_dvfs_info_release();
> + return ret;
> +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-14 7:43 ` Yingjoe Chen
0 siblings, 0 replies; 34+ messages in thread
From: Yingjoe Chen @ 2015-01-14 7:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, 2015-01-09 at 17:54 +0800, pi-cheng.chen wrote:
> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
> SoC works properly:
> 1. When doing frequency scaling of CPUs, the clock source should be switched
> to another PLL, then switched back to the orignal until it's stable.
> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
> controlled concurrently and 2 limitations should be followed:
> a. Vsram > Vproc
> b. Vsram - Vproc < 200 mV
It seems to me this is not much different then other mtk big little
SoCs. Is it possible to aim to support both mt8173 & mt8135 with this
driver?
> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
> cpufreq notifier.
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 6 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0f9a2c3..97ed7dd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_MT8173_CPUFREQ
> + bool "Mediatek MT8173 CPUFreq support"
> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
> + help
> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b3ca7b0..67b7f17 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
> +/*
> +* Copyright (c) 2015 Linaro.
> +* 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/pm_opp.h>
> +
> +#define CPUCLK_MUX_ARMPLL 0x1
> +#define CPUCLK_MUX_MAINPLL 0x2
> +#define VOLT_SHIFT_LIMIT 150000
> +
> +enum {
> + LITTLE_CLUSTER = 0,
> + BIG_CLUSTER,
> + NR_CLUSTERS
> +};
> +
> +static struct cluster_dvfs_info {
> + int cpu;
> + struct cpumask cpus;
> + struct device *cpu_dev;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + int inter_volt;
> + u32 volt_tol;
> +} *dvfs_info;
> +
> +static unsigned long mainpll_freq;
> +static void __iomem *clk_mux_regs;
> +static spinlock_t lock;
> +
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
> +
> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
> + unsigned long old_vproc,
> + unsigned long new_vproc)
> +{
> + int cur_vsram, cur_vproc, target_volt, tol;
> +
> + if (old_vproc > new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
I don't quite get the logic for tol calculation here. What's the
expected value for volt_tol? Care to explain?
> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
> + tol);
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> + }
> + } else if (old_vproc < new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> +
> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
> + cur_vsram = regulator_get_voltage(
> + dvfs->sram_reg);
> + target_volt = cur_vsram - 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + target_volt, tol);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
> +{
> + struct dev_pm_opp *opp;
> + int volt;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + return volt;
> +}
> +
> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> +
> + if (dvfs->inter_volt == 0)
> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
> + mainpll_freq);
> +
> + return dvfs->inter_volt;
> +}
> +
> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> + int ret = 0;
> +
> + if (cluster == LITTLE_CLUSTER) {
> + int tol;
> +
> + tol = new_volt * dvfs->volt_tol / 100;
> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
> + } else {
> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
> + }
> +
> + if (ret)
> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
> + __func__, cluster, old_volt, new_volt);
> +}
It seems the only reason to check if it is a BIG or LITTLE cluster is to
control vsram correctly. If we assume we need to do the control whenever
sram_reg exists, we can drop all the BIG/LITTLE cluster check. I think
this will make code looks more compact and generic.
> +
> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct cpufreq_freqs *freqs = data;
> + struct cluster_dvfs_info *dvfs;
> + unsigned long old_volt, new_volt, inter_volt;
> + int cluster;
> +
> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
> + cluster = BIG_CLUSTER;
> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
> + cluster = LITTLE_CLUSTER;
> + else
> + return NOTIFY_DONE;
> +
> + dvfs = &dvfs_info[cluster];
> +
> + old_volt = regulator_get_voltage(dvfs->proc_reg);
> +
> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
This is only used in PRECHANGE, please move this inside the if.
> +
> + if (action == CPUFREQ_PRECHANGE) {
> + if (old_volt < inter_volt || old_volt < new_volt) {
> + new_volt = inter_volt > new_volt ?
> + inter_volt : new_volt;
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
> + } else if (action == CPUFREQ_POSTCHANGE) {
> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
> +
> + if (new_volt < old_volt)
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mt8173_cpufreq_nb = {
> + .notifier_call = mt8173_cpufreq_notify,
> +};
> +
> +static struct cpufreq_dt_platform_data *pd;
Please drop this global variable. You can return it from
cpufreq_dt_pdata_init and pass to all the function need it.
> +
> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
> +{
> + struct list_head *node;
> +
> + list_for_each(node, domain_list) {
> + struct cpufreq_cpu_domain *domain;
> +
> + domain = container_of(node, struct cpufreq_cpu_domain, node);
> + if (cpumask_test_cpu(cpu, &domain->cpus))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_dt_pdata_release(void)
> +{
> + if (!pd)
> + return;
> +
> + if (!list_empty(&pd->domain_list)) {
> + struct list_head *node, *tmp;
> + struct cpufreq_cpu_domain *domain;
> +
> + list_for_each_safe(node, tmp, &pd->domain_list) {
> + list_del(node);
> + domain = container_of(node, struct cpufreq_cpu_domain,
> + node);
> + kfree(domain);
> + }
> + }
> +
> + kfree(pd);
> +}
> +
> +static int cpufreq_dt_pdata_init(void)
> +{
> + int cpu;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + pd->independent_clocks = 1,
> + INIT_LIST_HEAD(&pd->domain_list);
> +
> + for_each_possible_cpu(cpu) {
> + struct cpufreq_cpu_domain *new_domain;
> +
> + if (cpu_in_domain_list(&pd->domain_list, cpu))
> + continue;
> +
> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
> + if (!new_domain) {
> + cpufreq_dt_pdata_release();
> + return -ENOMEM;
> + }
> +
> + cpumask_copy(&new_domain->cpus,
> + &cpu_topology[cpu].core_sibling);
> + list_add(&new_domain->node, &pd->domain_list);
> + }
> +
> + return 0;
> +}
> +
> +static void mt8173_cpufreq_dvfs_info_release(void)
> +{
> + int i;
> +
> + if (!dvfs_info)
> + return;
> +
> + for (i = 0; i < NR_CLUSTERS; ++i) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> +
> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
> + regulator_put(dvfs->proc_reg);
> +
> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
> + regulator_put(dvfs->sram_reg);
> + }
> +
> + if (clk_mux_regs)
> + iounmap(clk_mux_regs);
> +}
> +
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
> + struct list_head *node;
> + struct device_node *np;
> + struct clk *mainpll;
> + int i, ret;
> +
> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
> + if (!dvfs_info)
> + return -ENOMEM;
> +
> + list_for_each(node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain = container_of(
> + node, struct cpufreq_cpu_domain, node);
> + int cpu = cpumask_next(0, &domain->cpus);
> +
> + np = of_get_cpu_node(cpu, NULL);
> +
> + if (of_property_match_string(np, "compatible",
> + "arm,cortex-a53") >= 0)
> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
> + &domain->cpus);
> + else if (of_property_match_string(np, "compatible",
> + "arm,cortex-a57") >= 0)
> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
> + &domain->cpus);
> + else {
> + of_node_put(np);
> + pr_err("%s: unknown CPU core\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> + }
It seems you hardcode the match CPU here to know which is big/little
cluster. If we still need this information, should we add this to
topology, maybe something similar to capacity in arm topology, instead
of coding here?
> +
> + for (i = 0; i < NR_CLUSTERS; i++) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> + int cpu;
> +
> + for_each_cpu_mask(cpu, dvfs->cpus) {
> + struct device_node *np;
> +
> + dvfs->cpu_dev = get_cpu_device(cpu);
> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
> + if (IS_ERR(dvfs->proc_reg))
> + continue;
> +
> + dvfs->cpu = cpu;
> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
> +
> + np = of_node_get(dvfs->cpu_dev->of_node);
> + of_property_read_u32(np, "voltage-tolerance",
> + &dvfs->volt_tol);
> + of_node_put(np);
> + break;
> + }
> +
> + if (IS_ERR(dvfs->proc_reg)) {
> + pr_err("%s: no proc regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
> + pr_err("%s: no sram regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> + }
> +
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
> +
> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
> + if (!np) {
> + pr_err("failed to find clock mux registers\n");
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + clk_mux_regs = of_iomap(np, 0);
> + if (!clk_mux_regs) {
> + pr_err("failed to map clock mux registers\n");
> + ret = -EFAULT;
> + goto dvfs_info_release;
> + }
> + of_node_put(np);
This is already used by mt8173 clock driver, and you are directly
accessing it registers to control clock mux. I think you should add
these 2 clk to clock driver and use clk API to control it. Also you can
drop the lock init below if you do this.
Joe.C
> +
> + spin_lock_init(&lock);
> +
> + return 0;
> +
> +dvfs_info_release:
> + mt8173_cpufreq_dvfs_info_release();
> + return ret;
> +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-14 7:43 ` Yingjoe Chen
@ 2015-01-15 2:02 ` Pi-Cheng Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-15 2:02 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger,
Thomas Petazzoni, Linaro Kernel Mailman List, Mike Turquette,
linux-pm, linux-kernel, Eddie Huang, linux-arm-kernel
Hi Joe,
Thanks for reviewing.
On 14 January 2015 at 15:43, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>
> Hi,
>
> On Fri, 2015-01-09 at 17:54 +0800, pi-cheng.chen wrote:
>> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
>> SoC works properly:
>> 1. When doing frequency scaling of CPUs, the clock source should be switched
>> to another PLL, then switched back to the orignal until it's stable.
>> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
>> controlled concurrently and 2 limitations should be followed:
>> a. Vsram > Vproc
>> b. Vsram - Vproc < 200 mV
>
> It seems to me this is not much different then other mtk big little
> SoCs. Is it possible to aim to support both mt8173 & mt8135 with this
> driver?
>
Yes. I will try to make it more flexibile so that the driver can support both
mt8173 and mt8135.
>> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
>> cpufreq notifier.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 6 +
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 466 insertions(+)
>> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0f9a2c3..97ed7dd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_MT8173_CPUFREQ
>> + bool "Mediatek MT8173 CPUFreq support"
>> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
>> + help
>> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index b3ca7b0..67b7f17 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>>
>> ##################################################################################
>> # PowerPC platform drivers
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
>> +/*
>> +* Copyright (c) 2015 Linaro.
>> +* 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/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu.h>
>> +#include <linux/pm_opp.h>
>> +
>> +#define CPUCLK_MUX_ARMPLL 0x1
>> +#define CPUCLK_MUX_MAINPLL 0x2
>> +#define VOLT_SHIFT_LIMIT 150000
>> +
>> +enum {
>> + LITTLE_CLUSTER = 0,
>> + BIG_CLUSTER,
>> + NR_CLUSTERS
>> +};
>> +
>> +static struct cluster_dvfs_info {
>> + int cpu;
>> + struct cpumask cpus;
>> + struct device *cpu_dev;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + int inter_volt;
>> + u32 volt_tol;
>> +} *dvfs_info;
>> +
>> +static unsigned long mainpll_freq;
>> +static void __iomem *clk_mux_regs;
>> +static spinlock_t lock;
>> +
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>> +
>> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
>> + unsigned long old_vproc,
>> + unsigned long new_vproc)
>> +{
>> + int cur_vsram, cur_vproc, target_volt, tol;
>> +
>> + if (old_vproc > new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>
> I don't quite get the logic for tol calculation here. What's the
> expected value for volt_tol? Care to explain?
>
Here I am trying to handle the problem: the voltage value can be set to
the regulator might be different from the voltage value got from OPP table.
And I am realizing that it might not be a good way to solve the problem. I
will turn to use regulator API to query the supported voltage values to get
rid of voltage tolerance calculation.
>> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
>> + tol);
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> + }
>> + } else if (old_vproc < new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> +
>> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
>> + cur_vsram = regulator_get_voltage(
>> + dvfs->sram_reg);
>> + target_volt = cur_vsram - 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + target_volt, tol);
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
>> +{
>> + struct dev_pm_opp *opp;
>> + int volt;
>> +
>> + rcu_read_lock();
>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
>> + return PTR_ERR(opp);
>> + }
>> + volt = dev_pm_opp_get_voltage(opp);
>> + rcu_read_unlock();
>> +
>> + return volt;
>> +}
>> +
>> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> +
>> + if (dvfs->inter_volt == 0)
>> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
>> + mainpll_freq);
>> +
>> + return dvfs->inter_volt;
>> +}
>> +
>> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> + int ret = 0;
>> +
>> + if (cluster == LITTLE_CLUSTER) {
>> + int tol;
>> +
>> + tol = new_volt * dvfs->volt_tol / 100;
>> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
>> + } else {
>> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
>> + }
>> +
>> + if (ret)
>> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
>> + __func__, cluster, old_volt, new_volt);
>> +}
>
> It seems the only reason to check if it is a BIG or LITTLE cluster is to
> control vsram correctly. If we assume we need to do the control whenever
> sram_reg exists, we can drop all the BIG/LITTLE cluster check. I think
> this will make code looks more compact and generic.
>
Yes. will fix it.
>> +
>> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct cpufreq_freqs *freqs = data;
>> + struct cluster_dvfs_info *dvfs;
>> + unsigned long old_volt, new_volt, inter_volt;
>> + int cluster;
>> +
>> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
>> + cluster = BIG_CLUSTER;
>> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
>> + cluster = LITTLE_CLUSTER;
>> + else
>> + return NOTIFY_DONE;
>> +
>> + dvfs = &dvfs_info[cluster];
>> +
>> + old_volt = regulator_get_voltage(dvfs->proc_reg);
>> +
>> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
>> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
>
> This is only used in PRECHANGE, please move this inside the if.
>
Yes. will fix it.
>> +
>> + if (action == CPUFREQ_PRECHANGE) {
>> + if (old_volt < inter_volt || old_volt < new_volt) {
>> + new_volt = inter_volt > new_volt ?
>> + inter_volt : new_volt;
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
>> + } else if (action == CPUFREQ_POSTCHANGE) {
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
>> +
>> + if (new_volt < old_volt)
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block mt8173_cpufreq_nb = {
>> + .notifier_call = mt8173_cpufreq_notify,
>> +};
>> +
>> +static struct cpufreq_dt_platform_data *pd;
>
> Please drop this global variable. You can return it from
> cpufreq_dt_pdata_init and pass to all the function need it.
>
Yes. will fix it.
>> +
>> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
>> +{
>> + struct list_head *node;
>> +
>> + list_for_each(node, domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + domain = container_of(node, struct cpufreq_cpu_domain, node);
>> + if (cpumask_test_cpu(cpu, &domain->cpus))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_dt_pdata_release(void)
>> +{
>> + if (!pd)
>> + return;
>> +
>> + if (!list_empty(&pd->domain_list)) {
>> + struct list_head *node, *tmp;
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + list_for_each_safe(node, tmp, &pd->domain_list) {
>> + list_del(node);
>> + domain = container_of(node, struct cpufreq_cpu_domain,
>> + node);
>> + kfree(domain);
>> + }
>> + }
>> +
>> + kfree(pd);
>> +}
>> +
>> +static int cpufreq_dt_pdata_init(void)
>> +{
>> + int cpu;
>> +
>> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + pd->independent_clocks = 1,
>> + INIT_LIST_HEAD(&pd->domain_list);
>> +
>> + for_each_possible_cpu(cpu) {
>> + struct cpufreq_cpu_domain *new_domain;
>> +
>> + if (cpu_in_domain_list(&pd->domain_list, cpu))
>> + continue;
>> +
>> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
>> + if (!new_domain) {
>> + cpufreq_dt_pdata_release();
>> + return -ENOMEM;
>> + }
>> +
>> + cpumask_copy(&new_domain->cpus,
>> + &cpu_topology[cpu].core_sibling);
>> + list_add(&new_domain->node, &pd->domain_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void mt8173_cpufreq_dvfs_info_release(void)
>> +{
>> + int i;
>> +
>> + if (!dvfs_info)
>> + return;
>> +
>> + for (i = 0; i < NR_CLUSTERS; ++i) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
>> + regulator_put(dvfs->proc_reg);
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
>> + regulator_put(dvfs->sram_reg);
>> + }
>> +
>> + if (clk_mux_regs)
>> + iounmap(clk_mux_regs);
>> +}
>> +
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>> + struct list_head *node;
>> + struct device_node *np;
>> + struct clk *mainpll;
>> + int i, ret;
>> +
>> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
>> + if (!dvfs_info)
>> + return -ENOMEM;
>> +
>> + list_for_each(node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain = container_of(
>> + node, struct cpufreq_cpu_domain, node);
>> + int cpu = cpumask_next(0, &domain->cpus);
>> +
>> + np = of_get_cpu_node(cpu, NULL);
>> +
>> + if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a53") >= 0)
>> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
>> + &domain->cpus);
>> + else if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a57") >= 0)
>> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
>> + &domain->cpus);
>> + else {
>> + of_node_put(np);
>> + pr_err("%s: unknown CPU core\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(np);
>> + }
>
> It seems you hardcode the match CPU here to know which is big/little
> cluster. If we still need this information, should we add this to
> topology, maybe something similar to capacity in arm topology, instead
> of coding here?
>
I am not sure if there's such information I could use here. I will check it
further. Probably I don't even need this information if I just use the
presence of dvfs->sram_reg to identify the cluster.
>> +
>> + for (i = 0; i < NR_CLUSTERS; i++) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> + int cpu;
>> +
>> + for_each_cpu_mask(cpu, dvfs->cpus) {
>> + struct device_node *np;
>> +
>> + dvfs->cpu_dev = get_cpu_device(cpu);
>> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
>> + if (IS_ERR(dvfs->proc_reg))
>> + continue;
>> +
>> + dvfs->cpu = cpu;
>> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
>> +
>> + np = of_node_get(dvfs->cpu_dev->of_node);
>> + of_property_read_u32(np, "voltage-tolerance",
>> + &dvfs->volt_tol);
>> + of_node_put(np);
>> + break;
>> + }
>> +
>> + if (IS_ERR(dvfs->proc_reg)) {
>> + pr_err("%s: no proc regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
>> + pr_err("%s: no sram regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> + }
>> +
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>> +
>> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
>> + if (!np) {
>> + pr_err("failed to find clock mux registers\n");
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + clk_mux_regs = of_iomap(np, 0);
>> + if (!clk_mux_regs) {
>> + pr_err("failed to map clock mux registers\n");
>> + ret = -EFAULT;
>> + goto dvfs_info_release;
>> + }
>> + of_node_put(np);
>
> This is already used by mt8173 clock driver, and you are directly
> accessing it registers to control clock mux. I think you should add
> these 2 clk to clock driver and use clk API to control it. Also you can
> drop the lock init below if you do this.
>
> Joe.C
>
Yes. I think I need to have some discussion about this with the guy
upstreaming CCF support for MT8173.
>> +
>> + spin_lock_init(&lock);
>> +
>> + return 0;
>> +
>> +dvfs_info_release:
>> + mt8173_cpufreq_dvfs_info_release();
>> + return ret;
>> +}
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-15 2:02 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-15 2:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi Joe,
Thanks for reviewing.
On 14 January 2015 at 15:43, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>
> Hi,
>
> On Fri, 2015-01-09 at 17:54 +0800, pi-cheng.chen wrote:
>> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
>> SoC works properly:
>> 1. When doing frequency scaling of CPUs, the clock source should be switched
>> to another PLL, then switched back to the orignal until it's stable.
>> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
>> controlled concurrently and 2 limitations should be followed:
>> a. Vsram > Vproc
>> b. Vsram - Vproc < 200 mV
>
> It seems to me this is not much different then other mtk big little
> SoCs. Is it possible to aim to support both mt8173 & mt8135 with this
> driver?
>
Yes. I will try to make it more flexibile so that the driver can support both
mt8173 and mt8135.
>> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
>> cpufreq notifier.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 6 +
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 466 insertions(+)
>> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0f9a2c3..97ed7dd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_MT8173_CPUFREQ
>> + bool "Mediatek MT8173 CPUFreq support"
>> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
>> + help
>> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index b3ca7b0..67b7f17 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>>
>> ##################################################################################
>> # PowerPC platform drivers
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
>> +/*
>> +* Copyright (c) 2015 Linaro.
>> +* 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/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu.h>
>> +#include <linux/pm_opp.h>
>> +
>> +#define CPUCLK_MUX_ARMPLL 0x1
>> +#define CPUCLK_MUX_MAINPLL 0x2
>> +#define VOLT_SHIFT_LIMIT 150000
>> +
>> +enum {
>> + LITTLE_CLUSTER = 0,
>> + BIG_CLUSTER,
>> + NR_CLUSTERS
>> +};
>> +
>> +static struct cluster_dvfs_info {
>> + int cpu;
>> + struct cpumask cpus;
>> + struct device *cpu_dev;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + int inter_volt;
>> + u32 volt_tol;
>> +} *dvfs_info;
>> +
>> +static unsigned long mainpll_freq;
>> +static void __iomem *clk_mux_regs;
>> +static spinlock_t lock;
>> +
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>> +
>> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
>> + unsigned long old_vproc,
>> + unsigned long new_vproc)
>> +{
>> + int cur_vsram, cur_vproc, target_volt, tol;
>> +
>> + if (old_vproc > new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>
> I don't quite get the logic for tol calculation here. What's the
> expected value for volt_tol? Care to explain?
>
Here I am trying to handle the problem: the voltage value can be set to
the regulator might be different from the voltage value got from OPP table.
And I am realizing that it might not be a good way to solve the problem. I
will turn to use regulator API to query the supported voltage values to get
rid of voltage tolerance calculation.
>> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
>> + tol);
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> + }
>> + } else if (old_vproc < new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> +
>> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
>> + cur_vsram = regulator_get_voltage(
>> + dvfs->sram_reg);
>> + target_volt = cur_vsram - 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + target_volt, tol);
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
>> +{
>> + struct dev_pm_opp *opp;
>> + int volt;
>> +
>> + rcu_read_lock();
>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
>> + return PTR_ERR(opp);
>> + }
>> + volt = dev_pm_opp_get_voltage(opp);
>> + rcu_read_unlock();
>> +
>> + return volt;
>> +}
>> +
>> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> +
>> + if (dvfs->inter_volt == 0)
>> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
>> + mainpll_freq);
>> +
>> + return dvfs->inter_volt;
>> +}
>> +
>> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> + int ret = 0;
>> +
>> + if (cluster == LITTLE_CLUSTER) {
>> + int tol;
>> +
>> + tol = new_volt * dvfs->volt_tol / 100;
>> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
>> + } else {
>> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
>> + }
>> +
>> + if (ret)
>> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
>> + __func__, cluster, old_volt, new_volt);
>> +}
>
> It seems the only reason to check if it is a BIG or LITTLE cluster is to
> control vsram correctly. If we assume we need to do the control whenever
> sram_reg exists, we can drop all the BIG/LITTLE cluster check. I think
> this will make code looks more compact and generic.
>
Yes. will fix it.
>> +
>> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct cpufreq_freqs *freqs = data;
>> + struct cluster_dvfs_info *dvfs;
>> + unsigned long old_volt, new_volt, inter_volt;
>> + int cluster;
>> +
>> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
>> + cluster = BIG_CLUSTER;
>> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
>> + cluster = LITTLE_CLUSTER;
>> + else
>> + return NOTIFY_DONE;
>> +
>> + dvfs = &dvfs_info[cluster];
>> +
>> + old_volt = regulator_get_voltage(dvfs->proc_reg);
>> +
>> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
>> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
>
> This is only used in PRECHANGE, please move this inside the if.
>
Yes. will fix it.
>> +
>> + if (action == CPUFREQ_PRECHANGE) {
>> + if (old_volt < inter_volt || old_volt < new_volt) {
>> + new_volt = inter_volt > new_volt ?
>> + inter_volt : new_volt;
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
>> + } else if (action == CPUFREQ_POSTCHANGE) {
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
>> +
>> + if (new_volt < old_volt)
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block mt8173_cpufreq_nb = {
>> + .notifier_call = mt8173_cpufreq_notify,
>> +};
>> +
>> +static struct cpufreq_dt_platform_data *pd;
>
> Please drop this global variable. You can return it from
> cpufreq_dt_pdata_init and pass to all the function need it.
>
Yes. will fix it.
>> +
>> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
>> +{
>> + struct list_head *node;
>> +
>> + list_for_each(node, domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + domain = container_of(node, struct cpufreq_cpu_domain, node);
>> + if (cpumask_test_cpu(cpu, &domain->cpus))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_dt_pdata_release(void)
>> +{
>> + if (!pd)
>> + return;
>> +
>> + if (!list_empty(&pd->domain_list)) {
>> + struct list_head *node, *tmp;
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + list_for_each_safe(node, tmp, &pd->domain_list) {
>> + list_del(node);
>> + domain = container_of(node, struct cpufreq_cpu_domain,
>> + node);
>> + kfree(domain);
>> + }
>> + }
>> +
>> + kfree(pd);
>> +}
>> +
>> +static int cpufreq_dt_pdata_init(void)
>> +{
>> + int cpu;
>> +
>> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + pd->independent_clocks = 1,
>> + INIT_LIST_HEAD(&pd->domain_list);
>> +
>> + for_each_possible_cpu(cpu) {
>> + struct cpufreq_cpu_domain *new_domain;
>> +
>> + if (cpu_in_domain_list(&pd->domain_list, cpu))
>> + continue;
>> +
>> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
>> + if (!new_domain) {
>> + cpufreq_dt_pdata_release();
>> + return -ENOMEM;
>> + }
>> +
>> + cpumask_copy(&new_domain->cpus,
>> + &cpu_topology[cpu].core_sibling);
>> + list_add(&new_domain->node, &pd->domain_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void mt8173_cpufreq_dvfs_info_release(void)
>> +{
>> + int i;
>> +
>> + if (!dvfs_info)
>> + return;
>> +
>> + for (i = 0; i < NR_CLUSTERS; ++i) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
>> + regulator_put(dvfs->proc_reg);
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
>> + regulator_put(dvfs->sram_reg);
>> + }
>> +
>> + if (clk_mux_regs)
>> + iounmap(clk_mux_regs);
>> +}
>> +
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>> + struct list_head *node;
>> + struct device_node *np;
>> + struct clk *mainpll;
>> + int i, ret;
>> +
>> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
>> + if (!dvfs_info)
>> + return -ENOMEM;
>> +
>> + list_for_each(node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain = container_of(
>> + node, struct cpufreq_cpu_domain, node);
>> + int cpu = cpumask_next(0, &domain->cpus);
>> +
>> + np = of_get_cpu_node(cpu, NULL);
>> +
>> + if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a53") >= 0)
>> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
>> + &domain->cpus);
>> + else if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a57") >= 0)
>> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
>> + &domain->cpus);
>> + else {
>> + of_node_put(np);
>> + pr_err("%s: unknown CPU core\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(np);
>> + }
>
> It seems you hardcode the match CPU here to know which is big/little
> cluster. If we still need this information, should we add this to
> topology, maybe something similar to capacity in arm topology, instead
> of coding here?
>
I am not sure if there's such information I could use here. I will check it
further. Probably I don't even need this information if I just use the
presence of dvfs->sram_reg to identify the cluster.
>> +
>> + for (i = 0; i < NR_CLUSTERS; i++) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> + int cpu;
>> +
>> + for_each_cpu_mask(cpu, dvfs->cpus) {
>> + struct device_node *np;
>> +
>> + dvfs->cpu_dev = get_cpu_device(cpu);
>> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
>> + if (IS_ERR(dvfs->proc_reg))
>> + continue;
>> +
>> + dvfs->cpu = cpu;
>> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
>> +
>> + np = of_node_get(dvfs->cpu_dev->of_node);
>> + of_property_read_u32(np, "voltage-tolerance",
>> + &dvfs->volt_tol);
>> + of_node_put(np);
>> + break;
>> + }
>> +
>> + if (IS_ERR(dvfs->proc_reg)) {
>> + pr_err("%s: no proc regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
>> + pr_err("%s: no sram regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> + }
>> +
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>> +
>> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
>> + if (!np) {
>> + pr_err("failed to find clock mux registers\n");
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + clk_mux_regs = of_iomap(np, 0);
>> + if (!clk_mux_regs) {
>> + pr_err("failed to map clock mux registers\n");
>> + ret = -EFAULT;
>> + goto dvfs_info_release;
>> + }
>> + of_node_put(np);
>
> This is already used by mt8173 clock driver, and you are directly
> accessing it registers to control clock mux. I think you should add
> these 2 clk to clock driver and use clk API to control it. Also you can
> drop the lock init below if you do this.
>
> Joe.C
>
Yes. I think I need to have some discussion about this with the guy
upstreaming CCF support for MT8173.
>> +
>> + spin_lock_init(&lock);
>> +
>> + return 0;
>> +
>> +dvfs_info_release:
>> + mt8173_cpufreq_dvfs_info_release();
>> + return ret;
>> +}
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
2015-01-09 9:54 ` pi-cheng.chen
(?)
@ 2015-01-19 8:00 ` Viresh Kumar
-1 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 8:00 UTC (permalink / raw)
To: pi-cheng.chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> Currently the DT based cpufreq driver is missing some way to check which
> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
> check which CPUs share clock/power.
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
> include/linux/cpufreq-dt.h | 6 ++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index fde97d6..ff8c266 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> pd = cpufreq_get_driver_data();
> if (!pd || !pd->independent_clocks)
> cpumask_setall(policy->cpus);
> + else if (pd && !list_empty(&pd->domain_list)) {
> + struct list_head *domain_node;
> +
> + list_for_each(domain_node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain;
Define this with domain_node.
> +
> + domain = container_of(domain_node,
> + struct cpufreq_cpu_domain, node);
> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
> + continue;
> +
> + cpumask_copy(policy->cpus, &domain->cpus);
> + break;
> + }
> + }
>
> of_node_put(np);
>
> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
> index 0414009..92bffd3 100644
> --- a/include/linux/cpufreq-dt.h
> +++ b/include/linux/cpufreq-dt.h
> @@ -10,6 +10,11 @@
> #ifndef __CPUFREQ_DT_H__
> #define __CPUFREQ_DT_H__
>
> +struct cpufreq_cpu_domain {
> + struct list_head node;
> + cpumask_t cpus;
> +};
> +
> struct cpufreq_dt_platform_data {
> /*
> * True when each CPU has its own clock to control its
> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
> * clock.
> */
> bool independent_clocks;
> + struct list_head domain_list;
> };
Though we need to keep only one of these two, but I don't think
any of these will stay for long time. So, its okay..
Looks fine. Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-19 8:00 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 8:00 UTC (permalink / raw)
To: pi-cheng.chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> Currently the DT based cpufreq driver is missing some way to check which
> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
> check which CPUs share clock/power.
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
> include/linux/cpufreq-dt.h | 6 ++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index fde97d6..ff8c266 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> pd = cpufreq_get_driver_data();
> if (!pd || !pd->independent_clocks)
> cpumask_setall(policy->cpus);
> + else if (pd && !list_empty(&pd->domain_list)) {
> + struct list_head *domain_node;
> +
> + list_for_each(domain_node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain;
Define this with domain_node.
> +
> + domain = container_of(domain_node,
> + struct cpufreq_cpu_domain, node);
> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
> + continue;
> +
> + cpumask_copy(policy->cpus, &domain->cpus);
> + break;
> + }
> + }
>
> of_node_put(np);
>
> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
> index 0414009..92bffd3 100644
> --- a/include/linux/cpufreq-dt.h
> +++ b/include/linux/cpufreq-dt.h
> @@ -10,6 +10,11 @@
> #ifndef __CPUFREQ_DT_H__
> #define __CPUFREQ_DT_H__
>
> +struct cpufreq_cpu_domain {
> + struct list_head node;
> + cpumask_t cpus;
> +};
> +
> struct cpufreq_dt_platform_data {
> /*
> * True when each CPU has its own clock to control its
> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
> * clock.
> */
> bool independent_clocks;
> + struct list_head domain_list;
> };
Though we need to keep only one of these two, but I don't think
any of these will stay for long time. So, its okay..
Looks fine. Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-19 8:00 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 8:00 UTC (permalink / raw)
To: linux-arm-kernel
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> Currently the DT based cpufreq driver is missing some way to check which
> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
> check which CPUs share clock/power.
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
> include/linux/cpufreq-dt.h | 6 ++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index fde97d6..ff8c266 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> pd = cpufreq_get_driver_data();
> if (!pd || !pd->independent_clocks)
> cpumask_setall(policy->cpus);
> + else if (pd && !list_empty(&pd->domain_list)) {
> + struct list_head *domain_node;
> +
> + list_for_each(domain_node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain;
Define this with domain_node.
> +
> + domain = container_of(domain_node,
> + struct cpufreq_cpu_domain, node);
> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
> + continue;
> +
> + cpumask_copy(policy->cpus, &domain->cpus);
> + break;
> + }
> + }
>
> of_node_put(np);
>
> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
> index 0414009..92bffd3 100644
> --- a/include/linux/cpufreq-dt.h
> +++ b/include/linux/cpufreq-dt.h
> @@ -10,6 +10,11 @@
> #ifndef __CPUFREQ_DT_H__
> #define __CPUFREQ_DT_H__
>
> +struct cpufreq_cpu_domain {
> + struct list_head node;
> + cpumask_t cpus;
> +};
> +
> struct cpufreq_dt_platform_data {
> /*
> * True when each CPU has its own clock to control its
> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
> * clock.
> */
> bool independent_clocks;
> + struct list_head domain_list;
> };
Though we need to keep only one of these two, but I don't think
any of these will stay for long time. So, its okay..
Looks fine. Thanks.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` pi-cheng.chen
(?)
@ 2015-01-19 10:42 ` Viresh Kumar
-1 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 10:42 UTC (permalink / raw)
To: pi-cheng.chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
> SoC works properly:
> 1. When doing frequency scaling of CPUs, the clock source should be switched
> to another PLL, then switched back to the orignal until it's stable.
> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
> controlled concurrently and 2 limitations should be followed:
> a. Vsram > Vproc
> b. Vsram - Vproc < 200 mV
>
> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
> cpufreq notifier.
I haven't understood how is all this working..
- How are you switching to intermediate freq when its support isn't there
in cpufreq-dt.
- How is changing of volt after switching to freq working? Specially when freq
is higher than older freq and it requires changing of voltage before freq..
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 6 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0f9a2c3..97ed7dd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_MT8173_CPUFREQ
> + bool "Mediatek MT8173 CPUFreq support"
> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
> + help
> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b3ca7b0..67b7f17 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
Add this in alphanumeric order please.
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
> +/*
> +* Copyright (c) 2015 Linaro.
> +* 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
This driver isn't a clock provider, right? Then why are
you including this file?
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/pm_opp.h>
keep them in alphanumeric order please.
> +
> +#define CPUCLK_MUX_ARMPLL 0x1
> +#define CPUCLK_MUX_MAINPLL 0x2
> +#define VOLT_SHIFT_LIMIT 150000
> +
> +enum {
> + LITTLE_CLUSTER = 0,
> + BIG_CLUSTER,
> + NR_CLUSTERS
> +};
> +
> +static struct cluster_dvfs_info {
> + int cpu;
> + struct cpumask cpus;
> + struct device *cpu_dev;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + int inter_volt;
> + u32 volt_tol;
> +} *dvfs_info;
> +
> +static unsigned long mainpll_freq;
> +static void __iomem *clk_mux_regs;
> +static spinlock_t lock;
> +
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
> +
> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
> + unsigned long old_vproc,
> + unsigned long new_vproc)
> +{
> + int cur_vsram, cur_vproc, target_volt, tol;
> +
> + if (old_vproc > new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
> + tol);
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> + }
> + } else if (old_vproc < new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> +
> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
> + cur_vsram = regulator_get_voltage(
> + dvfs->sram_reg);
> + target_volt = cur_vsram - 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + target_volt, tol);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
> +{
> + struct dev_pm_opp *opp;
> + int volt;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + return volt;
> +}
> +
> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> +
> + if (dvfs->inter_volt == 0)
> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
> + mainpll_freq);
> +
> + return dvfs->inter_volt;
> +}
> +
> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> + int ret = 0;
> +
> + if (cluster == LITTLE_CLUSTER) {
> + int tol;
> +
> + tol = new_volt * dvfs->volt_tol / 100;
> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
> + } else {
> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
> + }
> +
> + if (ret)
> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
> + __func__, cluster, old_volt, new_volt);
> +}
> +
> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct cpufreq_freqs *freqs = data;
> + struct cluster_dvfs_info *dvfs;
> + unsigned long old_volt, new_volt, inter_volt;
> + int cluster;
> +
> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
You should check it against cpumask. freqs->cpu can change on the event
of hotplug of CPUs.
> + cluster = BIG_CLUSTER;
> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
> + cluster = LITTLE_CLUSTER;
> + else
> + return NOTIFY_DONE;
> +
> + dvfs = &dvfs_info[cluster];
> +
> + old_volt = regulator_get_voltage(dvfs->proc_reg);
> +
> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
> +
> + if (action == CPUFREQ_PRECHANGE) {
> + if (old_volt < inter_volt || old_volt < new_volt) {
> + new_volt = inter_volt > new_volt ?
> + inter_volt : new_volt;
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
> + } else if (action == CPUFREQ_POSTCHANGE) {
> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
> +
> + if (new_volt < old_volt)
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mt8173_cpufreq_nb = {
> + .notifier_call = mt8173_cpufreq_notify,
> +};
> +
> +static struct cpufreq_dt_platform_data *pd;
> +
> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
> +{
> + struct list_head *node;
> +
> + list_for_each(node, domain_list) {
> + struct cpufreq_cpu_domain *domain;
> +
> + domain = container_of(node, struct cpufreq_cpu_domain, node);
> + if (cpumask_test_cpu(cpu, &domain->cpus))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_dt_pdata_release(void)
> +{
> + if (!pd)
> + return;
> +
> + if (!list_empty(&pd->domain_list)) {
> + struct list_head *node, *tmp;
> + struct cpufreq_cpu_domain *domain;
> +
> + list_for_each_safe(node, tmp, &pd->domain_list) {
> + list_del(node);
> + domain = container_of(node, struct cpufreq_cpu_domain,
> + node);
> + kfree(domain);
> + }
> + }
> +
> + kfree(pd);
> +}
> +
> +static int cpufreq_dt_pdata_init(void)
> +{
> + int cpu;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + pd->independent_clocks = 1,
> + INIT_LIST_HEAD(&pd->domain_list);
> +
> + for_each_possible_cpu(cpu) {
> + struct cpufreq_cpu_domain *new_domain;
> +
> + if (cpu_in_domain_list(&pd->domain_list, cpu))
> + continue;
> +
> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
> + if (!new_domain) {
> + cpufreq_dt_pdata_release();
> + return -ENOMEM;
> + }
> +
> + cpumask_copy(&new_domain->cpus,
> + &cpu_topology[cpu].core_sibling);
> + list_add(&new_domain->node, &pd->domain_list);
> + }
> +
> + return 0;
> +}
> +
> +static void mt8173_cpufreq_dvfs_info_release(void)
> +{
> + int i;
> +
> + if (!dvfs_info)
> + return;
> +
> + for (i = 0; i < NR_CLUSTERS; ++i) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> +
> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
> + regulator_put(dvfs->proc_reg);
> +
> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
> + regulator_put(dvfs->sram_reg);
> + }
> +
> + if (clk_mux_regs)
> + iounmap(clk_mux_regs);
> +}
> +
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
> + struct list_head *node;
> + struct device_node *np;
> + struct clk *mainpll;
> + int i, ret;
> +
> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
> + if (!dvfs_info)
> + return -ENOMEM;
> +
> + list_for_each(node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain = container_of(
> + node, struct cpufreq_cpu_domain, node);
> + int cpu = cpumask_next(0, &domain->cpus);
cpumask_first() ?
> +
> + np = of_get_cpu_node(cpu, NULL);
This can fail..
> +
> + if (of_property_match_string(np, "compatible",
> + "arm,cortex-a53") >= 0)
> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
> + &domain->cpus);
> + else if (of_property_match_string(np, "compatible",
> + "arm,cortex-a57") >= 0)
> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
> + &domain->cpus);
> + else {
> + of_node_put(np);
> + pr_err("%s: unknown CPU core\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> + }
> +
> + for (i = 0; i < NR_CLUSTERS; i++) {
You can merge this for-loop with the above for-loop..
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> + int cpu;
> +
> + for_each_cpu_mask(cpu, dvfs->cpus) {
> + struct device_node *np;
> +
> + dvfs->cpu_dev = get_cpu_device(cpu);
> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
> + if (IS_ERR(dvfs->proc_reg))
> + continue;
> +
> + dvfs->cpu = cpu;
> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
> +
> + np = of_node_get(dvfs->cpu_dev->of_node);
> + of_property_read_u32(np, "voltage-tolerance",
> + &dvfs->volt_tol);
> + of_node_put(np);
> + break;
> + }
> +
> + if (IS_ERR(dvfs->proc_reg)) {
> + pr_err("%s: no proc regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
> + pr_err("%s: no sram regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> + }
> +
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
> +
> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
> + if (!np) {
> + pr_err("failed to find clock mux registers\n");
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + clk_mux_regs = of_iomap(np, 0);
> + if (!clk_mux_regs) {
> + pr_err("failed to map clock mux registers\n");
> + ret = -EFAULT;
> + goto dvfs_info_release;
> + }
> + of_node_put(np);
> +
> + spin_lock_init(&lock);
> +
> + return 0;
> +
> +dvfs_info_release:
> + mt8173_cpufreq_dvfs_info_release();
> + return ret;
> +}
> +
> +static int mt8173_cpufreq_driver_init(void)
> +{
> + int ret;
> +
> + if (!of_machine_is_compatible("mediatek,mt8173"))
> + return -ENODEV;
> +
> + ret = cpufreq_dt_pdata_init();
> + if (ret) {
> + pr_err("failed to initialize platform data: %d\n", ret);
> + return ret;
> + }
> +
> + ret = mt8173_cpufreq_dvfs_info_init();
> + if (ret) {
> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
> + goto dt_pdata_release;
> + }
> +
> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + pr_err("failed to register cpufreq notifier: %d\n", ret);
> + goto dt_pdata_release;
dvfs_info_release here ?
> + }
> +
> + platform_device_register_data(NULL, "cpufreq-dt", -1,
> + pd, sizeof(*pd));
can't this fail?
> +
> + return 0;
> +
> +dt_pdata_release:
> + cpufreq_dt_pdata_release();
> + return ret;
> +}
> +
> +module_init(mt8173_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-19 10:42 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 10:42 UTC (permalink / raw)
To: pi-cheng.chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
> SoC works properly:
> 1. When doing frequency scaling of CPUs, the clock source should be switched
> to another PLL, then switched back to the orignal until it's stable.
> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
> controlled concurrently and 2 limitations should be followed:
> a. Vsram > Vproc
> b. Vsram - Vproc < 200 mV
>
> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
> cpufreq notifier.
I haven't understood how is all this working..
- How are you switching to intermediate freq when its support isn't there
in cpufreq-dt.
- How is changing of volt after switching to freq working? Specially when freq
is higher than older freq and it requires changing of voltage before freq..
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 6 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0f9a2c3..97ed7dd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_MT8173_CPUFREQ
> + bool "Mediatek MT8173 CPUFreq support"
> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
> + help
> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b3ca7b0..67b7f17 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
Add this in alphanumeric order please.
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
> +/*
> +* Copyright (c) 2015 Linaro.
> +* 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
This driver isn't a clock provider, right? Then why are
you including this file?
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/pm_opp.h>
keep them in alphanumeric order please.
> +
> +#define CPUCLK_MUX_ARMPLL 0x1
> +#define CPUCLK_MUX_MAINPLL 0x2
> +#define VOLT_SHIFT_LIMIT 150000
> +
> +enum {
> + LITTLE_CLUSTER = 0,
> + BIG_CLUSTER,
> + NR_CLUSTERS
> +};
> +
> +static struct cluster_dvfs_info {
> + int cpu;
> + struct cpumask cpus;
> + struct device *cpu_dev;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + int inter_volt;
> + u32 volt_tol;
> +} *dvfs_info;
> +
> +static unsigned long mainpll_freq;
> +static void __iomem *clk_mux_regs;
> +static spinlock_t lock;
> +
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
> +
> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
> + unsigned long old_vproc,
> + unsigned long new_vproc)
> +{
> + int cur_vsram, cur_vproc, target_volt, tol;
> +
> + if (old_vproc > new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
> + tol);
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> + }
> + } else if (old_vproc < new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> +
> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
> + cur_vsram = regulator_get_voltage(
> + dvfs->sram_reg);
> + target_volt = cur_vsram - 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + target_volt, tol);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
> +{
> + struct dev_pm_opp *opp;
> + int volt;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + return volt;
> +}
> +
> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> +
> + if (dvfs->inter_volt == 0)
> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
> + mainpll_freq);
> +
> + return dvfs->inter_volt;
> +}
> +
> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> + int ret = 0;
> +
> + if (cluster == LITTLE_CLUSTER) {
> + int tol;
> +
> + tol = new_volt * dvfs->volt_tol / 100;
> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
> + } else {
> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
> + }
> +
> + if (ret)
> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
> + __func__, cluster, old_volt, new_volt);
> +}
> +
> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct cpufreq_freqs *freqs = data;
> + struct cluster_dvfs_info *dvfs;
> + unsigned long old_volt, new_volt, inter_volt;
> + int cluster;
> +
> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
You should check it against cpumask. freqs->cpu can change on the event
of hotplug of CPUs.
> + cluster = BIG_CLUSTER;
> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
> + cluster = LITTLE_CLUSTER;
> + else
> + return NOTIFY_DONE;
> +
> + dvfs = &dvfs_info[cluster];
> +
> + old_volt = regulator_get_voltage(dvfs->proc_reg);
> +
> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
> +
> + if (action == CPUFREQ_PRECHANGE) {
> + if (old_volt < inter_volt || old_volt < new_volt) {
> + new_volt = inter_volt > new_volt ?
> + inter_volt : new_volt;
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
> + } else if (action == CPUFREQ_POSTCHANGE) {
> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
> +
> + if (new_volt < old_volt)
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mt8173_cpufreq_nb = {
> + .notifier_call = mt8173_cpufreq_notify,
> +};
> +
> +static struct cpufreq_dt_platform_data *pd;
> +
> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
> +{
> + struct list_head *node;
> +
> + list_for_each(node, domain_list) {
> + struct cpufreq_cpu_domain *domain;
> +
> + domain = container_of(node, struct cpufreq_cpu_domain, node);
> + if (cpumask_test_cpu(cpu, &domain->cpus))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_dt_pdata_release(void)
> +{
> + if (!pd)
> + return;
> +
> + if (!list_empty(&pd->domain_list)) {
> + struct list_head *node, *tmp;
> + struct cpufreq_cpu_domain *domain;
> +
> + list_for_each_safe(node, tmp, &pd->domain_list) {
> + list_del(node);
> + domain = container_of(node, struct cpufreq_cpu_domain,
> + node);
> + kfree(domain);
> + }
> + }
> +
> + kfree(pd);
> +}
> +
> +static int cpufreq_dt_pdata_init(void)
> +{
> + int cpu;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + pd->independent_clocks = 1,
> + INIT_LIST_HEAD(&pd->domain_list);
> +
> + for_each_possible_cpu(cpu) {
> + struct cpufreq_cpu_domain *new_domain;
> +
> + if (cpu_in_domain_list(&pd->domain_list, cpu))
> + continue;
> +
> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
> + if (!new_domain) {
> + cpufreq_dt_pdata_release();
> + return -ENOMEM;
> + }
> +
> + cpumask_copy(&new_domain->cpus,
> + &cpu_topology[cpu].core_sibling);
> + list_add(&new_domain->node, &pd->domain_list);
> + }
> +
> + return 0;
> +}
> +
> +static void mt8173_cpufreq_dvfs_info_release(void)
> +{
> + int i;
> +
> + if (!dvfs_info)
> + return;
> +
> + for (i = 0; i < NR_CLUSTERS; ++i) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> +
> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
> + regulator_put(dvfs->proc_reg);
> +
> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
> + regulator_put(dvfs->sram_reg);
> + }
> +
> + if (clk_mux_regs)
> + iounmap(clk_mux_regs);
> +}
> +
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
> + struct list_head *node;
> + struct device_node *np;
> + struct clk *mainpll;
> + int i, ret;
> +
> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
> + if (!dvfs_info)
> + return -ENOMEM;
> +
> + list_for_each(node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain = container_of(
> + node, struct cpufreq_cpu_domain, node);
> + int cpu = cpumask_next(0, &domain->cpus);
cpumask_first() ?
> +
> + np = of_get_cpu_node(cpu, NULL);
This can fail..
> +
> + if (of_property_match_string(np, "compatible",
> + "arm,cortex-a53") >= 0)
> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
> + &domain->cpus);
> + else if (of_property_match_string(np, "compatible",
> + "arm,cortex-a57") >= 0)
> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
> + &domain->cpus);
> + else {
> + of_node_put(np);
> + pr_err("%s: unknown CPU core\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> + }
> +
> + for (i = 0; i < NR_CLUSTERS; i++) {
You can merge this for-loop with the above for-loop..
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> + int cpu;
> +
> + for_each_cpu_mask(cpu, dvfs->cpus) {
> + struct device_node *np;
> +
> + dvfs->cpu_dev = get_cpu_device(cpu);
> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
> + if (IS_ERR(dvfs->proc_reg))
> + continue;
> +
> + dvfs->cpu = cpu;
> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
> +
> + np = of_node_get(dvfs->cpu_dev->of_node);
> + of_property_read_u32(np, "voltage-tolerance",
> + &dvfs->volt_tol);
> + of_node_put(np);
> + break;
> + }
> +
> + if (IS_ERR(dvfs->proc_reg)) {
> + pr_err("%s: no proc regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
> + pr_err("%s: no sram regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> + }
> +
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
> +
> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
> + if (!np) {
> + pr_err("failed to find clock mux registers\n");
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + clk_mux_regs = of_iomap(np, 0);
> + if (!clk_mux_regs) {
> + pr_err("failed to map clock mux registers\n");
> + ret = -EFAULT;
> + goto dvfs_info_release;
> + }
> + of_node_put(np);
> +
> + spin_lock_init(&lock);
> +
> + return 0;
> +
> +dvfs_info_release:
> + mt8173_cpufreq_dvfs_info_release();
> + return ret;
> +}
> +
> +static int mt8173_cpufreq_driver_init(void)
> +{
> + int ret;
> +
> + if (!of_machine_is_compatible("mediatek,mt8173"))
> + return -ENODEV;
> +
> + ret = cpufreq_dt_pdata_init();
> + if (ret) {
> + pr_err("failed to initialize platform data: %d\n", ret);
> + return ret;
> + }
> +
> + ret = mt8173_cpufreq_dvfs_info_init();
> + if (ret) {
> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
> + goto dt_pdata_release;
> + }
> +
> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + pr_err("failed to register cpufreq notifier: %d\n", ret);
> + goto dt_pdata_release;
dvfs_info_release here ?
> + }
> +
> + platform_device_register_data(NULL, "cpufreq-dt", -1,
> + pd, sizeof(*pd));
can't this fail?
> +
> + return 0;
> +
> +dt_pdata_release:
> + cpufreq_dt_pdata_release();
> + return ret;
> +}
> +
> +module_init(mt8173_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-19 10:42 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-19 10:42 UTC (permalink / raw)
To: linux-arm-kernel
On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
> SoC works properly:
> 1. When doing frequency scaling of CPUs, the clock source should be switched
> to another PLL, then switched back to the orignal until it's stable.
> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
> controlled concurrently and 2 limitations should be followed:
> a. Vsram > Vproc
> b. Vsram - Vproc < 200 mV
>
> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
> cpufreq notifier.
I haven't understood how is all this working..
- How are you switching to intermediate freq when its support isn't there
in cpufreq-dt.
- How is changing of volt after switching to freq working? Specially when freq
is higher than older freq and it requires changing of voltage before freq..
>
> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 6 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+)
> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0f9a2c3..97ed7dd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>
> If in doubt, say N.
> +
> +config ARM_MT8173_CPUFREQ
> + bool "Mediatek MT8173 CPUFreq support"
> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
> + help
> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index b3ca7b0..67b7f17 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
Add this in alphanumeric order please.
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
> +/*
> +* Copyright (c) 2015 Linaro.
> +* 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/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
This driver isn't a clock provider, right? Then why are
you including this file?
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu.h>
> +#include <linux/pm_opp.h>
keep them in alphanumeric order please.
> +
> +#define CPUCLK_MUX_ARMPLL 0x1
> +#define CPUCLK_MUX_MAINPLL 0x2
> +#define VOLT_SHIFT_LIMIT 150000
> +
> +enum {
> + LITTLE_CLUSTER = 0,
> + BIG_CLUSTER,
> + NR_CLUSTERS
> +};
> +
> +static struct cluster_dvfs_info {
> + int cpu;
> + struct cpumask cpus;
> + struct device *cpu_dev;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + int inter_volt;
> + u32 volt_tol;
> +} *dvfs_info;
> +
> +static unsigned long mainpll_freq;
> +static void __iomem *clk_mux_regs;
> +static spinlock_t lock;
> +
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
> +
> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
> + unsigned long old_vproc,
> + unsigned long new_vproc)
> +{
> + int cur_vsram, cur_vproc, target_volt, tol;
> +
> + if (old_vproc > new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
> + tol);
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> + }
> + } else if (old_vproc < new_vproc) {
> + while (1) {
> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
> + cur_vsram > new_vproc) {
> + tol = new_vproc * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + new_vproc, tol);
> + break;
> + }
> +
> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
> + tol);
> +
> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
> + cur_vsram = regulator_get_voltage(
> + dvfs->sram_reg);
> + target_volt = cur_vsram - 1;
> + tol = target_volt * dvfs->volt_tol / 100;
> + regulator_set_voltage_tol(dvfs->proc_reg,
> + target_volt, tol);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
> +{
> + struct dev_pm_opp *opp;
> + int volt;
> +
> + rcu_read_lock();
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> + if (IS_ERR(opp)) {
> + rcu_read_unlock();
> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
> + return PTR_ERR(opp);
> + }
> + volt = dev_pm_opp_get_voltage(opp);
> + rcu_read_unlock();
> +
> + return volt;
> +}
> +
> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> +
> + if (dvfs->inter_volt == 0)
> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
> + mainpll_freq);
> +
> + return dvfs->inter_volt;
> +}
> +
> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
> +{
> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
> + int ret = 0;
> +
> + if (cluster == LITTLE_CLUSTER) {
> + int tol;
> +
> + tol = new_volt * dvfs->volt_tol / 100;
> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
> + } else {
> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
> + }
> +
> + if (ret)
> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
> + __func__, cluster, old_volt, new_volt);
> +}
> +
> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct cpufreq_freqs *freqs = data;
> + struct cluster_dvfs_info *dvfs;
> + unsigned long old_volt, new_volt, inter_volt;
> + int cluster;
> +
> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
You should check it against cpumask. freqs->cpu can change on the event
of hotplug of CPUs.
> + cluster = BIG_CLUSTER;
> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
> + cluster = LITTLE_CLUSTER;
> + else
> + return NOTIFY_DONE;
> +
> + dvfs = &dvfs_info[cluster];
> +
> + old_volt = regulator_get_voltage(dvfs->proc_reg);
> +
> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
> +
> + if (action == CPUFREQ_PRECHANGE) {
> + if (old_volt < inter_volt || old_volt < new_volt) {
> + new_volt = inter_volt > new_volt ?
> + inter_volt : new_volt;
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
> + } else if (action == CPUFREQ_POSTCHANGE) {
> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
> +
> + if (new_volt < old_volt)
> + mt8173_cpufreq_set_voltage(cluster, old_volt,
> + new_volt);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mt8173_cpufreq_nb = {
> + .notifier_call = mt8173_cpufreq_notify,
> +};
> +
> +static struct cpufreq_dt_platform_data *pd;
> +
> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
> +{
> + struct list_head *node;
> +
> + list_for_each(node, domain_list) {
> + struct cpufreq_cpu_domain *domain;
> +
> + domain = container_of(node, struct cpufreq_cpu_domain, node);
> + if (cpumask_test_cpu(cpu, &domain->cpus))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void cpufreq_dt_pdata_release(void)
> +{
> + if (!pd)
> + return;
> +
> + if (!list_empty(&pd->domain_list)) {
> + struct list_head *node, *tmp;
> + struct cpufreq_cpu_domain *domain;
> +
> + list_for_each_safe(node, tmp, &pd->domain_list) {
> + list_del(node);
> + domain = container_of(node, struct cpufreq_cpu_domain,
> + node);
> + kfree(domain);
> + }
> + }
> +
> + kfree(pd);
> +}
> +
> +static int cpufreq_dt_pdata_init(void)
> +{
> + int cpu;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return -ENOMEM;
> +
> + pd->independent_clocks = 1,
> + INIT_LIST_HEAD(&pd->domain_list);
> +
> + for_each_possible_cpu(cpu) {
> + struct cpufreq_cpu_domain *new_domain;
> +
> + if (cpu_in_domain_list(&pd->domain_list, cpu))
> + continue;
> +
> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
> + if (!new_domain) {
> + cpufreq_dt_pdata_release();
> + return -ENOMEM;
> + }
> +
> + cpumask_copy(&new_domain->cpus,
> + &cpu_topology[cpu].core_sibling);
> + list_add(&new_domain->node, &pd->domain_list);
> + }
> +
> + return 0;
> +}
> +
> +static void mt8173_cpufreq_dvfs_info_release(void)
> +{
> + int i;
> +
> + if (!dvfs_info)
> + return;
> +
> + for (i = 0; i < NR_CLUSTERS; ++i) {
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> +
> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
> + regulator_put(dvfs->proc_reg);
> +
> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
> + regulator_put(dvfs->sram_reg);
> + }
> +
> + if (clk_mux_regs)
> + iounmap(clk_mux_regs);
> +}
> +
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
> + struct list_head *node;
> + struct device_node *np;
> + struct clk *mainpll;
> + int i, ret;
> +
> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
> + if (!dvfs_info)
> + return -ENOMEM;
> +
> + list_for_each(node, &pd->domain_list) {
> + struct cpufreq_cpu_domain *domain = container_of(
> + node, struct cpufreq_cpu_domain, node);
> + int cpu = cpumask_next(0, &domain->cpus);
cpumask_first() ?
> +
> + np = of_get_cpu_node(cpu, NULL);
This can fail..
> +
> + if (of_property_match_string(np, "compatible",
> + "arm,cortex-a53") >= 0)
> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
> + &domain->cpus);
> + else if (of_property_match_string(np, "compatible",
> + "arm,cortex-a57") >= 0)
> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
> + &domain->cpus);
> + else {
> + of_node_put(np);
> + pr_err("%s: unknown CPU core\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_node_put(np);
> + }
> +
> + for (i = 0; i < NR_CLUSTERS; i++) {
You can merge this for-loop with the above for-loop..
> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
> + int cpu;
> +
> + for_each_cpu_mask(cpu, dvfs->cpus) {
> + struct device_node *np;
> +
> + dvfs->cpu_dev = get_cpu_device(cpu);
> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
> + if (IS_ERR(dvfs->proc_reg))
> + continue;
> +
> + dvfs->cpu = cpu;
> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
> +
> + np = of_node_get(dvfs->cpu_dev->of_node);
> + of_property_read_u32(np, "voltage-tolerance",
> + &dvfs->volt_tol);
> + of_node_put(np);
> + break;
> + }
> +
> + if (IS_ERR(dvfs->proc_reg)) {
> + pr_err("%s: no proc regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
> + pr_err("%s: no sram regulator for cluster %d\n",
> + __func__, i);
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> + }
> +
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
> +
> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
> + if (!np) {
> + pr_err("failed to find clock mux registers\n");
> + ret = -ENODEV;
> + goto dvfs_info_release;
> + }
> +
> + clk_mux_regs = of_iomap(np, 0);
> + if (!clk_mux_regs) {
> + pr_err("failed to map clock mux registers\n");
> + ret = -EFAULT;
> + goto dvfs_info_release;
> + }
> + of_node_put(np);
> +
> + spin_lock_init(&lock);
> +
> + return 0;
> +
> +dvfs_info_release:
> + mt8173_cpufreq_dvfs_info_release();
> + return ret;
> +}
> +
> +static int mt8173_cpufreq_driver_init(void)
> +{
> + int ret;
> +
> + if (!of_machine_is_compatible("mediatek,mt8173"))
> + return -ENODEV;
> +
> + ret = cpufreq_dt_pdata_init();
> + if (ret) {
> + pr_err("failed to initialize platform data: %d\n", ret);
> + return ret;
> + }
> +
> + ret = mt8173_cpufreq_dvfs_info_init();
> + if (ret) {
> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
> + goto dt_pdata_release;
> + }
> +
> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + pr_err("failed to register cpufreq notifier: %d\n", ret);
> + goto dt_pdata_release;
dvfs_info_release here ?
> + }
> +
> + platform_device_register_data(NULL, "cpufreq-dt", -1,
> + pd, sizeof(*pd));
can't this fail?
> +
> + return 0;
> +
> +dt_pdata_release:
> + cpufreq_dt_pdata_release();
> + return ret;
> +}
> +
> +module_init(mt8173_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` pi-cheng.chen
(?)
@ 2015-01-19 16:00 ` Mike Turquette
-1 siblings, 0 replies; 34+ messages in thread
From: Mike Turquette @ 2015-01-19 16:00 UTC (permalink / raw)
To: pi-cheng.chen, Rafael J. Wysocki, Viresh Kumar, Matthias Brugger,
Thomas Petazzoni
Cc: pi-cheng.chen, linux-kernel, linux-pm, linux-arm-kernel,
linaro-kernel, Eddie Huang, Yingjoe Chen
Quoting pi-cheng.chen (2015-01-09 01:54:51)
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
Hello Pi-Cheng,
<snip>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
I'll echo what Viresh said here. CPUfreq drivers should typically be
clock consumers and only require clk.h, not clk-provider.h. More on that
below.
<snip>
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
like a clock that belong to the clock driver, but this cpufreq driver is
writing to that register directly.
<snip>
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
<snip>
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
This is definitely bad. Why not use clk_get() here? __clk_lookup should
not be exposed to clock consumer drivers (and I hope to get rid of it
completely some day).
Regards,
Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-19 16:00 ` Mike Turquette
0 siblings, 0 replies; 34+ messages in thread
From: Mike Turquette @ 2015-01-19 16:00 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger, Thomas Petazzoni
Cc: pi-cheng.chen, linux-kernel, linux-pm, linux-arm-kernel,
linaro-kernel, Eddie Huang, Yingjoe Chen
Quoting pi-cheng.chen (2015-01-09 01:54:51)
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
Hello Pi-Cheng,
<snip>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
I'll echo what Viresh said here. CPUfreq drivers should typically be
clock consumers and only require clk.h, not clk-provider.h. More on that
below.
<snip>
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
like a clock that belong to the clock driver, but this cpufreq driver is
writing to that register directly.
<snip>
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
<snip>
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
This is definitely bad. Why not use clk_get() here? __clk_lookup should
not be exposed to clock consumer drivers (and I hope to get rid of it
completely some day).
Regards,
Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-19 16:00 ` Mike Turquette
0 siblings, 0 replies; 34+ messages in thread
From: Mike Turquette @ 2015-01-19 16:00 UTC (permalink / raw)
To: linux-arm-kernel
Quoting pi-cheng.chen (2015-01-09 01:54:51)
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> new file mode 100644
> index 0000000..b578c10
> --- /dev/null
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -0,0 +1,459 @@
Hello Pi-Cheng,
<snip>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpufreq-dt.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
I'll echo what Viresh said here. CPUfreq drivers should typically be
clock consumers and only require clk.h, not clk-provider.h. More on that
below.
<snip>
> +static void cpuclk_mux_set(int cluster, u32 sel)
> +{
> + u32 val;
> + u32 mask = 0x3;
> +
> + if (cluster == BIG_CLUSTER) {
> + mask <<= 2;
> + sel <<= 2;
> + }
> +
> + spin_lock(&lock);
> +
> + val = readl(clk_mux_regs);
> + val = (val & ~mask) | sel;
> + writel(val, clk_mux_regs);
> +
> + spin_unlock(&lock);
> +}
Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
like a clock that belong to the clock driver, but this cpufreq driver is
writing to that register directly.
<snip>
> +static int mt8173_cpufreq_dvfs_info_init(void)
> +{
<snip>
> + mainpll = __clk_lookup("mainpll");
> + if (!mainpll) {
> + pr_err("failed to get mainpll clk\n");
> + ret = -ENOENT;
> + goto dvfs_info_release;
> + }
> + mainpll_freq = clk_get_rate(mainpll);
This is definitely bad. Why not use clk_get() here? __clk_lookup should
not be exposed to clock consumer drivers (and I hope to get rid of it
completely some day).
Regards,
Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-19 10:42 ` Viresh Kumar
(?)
@ 2015-01-20 7:06 ` Pi-Cheng Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:06 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 18:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
>> SoC works properly:
>> 1. When doing frequency scaling of CPUs, the clock source should be switched
>> to another PLL, then switched back to the orignal until it's stable.
>> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
>> controlled concurrently and 2 limitations should be followed:
>> a. Vsram > Vproc
>> b. Vsram - Vproc < 200 mV
>>
>> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
>> cpufreq notifier.
>
> I haven't understood how is all this working..
> - How are you switching to intermediate freq when its support isn't there
> in cpufreq-dt.
> - How is changing of volt after switching to freq working? Specially when freq
> is higher than older freq and it requires changing of voltage before freq..
>
I was trying to switch clock source to the intermediate clock in the
pre-notitifer
and post-notifier by calling the function cpuclk_mux_set defined below and I
understand it's not a goood idea. I will try to work on adding
intermediate freqs
suppport in cpufreq-dt so that it will also greatly reduce the
complexity of this
driver.
Voltage changing was also handled in pre-notifier and post-notifier. Please see
below.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 6 +
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 466 insertions(+)
>> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0f9a2c3..97ed7dd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_MT8173_CPUFREQ
>> + bool "Mediatek MT8173 CPUFreq support"
>> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
>> + help
>> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index b3ca7b0..67b7f17 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>
> Add this in alphanumeric order please.
>
Will fix it.
>>
>> ##################################################################################
>> # PowerPC platform drivers
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
>> +/*
>> +* Copyright (c) 2015 Linaro.
>> +* 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/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> This driver isn't a clock provider, right? Then why are
> you including this file?
>
I included this file because I was using __clk_lookup to get the
intermediate clk and I just realize it is not a right way.
Thank Mike for pointing me out the right API to do it.
Will fix it.
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu.h>
>> +#include <linux/pm_opp.h>
>
> keep them in alphanumeric order please.
>
Will fix it.
>> +
>> +#define CPUCLK_MUX_ARMPLL 0x1
>> +#define CPUCLK_MUX_MAINPLL 0x2
>> +#define VOLT_SHIFT_LIMIT 150000
>> +
>> +enum {
>> + LITTLE_CLUSTER = 0,
>> + BIG_CLUSTER,
>> + NR_CLUSTERS
>> +};
>> +
>> +static struct cluster_dvfs_info {
>> + int cpu;
>> + struct cpumask cpus;
>> + struct device *cpu_dev;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + int inter_volt;
>> + u32 volt_tol;
>> +} *dvfs_info;
>> +
>> +static unsigned long mainpll_freq;
>> +static void __iomem *clk_mux_regs;
>> +static spinlock_t lock;
>> +
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>> +
>> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
>> + unsigned long old_vproc,
>> + unsigned long new_vproc)
>> +{
>> + int cur_vsram, cur_vproc, target_volt, tol;
>> +
>> + if (old_vproc > new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
>> + tol);
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> + }
>> + } else if (old_vproc < new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> +
>> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
>> + cur_vsram = regulator_get_voltage(
>> + dvfs->sram_reg);
>> + target_volt = cur_vsram - 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + target_volt, tol);
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
>> +{
>> + struct dev_pm_opp *opp;
>> + int volt;
>> +
>> + rcu_read_lock();
>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
>> + return PTR_ERR(opp);
>> + }
>> + volt = dev_pm_opp_get_voltage(opp);
>> + rcu_read_unlock();
>> +
>> + return volt;
>> +}
>> +
>> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> +
>> + if (dvfs->inter_volt == 0)
>> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
>> + mainpll_freq);
>> +
>> + return dvfs->inter_volt;
>> +}
>> +
>> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> + int ret = 0;
>> +
>> + if (cluster == LITTLE_CLUSTER) {
>> + int tol;
>> +
>> + tol = new_volt * dvfs->volt_tol / 100;
>> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
>> + } else {
>> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
>> + }
>> +
>> + if (ret)
>> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
>> + __func__, cluster, old_volt, new_volt);
>> +}
>> +
>> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct cpufreq_freqs *freqs = data;
>> + struct cluster_dvfs_info *dvfs;
>> + unsigned long old_volt, new_volt, inter_volt;
>> + int cluster;
>> +
>> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
>
> You should check it against cpumask. freqs->cpu can change on the event
> of hotplug of CPUs.
>
Will fix it.
>> + cluster = BIG_CLUSTER;
>> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
>> + cluster = LITTLE_CLUSTER;
>> + else
>> + return NOTIFY_DONE;
>> +
>> + dvfs = &dvfs_info[cluster];
>> +
>> + old_volt = regulator_get_voltage(dvfs->proc_reg);
>> +
>> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
>> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
>> +
>> + if (action == CPUFREQ_PRECHANGE) {
>> + if (old_volt < inter_volt || old_volt < new_volt) {
>> + new_volt = inter_volt > new_volt ?
>> + inter_volt : new_volt;
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
>> + } else if (action == CPUFREQ_POSTCHANGE) {
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
>> +
>> + if (new_volt < old_volt)
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
Here is the place I am handling voltage changing.
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block mt8173_cpufreq_nb = {
>> + .notifier_call = mt8173_cpufreq_notify,
>> +};
>> +
>> +static struct cpufreq_dt_platform_data *pd;
>> +
>> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
>> +{
>> + struct list_head *node;
>> +
>> + list_for_each(node, domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + domain = container_of(node, struct cpufreq_cpu_domain, node);
>> + if (cpumask_test_cpu(cpu, &domain->cpus))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_dt_pdata_release(void)
>> +{
>> + if (!pd)
>> + return;
>> +
>> + if (!list_empty(&pd->domain_list)) {
>> + struct list_head *node, *tmp;
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + list_for_each_safe(node, tmp, &pd->domain_list) {
>> + list_del(node);
>> + domain = container_of(node, struct cpufreq_cpu_domain,
>> + node);
>> + kfree(domain);
>> + }
>> + }
>> +
>> + kfree(pd);
>> +}
>> +
>> +static int cpufreq_dt_pdata_init(void)
>> +{
>> + int cpu;
>> +
>> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + pd->independent_clocks = 1,
>> + INIT_LIST_HEAD(&pd->domain_list);
>> +
>> + for_each_possible_cpu(cpu) {
>> + struct cpufreq_cpu_domain *new_domain;
>> +
>> + if (cpu_in_domain_list(&pd->domain_list, cpu))
>> + continue;
>> +
>> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
>> + if (!new_domain) {
>> + cpufreq_dt_pdata_release();
>> + return -ENOMEM;
>> + }
>> +
>> + cpumask_copy(&new_domain->cpus,
>> + &cpu_topology[cpu].core_sibling);
>> + list_add(&new_domain->node, &pd->domain_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void mt8173_cpufreq_dvfs_info_release(void)
>> +{
>> + int i;
>> +
>> + if (!dvfs_info)
>> + return;
>> +
>> + for (i = 0; i < NR_CLUSTERS; ++i) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
>> + regulator_put(dvfs->proc_reg);
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
>> + regulator_put(dvfs->sram_reg);
>> + }
>> +
>> + if (clk_mux_regs)
>> + iounmap(clk_mux_regs);
>> +}
>> +
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>> + struct list_head *node;
>> + struct device_node *np;
>> + struct clk *mainpll;
>> + int i, ret;
>> +
>> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
>> + if (!dvfs_info)
>> + return -ENOMEM;
>> +
>> + list_for_each(node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain = container_of(
>> + node, struct cpufreq_cpu_domain, node);
>> + int cpu = cpumask_next(0, &domain->cpus);
>
> cpumask_first() ?
>
Yes. Will fix it.
>> +
>> + np = of_get_cpu_node(cpu, NULL);
>
> This can fail..
>
I'll add error checking for this.
>> +
>> + if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a53") >= 0)
>> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
>> + &domain->cpus);
>> + else if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a57") >= 0)
>> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
>> + &domain->cpus);
>> + else {
>> + of_node_put(np);
>> + pr_err("%s: unknown CPU core\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(np);
>> + }
>> +
>> + for (i = 0; i < NR_CLUSTERS; i++) {
>
> You can merge this for-loop with the above for-loop..
>
Will fix it.
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> + int cpu;
>> +
>> + for_each_cpu_mask(cpu, dvfs->cpus) {
>> + struct device_node *np;
>> +
>> + dvfs->cpu_dev = get_cpu_device(cpu);
>> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
>> + if (IS_ERR(dvfs->proc_reg))
>> + continue;
>> +
>> + dvfs->cpu = cpu;
>> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
>> +
>> + np = of_node_get(dvfs->cpu_dev->of_node);
>> + of_property_read_u32(np, "voltage-tolerance",
>> + &dvfs->volt_tol);
>> + of_node_put(np);
>> + break;
>> + }
>> +
>> + if (IS_ERR(dvfs->proc_reg)) {
>> + pr_err("%s: no proc regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
>> + pr_err("%s: no sram regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> + }
>> +
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>> +
>> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
>> + if (!np) {
>> + pr_err("failed to find clock mux registers\n");
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + clk_mux_regs = of_iomap(np, 0);
>> + if (!clk_mux_regs) {
>> + pr_err("failed to map clock mux registers\n");
>> + ret = -EFAULT;
>> + goto dvfs_info_release;
>> + }
>> + of_node_put(np);
>> +
>> + spin_lock_init(&lock);
>> +
>> + return 0;
>> +
>> +dvfs_info_release:
>> + mt8173_cpufreq_dvfs_info_release();
>> + return ret;
>> +}
>> +
>> +static int mt8173_cpufreq_driver_init(void)
>> +{
>> + int ret;
>> +
>> + if (!of_machine_is_compatible("mediatek,mt8173"))
>> + return -ENODEV;
>> +
>> + ret = cpufreq_dt_pdata_init();
>> + if (ret) {
>> + pr_err("failed to initialize platform data: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = mt8173_cpufreq_dvfs_info_init();
>> + if (ret) {
>> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
>> + goto dt_pdata_release;
>> + }
>> +
>> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>> + if (ret) {
>> + pr_err("failed to register cpufreq notifier: %d\n", ret);
>> + goto dt_pdata_release;
>
> dvfs_info_release here ?
>
Yes. Will fix this error path.
>> + }
>> +
>> + platform_device_register_data(NULL, "cpufreq-dt", -1,
>> + pd, sizeof(*pd));
>
> can't this fail?
>
Yes. I'll add error checking for this.
Thanks again for your reviewing.
Best Regards,
Pi-Cheng
>> +
>> + return 0;
>> +
>> +dt_pdata_release:
>> + cpufreq_dt_pdata_release();
>> + return ret;
>> +}
>> +
>> +module_init(mt8173_cpufreq_driver_init);
>> +
>> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
>> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
>> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-20 7:06 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:06 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 18:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
>> SoC works properly:
>> 1. When doing frequency scaling of CPUs, the clock source should be switched
>> to another PLL, then switched back to the orignal until it's stable.
>> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
>> controlled concurrently and 2 limitations should be followed:
>> a. Vsram > Vproc
>> b. Vsram - Vproc < 200 mV
>>
>> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
>> cpufreq notifier.
>
> I haven't understood how is all this working..
> - How are you switching to intermediate freq when its support isn't there
> in cpufreq-dt.
> - How is changing of volt after switching to freq working? Specially when freq
> is higher than older freq and it requires changing of voltage before freq..
>
I was trying to switch clock source to the intermediate clock in the
pre-notitifer
and post-notifier by calling the function cpuclk_mux_set defined below and I
understand it's not a goood idea. I will try to work on adding
intermediate freqs
suppport in cpufreq-dt so that it will also greatly reduce the
complexity of this
driver.
Voltage changing was also handled in pre-notifier and post-notifier. Please see
below.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 6 +
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 466 insertions(+)
>> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0f9a2c3..97ed7dd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_MT8173_CPUFREQ
>> + bool "Mediatek MT8173 CPUFreq support"
>> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
>> + help
>> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index b3ca7b0..67b7f17 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>
> Add this in alphanumeric order please.
>
Will fix it.
>>
>> ##################################################################################
>> # PowerPC platform drivers
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
>> +/*
>> +* Copyright (c) 2015 Linaro.
>> +* 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/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> This driver isn't a clock provider, right? Then why are
> you including this file?
>
I included this file because I was using __clk_lookup to get the
intermediate clk and I just realize it is not a right way.
Thank Mike for pointing me out the right API to do it.
Will fix it.
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu.h>
>> +#include <linux/pm_opp.h>
>
> keep them in alphanumeric order please.
>
Will fix it.
>> +
>> +#define CPUCLK_MUX_ARMPLL 0x1
>> +#define CPUCLK_MUX_MAINPLL 0x2
>> +#define VOLT_SHIFT_LIMIT 150000
>> +
>> +enum {
>> + LITTLE_CLUSTER = 0,
>> + BIG_CLUSTER,
>> + NR_CLUSTERS
>> +};
>> +
>> +static struct cluster_dvfs_info {
>> + int cpu;
>> + struct cpumask cpus;
>> + struct device *cpu_dev;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + int inter_volt;
>> + u32 volt_tol;
>> +} *dvfs_info;
>> +
>> +static unsigned long mainpll_freq;
>> +static void __iomem *clk_mux_regs;
>> +static spinlock_t lock;
>> +
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>> +
>> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
>> + unsigned long old_vproc,
>> + unsigned long new_vproc)
>> +{
>> + int cur_vsram, cur_vproc, target_volt, tol;
>> +
>> + if (old_vproc > new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
>> + tol);
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> + }
>> + } else if (old_vproc < new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> +
>> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
>> + cur_vsram = regulator_get_voltage(
>> + dvfs->sram_reg);
>> + target_volt = cur_vsram - 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + target_volt, tol);
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
>> +{
>> + struct dev_pm_opp *opp;
>> + int volt;
>> +
>> + rcu_read_lock();
>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
>> + return PTR_ERR(opp);
>> + }
>> + volt = dev_pm_opp_get_voltage(opp);
>> + rcu_read_unlock();
>> +
>> + return volt;
>> +}
>> +
>> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> +
>> + if (dvfs->inter_volt == 0)
>> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
>> + mainpll_freq);
>> +
>> + return dvfs->inter_volt;
>> +}
>> +
>> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> + int ret = 0;
>> +
>> + if (cluster == LITTLE_CLUSTER) {
>> + int tol;
>> +
>> + tol = new_volt * dvfs->volt_tol / 100;
>> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
>> + } else {
>> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
>> + }
>> +
>> + if (ret)
>> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
>> + __func__, cluster, old_volt, new_volt);
>> +}
>> +
>> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct cpufreq_freqs *freqs = data;
>> + struct cluster_dvfs_info *dvfs;
>> + unsigned long old_volt, new_volt, inter_volt;
>> + int cluster;
>> +
>> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
>
> You should check it against cpumask. freqs->cpu can change on the event
> of hotplug of CPUs.
>
Will fix it.
>> + cluster = BIG_CLUSTER;
>> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
>> + cluster = LITTLE_CLUSTER;
>> + else
>> + return NOTIFY_DONE;
>> +
>> + dvfs = &dvfs_info[cluster];
>> +
>> + old_volt = regulator_get_voltage(dvfs->proc_reg);
>> +
>> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
>> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
>> +
>> + if (action == CPUFREQ_PRECHANGE) {
>> + if (old_volt < inter_volt || old_volt < new_volt) {
>> + new_volt = inter_volt > new_volt ?
>> + inter_volt : new_volt;
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
>> + } else if (action == CPUFREQ_POSTCHANGE) {
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
>> +
>> + if (new_volt < old_volt)
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
Here is the place I am handling voltage changing.
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block mt8173_cpufreq_nb = {
>> + .notifier_call = mt8173_cpufreq_notify,
>> +};
>> +
>> +static struct cpufreq_dt_platform_data *pd;
>> +
>> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
>> +{
>> + struct list_head *node;
>> +
>> + list_for_each(node, domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + domain = container_of(node, struct cpufreq_cpu_domain, node);
>> + if (cpumask_test_cpu(cpu, &domain->cpus))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_dt_pdata_release(void)
>> +{
>> + if (!pd)
>> + return;
>> +
>> + if (!list_empty(&pd->domain_list)) {
>> + struct list_head *node, *tmp;
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + list_for_each_safe(node, tmp, &pd->domain_list) {
>> + list_del(node);
>> + domain = container_of(node, struct cpufreq_cpu_domain,
>> + node);
>> + kfree(domain);
>> + }
>> + }
>> +
>> + kfree(pd);
>> +}
>> +
>> +static int cpufreq_dt_pdata_init(void)
>> +{
>> + int cpu;
>> +
>> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + pd->independent_clocks = 1,
>> + INIT_LIST_HEAD(&pd->domain_list);
>> +
>> + for_each_possible_cpu(cpu) {
>> + struct cpufreq_cpu_domain *new_domain;
>> +
>> + if (cpu_in_domain_list(&pd->domain_list, cpu))
>> + continue;
>> +
>> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
>> + if (!new_domain) {
>> + cpufreq_dt_pdata_release();
>> + return -ENOMEM;
>> + }
>> +
>> + cpumask_copy(&new_domain->cpus,
>> + &cpu_topology[cpu].core_sibling);
>> + list_add(&new_domain->node, &pd->domain_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void mt8173_cpufreq_dvfs_info_release(void)
>> +{
>> + int i;
>> +
>> + if (!dvfs_info)
>> + return;
>> +
>> + for (i = 0; i < NR_CLUSTERS; ++i) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
>> + regulator_put(dvfs->proc_reg);
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
>> + regulator_put(dvfs->sram_reg);
>> + }
>> +
>> + if (clk_mux_regs)
>> + iounmap(clk_mux_regs);
>> +}
>> +
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>> + struct list_head *node;
>> + struct device_node *np;
>> + struct clk *mainpll;
>> + int i, ret;
>> +
>> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
>> + if (!dvfs_info)
>> + return -ENOMEM;
>> +
>> + list_for_each(node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain = container_of(
>> + node, struct cpufreq_cpu_domain, node);
>> + int cpu = cpumask_next(0, &domain->cpus);
>
> cpumask_first() ?
>
Yes. Will fix it.
>> +
>> + np = of_get_cpu_node(cpu, NULL);
>
> This can fail..
>
I'll add error checking for this.
>> +
>> + if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a53") >= 0)
>> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
>> + &domain->cpus);
>> + else if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a57") >= 0)
>> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
>> + &domain->cpus);
>> + else {
>> + of_node_put(np);
>> + pr_err("%s: unknown CPU core\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(np);
>> + }
>> +
>> + for (i = 0; i < NR_CLUSTERS; i++) {
>
> You can merge this for-loop with the above for-loop..
>
Will fix it.
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> + int cpu;
>> +
>> + for_each_cpu_mask(cpu, dvfs->cpus) {
>> + struct device_node *np;
>> +
>> + dvfs->cpu_dev = get_cpu_device(cpu);
>> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
>> + if (IS_ERR(dvfs->proc_reg))
>> + continue;
>> +
>> + dvfs->cpu = cpu;
>> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
>> +
>> + np = of_node_get(dvfs->cpu_dev->of_node);
>> + of_property_read_u32(np, "voltage-tolerance",
>> + &dvfs->volt_tol);
>> + of_node_put(np);
>> + break;
>> + }
>> +
>> + if (IS_ERR(dvfs->proc_reg)) {
>> + pr_err("%s: no proc regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
>> + pr_err("%s: no sram regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> + }
>> +
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>> +
>> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
>> + if (!np) {
>> + pr_err("failed to find clock mux registers\n");
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + clk_mux_regs = of_iomap(np, 0);
>> + if (!clk_mux_regs) {
>> + pr_err("failed to map clock mux registers\n");
>> + ret = -EFAULT;
>> + goto dvfs_info_release;
>> + }
>> + of_node_put(np);
>> +
>> + spin_lock_init(&lock);
>> +
>> + return 0;
>> +
>> +dvfs_info_release:
>> + mt8173_cpufreq_dvfs_info_release();
>> + return ret;
>> +}
>> +
>> +static int mt8173_cpufreq_driver_init(void)
>> +{
>> + int ret;
>> +
>> + if (!of_machine_is_compatible("mediatek,mt8173"))
>> + return -ENODEV;
>> +
>> + ret = cpufreq_dt_pdata_init();
>> + if (ret) {
>> + pr_err("failed to initialize platform data: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = mt8173_cpufreq_dvfs_info_init();
>> + if (ret) {
>> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
>> + goto dt_pdata_release;
>> + }
>> +
>> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>> + if (ret) {
>> + pr_err("failed to register cpufreq notifier: %d\n", ret);
>> + goto dt_pdata_release;
>
> dvfs_info_release here ?
>
Yes. Will fix this error path.
>> + }
>> +
>> + platform_device_register_data(NULL, "cpufreq-dt", -1,
>> + pd, sizeof(*pd));
>
> can't this fail?
>
Yes. I'll add error checking for this.
Thanks again for your reviewing.
Best Regards,
Pi-Cheng
>> +
>> + return 0;
>> +
>> +dt_pdata_release:
>> + cpufreq_dt_pdata_release();
>> + return ret;
>> +}
>> +
>> +module_init(mt8173_cpufreq_driver_init);
>> +
>> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
>> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
>> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-20 7:06 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 18:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> When doing DVFS on MT8173 SoC, 2 rules should be followed to make sure the
>> SoC works properly:
>> 1. When doing frequency scaling of CPUs, the clock source should be switched
>> to another PLL, then switched back to the orignal until it's stable.
>> 2. When doing voltage scaling of CA57 cluster, Vproc and Vsram need to be
>> controlled concurrently and 2 limitations should be followed:
>> a. Vsram > Vproc
>> b. Vsram - Vproc < 200 mV
>>
>> To address these needs, we reuse cpufreq-dt but do voltage scaling in the
>> cpufreq notifier.
>
> I haven't understood how is all this working..
> - How are you switching to intermediate freq when its support isn't there
> in cpufreq-dt.
> - How is changing of volt after switching to freq working? Specially when freq
> is higher than older freq and it requires changing of voltage before freq..
>
I was trying to switch clock source to the intermediate clock in the
pre-notitifer
and post-notifier by calling the function cpuclk_mux_set defined below and I
understand it's not a goood idea. I will try to work on adding
intermediate freqs
suppport in cpufreq-dt so that it will also greatly reduce the
complexity of this
driver.
Voltage changing was also handled in pre-notifier and post-notifier. Please see
below.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/Kconfig.arm | 6 +
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/mt8173-cpufreq.c | 459 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 466 insertions(+)
>> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 0f9a2c3..97ed7dd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -255,3 +255,9 @@ config ARM_PXA2xx_CPUFREQ
>> This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>> If in doubt, say N.
>> +
>> +config ARM_MT8173_CPUFREQ
>> + bool "Mediatek MT8173 CPUFreq support"
>> + depends on ARCH_MEDIATEK && CPUFREQ_DT && REGULATOR
>> + help
>> + This adds the CPUFreq driver support for Mediatek MT8173 SoC.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index b3ca7b0..67b7f17 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o
>
> Add this in alphanumeric order please.
>
Will fix it.
>>
>> ##################################################################################
>> # PowerPC platform drivers
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
>> +/*
>> +* Copyright (c) 2015 Linaro.
>> +* 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/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> This driver isn't a clock provider, right? Then why are
> you including this file?
>
I included this file because I was using __clk_lookup to get the
intermediate clk and I just realize it is not a right way.
Thank Mike for pointing me out the right API to do it.
Will fix it.
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu.h>
>> +#include <linux/pm_opp.h>
>
> keep them in alphanumeric order please.
>
Will fix it.
>> +
>> +#define CPUCLK_MUX_ARMPLL 0x1
>> +#define CPUCLK_MUX_MAINPLL 0x2
>> +#define VOLT_SHIFT_LIMIT 150000
>> +
>> +enum {
>> + LITTLE_CLUSTER = 0,
>> + BIG_CLUSTER,
>> + NR_CLUSTERS
>> +};
>> +
>> +static struct cluster_dvfs_info {
>> + int cpu;
>> + struct cpumask cpus;
>> + struct device *cpu_dev;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + int inter_volt;
>> + u32 volt_tol;
>> +} *dvfs_info;
>> +
>> +static unsigned long mainpll_freq;
>> +static void __iomem *clk_mux_regs;
>> +static spinlock_t lock;
>> +
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>> +
>> +static int mt8173_cpufreq_voltage_step(struct cluster_dvfs_info *dvfs,
>> + unsigned long old_vproc,
>> + unsigned long new_vproc)
>> +{
>> + int cur_vsram, cur_vproc, target_volt, tol;
>> +
>> + if (old_vproc > new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + target_volt = cur_vsram - VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg, target_volt,
>> + tol);
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> + }
>> + } else if (old_vproc < new_vproc) {
>> + while (1) {
>> + cur_vsram = regulator_get_voltage(dvfs->sram_reg);
>> + if (cur_vsram - new_vproc < VOLT_SHIFT_LIMIT &&
>> + cur_vsram > new_vproc) {
>> + tol = new_vproc * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + new_vproc, tol);
>> + break;
>> + }
>> +
>> + cur_vproc = regulator_get_voltage(dvfs->proc_reg);
>> + target_volt = cur_vproc + VOLT_SHIFT_LIMIT;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->sram_reg, target_volt,
>> + tol);
>> +
>> + if (new_vproc - cur_vproc > VOLT_SHIFT_LIMIT) {
>> + cur_vsram = regulator_get_voltage(
>> + dvfs->sram_reg);
>> + target_volt = cur_vsram - 1;
>> + tol = target_volt * dvfs->volt_tol / 100;
>> + regulator_set_voltage_tol(dvfs->proc_reg,
>> + target_volt, tol);
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int get_opp_voltage(struct device *dev, unsigned long freq_hz)
>> +{
>> + struct dev_pm_opp *opp;
>> + int volt;
>> +
>> + rcu_read_lock();
>> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> + if (IS_ERR(opp)) {
>> + rcu_read_unlock();
>> + pr_err("%s: failed to find OPP for %lu\n", __func__, freq_hz);
>> + return PTR_ERR(opp);
>> + }
>> + volt = dev_pm_opp_get_voltage(opp);
>> + rcu_read_unlock();
>> +
>> + return volt;
>> +}
>> +
>> +static int mt8173_cpufreq_get_intermediate_voltage(int cluster)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> +
>> + if (dvfs->inter_volt == 0)
>> + dvfs->inter_volt = get_opp_voltage(dvfs->cpu_dev,
>> + mainpll_freq);
>> +
>> + return dvfs->inter_volt;
>> +}
>> +
>> +static void mt8173_cpufreq_set_voltage(int cluster, int old_volt, int new_volt)
>> +{
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[cluster];
>> + int ret = 0;
>> +
>> + if (cluster == LITTLE_CLUSTER) {
>> + int tol;
>> +
>> + tol = new_volt * dvfs->volt_tol / 100;
>> + ret = regulator_set_voltage_tol(dvfs->proc_reg, new_volt, tol);
>> + } else {
>> + ret = mt8173_cpufreq_voltage_step(dvfs, old_volt, new_volt);
>> + }
>> +
>> + if (ret)
>> + pr_err("%s: cluster%d failed to scale voltage (%dmV to %dmV)",
>> + __func__, cluster, old_volt, new_volt);
>> +}
>> +
>> +static int mt8173_cpufreq_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct cpufreq_freqs *freqs = data;
>> + struct cluster_dvfs_info *dvfs;
>> + unsigned long old_volt, new_volt, inter_volt;
>> + int cluster;
>> +
>> + if (freqs->cpu == dvfs_info[BIG_CLUSTER].cpu)
>
> You should check it against cpumask. freqs->cpu can change on the event
> of hotplug of CPUs.
>
Will fix it.
>> + cluster = BIG_CLUSTER;
>> + else if (freqs->cpu == dvfs_info[LITTLE_CLUSTER].cpu)
>> + cluster = LITTLE_CLUSTER;
>> + else
>> + return NOTIFY_DONE;
>> +
>> + dvfs = &dvfs_info[cluster];
>> +
>> + old_volt = regulator_get_voltage(dvfs->proc_reg);
>> +
>> + new_volt = get_opp_voltage(dvfs->cpu_dev, freqs->new * 1000);
>> + inter_volt = mt8173_cpufreq_get_intermediate_voltage(cluster);
>> +
>> + if (action == CPUFREQ_PRECHANGE) {
>> + if (old_volt < inter_volt || old_volt < new_volt) {
>> + new_volt = inter_volt > new_volt ?
>> + inter_volt : new_volt;
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_MAINPLL);
>> + } else if (action == CPUFREQ_POSTCHANGE) {
>> + cpuclk_mux_set(cluster, CPUCLK_MUX_ARMPLL);
>> +
>> + if (new_volt < old_volt)
>> + mt8173_cpufreq_set_voltage(cluster, old_volt,
>> + new_volt);
>> + }
>> +
Here is the place I am handling voltage changing.
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block mt8173_cpufreq_nb = {
>> + .notifier_call = mt8173_cpufreq_notify,
>> +};
>> +
>> +static struct cpufreq_dt_platform_data *pd;
>> +
>> +static int cpu_in_domain_list(struct list_head *domain_list, int cpu)
>> +{
>> + struct list_head *node;
>> +
>> + list_for_each(node, domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + domain = container_of(node, struct cpufreq_cpu_domain, node);
>> + if (cpumask_test_cpu(cpu, &domain->cpus))
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cpufreq_dt_pdata_release(void)
>> +{
>> + if (!pd)
>> + return;
>> +
>> + if (!list_empty(&pd->domain_list)) {
>> + struct list_head *node, *tmp;
>> + struct cpufreq_cpu_domain *domain;
>> +
>> + list_for_each_safe(node, tmp, &pd->domain_list) {
>> + list_del(node);
>> + domain = container_of(node, struct cpufreq_cpu_domain,
>> + node);
>> + kfree(domain);
>> + }
>> + }
>> +
>> + kfree(pd);
>> +}
>> +
>> +static int cpufreq_dt_pdata_init(void)
>> +{
>> + int cpu;
>> +
>> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
>> + if (!pd)
>> + return -ENOMEM;
>> +
>> + pd->independent_clocks = 1,
>> + INIT_LIST_HEAD(&pd->domain_list);
>> +
>> + for_each_possible_cpu(cpu) {
>> + struct cpufreq_cpu_domain *new_domain;
>> +
>> + if (cpu_in_domain_list(&pd->domain_list, cpu))
>> + continue;
>> +
>> + new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
>> + if (!new_domain) {
>> + cpufreq_dt_pdata_release();
>> + return -ENOMEM;
>> + }
>> +
>> + cpumask_copy(&new_domain->cpus,
>> + &cpu_topology[cpu].core_sibling);
>> + list_add(&new_domain->node, &pd->domain_list);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void mt8173_cpufreq_dvfs_info_release(void)
>> +{
>> + int i;
>> +
>> + if (!dvfs_info)
>> + return;
>> +
>> + for (i = 0; i < NR_CLUSTERS; ++i) {
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->proc_reg))
>> + regulator_put(dvfs->proc_reg);
>> +
>> + if (!IS_ERR_OR_NULL(dvfs->sram_reg))
>> + regulator_put(dvfs->sram_reg);
>> + }
>> +
>> + if (clk_mux_regs)
>> + iounmap(clk_mux_regs);
>> +}
>> +
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>> + struct list_head *node;
>> + struct device_node *np;
>> + struct clk *mainpll;
>> + int i, ret;
>> +
>> + dvfs_info = kzalloc(sizeof(*dvfs) * NR_CLUSTERS, GFP_KERNEL);
>> + if (!dvfs_info)
>> + return -ENOMEM;
>> +
>> + list_for_each(node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain = container_of(
>> + node, struct cpufreq_cpu_domain, node);
>> + int cpu = cpumask_next(0, &domain->cpus);
>
> cpumask_first() ?
>
Yes. Will fix it.
>> +
>> + np = of_get_cpu_node(cpu, NULL);
>
> This can fail..
>
I'll add error checking for this.
>> +
>> + if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a53") >= 0)
>> + cpumask_copy(&dvfs_info[LITTLE_CLUSTER].cpus,
>> + &domain->cpus);
>> + else if (of_property_match_string(np, "compatible",
>> + "arm,cortex-a57") >= 0)
>> + cpumask_copy(&dvfs_info[BIG_CLUSTER].cpus,
>> + &domain->cpus);
>> + else {
>> + of_node_put(np);
>> + pr_err("%s: unknown CPU core\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + of_node_put(np);
>> + }
>> +
>> + for (i = 0; i < NR_CLUSTERS; i++) {
>
> You can merge this for-loop with the above for-loop..
>
Will fix it.
>> + struct cluster_dvfs_info *dvfs = &dvfs_info[i];
>> + int cpu;
>> +
>> + for_each_cpu_mask(cpu, dvfs->cpus) {
>> + struct device_node *np;
>> +
>> + dvfs->cpu_dev = get_cpu_device(cpu);
>> + dvfs->proc_reg = regulator_get(dvfs->cpu_dev, "proc");
>> + if (IS_ERR(dvfs->proc_reg))
>> + continue;
>> +
>> + dvfs->cpu = cpu;
>> + dvfs->sram_reg = regulator_get(dvfs->cpu_dev, "sram");
>> +
>> + np = of_node_get(dvfs->cpu_dev->of_node);
>> + of_property_read_u32(np, "voltage-tolerance",
>> + &dvfs->volt_tol);
>> + of_node_put(np);
>> + break;
>> + }
>> +
>> + if (IS_ERR(dvfs->proc_reg)) {
>> + pr_err("%s: no proc regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + if (IS_ERR(dvfs->sram_reg) && BIG_CLUSTER == i) {
>> + pr_err("%s: no sram regulator for cluster %d\n",
>> + __func__, i);
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> + }
>> +
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>> +
>> + np = of_find_compatible_node(NULL, NULL, "mediatek,mt8173-infrasys");
>> + if (!np) {
>> + pr_err("failed to find clock mux registers\n");
>> + ret = -ENODEV;
>> + goto dvfs_info_release;
>> + }
>> +
>> + clk_mux_regs = of_iomap(np, 0);
>> + if (!clk_mux_regs) {
>> + pr_err("failed to map clock mux registers\n");
>> + ret = -EFAULT;
>> + goto dvfs_info_release;
>> + }
>> + of_node_put(np);
>> +
>> + spin_lock_init(&lock);
>> +
>> + return 0;
>> +
>> +dvfs_info_release:
>> + mt8173_cpufreq_dvfs_info_release();
>> + return ret;
>> +}
>> +
>> +static int mt8173_cpufreq_driver_init(void)
>> +{
>> + int ret;
>> +
>> + if (!of_machine_is_compatible("mediatek,mt8173"))
>> + return -ENODEV;
>> +
>> + ret = cpufreq_dt_pdata_init();
>> + if (ret) {
>> + pr_err("failed to initialize platform data: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = mt8173_cpufreq_dvfs_info_init();
>> + if (ret) {
>> + pr_err("failed to initialize mt8173 dvfs info: %d\n", ret);
>> + goto dt_pdata_release;
>> + }
>> +
>> + ret = cpufreq_register_notifier(&mt8173_cpufreq_nb,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>> + if (ret) {
>> + pr_err("failed to register cpufreq notifier: %d\n", ret);
>> + goto dt_pdata_release;
>
> dvfs_info_release here ?
>
Yes. Will fix this error path.
>> + }
>> +
>> + platform_device_register_data(NULL, "cpufreq-dt", -1,
>> + pd, sizeof(*pd));
>
> can't this fail?
>
Yes. I'll add error checking for this.
Thanks again for your reviewing.
Best Regards,
Pi-Cheng
>> +
>> + return 0;
>> +
>> +dt_pdata_release:
>> + cpufreq_dt_pdata_release();
>> + return ret;
>> +}
>> +
>> +module_init(mt8173_cpufreq_driver_init);
>> +
>> +MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
>> +MODULE_DESCRIPTION("Mediatek MT8173 cpufreq driver");
>> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-19 16:00 ` Mike Turquette
(?)
@ 2015-01-20 7:18 ` Pi-Cheng Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:18 UTC (permalink / raw)
To: Mike Turquette
Cc: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger,
Thomas Petazzoni, Linux Kernel Mailing List, linux-pm,
linux-arm-kernel, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 00:00, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting pi-cheng.chen (2015-01-09 01:54:51)
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
Hello Mike,
>
> Hello Pi-Cheng,
>
> <snip>
>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> I'll echo what Viresh said here. CPUfreq drivers should typically be
> clock consumers and only require clk.h, not clk-provider.h. More on that
> below.
>
I included it because of the __clk_lookup call. But yes, I will use the clk_get
to get the intermediate clock to get rid of this.
> <snip>
>
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>
> Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
> like a clock that belong to the clock driver, but this cpufreq driver is
> writing to that register directly.
>
Yes, it's a mux but not presented in MT8173 clock driver yet. Therefore I
map the HW register and writing them directly to do reparent. I will get
rid of it once I got those muxes presented in MT8173 clock driver.
> <snip>
>
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>
> <snip>
>
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>
> This is definitely bad. Why not use clk_get() here? __clk_lookup should
> not be exposed to clock consumer drivers (and I hope to get rid of it
> completely some day).
>
Thanks for pointing me out the right API to do it. I will fix it in
next version.
Best Regards,
Pi-Cheng
> Regards,
> Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-20 7:18 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:18 UTC (permalink / raw)
To: Mike Turquette
Cc: Rafael J. Wysocki, Viresh Kumar, Matthias Brugger,
Thomas Petazzoni, Linux Kernel Mailing List, linux-pm,
linux-arm-kernel, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 00:00, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting pi-cheng.chen (2015-01-09 01:54:51)
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
Hello Mike,
>
> Hello Pi-Cheng,
>
> <snip>
>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> I'll echo what Viresh said here. CPUfreq drivers should typically be
> clock consumers and only require clk.h, not clk-provider.h. More on that
> below.
>
I included it because of the __clk_lookup call. But yes, I will use the clk_get
to get the intermediate clock to get rid of this.
> <snip>
>
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>
> Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
> like a clock that belong to the clock driver, but this cpufreq driver is
> writing to that register directly.
>
Yes, it's a mux but not presented in MT8173 clock driver yet. Therefore I
map the HW register and writing them directly to do reparent. I will get
rid of it once I got those muxes presented in MT8173 clock driver.
> <snip>
>
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>
> <snip>
>
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>
> This is definitely bad. Why not use clk_get() here? __clk_lookup should
> not be exposed to clock consumer drivers (and I hope to get rid of it
> completely some day).
>
Thanks for pointing me out the right API to do it. I will fix it in
next version.
Best Regards,
Pi-Cheng
> Regards,
> Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
@ 2015-01-20 7:18 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:18 UTC (permalink / raw)
To: linux-arm-kernel
On 20 January 2015 at 00:00, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting pi-cheng.chen (2015-01-09 01:54:51)
>> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
>> new file mode 100644
>> index 0000000..b578c10
>> --- /dev/null
>> +++ b/drivers/cpufreq/mt8173-cpufreq.c
>> @@ -0,0 +1,459 @@
Hello Mike,
>
> Hello Pi-Cheng,
>
> <snip>
>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpufreq-dt.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> I'll echo what Viresh said here. CPUfreq drivers should typically be
> clock consumers and only require clk.h, not clk-provider.h. More on that
> below.
>
I included it because of the __clk_lookup call. But yes, I will use the clk_get
to get the intermediate clock to get rid of this.
> <snip>
>
>> +static void cpuclk_mux_set(int cluster, u32 sel)
>> +{
>> + u32 val;
>> + u32 mask = 0x3;
>> +
>> + if (cluster == BIG_CLUSTER) {
>> + mask <<= 2;
>> + sel <<= 2;
>> + }
>> +
>> + spin_lock(&lock);
>> +
>> + val = readl(clk_mux_regs);
>> + val = (val & ~mask) | sel;
>> + writel(val, clk_mux_regs);
>> +
>> + spin_unlock(&lock);
>> +}
>
> Is cpuclk a mux that is represented in the MT8173 clock driver? It looks
> like a clock that belong to the clock driver, but this cpufreq driver is
> writing to that register directly.
>
Yes, it's a mux but not presented in MT8173 clock driver yet. Therefore I
map the HW register and writing them directly to do reparent. I will get
rid of it once I got those muxes presented in MT8173 clock driver.
> <snip>
>
>> +static int mt8173_cpufreq_dvfs_info_init(void)
>> +{
>
> <snip>
>
>> + mainpll = __clk_lookup("mainpll");
>> + if (!mainpll) {
>> + pr_err("failed to get mainpll clk\n");
>> + ret = -ENOENT;
>> + goto dvfs_info_release;
>> + }
>> + mainpll_freq = clk_get_rate(mainpll);
>
> This is definitely bad. Why not use clk_get() here? __clk_lookup should
> not be exposed to clock consumer drivers (and I hope to get rid of it
> completely some day).
>
Thanks for pointing me out the right API to do it. I will fix it in
next version.
Best Regards,
Pi-Cheng
> Regards,
> Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
2015-01-19 8:00 ` Viresh Kumar
(?)
@ 2015-01-20 7:33 ` Pi-Cheng Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:33 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 16:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> Currently the DT based cpufreq driver is missing some way to check which
>> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
>> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
>> check which CPUs share clock/power.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
>> include/linux/cpufreq-dt.h | 6 ++++++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
>> index fde97d6..ff8c266 100644
>> --- a/drivers/cpufreq/cpufreq-dt.c
>> +++ b/drivers/cpufreq/cpufreq-dt.c
>> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>> pd = cpufreq_get_driver_data();
>> if (!pd || !pd->independent_clocks)
>> cpumask_setall(policy->cpus);
>> + else if (pd && !list_empty(&pd->domain_list)) {
>> + struct list_head *domain_node;
>> +
>> + list_for_each(domain_node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>
> Define this with domain_node.
>
Will do it.
>> +
>> + domain = container_of(domain_node,
>> + struct cpufreq_cpu_domain, node);
>> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
>> + continue;
>> +
>> + cpumask_copy(policy->cpus, &domain->cpus);
>> + break;
>> + }
>> + }
>>
>> of_node_put(np);
>>
>> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
>> index 0414009..92bffd3 100644
>> --- a/include/linux/cpufreq-dt.h
>> +++ b/include/linux/cpufreq-dt.h
>> @@ -10,6 +10,11 @@
>> #ifndef __CPUFREQ_DT_H__
>> #define __CPUFREQ_DT_H__
>>
>> +struct cpufreq_cpu_domain {
>> + struct list_head node;
>> + cpumask_t cpus;
>> +};
>> +
>> struct cpufreq_dt_platform_data {
>> /*
>> * True when each CPU has its own clock to control its
>> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
>> * clock.
>> */
>> bool independent_clocks;
>> + struct list_head domain_list;
>> };
>
> Though we need to keep only one of these two, but I don't think
> any of these will stay for long time. So, its okay..
>
> Looks fine. Thanks.
Thanks.
I will also try to add intermediate frequency support in next version.
BTW, do you think it's a good idea to add a new device tree binding like
intermediate_clock = <&clksys MAINPLL>
and so that cpufreq-dt could get the intermediate clock?
Best Regards,
Pi-Cheng
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 7:33 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:33 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 16:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> Currently the DT based cpufreq driver is missing some way to check which
>> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
>> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
>> check which CPUs share clock/power.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
>> include/linux/cpufreq-dt.h | 6 ++++++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
>> index fde97d6..ff8c266 100644
>> --- a/drivers/cpufreq/cpufreq-dt.c
>> +++ b/drivers/cpufreq/cpufreq-dt.c
>> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>> pd = cpufreq_get_driver_data();
>> if (!pd || !pd->independent_clocks)
>> cpumask_setall(policy->cpus);
>> + else if (pd && !list_empty(&pd->domain_list)) {
>> + struct list_head *domain_node;
>> +
>> + list_for_each(domain_node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>
> Define this with domain_node.
>
Will do it.
>> +
>> + domain = container_of(domain_node,
>> + struct cpufreq_cpu_domain, node);
>> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
>> + continue;
>> +
>> + cpumask_copy(policy->cpus, &domain->cpus);
>> + break;
>> + }
>> + }
>>
>> of_node_put(np);
>>
>> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
>> index 0414009..92bffd3 100644
>> --- a/include/linux/cpufreq-dt.h
>> +++ b/include/linux/cpufreq-dt.h
>> @@ -10,6 +10,11 @@
>> #ifndef __CPUFREQ_DT_H__
>> #define __CPUFREQ_DT_H__
>>
>> +struct cpufreq_cpu_domain {
>> + struct list_head node;
>> + cpumask_t cpus;
>> +};
>> +
>> struct cpufreq_dt_platform_data {
>> /*
>> * True when each CPU has its own clock to control its
>> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
>> * clock.
>> */
>> bool independent_clocks;
>> + struct list_head domain_list;
>> };
>
> Though we need to keep only one of these two, but I don't think
> any of these will stay for long time. So, its okay..
>
> Looks fine. Thanks.
Thanks.
I will also try to add intermediate frequency support in next version.
BTW, do you think it's a good idea to add a new device tree binding like
intermediate_clock = <&clksys MAINPLL>
and so that cpufreq-dt could get the intermediate clock?
Best Regards,
Pi-Cheng
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 7:33 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 7:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Viresh,
Thanks for reviewing.
On 19 January 2015 at 16:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2015 at 15:24, pi-cheng.chen <pi-cheng.chen@linaro.org> wrote:
>> Currently the DT based cpufreq driver is missing some way to check which
>> CPUs share clocks. In the 1st patch, CPU clock/power domain information is
>> added to the platform_data of cpufreq-dt so that cpufreq-dt driver could
>> check which CPUs share clock/power.
>>
>> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
>> ---
>> drivers/cpufreq/cpufreq-dt.c | 15 +++++++++++++++
>> include/linux/cpufreq-dt.h | 6 ++++++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
>> index fde97d6..ff8c266 100644
>> --- a/drivers/cpufreq/cpufreq-dt.c
>> +++ b/drivers/cpufreq/cpufreq-dt.c
>> @@ -296,6 +296,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
>> pd = cpufreq_get_driver_data();
>> if (!pd || !pd->independent_clocks)
>> cpumask_setall(policy->cpus);
>> + else if (pd && !list_empty(&pd->domain_list)) {
>> + struct list_head *domain_node;
>> +
>> + list_for_each(domain_node, &pd->domain_list) {
>> + struct cpufreq_cpu_domain *domain;
>
> Define this with domain_node.
>
Will do it.
>> +
>> + domain = container_of(domain_node,
>> + struct cpufreq_cpu_domain, node);
>> + if (!cpumask_test_cpu(policy->cpu, &domain->cpus))
>> + continue;
>> +
>> + cpumask_copy(policy->cpus, &domain->cpus);
>> + break;
>> + }
>> + }
>>
>> of_node_put(np);
>>
>> diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
>> index 0414009..92bffd3 100644
>> --- a/include/linux/cpufreq-dt.h
>> +++ b/include/linux/cpufreq-dt.h
>> @@ -10,6 +10,11 @@
>> #ifndef __CPUFREQ_DT_H__
>> #define __CPUFREQ_DT_H__
>>
>> +struct cpufreq_cpu_domain {
>> + struct list_head node;
>> + cpumask_t cpus;
>> +};
>> +
>> struct cpufreq_dt_platform_data {
>> /*
>> * True when each CPU has its own clock to control its
>> @@ -17,6 +22,7 @@ struct cpufreq_dt_platform_data {
>> * clock.
>> */
>> bool independent_clocks;
>> + struct list_head domain_list;
>> };
>
> Though we need to keep only one of these two, but I don't think
> any of these will stay for long time. So, its okay..
>
> Looks fine. Thanks.
Thanks.
I will also try to add intermediate frequency support in next version.
BTW, do you think it's a good idea to add a new device tree binding like
intermediate_clock = <&clksys MAINPLL>
and so that cpufreq-dt could get the intermediate clock?
Best Regards,
Pi-Cheng
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
2015-01-20 7:33 ` Pi-Cheng Chen
(?)
@ 2015-01-20 7:39 ` Viresh Kumar
-1 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-20 7:39 UTC (permalink / raw)
To: Pi-Cheng Chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
> I will also try to add intermediate frequency support in next version.
Sure
> BTW, do you think it's a good idea to add a new device tree binding like
> intermediate_clock = <&clksys MAINPLL>
> and so that cpufreq-dt could get the intermediate clock?
Yeah, we need that. I am trying to implement new opp bindings and will
take care of that. Until that time, use platform data to do that.
--
viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 7:39 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-20 7:39 UTC (permalink / raw)
To: Pi-Cheng Chen
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
> I will also try to add intermediate frequency support in next version.
Sure
> BTW, do you think it's a good idea to add a new device tree binding like
> intermediate_clock = <&clksys MAINPLL>
> and so that cpufreq-dt could get the intermediate clock?
Yeah, we need that. I am trying to implement new opp bindings and will
take care of that. Until that time, use platform data to do that.
--
viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 7:39 ` Viresh Kumar
0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2015-01-20 7:39 UTC (permalink / raw)
To: linux-arm-kernel
On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
> I will also try to add intermediate frequency support in next version.
Sure
> BTW, do you think it's a good idea to add a new device tree binding like
> intermediate_clock = <&clksys MAINPLL>
> and so that cpufreq-dt could get the intermediate clock?
Yeah, we need that. I am trying to implement new opp bindings and will
take care of that. Until that time, use platform data to do that.
--
viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
2015-01-20 7:39 ` Viresh Kumar
(?)
@ 2015-01-20 8:47 ` Pi-Cheng Chen
-1 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 8:47 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 15:39, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
>> I will also try to add intermediate frequency support in next version.
>
> Sure
>
>> BTW, do you think it's a good idea to add a new device tree binding like
>> intermediate_clock = <&clksys MAINPLL>
>> and so that cpufreq-dt could get the intermediate clock?
>
> Yeah, we need that. I am trying to implement new opp bindings and will
> take care of that. Until that time, use platform data to do that.
>
Got it.
Thanks.
Best Regards,
Pi-Cheng
> --
> viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 8:47 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 8:47 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Matthias Brugger, Thomas Petazzoni,
Linux Kernel Mailing List, linux-pm, linux-arm-kernel,
Mike Turquette, Linaro Kernel Mailman List, Eddie Huang,
Yingjoe Chen
On 20 January 2015 at 15:39, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
>> I will also try to add intermediate frequency support in next version.
>
> Sure
>
>> BTW, do you think it's a good idea to add a new device tree binding like
>> intermediate_clock = <&clksys MAINPLL>
>> and so that cpufreq-dt could get the intermediate clock?
>
> Yeah, we need that. I am trying to implement new opp bindings and will
> take care of that. Until that time, use platform data to do that.
>
Got it.
Thanks.
Best Regards,
Pi-Cheng
> --
> viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing
@ 2015-01-20 8:47 ` Pi-Cheng Chen
0 siblings, 0 replies; 34+ messages in thread
From: Pi-Cheng Chen @ 2015-01-20 8:47 UTC (permalink / raw)
To: linux-arm-kernel
On 20 January 2015 at 15:39, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 January 2015 at 13:03, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote:
>> I will also try to add intermediate frequency support in next version.
>
> Sure
>
>> BTW, do you think it's a good idea to add a new device tree binding like
>> intermediate_clock = <&clksys MAINPLL>
>> and so that cpufreq-dt could get the intermediate clock?
>
> Yeah, we need that. I am trying to implement new opp bindings and will
> take care of that. Until that time, use platform data to do that.
>
Got it.
Thanks.
Best Regards,
Pi-Cheng
> --
> viresh
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-01-20 8:48 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-09 9:54 [PATCH 0/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
2015-01-09 9:54 ` pi-cheng.chen
2015-01-09 9:54 ` [PATCH 1/2] cpufreq-dt: check CPU clock/power domain during initializing pi-cheng.chen
2015-01-09 9:54 ` pi-cheng.chen
2015-01-19 8:00 ` Viresh Kumar
2015-01-19 8:00 ` Viresh Kumar
2015-01-19 8:00 ` Viresh Kumar
2015-01-20 7:33 ` Pi-Cheng Chen
2015-01-20 7:33 ` Pi-Cheng Chen
2015-01-20 7:33 ` Pi-Cheng Chen
2015-01-20 7:39 ` Viresh Kumar
2015-01-20 7:39 ` Viresh Kumar
2015-01-20 7:39 ` Viresh Kumar
2015-01-20 8:47 ` Pi-Cheng Chen
2015-01-20 8:47 ` Pi-Cheng Chen
2015-01-20 8:47 ` Pi-Cheng Chen
2015-01-09 9:54 ` [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
2015-01-09 9:54 ` pi-cheng.chen
2015-01-14 7:43 ` Yingjoe Chen
2015-01-14 7:43 ` Yingjoe Chen
2015-01-15 2:02 ` Pi-Cheng Chen
2015-01-15 2:02 ` Pi-Cheng Chen
2015-01-19 10:42 ` Viresh Kumar
2015-01-19 10:42 ` Viresh Kumar
2015-01-19 10:42 ` Viresh Kumar
2015-01-20 7:06 ` Pi-Cheng Chen
2015-01-20 7:06 ` Pi-Cheng Chen
2015-01-20 7:06 ` Pi-Cheng Chen
2015-01-19 16:00 ` Mike Turquette
2015-01-19 16:00 ` Mike Turquette
2015-01-19 16:00 ` Mike Turquette
2015-01-20 7:18 ` Pi-Cheng Chen
2015-01-20 7:18 ` Pi-Cheng Chen
2015-01-20 7:18 ` Pi-Cheng Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.