* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
@ 2015-01-14 7:43 ` Yingjoe Chen
2015-01-15 2:02 ` Pi-Cheng Chen
2015-01-19 10:42 ` Viresh Kumar
2015-01-19 16:00 ` Mike Turquette
2 siblings, 1 reply; 13+ 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] 13+ messages in thread* [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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
2015-01-14 7:43 ` Yingjoe Chen
@ 2015-01-19 10:42 ` Viresh Kumar
2015-01-20 7:06 ` Pi-Cheng Chen
2015-01-19 16:00 ` Mike Turquette
2 siblings, 1 reply; 13+ 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] 13+ messages in thread* [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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC
2015-01-09 9:54 ` [PATCH 2/2] cpufreq: add cpufreq driver for Mediatek MT8173 SoC pi-cheng.chen
2015-01-14 7:43 ` Yingjoe Chen
2015-01-19 10:42 ` Viresh Kumar
@ 2015-01-19 16:00 ` Mike Turquette
2015-01-20 7:18 ` Pi-Cheng Chen
2 siblings, 1 reply; 13+ 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] 13+ messages in thread* [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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread