[RFC] omap3: Enable SmartReflex calculations for 720MHz

[RFC] omap3: Enable SmartReflex calculations for 720MHz

[Sanjeev Premi]

The eFuse registers do not contain the nValue to be used
with 720MHz (OPP6). This patch implements procedure to
calculate the nValue(OPP6) based on the nValue(OPP5).

  [1] SPRUFF1D-June 2009 section 1.5.2.1.1

This is first attempt to fit this mechanism into new
smartreflex framework. Do note a FIXME which is added
to illustrate the calculations; and express need for
a mechanism to get nValues for each OPP.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/sr_device.c           |   54 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/voltage.c             |    1 +
 arch/arm/plat-omap/include/plat/voltage.h |    1 +
 3 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
index 786d685..43640a3 100644
--- a/arch/arm/mach-omap2/sr_device.c
+++ b/arch/arm/mach-omap2/sr_device.c
@@ -38,6 +38,51 @@ static struct omap_device_pm_latency omap_sr_latency[] = {
 	},
 };
 
+static void swcalc_opp6_RG(u32 rFuse, u32 gainFuse, u32 deltaNT,
+                               u32* rAdj, u32* gainAdj)
+{
+       u32 nAdj;
+       u32 g, r;
+
+       nAdj = ((1 << (gainFuse + 8))/rFuse) + deltaNT;
+
+       for (g = 0; g < GAIN_MAXLIMIT; g++) {
+               r = (1 << (g + 8)) / nAdj;
+               if (r < 256) {
+                       *rAdj = r;
+                       *gainAdj = g;
+               }
+       }
+}
+
+#define SWCALC_OPP6_DELTA_NNT	379
+#define SWCALC_OPP6_DELTA_PNT	227
+#define GAIN_MAXLIMIT		16
+
+static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)
+{
+       u32 opp6_nvalue;
+       u32 opp5_senPgain, opp5_senNgain, opp5_senPRN, opp5_senNRN;
+       u32 opp6_senPgain, opp6_senNgain, opp6_senPRN, opp6_senNRN;
+
+       opp5_senPgain = (opp5_nvalue & 0x00f00000) >> 0x14;
+       opp5_senNgain = (opp5_nvalue & 0x000f0000) >> 0x10;
+
+       opp5_senPRN = (opp5_nvalue & 0x0000ff00) >> 0x8;
+       opp5_senNRN = (opp5_nvalue & 0x000000ff);
+
+       swcalc_opp6_RG(opp5_senNRN, opp5_senNgain, SWCALC_OPP6_DELTA_NNT,
+                               &opp6_senNRN, &opp6_senNgain);
+
+       swcalc_opp6_RG(opp5_senPRN, opp5_senPgain, SWCALC_OPP6_DELTA_PNT,
+                               &opp6_senPRN, &opp6_senPgain);
+
+       opp6_nvalue = (opp6_senPgain << 0x14) | (opp6_senNgain < 0x10) |
+                       (opp6_senPRN << 0x8) | opp6_senNRN;
+
+       return opp6_nvalue;
+}
+
 /* Read EFUSE values from control registers for OMAP3430 */
 static void __init sr_set_nvalues(struct omap_volt_data *volt_data,
 				struct omap_sr_data *sr_data)
@@ -72,6 +117,15 @@ static void __init sr_set_nvalues(struct omap_volt_data *volt_data,
 		nvalue_table[i].nvalue = v;
 	}
 
+	/*
+	 * FIXME: This is a temporary hack. Need to identify better
+	 *        mechanism to find nvalues corresponding to an OPP.
+	 */
+	if (cpu_is_omap34xx() && omap3_has_720mhz()) {
+		nvalue_table[count-1].nvalue = swcalc_opp6_nvalue(
+					nvalue_table[count-2].nvalue);
+	}
+
 	sr_data->nvalue_table = nvalue_table;
 	sr_data->nvalue_count = count;
 }
diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index ed6079c..f23b6d7 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -258,6 +258,7 @@ static struct omap_volt_data omap34xx_vddmpu_volt_data[] = {
 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP3_UV, OMAP343X_CONTROL_FUSE_OPP3_VDD1, 0xf9, 0x18),
 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP4_UV, OMAP343X_CONTROL_FUSE_OPP4_VDD1, 0xf9, 0x18),
 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP5_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
+	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP6_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
 	VOLT_DATA_DEFINE(0, 0, 0, 0),
 };
 
diff --git a/arch/arm/plat-omap/include/plat/voltage.h b/arch/arm/plat-omap/include/plat/voltage.h
index 0ff1233..f3f87a6 100644
--- a/arch/arm/plat-omap/include/plat/voltage.h
+++ b/arch/arm/plat-omap/include/plat/voltage.h
@@ -31,6 +31,7 @@
 #define OMAP3430_VDD_MPU_OPP3_UV		1200000
 #define OMAP3430_VDD_MPU_OPP4_UV		1270000
 #define OMAP3430_VDD_MPU_OPP5_UV		1350000
+#define OMAP3430_VDD_MPU_OPP6_UV                1350000
 
 #define OMAP3430_VDD_CORE_OPP1_UV		975000
 #define OMAP3430_VDD_CORE_OPP2_UV		1050000
-- 
1.7.2.2

Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Nishanth Menon]

