On Thursday, September 06, 2012, Shawn Guo wrote:
> On Wed, Sep 05, 2012 at 10:18:50PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, September 05, 2012, Shawn Guo wrote:
> > > On Wed, Sep 05, 2012 at 03:38:12PM +0200, Rafael J. Wysocki wrote:
> > > > Well, what about actually avoiding the code duplication? That is,
> > > > can you make OMAP be a user of your new core function and drop the
> > > > OMAP-specific one, perhaps?
> > > >
> > > I would expect that the driver will replace the omap-cpufreq driver
> > > completely at some point, where omap becomes a DT only platform.
> >
> > That's fine, but why do we need two almost identical functions to start with?
> >
> Probably because it does not worth the churn considering that any
> particular cpufreq driver having an identical .set_target as this
> generic one should eventually be replaced by this driver completely,
> IMO.
>
> But anyway, it could be another patch which may need more discussion,
> so I just submitted the v4 with leaving this particular comment to be
> addressed in another patch.
It may be a different patch, I'm only concerned about the final outcome
(i.e. no added code duplication, please).
> I'm unsure this is what you are ordering. But here is what I come up
> with to address your comment. Please let me know if this is exactly
> what you are asking for, or you expect something different.
Yes, it is.
Thanks,
Rafael
> 8<---
> drivers/cpufreq/Kconfig | 4 ++
> drivers/cpufreq/Kconfig.arm | 1 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-cpu0.c | 94 +++++-----------------------------
> drivers/cpufreq/cpufreq.h | 31 +++++++++++
> drivers/cpufreq/cpufreq_target.c | 99 +++++++++++++++++++++++++++++++++++
> drivers/cpufreq/omap-cpufreq.c | 105 +++++---------------------------------
> 7 files changed, 163 insertions(+), 172 deletions(-)
> create mode 100644 drivers/cpufreq/cpufreq.h
> create mode 100644 drivers/cpufreq/cpufreq_target.c
>
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index ea512f4..206eec9 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -20,6 +20,9 @@ if CPU_FREQ
> config CPU_FREQ_TABLE
> tristate
>
> +config CPU_FREQ_TARGET
> + tristate
> +
> config CPU_FREQ_STAT
> tristate "CPU frequency translation statistics"
> select CPU_FREQ_TABLE
> @@ -183,6 +186,7 @@ config GENERIC_CPUFREQ_CPU0
> bool "Generic CPU0 cpufreq driver"
> depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> select CPU_FREQ_TABLE
> + select CPU_FREQ_TARGET
> help
> This adds a generic cpufreq driver for CPU0 frequency management.
> It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 5961e64..60d81d0 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -7,6 +7,7 @@ config ARM_OMAP2PLUS_CPUFREQ
> depends on ARCH_OMAP2PLUS
> default ARCH_OMAP2PLUS
> select CPU_FREQ_TABLE
> + select CPU_FREQ_TARGET
>
> config ARM_S3C2416_CPUFREQ
> bool "S3C2416 CPU Frequency scaling support"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index a378ed2..f7d19d1 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o
>
> # CPUfreq cross-arch helpers
> obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
> +obj-$(CONFIG_CPU_FREQ_TARGET) += cpufreq_target.o
>
> obj-$(CONFIG_GENERIC_CPUFREQ_CPU0) += cpufreq-cpu0.o
>
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> index e915827..51096e8 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -1,9 +1,6 @@
> /*
> * Copyright (C) 2012 Freescale Semiconductor, Inc.
> *
> - * The OPP code in function cpu0_set_target() is reused from
> - * drivers/cpufreq/omap-cpufreq.c
> - *
> * 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.
> @@ -20,6 +17,7 @@
> #include <linux/opp.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> +#include "cpufreq.h"
>
> static unsigned int transition_latency;
> static unsigned int voltage_tolerance; /* in percentage */
> @@ -42,84 +40,18 @@ static unsigned int cpu0_get_speed(unsigned int cpu)
> static int cpu0_set_target(struct cpufreq_policy *policy,
> unsigned int target_freq, unsigned int relation)
> {
> - struct cpufreq_freqs freqs;
> - struct opp *opp;
> - unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> - unsigned int index, cpu;
> - int ret;
> -
> - ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> - relation, &index);
> - if (ret) {
> - pr_err("failed to match target freqency %d: %d\n",
> - target_freq, ret);
> - return ret;
> - }
> -
> - freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> - if (freq_Hz < 0)
> - freq_Hz = freq_table[index].frequency * 1000;
> - freqs.new = freq_Hz / 1000;
> - freqs.old = clk_get_rate(cpu_clk) / 1000;
> -
> - if (freqs.old == freqs.new)
> - return 0;
> -
> - for_each_online_cpu(cpu) {
> - freqs.cpu = cpu;
> - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> - }
> -
> - if (cpu_reg) {
> - opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> - if (IS_ERR(opp)) {
> - pr_err("failed to find OPP for %ld\n", freq_Hz);
> - return PTR_ERR(opp);
> - }
> - volt = opp_get_voltage(opp);
> - tol = volt * voltage_tolerance / 100;
> - volt_old = regulator_get_voltage(cpu_reg);
> - }
> -
> - pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> - freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> - freqs.new / 1000, volt ? volt / 1000 : -1);
> -
> - /* scaling up? scale voltage before frequency */
> - if (cpu_reg && freqs.new > freqs.old) {
> - ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
> - if (ret) {
> - pr_err("failed to scale voltage up: %d\n", ret);
> - freqs.new = freqs.old;
> - return ret;
> - }
> - }
> -
> - ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> - if (ret) {
> - pr_err("failed to set clock rate: %d\n", ret);
> - if (cpu_reg)
> - regulator_set_voltage_tol(cpu_reg, volt_old, tol);
> - return ret;
> - }
> -
> - /* scaling down? scale voltage after frequency */
> - if (cpu_reg && freqs.new < freqs.old) {
> - ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
> - if (ret) {
> - pr_err("failed to scale voltage down: %d\n", ret);
> - clk_set_rate(cpu_clk, freqs.old * 1000);
> - freqs.new = freqs.old;
> - return ret;
> - }
> - }
> -
> - for_each_online_cpu(cpu) {
> - freqs.cpu = cpu;
> - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> - }
> -
> - return 0;
> + struct cpufreq_target_data data;
> +
> + data.dev = cpu_dev;
> + data.clk = cpu_clk;
> + data.reg = cpu_reg;
> + data.tol = voltage_tolerance;
> + data.freq_table = freq_table;
> + data.policy = policy;
> + data.target_freq = target_freq;
> + data.relation = relation;
> +
> + return cpufreq_set_target(&data);
> }
>
> static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> diff --git a/drivers/cpufreq/cpufreq.h b/drivers/cpufreq/cpufreq.h
> new file mode 100644
> index 0000000..ae380b3
> --- /dev/null
> +++ b/drivers/cpufreq/cpufreq.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * 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.
> + */
> +
> +#ifndef _DRIVERS_CPUFREQ_H
> +#define _DRIVERS_CPUFREQ_H
> +
> +struct device;
> +struct clk;
> +struct regulator;
> +struct cpufreq_frequency_table;
> +struct cpufreq_policy;
> +
> +struct cpufreq_target_data {
> + struct device *dev;
> + struct clk *clk;
> + struct regulator *reg;
> + unsigned int tol;
> + struct cpufreq_frequency_table *freq_table;
> + struct cpufreq_policy *policy;
> + unsigned int target_freq;
> + unsigned int relation;
> +};
> +
> +int cpufreq_set_target(struct cpufreq_target_data *data);
> +
> +#endif /* _DRIVERS_CPUFREQ_H */
> diff --git a/drivers/cpufreq/cpufreq_target.c b/drivers/cpufreq/cpufreq_target.c
> new file mode 100644
> index 0000000..02a5584
> --- /dev/null
> +++ b/drivers/cpufreq/cpufreq_target.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Based on function omap_target() from drivers/cpufreq/omap-cpufreq.c
> + *
> + * 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.
> + */
> +#include <linux/clk.h>
> +#include <linux/cpufreq.h>
> +#include <linux/module.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include "cpufreq.h"
> +
> +int cpufreq_set_target(struct cpufreq_target_data *d)
> +{
> + struct cpufreq_freqs freqs;
> + struct opp *opp;
> + unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> + unsigned int index, cpu;
> + int ret;
> +
> + ret = cpufreq_frequency_table_target(d->policy, d->freq_table,
> + d->target_freq, d->relation,
> + &index);
> + if (ret) {
> + pr_err("failed to match target freqency %d: %d\n",
> + d->target_freq, ret);
> + return ret;
> + }
> +
> + freq_Hz = clk_round_rate(d->clk, d->freq_table[index].frequency * 1000);
> + if (freq_Hz < 0)
> + freq_Hz = d->freq_table[index].frequency * 1000;
> + freqs.new = freq_Hz / 1000;
> + freqs.old = clk_get_rate(d->clk) / 1000;
> +
> + if (freqs.old == freqs.new)
> + return 0;
> +
> + for_each_online_cpu(cpu) {
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> + }
> +
> + if (d->reg) {
> + opp = opp_find_freq_ceil(d->dev, &freq_Hz);
> + if (IS_ERR(opp)) {
> + pr_err("failed to find OPP for %ld\n", freq_Hz);
> + return PTR_ERR(opp);
> + }
> + volt = opp_get_voltage(opp);
> + tol = volt * d->tol / 100;
> + volt_old = regulator_get_voltage(d->reg);
> + }
> +
> + pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> + freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> + freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> + /* scaling up? scale voltage before frequency */
> + if (d->reg && freqs.new > freqs.old) {
> + ret = regulator_set_voltage_tol(d->reg, volt, tol);
> + if (ret) {
> + pr_err("failed to scale voltage up: %d\n", ret);
> + freqs.new = freqs.old;
> + return ret;
> + }
> + }
> +
> + ret = clk_set_rate(d->clk, freqs.new * 1000);
> + if (ret) {
> + pr_err("failed to set clock rate: %d\n", ret);
> + if (d->reg)
> + regulator_set_voltage_tol(d->reg, volt_old, tol);
> + return ret;
> + }
> +
> + /* scaling down? scale voltage after frequency */
> + if (d->reg && freqs.new < freqs.old) {
> + ret = regulator_set_voltage_tol(d->reg, volt, tol);
> + if (ret) {
> + pr_err("failed to scale voltage down: %d\n", ret);
> + clk_set_rate(d->clk, freqs.old * 1000);
> + freqs.new = freqs.old;
> + return ret;
> + }
> + }
> +
> + for_each_online_cpu(cpu) {
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_set_target);
> diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
> index 83a78ad..0772df5 100644
> --- a/drivers/cpufreq/omap-cpufreq.c
> +++ b/drivers/cpufreq/omap-cpufreq.c
> @@ -37,6 +37,8 @@
>
> #include <mach/hardware.h>
>
> +#include "cpufreq.h"
> +
> /* OPP tolerance in percentage */
> #define OPP_TOLERANCE 4
>
> @@ -69,97 +71,18 @@ static int omap_target(struct cpufreq_policy *policy,
> unsigned int target_freq,
> unsigned int relation)
> {
> - unsigned int i;
> - int r, ret = 0;
> - struct cpufreq_freqs freqs;
> - struct opp *opp;
> - unsigned long freq, volt = 0, volt_old = 0, tol = 0;
> -
> - if (!freq_table) {
> - dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
> - policy->cpu);
> - return -EINVAL;
> - }
> -
> - ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> - relation, &i);
> - if (ret) {
> - dev_dbg(mpu_dev, "%s: cpu%d: no freq match for %d(ret=%d)\n",
> - __func__, policy->cpu, target_freq, ret);
> - return ret;
> - }
> - freqs.new = freq_table[i].frequency;
> - if (!freqs.new) {
> - dev_err(mpu_dev, "%s: cpu%d: no match for freq %d\n", __func__,
> - policy->cpu, target_freq);
> - return -EINVAL;
> - }
> -
> - freqs.old = omap_getspeed(policy->cpu);
> - freqs.cpu = policy->cpu;
> -
> - if (freqs.old == freqs.new && policy->cur == freqs.new)
> - return ret;
> -
> - /* notifiers */
> - for_each_cpu(i, policy->cpus) {
> - freqs.cpu = i;
> - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> - }
> -
> - freq = freqs.new * 1000;
> -
> - if (mpu_reg) {
> - opp = opp_find_freq_ceil(mpu_dev, &freq);
> - if (IS_ERR(opp)) {
> - dev_err(mpu_dev, "%s: unable to find MPU OPP for %d\n",
> - __func__, freqs.new);
> - return -EINVAL;
> - }
> - volt = opp_get_voltage(opp);
> - tol = volt * OPP_TOLERANCE / 100;
> - volt_old = regulator_get_voltage(mpu_reg);
> - }
> -
> - dev_dbg(mpu_dev, "cpufreq-omap: %u MHz, %ld mV --> %u MHz, %ld mV\n",
> - freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> - freqs.new / 1000, volt ? volt / 1000 : -1);
> -
> - /* scaling up? scale voltage before frequency */
> - if (mpu_reg && (freqs.new > freqs.old)) {
> - r = regulator_set_voltage(mpu_reg, volt - tol, volt + tol);
> - if (r < 0) {
> - dev_warn(mpu_dev, "%s: unable to scale voltage up.\n",
> - __func__);
> - freqs.new = freqs.old;
> - goto done;
> - }
> - }
> -
> - ret = clk_set_rate(mpu_clk, freqs.new * 1000);
> -
> - /* scaling down? scale voltage after frequency */
> - if (mpu_reg && (freqs.new < freqs.old)) {
> - r = regulator_set_voltage(mpu_reg, volt - tol, volt + tol);
> - if (r < 0) {
> - dev_warn(mpu_dev, "%s: unable to scale voltage down.\n",
> - __func__);
> - ret = clk_set_rate(mpu_clk, freqs.old * 1000);
> - freqs.new = freqs.old;
> - goto done;
> - }
> - }
> -
> - freqs.new = omap_getspeed(policy->cpu);
> -
> -done:
> - /* notifiers */
> - for_each_cpu(i, policy->cpus) {
> - freqs.cpu = i;
> - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> - }
> -
> - return ret;
> + struct cpufreq_target_data data;
> +
> + data.dev = mpu_dev;
> + data.clk = mpu_clk;
> + data.reg = mpu_reg;
> + data.tol = OPP_TOLERANCE;
> + data.freq_table = freq_table;
> + data.policy = policy;
> + data.target_freq = target_freq;
> + data.relation = relation;
> +
> + return cpufreq_set_target(&data);
> }
>
> static inline void freq_table_free(void)
>
--
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
