[PATCH 3/4] ARM: imx: add VDDSOC/PU setpoint info into dts

[PATCH 3/4] ARM: imx: add VDDSOC/PU setpoint info into dts

[Anson Huang]

i.MX6Q needs to update VDDARM, VDDSOC/PU regulator when CPUFreq
is changed, each setpoint has different voltage, so we need to
pass VDDARM, VDDSOC/PU's freq-voltage info from dts.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 arch/arm/boot/dts/imx6q.dtsi |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index e7e8332..03da628 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -30,6 +30,13 @@
 				792000  1150000
 				396000  975000
 			>;
+			fsl,soc-operating-points = <
+			/* ARM kHz  SOC-PU uV */
+				1200000	1275000
+				996000	1250000
+				792000	1175000
+				396000	1175000
+			>;
 			clock-latency = <61036>; /* two CLK32 periods */
 			clocks = <&clks 104>, <&clks 6>, <&clks 16>,
 				 <&clks 17>, <&clks 170>;
-- 
1.7.9.5

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Anson Huang]

on i.MX6Q, cpu freq change need to follow below flows:

1. each setpoint has different VDDARM, VDDSOC/PU voltage, get the setpoint
table from dts;
2. when cpu freq is scaling up, need to increase VDDSOC/PU voltage before
VDDARM, if VDDPU is off, no need to change it;
3. when cpu freq is scaling down, need to decrease VDDARM voltage before
VDDSOC/PU, if VDDPU is off, no need to change it;

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 drivers/cpufreq/imx6q-cpufreq.c |  161 ++++++++++++++++++++++++++++++---------
 1 file changed, 126 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 4b3f18e..5fb302e 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -17,10 +17,6 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 
-#define PU_SOC_VOLTAGE_NORMAL	1250000
-#define PU_SOC_VOLTAGE_HIGH	1275000
-#define FREQ_1P2_GHZ		1200000000
-
 static struct regulator *arm_reg;
 static struct regulator *pu_reg;
 static struct regulator *soc_reg;
@@ -35,6 +31,14 @@ static struct device *cpu_dev;
 static struct cpufreq_frequency_table *freq_table;
 static unsigned int transition_latency;
 
+struct soc_opp {
+	u32 arm_freq;
+	u32 soc_volt;
+};
+
+static struct soc_opp *imx6_soc_opp;
+static u32 soc_opp_count;
+
 static unsigned int imx6q_get_speed(unsigned int cpu)
 {
 	return clk_get_rate(arm_clk) / 1000;
@@ -45,6 +49,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	struct dev_pm_opp *opp;
 	unsigned long freq_hz, volt, volt_old;
 	unsigned int old_freq, new_freq;
+	unsigned int soc_opp_index = 0;
 	int ret;
 
 	new_freq = freq_table[index].frequency;
@@ -63,29 +68,48 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 	rcu_read_unlock();
 	volt_old = regulator_get_voltage(arm_reg);
 
-	dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
+	/* Find the matching VDDSOC/VDDPU operating voltage */
+	while (soc_opp_index < soc_opp_count) {
+		if (new_freq == imx6_soc_opp[soc_opp_index].arm_freq)
+			break;
+		soc_opp_index++;
+	}
+	if (soc_opp_index >= soc_opp_count) {
+		dev_err(cpu_dev,
+			"Can NOT find matching imx6_soc_opp voltage!\n");
+			return -EINVAL;
+	}
+
+	dev_dbg(cpu_dev, "%u MHz, arm %ld mV, soc-pu %d mV --> %u MHz, arm %ld mV, soc-pu %d mV\n",
 		old_freq / 1000, volt_old / 1000,
-		new_freq / 1000, volt / 1000);
+		imx6_soc_opp[soc_opp_index].soc_volt / 1000,
+		new_freq / 1000, volt / 1000,
+		imx6_soc_opp[soc_opp_index].soc_volt / 1000);
 
 	/* scaling up?  scale voltage before frequency */
 	if (new_freq > old_freq) {
+		if (regulator_is_enabled(pu_reg)) {
+			ret = regulator_set_voltage_tol(pu_reg,
+				imx6_soc_opp[soc_opp_index].soc_volt, 0);
+			if (ret) {
+				dev_err(cpu_dev,
+					"failed to scale vddpu up: %d\n", ret);
+				return ret;
+			}
+		}
+		ret = regulator_set_voltage_tol(soc_reg,
+				imx6_soc_opp[soc_opp_index].soc_volt, 0);
+		if (ret) {
+			dev_err(cpu_dev,
+				"failed to scale vddsoc up: %d\n", ret);
+			return ret;
+		}
 		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
 		if (ret) {
 			dev_err(cpu_dev,
 				"failed to scale vddarm up: %d\n", ret);
 			return ret;
 		}
-
-		/*
-		 * Need to increase vddpu and vddsoc for safety
-		 * if we are about to run at 1.2 GHz.
-		 */
-		if (new_freq == FREQ_1P2_GHZ / 1000) {
-			regulator_set_voltage_tol(pu_reg,
-					PU_SOC_VOLTAGE_HIGH, 0);
-			regulator_set_voltage_tol(soc_reg,
-					PU_SOC_VOLTAGE_HIGH, 0);
-		}
 	}
 
 	/*
@@ -120,12 +144,22 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 				 "failed to scale vddarm down: %d\n", ret);
 			ret = 0;
 		}
-
-		if (old_freq == FREQ_1P2_GHZ / 1000) {
-			regulator_set_voltage_tol(pu_reg,
-					PU_SOC_VOLTAGE_NORMAL, 0);
-			regulator_set_voltage_tol(soc_reg,
-					PU_SOC_VOLTAGE_NORMAL, 0);
+		ret = regulator_set_voltage_tol(soc_reg,
+				imx6_soc_opp[soc_opp_index].soc_volt, 0);
+		if (ret) {
+			dev_warn(cpu_dev,
+				"failed to scale vddsoc down: %d\n", ret);
+			ret = 0;
+		}
+		if (regulator_is_enabled(pu_reg)) {
+			ret = regulator_set_voltage_tol(pu_reg,
+				imx6_soc_opp[soc_opp_index].soc_volt, 0);
+			if (ret) {
+				dev_warn(cpu_dev,
+					"failed to scale vddpu down: %d\n",
+					ret);
+				ret = 0;
+			}
 		}
 	}
 
@@ -153,6 +187,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	struct dev_pm_opp *opp;
 	unsigned long min_volt, max_volt;
 	int num, ret;
+	const struct property *prop;
+	const __be32 *val;
+	u32 nr, i;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev) {
@@ -201,9 +238,75 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		goto put_node;
 	}
 
+	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
+	if (!prop) {
+		dev_err(cpu_dev,
+			"fsl,soc-operating-points node not found!\n");
+		goto free_freq_table;
+	}
+	if (!prop->value) {
+		dev_err(cpu_dev,
+			"No entries in fsl-soc-operating-points node!\n");
+		goto free_freq_table;
+	}
+
+	/*
+	 * Each OPP is a set of tuples consisting of frequency and
+	 * voltage like <freq-kHz vol-uV>.
+	 */
+	nr = prop->length / sizeof(u32);
+	if (nr % 2) {
+		dev_err(cpu_dev, "Invalid fsl-soc-operating-points list!\n");
+		goto free_freq_table;
+	}
+
+	/* Get the VDDSOC/VDDPU voltages that need to track the CPU voltages. */
+	imx6_soc_opp = devm_kzalloc(cpu_dev,
+		sizeof(struct soc_opp) * (nr / 2), GFP_KERNEL);
+
+	if (imx6_soc_opp == NULL) {
+		dev_err(cpu_dev, "No Memory for VDDSOC/PU table!\n");
+		goto free_freq_table;
+	}
+
+	rcu_read_lock();
+	val = prop->value;
+
+	min_volt = max_volt = 0;
+	for (i = 0; i < nr / 2; i++) {
+		unsigned long freq = be32_to_cpup(val++);
+		unsigned long volt = be32_to_cpup(val++);
+
+		if (i == 0)
+			min_volt = max_volt = volt;
+		if (volt < min_volt)
+			min_volt = volt;
+		if (volt > max_volt)
+			max_volt = volt;
+		opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
+		imx6_soc_opp[i].arm_freq = freq;
+		imx6_soc_opp[i].soc_volt = volt;
+		soc_opp_count++;
+	}
+	rcu_read_unlock();
+
 	if (of_property_read_u32(np, "clock-latency", &transition_latency))
 		transition_latency = CPUFREQ_ETERNAL;
 