Sanjeev Premi had written, on 01/07/2011 11:26 AM, the following:
> The eFuse registers do not contain the nValue to be used
> with 720MHz (OPP6). This patch implements procedure to
> calculate the nValue(OPP6) based on the nValue(OPP5).
> 
>   [1] SPRUFF1D-June 2009 section 1.5.2.1.1
Where is this doc?
http://focus-webapps.ti.com/general/docs/sitesearch/searchsite.tsp;jsessionid=TKKPEXXTMNJCPQC1JAWBVQQ?selectedTopic=1653260327&numRecords=25&searchTerm=SPRUFF1D&statusCode=null

Google seems to point to:
http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
but ti.com responds with document not found.


> 
> This is first attempt to fit this mechanism into new
> smartreflex framework. Do note a FIXME which is added
> to illustrate the calculations; and express need for
> a mechanism to get nValues for each OPP.
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  arch/arm/mach-omap2/sr_device.c           |   54 +++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/voltage.c             |    1 +
>  arch/arm/plat-omap/include/plat/voltage.h |    1 +
>  3 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
> index 786d685..43640a3 100644
> --- a/arch/arm/mach-omap2/sr_device.c
> +++ b/arch/arm/mach-omap2/sr_device.c
> @@ -38,6 +38,51 @@ static struct omap_device_pm_latency omap_sr_latency[] = {
>  	},
>  };
>  
> +static void swcalc_opp6_RG(u32 rFuse, u32 gainFuse, u32 deltaNT,
> +                               u32* rAdj, u32* gainAdj)
> +{
> +       u32 nAdj;
> +       u32 g, r;
> +
> +       nAdj = ((1 << (gainFuse + 8))/rFuse) + deltaNT;
> +
> +       for (g = 0; g < GAIN_MAXLIMIT; g++) {
> +               r = (1 << (g + 8)) / nAdj;
> +               if (r < 256) {
> +                       *rAdj = r;
> +                       *gainAdj = g;
> +               }
> +       }
> +}
> +
> +#define SWCALC_OPP6_DELTA_NNT	379
> +#define SWCALC_OPP6_DELTA_PNT	227
> +#define GAIN_MAXLIMIT		16

Magic numbers - do they scale from 3430 to 3630?

> +
> +static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)
so what does this actually do?
> +{
> +       u32 opp6_nvalue;
> +       u32 opp5_senPgain, opp5_senNgain, opp5_senPRN, opp5_senNRN;
> +       u32 opp6_senPgain, opp6_senNgain, opp6_senPRN, opp6_senNRN;
> +
> +       opp5_senPgain = (opp5_nvalue & 0x00f00000) >> 0x14;
> +       opp5_senNgain = (opp5_nvalue & 0x000f0000) >> 0x10;
> +
> +       opp5_senPRN = (opp5_nvalue & 0x0000ff00) >> 0x8;
> +       opp5_senNRN = (opp5_nvalue & 0x000000ff);
> +
> +       swcalc_opp6_RG(opp5_senNRN, opp5_senNgain, SWCALC_OPP6_DELTA_NNT,
> +                               &opp6_senNRN, &opp6_senNgain);
> +
> +       swcalc_opp6_RG(opp5_senPRN, opp5_senPgain, SWCALC_OPP6_DELTA_PNT,
> +                               &opp6_senPRN, &opp6_senPgain);
> +
> +       opp6_nvalue = (opp6_senPgain << 0x14) | (opp6_senNgain < 0x10) |
> +                       (opp6_senPRN << 0x8) | opp6_senNRN;
> +
> +       return opp6_nvalue;
> +}
> +
>  /* Read EFUSE values from control registers for OMAP3430 */
>  static void __init sr_set_nvalues(struct omap_volt_data *volt_data,
>  				struct omap_sr_data *sr_data)
> @@ -72,6 +117,15 @@ static void __init sr_set_nvalues(struct omap_volt_data *volt_data,
>  		nvalue_table[i].nvalue = v;
>  	}
>  
> +	/*
> +	 * FIXME: This is a temporary hack. Need to identify better
> +	 *        mechanism to find nvalues corresponding to an OPP.
> +	 */
> +	if (cpu_is_omap34xx() && omap3_has_720mhz()) {
> +		nvalue_table[count-1].nvalue = swcalc_opp6_nvalue(
> +					nvalue_table[count-2].nvalue);
> +	}
> +
Sorry, Dumb question:
a) You are using OPP5's nTarget to use in OPP6's nTarget? is'nt it fused 
in for OPP6 offset?
b) you are using OMAP343X_CONTROL_FUSE_OPP5_VDD1 itself
c) these should be __init functions.
d) how would you allow this code to work with 3440?
e) next time could you please try STOP using CaMELCASInG in your 
variables and functions?
f) please add sufficient documentation in kernel-doc style to allow us 
to understand the story below?
g) dont use magic numbers
h) please take care of ROUND_DOWN and ROUND_UP and truncation errors
i) is this equation ONLY valid for opp5 ntarget modification on 
OMAP3530? or valid for 3440 as well? or valid for any OPP ntarget 
modification?

