[PATCH 1/2] [CPUFREQ] s3c64xx: Use pr_fmt() for consistent log messages

[PATCH 1/2] [CPUFREQ] s3c64xx: Use pr_fmt() for consistent log messages

[Mark Brown]

They're already consistent but it saves remembering to do so.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/cpufreq/s3c64xx-cpufreq.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index 3475f65..a5e72cb 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -8,6 +8,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt) "cpufreq: " fmt
+
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/init.h>
@@ -91,7 +93,7 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 	if (freqs.old == freqs.new)
 		return 0;
 
-	pr_debug("cpufreq: Transition %d-%dkHz\n", freqs.old, freqs.new);
+	pr_debug("Transition %d-%dkHz\n", freqs.old, freqs.new);
 
 	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
 
@@ -101,7 +103,7 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 					    dvfs->vddarm_min,
 					    dvfs->vddarm_max);
 		if (ret != 0) {
-			pr_err("cpufreq: Failed to set VDDARM for %dkHz: %d\n",
+			pr_err("Failed to set VDDARM for %dkHz: %d\n",
 			       freqs.new, ret);
 			goto err;
 		}
@@ -110,7 +112,7 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	ret = clk_set_rate(armclk, freqs.new * 1000);
 	if (ret < 0) {
-		pr_err("cpufreq: Failed to set rate %dkHz: %d\n",
+		pr_err("Failed to set rate %dkHz: %d\n",
 		       freqs.new, ret);
 		goto err;
 	}
@@ -123,14 +125,14 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 					    dvfs->vddarm_min,
 					    dvfs->vddarm_max);
 		if (ret != 0) {
-			pr_err("cpufreq: Failed to set VDDARM for %dkHz: %d\n",
+			pr_err("Failed to set VDDARM for %dkHz: %d\n",
 			       freqs.new, ret);
 			goto err_clk;
 		}
 	}
 #endif
 
-	pr_debug("cpufreq: Set actual frequency %lukHz\n",
+	pr_debug("Set actual frequency %lukHz\n",
 		 clk_get_rate(armclk) / 1000);
 
 	return 0;
@@ -153,7 +155,7 @@ static void __init s3c64xx_cpufreq_config_regulator(void)
 
 	count = regulator_count_voltages(vddarm);
 	if (count < 0) {
-		pr_err("cpufreq: Unable to check supported voltages\n");
+		pr_err("Unable to check supported voltages\n");
 	}
 
 	freq = s3c64xx_freq_table;
@@ -171,7 +173,7 @@ static void __init s3c64xx_cpufreq_config_regulator(void)
 		}
 
 		if (!found) {
-			pr_debug("cpufreq: %dkHz unsupported by regulator\n",
+			pr_debug("%dkHz unsupported by regulator\n",
 				 freq->frequency);
 			freq->frequency = CPUFREQ_ENTRY_INVALID;
 		}
@@ -194,13 +196,13 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 		return -EINVAL;
 
 	if (s3c64xx_freq_table == NULL) {
-		pr_err("cpufreq: No frequency information for this CPU\n");
+		pr_err("No frequency information for this CPU\n");
 		return -ENODEV;
 	}
 
 	armclk = clk_get(NULL, "armclk");
 	if (IS_ERR(armclk)) {
-		pr_err("cpufreq: Unable to obtain ARMCLK: %ld\n",
+		pr_err("Unable to obtain ARMCLK: %ld\n",
 		       PTR_ERR(armclk));
 		return PTR_ERR(armclk);
 	}
@@ -209,12 +211,19 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 	vddarm = regulator_get(NULL, "vddarm");
 	if (IS_ERR(vddarm)) {
 		ret = PTR_ERR(vddarm);
-		pr_err("cpufreq: Failed to obtain VDDARM: %d\n", ret);
-		pr_err("cpufreq: Only frequency scaling available\n");
+		pr_err("Failed to obtain VDDARM: %d\n", ret);
+		pr_err("Only frequency scaling available\n");
 		vddarm = NULL;
 	} else {
 		s3c64xx_cpufreq_config_regulator();
 	}
+
+	vddint = regulator_get(NULL, "vddint");
+	if (IS_ERR(vddint)) {
+		ret = PTR_ERR(vddint);
+		pr_err("Failed to obtain VDDINT: %d\n", ret);
+		vddint = NULL;
+	}
 #endif
 
 	freq = s3c64xx_freq_table;
@@ -225,7 +234,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 		r = clk_round_rate(armclk, freq->frequency * 1000);
 		r /= 1000;
 		if (r != freq->frequency) {
-			pr_debug("cpufreq: %dkHz unsupported by clock\n",
+			pr_debug("%dkHz unsupported by clock\n",
 				 freq->frequency);
 			freq->frequency = CPUFREQ_ENTRY_INVALID;
 		}
@@ -248,7 +257,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 
 	ret = cpufreq_frequency_table_cpuinfo(policy, s3c64xx_freq_table);
 	if (ret != 0) {
-		pr_err("cpufreq: Failed to configure frequency table: %d\n",
+		pr_err("Failed to configure frequency table: %d\n",
 		       ret);
 		regulator_put(vddarm);
 		clk_put(armclk);
-- 
1.7.7.3

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Mark Brown]

Save a bit more power by scaling VDDINT as well as VDDARM when we have a
regulator for it. Don't worry too much if we don't have one, and just log
errors when scaling as they're unlikely to happen in practice without the
system being in great trouble and proper handling would complicate the
code quite a bit.

The documentation on the operating points is somewhat patchy but I've been
running happily on my systems with the current set of values which is
cobbled together from what I have. One possible issue is that systems
which run with ARMCLK in sync mode seem to need a higher VDDINT. However
these systems would need to explicitly hook up the VDDINT regulator so
the risk to them should be low; I don't have such a system to test on.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/cpufreq/s3c64xx-cpufreq.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index a5e72cb..ca1a249 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -21,20 +21,22 @@
 
 static struct clk *armclk;
 static struct regulator *vddarm;
+static struct regulator *vddint;
 static unsigned long regulator_latency;
 
 #ifdef CONFIG_CPU_S3C6410
 struct s3c64xx_dvfs {
 	unsigned int vddarm_min;
 	unsigned int vddarm_max;
+	unsigned int vddint_min;
 };
 
 static struct s3c64xx_dvfs s3c64xx_dvfs_table[] = {
-	[0] = { 1000000, 1150000 },
-	[1] = { 1050000, 1150000 },
-	[2] = { 1100000, 1150000 },
-	[3] = { 1200000, 1350000 },
-	[4] = { 1300000, 1350000 },
+	[0] = { 1000000, 1150000, 1050000 },
+	[1] = { 1050000, 1150000, 1150000 },
+	[2] = { 1100000, 1150000, 1200000 },
+	[3] = { 1200000, 1350000, 1200000 },
+	[4] = { 1300000, 1350000, 1200000 },
 };
 
 static struct cpufreq_frequency_table s3c64xx_freq_table[] = {
@@ -107,6 +109,16 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 			       freqs.new, ret);
 			goto err;
 		}
+
+		if (vddint) {
+			ret = regulator_set_voltage(vddint,
+						    dvfs->vddint_min,
+						    13500000);
+			if (ret != 0) {
+				pr_err("Failed to set VDDINT for %dkHz: %d\n",
+				       freqs.new, ret);
+			}
+		}
 	}
 #endif
 
@@ -129,6 +141,16 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 			       freqs.new, ret);
 			goto err_clk;
 		}
+
+		if (vddint) {
+			ret = regulator_set_voltage(vddint,
+						    dvfs->vddint_min,
+						    13500000);
+			if (ret != 0) {
+				pr_err("Failed to set VDDINT for %dkHz: %d\n",
+				       freqs.new, ret);
+			}
+		}
 	}
 #endif
 
