[PATCH] omap3: Add basic support for 720MHz part

[PATCH] omap3: Add basic support for 720MHz part

[Sanjeev Premi]

This patch adds support for new speed enhanced parts with ARM
and IVA running at 720MHz and 520MHz respectively. These parts
can be probed at run-time by reading PRODID.SKUID[3:0] at
0x4830A20C [1].

This patch specifically does following:
 * Detect devices capable of 720MHz.
 * Add new OPP
 * Ensure that OPP is conditionally enabled.
 * Check for presence of IVA before attempting to enable
   the corresponding OPP.

  [1] http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
Since v2:
1) pr_xxx() -> dev_xxx() functions - suggested by 
   Manjunath (manjugk@ti.com)
2) Add check for presense of IVA - earlier planned to
   be in a separate patch; but we multiple discussions
   on optimizations.
3) Do look-up for hwmod corresponding for iva only if
   iva is present. Should save multiple strcmp()s in
   _lookup().

Since v1:
1) Using opp_enable() to enable the OPP after the OPP
   table has been initialized.
2) Starting at 3 levels of indent, the statements had
   be broken into multiple lines for most of the code.
   So, opted to create a new static that enables the
   OPPs corresponding to 720MHz.
3) I have only build tested this patch - will be able
   to confirm working tomorrow. With any further change,
   if needed. (However, functionally nothing has changed.)


 arch/arm/mach-omap2/control.h         |    7 ++++
 arch/arm/mach-omap2/id.c              |   10 +++++
 arch/arm/mach-omap2/opp3xxx_data.c    |   63 ++++++++++++++++++++++++++++++++-
 arch/arm/plat-omap/include/plat/cpu.h |    2 +
 4 files changed, 81 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f0629ae..eebc045 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -365,6 +365,13 @@
 #define		FEAT_NEON		0
 #define		FEAT_NEON_NONE		1
 
+/*
+ * Product ID register
+ */
+#define OMAP3_PRODID			0x020C
+
+#define OMAP3_SKUID_MASK		0x0f
+#define		OMAP3_SKUID_720MHZ	0x08
 
 #ifndef __ASSEMBLY__
 #ifdef CONFIG_ARCH_OMAP2PLUS
diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 5f9086c..53fbe01 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
 	 * TODO: Get additional info (where applicable)
 	 *       e.g. Size of L2 cache.
 	 */
+
+	/*
+	 * Does it support 720MHz?
+	 */
+	status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
+
+	if (status & OMAP3_SKUID_720MHZ) {
+		omap3_features |= OMAP3_HAS_720MHZ;
+	}
 }
 
 static void __init omap3_check_revision(void)
@@ -445,6 +454,7 @@ static void __init omap3_cpuinfo(void)
 	OMAP3_SHOW_FEATURE(neon);
 	OMAP3_SHOW_FEATURE(isp);
 	OMAP3_SHOW_FEATURE(192mhz_clk);
+	OMAP3_SHOW_FEATURE(720mhz);
 
 	printk(")\n");
 }
diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
index 0486fce..cba17f7 100644
--- a/arch/arm/mach-omap2/opp3xxx_data.c
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -17,8 +17,10 @@
  * GNU General Public License for more details.
  */
 #include <linux/module.h>
+#include <linux/opp.h>
 
 #include <plat/cpu.h>
+#include <plat/omap_device.h>
 
 #include "omap_opp_data.h"
 
@@ -33,6 +35,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("mpu", true, 550000000, 1270000),
 	/* MPU OPP5 */
 	OPP_INITIALIZER("mpu", true, 600000000, 1350000),
+	/* MPU OPP6 */
+	OPP_INITIALIZER("mpu", false, 720000000, 1350000),
 
 	/*
 	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
@@ -58,6 +62,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("iva", true, 400000000, 1270000),
 	/* DSP OPP5 */
 	OPP_INITIALIZER("iva", true, 430000000, 1350000),
+	/* DSP OPP6 */
+	OPP_INITIALIZER("iva", false, 520000000, 1350000),
 };
 
 static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
@@ -85,6 +91,57 @@ static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
 	OPP_INITIALIZER("iva", false, 800000000, 1375000),
 };
 
+
+/**
+ * omap3_opp_enable_720Mhz() - Enable the OPP corresponding to 720MHz
+ *
+ * This function would be executed only if the silicon is capable of
+ * running at the 720MHz.
+ */
+static int __init omap3_opp_enable_720Mhz(void)
+{
+	int r = -ENODEV;
+	struct omap_hwmod *oh_mpu = omap_hwmod_lookup("mpu");
+	struct omap_hwmod *oh_iva;
+	struct platform_device *pdev;
+
+	if (!oh_mpu || !oh_mpu->od) {
+		goto err;
+	} else {
+		pdev = &oh_mpu->od->pdev;
+
+		r = opp_enable(&pdev->dev, 720000000);
+		if (r < 0) {
+			dev_err(&pdev->dev,
+				"opp_enable() failed for mpu@720MHz");
+			goto err;
+		}
+	}
+
+	if (omap3_has_iva()) {
+		oh_iva = omap_hwmod_lookup("iva");
+
+		if (!oh_iva || !oh_iva->od) {
+			r = -ENODEV;
+			goto err;
+		} else {
+			pdev = &oh_iva->od->pdev;
+
+			r = opp_enable(&pdev->dev, 520000000);
+			if (r < 0) {
+				dev_err(&pdev->dev,
+					"opp_enable() failed for iva@520MHz");
+				goto err;
+			}
+		}
+	}
+
+	dev_info(&pdev->dev, "Enabled OPP corresponding to 720MHz\n");
+
+err:
+	return r;
+}
+
 /**
  * omap3_opp_init() - initialize omap3 opp table
  */
@@ -98,10 +155,14 @@ static int __init omap3_opp_init(void)
 	if (cpu_is_omap3630())
 		r = omap_init_opp_table(omap36xx_opp_def_list,
 			ARRAY_SIZE(omap36xx_opp_def_list));
-	else
+	else {
 		r = omap_init_opp_table(omap34xx_opp_def_list,
 			ARRAY_SIZE(omap34xx_opp_def_list));
 
+		if (omap3_has_720mhz())
+			r = omap3_opp_enable_720Mhz();
+	}
+
 	return r;
 }
 device_initcall(omap3_opp_init);
diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index 3fd8b40..5c77987 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -455,6 +455,7 @@ extern u32 omap3_features;
 #define OMAP3_HAS_ISP			BIT(4)
 #define OMAP3_HAS_192MHZ_CLK		BIT(5)
 #define OMAP3_HAS_IO_WAKEUP		BIT(6)
+#define OMAP3_HAS_720MHZ		BIT(7)
 
 #define OMAP3_HAS_FEATURE(feat,flag)			\
 static inline unsigned int omap3_has_ ##feat(void)	\
@@ -469,5 +470,6 @@ OMAP3_HAS_FEATURE(neon, NEON)
 OMAP3_HAS_FEATURE(isp, ISP)
 OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
 OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
+OMAP3_HAS_FEATURE(720mhz, 720MHZ)
 
 #endif
-- 
1.7.2.2

Re: [PATCH] omap3: Add basic support for 720MHz part

[Varadarajan, Charulatha]

Couple of minor comments.

On Tue, Jan 18, 2011 at 13:19, Sanjeev Premi <premi@ti.com> wrote:
> This patch adds support for new speed enhanced parts with ARM
> and IVA running at 720MHz and 520MHz respectively. These parts
> can be probed at run-time by reading PRODID.SKUID[3:0] at
> 0x4830A20C [1].
>
> This patch specifically does following:
>  * Detect devices capable of 720MHz.
>  * Add new OPP
>  * Ensure that OPP is conditionally enabled.
>  * Check for presence of IVA before attempting to enable
>   the corresponding OPP.
>
>  [1] http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
> Since v2:
> 1) pr_xxx() -> dev_xxx() functions - suggested by
>   Manjunath (manjugk@ti.com)
> 2) Add check for presense of IVA - earlier planned to
>   be in a separate patch; but we multiple discussions
>   on optimizations.
> 3) Do look-up for hwmod corresponding for iva only if
>   iva is present. Should save multiple strcmp()s in
>   _lookup().
>
> Since v1:
> 1) Using opp_enable() to enable the OPP after the OPP
>   table has been initialized.
> 2) Starting at 3 levels of indent, the statements had
>   be broken into multiple lines for most of the code.
>   So, opted to create a new static that enables the
>   OPPs corresponding to 720MHz.
> 3) I have only build tested this patch - will be able
>   to confirm working tomorrow. With any further change,
>   if needed. (However, functionally nothing has changed.)
>
>
>  arch/arm/mach-omap2/control.h         |    7 ++++
>  arch/arm/mach-omap2/id.c              |   10 +++++
>  arch/arm/mach-omap2/opp3xxx_data.c    |   63 ++++++++++++++++++++++++++++++++-
>  arch/arm/plat-omap/include/plat/cpu.h |    2 +
>  4 files changed, 81 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index f0629ae..eebc045 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -365,6 +365,13 @@
>  #define                FEAT_NEON               0
>  #define                FEAT_NEON_NONE          1
>
> +/*
> + * Product ID register
> + */

Do not use multiline comment style for single line comments

> +#define OMAP3_PRODID                   0x020C
> +
> +#define OMAP3_SKUID_MASK               0x0f
> +#define                OMAP3_SKUID_720MHZ      0x08
>
>  #ifndef __ASSEMBLY__
>  #ifdef CONFIG_ARCH_OMAP2PLUS
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index 5f9086c..53fbe01 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
>         * TODO: Get additional info (where applicable)
>         *       e.g. Size of L2 cache.
>         */
> +
> +       /*
> +        * Does it support 720MHz?
> +        */

Ditto

> +       status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> +
> +       if (status & OMAP3_SKUID_720MHZ) {
> +               omap3_features |= OMAP3_HAS_720MHZ;
> +       }

No need of { } as there is only one line statement inside the if condition.

>  }
>
>  static void __init omap3_check_revision(void)
> @@ -445,6 +454,7 @@ static void __init omap3_cpuinfo(void)
>        OMAP3_SHOW_FEATURE(neon);
>        OMAP3_SHOW_FEATURE(isp);
>        OMAP3_SHOW_FEATURE(192mhz_clk);
> +       OMAP3_SHOW_FEATURE(720mhz);
>
>        printk(")\n");
>  }

<<snip>>
--
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: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: Varadarajan, Charulatha 
> Sent: Tuesday, January 18, 2011 2:44 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> 
> Couple of minor comments.

[snip]...[snip]

> > --- a/arch/arm/mach-omap2/control.h
> > +++ b/arch/arm/mach-omap2/control.h
> > @@ -365,6 +365,13 @@
> >  #define                FEAT_NEON               0
> >  #define                FEAT_NEON_NONE          1
> >
> > +/*
> > + * Product ID register
> > + */
> 
> Do not use multiline comment style for single line comments

[sp] Was only following the multi-line comment stye few lines
     above...

> 
> > +#define OMAP3_PRODID                   0x020C
> > +
> > +#define OMAP3_SKUID_MASK               0x0f
> > +#define                OMAP3_SKUID_720MHZ      0x08
> >
> >  #ifndef __ASSEMBLY__
> >  #ifdef CONFIG_ARCH_OMAP2PLUS
> > diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> > index 5f9086c..53fbe01 100644
> > --- a/arch/arm/mach-omap2/id.c
> > +++ b/arch/arm/mach-omap2/id.c
> > @@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
> >         * TODO: Get additional info (where applicable)
> >         *       e.g. Size of L2 cache.
> >         */
> > +
> > +       /*
> > +        * Does it support 720MHz?
> > +        */
> 
> Ditto

[sp] Same here.
     But since the code is quite evident, I will remove the comment
     altogether.

> 
> > +       status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> > +
> > +       if (status & OMAP3_SKUID_720MHZ) {
> > +               omap3_features |= OMAP3_HAS_720MHZ;
> > +       }
> 
> No need of { } as there is only one line statement inside the 
> if condition.

[sp] missed this.

~sanjeev

[snip]...[snip]--
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: [PATCH] omap3: Add basic support for 720MHz part

[Varadarajan, Charulatha]

On Thu, Jan 20, 2011 at 16:28, Premi, Sanjeev <premi@ti.com> wrote:
>> -----Original Message-----
>> From: Varadarajan, Charulatha
>> Sent: Tuesday, January 18, 2011 2:44 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part

Also do not miss the version number while posting patches.

Re: [PATCH] omap3: Add basic support for 720MHz part

[G, Manjunath Kondaiah]

On Thu, Jan 20, 2011 at 04:28:11PM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Varadarajan, Charulatha 
> > Sent: Tuesday, January 18, 2011 2:44 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> > 
> > Couple of minor comments.
> 
> [snip]...[snip]
> 
> > > --- a/arch/arm/mach-omap2/control.h
> > > +++ b/arch/arm/mach-omap2/control.h
> > > @@ -365,6 +365,13 @@
> > >  #define                FEAT_NEON               0
> > >  #define                FEAT_NEON_NONE          1
> > >
> > > +/*
> > > + * Product ID register
> > > + */
> > 
> > Do not use multiline comment style for single line comments
> 
> [sp] Was only following the multi-line comment stye few lines
>      above...
> 
> > 
> > > +#define OMAP3_PRODID                   0x020C
> > > +
> > > +#define OMAP3_SKUID_MASK               0x0f
> > > +#define                OMAP3_SKUID_720MHZ      0x08
> > >
> > >  #ifndef __ASSEMBLY__
> > >  #ifdef CONFIG_ARCH_OMAP2PLUS
> > > diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> > > index 5f9086c..53fbe01 100644
> > > --- a/arch/arm/mach-omap2/id.c
> > > +++ b/arch/arm/mach-omap2/id.c
> > > @@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
> > >         * TODO: Get additional info (where applicable)
> > >         *       e.g. Size of L2 cache.
> > >         */
> > > +
> > > +       /*
> > > +        * Does it support 720MHz?
> > > +        */
> > 
> > Ditto
> 
> [sp] Same here.
>      But since the code is quite evident, I will remove the comment
>      altogether.
> 
> > 
> > > +       status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> > > +
> > > +       if (status & OMAP3_SKUID_720MHZ) {
> > > +               omap3_features |= OMAP3_HAS_720MHZ;
> > > +       }
> > 
> > No need of { } as there is only one line statement inside the 
> > if condition.
> 
> [sp] missed this.

Looks fine. Feel free to add after the above changes:

Reviewed-by: G, Manjunath Kondaiah <manjugk@ti.com>

-Manjunath

--
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: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: G, Manjunath Kondaiah [mailto:manjugk@ti.com] 
> Sent: Thursday, January 20, 2011 4:51 PM
> To: Premi, Sanjeev
> Cc: Varadarajan, Charulatha; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> 

[snip]

> 
> Looks fine. Feel free to add after the above changes:
> 
> Reviewed-by: G, Manjunath Kondaiah <manjugk@ti.com>
> 

Sorry, noticed this mail after sending the patch from linux box :(

> -Manjunath
> 
> 

Re: [PATCH] omap3: Add basic support for 720MHz part

[Koen Kooi]

Op 18 jan 2011, om 08:49 heeft Sanjeev Premi het volgende geschreven:

> This patch adds support for new speed enhanced parts with ARM
> and IVA running at 720MHz and 520MHz respectively. These parts
> can be probed at run-time by reading PRODID.SKUID[3:0] at
> 0x4830A20C [1].
> 
> This patch specifically does following:
> * Detect devices capable of 720MHz.
> * Add new OPP
> * Ensure that OPP is conditionally enabled.
> * Check for presence of IVA before attempting to enable
>   the corresponding OPP.

Thanks for the updated patch! 

I'm still having problem using this together with DVFS, the kernel won't scale beyond 600MHz because 600MHz and 720MHz share the same voltage. Thara, Nishant, do you have any suggestions on how to convince the kernel that 2 frequencies can share the same voltage settings? 

If that isn't possible, should we lower the voltages for the lower opps when detecting a 720MHz capable SoC? It would make sense that if 720MHz works at X volt that 600MHz would work at less  than X volt. (I don't have a PDF reader at hand to check the TRM at the moment).

Complete tree at http://dominion.thruhere.net/git/cgit.cgi/linux-omap/log/?h=koen/beagle-next but this can be reproduced as well by merging Thara's dvfs branch and applying Sanjeevs patch.

regards,

Koen

Re: [PATCH] omap3: Add basic support for 720MHz part

[Nishanth Menon]

Koen Kooi wrote, on 01/18/2011 05:38 PM:
>
> Op 18 jan 2011, om 08:49 heeft Sanjeev Premi het volgende geschreven:
>
>> This patch adds support for new speed enhanced parts with ARM
>> and IVA running at 720MHz and 520MHz respectively. These parts
>> can be probed at run-time by reading PRODID.SKUID[3:0] at
>> 0x4830A20C [1].
>>
>> This patch specifically does following:
>> * Detect devices capable of 720MHz.
>> * Add new OPP
>> * Ensure that OPP is conditionally enabled.
>> * Check for presence of IVA before attempting to enable
>>    the corresponding OPP.
>
> Thanks for the updated patch!
>
> I'm still having problem using this together with DVFS, the kernel won't scale beyond 600MHz because 600MHz and 720MHz share the same voltage. Thara, Nishant, do you have any suggestions on how to convince the kernel that 2 frequencies can share the same voltage settings?
+ Vishwa

>
> If that isn't possible, should we lower the voltages for the lower opps when detecting a 720MHz capable SoC? It would make sense that if 720MHz works at X volt that 600MHz would work at less  than X volt. (I don't have a PDF reader at hand to check the TRM at the moment).
>
> Complete tree at http://dominion.thruhere.net/git/cgit.cgi/linux-omap/log/?h=koen/beagle-next but this can be reproduced as well by merging Thara's dvfs branch and applying Sanjeevs patch.
I suspect the problem lies here:

http://dominion.thruhere.net/git/cgit.cgi/linux-omap/tree/arch/arm/mach-omap2/voltage.c?h=koen/beagle-next#n1847
if two OPP frequencies have same voltage, the api introduced in
http://dominion.thruhere.net/git/cgit.cgi/linux-omap/commit/?h=koen/beagle-next&id=fd2991c20a8f770c7b8f5a29b3cbb17b9fca4768

will return back the first OPP entry which matches, which would be the 
lower frequency as OPPs are organized in the OPP layer in increasing order.

