Skip to contentSkip to conversation

omap3: Add basic support for 720MHz part

PATCH Superseded Sanjeev Premi 2 participants 8 messages Jan 12, 2011 – Jan 13, 2011

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sanjeev Premi

author
#0 lore raw

PATCH v2 1/1 omap3: Add basic support for 720MHz part

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 <[email protected]>

---
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(-)
arch/arm/mach-omap2/control.h +7 -0
index f0629ae..eebc045 100644
@@ -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
arch/arm/mach-omap2/id.c +10 -0
index 5f9086c..53fbe01 100644
@@ -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");
 }
arch/arm/mach-omap2/opp3xxx_data.c +52 -1

88 lines not on this page · raw message

arch/arm/plat-omap/include/plat/cpu.h +2 -0
index 3fd8b40..5c77987 100644
@@ -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
Signature · 1 line
1.7.2.2
#1 lore raw

Hi Sanjeev,

Quoted 54 lines · 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 <[email protected]>

---
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(-)
arch/arm/mach-omap2/control.h +7 -0
index f0629ae..eebc045 100644
@@ -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
arch/arm/mach-omap2/id.c +0 -0
index 5f9086c..53fbe01 100644

arch/arm/mach-omap2/id.c static void __init omap3_check_features(void) · line 201

Quoted 7 lines
arch/arm/mach-omap2/id.c +4 -0
@@ -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

arch/arm/mach-omap2/id.c static void __init omap3_check_features(void) · line 206

Quoted 5 lines
arch/arm/mach-omap2/id.c +5 -0
+	status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
+
+	if (status & OMAP3_SKUID_720MHZ) {
+		omap3_features |= OMAP3_HAS_720MHZ;
+	}

braces not required

Quoted 57 lines
arch/arm/mach-omap2/id.c +1 -0
 }
 
 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");
 }
arch/arm/mach-omap2/opp3xxx_data.c +15 -0
index 0486fce..9405c3f 100644
@@ -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)
+{

arch/arm/mach-omap2/opp3xxx_data.c static struct omap_opp_def __initdata omap36xx_opp_def_list[] = { · line 112

Quoted 10 lines
arch/arm/mach-omap2/opp3xxx_data.c +10 -0
+	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

Quoted 4 lines
arch/arm/mach-omap2/opp3xxx_data.c +4 -0
+			goto err;
+		}
+	}
+

arch/arm/mach-omap2/opp3xxx_data.c static struct omap_opp_def __initdata omap36xx_opp_def_list[] = { · line 126

Quoted 10 lines
arch/arm/mach-omap2/opp3xxx_data.c +10 -0
+	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

[...]

author
#2 lore raw
Quoted 9 lines

-----Original Message-----
From: G, Manjunath Kondaiah [mailto:[email protected]]
Sent: Thursday, January 13, 2011 9:24 AM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Hi Sanjeev,

[snip]...[snip]

Quoted 6 lines

arch/arm/mach-omap2/id.c static void __init omap3_check_features(void) · line 201

Quoted 5 lines
arch/arm/mach-omap2/id.c +4 -0
 	 */
+
+	/*
+	 * 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.

Quoted 6 lines

arch/arm/mach-omap2/id.c static void __init omap3_check_features(void) · line 206

Quoted 5 lines
arch/arm/mach-omap2/id.c +5 -0
+	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.

Quoted 1 line
Quoted 1 line

}

[snip]...[snip]

Quoted 5 lines

arch/arm/mach-omap2/opp3xxx_data.c static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {

Quoted 3 lines
arch/arm/mach-omap2/opp3xxx_data.c +3 -0
+		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.

Quoted 12 lines

arch/arm/mach-omap2/opp3xxx_data.c static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {

Quoted 11 lines
arch/arm/mach-omap2/opp3xxx_data.c +11 -0
+			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__);

Quoted 6 lines

arch/arm/mach-omap2/opp3xxx_data.c static struct omap_opp_def __initdata omap36xx_opp_def_list[] = { · line 115

Quoted 3 lines
arch/arm/mach-omap2/opp3xxx_data.c +3 -0
+			goto err;
+		}
+	}

Can be optimized as(not tested and compiled):

	struct platform_device *pdev;

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

Quoted 20 lines
	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;
	}
Quoted 1 line

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

Quoted 5 lines

-Manjunath

[...]

#3 lore raw
Quoted 11 lines · Sanjeev
Quoted 9 lines

-----Original Message-----
From: G, Manjunath Kondaiah [mailto:[email protected]]
Sent: Thursday, January 13, 2011 9:24 AM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Hi Sanjeev,

[snip]...[snip]

Quoted 7 lines
Quoted 6 lines
Quoted 5 lines

     */
+
+   /*
+    * Does it support 720MHz?
+    */

multiline style comment

[sp] This is used many places.

It will be good if we fix that also.

Quoted 2 lines

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

Quoted 15 lines
Quoted 7 lines
Quoted 6 lines

+   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.

Quoted 1 line
Quoted 1 line

 }