@@ -259,6 +281,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
 	if (ret != 0) {
 		pr_err("Failed to configure frequency table: %d\n",
 		       ret);
+		regulator_put(vddint);
 		regulator_put(vddarm);
 		clk_put(armclk);
 	}
-- 
1.7.7.3

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Tomasz Figa]

On Monday 05 of December 2011 18:22:02 Mark Brown wrote:
> Save a bit more power by scaling VDDINT as well as VDDARM when we have a
> regulator for it. Don't worry too much if we don't have one, and just log
> errors when scaling as they're unlikely to happen in practice without the
> system being in great trouble and proper handling would complicate the
> code quite a bit.
> 
> The documentation on the operating points is somewhat patchy but I've been
> running happily on my systems with the current set of values which is
> cobbled together from what I have. One possible issue is that systems
> which run with ARMCLK in sync mode seem to need a higher VDDINT. However
> these systems would need to explicitly hook up the VDDINT regulator so
> the risk to them should be low; I don't have such a system to test on.

I would oppose here. According to my information, VDDint depends on peripheral 
clock frequencies not ARM frequency and it should be 1.25V (min 1.15, max 1.3) 
for HCLK at 133 MHz and 1.05V (min 0.95, max 1.3) for HCLK running at 66 MHz.

Anyway, the behavior will probably vary from board to board, so I would rather 
think about extending s3c64xx-cpufreq driver to allow per board 
frequency/voltage configuration., which would also include settings optimized 
for your boards in their board-specific code.