-- 
Regards,
Nishanth Menon

RE: [PATCH] omap3: Add basic support for 720MHz part

[Vishwanath Sripathy]

Kooi,

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> Sent: Tuesday, January 18, 2011 10:19 PM
> To: Koen Kooi
> Cc: Sanjeev Premi; l-o List; thara@ti.com; Vishwanath Sripathy
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
>
> Koen Kooi wrote, on 01/18/2011 05:38 PM:
> >
> > Op 18 jan 2011, om 08:49 heeft Sanjeev Premi het volgende
> geschreven:
> >
> >> This patch adds support for new speed enhanced parts with ARM
> >> and IVA running at 720MHz and 520MHz respectively. These parts
> >> can be probed at run-time by reading PRODID.SKUID[3:0] at
> >> 0x4830A20C [1].
> >>
> >> This patch specifically does following:
> >> * Detect devices capable of 720MHz.
> >> * Add new OPP
> >> * Ensure that OPP is conditionally enabled.
> >> * Check for presence of IVA before attempting to enable
> >>    the corresponding OPP.
> >
> > Thanks for the updated patch!
> >
> > I'm still having problem using this together with DVFS, the kernel
> won't scale beyond 600MHz because 600MHz and 720MHz share the
> same voltage. Thara, Nishant, do you have any suggestions on how to
> convince the kernel that 2 frequencies can share the same voltage
> settings?
> + Vishwa
Yes, this is a limitation with the current set of DVFS patches.
With the next version of DVFS patches (will be out soon) this feature is
going to be supported. That means you can have 2 opps with the same
voltage but with different frequency. The device will be configured based
on user requested frequency.

Vishwa

>
> >
> > If that isn't possible, should we lower the voltages for the lower
opps
> when detecting a 720MHz capable SoC? It would make sense that if
> 720MHz works at X volt that 600MHz would work at less  than X volt. (I
> don't have a PDF reader at hand to check the TRM at the moment).
> >
> > Complete tree at http://dominion.thruhere.net/git/cgit.cgi/linux-
> omap/log/?h=koen/beagle-next but this can be reproduced as well by
> merging Thara's dvfs branch and applying Sanjeevs patch.
> I suspect the problem lies here:
>
> http://dominion.thruhere.net/git/cgit.cgi/linux-
> omap/tree/arch/arm/mach-omap2/voltage.c?h=koen/beagle-
> next#n1847
> if two OPP frequencies have same voltage, the api introduced in
> http://dominion.thruhere.net/git/cgit.cgi/linux-
> omap/commit/?h=koen/beagle-
> next&id=fd2991c20a8f770c7b8f5a29b3cbb17b9fca4768
>
> will return back the first OPP entry which matches, which would be the
> lower frequency as OPPs are organized in the OPP layer in increasing
> order.
>
> --
> Regards,
> Nishanth Menon

Re: [PATCH] omap3: Add basic support for 720MHz part

[Koen Kooi]

Op 18 jan 2011, om 18:18 heeft Vishwanath Sripathy het volgende geschreven:

> Kooi,
> 
>> -----Original Message-----
>> From: Nishanth Menon [mailto:nm@ti.com]
>> Sent: Tuesday, January 18, 2011 10:19 PM
>> To: Koen Kooi
>> Cc: Sanjeev Premi; l-o List; thara@ti.com; Vishwanath Sripathy
>> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
>> 
>> Koen Kooi wrote, on 01/18/2011 05:38 PM:
>>> 
>>> Op 18 jan 2011, om 08:49 heeft Sanjeev Premi het volgende
>> geschreven:
>>> 
>>>> This patch adds support for new speed enhanced parts with ARM
>>>> and IVA running at 720MHz and 520MHz respectively. These parts
>>>> can be probed at run-time by reading PRODID.SKUID[3:0] at
>>>> 0x4830A20C [1].
>>>> 
>>>> This patch specifically does following:
>>>> * Detect devices capable of 720MHz.
>>>> * Add new OPP
>>>> * Ensure that OPP is conditionally enabled.
>>>> * Check for presence of IVA before attempting to enable
>>>>   the corresponding OPP.
>>> 
>>> Thanks for the updated patch!
>>> 
>>> I'm still having problem using this together with DVFS, the kernel
>> won't scale beyond 600MHz because 600MHz and 720MHz share the
>> same voltage. Thara, Nishant, do you have any suggestions on how to
>> convince the kernel that 2 frequencies can share the same voltage
>> settings?
>> + Vishwa
> Yes, this is a limitation with the current set of DVFS patches.
> With the next version of DVFS patches (will be out soon) this feature is
> going to be supported. That means you can have 2 opps with the same
> voltage but with different frequency. The device will be configured based
> on user requested frequency.

Just to have a crude hack, would something like this work?:

	/* MPU OPP6 */
-	OPP_INITIALIZER("mpu", false, 720000000, 1350000),
+	OPP_INITIALIZER("mpu", false, 720000000, 1350001),

Or will I fall into a (post)divider trap where the kernel panics because it can't do that exact voltage?

The TRM[1] chickens out with the OPP definitions:

------------------>8---------------------------------------------------------------
Six generic processor OPPs can be defined as:
1. OPP6 (VDD1 = v4, (MPU_CLK = fmpu5, IVA2_CLK = fiva5))
2. OPP5 (VDD1 = v4, (MPU_CLK = fmpu4, IVA2_CLK = fiva4))
3. OPP4 (VDD1 = v3, (MPU_CLK = fmpu3, IVA2_CLK = fiva3))
4. OPP3 (VDD1 = v2 , (MPU_CLK = fmpu2, IVA2_CLK = fiva2))
5. OPP2 (VDD1 = v1 , (MPU_CLK = fmpu1, IVA2_CLK = fiva1))
6. OPP1 (VDD1 = v0 , (MPU_CLK = fmpu0, IVA2_CLK = fiva0))
where v4 > v3 > v2 > v1 > v0,
------------------8<---------------------------------------------------------------

My limited sample size of 3 board (2x beagle C4, 1x overo tide) shows 600MHz@1.3V works as well, but I don't know how much slack that has. And I haven't check if the hardware can do 1.3V exactly or has rounded up.

So, what's the quickest way with the current code to get 720MHz "working"? I've voided my warranty on everything already, so that isn't a problem :)

regards,

Koen

[1] spruf98m.pdf page 355

RE: [PATCH] omap3: Add basic support for 720MHz part

[Vishwanath Sripathy]

Kooi,

> -----Original Message-----
> From: Koen Kooi [mailto:koen@dominion.thruhere.net]
> Sent: Tuesday, January 18, 2011 11:47 PM
> To: Vishwanath Sripathy
> Cc: Nishanth Menon; Sanjeev Premi; l-o List; Thara Gopinath
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
>
>
> Op 18 jan 2011, om 18:18 heeft Vishwanath Sripathy het volgende
> geschreven:
>
> > Kooi,
> >
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:nm@ti.com]
> >> Sent: Tuesday, January 18, 2011 10:19 PM
> >> To: Koen Kooi
> >> Cc: Sanjeev Premi; l-o List; thara@ti.com; Vishwanath Sripathy
> >> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> >>
> >> Koen Kooi wrote, on 01/18/2011 05:38 PM:
> >>>
> >>> Op 18 jan 2011, om 08:49 heeft Sanjeev Premi het volgende
> >> geschreven:
> >>>
> >>>> This patch adds support for new speed enhanced parts with ARM
> >>>> and IVA running at 720MHz and 520MHz respectively. These parts
> >>>> can be probed at run-time by reading PRODID.SKUID[3:0] at
> >>>> 0x4830A20C [1].
> >>>>
> >>>> This patch specifically does following:
> >>>> * Detect devices capable of 720MHz.
> >>>> * Add new OPP
> >>>> * Ensure that OPP is conditionally enabled.
> >>>> * Check for presence of IVA before attempting to enable
> >>>>   the corresponding OPP.
> >>>
> >>> Thanks for the updated patch!
> >>>
> >>> I'm still having problem using this together with DVFS, the kernel
> >> won't scale beyond 600MHz because 600MHz and 720MHz share the
> >> same voltage. Thara, Nishant, do you have any suggestions on how
> to
> >> convince the kernel that 2 frequencies can share the same voltage
> >> settings?
> >> + Vishwa
> > Yes, this is a limitation with the current set of DVFS patches.
> > With the next version of DVFS patches (will be out soon) this feature
is
> > going to be supported. That means you can have 2 opps with the same
> > voltage but with different frequency. The device will be configured
> based
> > on user requested frequency.
>
> Just to have a crude hack, would something like this work?:
>
> 	/* MPU OPP6 */
> -	OPP_INITIALIZER("mpu", false, 720000000, 1350000),
> +	OPP_INITIALIZER("mpu", false, 720000000, 1350001),
>
> Or will I fall into a (post)divider trap where the kernel panics because
it
> can't do that exact voltage?
>
> The TRM[1] chickens out with the OPP definitions:
>
>
------------------>8------------------------------------------------------
------
> ---
> Six generic processor OPPs can be defined as:
> 1. OPP6 (VDD1 = v4, (MPU_CLK = fmpu5, IVA2_CLK = fiva5))
> 2. OPP5 (VDD1 = v4, (MPU_CLK = fmpu4, IVA2_CLK = fiva4))
> 3. OPP4 (VDD1 = v3, (MPU_CLK = fmpu3, IVA2_CLK = fiva3))
> 4. OPP3 (VDD1 = v2 , (MPU_CLK = fmpu2, IVA2_CLK = fiva2))
> 5. OPP2 (VDD1 = v1 , (MPU_CLK = fmpu1, IVA2_CLK = fiva1))
> 6. OPP1 (VDD1 = v0 , (MPU_CLK = fmpu0, IVA2_CLK = fiva0))
> where v4 > v3 > v2 > v1 > v0,
>
------------------8<------------------------------------------------------
------
> ---
>
> My limited sample size of 3 board (2x beagle C4, 1x overo tide) shows
> 600MHz@1.3V works as well, but I don't know how much slack that has.
> And I haven't check if the hardware can do 1.3V exactly or has rounded
> up.
>
> So, what's the quickest way with the current code to get 720MHz
> "working"? I've voided my warranty on everything already, so that isn't
a
> problem :)
If you want a quick hack, you can modify the voltage for 700M marginally
(like you have done above). But it needs to be done in multiple places.
You need to modify IVA OPP6 entry in opp table and OPP6 voltage value in
voltage.h as well besides above change. This should work.