+	if (min_volt * max_volt != 0) {
+		/*
+		 * Calculate the ramp time for max voltage change in the
+		 * VDDSOC and VDDPU regulators.
+		 */
+		ret = regulator_set_voltage_time(soc_reg, min_volt, max_volt);
+		if (ret > 0)
+			transition_latency += ret * 1000;
+
+		ret = regulator_set_voltage_time(pu_reg, min_volt, max_volt);
+		if (ret > 0)
+			transition_latency += ret * 1000;
+	}
+
 	/*
 	 * OPP is maintained in order of increasing frequency, and
 	 * freq_table initialised from OPP is therefore sorted in the
@@ -221,18 +324,6 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 	if (ret > 0)
 		transition_latency += ret * 1000;
 
-	/* Count vddpu and vddsoc latency in for 1.2 GHz support */
-	if (freq_table[num].frequency == FREQ_1P2_GHZ / 1000) {
-		ret = regulator_set_voltage_time(pu_reg, PU_SOC_VOLTAGE_NORMAL,
-						 PU_SOC_VOLTAGE_HIGH);
-		if (ret > 0)
-			transition_latency += ret * 1000;
-		ret = regulator_set_voltage_time(soc_reg, PU_SOC_VOLTAGE_NORMAL,
-						 PU_SOC_VOLTAGE_HIGH);
-		if (ret > 0)
-			transition_latency += ret * 1000;
-	}
-
 	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
 	if (ret) {
 		dev_err(cpu_dev, "failed register driver: %d\n", ret);
-- 
1.7.9.5

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Mon, Dec 16, 2013 at 04:14:10PM -0500, Anson Huang wrote:
> on i.MX6Q, cpu freq change need to follow below flows:
> 
> 1. each setpoint has different VDDARM, VDDSOC/PU voltage, get the setpoint
> table from dts;
> 2. when cpu freq is scaling up, need to increase VDDSOC/PU voltage before
> VDDARM, if VDDPU is off, no need to change it;
> 3. when cpu freq is scaling down, need to decrease VDDARM voltage before
> VDDSOC/PU, if VDDPU is off, no need to change it;
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c |  161 ++++++++++++++++++++++++++++++---------
>  1 file changed, 126 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 4b3f18e..5fb302e 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -17,10 +17,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
> -#define PU_SOC_VOLTAGE_NORMAL	1250000
> -#define PU_SOC_VOLTAGE_HIGH	1275000
> -#define FREQ_1P2_GHZ		1200000000
> -
>  static struct regulator *arm_reg;
>  static struct regulator *pu_reg;
>  static struct regulator *soc_reg;
> @@ -35,6 +31,14 @@ static struct device *cpu_dev;
>  static struct cpufreq_frequency_table *freq_table;
>  static unsigned int transition_latency;
>  
> +struct soc_opp {
> +	u32 arm_freq;
> +	u32 soc_volt;
> +};
> +
> +static struct soc_opp *imx6_soc_opp;
> +static u32 soc_opp_count;
> +
>  static unsigned int imx6q_get_speed(unsigned int cpu)
>  {
>  	return clk_get_rate(arm_clk) / 1000;
> @@ -45,6 +49,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  	struct dev_pm_opp *opp;
>  	unsigned long freq_hz, volt, volt_old;
>  	unsigned int old_freq, new_freq;
> +	unsigned int soc_opp_index = 0;
>  	int ret;
>  
>  	new_freq = freq_table[index].frequency;
> @@ -63,29 +68,48 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  	rcu_read_unlock();
>  	volt_old = regulator_get_voltage(arm_reg);
>  
> -	dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
> +	/* Find the matching VDDSOC/VDDPU operating voltage */
> +	while (soc_opp_index < soc_opp_count) {
> +		if (new_freq == imx6_soc_opp[soc_opp_index].arm_freq)
> +			break;
> +		soc_opp_index++;
> +	}

I'm not comfortable with this lookup every time imx6q_set_target() is
called.  I think we can use the 'index' variable to find VDDSOC/VDDPU
operating voltage from imx6_soc_opp table directly, if we sort the table
in the same order that freq_table is sorted.

> +	if (soc_opp_index >= soc_opp_count) {

Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
the condition check below is good enough?

	if (soc_opp_index == soc_opp_count)

> +		dev_err(cpu_dev,
> +			"Can NOT find matching imx6_soc_opp voltage!\n");

Put them on the same line, since we can ignore 80 column warning for
message.

> +			return -EINVAL;
> +	}
> +
> +	dev_dbg(cpu_dev, "%u MHz, arm %ld mV, soc-pu %d mV --> %u MHz, arm %ld mV, soc-pu %d mV\n",
>  		old_freq / 1000, volt_old / 1000,
> -		new_freq / 1000, volt / 1000);
> +		imx6_soc_opp[soc_opp_index].soc_volt / 1000,
> +		new_freq / 1000, volt / 1000,
> +		imx6_soc_opp[soc_opp_index].soc_volt / 1000);

You print the same soc_volt for both old and new ones?

>  
>  	/* scaling up?  scale voltage before frequency */
>  	if (new_freq > old_freq) {
> +		if (regulator_is_enabled(pu_reg)) {
> +			ret = regulator_set_voltage_tol(pu_reg,
> +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> +			if (ret) {
> +				dev_err(cpu_dev,
> +					"failed to scale vddpu up: %d\n", ret);
> +				return ret;
> +			}
> +		}
> +		ret = regulator_set_voltage_tol(soc_reg,
> +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> +		if (ret) {
> +			dev_err(cpu_dev,
> +				"failed to scale vddsoc up: %d\n", ret);
> +			return ret;
> +		}
>  		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
>  		if (ret) {
>  			dev_err(cpu_dev,
>  				"failed to scale vddarm up: %d\n", ret);
>  			return ret;
>  		}
> -
> -		/*
> -		 * Need to increase vddpu and vddsoc for safety
> -		 * if we are about to run at 1.2 GHz.
> -		 */
> -		if (new_freq == FREQ_1P2_GHZ / 1000) {
> -			regulator_set_voltage_tol(pu_reg,
> -					PU_SOC_VOLTAGE_HIGH, 0);
> -			regulator_set_voltage_tol(soc_reg,
> -					PU_SOC_VOLTAGE_HIGH, 0);
> -		}
>  	}
>  
>  	/*
> @@ -120,12 +144,22 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  				 "failed to scale vddarm down: %d\n", ret);
>  			ret = 0;
>  		}
> -
> -		if (old_freq == FREQ_1P2_GHZ / 1000) {
> -			regulator_set_voltage_tol(pu_reg,
> -					PU_SOC_VOLTAGE_NORMAL, 0);
> -			regulator_set_voltage_tol(soc_reg,
> -					PU_SOC_VOLTAGE_NORMAL, 0);
> +		ret = regulator_set_voltage_tol(soc_reg,
> +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> +		if (ret) {
> +			dev_warn(cpu_dev,
> +				"failed to scale vddsoc down: %d\n", ret);
> +			ret = 0;
> +		}
> +		if (regulator_is_enabled(pu_reg)) {
> +			ret = regulator_set_voltage_tol(pu_reg,
> +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> +			if (ret) {
> +				dev_warn(cpu_dev,
> +					"failed to scale vddpu down: %d\n",
> +					ret);
> +				ret = 0;
> +			}
>  		}
>  	}
>  
> @@ -153,6 +187,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	struct dev_pm_opp *opp;
>  	unsigned long min_volt, max_volt;
>  	int num, ret;
> +	const struct property *prop;
> +	const __be32 *val;
> +	u32 nr, i;
>  
>  	cpu_dev = get_cpu_device(0);
>  	if (!cpu_dev) {
> @@ -201,9 +238,75 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		goto put_node;
>  	}
>  
> +	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
> +	if (!prop) {
> +		dev_err(cpu_dev,
> +			"fsl,soc-operating-points node not found!\n");

'fsl,soc-operating-points' is not a node but a property.

> +		goto free_freq_table;
> +	}
> +	if (!prop->value) {
> +		dev_err(cpu_dev,
> +			"No entries in fsl-soc-operating-points node!\n");

s/fsl-soc-operating-points/fsl,soc-operating-points

> +		goto free_freq_table;
> +	}

To simplify the code a little bit and populate the return code:

	if (!prop || !prop->value) {
		dev_err(cpu_dev, "No valid fsl,soc-operating-points property is found");
		ret = -ENOENT;
		goto free_freq_table;
	}

Note, it's okay to ignore 80 column warning in case of message output.

Also, you need to understand the working flow of upstreaming kernel.
Generally, we have drivers/cpufreq change go upstream via cpufreq tree
maintained by Rafael, and arch/arm/boot/dts/imx* change go via IMX tree
maintained by myself.  That means your patches should be properly
partitioned to not cause any regression on either of these trees.

Right now, if Rafael merges the patch as it is, it will break imx6q
cpufreq driver on his tree immediately, because fsl,soc-operating-points
change is not on his tree.

To make it easier, you may want to merge dts changes into this patch,
and have the patch go via single tree, either Rafael's or mine (we two
will sort it out), to avoid breakage and maintain git bisect.

> +
> +	/*
> +	 * Each OPP is a set of tuples consisting of frequency and
> +	 * voltage like <freq-kHz vol-uV>.
> +	 */
> +	nr = prop->length / sizeof(u32);
> +	if (nr % 2) {
> +		dev_err(cpu_dev, "Invalid fsl-soc-operating-points list!\n");

fsl,soc-operating-points

and

		ret = -EINVAL;

> +		goto free_freq_table;
> +	}
> +
> +	/* Get the VDDSOC/VDDPU voltages that need to track the CPU voltages. */
> +	imx6_soc_opp = devm_kzalloc(cpu_dev,
> +		sizeof(struct soc_opp) * (nr / 2), GFP_KERNEL);

Quote from Documentation/CodingStyle:

The preferred form for passing a size of a struct is the following:

        p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

Also to follow the indentation style used in the file, the following is
what I would have.

	imx6_soc_opp = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_opp) * (nr / 2),
				    GFP_KERNEL);

> +

Drop this blank line.

> +	if (imx6_soc_opp == NULL) {

		ret = -ENOMEM;

> +		dev_err(cpu_dev, "No Memory for VDDSOC/PU table!\n");

With the error code -ENOMEM returned, we can save this message since I
doubt you will ever get a chance to see it.

> +		goto free_freq_table;
> +	}
> +
> +	rcu_read_lock();
> +	val = prop->value;

What is this assignment used for?

> +
> +	min_volt = max_volt = 0;
> +	for (i = 0; i < nr / 2; i++) {
> +		unsigned long freq = be32_to_cpup(val++);
> +		unsigned long volt = be32_to_cpup(val++);
> +
> +		if (i == 0)
> +			min_volt = max_volt = volt;
> +		if (volt < min_volt)
> +			min_volt = volt;
> +		if (volt > max_volt)
> +			max_volt = volt;
> +		opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
> +		imx6_soc_opp[i].arm_freq = freq;
> +		imx6_soc_opp[i].soc_volt = volt;
> +		soc_opp_count++;
> +	}
> +	rcu_read_unlock();

We may need another sanity check to see if soc_opp_count == num (arm opp
count) here.

Shawn

> +
>  	if (of_property_read_u32(np, "clock-latency", &transition_latency))
>  		transition_latency = CPUFREQ_ETERNAL;
>  
> +	if (min_volt * max_volt != 0) {
> +		/*
> +		 * Calculate the ramp time for max voltage change in the
> +		 * VDDSOC and VDDPU regulators.
> +		 */
> +		ret = regulator_set_voltage_time(soc_reg, min_volt, max_volt);
> +		if (ret > 0)
> +			transition_latency += ret * 1000;
> +
> +		ret = regulator_set_voltage_time(pu_reg, min_volt, max_volt);
> +		if (ret > 0)
> +			transition_latency += ret * 1000;
> +	}
> +
>  	/*
>  	 * OPP is maintained in order of increasing frequency, and
>  	 * freq_table initialised from OPP is therefore sorted in the
> @@ -221,18 +324,6 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  	if (ret > 0)
>  		transition_latency += ret * 1000;
>  
> -	/* Count vddpu and vddsoc latency in for 1.2 GHz support */
> -	if (freq_table[num].frequency == FREQ_1P2_GHZ / 1000) {
> -		ret = regulator_set_voltage_time(pu_reg, PU_SOC_VOLTAGE_NORMAL,
> -						 PU_SOC_VOLTAGE_HIGH);
> -		if (ret > 0)
> -			transition_latency += ret * 1000;
> -		ret = regulator_set_voltage_time(soc_reg, PU_SOC_VOLTAGE_NORMAL,
> -						 PU_SOC_VOLTAGE_HIGH);
> -		if (ret > 0)
> -			transition_latency += ret * 1000;
> -	}
> -
>  	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
>  	if (ret) {
>  		dev_err(cpu_dev, "failed register driver: %d\n", ret);
> -- 
> 1.7.9.5
> 
> 

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Tue, Dec 17, 2013 at 10:56:18AM +0800, Shawn Guo wrote:
> > +	rcu_read_lock();
> > +	val = prop->value;
> 
> What is this assignment used for?

Sorry.  This is a brain-dead comment.  Ignore it.

Shawn

> 
> > +
> > +	min_volt = max_volt = 0;
> > +	for (i = 0; i < nr / 2; i++) {
> > +		unsigned long freq = be32_to_cpup(val++);
> > +		unsigned long volt = be32_to_cpup(val++);
> > +
> > +		if (i == 0)
> > +			min_volt = max_volt = volt;
> > +		if (volt < min_volt)
> > +			min_volt = volt;
> > +		if (volt > max_volt)
> > +			max_volt = volt;
> > +		opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
> > +		imx6_soc_opp[i].arm_freq = freq;
> > +		imx6_soc_opp[i].soc_volt = volt;
> > +		soc_opp_count++;
> > +	}
> > +	rcu_read_unlock();

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Lothar Waßmann]

Hi,

Shawn Guo wrote:
> On Mon, Dec 16, 2013 at 04:14:10PM -0500, Anson Huang wrote:
> > on i.MX6Q, cpu freq change need to follow below flows:
> > 
> > 1. each setpoint has different VDDARM, VDDSOC/PU voltage, get the setpoint
> > table from dts;
> > 2. when cpu freq is scaling up, need to increase VDDSOC/PU voltage before
> > VDDARM, if VDDPU is off, no need to change it;
> > 3. when cpu freq is scaling down, need to decrease VDDARM voltage before
> > VDDSOC/PU, if VDDPU is off, no need to change it;
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
[...]
> > +	if (soc_opp_index >= soc_opp_count) {
> 
> Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
> the condition check below is good enough?
> 
> 	if (soc_opp_index == soc_opp_count)
>
it doen't harm to be on the safe side and use >= anyway!


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Tue, Dec 17, 2013 at 10:53:47AM +0100, Lothar Wa?mann wrote:
> > > +	if (soc_opp_index >= soc_opp_count) {
> > 
> > Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
> > the condition check below is good enough?
> > 
> > 	if (soc_opp_index == soc_opp_count)
> >
> it doen't harm to be on the safe side and use >= anyway!

Well, it may confuse reader.  At least, it took me some time understand
how that ">" condition will happen.  And it turns out never.

Shawn

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Anson.Huang at freescale.com]

Sent from Anson's iPhone

> ? 2013?12?17??20:13?"Shawn Guo" <shawn.guo@linaro.org> ???
> 
> On Tue, Dec 17, 2013 at 10:53:47AM +0100, Lothar Wa?mann wrote:
>>>> +    if (soc_opp_index >= soc_opp_count) {
>>> 
>>> Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
>>> the condition check below is good enough?
>>> 
>>>    if (soc_opp_index == soc_opp_count)
>> it doen't harm to be on the safe side and use >= anyway!
> 
> Well, it may confuse reader.  At least, it took me some time understand
> how that ">" condition will happen.  And it turns out never.

it does not matter now, since in V2 patch, i use index passed from
freq table, no need to look up the soc opp. please help review V2 patch set, thanks.

Anson.


> 
> Shawn
> 

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Lothar Waßmann]

Hi,

Shawn Guo wrote:
> On Tue, Dec 17, 2013 at 10:53:47AM +0100, Lothar Wa?mann wrote:
> > > > +	if (soc_opp_index >= soc_opp_count) {
> > > 
> > > Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
> > > the condition check below is good enough?
> > > 
> > > 	if (soc_opp_index == soc_opp_count)
> > >
> > it doen't harm to be on the safe side and use >= anyway!
> 
> Well, it may confuse reader.  At least, it took me some time understand
> how that ">" condition will happen.  And it turns out never.
> 
man "defensive programming"


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Anson Huang]

On Tue, Dec 17, 2013 at 10:56:18AM +0800, Shawn Guo wrote:
> On Mon, Dec 16, 2013 at 04:14:10PM -0500, Anson Huang wrote:
> > on i.MX6Q, cpu freq change need to follow below flows:
> > 
> > 1. each setpoint has different VDDARM, VDDSOC/PU voltage, get the setpoint
> > table from dts;
> > 2. when cpu freq is scaling up, need to increase VDDSOC/PU voltage before
> > VDDARM, if VDDPU is off, no need to change it;
> > 3. when cpu freq is scaling down, need to decrease VDDARM voltage before
> > VDDSOC/PU, if VDDPU is off, no need to change it;
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
> >  drivers/cpufreq/imx6q-cpufreq.c |  161 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 126 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > index 4b3f18e..5fb302e 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -17,10 +17,6 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> >  
> > -#define PU_SOC_VOLTAGE_NORMAL	1250000
> > -#define PU_SOC_VOLTAGE_HIGH	1275000
> > -#define FREQ_1P2_GHZ		1200000000
> > -
> >  static struct regulator *arm_reg;
> >  static struct regulator *pu_reg;
> >  static struct regulator *soc_reg;
> > @@ -35,6 +31,14 @@ static struct device *cpu_dev;
> >  static struct cpufreq_frequency_table *freq_table;
> >  static unsigned int transition_latency;
> >  
> > +struct soc_opp {
> > +	u32 arm_freq;
> > +	u32 soc_volt;
> > +};
> > +
> > +static struct soc_opp *imx6_soc_opp;
> > +static u32 soc_opp_count;
> > +
> >  static unsigned int imx6q_get_speed(unsigned int cpu)
> >  {
> >  	return clk_get_rate(arm_clk) / 1000;
> > @@ -45,6 +49,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> >  	struct dev_pm_opp *opp;
> >  	unsigned long freq_hz, volt, volt_old;
> >  	unsigned int old_freq, new_freq;
> > +	unsigned int soc_opp_index = 0;
> >  	int ret;
> >  
> >  	new_freq = freq_table[index].frequency;
> > @@ -63,29 +68,48 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> >  	rcu_read_unlock();
> >  	volt_old = regulator_get_voltage(arm_reg);
> >  
> > -	dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > +	/* Find the matching VDDSOC/VDDPU operating voltage */
> > +	while (soc_opp_index < soc_opp_count) {
> > +		if (new_freq == imx6_soc_opp[soc_opp_index].arm_freq)
> > +			break;
> > +		soc_opp_index++;
> > +	}
> 
> I'm not comfortable with this lookup every time imx6q_set_target() is
> called.  I think we can use the 'index' variable to find VDDSOC/VDDPU
> operating voltage from imx6_soc_opp table directly, if we sort the table
> in the same order that freq_table is sorted.
>

yes, we can use the index passed from OPP, then I will add some comments that
the freq/volt table of vddsoc should be sorted in same order.

> > +	if (soc_opp_index >= soc_opp_count) {
> 
> Can soc_opp_index be possibly greater than soc_opp_count?  Otherwise,
> the condition check below is good enough?

right, using "==" is good enough, will improve it in V2 patch.

> 
> 	if (soc_opp_index == soc_opp_count)
> 
> > +		dev_err(cpu_dev,
> > +			"Can NOT find matching imx6_soc_opp voltage!\n");
> 
> Put them on the same line, since we can ignore 80 column warning for
> message.

okay.

> 
> > +			return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(cpu_dev, "%u MHz, arm %ld mV, soc-pu %d mV --> %u MHz, arm %ld mV, soc-pu %d mV\n",
> >  		old_freq / 1000, volt_old / 1000,
> > -		new_freq / 1000, volt / 1000);
> > +		imx6_soc_opp[soc_opp_index].soc_volt / 1000,
> > +		new_freq / 1000, volt / 1000,
> > +		imx6_soc_opp[soc_opp_index].soc_volt / 1000);
> 
> You print the same soc_volt for both old and new ones?

this is my mistake, maybe I can leave the original code here to only
print out vddarm's voltage? Otherwise, I have to add variable to keep
old vddsoc/pu's voltage.

> 
> >  
> >  	/* scaling up?  scale voltage before frequency */
> >  	if (new_freq > old_freq) {
> > +		if (regulator_is_enabled(pu_reg)) {
> > +			ret = regulator_set_voltage_tol(pu_reg,
> > +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> > +			if (ret) {
> > +				dev_err(cpu_dev,
> > +					"failed to scale vddpu up: %d\n", ret);
> > +				return ret;
> > +			}
> > +		}
> > +		ret = regulator_set_voltage_tol(soc_reg,
> > +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> > +		if (ret) {
> > +			dev_err(cpu_dev,
> > +				"failed to scale vddsoc up: %d\n", ret);
> > +			return ret;
> > +		}
> >  		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
> >  		if (ret) {
> >  			dev_err(cpu_dev,
> >  				"failed to scale vddarm up: %d\n", ret);
> >  			return ret;
> >  		}
> > -
> > -		/*
> > -		 * Need to increase vddpu and vddsoc for safety
> > -		 * if we are about to run at 1.2 GHz.
> > -		 */
> > -		if (new_freq == FREQ_1P2_GHZ / 1000) {
> > -			regulator_set_voltage_tol(pu_reg,
> > -					PU_SOC_VOLTAGE_HIGH, 0);
> > -			regulator_set_voltage_tol(soc_reg,
> > -					PU_SOC_VOLTAGE_HIGH, 0);
> > -		}
> >  	}
> >  
> >  	/*
> > @@ -120,12 +144,22 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> >  				 "failed to scale vddarm down: %d\n", ret);
> >  			ret = 0;
> >  		}
> > -
> > -		if (old_freq == FREQ_1P2_GHZ / 1000) {
> > -			regulator_set_voltage_tol(pu_reg,
> > -					PU_SOC_VOLTAGE_NORMAL, 0);
> > -			regulator_set_voltage_tol(soc_reg,
> > -					PU_SOC_VOLTAGE_NORMAL, 0);
> > +		ret = regulator_set_voltage_tol(soc_reg,
> > +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> > +		if (ret) {
> > +			dev_warn(cpu_dev,
> > +				"failed to scale vddsoc down: %d\n", ret);
> > +			ret = 0;
> > +		}
> > +		if (regulator_is_enabled(pu_reg)) {
> > +			ret = regulator_set_voltage_tol(pu_reg,
> > +				imx6_soc_opp[soc_opp_index].soc_volt, 0);
> > +			if (ret) {
> > +				dev_warn(cpu_dev,
> > +					"failed to scale vddpu down: %d\n",
> > +					ret);
> > +				ret = 0;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -153,6 +187,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> >  	struct dev_pm_opp *opp;
> >  	unsigned long min_volt, max_volt;
> >  	int num, ret;
> > +	const struct property *prop;
> > +	const __be32 *val;
> > +	u32 nr, i;
> >  
> >  	cpu_dev = get_cpu_device(0);
> >  	if (!cpu_dev) {
> > @@ -201,9 +238,75 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> >  		goto put_node;
> >  	}
> >  
> > +	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
> > +	if (!prop) {
> > +		dev_err(cpu_dev,
> > +			"fsl,soc-operating-points node not found!\n");
> 
> 'fsl,soc-operating-points' is not a node but a property.

will improve it.

> 
> > +		goto free_freq_table;
> > +	}
> > +	if (!prop->value) {
> > +		dev_err(cpu_dev,
> > +			"No entries in fsl-soc-operating-points node!\n");
> 
> s/fsl-soc-operating-points/fsl,soc-operating-points

will do it.

> 
> > +		goto free_freq_table;
> > +	}
> 
> To simplify the code a little bit and populate the return code:
> 
> 	if (!prop || !prop->value) {
> 		dev_err(cpu_dev, "No valid fsl,soc-operating-points property is found");
> 		ret = -ENOENT;
> 		goto free_freq_table;
> 	}
> 
> Note, it's okay to ignore 80 column warning in case of message output.
> 
> Also, you need to understand the working flow of upstreaming kernel.
> Generally, we have drivers/cpufreq change go upstream via cpufreq tree
> maintained by Rafael, and arch/arm/boot/dts/imx* change go via IMX tree
> maintained by myself.  That means your patches should be properly
> partitioned to not cause any regression on either of these trees.
> 
> Right now, if Rafael merges the patch as it is, it will break imx6q
> cpufreq driver on his tree immediately, because fsl,soc-operating-points
> change is not on his tree.
> 
> To make it easier, you may want to merge dts changes into this patch,
> and have the patch go via single tree, either Rafael's or mine (we two
> will sort it out), to avoid breakage and maintain git bisect.

sorry for that, I was confused before, now I think I should merge the dts
and cpufreq driver change into a patch, and send them to both of you for
review.

> 
> > +
> > +	/*
> > +	 * Each OPP is a set of tuples consisting of frequency and
> > +	 * voltage like <freq-kHz vol-uV>.
> > +	 */
> > +	nr = prop->length / sizeof(u32);
> > +	if (nr % 2) {
> > +		dev_err(cpu_dev, "Invalid fsl-soc-operating-points list!\n");
> 
> fsl,soc-operating-points
> 
> and
> 
> 		ret = -EINVAL;

no need to free freq table here?

> 
> > +		goto free_freq_table;
> > +	}
> > +
> > +	/* Get the VDDSOC/VDDPU voltages that need to track the CPU voltages. */
> > +	imx6_soc_opp = devm_kzalloc(cpu_dev,
> > +		sizeof(struct soc_opp) * (nr / 2), GFP_KERNEL);
> 
> Quote from Documentation/CodingStyle:
> 
> The preferred form for passing a size of a struct is the following:
> 
>         p = kmalloc(sizeof(*p), ...);
> 
> The alternative form where struct name is spelled out hurts readability and
> introduces an opportunity for a bug when the pointer variable type is changed
> but the corresponding sizeof that is passed to a memory allocator is not.
> 
> Also to follow the indentation style used in the file, the following is
> what I would have.
> 
> 	imx6_soc_opp = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_opp) * (nr / 2),
> 				    GFP_KERNEL);

will improve it.

> 
> > +
> 
> Drop this blank line.

Okay.

> 
> > +	if (imx6_soc_opp == NULL) {
> 
> 		ret = -ENOMEM;
> 
> > +		dev_err(cpu_dev, "No Memory for VDDSOC/PU table!\n");
> 
> With the error code -ENOMEM returned, we can save this message since I
> doubt you will ever get a chance to see it.

will remove it.

> 
> > +		goto free_freq_table;
> > +	}
> > +
> > +	rcu_read_lock();
> > +	val = prop->value;
> 
> What is this assignment used for?

mistake, will remove it.

> 
> > +
> > +	min_volt = max_volt = 0;
> > +	for (i = 0; i < nr / 2; i++) {
> > +		unsigned long freq = be32_to_cpup(val++);
> > +		unsigned long volt = be32_to_cpup(val++);
> > +
> > +		if (i == 0)
> > +			min_volt = max_volt = volt;
> > +		if (volt < min_volt)
> > +			min_volt = volt;
> > +		if (volt > max_volt)
> > +			max_volt = volt;
> > +		opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
> > +		imx6_soc_opp[i].arm_freq = freq;
> > +		imx6_soc_opp[i].soc_volt = volt;
> > +		soc_opp_count++;
> > +	}
> > +	rcu_read_unlock();
> 
> We may need another sanity check to see if soc_opp_count == num (arm opp
> count) here.

yes, will add it, need to make sure soc_opp_count is same as arm opp count,
but since we did NOT have 1G2 check, so the soc_opp_count will ">=" arm opp
count.

> 
> Shawn
> 
> > +
> >  	if (of_property_read_u32(np, "clock-latency", &transition_latency))
> >  		transition_latency = CPUFREQ_ETERNAL;
> >  
> > +	if (min_volt * max_volt != 0) {
> > +		/*
> > +		 * Calculate the ramp time for max voltage change in the
> > +		 * VDDSOC and VDDPU regulators.
> > +		 */
> > +		ret = regulator_set_voltage_time(soc_reg, min_volt, max_volt);
> > +		if (ret > 0)
> > +			transition_latency += ret * 1000;
> > +
> > +		ret = regulator_set_voltage_time(pu_reg, min_volt, max_volt);
> > +		if (ret > 0)
> > +			transition_latency += ret * 1000;
> > +	}
> > +
> >  	/*
> >  	 * OPP is maintained in order of increasing frequency, and
> >  	 * freq_table initialised from OPP is therefore sorted in the
> > @@ -221,18 +324,6 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> >  	if (ret > 0)
> >  		transition_latency += ret * 1000;
> >  
> > -	/* Count vddpu and vddsoc latency in for 1.2 GHz support */
> > -	if (freq_table[num].frequency == FREQ_1P2_GHZ / 1000) {
> > -		ret = regulator_set_voltage_time(pu_reg, PU_SOC_VOLTAGE_NORMAL,
> > -						 PU_SOC_VOLTAGE_HIGH);
> > -		if (ret > 0)
> > -			transition_latency += ret * 1000;
> > -		ret = regulator_set_voltage_time(soc_reg, PU_SOC_VOLTAGE_NORMAL,
> > -						 PU_SOC_VOLTAGE_HIGH);
> > -		if (ret > 0)
> > -			transition_latency += ret * 1000;
> > -	}
> > -
> >  	ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
> >  	if (ret) {
> >  		dev_err(cpu_dev, "failed register driver: %d\n", ret);
> > -- 
> > 1.7.9.5
> > 
> > 
> 

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Tue, Dec 17, 2013 at 11:52:57AM -0500, Anson Huang wrote:
> > > @@ -63,29 +68,48 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> > >  	rcu_read_unlock();
> > >  	volt_old = regulator_get_voltage(arm_reg);
> > >  
> > > -	dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > > +	/* Find the matching VDDSOC/VDDPU operating voltage */
> > > +	while (soc_opp_index < soc_opp_count) {
> > > +		if (new_freq == imx6_soc_opp[soc_opp_index].arm_freq)
> > > +			break;
> > > +		soc_opp_index++;
> > > +	}
> > 
> > I'm not comfortable with this lookup every time imx6q_set_target() is
> > called.  I think we can use the 'index' variable to find VDDSOC/VDDPU
> > operating voltage from imx6_soc_opp table directly, if we sort the table
> > in the same order that freq_table is sorted.
> >
> 
> yes, we can use the index passed from OPP, then I will add some comments that
> the freq/volt table of vddsoc should be sorted in same order.

It's not as easy as adding some comments.  I think we need to ensure the
order in probe function, probably sorting them in code.  Otherwise, it's
dangerous if we're running at a frequency with a wrong voltage.

Also, if we sort the table in the same order as freq_table, we do not
even need to have arm_freq in the soc_opp table.

<snip>

> > > +	dev_dbg(cpu_dev, "%u MHz, arm %ld mV, soc-pu %d mV --> %u MHz, arm %ld mV, soc-pu %d mV\n",
> > >  		old_freq / 1000, volt_old / 1000,
> > > -		new_freq / 1000, volt / 1000);
> > > +		imx6_soc_opp[soc_opp_index].soc_volt / 1000,
> > > +		new_freq / 1000, volt / 1000,
> > > +		imx6_soc_opp[soc_opp_index].soc_volt / 1000);
> > 
> > You print the same soc_volt for both old and new ones?
> 
> this is my mistake, maybe I can leave the original code here to only
> print out vddarm's voltage? Otherwise, I have to add variable to keep
> old vddsoc/pu's voltage.

Yea, that's okay with leaving it as the existing code.

<snip>

> > > +	/*
> > > +	 * Each OPP is a set of tuples consisting of frequency and
> > > +	 * voltage like <freq-kHz vol-uV>.
> > > +	 */
> > > +	nr = prop->length / sizeof(u32);
> > > +	if (nr % 2) {
> > > +		dev_err(cpu_dev, "Invalid fsl-soc-operating-points list!\n");
> > 
> > fsl,soc-operating-points
> > 
> > and
> > 
> > 		ret = -EINVAL;
> 
> no need to free freq table here?

We're only populating the error code here, and will still goto
free_freq_table below.

> > > +		goto free_freq_table;
> > > +	}

<snip>

> > > +	min_volt = max_volt = 0;
> > > +	for (i = 0; i < nr / 2; i++) {
> > > +		unsigned long freq = be32_to_cpup(val++);
> > > +		unsigned long volt = be32_to_cpup(val++);
> > > +
> > > +		if (i == 0)
> > > +			min_volt = max_volt = volt;
> > > +		if (volt < min_volt)
> > > +			min_volt = volt;
> > > +		if (volt > max_volt)
> > > +			max_volt = volt;
> > > +		opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
> > > +		imx6_soc_opp[i].arm_freq = freq;
> > > +		imx6_soc_opp[i].soc_volt = volt;
> > > +		soc_opp_count++;
> > > +	}
> > > +	rcu_read_unlock();
> > 
> > We may need another sanity check to see if soc_opp_count == num (arm opp
> > count) here.
> 
> yes, will add it, need to make sure soc_opp_count is same as arm opp count,
> but since we did NOT have 1G2 check, so the soc_opp_count will ">=" arm opp
> count.

Hmm, this is another problem with the for-loop above.  The soc_volt
should be added into imx6_soc_opp table and soc_opp_count increases,
only when dev_pm_opp_find_freq_exact() succeeds (IOW, the freq can be
found in arm opp table).

Shawn

[PATCH 4/4] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Mon, Dec 16, 2013 at 04:14:10PM -0500, Anson Huang wrote:
> @@ -201,9 +238,75 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		goto put_node;
>  	}
>  
> +	prop = of_find_property(np, "fsl,soc-operating-points", NULL);

This is a new property, and it should be documented in binding doc.
Remember to copy devicetree at vger.kernel.org when you send a patch with
binding doc update.

Shawn

[PATCH 3/4] ARM: imx: add VDDSOC/PU setpoint info into dts

[Shawn Guo]

On Mon, Dec 16, 2013 at 04:14:09PM -0500, Anson Huang wrote:
> i.MX6Q needs to update VDDARM, VDDSOC/PU regulator when CPUFreq
> is changed, each setpoint has different voltage, so we need to
> pass VDDARM, VDDSOC/PU's freq-voltage info from dts.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q.dtsi |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7e8332..03da628 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -30,6 +30,13 @@
>  				792000  1150000
>  				396000  975000
>  			>;
> +			fsl,soc-operating-points = <
> +			/* ARM kHz  SOC-PU uV */
> +				1200000	1275000
> +				996000	1250000
> +				792000	1175000
> +				396000	1175000

Okay.  Now I get it why you're increasing VDDARM_CAP to 975mV in the
first patch.  You expect VDDSOC_CAP to be 1175mV rather than 1250mV for
396MHz operating-point, which is hard-coded in cpufreq driver right now.

Shawn

> +			>;
>  			clock-latency = <61036>; /* two CLK32 periods */
>  			clocks = <&clks 104>, <&clks 6>, <&clks 16>,
>  				 <&clks 17>, <&clks 170>;
> -- 
> 1.7.9.5
> 
>