This would also solve a major problem of current s3c64xx-cpufreq 
implementation, namely crashing boards running in synchronous mode, because of 
odd frequencies specified, like 200 MHz (achievable with PLL set to 800 MHz), 
while the requirement is to keep the ARM frequency a multiple of HCLK when 
running synchronously.

I have two boards running in synchronous mode (Samsung Galaxy GT-i5700 phone 
and FriendlyARM Tiny6410 board), I might be able to test things related to 
synchronous mode.

> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/cpufreq/s3c64xx-cpufreq.c |   33 
++++++++++++++++++++++++++++-----
>  1 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c
> b/drivers/cpufreq/s3c64xx-cpufreq.c index a5e72cb..ca1a249 100644
> --- a/drivers/cpufreq/s3c64xx-cpufreq.c
> +++ b/drivers/cpufreq/s3c64xx-cpufreq.c
> @@ -21,20 +21,22 @@
> 
>  static struct clk *armclk;
>  static struct regulator *vddarm;
> +static struct regulator *vddint;
>  static unsigned long regulator_latency;
> 
>  #ifdef CONFIG_CPU_S3C6410
>  struct s3c64xx_dvfs {
>  	unsigned int vddarm_min;
>  	unsigned int vddarm_max;
> +	unsigned int vddint_min;
>  };
> 
>  static struct s3c64xx_dvfs s3c64xx_dvfs_table[] = {
> -	[0] = { 1000000, 1150000 },
> -	[1] = { 1050000, 1150000 },
> -	[2] = { 1100000, 1150000 },
> -	[3] = { 1200000, 1350000 },
> -	[4] = { 1300000, 1350000 },
> +	[0] = { 1000000, 1150000, 1050000 },
> +	[1] = { 1050000, 1150000, 1150000 },
> +	[2] = { 1100000, 1150000, 1200000 },
> +	[3] = { 1200000, 1350000, 1200000 },
> +	[4] = { 1300000, 1350000, 1200000 },
>  };
> 
>  static struct cpufreq_frequency_table s3c64xx_freq_table[] = {
> @@ -107,6 +109,16 @@ static int s3c64xx_cpufreq_set_target(struct
> cpufreq_policy *policy, freqs.new, ret);
>  			goto err;
>  		}
> +
> +		if (vddint) {
> +			ret = regulator_set_voltage(vddint,
> +						    dvfs->vddint_min,
> +						    13500000);
> +			if (ret != 0) {
> +				pr_err("Failed to set VDDINT for %dkHz: %d\n",
> +				       freqs.new, ret);
> +			}
> +		}
>  	}
>  #endif
> 
> @@ -129,6 +141,16 @@ static int s3c64xx_cpufreq_set_target(struct
> cpufreq_policy *policy, freqs.new, ret);
>  			goto err_clk;
>  		}
> +
> +		if (vddint) {
> +			ret = regulator_set_voltage(vddint,
> +						    dvfs->vddint_min,
> +						    13500000);
> +			if (ret != 0) {
> +				pr_err("Failed to set VDDINT for %dkHz: %d\n",
> +				       freqs.new, ret);
> +			}
> +		}
>  	}
>  #endif
> 
> @@ -259,6 +281,7 @@ static int s3c64xx_cpufreq_driver_init(struct
> cpufreq_policy *policy) if (ret != 0) {
>  		pr_err("Failed to configure frequency table: %d\n",
>  		       ret);
> +		regulator_put(vddint);
>  		regulator_put(vddarm);
>  		clk_put(armclk);
>  	}

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Mark Brown]

On Mon, Dec 05, 2011 at 07:45:43PM +0100, Tomasz Figa wrote:

Please fix your mailer to word wrap at less than 80 columns, I've
reflowed your text for legibility.

> I would oppose here. According to my information, VDDint depends on
> peripheral clock frequencies not ARM frequency and it should be 1.25V

That's quite frankly what my electrical engineering knowledge would
suggest but looking at both the extant drivers implementing this (Google
should turn some up) and the documentation I have that doesn't seem to
be the case.  The 1.25V number certainly looks off.