Vishwa
>
> regards,
>
> Koen
>
> [1] spruf98m.pdf page 355

Re: [PATCH] omap3: Add basic support for 720MHz part

[Nishanth Menon]

Vishwanath Sripathy wrote, on 01/19/2011 10:56 AM:
> If you want a quick hack, you can modify the voltage for 700M marginally
> (like you have done above). But it needs to be done in multiple places.
> You need to modify IVA OPP6 entry in opp table and OPP6 voltage value in
> voltage.h as well besides above change. This should work.
we may want to be sure about keeping the additional voltages in 1 step 
increment (12.5mV - to ensure twl conversion routines sane)- i think we 
may be pretty much maxed out for the tps chip used, no?

-- 
Regards,
Nishanth Menon

Re: [PATCH] omap3: Add basic support for 720MHz part

[Koen Kooi]

Op 19 jan 2011, om 10:06 heeft Nishanth Menon het volgende geschreven:

> Vishwanath Sripathy wrote, on 01/19/2011 10:56 AM:
>> If you want a quick hack, you can modify the voltage for 700M marginally
>> (like you have done above). But it needs to be done in multiple places.
>> You need to modify IVA OPP6 entry in opp table and OPP6 voltage value in
>> voltage.h as well besides above change. This should work.
> we may want to be sure about keeping the additional voltages in 1 step increment (12.5mV - to ensure twl conversion routines sane)- i think we may be pretty much maxed out for the tps chip used, no?

Not using the 12.5mV steps gives me:

[    3.076873] calc_dep_vdd_volt: Not able to find a matching volt forvdd_core corresponding to vdd_mpu 1350001 volt
[    3.095306] omap_voltage_scale: Error in calculating dependent vdd voltagesfor vdd_mpu

It does seem to work, though:

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

I think I'll just drop the OPP5 voltage by 12.5mV since I know what silicon my usrp-e1xx has, but I can't really give out that patch to others, since someone might replace the overo with another one.

This is the hack I ended up using:

diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
index 76d26c7..c913240 100644
--- a/arch/arm/mach-omap2/opp3xxx_data.c
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -37,7 +37,7 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
        /* MPU OPP5 */
        OPP_INITIALIZER("mpu", true, 600000000, 1350000),
        /* MPU OPP6 */
-       OPP_INITIALIZER("mpu", false, 720000000, 1350000),
+       OPP_INITIALIZER("mpu", false, 720000000, 1350001),
 
        /*
         * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
@@ -64,7 +64,7 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
        /* DSP OPP5 */
        OPP_INITIALIZER("iva", true, 430000000, 1350000),
        /* DSP OPP6 */
-       OPP_INITIALIZER("iva", false, 520000000, 1350000),
+       OPP_INITIALIZER("iva", false, 520000000, 1350001),
 };
 
 static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index d10cb1b..2b7ff8f 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -324,6 +324,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 6782c5e..42b89db 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               1350001
 
 #define OMAP3430_VDD_CORE_OPP1_UV              975000
 #define OMAP3430_VDD_CORE_OPP2_UV              1050000

RE: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: Koen Kooi [mailto:koen@dominion.thruhere.net] 
> Sent: Tuesday, January 18, 2011 11:47 PM
> To: Sripathy, Vishwanath
> Cc: Menon, Nishanth; Premi, Sanjeev; l-o List; Gopinath, Thara
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part

[snip]...[snip]

> The TRM[1] chickens out with the OPP definitions:

Refer to page 122 of the OMAP3503 datasheet for the OPP definitions:
http://www.ti.com/litv/pdf/sprs505f

The nominal voltage for both (VDD1) OPP5 and OPP6 is 1.35V.

~sanjeev

RE: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: Koen Kooi [mailto:koen@dominion.thruhere.net] 
> Sent: Tuesday, January 18, 2011 9:08 PM
> To: Premi, Sanjeev
> Cc: l-o List; Menon, Nishanth; Gopinath, Thara
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> 
[snip]...[snip]
> 
> Thanks for the updated patch! 
> 
> I'm still having problem using this together with DVFS, the 
> kernel won't scale beyond 600MHz because 600MHz and 720MHz 
> share the same voltage. Thara, Nishant, do you have any 
> suggestions on how to convince the kernel that 2 frequencies 
> can share the same voltage settings? 

Koen,

I still haven't spent time testing with DVFS.

There are few issue that need to be resolved with the DVFS
infra before DVFS can be functional.

Would like this patch to be de-coupled from DVFS discussions.
It was not the intent of the patch...

Am working to list all the issues and possible solutions for
the DVFS... getting delayed due to other activities.

~sanjeev

[snip]...[snip]

Re: [PATCH] omap3: Add basic support for 720MHz part

[Koen Kooi]

Op 19 jan 2011, om 10:49 heeft Premi, Sanjeev het volgende geschreven:

>> -----Original Message-----
>> From: Koen Kooi [mailto:koen@dominion.thruhere.net] 
>> Sent: Tuesday, January 18, 2011 9:08 PM
>> To: Premi, Sanjeev
>> Cc: l-o List; Menon, Nishanth; Gopinath, Thara
>> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
>> 
> [snip]...[snip]
>> 
>> Thanks for the updated patch! 
>> 
>> I'm still having problem using this together with DVFS, the 
>> kernel won't scale beyond 600MHz because 600MHz and 720MHz 
>> share the same voltage. Thara, Nishant, do you have any 
>> suggestions on how to convince the kernel that 2 frequencies 
>> can share the same voltage settings? 
> 
> Koen,
> 
> I still haven't spent time testing with DVFS.
> 
> There are few issue that need to be resolved with the DVFS
> infra before DVFS can be functional.
> 
> Would like this patch to be de-coupled from DVFS discussions.
> It was not the intent of the patch...

It was not my intent to couple it to DVFS discussions, I just used it as an example to highlight the bugs I'm hitting with DVFS and 720MHz operation :) I don't want this patch to be held back by bugs in the DVFS code!

[PATCH] omap3: Add basic support for 720MHz part

[Sanjeev Premi]

This patch adds support for new speed enhanced parts with ARM
and IVA running at 720MHz and 520MHz respectively. These parts
can be probed at run-time by reading PRODID.SKUID[3:0] at
0x4830A20C [1].

This patch specifically does following:
 * Detect devices capable of 720MHz.
 * Add new OPP
 * Ensure that OPP is conditionally enabled.

The patch was tested on OMAP3EVM.

On 720MHz capable device:
# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
720000

On other devices:
# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
600000

  [1] http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/control.h         |    7 +++++++
 arch/arm/mach-omap2/id.c              |   10 ++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c    |   19 ++++++++++++++++++-
 arch/arm/plat-omap/include/plat/cpu.h |    2 ++
 4 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f0629ae..eebc045 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -365,6 +365,13 @@
 #define		FEAT_NEON		0
 #define		FEAT_NEON_NONE		1
 
+/*
+ * Product ID register
+ */
+#define OMAP3_PRODID			0x020C
+
+#define OMAP3_SKUID_MASK		0x0f
+#define		OMAP3_SKUID_720MHZ	0x08
 
 #ifndef __ASSEMBLY__
 #ifdef CONFIG_ARCH_OMAP2PLUS
diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 5f9086c..53fbe01 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
 	 * TODO: Get additional info (where applicable)
 	 *       e.g. Size of L2 cache.
 	 */
+
+	/*
+	 * Does it support 720MHz?
+	 */
+	status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
+
+	if (status & OMAP3_SKUID_720MHZ) {
+		omap3_features |= OMAP3_HAS_720MHZ;
+	}
 }
 
 static void __init omap3_check_revision(void)
@@ -445,6 +454,7 @@ static void __init omap3_cpuinfo(void)
 	OMAP3_SHOW_FEATURE(neon);
 	OMAP3_SHOW_FEATURE(isp);
 	OMAP3_SHOW_FEATURE(192mhz_clk);
+	OMAP3_SHOW_FEATURE(720mhz);
 
 	printk(")\n");
 }
diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
index 0486fce..01582b7 100644
--- a/arch/arm/mach-omap2/opp3xxx_data.c
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -22,6 +22,9 @@
 
 #include "omap_opp_data.h"
 