>  	sr_data->nvalue_table = nvalue_table;
>  	sr_data->nvalue_count = count;
>  }
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index ed6079c..f23b6d7 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -258,6 +258,7 @@ static struct omap_volt_data omap34xx_vddmpu_volt_data[] = {
>  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP3_UV, OMAP343X_CONTROL_FUSE_OPP3_VDD1, 0xf9, 0x18),
>  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP4_UV, OMAP343X_CONTROL_FUSE_OPP4_VDD1, 0xf9, 0x18),
>  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP5_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP6_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
you might as well use OPP6_VDD1 for CONTROL_FUSE?

>  	VOLT_DATA_DEFINE(0, 0, 0, 0),
>  };
>  
> diff --git a/arch/arm/plat-omap/include/plat/voltage.h b/arch/arm/plat-omap/include/plat/voltage.h
> index 0ff1233..f3f87a6 100644
> --- a/arch/arm/plat-omap/include/plat/voltage.h
> +++ b/arch/arm/plat-omap/include/plat/voltage.h
> @@ -31,6 +31,7 @@
>  #define OMAP3430_VDD_MPU_OPP3_UV		1200000
>  #define OMAP3430_VDD_MPU_OPP4_UV		1270000
>  #define OMAP3430_VDD_MPU_OPP5_UV		1350000
> +#define OMAP3430_VDD_MPU_OPP6_UV                1350000
>  
>  #define OMAP3430_VDD_CORE_OPP1_UV		975000
>  #define OMAP3430_VDD_CORE_OPP2_UV		1050000


-- 
Regards,
Nishanth Menon

Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Koen Kooi]

Op 7 jan 2011, om 18:26 heeft Sanjeev Premi het volgende geschreven:

> The eFuse registers do not contain the nValue to be used
> with 720MHz (OPP6). This patch implements procedure to
> calculate the nValue(OPP6) based on the nValue(OPP5).
> 
>  [1] SPRUFF1D-June 2009 section 1.5.2.1.1
> 
> This is first attempt to fit this mechanism into new
> smartreflex framework. Do note a FIXME which is added
> to illustrate the calculations; and express need for
> a mechanism to get nValues for each OPP.
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
> arch/arm/mach-omap2/sr_device.c           |   54 +++++++++++++++++++++++++++++
> arch/arm/mach-omap2/voltage.c             |    1 +
> arch/arm/plat-omap/include/plat/voltage.h |    1 +
> 3 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
> index 786d685..43640a3 100644
> --- a/arch/arm/mach-omap2/sr_device.c
> +++ b/arch/arm/mach-omap2/sr_device.c
> @@ -38,6 +38,51 @@ static struct omap_device_pm_latency omap_sr_latency[] = {
> 	},
> };
> 
> +static void swcalc_opp6_RG(u32 rFuse, u32 gainFuse, u32 deltaNT,
> +                               u32* rAdj, u32* gainAdj)
> +{
> +       u32 nAdj;
> +       u32 g, r;
> +
> +       nAdj = ((1 << (gainFuse + 8))/rFuse) + deltaNT;
> +
> +       for (g = 0; g < GAIN_MAXLIMIT; g++) {

GAIN_MAXLIMIT is undeclared at this point:

arch/arm/mach-omap2/sr_device.c: In function ‘swcalc_opp6_RG’:
arch/arm/mach-omap2/sr_device.c:49: error: ‘GAIN_MAXLIMIT’ undeclared (first use in this function)


> +               r = (1 << (g + 8)) / nAdj;
> +               if (r < 256) {
> +                       *rAdj = r;
> +                       *gainAdj = g;
> +               }
> +       }
> +}
> +
> +#define SWCALC_OPP6_DELTA_NNT	379
> +#define SWCALC_OPP6_DELTA_PNT	227
> +#define GAIN_MAXLIMIT		16

So these have to move up at bit.

regards,

Koen--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Premi, Sanjeev]

_______________________________________
From: Koen Kooi [koen@dominion.thruhere.net]
Sent: Saturday, January 08, 2011 3:42 AM
To: Premi, Sanjeev
Cc: linux-omap@vger.kernel.org
Subject: Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz

Op 7 jan 2011, om 18:26 heeft Sanjeev Premi het volgende geschreven:

> The eFuse registers do not contain the nValue to be used
> with 720MHz (OPP6). This patch implements procedure to
> calculate the nValue(OPP6) based on the nValue(OPP5).
>
>  [1] SPRUFF1D-June 2009 section 1.5.2.1.1
>
> This is first attempt to fit this mechanism into new
> smartreflex framework. Do note a FIXME which is added
> to illustrate the calculations; and express need for
> a mechanism to get nValues for each OPP.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
> arch/arm/mach-omap2/sr_device.c           |   54 +++++++++++++++++++++++++++++
> arch/arm/mach-omap2/voltage.c             |    1 +
> arch/arm/plat-omap/include/plat/voltage.h |    1 +
> 3 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach-omap2/sr_device.c
> index 786d685..43640a3 100644
> --- a/arch/arm/mach-omap2/sr_device.c
> +++ b/arch/arm/mach-omap2/sr_device.c
> @@ -38,6 +38,51 @@ static struct omap_device_pm_latency omap_sr_latency[] = {
>       },
> };
>
> +static void swcalc_opp6_RG(u32 rFuse, u32 gainFuse, u32 deltaNT,
> +                               u32* rAdj, u32* gainAdj)
> +{
> +       u32 nAdj;
> +       u32 g, r;
> +
> +       nAdj = ((1 << (gainFuse + 8))/rFuse) + deltaNT;
> +
> +       for (g = 0; g < GAIN_MAXLIMIT; g++) {

GAIN_MAXLIMIT is undeclared at this point:

arch/arm/mach-omap2/sr_device.c: In function ‘swcalc_opp6_RG’:
arch/arm/mach-omap2/sr_device.c:49: error: ‘GAIN_MAXLIMIT’ undeclared (first use in this function)


> +               r = (1 << (g + 8)) / nAdj;
> +               if (r < 256) {
> +                       *rAdj = r;
> +                       *gainAdj = g;
> +               }
> +       }
> +}
> +
> +#define SWCALC_OPP6_DELTA_NNT        379
> +#define SWCALC_OPP6_DELTA_PNT        227
> +#define GAIN_MAXLIMIT                16

