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

[PATCHv2] 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 doe following:
 * Detect devices capable of 720MHz.
 * Add new OPP
 * Ensure that OPP is conditionally enabled.

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

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
Since last revision:
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    |   53 ++++++++++++++++++++++++++++++++-
 arch/arm/plat-omap/include/plat/cpu.h |    2 +
 4 files changed, 71 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..9405c3f 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,46 @@ 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 = omap_hwmod_lookup("iva");
+
+	if (!oh_mpu || !oh_mpu->od) {
+		goto err;
+	} else {
+		r = opp_enable(&(oh_mpu->od->pdev.dev),	720000000);
+		if (r < 0) {
+			pr_err("%s: Unable to enable OPP for mpu.", __func__);
+			goto err;
+		}
+	}
+
+	if (!oh_iva || !oh_iva->od) {
+		r = -ENODEV;
+		goto err;
+	} else {
+		r = opp_enable(&(oh_iva->od->pdev.dev), 520000000);
+		if (r < 0) {
+			pr_err("%s: Unable to enable OPP for iva.", __func__);
+			goto err;
+		}
+	}
+
+	pr_info("Enabled OPP corresponding to 720MHz\n");
+
+err:
+	return r;
+}
+
 /**
  * omap3_opp_init() - initialize omap3 opp table
  */
@@ -98,10 +144,15 @@ 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: [PATCHv2] omap3: Add basic support for 720MHz part

[G, Manjunath Kondaiah]

Hi Sanjeev,

On Thu, Jan 13, 2011 at 12:03:54AM +0530, Sanjeev Premi 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 doe following:
>  * Detect devices capable of 720MHz.
>  * Add new OPP
>  * Ensure that OPP is conditionally enabled.
> 
>   [1] http://focus.ti.com/lit/ug/spruff1d/spruff1d.pdf
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
> Since last revision:
> 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    |   53 ++++++++++++++++++++++++++++++++-
>  arch/arm/plat-omap/include/plat/cpu.h |    2 +
>  4 files changed, 71 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?
> +	 */
multiline style comment
> +	status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> +
> +	if (status & OMAP3_SKUID_720MHZ) {
> +		omap3_features |= OMAP3_HAS_720MHZ;
> +	}
braces not required
>  }
>  
>  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..9405c3f 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,46 @@ 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 = omap_hwmod_lookup("iva");
> +
> +	if (!oh_mpu || !oh_mpu->od) {
> +		goto err;
> +	} else {
> +		r = opp_enable(&(oh_mpu->od->pdev.dev),	720000000);
> +		if (r < 0) {
> +			pr_err("%s: Unable to enable OPP for mpu.", __func__);
since you have dev pointer, pr_err can be replaced with dev_err
> +			goto err;
> +		}
> +	}
> +
> +	if (!oh_iva || !oh_iva->od) {
> +		r = -ENODEV;
> +		goto err;
> +	} else {
> +		r = opp_enable(&(oh_iva->od->pdev.dev), 520000000);
> +		if (r < 0) {
> +			pr_err("%s: Unable to enable OPP for iva.", __func__);
> +			goto err;
> +		}
> +	}
Can be optimized as(not tested and compiled):

	struct platform_device *pdev;
	struct omap_hwmod *oh_mpu = omap_hwmod_lookup("mpu");
	struct omap_hwmod *oh_iva = omap_hwmod_lookup("iva");
	int r = -ENODEV;

	if (!oh_mpu || !oh_mpu->od || !oh_iva || !oh_iva->od)
		goto err;

	pdev = oh_mpu->od->pdev;
	r = opp_enable(&pdev->dev, 720000000);
	if (r < 0) {
		dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n");
		goto err;
	}
	pdev = oh_iva->od->pdev;
	r = opp_enable(&pdev->dev, 520000000);
	if (r < 0) {
		dev_err(&pdev->dev, "Unable to enable OPP for iva.\n");
		goto err;
	}

Do you see any issues?

-Manjunath

[...]

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

[Premi, Sanjeev]

> -----Original Message-----
> From: G, Manjunath Kondaiah [mailto:manjugk@ti.com] 
> Sent: Thursday, January 13, 2011 9:24 AM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
> 
> Hi Sanjeev,
> 
[snip]...[snip]

> >  	 */
> > +
> > +	/*
> > +	 * Does it support 720MHz?
> > +	 */
> multiline style comment
[sp] This is used many places. Don't see any issue with it.
     unless you really want to see this on single line.

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

[sp] Will change. forgot to remove after moving code to a
     different function.

> >  }

[snip]...[snip]