> (min 1.15, max 1.3) for HCLK at 133 MHz and 1.05V (min 0.95, max 1.3)
> for HCLK running at 66 MHz.

I've certainly got documentation that contradicts this, and for some
reason synchronous clocks seem to require a higher VDDINT supply which
baffles me rather.

> Anyway, the behavior will probably vary from board to board, so I would rather 
> think about extending s3c64xx-cpufreq driver to allow per board 
> frequency/voltage configuration., which would also include settings optimized 
> for your boards in their board-specific code.

That seems quite tasteless frankly - the behaviour of the silicon is
not board specific, and we do already have a facility in the regulator
API to compensate for voltage drops in the board if that's an issue.
Anything beyond that is policy and is in the domain of the governors.

The ideal thing would be to be able to scale the HCLK independently of
the ARM clock based on bus loading (taking into account the restrictions
on the relationship between the two clocks) that's very much a long term
goal at this point.

> This would also solve a major problem of current s3c64xx-cpufreq 
> implementation, namely crashing boards running in synchronous mode, because of 
> odd frequencies specified, like 200 MHz (achievable with PLL set to 800 MHz), 
> while the requirement is to keep the ARM frequency a multiple of HCLK when 
> running synchronously.

> I have two boards running in synchronous mode (Samsung Galaxy GT-i5700 phone 
> and FriendlyARM Tiny6410 board), I might be able to test things related to 
> synchronous mode.

The clock code should just be adjusted to refuse to set invalid ARM
clock ratios.

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Tomasz Figa]

On Monday 05 of December 2011 18:57:58 Mark Brown wrote:
> On Mon, Dec 05, 2011 at 07:45:43PM +0100, Tomasz Figa wrote:
> 
> Please fix your mailer to word wrap at less than 80 columns, I've
> reflowed your text for legibility.

Interesting, I had it set to wrap at 78 characters and this seems to match 
with what I have in my Sent mailbox. Anyway, I have switched it to 75 
characters.

> > I would oppose here. According to my information, VDDint depends on
> > peripheral clock frequencies not ARM frequency and it should be 1.25V
> 
> That's quite frankly what my electrical engineering knowledge would
> suggest but looking at both the extant drivers implementing this (Google
> should turn some up) and the documentation I have that doesn't seem to
> be the case.  The 1.25V number certainly looks off.
>
> > (min 1.15, max 1.3) for HCLK at 133 MHz and 1.05V (min 0.95, max 1.3)
> > for HCLK running at 66 MHz.
> 
> I've certainly got documentation that contradicts this, and for some
> reason synchronous clocks seem to require a higher VDDINT supply which
> baffles me rather.

I'm basing my statements on S3C6410 power design guide and source code of 
original kernel for Samsung Galaxy GT-i5700 released by Samsung on their 
open source portal.
 
> > Anyway, the behavior will probably vary from board to board, so I
> > would rather think about extending s3c64xx-cpufreq driver to allow
> > per board frequency/voltage configuration., which would also include
> > settings optimized for your boards in their board-specific code.
> 
> That seems quite tasteless frankly - the behaviour of the silicon is
> not board specific, and we do already have a facility in the regulator
> API to compensate for voltage drops in the board if that's an issue.
> Anything beyond that is policy and is in the domain of the governors.

Well, you are mostly right, but there is one aspect that varies between 
boards, configuration. Boards can have different PLL settings, run in 
synchronous or asynchronous mode and might have power regulators of 
different qualities which will vary in voltage ripple.

For example, in case of your boards, the regulator is nice and allow 
reduction of VDDint voltage to minimal value (1.15V), but in general Power 
Design Guide it is recommended to use the typical value (1.25V).

As far as I know, there are also different revisions of S3C6410, capable of 
running at different maximal frequencies, at least 667 MHz and 800 MHz 
versions.
 
> The ideal thing would be to be able to scale the HCLK independently of
> the ARM clock based on bus loading (taking into account the restrictions
> on the relationship between the two clocks) that's very much a long term
> goal at this point.
> 
> > This would also solve a major problem of current s3c64xx-cpufreq
> > implementation, namely crashing boards running in synchronous mode,
> > because of odd frequencies specified, like 200 MHz (achievable with
> > PLL set to 800 MHz), while the requirement is to keep the ARM
> > frequency a multiple of HCLK when running synchronously.
> > 
> > I have two boards running in synchronous mode (Samsung Galaxy
> > GT-i5700 phone and FriendlyARM Tiny6410 board), I might be able to
> > test things related to synchronous mode.
> 
> The clock code should just be adjusted to refuse to set invalid ARM
> clock ratios.