So these have to move up at bit.

[sp] Thanks for catching this. For purpose of minimal changes I moved this decl
manually from top of file leading to the error.

regards,

Koen
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Koen Kooi]

Op 7 jan 2011, om 18:26 heeft Sanjeev Premi het volgende geschreven:

> The eFuse registers do not contain the nValue to be used
> with 720MHz (OPP6). This patch implements procedure to
> calculate the nValue(OPP6) based on the nValue(OPP5).

[..]

> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -258,6 +258,7 @@ static struct omap_volt_data omap34xx_vddmpu_volt_data[] = {
> 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP3_UV, OMAP343X_CONTROL_FUSE_OPP3_VDD1, 0xf9, 0x18),
> 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP4_UV, OMAP343X_CONTROL_FUSE_OPP4_VDD1, 0xf9, 0x18),
> 	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP5_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP6_UV, OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> 	VOLT_DATA_DEFINE(0, 0, 0, 0),
> };

When using the 720MHz patches on top of Thara's DVFS patches it highlights some bugs in the voltage code. When testing on an USRP-E100 with an Gumstix Overo Tide inside:

root@usrp-e1xx:~# uname -a
Linux usrp-e1xx 2.6.37-rc8+ #87 PREEMPT Sat Jan 8 09:58:26 CET 2011 armv7l GNU/Linux
root@usrp-e1xx:~# cat /proc/cmdline 
console=ttyO2,115200n8 mpurate=720 [..] root=/dev/mmcblk0p2 rw rootfstype=ext3 rootwait

root@usrp-e1xx:~# cat /proc/cpuinfo  | grep MIPS
BogoMIPS	: 717.37

So right after boot it's running at 720MHz, good.

root@usrp-e1xx:~# cpufreq-set -g ondemand ; sleep 1 ; cpufreq-set -g performance
root@usrp-e1xx:~# dmesg | grep -e platform -e omap
[ 5281.306762] cpufreq-omap: transition: 720000 --> 125000
[ 5282.017913] cpufreq-omap: transition: 125000 --> 720000
[ 5282.019409] cpufreq-omap: transition: 720000 --> 720000
[ 5282.019439] platform iva.0: omap_voltage_scale: Already at the requestedrate 430000000
[ 5282.029998] platform mpu.0: omap_voltage_scale: Already at the requestedrate 600000000
root@usrp-e1xx:~# cpufreq-info 
cpufrequtils 006: cpufreq-info (C) Dominik Brodowski 2004-2009
Report errors and bugs to cpufreq@vger.kernel.org, please.
analyzing CPU 0:
  driver: omap
  CPUs which run at the same hardware frequency: 0
  CPUs which need to have their frequency coordinated by software: 0
  maximum transition latency: 300 us.
  hardware limits: 125 MHz - 720 MHz
  available frequency steps: 125 MHz, 250 MHz, 500 MHz, 550 MHz, 600 MHz, 720 MHz
  available cpufreq governors: conservative, ondemand, userspace, powersave, performance
  current policy: frequency should be within 125 MHz and 720 MHz.
                  The governor "performance" may decide which speed to use
                  within this range.
  current CPU frequency is 600 MHz (asserted by call to hardware).
  cpufreq stats: 125 MHz:0.13%, 250 MHz:0.00%, 500 MHz:0.00%, 550 MHz:0.00%, 600 MHz:0.00%, 720 MHz:99.87%  (10)

It seems that because the voltages are the same for OPP5 and OPP6 it doesn't want to switch back to 720MHz.  But the strange this is:

root@usrp-e1xx:~# cat /proc/cpuinfo  | grep MIPS
BogoMIPS	: 717.37

I haven't run any benchmarks yet to see what the actual frequency is, but the above should demonstrate that things don't work quite right. 

regards,

Koen

RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Premi, Sanjeev]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com] 
> Sent: Saturday, January 08, 2011 1:25 AM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz
> 
> Sanjeev Premi had written, on 01/07/2011 11:26 AM, the following:

[snip]...[snip]

> Sorry, Dumb question:
> a) You are using OPP5's nTarget to use in OPP6's nTarget? 
> is'nt it fused 
> in for OPP6 offset?
> b) you are using OMAP343X_CONTROL_FUSE_OPP5_VDD1 itself
> c) these should be __init functions.
> d) how would you allow this code to work with 3440?
> e) next time could you please try STOP using CaMELCASInG in your 
> variables and functions?
> f) please add sufficient documentation in kernel-doc style to 
> allow us 
> to understand the story below?
> g) dont use magic numbers
> h) please take care of ROUND_DOWN and ROUND_UP and truncation errors
> i) is this equation ONLY valid for opp5 ntarget modification on 
> OMAP3530? or valid for 3440 as well? or valid for any OPP ntarget 
> modification?
> 