> > +		r = opp_enable(&(oh_mpu->od->pdev.dev),	720000000);
> > +		if (r < 0) {
> > +			pr_err("%s: Unable to enable OPP for 
> mpu.", __func__);
> since you have dev pointer, pr_err can be replaced with dev_err

[sp] Idea was to get to this function - in case of error; but can
     be changed as well.

> > +			goto err;
> > +		}
> > +	}
> > +
> > +	if (!oh_iva || !oh_iva->od) {
> > +		r = -ENODEV;
> > +		goto err;
> > +	} else {
> > +		r = opp_enable(&(oh_iva->od->pdev.dev), 520000000);
> > +		if (r < 0) {
> > +			pr_err("%s: Unable to enable OPP for 
> iva.", __func__);
> > +			goto err;
> > +		}
> > +	}
> Can be optimized as(not tested and compiled):
> 
> 	struct platform_device *pdev;

[sp] Like this! ... to get to "dev".

> 	struct omap_hwmod *oh_mpu = omap_hwmod_lookup("mpu");
> 	struct omap_hwmod *oh_iva = omap_hwmod_lookup("iva");
> 	int r = -ENODEV;
> 
> 	if (!oh_mpu || !oh_mpu->od || !oh_iva || !oh_iva->od)
> 		goto err;
> 
> 	pdev = oh_mpu->od->pdev;
> 	r = opp_enable(&pdev->dev, 720000000);
> 	if (r < 0) {
> 		dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n");
> 		goto err;
> 	}
> 	pdev = oh_iva->od->pdev;
> 	r = opp_enable(&pdev->dev, 520000000);
> 	if (r < 0) {
> 		dev_err(&pdev->dev, "Unable to enable OPP for iva.\n");
> 		goto err;
> 	}
> 
> Do you see any issues?

Though it is not considered in this patch, but in the devices that
don't contain iva, the code related to enabling the iva freq can
be easily excluded in current flow.

Plan to send v3 with all other changes by evening. The optimization
will only be undone once I introduce the check for presence of IVA.

~sanjeev

> 
> -Manjunath
> 
> [...]
> 

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

[G, Manjunath Kondaiah]

On Thu, Jan 13, 2011 at 12:33 PM, Premi, Sanjeev <premi@ti.com> wrote:
>> -----Original Message-----
>> From: G, Manjunath Kondaiah [mailto:manjugk@ti.com]
>> Sent: Thursday, January 13, 2011 9:24 AM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
>>
>> Hi Sanjeev,
>>
> [snip]...[snip]
>
>> >      */
>> > +
>> > +   /*
>> > +    * Does it support 720MHz?
>> > +    */
>> multiline style comment
> [sp] This is used many places.

It will be good if we fix that also.

>Don't see any issue with it.
>     unless you really want to see this on single line.

It is mentioned in http://www.kernel.org/doc/Documentation/CodingStyle

>
>> > +   status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
>> > +
>> > +   if (status & OMAP3_SKUID_720MHZ) {
>> > +           omap3_features |= OMAP3_HAS_720MHZ;
>> > +   }
>> braces not required
>
> [sp] Will change. forgot to remove after moving code to a
>     different function.
>
>> >  }
>
> [snip]...[snip]
>
>> > +           r = opp_enable(&(oh_mpu->od->pdev.dev), 720000000);
>> > +           if (r < 0) {
>> > +                   pr_err("%s: Unable to enable OPP for
>> mpu.", __func__);
>> since you have dev pointer, pr_err can be replaced with dev_err
>
> [sp] Idea was to get to this function - in case of error; but can
>     be changed as well.

you can also use __func__ with dev_err but it may not be required
since dev_err provides
enough information about the error.

>
>> > +                   goto err;
>> > +           }
>> > +   }
>> > +
>> > +   if (!oh_iva || !oh_iva->od) {
>> > +           r = -ENODEV;
>> > +           goto err;
>> > +   } else {
>> > +           r = opp_enable(&(oh_iva->od->pdev.dev), 520000000);
>> > +           if (r < 0) {
>> > +                   pr_err("%s: Unable to enable OPP for
>> iva.", __func__);
>> > +                   goto err;
>> > +           }
>> > +   }
>> Can be optimized as(not tested and compiled):
>>
>>       struct platform_device *pdev;
>
> [sp] Like this! ... to get to "dev".
>
>>       struct omap_hwmod *oh_mpu = omap_hwmod_lookup("mpu");
>>       struct omap_hwmod *oh_iva = omap_hwmod_lookup("iva");
>>       int r = -ENODEV;
>>
>>       if (!oh_mpu || !oh_mpu->od || !oh_iva || !oh_iva->od)
>>               goto err;
>>
>>       pdev = oh_mpu->od->pdev;
>>       r = opp_enable(&pdev->dev, 720000000);
>>       if (r < 0) {
>>               dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n");
>>               goto err;
>>       }
>>       pdev = oh_iva->od->pdev;
>>       r = opp_enable(&pdev->dev, 520000000);
>>       if (r < 0) {
>>               dev_err(&pdev->dev, "Unable to enable OPP for iva.\n");
>>               goto err;
>>       }
>>
>> Do you see any issues?
>
> Though it is not considered in this patch, but in the devices that
> don't contain iva, the code related to enabling the iva freq can
> be easily excluded in current flow.

In this case, the function returns error if we have device with no
iva. The API should
enable only mpu opp and return sane value for devices with no iva.

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

[Premi, Sanjeev]

> -----Original Message-----
> From: G, Manjunath Kondaiah 
> Sent: Thursday, January 13, 2011 2:58 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
> 

[....]

> >>               goto err;
> >>       }
> >>
> >> Do you see any issues?
> >
> > Though it is not considered in this patch, but in the devices that
> > don't contain iva, the code related to enabling the iva freq can
> > be easily excluded in current flow.
> 
> In this case, the function returns error if we have device with no
> iva. The API should
> enable only mpu opp and return sane value for devices with no iva.
> 

