Re: [PATCH v3 3/3] cpufreq: Add a generic cpufreq-cpu0 driver

["Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>]

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)
>