Under garb of asking dumb questions, you seem to be exposing yourself as one!!
(Specifically since we exchanged multiple private emails on this topic yesterday)

You have completely missed the whole intent of this RFC:

1) Look for possible problems that might arise because adding additional entry
   in the omap34xx_vddmpu_volt_data[]

   I didn't thinks so, since the array is static data and currently this array
   size is related to the array holding nValues for each OPP.

2) Look for advice on (or work in procress if) any that may help fix the kludge
   in the code section marked FIXME where better mechanism is required to extract
   the nvalues corresponding to OPP5.

3) Use this weekend to esp the lead time available due to time difference to have
   something valuable to start working on the 

I would have loved to see coding and documentation related comments had this been
a formal patch.

I will respond to some of other 'valid' queries in separate mail.

Have tried to stop myself a lot before, but couldn't find a better way to
respond :)

~sanjeev

[snip]...[snip]

RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Premi, Sanjeev]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com] 
> Sent: Saturday, January 08, 2011 1:25 AM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz
> 
> Sanjeev Premi had written, on 01/07/2011 11:26 AM, the following:
> > The eFuse registers do not contain the nValue to be used
> > with 720MHz (OPP6). This patch implements procedure to
> > calculate the nValue(OPP6) based on the nValue(OPP5).
> > 
> >   [1] SPRUFF1D-June 2009 section 1.5.2.1.1
> Where is this doc?
> http://focus-webapps.ti.com/general/docs/sitesearch/searchsite
> .tsp;jsessionid=TKKPEXXTMNJCPQC1JAWBVQQ?selectedTopic=16532603
> 27&numRecords=25&searchTerm=SPRUFF1D&statusCode=null
> 
> Google seems to point to:
> http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
> but ti.com responds with document not found.
> 

[sp] Thanks for pointing out. It is not accessible for some reasons.
     It can be...  and is being looked into as i write this message.

[snip]...[snip]

> > +#define SWCALC_OPP6_DELTA_NNT	379
> > +#define SWCALC_OPP6_DELTA_PNT	227
> > +#define GAIN_MAXLIMIT		16
> 
> Magic numbers - do they scale from 3430 to 3630?

[sp] The patch is meant for 720MHz - which is applicable for 3430 only.
     So, these values are meant for 3430.

[snip]...[snip]

> 
> > +
> > +static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)
> so what does this actually do?

[sp] Returns a nvalue which corresponds to opp6 based on input which
     is nvalue of opp5. Elementary?

     I don't think it is as bad as: u32 process (u32 n)

> > +{

[snip]...[snip]

> > +       opp6_nvalue = (opp6_senPgain << 0x14) | 
> (opp6_senNgain < 0x10) |
> > +                       (opp6_senPRN << 0x8) | opp6_senNRN;
> > +
> > +       return opp6_nvalue;

[sp] Elementary, if you preferred to read the code. Rather that glossing
     over in hurry to respond compulsively. 

> > +}

[snip]...[snip]

> Sorry, Dumb question:
> a) You are using OPP5's nTarget to use in OPP6's nTarget? 
> is'nt it fused 
> in for OPP6 offset?

[sp] Again, in hurry to respond, you missed the description for the patch
     [quote]
     The eFuse registers do not contain the nValue to be used
     with 720MHz (OPP6). This patch implements procedure to
     calculate the nValue(OPP6) based on the nValue(OPP5).
     [/quote]

> b) you are using OMAP343X_CONTROL_FUSE_OPP5_VDD1 itself

[sp] Quoting the description:
     [quote]
     The eFuse registers do not contain the nValue to be used
     with 720MHz (OPP6). This patch implements procedure to
     calculate the nValue(OPP6) based on the nValue(OPP5).
     [/quote]

     Repeating again, just in case you missed: nvalues for OPP6 are
     calculated based on the nvalues for OPP5. So, i need to use
     OMAP343X_CONTROL_FUSE_OPP5_VDD1.

> c) these should be __init functions.

[sp] They will be when i post formal patch.

> d) how would you allow this code to work with 3440?

[sp] Does 3440 have same behavior? If so, it is quite easy by
     updating this check.
     [quote]
     +	if (cpu_is_omap34xx() && omap3_has_720mhz()) {
     +		nvalue_table[count-1].nvalue = swcalc_opp6_nvalue(
     +					nvalue_table[count-2].nvalue);
     +	}
     [/quote]

     However, i am not conversant with 3440. This will have to be
     covered by a separate patch.

> e) next time could you please try STOP using CaMELCASInG in your 
> variables and functions?

[sp] Did you ever notice any camelcase in any of my formal patch
     submissions earlier. 10 brownie points for catching it here!

> f) please add sufficient documentation in kernel-doc style to 
> allow us 
> to understand the story below?
[sp] Is there anything more to "story" than mentioned in the patch
     description and the SPRU referenced.
     
     It is not accessible for some reasons - may be re-indexing
     done last week. But that can be... and is being looked into
     as i write this message.

     If something isn't clear, can you be more specific? I'd rather
     write code than stories!

> g) dont use magic numbers