[sp] Don't want to get into details of next patch here, but when there
is no IVA, the "if" condition for for iva won't/shouldn't be entered.

Values returned should just be the status opp_enable. OR am I missing
something in your comment.
e.g.
	pdev = oh_mpu->od->pdev;
	r = opp_enable(&pdev->dev, 720000000);
	if (r < 0) {
		dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n");
		goto err;
	}

	if omap3_has_iva()) {
		oh_iva = omap_hwmod_lookup("iva"); //move from current impl.

		pdev = oh_iva->od->pdev;
		r = opp_enable(&pdev->dev, 520000000);
		if (r < 0) {
			dev_err(&pdev->dev, "Unable to enable OPP for iva.\n");
			goto err;
		}
	}

~sanjeev

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

[G, Manjunath Kondaiah]

On Thu, Jan 13, 2011 at 4:48 PM, Premi, Sanjeev <premi@ti.com> wrote:
>> -----Original Message-----
>> From: G, Manjunath Kondaiah
>> Sent: Thursday, January 13, 2011 2:58 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
>>
>
> [....]
>
>> >>               goto err;
>> >>       }
>> >>
>> >> Do you see any issues?
>> >
>> > Though it is not considered in this patch, but in the devices that
>> > don't contain iva, the code related to enabling the iva freq can
>> > be easily excluded in current flow.
>>
>> In this case, the function returns error if we have device with no
>> iva. The API should
>> enable only mpu opp and return sane value for devices with no iva.
>>
>
> [sp] Don't want to get into details of next patch here, but when there
> is no IVA, the "if" condition for for iva won't/shouldn't be entered.
exactly
>
> Values returned should just be the status opp_enable. OR am I missing
> something in your comment.
> e.g.
>        pdev = oh_mpu->od->pdev;
>        r = opp_enable(&pdev->dev, 720000000);
>        if (r < 0) {
>                dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n");
>                goto err;
>        }
>
>        if omap3_has_iva()) {
>                oh_iva = omap_hwmod_lookup("iva"); //move from current impl.

This looks fine and it can translate into:
	if (!oh_mpu || !oh_mpu->od)
               goto err_nodevice;

       pdev = oh_mpu->od->pdev;
       r = opp_enable(&pdev->dev, 720000000);
       if (r < 0)
               goto err_oppenable;

	if (!omap3_has_iva())
		goto err_nodevice;

	r = -ENODEV;
	if(!oh_iva || !oh_iva->od)
               goto err_nodevice;

       pdev = oh_iva->od->pdev;
       r = opp_enable(&pdev->dev, 520000000);
       if (r < 0)
               goto err_oppenable;

	return r;

err_oppenable:
	dev_err(&pdev->dev, "Unable to enable OPP\n");
err_nodevice:
	return r;

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

[Premi, Sanjeev]

> -----Original Message-----
> From: G, Manjunath Kondaiah [mailto:manjugk@ti.com] 
> Sent: Thursday, January 13, 2011 5:16 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
> 
> On Thu, Jan 13, 2011 at 4:48 PM, Premi, Sanjeev <premi@ti.com> wrote:
> >> -----Original Message-----
> >> From: G, Manjunath Kondaiah
> >> Sent: Thursday, January 13, 2011 2:58 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
> >>
> >
> > [....]
> >
> >> >>               goto err;
> >> >>       }
> >> >>
> >> >> Do you see any issues?
> >> >
> >> > Though it is not considered in this patch, but in the 
> devices that
> >> > don't contain iva, the code related to enabling the iva freq can
> >> > be easily excluded in current flow.
> >>
> >> In this case, the function returns error if we have device with no
> >> iva. The API should
> >> enable only mpu opp and return sane value for devices with no iva.
> >>
> >
> > [sp] Don't want to get into details of next patch here, but 
> when there
> > is no IVA, the "if" condition for for iva won't/shouldn't 
> be entered.
> exactly
> >
> > Values returned should just be the status opp_enable. OR am 
> I missing
> > something in your comment.
> > e.g.
> >        pdev = oh_mpu->od->pdev;
> >        r = opp_enable(&pdev->dev, 720000000);
> >        if (r < 0) {
> >                dev_err(&pdev->dev, "Unable to enable OPP 
> for mpu.\n");
> >                goto err;
> >        }
> >
> >        if omap3_has_iva()) {
> >                oh_iva = omap_hwmod_lookup("iva"); //move 
> from current impl.
> 
> This looks fine and it can translate into:
> 	if (!oh_mpu || !oh_mpu->od)
>                goto err_nodevice;
> 
>        pdev = oh_mpu->od->pdev;
>        r = opp_enable(&pdev->dev, 720000000);
>        if (r < 0)
>                goto err_oppenable;
> 
> 	if (!omap3_has_iva())
> 		goto err_nodevice;

Not having an IVA is not error! and goto's really aren't saving anything.

> 
> 	r = -ENODEV;
> 	if(!oh_iva || !oh_iva->od)
>                goto err_nodevice;
> 
>        pdev = oh_iva->od->pdev;
>        r = opp_enable(&pdev->dev, 520000000);
>        if (r < 0)
>                goto err_oppenable;
> 
> 	return r;
> 
> err_oppenable:
> 	dev_err(&pdev->dev, "Unable to enable OPP\n");
> err_nodevice:
> 	return r;
> 
> -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: [PATCHv2] omap3: Add basic support for 720MHz part

[G, Manjunath Kondaiah]

On Thu, Jan 13, 2011 at 5:21 PM, Premi, Sanjeev <premi@ti.com> wrote:
>
>> -----Original Message-----
>> From: G, Manjunath Kondaiah [mailto:manjugk@ti.com]
>> Sent: Thursday, January 13, 2011 5:16 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
>>
>> On Thu, Jan 13, 2011 at 4:48 PM, Premi, Sanjeev <premi@ti.com> wrote:
>> >> -----Original Message-----
>> >> From: G, Manjunath Kondaiah
>> >> Sent: Thursday, January 13, 2011 2:58 PM
>> >> To: Premi, Sanjeev
>> >> Cc: linux-omap@vger.kernel.org
>> >> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part
>> >>
>> >
>> > [....]
>> >
>> >> >>               goto err;
>> >> >>       }
>> >> >>
>> >> >> Do you see any issues?
>> >> >
>> >> > Though it is not considered in this patch, but in the
>> devices that
>> >> > don't contain iva, the code related to enabling the iva freq can
>> >> > be easily excluded in current flow.
>> >>
>> >> In this case, the function returns error if we have device with no
>> >> iva. The API should
>> >> enable only mpu opp and return sane value for devices with no iva.
>> >>
>> >
>> > [sp] Don't want to get into details of next patch here, but
>> when there
>> > is no IVA, the "if" condition for for iva won't/shouldn't
>> be entered.
>> exactly
>> >
>> > Values returned should just be the status opp_enable. OR am
>> I missing
>> > something in your comment.
>> > e.g.
>> >        pdev = oh_mpu->od->pdev;
>> >        r = opp_enable(&pdev->dev, 720000000);
>> >        if (r < 0) {
>> >                dev_err(&pdev->dev, "Unable to enable OPP
>> for mpu.\n");
>> >                goto err;
>> >        }
>> >
>> >        if omap3_has_iva()) {
>> >                oh_iva = omap_hwmod_lookup("iva"); //move
>> from current impl.
>>
>> This looks fine and it can translate into:
>>       if (!oh_mpu || !oh_mpu->od)
>>                goto err_nodevice;
>>
>>        pdev = oh_mpu->od->pdev;
>>        r = opp_enable(&pdev->dev, 720000000);
>>        if (r < 0)
>>                goto err_oppenable;
>>
>>       if (!omap3_has_iva())
>>               goto err_nodevice;
>
> Not having an IVA is not error! and goto's really aren't saving anything.

it is returning value from mpu opp enable and goto naming is
misleading. You can change the name if it is misleading.

-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