This might be some solution as well. I'm not yet sure if it solves all the 
problems, but I will think about it.

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Mark Brown]

On Mon, Dec 05, 2011 at 08:11:03PM +0100, Tomasz Figa wrote:
> On Monday 05 of December 2011 18:57:58 Mark Brown wrote:

> > > (min 1.15, max 1.3) for HCLK at 133 MHz and 1.05V (min 0.95, max 1.3)
> > > for HCLK running at 66 MHz.

> > I've certainly got documentation that contradicts this, and for some
> > reason synchronous clocks seem to require a higher VDDINT supply which
> > baffles me rather.

> I'm basing my statements on S3C6410 power design guide and source code of 
> original kernel for Samsung Galaxy GT-i5700 released by Samsung on their 
> open source portal.

So, if you look at the power design guide (or at least the version I
have) that doesn't quite match that - for example, the recommended
operating conditions section quotes 133MHz as 1.2V typical for async
operation and 1.3V typical for sync operation.  There's also some
examples in the power guide showing periods of operation with HCLK at
133MHz and VDDINT at 1.05V.  And driver code showing some variation from
the power design guide :/

> > > Anyway, the behavior will probably vary from board to board, so I
> > > would rather think about extending s3c64xx-cpufreq driver to allow
> > > per board frequency/voltage configuration., which would also include
> > > settings optimized for your boards in their board-specific code.

> > That seems quite tasteless frankly - the behaviour of the silicon is
> > not board specific, and we do already have a facility in the regulator
> > API to compensate for voltage drops in the board if that's an issue.
> > Anything beyond that is policy and is in the domain of the governors.

> Well, you are mostly right, but there is one aspect that varies between 
> boards, configuration. Boards can have different PLL settings, run in 
> synchronous or asynchronous mode and might have power regulators of 
> different qualities which will vary in voltage ripple.

The regulators we should be able to take care of through the framework;
it's a moderately common issue on brass board designs due to the
physically larger form factors.  The PLL settings we'd ideally be able
to change at runtime (especially the ARM one which would give us a lot
more flexibility) and at worst should be able to figure out what to do
with based on readback.

> For example, in case of your boards, the regulator is nice and allow 
> reduction of VDDint voltage to minimal value (1.15V), but in general Power 
> Design Guide it is recommended to use the typical value (1.25V).

If it's a question of regulator quality the board should just be adding
an offset in the constraints without the SoC drivers having to worry
their pretty little heads about it.  Like I say it's not an unknown
problem.  Often it's not actually the regulator but rather the board
design that causes the issue, for example if you've got very long traces
from the regulator to the device then you may experience voltage drops
or other effects which impair the accuracy of the supply seen by the
device.

> As far as I know, there are also different revisions of S3C6410, capable of 
> running at different maximal frequencies, at least 667 MHz and 800 MHz 
> versions.

Yes, there are - I've got boards with both variants.  At the minute
we're managing to cope with them by virtue of the different PLL settings
constraining the frequencies we can set, if we ever get control of the
ARM PLL the drivers will need to get a bit smarter.

> > > This would also solve a major problem of current s3c64xx-cpufreq
> > > implementation, namely crashing boards running in synchronous mode,
> > > because of odd frequencies specified, like 200 MHz (achievable with
> > > PLL set to 800 MHz), while the requirement is to keep the ARM
> > > frequency a multiple of HCLK when running synchronously.

> > The clock code should just be adjusted to refuse to set invalid ARM
> > clock ratios.

> This might be some solution as well. I'm not yet sure if it solves all the 
> problems, but I will think about it.

It'd at least stop the system falling over in a big fat heap which seems
like an improvement :)

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Tomasz Figa]

On Monday 05 of December 2011 19:25:06 Mark Brown wrote:
> On Mon, Dec 05, 2011 at 08:11:03PM +0100, Tomasz Figa wrote:
> > On Monday 05 of December 2011 18:57:58 Mark Brown wrote:
> > > > (min 1.15, max 1.3) for HCLK at 133 MHz and 1.05V (min 0.95,
> > > > max 1.3) for HCLK running at 66 MHz.
> > > 
> > > I've certainly got documentation that contradicts this, and for
> > > some
> > > reason synchronous clocks seem to require a higher VDDINT supply
> > > which baffles me rather.
> > 
> > I'm basing my statements on S3C6410 power design guide and source
> > code of original kernel for Samsung Galaxy GT-i5700 released by
> > Samsung on their open source portal.
> 
> So, if you look at the power design guide (or at least the version I
> have) that doesn't quite match that - for example, the recommended
> operating conditions section quotes 133MHz as 1.2V typical for async
> operation and 1.3V typical for sync operation.  There's also some
> examples in the power guide showing periods of operation with HCLK at
> 133MHz and VDDINT at 1.05V.  And driver code showing some variation from
> the power design guide :/