[sp] Some magin numbers here cannot be avoided. Specifically in shift
     operations as they are (possibly) optimization for costly multiple
     and divide operations.

> h) please take care of ROUND_DOWN and ROUND_UP and truncation errors

[sp] Where?

> i) is this equation ONLY valid for opp5 ntarget modification on 
> OMAP3530? or valid for 3440 as well? or valid for any OPP ntarget 
> modification?

[sp] Repeating the patch description here would have helped you;
     but may not be convenient for other reviewers. Can you browse
     to top of this mail again and read the description again?

> 
> >  	sr_data->nvalue_table = nvalue_table;
> >  	sr_data->nvalue_count = count;
> >  }
> > diff --git a/arch/arm/mach-omap2/voltage.c 
> b/arch/arm/mach-omap2/voltage.c
> > index ed6079c..f23b6d7 100644
> > --- a/arch/arm/mach-omap2/voltage.c
> > +++ b/arch/arm/mach-omap2/voltage.c
> > @@ -258,6 +258,7 @@ static struct omap_volt_data 
> omap34xx_vddmpu_volt_data[] = {
> >  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP3_UV, 
> OMAP343X_CONTROL_FUSE_OPP3_VDD1, 0xf9, 0x18),
> >  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP4_UV, 
> OMAP343X_CONTROL_FUSE_OPP4_VDD1, 0xf9, 0x18),
> >  	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP5_UV, 
> OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> > +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP6_UV, 
> OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> you might as well use OPP6_VDD1 for CONTROL_FUSE?

[sp] Risking repetition once more. There is not EFUSE register for OPP6.
     Defining it - with obviously the same values at OPP5_VDD1 - would
     only mislead the person reading the code.

~sanjeev

Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Nishanth Menon]

Premi, Sanjeev wrote, on 01/08/2011 06:29 AM:
[..]
>>>    [1] SPRUFF1D-June 2009 section 1.5.2.1.1
>> Where is this doc?
[..]
> [sp] Thanks for pointing out. It is not accessible for some reasons.
>       It can be...  and is being looked into as i write this message.
>
thanks.
> [snip]...[snip]
>
>>> +#define SWCALC_OPP6_DELTA_NNT	379
>>> +#define SWCALC_OPP6_DELTA_PNT	227
>>> +#define GAIN_MAXLIMIT		16
>>
>> Magic numbers - do they scale from 3430 to 3630?
>
> [sp] The patch is meant for 720MHz - which is applicable for 3430 only.
>       So, these values are meant for 3430.
Right - and since sr_device is common for omap3, omap4 etc.. does'nt it 
make sense to isolate code properly out?

>
> [snip]...[snip]
>
>>
>>> +
>>> +static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)
>> so what does this actually do?
>
> [sp] Returns a nvalue which corresponds to opp6 based on input which
>       is nvalue of opp5. Elementary?
yes Sherlock :). I apologize that I was'nt clear. But I was wondering 
about the following:
what does the algo actually do - does it add a specific voltage delta or 
some %v to previous nTarget?

It takes an input and does some magic - what does that magic algo do to 
produce output?

it is possible that it is self evident to you, but it is not to me - 
probably because I dont have the mentioned doc to refer to.

what is the overall objective of this code?
we have start nTarget from OPP5 and we are trying to put in a new one 
for OPP6 out of the previous one.

things that pop in my mind are:
a) if this was the case, I just need OPP1 value, I could generate values 
for all other OPPs.
b) if the algo itself was generic enough, we could use it in OMAP3430, 
3630, OMAP4 etc.. - as far as I have confirmed from h/w team in the last 
couple of years multiple times, ntarget generation algo for OMAP34,36,4x 
series is not algo based, but production floor operation based (making 
it unique and not really an algo based mechanism of nTarget programming).

>
>>> +{
>
> [snip]...[snip]
>
>>> +       opp6_nvalue = (opp6_senPgain<<  0x14) |
>> (opp6_senNgain<  0x10) |
>>> +                       (opp6_senPRN<<  0x8) | opp6_senNRN;
>>> +
>>> +       return opp6_nvalue;
>
> [sp] Elementary, if you preferred to read the code. Rather that glossing
>       over in hurry to respond compulsively.
I had hoped you could help my time a little (I do other things other 
than review as well ;)) by documenting the algo in the function header?

>
>>> +}
>
> [snip]...[snip]
>
>> Sorry, Dumb question:
>> a) You are using OPP5's nTarget to use in OPP6's nTarget?
>> is'nt it fused
>> in for OPP6 offset?
>
> [sp] Again, in hurry to respond, you missed the description for the patch
>       [quote]
>       The eFuse registers do not contain the nValue to be used
>       with 720MHz (OPP6). This patch implements procedure to
>       calculate the nValue(OPP6) based on the nValue(OPP5).
>       [/quote]

Right - how does it calculate the value for OPP6 from OPP5?

>
>> b) you are using OMAP343X_CONTROL_FUSE_OPP5_VDD1 itself
>
> [sp] Quoting the description:
>       [quote]
>       The eFuse registers do not contain the nValue to be used
>       with 720MHz (OPP6). This patch implements procedure to
>       calculate the nValue(OPP6) based on the nValue(OPP5).
>       [/quote]
>
>       Repeating again, just in case you missed: nvalues for OPP6 are
>       calculated based on the nvalues for OPP5. So, i need to use
>       OMAP343X_CONTROL_FUSE_OPP5_VDD1.
why not define CONTROL_FUSE_OPP6_VDD1 with the same offset?

