Hi Viresh, Again thanks for your review comments. On Tue, Mar 5, 2013 at 6:48 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote: > On 2 March 2013 15:04, Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx> wrote: >> This patch adds dvfs support for exynos5440 SOC. This soc has 4 cores and >> they run at same frequency. The nature of exynos5440 clock controller is >> different from previous exynos controllers so not using the common exynos >> cpufreq framework. The major difference being interrupt notfication for >> frequency change. Also, OPP library is used for device tree parsing to get >> different parameters like frequency, voltage etc. Since the opp library sorts >> the frequency table in ascending order so they are again re-arranged in >> descending order. This will have one-to-one mapping with the clock controller >> state management logic. >> >> Signed-off-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx> >> --- >> Changes in V2: >> * Added OPP library support to parse DT parameters. >> * Removed a hack to handle interrupts in bootup. >> * Added many review comments from Viresh and Inder. > > Added? Thanks :) typo error :) > >> >> All these patches are dependent on Thomas Abraham common clock patches. >> http://www.mail-archive.com/linux-samsung-soc@xxxxxxxxxxxxxxx/msg15860.html >> >> .../bindings/cpufreq/cpufreq-exynos5440.txt | 32 ++ >> drivers/cpufreq/Kconfig.arm | 9 + >> drivers/cpufreq/Makefile | 1 + >> drivers/cpufreq/exynos5440-cpufreq.c | 450 ++++++++++++++++++++ >> 4 files changed, 492 insertions(+), 0 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-exynos5440.txt >> create mode 100644 drivers/cpufreq/exynos5440-cpufreq.c >> >> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-exynos5440.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-exynos5440.txt >> new file mode 100644 >> index 0000000..caa3f8d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-exynos5440.txt >> @@ -0,0 +1,32 @@ >> + >> +Exynos5440 cpufreq driver >> +------------------- >> + >> +Exynos5440 SoC cpufreq driver for CPU frequency scaling. >> + >> +Required properties: >> +- interrupts: Interrupt to know the completion of cpu frequency change. >> +- operating-points: Table of frequencies and voltage CPU could be transitioned into, >> + in the decreasing order. Frequency should be in KHZ units and voltage >> + should be in microvolts. >> +- clocks: phandle of the clock from common clock binding. More description can >> + be found in Documentation/devicetree/bindings/clock/clock-bindings.txt. >> + >> +Optional properties: >> +- clock-latency: Clock transition latency in microsecond. >> + >> +All the required listed above must be defined under node cpufreq. >> + >> +Example: >> +-------- >> + cpufreq@160000 { >> + compatible = "samsung,exynos5440-cpufreq"; >> + reg = <0x160000 0x1000>; >> + interrupts = <0 57 0>; >> + operating-points = < >> + 1000000 975000 >> + 800000 925000>; >> + clocks = <&clock 2>; >> + clock-latency = <100000>; >> + }; >> + >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 030ddf6..7ed9c4a 100644 >> --- a/drivers/cpufreq/Kconfig.arm >> +++ b/drivers/cpufreq/Kconfig.arm >> @@ -77,6 +77,15 @@ config ARM_EXYNOS5250_CPUFREQ >> This adds the CPUFreq driver for Samsung EXYNOS5250 >> SoC. >> >> +config ARM_EXYNOS5440_CPUFREQ >> + def_bool SOC_EXYNOS5440 >> + depends on HAVE_CLK && PM_OPP && OF >> + help >> + This adds the CPUFreq driver for Samsung EXYNOS5440 >> + SoC. The nature of exynos5440 clock controller is >> + different than previous exynos controllers so not using >> + the common exynos framework. >> + >> config ARM_KIRKWOOD_CPUFREQ >> def_bool ARCH_KIRKWOOD && OF >> help >> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile >> index 863fd18..c841438 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -52,6 +52,7 @@ obj-$(CONFIG_ARM_EXYNOS_CPUFREQ) += exynos-cpufreq.o >> obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ) += exynos4210-cpufreq.o >> obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ) += exynos4x12-cpufreq.o >> obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ) += exynos5250-cpufreq.o >> +obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ) += exynos5440-cpufreq.o >> obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o >> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o >> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o >> diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c >> new file mode 100644 >> index 0000000..2dc43b1 >> --- /dev/null >> +++ b/drivers/cpufreq/exynos5440-cpufreq.c >> @@ -0,0 +1,450 @@ >> +/* >> + * Copyright (c) 2013 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx> >> + * >> + * EXYNOS5440 - CPU frequency scaling support >> + * >> + * 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. >> +*/ > > Add following here to get better prints from the driver: > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt ok will add. > >> + >> +#include <linux/clk.h> >> +#include <linux/cpu.h> >> +#include <linux/cpufreq.h> >> +#include <linux/err.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> > > Probably include of.h instead of above two? No both are needed . actually these two .h are superset of of.h > >> +#include <linux/opp.h> >> +#include <linux/slab.h> >> + > >> +struct exynos_dvfs_data { >> + void __iomem *base; >> + struct resource *mem; >> + int irq; >> + struct clk *cpu_clk; >> + unsigned int cur_frequency; >> + unsigned int latency; >> + struct cpufreq_frequency_table *freq_table; >> + unsigned int freq_count; >> + struct device *dev; >> + struct work_struct irq_work; >> +}; >> + >> +static struct exynos_dvfs_data *dvfs_info; >> +static DEFINE_MUTEX(cpufreq_lock); >> +static struct cpufreq_freqs freqs; >> + >> +static int init_div_table(void) >> +{ >> + struct cpufreq_frequency_table *freq_tbl = dvfs_info->freq_table; >> + unsigned int tmp, clk_div, ema_div, freq, volt_id; >> + int i = 0; >> + struct opp *opp; >> + >> + for (i = 0; freq_tbl[i].frequency != CPUFREQ_TABLE_END; i++) { >> + >> + opp = opp_find_freq_exact(dvfs_info->dev, >> + freq_tbl[i].frequency * 1000, true); >> + if (IS_ERR(opp)) { >> + pr_err("failed to find valid OPP for %u KHZ\n", >> + freq_tbl[i].frequency); >> + return PTR_ERR(opp); >> + } >> + >> + freq = freq_tbl[i].frequency / 1000; /*In MHZ*/ >> + clk_div = ((freq / CPU_DIV_FREQ_MAX) & P0_7_CPUCLKDEV_MASK) >> + << P0_7_CPUCLKDEV_SHIFT; >> + clk_div |= ((freq / CPU_ATB_FREQ_MAX) & P0_7_ATBCLKDEV_MASK) >> + << P0_7_ATBCLKDEV_SHIFT; >> + clk_div |= ((freq / CPU_DBG_FREQ_MAX) & P0_7_CSCLKDEV_MASK) >> + << P0_7_CSCLKDEV_SHIFT; >> + >> + /*Calculate EMA*/ >> + volt_id = opp_get_voltage(opp); >> + volt_id = (MAX_VOLTAGE - volt_id) / VOLTAGE_STEP; >> + if (volt_id < PMIC_HIGH_VOLT) { >> + ema_div = (CPUEMA_HIGH << P0_7_CPUEMA_SHIFT) | >> + (L2EMA_HIGH << P0_7_L2EMA_SHIFT); >> + } else if (volt_id > PMIC_LOW_VOLT) { >> + ema_div = (CPUEMA_LOW << P0_7_CPUEMA_SHIFT) | >> + (L2EMA_LOW << P0_7_L2EMA_SHIFT); >> + } else { >> + ema_div = (CPUEMA_MID << P0_7_CPUEMA_SHIFT) | >> + (L2EMA_MID << P0_7_L2EMA_SHIFT); >> + } >> + >> + tmp = (clk_div | ema_div | (volt_id << P0_7_VDD_SHIFT) >> + | ((freq / FREQ_UNIT) << P0_7_FREQ_SHIFT)); >> + >> + __raw_writel(tmp, dvfs_info->base + XMU_PMU_P0_7 + 4 * i); >> + } >> + >> + return 0; >> +} >> + >> +void exynos_enable_dvfs(void) >> +{ >> + unsigned int tmp, i, cpu; >> + struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table; >> + /*Disable DVFS*/ > > Please give space after /* and before */ all over your driver. ok. > >> + __raw_writel(0, dvfs_info->base + XMU_DVFS_CTRL); >> + >> + /*Enable PSTATE Change Event*/ >> + tmp = __raw_readl(dvfs_info->base + XMU_PMUEVTEN); >> + tmp |= (1 << PSTATE_CHANGED_EVTEN_SHIFT); >> + __raw_writel(tmp, dvfs_info->base + XMU_PMUEVTEN); >> + >> + /Enable* PSTATE Change IRQ*/ >> + tmp = __raw_readl(dvfs_info->base + XMU_PMUIRQEN); >> + tmp |= (1 << PSTATE_CHANGED_IRQEN_SHIFT); >> + __raw_writel(tmp, dvfs_info->base + XMU_PMUIRQEN); >> + >> + /*Set initial performance index*/ >> + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) >> + if (freq_table[i].frequency == dvfs_info->cur_frequency) >> + break; >> + >> + if (freq_table[i].frequency == CPUFREQ_TABLE_END) { >> + pr_crit("Boot up frequency not supported\n"); >> + /*Assign the highest frequency*/ >> + i = 0; >> + dvfs_info->cur_frequency = freq_table[i].frequency; >> + } >> + >> + pr_info("Setting dvfs initial frequency = %u", >> + dvfs_info->cur_frequency); >> + >> + for (cpu = 0; cpu < CONFIG_NR_CPUS; cpu++) { >> + tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4); >> + tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT); >> + tmp |= (i << C0_3_PSTATE_NEW_SHIFT); >> + __raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4); >> + } >> + >> + /*Enable DVFS*/ >> + __raw_writel(1 << XMU_DVFS_CTRL_EN_SHIFT, >> + dvfs_info->base + XMU_DVFS_CTRL); >> +} >> + >> +static int exynos_verify_speed(struct cpufreq_policy *policy) >> +{ >> + return cpufreq_frequency_table_verify(policy, >> + dvfs_info->freq_table); >> +} >> + >> +static unsigned int exynos_getspeed(unsigned int cpu) >> +{ >> + return dvfs_info->cur_frequency; >> +} >> + >> +static int exynos_target(struct cpufreq_policy *policy, >> + unsigned int target_freq, >> + unsigned int relation) >> +{ >> + unsigned int index, tmp; >> + int ret = 0, i; >> + struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table; >> + >> + mutex_lock(&cpufreq_lock); >> + freqs.old = dvfs_info->cur_frequency; >> + >> + if (cpufreq_frequency_table_target(policy, freq_table, >> + target_freq, relation, &index)) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + freqs.new = freq_table[index].frequency; >> + freqs.cpu = policy->cpu; >> + >> + for_each_cpu(freqs.cpu, policy->cpus) >> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE); >> + >> + /*Set the target frequency in all C0_3_PSTATE register*/ >> + for_each_cpu(i, policy->cpus) { >> + tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + i * 4); >> + tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT); >> + tmp |= (index << C0_3_PSTATE_NEW_SHIFT); >> + >> + __raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4); >> + } >> +out: >> + mutex_unlock(&cpufreq_lock); >> + return ret; >> +} >> + >> +static void exynos_cpufreq_work(struct work_struct *work) >> +{ >> + unsigned int cur_pstate, index; >> + struct cpufreq_policy *policy = cpufreq_cpu_get(0); /* boot CPU */ >> + struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table; >> + >> + if (policy == NULL) >> + goto skip_work; > > How can that be true? And if it can be, you will miss POSTCHANGE notifications > too. This skip of POSTCHANGE notifications is needed in one case where bootup frequency does not match with any elements of cpufreq table and hence I simply assign the highest frequency. Also cpufreq driver is not registered at this point. > >> + mutex_lock(&cpufreq_lock); >> + freqs.old = dvfs_info->cur_frequency; >> + >> + cur_pstate = __raw_readl(dvfs_info->base + XMU_P_STATUS); >> + if (cur_pstate >> C0_3_PSTATE_VALID_SHIFT & 0x1) >> + index = (cur_pstate >> C0_3_PSTATE_CURR_SHIFT) & P_VALUE_MASK; >> + else >> + index = (cur_pstate >> C0_3_PSTATE_NEW_SHIFT) & P_VALUE_MASK; >> + >> + if (index < dvfs_info->freq_count) { > > What if index is greater than count? That's a problem, isn't it? yes will check this. > >> + freqs.new = freq_table[index].frequency; >> + for_each_cpu(freqs.cpu, policy->cpus) >> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE); >> + dvfs_info->cur_frequency = freqs.new; >> + } >> + >> + cpufreq_cpu_put(policy); >> + mutex_unlock(&cpufreq_lock); >> +skip_work: >> + enable_irq(dvfs_info->irq); >> +} >> + >> +static irqreturn_t exynos_cpufreq_irq(int irq, void *id) >> +{ >> + unsigned int tmp; >> + >> + tmp = __raw_readl(dvfs_info->base + XMU_PMUIRQ); >> + if (tmp >> PSTATE_CHANGED_SHIFT & 0x1) { >> + __raw_writel(tmp, dvfs_info->base + XMU_PMUIRQ); >> + disable_irq_nosync(irq); >> + schedule_work(&dvfs_info->irq_work); >> + } >> + return IRQ_HANDLED; >> +} >> + >> +static void exynos_sort_descend_freq_table(void) >> +{ >> + struct cpufreq_frequency_table *freq_tbl = dvfs_info->freq_table; >> + int i = 0, index; >> + unsigned int tmp_freq; >> + >> + /* >> + *Freq table is already in ascending order as it is created from > > You need space after * here too :) ok. > >> + *OPP library, so just swap the elements to make it descending. >> + */ >> + for (i = 0; i < dvfs_info->freq_count / 2; i++) { >> + index = dvfs_info->freq_count - i - 1; >> + tmp_freq = freq_tbl[i].frequency; >> + freq_tbl[i].frequency = freq_tbl[index].frequency; >> + freq_tbl[index].frequency = tmp_freq; >> + } >> +} >> + >> +static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy) >> +{ >> + >> + policy->cur = dvfs_info->cur_frequency; >> + cpufreq_frequency_table_get_attr(dvfs_info->freq_table, policy->cpu); >> + >> + /* set the transition latency value */ >> + policy->cpuinfo.transition_latency = dvfs_info->latency; >> + >> + cpumask_setall(policy->cpus); >> + >> + return cpufreq_frequency_table_cpuinfo(policy, dvfs_info->freq_table); >> +} >> + >> +static struct cpufreq_driver exynos_driver = { >> + .flags = CPUFREQ_STICKY, >> + .verify = exynos_verify_speed, >> + .target = exynos_target, >> + .get = exynos_getspeed, >> + .init = exynos_cpufreq_cpu_init, >> + .name = DRIVER_NAME, >> +}; >> + >> +static int __init exynos_cpufreq_init(void) > > probably calling it probe might make more sense? Yes I just checked the recent changes in cpu0-cpufreq driver. probe makes more sense. > >> +{ >> + int ret = -EINVAL; >> + struct device_node *np; >> + >> + np = of_find_compatible_node(NULL, NULL, "samsung,exynos5440-cpufreq"); >> + if (IS_ERR(np)) > > !np ?? ok. > >> + return -ENODEV; >> + >> + dvfs_info = kzalloc(sizeof(struct exynos_dvfs_data), GFP_KERNEL); > > sizeof(*dvfs_info) ?? Isn't both same? > >> + if (IS_ERR(dvfs_info)) { > > IS_ERR_OR_NULL? But i believe kzalloc only returns NULL to show error, so > you may end up using if(!dvfs_info) yes right. > >> + ret = -ENOMEM; >> + goto err_mem_data; >> + } >> + >> + dvfs_info->base = of_iomap(np, 0); >> + if (IS_ERR(dvfs_info->base)) { > > same here too.. ok > >> + pr_err("No cpufreq memory map found\n"); >> + ret = -ENODEV; >> + goto err_map; >> + } >> + >> + dvfs_info->irq = irq_of_parse_and_map(np, 0); >> + > > can remove blank line. > >> + if (dvfs_info->irq == 0) { >> + pr_err("No cpufreq irq found\n"); >> + ret = -ENODEV; >> + goto err_irq_parse; >> + } >> + >> + dvfs_info->dev = get_cpu_device(0); > > What about using this dev instance to call devm_*** routines everywhere in init. ok, with probe changes all devm_ api's can be used. > >> + if (IS_ERR(dvfs_info->dev)) { >> + pr_err("failed to get cpu0 device\n"); >> + ret = -ENODEV; >> + goto err_irq_parse; >> + } >> + >> + dvfs_info->dev->of_node = np; > > So you are setting cpufreq node as node of cpu, I don't know if this is a > good idea. In that case it would be better to create the cpu node instead > and put the table into it. yes right. > >> + ret = of_init_opp_table(dvfs_info->dev); >> + if (ret) { >> + pr_err("failed to init OPP table: %d\n", ret); >> + goto err_irq_parse; >> + } >> + >> + ret = opp_init_cpufreq_table(dvfs_info->dev, &dvfs_info->freq_table); >> + if (ret) { >> + pr_err("failed to init cpufreq table: %d\n", ret); >> + goto err_irq_parse; >> + } >> + dvfs_info->freq_count = opp_get_opp_count(dvfs_info->dev); >> + exynos_sort_descend_freq_table(); >> + >> + if (of_property_read_u32(np, "clock-latency", &dvfs_info->latency)) >> + dvfs_info->latency = DEF_TRANS_LATENCY; >> + >> + dvfs_info->cpu_clk = of_clk_get(np, 0); >> + if (IS_ERR(dvfs_info->cpu_clk)) { >> + pr_err("Failed to get cpu clock\n"); >> + ret = PTR_ERR(dvfs_info->cpu_clk); >> + goto err_clk_get; >> + } >> + >> + dvfs_info->cur_frequency = clk_get_rate(dvfs_info->cpu_clk); >> + if (!dvfs_info->cur_frequency) { >> + pr_err("Failed to get clock rate\n"); >> + ret = -EINVAL; >> + goto err_clk_get_rate; >> + } >> + dvfs_info->cur_frequency /= 1000; >> + >> + INIT_WORK(&dvfs_info->irq_work, exynos_cpufreq_work); >> + if (request_irq(dvfs_info->irq, exynos_cpufreq_irq, >> + IRQF_TRIGGER_NONE, DRIVER_NAME, dvfs_info)) { >> + pr_err("Failed to register IRQ\n"); >> + ret = -ENODEV; >> + goto err_clk_get_rate; >> + } >> + >> + ret = init_div_table(); >> + if (ret) { >> + pr_err("Failed to initialise div table\n"); > > s/initialise/initialize ok. Thanks, Amit D > >> + goto err_div_table; >> + } >> + >> + exynos_enable_dvfs(); >> + ret = cpufreq_register_driver(&exynos_driver); >> + if (ret) { >> + pr_err("%s: failed to register cpufreq driver\n", __func__); >> + goto err_div_table; >> + } >> + >> + of_node_put(np); >> + >> + pr_info("exynos5440 DVFS initialised.\n"); >> + >> + return 0; >> + >> +err_div_table: >> + free_irq(dvfs_info->irq, dvfs_info); >> +err_clk_get_rate: >> + clk_put(dvfs_info->cpu_clk); >> +err_clk_get: >> + opp_free_cpufreq_table(dvfs_info->dev, &dvfs_info->freq_table); >> +err_irq_parse: >> + iounmap(dvfs_info->base); >> +err_map: >> + kfree(dvfs_info); >> +err_mem_data: >> + of_node_put(np); >> + pr_err("%s: failed initialization\n", __func__); >> + return ret; >> +} >> +late_initcall(exynos_cpufreq_init); >> -- >> 1.7.1 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