+#define INDEX_MPU_720MHZ	5
+#define INDEX_IVA_720MHZ	14
+
 static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	/* MPU OPP1 */
 	OPP_INITIALIZER("mpu", true, 125000000, 975000),
@@ -33,6 +36,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("mpu", true, 550000000, 1270000),
 	/* MPU OPP5 */
 	OPP_INITIALIZER("mpu", true, 600000000, 1350000),
+	/* MPU OPP6 */
+	OPP_INITIALIZER("mpu", false, 720000000, 1350000),
 
 	/*
 	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
@@ -58,6 +63,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("iva", true, 400000000, 1270000),
 	/* DSP OPP5 */
 	OPP_INITIALIZER("iva", true, 430000000, 1350000),
+	/* DSP OPP6 */
+	OPP_INITIALIZER("iva", false, 520000000, 1350000),
 };
 
 static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
@@ -98,9 +105,19 @@ static int __init omap3_opp_init(void)
 	if (cpu_is_omap3630())
 		r = omap_init_opp_table(omap36xx_opp_def_list,
 			ARRAY_SIZE(omap36xx_opp_def_list));
-	else
+	else {
+		if (omap3_has_720mhz()) {
+			pr_info("Enabled OPP corresponding to 720MHz\n");
+
+			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
+				.default_available = true;
+			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
+				.default_available = true;
+		}
+
 		r = omap_init_opp_table(omap34xx_opp_def_list,
 			ARRAY_SIZE(omap34xx_opp_def_list));
+	}
 
 	return r;
 }
diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index 3fd8b40..5c77987 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -455,6 +455,7 @@ extern u32 omap3_features;
 #define OMAP3_HAS_ISP			BIT(4)
 #define OMAP3_HAS_192MHZ_CLK		BIT(5)
 #define OMAP3_HAS_IO_WAKEUP		BIT(6)
+#define OMAP3_HAS_720MHZ		BIT(7)
 
 #define OMAP3_HAS_FEATURE(feat,flag)			\
 static inline unsigned int omap3_has_ ##feat(void)	\
@@ -469,5 +470,6 @@ OMAP3_HAS_FEATURE(neon, NEON)
 OMAP3_HAS_FEATURE(isp, ISP)
 OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
 OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
+OMAP3_HAS_FEATURE(720mhz, 720MHZ)
 
 #endif
-- 
1.7.2.2

Re: [PATCH] omap3: Add basic support for 720MHz part

[Nishanth Menon]