>
>> c) these should be __init functions.
>
> [sp] They will be when i post formal patch.
>
>> d) how would you allow this code to work with 3440?
>
> [sp] Does 3440 have same behavior? If so, it is quite easy by
>       updating this check.
>       [quote]
>       +	if (cpu_is_omap34xx()&&  omap3_has_720mhz()) {
>       +		nvalue_table[count-1].nvalue = swcalc_opp6_nvalue(
>       +					nvalue_table[count-2].nvalue);
>       +	}
>       [/quote]
>
>       However, i am not conversant with 3440. This will have to be
>       covered by a separate patch.
Right - could you help start a TI internal conversation and alignment to 
ensure 3440 is not broken by this change?

>
>> e) next time could you please try STOP using CaMELCASInG in your
>> variables and functions?
>
> [sp] Did you ever notice any camelcase in any of my formal patch
>       submissions earlier. 10 brownie points for catching it here!
why do you waste reviewer time by posting CamelCasing anyways? your 
responses make me wonder why did I even care to spend time to review 
your code? could have waited for your formal patch and NAKed it :). ok.. 
no more discussions from me anymore untill your formal patch arrives

-- 
Regards,
Nishanth Menon

RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Saturday, January 08, 2011 5:59 PM
> To: Menon, Nishanth
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz
> 

[snip]...[snip]

> > >   [1] SPRUFF1D-June 2009 section 1.5.2.1.1
> > Where is this doc?
> > http://focus-webapps.ti.com/general/docs/sitesearch/searchsite
> > .tsp;jsessionid=TKKPEXXTMNJCPQC1JAWBVQQ?selectedTopic=16532603
> > 27&numRecords=25&searchTerm=SPRUFF1D&statusCode=null
> > 
> > Google seems to point to:
> > http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
> > but ti.com responds with document not found.
> > 
> 
> [sp] Thanks for pointing out. It is not accessible for some reasons.
>      It can be...  and is being looked into as i write this message.
> 

[sp] The document seems to have been merged with main TRM. Hence, this
     document in not reachable.

     Procedure to calculate the nValue for 720MHz (OPP6) is documented
     in the TRM section 1.5.2.1.1.

     Current version of TRM is SPRUF98M. It is vailable at:
     http://www.ti.com/litv/pdf/spruf98m

     Latest version of TRM can be found at:
     http://focus.ti.com/docs/prod/folders/print/omap3503.html

~sanjeev

[snip]...[snip]

RE: [RFC] omap3: Enable SmartReflex calculations for 720MHz

[Premi, Sanjeev]

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com] 
> Sent: Saturday, January 08, 2011 8:26 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RFC] omap3: Enable SmartReflex calculations for 720MHz
> 
> Premi, Sanjeev wrote, on 01/08/2011 06:29 AM:

[snip]...[snip]

> >>
> >> Magic numbers - do they scale from 3430 to 3630?
> >
> > [sp] The patch is meant for 720MHz - which is applicable 
> for 3430 only.
> >       So, these values are meant for 3430.
> Right - and since sr_device is common for omap3, omap4 etc.. 
> does'nt it 
> make sense to isolate code properly out?

[sp] The code is already being protected with:
	if (cpu_is_omap34xx() && omap3_has_720mhz()) {...}

     So, it won't even run for either 36XX OR 44XX as neither
     will return success for this condition.

     What more isolation do you want?

[snip]...[snip]

> >>> +static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)
> >> so what does this actually do?
> >
> > [sp] Returns a nvalue which corresponds to opp6 based on input which
> >       is nvalue of opp5. Elementary?
> yes Sherlock :). I apologize that I was'nt clear. But I was wondering 
> about the following:
> what does the algo actually do - does it add a specific 
> voltage delta or some %v to previous nTarget?
> 
> It takes an input and does some magic - what does that magic 
> algo do to produce output?
> 
> it is possible that it is self evident to you, but it is not to me - 
> probably because I dont have the mentioned doc to refer to.

[sp] ?? ?? ?? ?? Is this supposed to mean something?

> what is the overall objective of this code?
> we have start nTarget from OPP5 and we are trying to put in a new one 
> for OPP6 out of the previous one.

[sp] "Maxima enim... patientia virtus" - Piers Plowman (1377).

     You are obviously driven by compulsion to just write something;
     couldn't wait for the referenced document to be made available.

     Your response was approx 6 hours ahead of me updating the link.

> things that pop in my mind are:
> a) if this was the case, I just need OPP1 value, I could 
> generate values for all other OPPs.

[sp] Having a hammer in your hand doesn't mean you can bang it
     everywhere.

     The patch clearly indicates the purpose and relationship
     between OPP5 and OPP6 established by HW folks. You can
     spend time "imagine"ering and extrapolating to your whims;
     but that is only you.

     The algo used (and referenced) here is pretty clear in its
     purpose and application.

> b) if the algo itself was generic enough, we could use it in 
> OMAP3430, 3630, OMAP4 etc..

[sp] Read this condition once more:
	if (cpu_is_omap34xx() && omap3_has_720mhz()) {...}