I think that people from Samsung should be able to say something a bit more 
precise on this. Am I right?

> > > > Anyway, the behavior will probably vary from board to board,
> > > > so I
> > > > would rather think about extending s3c64xx-cpufreq driver to
> > > > allow
> > > > per board frequency/voltage configuration., which would also
> > > > include settings optimized for your boards in their
> > > > board-specific code.> > 
> > > That seems quite tasteless frankly - the behaviour of the silicon
> > > is
> > > not board specific, and we do already have a facility in the
> > > regulator API to compensate for voltage drops in the board if
> > > that's an issue. Anything beyond that is policy and is in the
> > > domain of the governors.> 
> > Well, you are mostly right, but there is one aspect that varies
> > between boards, configuration. Boards can have different PLL
> > settings, run in synchronous or asynchronous mode and might have
> > power regulators of different qualities which will vary in voltage
> > ripple.
> 
> The regulators we should be able to take care of through the framework;
> it's a moderately common issue on brass board designs due to the
> physically larger form factors.  The PLL settings we'd ideally be able
> to change at runtime (especially the ARM one which would give us a lot
> more flexibility) and at worst should be able to figure out what to do
> with based on readback.

OK.

> > For example, in case of your boards, the regulator is nice and allow
> > reduction of VDDint voltage to minimal value (1.15V), but in general
> > Power Design Guide it is recommended to use the typical value
> > (1.25V).
> If it's a question of regulator quality the board should just be adding
> an offset in the constraints without the SoC drivers having to worry
> their pretty little heads about it.  Like I say it's not an unknown
> problem.  Often it's not actually the regulator but rather the board
> design that causes the issue, for example if you've got very long traces
> from the regulator to the device then you may experience voltage drops
> or other effects which impair the accuracy of the supply seen by the
> device.

Right, there is a field for regulator offset in regulation_constraints 
struct indeed. Somehow I missed it, sorry.

> > As far as I know, there are also different revisions of S3C6410,
> > capable of running at different maximal frequencies, at least 667
> > MHz and 800 MHz versions.
> 
> Yes, there are - I've got boards with both variants.  At the minute
> we're managing to cope with them by virtue of the different PLL settings
> constraining the frequencies we can set, if we ever get control of the
> ARM PLL the drivers will need to get a bit smarter.

OK.

> > > > This would also solve a major problem of current
> > > > s3c64xx-cpufreq
> > > > implementation, namely crashing boards running in synchronous
> > > > mode, because of odd frequencies specified, like 200 MHz
> > > > (achievable with PLL set to 800 MHz), while the requirement
> > > > is to keep the ARM frequency a multiple of HCLK when running
> > > > synchronously.
> > > 
> > > The clock code should just be adjusted to refuse to set invalid
> > > ARM
> > > clock ratios.
> > 
> > This might be some solution as well. I'm not yet sure if it solves
> > all the problems, but I will think about it.
> 
> It'd at least stop the system falling over in a big fat heap which seems
> like an improvement :)

OK. :)

The problem with voltages would be solved by offset given in constraints 
and the problem of incorrect frequencies should be fixable in s3c64xx-
cpufreq, so no more problems left. I think.

[PATCH 2/2] [CPUFREQ] s3c64xx: Add VDDINT voltage scaling

[Mark Brown]

On Mon, Dec 05, 2011 at 08:45:47PM +0100, Tomasz Figa wrote:
> On Monday 05 of December 2011 19:25:06 Mark Brown wrote:

> > 133MHz and VDDINT at 1.05V.  And driver code showing some variation from
> > the power design guide :/

> I think that people from Samsung should be able to say something a bit more 
> precise on this. Am I right?

Here's hoping :)

> > If it's a question of regulator quality the board should just be adding
> > an offset in the constraints without the SoC drivers having to worry
> > their pretty little heads about it.  Like I say it's not an unknown

> Right, there is a field for regulator offset in regulation_constraints 
> struct indeed. Somehow I missed it, sorry.

It's possible you first looked at this with a kernel that didn't have
that feature - I added it sometime this year.