Sanjeev Premi wrote, on 01/07/2011 07:07 AM:
> +		if (omap3_has_720mhz()) {
> +			pr_info("Enabled OPP corresponding to 720MHz\n");
> +
> +			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
> +				.default_available = true;
> +			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
> +				.default_available = true;
for many reasons, I dont like this indexing - I am ok with most part of 
the patch otherwise - how about opp_enable(dev, freq) instead?

-- 
Regards,
Nishanth Menon

RE: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Friday, January 07, 2011 7:04 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> 
> Sanjeev Premi wrote, on 01/07/2011 07:07 AM:
> > +		if (omap3_has_720mhz()) {
> > +			pr_info("Enabled OPP corresponding to 
> 720MHz\n");
> > +
> > +			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
> > +				.default_available = true;
> > +			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
> > +				.default_available = true;
> for many reasons, I dont like this indexing - I am ok with 
> most part of 
> the patch otherwise - how about opp_enable(dev, freq) instead?

I had thought about it, but opp_enable would have to be done after
omap_init_opp_table(). Two factors led me to current implementation:

1) Numer of lines of code required to get same thing done one the
   opp table has been initialized.

2) The index definition and usage are localized in same file.

> 
> -- 
> Regards,
> Nishanth Menon
> 

Re: [PATCH] omap3: Add basic support for 720MHz part

[Nishanth Menon]

Premi, Sanjeev wrote, on 01/07/2011 07:50 AM:
>> -----Original Message-----
>> From: Menon, Nishanth
>> Sent: Friday, January 07, 2011 7:04 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
>>
>> Sanjeev Premi wrote, on 01/07/2011 07:07 AM:
>>> +		if (omap3_has_720mhz()) {
>>> +			pr_info("Enabled OPP corresponding to
>> 720MHz\n");
>>> +
>>> +			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
>>> +				.default_available = true;
>>> +			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
>>> +				.default_available = true;
>> for many reasons, I dont like this indexing - I am ok with
>> most part of
>> the patch otherwise - how about opp_enable(dev, freq) instead?
>
> I had thought about it, but opp_enable would have to be done after
> omap_init_opp_table(). Two factors led me to current implementation:
>
> 1) Numer of lines of code required to get same thing done one the
>     opp table has been initialized.
>
> 2) The index definition and usage are localized in same file.

right - i will leave it to kevin to comment - IMHO both will work, 
though (1) might be a little more elgant and resistant to future changes 
to the table (I am not sure if there would be any, but what the heck.)

-- 
Regards,
Nishanth Menon

Re: [PATCH] omap3: Add basic support for 720MHz part

[Kevin Hilman]

Sanjeev Premi <premi@ti.com> writes:

> This patch adds support for new speed enhanced parts with ARM
> and IVA running at 720MHz and 520MHz respectively. These parts
> can be probed at run-time by reading PRODID.SKUID[3:0] at
> 0x4830A20C [1].

Please expand this a little to describe exactly which parts have this
feature.  All OMAP3?  34xx? 35xx? what about 36xx/37xx?  ISTR the
runtime probing for this feature is available on 35xx but not on 34xx,
but a summary of this should be here.

> This patch specifically does following:
>  * Detect devices capable of 720MHz.
>  * Add new OPP
>  * Ensure that OPP is conditionally enabled.
>
> The patch was tested on OMAP3EVM.

nit: Unrelated to this patch, but OMAP3EVM ethernet is still broken due to
missing GPIO configuration for the network reset.  This has been broken
since the GPIO hwmod merge and pointed out but nobody seems to be fixing
it.

> On 720MHz capable device:
> # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 720000
>
> On other devices:
> # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 600000
>
>   [1] http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  arch/arm/mach-omap2/control.h         |    7 +++++++
>  arch/arm/mach-omap2/id.c              |   10 ++++++++++
>  arch/arm/mach-omap2/opp3xxx_data.c    |   19 ++++++++++++++++++-
>  arch/arm/plat-omap/include/plat/cpu.h |    2 ++
>  4 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index f0629ae..eebc045 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -365,6 +365,13 @@
>  #define		FEAT_NEON		0
>  #define		FEAT_NEON_NONE		1
>  
> +/*
> + * Product ID register
> + */
> +#define OMAP3_PRODID			0x020C
> +
> +#define OMAP3_SKUID_MASK		0x0f
> +#define		OMAP3_SKUID_720MHZ	0x08
>  
>  #ifndef __ASSEMBLY__
>  #ifdef CONFIG_ARCH_OMAP2PLUS
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index 5f9086c..53fbe01 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
>  	 * TODO: Get additional info (where applicable)
>  	 *       e.g. Size of L2 cache.
>  	 */
> +
> +	/*
> +	 * Does it support 720MHz?
> +	 */
> +	status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> +
> +	if (status & OMAP3_SKUID_720MHZ) {
> +		omap3_features |= OMAP3_HAS_720MHZ;
> +	}
>  }
>  
>  static void __init omap3_check_revision(void)
> @@ -445,6 +454,7 @@ static void __init omap3_cpuinfo(void)
>  	OMAP3_SHOW_FEATURE(neon);
>  	OMAP3_SHOW_FEATURE(isp);
>  	OMAP3_SHOW_FEATURE(192mhz_clk);
> +	OMAP3_SHOW_FEATURE(720mhz);
>  
>  	printk(")\n");
>  }
> diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
> index 0486fce..01582b7 100644
> --- a/arch/arm/mach-omap2/opp3xxx_data.c
> +++ b/arch/arm/mach-omap2/opp3xxx_data.c
> @@ -22,6 +22,9 @@
>  
>  #include "omap_opp_data.h"
>  
> +#define INDEX_MPU_720MHZ	5
> +#define INDEX_IVA_720MHZ	14
> +
>  static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
>  	/* MPU OPP1 */
>  	OPP_INITIALIZER("mpu", true, 125000000, 975000),
> @@ -33,6 +36,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
>  	OPP_INITIALIZER("mpu", true, 550000000, 1270000),
>  	/* MPU OPP5 */
>  	OPP_INITIALIZER("mpu", true, 600000000, 1350000),
> +	/* MPU OPP6 */
> +	OPP_INITIALIZER("mpu", false, 720000000, 1350000),
>  
>  	/*
>  	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
> @@ -58,6 +63,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
>  	OPP_INITIALIZER("iva", true, 400000000, 1270000),
>  	/* DSP OPP5 */
>  	OPP_INITIALIZER("iva", true, 430000000, 1350000),
> +	/* DSP OPP6 */
> +	OPP_INITIALIZER("iva", false, 520000000, 1350000),
>  };
>  
>  static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
> @@ -98,9 +105,19 @@ static int __init omap3_opp_init(void)
>  	if (cpu_is_omap3630())
>  		r = omap_init_opp_table(omap36xx_opp_def_list,
>  			ARRAY_SIZE(omap36xx_opp_def_list));
> -	else
> +	else {
> +		if (omap3_has_720mhz()) {
> +			pr_info("Enabled OPP corresponding to 720MHz\n");
> +
> +			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
> +				.default_available = true;
> +			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
> +				.default_available = true;
> +		}
> +

I'm with Nishanth here.  Please stick to the OPP API for enabling OPPs.

First, it is future proof to internal table changes, but it also sets a
better precedent for others wanting to fiddle with OPPs.

Thanks,

Kevin

>  		r = omap_init_opp_table(omap34xx_opp_def_list,
>  			ARRAY_SIZE(omap34xx_opp_def_list));
> +	}
>  
>  	return r;
>  }
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index 3fd8b40..5c77987 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -455,6 +455,7 @@ extern u32 omap3_features;
>  #define OMAP3_HAS_ISP			BIT(4)
>  #define OMAP3_HAS_192MHZ_CLK		BIT(5)
>  #define OMAP3_HAS_IO_WAKEUP		BIT(6)
> +#define OMAP3_HAS_720MHZ		BIT(7)
>  
>  #define OMAP3_HAS_FEATURE(feat,flag)			\
>  static inline unsigned int omap3_has_ ##feat(void)	\
> @@ -469,5 +470,6 @@ OMAP3_HAS_FEATURE(neon, NEON)
>  OMAP3_HAS_FEATURE(isp, ISP)
>  OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
>  OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> +OMAP3_HAS_FEATURE(720mhz, 720MHZ)
>  
>  #endif

RE: [PATCH] omap3: Add basic support for 720MHz part

[Premi, Sanjeev]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@ti.com] 
> Sent: Friday, January 07, 2011 10:18 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> 
> Sanjeev Premi <premi@ti.com> writes:
> 
> > This patch adds support for new speed enhanced parts with ARM
> > and IVA running at 720MHz and 520MHz respectively. These parts
> > can be probed at run-time by reading PRODID.SKUID[3:0] at
> > 0x4830A20C [1].
> 
> Please expand this a little to describe exactly which parts have this
> feature.  All OMAP3?  34xx? 35xx? what about 36xx/37xx?  ISTR the
> runtime probing for this feature is available on 35xx but not on 34xx,
> but a summary of this should be here.
> 

[sp] Will update...

[snip]...[snip]

> nit: Unrelated to this patch, but OMAP3EVM ethernet is still 
> broken due to
> missing GPIO configuration for the network reset.  This has 
> been broken
> since the GPIO hwmod merge and pointed out but nobody seems 
> to be fixing
> it.

[sp] It is currently in works.

[snip]...[snip] 

> > +			omap34xx_opp_def_list[INDEX_MPU_720MHZ]
> > +				.default_available = true;
> > +			omap34xx_opp_def_list[INDEX_IVA_720MHZ]
> > +				.default_available = true;
> > +		}
> > +
> 
> I'm with Nishanth here.  Please stick to the OPP API for 
> enabling OPPs.
> 
> First, it is future proof to internal table changes, but it 
> also sets a
> better precedent for others wanting to fiddle with OPPs.
> 

[sp] Okay. I will have to revert the a patch on my repo
     to to get there. Will send an update patch on MON.

> Thanks,
> 
> Kevin

[snip]...[snip]