> - as far as I have confirmed from h/w team in the last 
> couple of years multiple times, ntarget generation algo for 
> OMAP34,36,4x series is not algo based, but production floor
> operation based (making it unique and not really an algo based
> mechanism of nTarget programming).

[sp] Why is is that you have always confirmed something and it
     is never official?
     
     The TRM is an official document and certain process is
     followed before information finds its way into TRM. Errors
     possible, but until they need to be investigated, verified
     and officially recognized.
     
     If you have found any issue, work with HW team to validate
     it and fix/update the TRM.

     Until then, refrain from spreading rumors.
>
> >
> >>> +{
> >
> > [snip]...[snip]
> >
> >>> +       opp6_nvalue = (opp6_senPgain<<  0x14) |
> >> (opp6_senNgain<  0x10) |
> >>> +                       (opp6_senPRN<<  0x8) | opp6_senNRN;
> >>> +
> >>> +       return opp6_nvalue;
> >
> > [sp] Elementary, if you preferred to read the code. Rather 
> that glossing
> >       over in hurry to respond compulsively.
> I had hoped you could help my time a little (I do other things other 
> than review as well ;)) by documenting the algo in the 
> function header?

[sp] If you can say something in code or say something in comments,
     then say it in code. - Dan Saks (C++ Programming Style, 1996)

     "static u32 swcalc_opp6_nvalue(u32 opp5_nvalue)"

     Algo is referenced from TRM and that's best it can be documented.

> 
> >
> >>> +}
> >
> > [snip]...[snip]
> >
> >> Sorry, Dumb question:
> >> a) You are using OPP5's nTarget to use in OPP6's nTarget?
> >> is'nt it fused
> >> in for OPP6 offset?
> >
> > [sp] Again, in hurry to respond, you missed the description 
> for the patch
> >       [quote]
> >       The eFuse registers do not contain the nValue to be used
> >       with 720MHz (OPP6). This patch implements procedure to
> >       calculate the nValue(OPP6) based on the nValue(OPP5).
> >       [/quote]
> 
> Right - how does it calculate the value for OPP6 from OPP5?

[sp] Read either the code/ the referenced document.

> 
> >
> >> b) you are using OMAP343X_CONTROL_FUSE_OPP5_VDD1 itself
> >
> > [sp] Quoting the description:
> >       [quote]
> >       The eFuse registers do not contain the nValue to be used
> >       with 720MHz (OPP6). This patch implements procedure to
> >       calculate the nValue(OPP6) based on the nValue(OPP5).
> >       [/quote]
> >
> >       Repeating again, just in case you missed: nvalues for OPP6 are
> >       calculated based on the nvalues for OPP5. So, i need to use
> >       OMAP343X_CONTROL_FUSE_OPP5_VDD1.
> why not define CONTROL_FUSE_OPP6_VDD1 with the same offset?

[sp] And let someone trying to understand the code, few months down,
     believe that there is in fact placeholder for OPP6 in the eFuse?

     I'd rather him be questioning why offset to OPP5 value is being
     used in OPP6? And follow thru to find right information.

> 
> >
> >> c) these should be __init functions.
> >
> > [sp] They will be when i post formal patch.
> >
> >> d) how would you allow this code to work with 3440?
> >
> > [sp] Does 3440 have same behavior? If so, it is quite easy by
> >       updating this check.
> >       [quote]
> >       +	if (cpu_is_omap34xx()&&  omap3_has_720mhz()) {
> >       +		nvalue_table[count-1].nvalue = 
> swcalc_opp6_nvalue(
> >       +					
> nvalue_table[count-2].nvalue);
> >       +	}
> >       [/quote]
> >
> >       However, i am not conversant with 3440. This will have to be
> >       covered by a separate patch.
> Right - could you help start a TI internal conversation and 
> alignment to ensure 3440 is not broken by this change?

[sp] Where are you trying to arrive with 3440 argument each time?

     Support for 720MHz didn't exist in the kernel so far. There was
     not entry in either the OPP table or the SR table containing EFUSE
     offsets. How does this change impact 3440?
     
     Even if, hypothetically, 3440 breaks after the patch is accepted;
     rebase, fix and post the patch!

> 
> >
> >> e) next time could you please try STOP using CaMELCASInG in your
> >> variables and functions?
> >
> > [sp] Did you ever notice any camelcase in any of my formal patch
> >       submissions earlier. 10 brownie points for catching it here!
> why do you waste reviewer time by posting CamelCasing anyways? your 
> responses make me wonder why did I even care to spend time to review 
> your code? could have waited for your formal patch and NAKed 
> it :). ok.. 
> no more discussions from me anymore untill your formal patch arrives

[sp] Thanks! saves lot of time!!

     I have better prospect of getting back to original problem;
     for which the RFC was posted.

     [problem]
	/*
	 * FIXME: This is a temporary hack. Need to identify better
	 *        mechanism to find nvalues corresponding to an OPP.
	 */
	if (cpu_is_omap34xx() && omap3_has_720mhz()) {
		nvalue_table[count-1].nvalue = swcalc_opp6_nvalue(
					nvalue_table[count-2].nvalue);
	}
        
        We either need an API or set of macros to return the nvalue
        corresponding to each OPP.

        We could also go with an assumption that array index is OPPN-1.
        but rather than hack above, it could be curtained by a better
        macro/ inline function.
     [/problem]

~sanjeev