[snip]...[snip]

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

+           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.

Quoted 42 lines
Quoted 12 lines
Quoted 12 lines

+                   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

Quoted 7 lines

iva.", __func__);

Quoted 3 lines

+                   goto err;
+           }
+   }

Can be optimized as(not tested and compiled):

      struct platform_device *pdev;

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

Quoted 20 lines

      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;
      }

Quoted 5 lines
Quoted 1 line

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw
Quoted 7 lines

-----Original Message-----
From: G, Manjunath Kondaiah
Sent: Thursday, January 13, 2011 2:58 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

[....]

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

              goto err;
      }

Do you see any issues?

Quoted 8 lines
Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 19 lines · Sanjeev
Quoted 7 lines

-----Original Message-----
From: G, Manjunath Kondaiah
Sent: Thursday, January 13, 2011 2:58 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

[....]

Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

              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.

Quoted 7 lines
Quoted 4 lines

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

Quoted 11 lines

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;
       }

Quoted 2 lines

       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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#6 lore raw
Quoted 57 lines

-----Original Message-----
From: G, Manjunath Kondaiah [mailto:[email protected]]
Sent: Thursday, January 13, 2011 5:16 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Quoted 8 lines · Sanjeev
Quoted 7 lines

-----Original Message-----
From: G, Manjunath Kondaiah
Sent: Thursday, January 13, 2011 2:58 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Quoted 8 lines

[....]

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

              goto err;
      }

Do you see any issues?

Though it is not considered in this patch, but in the

devices that

Quoted 9 lines
Quoted 7 lines
Quoted 2 lines

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

Quoted 1 line

is no IVA, the "if" condition for for iva won't/shouldn't

be entered.
exactly

Quoted 2 lines

Values returned should just be the status opp_enable. OR am

I missing

Quoted 6 lines

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");

Quoted 5 lines

               goto err;
       }

       if omap3_has_iva()) {
               oh_iva = omap_hwmod_lookup("iva"); //move

from current impl.

This looks fine and it can translate into:

Quoted 10 lines
	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.

Quoted 13 lines
	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:

Quoted 8 lines
	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 65 lines · Sanjeev
Quoted 65 lines

-----Original Message-----
From: G, Manjunath Kondaiah [mailto:[email protected]]
Sent: Thursday, January 13, 2011 5:16 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Quoted 8 lines · Sanjeev
Quoted 7 lines

-----Original Message-----
From: G, Manjunath Kondaiah
Sent: Thursday, January 13, 2011 2:58 PM
To: Premi, Sanjeev
Cc: [email protected]
Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part

Quoted 8 lines

[....]

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

              goto err;
      }

Do you see any issues?

Though it is not considered in this patch, but in the

devices that

Quoted 9 lines
Quoted 7 lines
Quoted 2 lines

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

Quoted 1 line

is no IVA, the "if" condition for for iva won't/shouldn't

be entered.
exactly

Quoted 2 lines

Values returned should just be the status opp_enable. OR am

I missing

Quoted 6 lines

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");

Quoted 5 lines

               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;

Quoted 4 lines
Quoted 2 lines

      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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html