Re: [PATCH V2 2/2] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

Re: [PATCH V2 2/2] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Tue, Dec 17, 2013 at 05:08:22PM -0500, Anson Huang wrote:

>  	if (new_freq > old_freq) {
> +		if (regulator_is_enabled(pu_reg)) {
> +			ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);

As I pointed out while reviewing the regulator driver this is racy,
nothing stops the regulator state being changed after you read it and if
you skip setting the voltage then when something comes along and enables
the regulator later it will be enabled with a possibly incorrect
voltage.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Anson.Huang]

Sent from Anson's iPhone

> 在 2013年12月17日，21:23，"Mark Brown" <broonie@kernel.org> 写道：
> 
>> On Tue, Dec 17, 2013 at 05:08:22PM -0500, Anson Huang wrote:
>> 
>>    if (new_freq > old_freq) {
>> +        if (regulator_is_enabled(pu_reg)) {
>> +            ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
> 
> As I pointed out while reviewing the regulator driver this is racy,
> nothing stops the regulator state being changed after you read it and if
> you skip setting the voltage then when something comes along and enables
> the regulator later it will be enabled with a possibly incorrect
> voltage.

yes, you are correct, I will remove this PU status check in V3 patch.

Anson

[PATCH V2 1/2] ARM: imx: add vddsoc/pu setpoint info into dts

[Anson Huang]

i.MX6Q needs to update vddarm, vddsoc/pu regulators when cpu freq
is changed, each setpoint has different voltage, so we need to
pass vddarm, vddsoc/pu's freq-voltage info from dts together.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 .../devicetree/bindings/cpufreq/cpufreq-imx6.txt   |   39 ++++++++++++++++++++
 arch/arm/boot/dts/imx6q.dtsi                       |    7 ++++
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
new file mode 100644
index 0000000..0c71dbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
@@ -0,0 +1,39 @@
+i.MX6 cpufreq driver
+-------------------
+
+i.MX6 SoC cpufreq driver for CPU frequency scaling.
+
+Optional properties:
+-fsl,soc-operating-points: Specify vddsoc/pu voltage settings that must
+ go with cpu0's operating-points.
+
+Examples:
+
+	cpu@0 {
+		compatible = "arm,cortex-a9";
+		device_type = "cpu";
+		reg = <0>;
+		next-level-cache = <&L2>;
+		operating-points = <
+			/* kHz    uV */
+			1200000 1275000
+			996000  1250000
+			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>;
+		clock-names = "arm", "pll2_pfd2_396m", "step",
+			      "pll1_sw", "pll1_sys";
+		arm-supply = <&reg_arm>;
+		pu-supply = <&reg_pu>;
+		soc-supply = <&reg_soc>;
+	};
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index e7e8332..021e0cb 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 V2 2/2] 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;

normally dts will pass vddsoc/pu freq/volt info to kernel, if not, will
use fixed value for vddsoc/pu voltage setting.

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

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 4b3f18e..65c1df1 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -35,6 +35,9 @@ static struct device *cpu_dev;
 static struct cpufreq_frequency_table *freq_table;
 static unsigned int transition_latency;
 
+static u32 *imx6_soc_volt;
+static u32 soc_opp_count;
+
 static unsigned int imx6q_get_speed(unsigned int cpu)
 {
 	return clk_get_rate(arm_clk) / 1000;
@@ -69,23 +72,24 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
 
 	/* 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_volt[index], 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_volt[index], 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 +124,17 @@ 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_volt[index], 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_volt[index], 0);
+			if (ret) {
+				dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
+				ret = 0;
+			}
 		}
 	}
 
@@ -153,6 +162,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, j;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev) {
@@ -201,10 +213,69 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		goto put_node;
 	}
 
+	/* Make imx6_soc_volt array's size same as arm opp number */
+	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
+	if (imx6_soc_volt == NULL) {
+		dev_warn(cpu_dev, "No valid memory for imx6_soc_volt!\n");
+		ret = -ENOMEM;
+		goto free_freq_table;
+	}
+
+	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
+	if (!prop || !prop->value) {
+		dev_warn(cpu_dev, "No valid fsl,soc-operating-points property is found!\n");
+		goto soc_opp_out;
+	}
+
+	/*
+	 * 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 || (nr / 2) < num) {
+		dev_warn(cpu_dev, "Invalid fsl,soc-operating-points list!\n");
+		goto soc_opp_out;
+	}
+
+	rcu_read_lock();
+	for (j = 0; j < num; j++) {
+		val = prop->value;
+		for (i = 0; i < nr / 2; i++) {
+			unsigned long freq = be32_to_cpup(val++);
+			unsigned long volt = be32_to_cpup(val++);
+			if (freq_table[j].frequency == freq) {
+				imx6_soc_volt[soc_opp_count++] = volt;
+				break;
+			}
+		}
+	}
+	rcu_read_unlock();
+
+soc_opp_out:
+	/* use fixed soc opp volt if no valid soc opp info found in dtb */
+	if (soc_opp_count != num) {
+		dev_warn(cpu_dev, "can NOT find valid soc opp info in dtb, use default value!\n");
+		for (j = 0; j < num; j++)
+			imx6_soc_volt[j] = PU_SOC_VOLTAGE_NORMAL;
+		if (freq_table[num - 1].frequency * 1000 == FREQ_1P2_GHZ)
+			imx6_soc_volt[num - 1] = PU_SOC_VOLTAGE_HIGH;
+	}
+
 	if (of_property_read_u32(np, "clock-latency", &transition_latency))
 		transition_latency = CPUFREQ_ETERNAL;
 
 	/*
+	 * Calculate the ramp time for max voltage change in the
+	 * VDDSOC and VDDPU regulators.
+	 */
+	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
+	if (ret > 0)
+		transition_latency += ret * 1000;
+	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
+	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
 	 * same order.
@@ -221,18 +292,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

Re: [PATCH V2 1/2] ARM: imx: add vddsoc/pu setpoint info into dts

[Shawn Guo]

On Tue, Dec 17, 2013 at 05:08:21PM -0500, Anson Huang wrote:
> i.MX6Q needs to update vddarm, vddsoc/pu regulators when cpu freq
> is changed, each setpoint has different voltage, so we need to
> pass vddarm, vddsoc/pu's freq-voltage info from dts together.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-imx6.txt   |   39 ++++++++++++++++++++

The binding doc changes should generally be part of driver changes or a
separate patch, but never be part of dts patch.

>  arch/arm/boot/dts/imx6q.dtsi                       |    7 ++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> new file mode 100644
> index 0000000..0c71dbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> @@ -0,0 +1,39 @@
> +i.MX6 cpufreq driver
> +-------------------
> +
> +i.MX6 SoC cpufreq driver for CPU frequency scaling.
> +

I think you need to either copy the text from cpufreq-cpu0.txt or refer
to it for some necessary info such as where the properties should be
defined, what required properties should be defined, and what properties
are optional etc.

> +Optional properties:
> +-fsl,soc-operating-points: Specify vddsoc/pu voltage settings that must
> + go with cpu0's operating-points.
> +
> +Examples:
> +
> +	cpu@0 {
> +		compatible = "arm,cortex-a9";
> +		device_type = "cpu";
> +		reg = <0>;
> +		next-level-cache = <&L2>;

Above properties are not related to cpufreq driver, maybe we can save
them.

> +		operating-points = <
> +			/* kHz    uV */
> +			1200000 1275000
> +			996000  1250000
> +			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>;
> +		clock-names = "arm", "pll2_pfd2_396m", "step",
> +			      "pll1_sw", "pll1_sys";
> +		arm-supply = <&reg_arm>;
> +		pu-supply = <&reg_pu>;
> +		soc-supply = <&reg_soc>;

Any properties in the example that are required for a successful driver
probe should be documented in 'Required properties', and others are in
'Optional properties'.

Shawn

> +	};
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index e7e8332..021e0cb 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
> 
> 

Re: [PATCH V2 2/2] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Shawn Guo]

On Tue, Dec 17, 2013 at 05:08:22PM -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;
> 
> normally dts will pass vddsoc/pu freq/volt info to kernel, if not, will
> use fixed value for vddsoc/pu voltage setting.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c |  117 +++++++++++++++++++++++++++++----------
>  1 file changed, 88 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 4b3f18e..65c1df1 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -35,6 +35,9 @@ static struct device *cpu_dev;
>  static struct cpufreq_frequency_table *freq_table;
>  static unsigned int transition_latency;
>  
> +static u32 *imx6_soc_volt;
> +static u32 soc_opp_count;
> +
>  static unsigned int imx6q_get_speed(unsigned int cpu)
>  {
>  	return clk_get_rate(arm_clk) / 1000;
> @@ -69,23 +72,24 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>  
>  	/* 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_volt[index], 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_volt[index], 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 +124,17 @@ 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_volt[index], 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_volt[index], 0);
> +			if (ret) {
> +				dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> +				ret = 0;
> +			}
>  		}
>  	}
>  
> @@ -153,6 +162,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, j;
>  
>  	cpu_dev = get_cpu_device(0);
>  	if (!cpu_dev) {
> @@ -201,10 +213,69 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		goto put_node;
>  	}
>  
> +	/* Make imx6_soc_volt array's size same as arm opp number */
> +	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
> +	if (imx6_soc_volt == NULL) {
> +		dev_warn(cpu_dev, "No valid memory for imx6_soc_volt!\n");

Drop this message.  Error code -ENOMEM is enough to tell what's going
wrong.

> +		ret = -ENOMEM;
> +		goto free_freq_table;
> +	}
> +
> +	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
> +	if (!prop || !prop->value) {
> +		dev_warn(cpu_dev, "No valid fsl,soc-operating-points property is found!\n");
> +		goto soc_opp_out;
> +	}
> +
> +	/*
> +	 * 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 || (nr / 2) < num) {
> +		dev_warn(cpu_dev, "Invalid fsl,soc-operating-points list!\n");
> +		goto soc_opp_out;
> +	}
> +
> +	rcu_read_lock();

You do not need this lock any more.

> +	for (j = 0; j < num; j++) {
> +		val = prop->value;
> +		for (i = 0; i < nr / 2; i++) {
> +			unsigned long freq = be32_to_cpup(val++);
> +			unsigned long volt = be32_to_cpup(val++);
> +			if (freq_table[j].frequency == freq) {
> +				imx6_soc_volt[soc_opp_count++] = volt;
> +				break;
> +			}
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +soc_opp_out:
> +	/* use fixed soc opp volt if no valid soc opp info found in dtb */
> +	if (soc_opp_count != num) {
> +		dev_warn(cpu_dev, "can NOT find valid soc opp info in dtb, use default value!\n");

It will be a little noisy for old DTB case with print so many warnings.
Maybe we should drop above two dev_warn() in fsl,soc-operating-points
property check and do once here.

> +		for (j = 0; j < num; j++)
> +			imx6_soc_volt[j] = PU_SOC_VOLTAGE_NORMAL;
> +		if (freq_table[num - 1].frequency * 1000 == FREQ_1P2_GHZ)
> +			imx6_soc_volt[num - 1] = PU_SOC_VOLTAGE_HIGH;
> +	}
> +
>  	if (of_property_read_u32(np, "clock-latency", &transition_latency))
>  		transition_latency = CPUFREQ_ETERNAL;
>  
>  	/*
> +	 * Calculate the ramp time for max voltage change in the
> +	 * VDDSOC and VDDPU regulators.
> +	 */
> +	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> +	if (ret > 0)
> +		transition_latency += ret * 1000;
> +	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);

One of the regulator_set_voltage_time() should be for pu_reg.

Shawn

> +	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
>  	 * same order.
> @@ -221,18 +292,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
> 
> 

Re: [PATCH V2 1/2] ARM: imx: add vddsoc/pu setpoint info into dts

[Shawn Guo]

On Wed, Dec 18, 2013 at 03:17:42PM -0500, Anson Huang wrote:
> > > +		operating-points = <
> > > +			/* kHz    uV */
> > > +			1200000 1275000
> > > +			996000  1250000
> > > +			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>;
> > > +		clock-names = "arm", "pll2_pfd2_396m", "step",
> > > +			      "pll1_sw", "pll1_sys";
> > > +		arm-supply = <&reg_arm>;
> > > +		pu-supply = <&reg_pu>;
> > > +		soc-supply = <&reg_soc>;
> > 
> > Any properties in the example that are required for a successful driver
> > probe should be documented in 'Required properties', and others are in
> > 'Optional properties'.
> 
> I will not list so many properties as example as long as I describe this
> preoperties should be included in cpu@0's node?

The example is good.  But you also need to document properties clocks,
clock-names, arm-supply etc.

Shawn

Re: [PATCH V2 2/2] cpufreq: imx6q: correct VDDSOC/PU voltage scaling when cpufreq is changed

[Anson Huang]

On Wed, Dec 18, 2013 at 03:33:23PM +0800, Shawn Guo wrote:
> On Tue, Dec 17, 2013 at 05:08:22PM -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;
> > 
> > normally dts will pass vddsoc/pu freq/volt info to kernel, if not, will
> > use fixed value for vddsoc/pu voltage setting.
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
> >  drivers/cpufreq/imx6q-cpufreq.c |  117 +++++++++++++++++++++++++++++----------
> >  1 file changed, 88 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> > index 4b3f18e..65c1df1 100644
> > --- a/drivers/cpufreq/imx6q-cpufreq.c
> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
> > @@ -35,6 +35,9 @@ static struct device *cpu_dev;
> >  static struct cpufreq_frequency_table *freq_table;
> >  static unsigned int transition_latency;
> >  
> > +static u32 *imx6_soc_volt;
> > +static u32 soc_opp_count;
> > +
> >  static unsigned int imx6q_get_speed(unsigned int cpu)
> >  {
> >  	return clk_get_rate(arm_clk) / 1000;
> > @@ -69,23 +72,24 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> >  
> >  	/* 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_volt[index], 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_volt[index], 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 +124,17 @@ 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_volt[index], 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_volt[index], 0);
> > +			if (ret) {
> > +				dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
> > +				ret = 0;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -153,6 +162,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, j;
> >  
> >  	cpu_dev = get_cpu_device(0);
> >  	if (!cpu_dev) {
> > @@ -201,10 +213,69 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> >  		goto put_node;
> >  	}
> >  
> > +	/* Make imx6_soc_volt array's size same as arm opp number */
> > +	imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
> > +	if (imx6_soc_volt == NULL) {
> > +		dev_warn(cpu_dev, "No valid memory for imx6_soc_volt!\n");
> 
> Drop this message.  Error code -ENOMEM is enough to tell what's going
> wrong.

Oh, sorry, I forget this one, you have told me in last patch's comment. Will drop it in V3.


> 
> > +		ret = -ENOMEM;
> > +		goto free_freq_table;
> > +	}
> > +
> > +	prop = of_find_property(np, "fsl,soc-operating-points", NULL);
> > +	if (!prop || !prop->value) {
> > +		dev_warn(cpu_dev, "No valid fsl,soc-operating-points property is found!\n");
> > +		goto soc_opp_out;
> > +	}
> > +
> > +	/*
> > +	 * 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 || (nr / 2) < num) {
> > +		dev_warn(cpu_dev, "Invalid fsl,soc-operating-points list!\n");
> > +		goto soc_opp_out;
> > +	}
> > +
> > +	rcu_read_lock();
> 
> You do not need this lock any more.

Right, the lock is only required by opp_find_freq_exact routine, will remove it in V3.

> 
> > +	for (j = 0; j < num; j++) {
> > +		val = prop->value;
> > +		for (i = 0; i < nr / 2; i++) {
> > +			unsigned long freq = be32_to_cpup(val++);
> > +			unsigned long volt = be32_to_cpup(val++);
> > +			if (freq_table[j].frequency == freq) {
> > +				imx6_soc_volt[soc_opp_count++] = volt;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +
> > +soc_opp_out:
> > +	/* use fixed soc opp volt if no valid soc opp info found in dtb */
> > +	if (soc_opp_count != num) {
> > +		dev_warn(cpu_dev, "can NOT find valid soc opp info in dtb, use default value!\n");
> 
> It will be a little noisy for old DTB case with print so many warnings.
> Maybe we should drop above two dev_warn() in fsl,soc-operating-points
> property check and do once here.

Agreed, will do that in V3.

> 
> > +		for (j = 0; j < num; j++)
> > +			imx6_soc_volt[j] = PU_SOC_VOLTAGE_NORMAL;
> > +		if (freq_table[num - 1].frequency * 1000 == FREQ_1P2_GHZ)
> > +			imx6_soc_volt[num - 1] = PU_SOC_VOLTAGE_HIGH;
> > +	}
> > +
> >  	if (of_property_read_u32(np, "clock-latency", &transition_latency))
> >  		transition_latency = CPUFREQ_ETERNAL;
> >  
> >  	/*
> > +	 * Calculate the ramp time for max voltage change in the
> > +	 * VDDSOC and VDDPU regulators.
> > +	 */
> > +	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> > +	if (ret > 0)
> > +		transition_latency += ret * 1000;
> > +	ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
> 
> One of the regulator_set_voltage_time() should be for pu_reg.

Yes, I saw this right after sending out V2 patch set, will do that in V3.

> 
> Shawn
> 
> > +	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
> >  	 * same order.
> > @@ -221,18 +292,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
> > 
> > 
> 

Re: [PATCH V2 1/2] ARM: imx: add vddsoc/pu setpoint info into dts

[Anson Huang]

On Wed, Dec 18, 2013 at 03:05:39PM +0800, Shawn Guo wrote:
> On Tue, Dec 17, 2013 at 05:08:21PM -0500, Anson Huang wrote:
> > i.MX6Q needs to update vddarm, vddsoc/pu regulators when cpu freq
> > is changed, each setpoint has different voltage, so we need to
> > pass vddarm, vddsoc/pu's freq-voltage info from dts together.
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
> >  .../devicetree/bindings/cpufreq/cpufreq-imx6.txt   |   39 ++++++++++++++++++++
> 
> The binding doc changes should generally be part of driver changes or a
> separate patch, but never be part of dts patch.
> 
> >  arch/arm/boot/dts/imx6q.dtsi                       |    7 ++++
> >  2 files changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> > new file mode 100644
> > index 0000000..0c71dbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-imx6.txt
> > @@ -0,0 +1,39 @@
> > +i.MX6 cpufreq driver
> > +-------------------
> > +
> > +i.MX6 SoC cpufreq driver for CPU frequency scaling.
> > +
> 
> I think you need to either copy the text from cpufreq-cpu0.txt or refer
> to it for some necessary info such as where the properties should be
> defined, what required properties should be defined, and what properties
> are optional etc.

Right, I was confused before and not sure how to do that, so I copy the whole
cpu@0's dts as example, will add info to let user know this is a sub property
of cpu@0, and refer to cpufreq-cpu0.txt.

> 
> > +Optional properties:
> > +-fsl,soc-operating-points: Specify vddsoc/pu voltage settings that must
> > + go with cpu0's operating-points.
> > +
> > +Examples:
> > +
> > +	cpu@0 {
> > +		compatible = "arm,cortex-a9";
> > +		device_type = "cpu";
> > +		reg = <0>;
> > +		next-level-cache = <&L2>;
> 
> Above properties are not related to cpufreq driver, maybe we can save
> them.

Yes, with above change, I can remove these properties.

> 
> > +		operating-points = <
> > +			/* kHz    uV */
> > +			1200000 1275000
> > +			996000  1250000
> > +			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>;
> > +		clock-names = "arm", "pll2_pfd2_396m", "step",
> > +			      "pll1_sw", "pll1_sys";
> > +		arm-supply = <&reg_arm>;
> > +		pu-supply = <&reg_pu>;
> > +		soc-supply = <&reg_soc>;
> 
> Any properties in the example that are required for a successful driver
> probe should be documented in 'Required properties', and others are in
> 'Optional properties'.

I will not list so many properties as example as long as I describe this
preoperties should be included in cpu@0's node?

> 
> Shawn
> 
> > +	};
> > diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> > index e7e8332..021e0cb 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
> > 
> > 
>