[PATCH v4 0/3] OMAP: Add opp data

[PATCH v4 0/3] OMAP: Add opp data

[Nishanth Menon]

Major changes in V4:
	Rename of oppxxx_data.h to data.c, move device_init there
	omap_init_opp_table now will return -EEXIST if
	it was called previously.

V3: http://marc.info/?l=linux-omap&m=128984926812800&w=2
V2: http://marc.info/?t=128753665300003&r=1&w=2

Kevin Hilman (1):
  OMAP3: remove OPP interfaces from OMAP PM layer

Nishanth Menon (2):
  omap: opp: add OMAP3 OPP table data and common init
  omap4: opp: add OPP table data

 Documentation/arm/OMAP/omap_pm            |   26 ++++++
 arch/arm/mach-omap2/Kconfig               |    2 +
 arch/arm/mach-omap2/Makefile              |    2 +
 arch/arm/mach-omap2/io.c                  |    3 +-
 arch/arm/mach-omap2/opp.c                 |  135 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c        |   98 +++++++++++++++++++++
 arch/arm/mach-omap2/opp4xxx_data.c        |   49 +++++++++++
 arch/arm/mach-omap2/pm.h                  |   14 +++
 arch/arm/plat-omap/include/plat/omap-pm.h |   31 ++-----
 arch/arm/plat-omap/omap-pm-noop.c         |   11 +--
 10 files changed, 338 insertions(+), 33 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp.c
 create mode 100644 arch/arm/mach-omap2/opp3xxx_data.c
 create mode 100644 arch/arm/mach-omap2/opp4xxx_data.c

 Regards,
 Nishanth Menon

[PATCH v4 1/3] omap: opp: add OMAP3 OPP table data and common init

[Nishanth Menon]

Add OPP data for OMAP34xx and OMAP36xx and initialization functions
to populate OPP tables based on current SoC.
introduce an OMAP generic opp initialization routine which OMAP3
and OMAP4+ SoCs can use to register their OPP definitions.

Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Warning: http://lkml.org/lkml/2010/11/9/389
Introduces ARCH_HAS_OPP which needs to be enabled as well
for OMAP3 in the Kconfig
v4:
	Comments from Thomas addressed:
	* Data switched to .c file and the c file included in opp.c
	* init_table will fail with -EEXIST if already called
	* minor comment improvements
	Not addressed:
	* request for board files to explicitly call init table
	  as discussed http://marc.info/?l=linux-omap&m=128992417530385&w=2

v3: http://marc.info/?t=128984939100006&r=1&w=2
	* added documentation for custom opp modification
	  by board files
	* switched to using device_initcall to autoinitialize the
	  opp tables
v2: https://patchwork.kernel.org/patch/266911/

 Documentation/arm/OMAP/omap_pm     |   26 +++++++
 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/Makefile       |    2 +
 arch/arm/mach-omap2/opp.c          |  134 ++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c |   98 ++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h           |    9 +++
 6 files changed, 270 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp.c
 create mode 100644 arch/arm/mach-omap2/opp3xxx_data.c

diff --git a/Documentation/arm/OMAP/omap_pm b/Documentation/arm/OMAP/omap_pm
index 5389440..88341f0 100644
--- a/Documentation/arm/OMAP/omap_pm
+++ b/Documentation/arm/OMAP/omap_pm
@@ -127,3 +127,29 @@ implementation needs:
 10. (*pdata->cpu_set_freq)(unsigned long f)
 
 11. (*pdata->cpu_get_freq)(void)
+
+Customizing OPP for platform
+============================
+Defining CONFIG_PM should enable OPP layer for the silicon
+and the registration of OPP table should take place automatically.
+However, in special cases, the default OPP table may need to be
+tweaked, for e.g.:
+ * enable default OPPs which are disabled by default, but which
+   could be enabled on a platform
+ * Disable an unsupported OPP on the platform
+ * Define and add a custom opp table entry
+in these cases, the board file needs to do additional steps as follows:
+arch/arm/mach-omapx/board-xyz.c
+	#include "pm.h"
+	....
+	static void __init omap_xyz_init_irq(void)
+	{
+		....
+		/* Initialize the default table */
+		omapx_opp_init();
+		/* Do customization to the defaults */
+		....
+	}
+NOTE: omapx_opp_init will be omap3_opp_init or as required
+based on the omap family.
+
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index ab784bf..93a91ff 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -35,6 +35,7 @@ config ARCH_OMAP3
 	select CPU_V7
 	select USB_ARCH_HAS_EHCI
 	select ARM_L1_CACHE_SHIFT_6 if !ARCH_OMAP4
+ 	select PM_OPP if PM
 
 config ARCH_OMAP4
 	bool "TI OMAP4"
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 60e51bc..1650a62 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -64,6 +64,8 @@ endif
 
 endif
 
+obj-$(CONFIG_PM_OPP)		+= opp.o
+
 # PRCM
 obj-$(CONFIG_ARCH_OMAP2)		+= cm.o
 obj-$(CONFIG_ARCH_OMAP3)		+= cm.o
diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
new file mode 100644
index 0000000..66e12be
--- /dev/null
+++ b/arch/arm/mach-omap2/opp.c
@@ -0,0 +1,134 @@
+/*
+ *  OMAP SoC specific OPP wrapper function
+ *
+ * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
+ *	Nishanth Menon
+ * Copyright (C) 2009 - 2010 Deep Root Systems, LLC.
+ *	Kevin Hilman
+ * Copyright (C) 2010 Nokia Corporation.
+ *      Eduardo Valentin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/opp.h>
+
+#include <plat/cpu.h>
+#include <plat/omap_device.h>
+
+#include "pm.h"
+
+/**
+ * struct omap_opp_def - OMAP OPP Definition
+ * @hwmod_name:	Name of the hwmod for this domain
+ * @freq:	Frequency in hertz corresponding to this OPP
+ * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
+ * @enabled:	True/false - is this OPP enabled/disabled by default
+ *
+ * OMAP SOCs have a standard set of tuples consisting of frequency and voltage
+ * pairs that the device will support per voltage domain. This is called
+ * Operating Points or OPP. The actual definitions of OMAP Operating Points
+ * varies over silicon within the same family of devices. For a specific
+ * domain, you can have a set of {frequency, voltage} pairs and this is denoted
+ * by an array of omap_opp_def. As the kernel boots and more information is
+ * available, a set of these are activated based on the precise nature of
+ * device the kernel boots up on. It is interesting to remember that each IP
+ * which belongs to a voltage domain may define their own set of OPPs on top
+ * of this - but this is handled by the appropriate driver.
+ */
+struct omap_opp_def {
+	char *hwmod_name;
+
+	unsigned long freq;
+	unsigned long u_volt;
+
+	bool default_available;
+};
+
+/*
+ * Initialization wrapper used to define an OPP for OMAP variants.
+ */
+#define OPP_INITIALIZER(_hwmod_name, _enabled, _freq, _uv)	\
+{								\
+	.hwmod_name	= _hwmod_name,				\
+	.default_available	= _enabled,			\
+	.freq		= _freq,				\
+	.u_volt		= _uv,					\
+}
+
+/* Temp variable to allow multiple calls */
+static u8 __initdata omap_table_init;
+
+/**
+ * omap_init_opp_table() - Initialize opp table as per the CPU type
+ * @opp_def:		opp default list for this silicon
+ * @opp_def_size:	number of opp entries for this silicon
+ *
+ * Register the initial OPP table with the OPP library based on the CPU
+ * type.
+ */
+static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
+		u32 opp_def_size)
+{
+	int i, r;
+
+	if (!opp_def || !opp_def_size) {
+		pr_err("%s: invalid params!\n", __func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * Initialize only if not already initialized even if the previous
+	 * call failed, because, no reason we'd succeed again.
+	 */
+	if (omap_table_init)
+		return -EEXIST;
+	omap_table_init = 1;
+
+	/* Lets now register with OPP library */
+	for (i = 0; i < opp_def_size; i++) {
+		struct omap_hwmod *oh;
+		struct device *dev;
+
+		if (!opp_def->hwmod_name) {
+			pr_err("%s: NULL name of omap_hwmod, failing [%d].\n",
+				__func__, i);
+			return -EINVAL;
+		}
+		oh = omap_hwmod_lookup(opp_def->hwmod_name);
+		if (!oh || !oh->od) {
+			pr_warn("%s: no hwmod or odev for %s, [%d] "
+				"cannot add OPPs.\n", __func__,
+				opp_def->hwmod_name, i);
+			return -EINVAL;
+		}
+		dev = &oh->od->pdev.dev;
+
+		r = opp_add(dev, opp_def->freq, opp_def->u_volt);
+		if (r) {
+			dev_err(dev, "%s: add OPP %ld failed for %s [%d] "
+				"result=%d\n",
+			       __func__, opp_def->freq,
+			       opp_def->hwmod_name, i, r);
+		} else {
+			if (!opp_def->default_available)
+				r = opp_disable(dev, opp_def->freq);
+			if (r)
+				dev_err(dev, "%s: disable %ld failed for %s "
+					"[%d] result=%d\n",
+					__func__, opp_def->freq,
+					opp_def->hwmod_name, i, r);
+		}
+		opp_def++;
+	}
+
+	return 0;
+}
+
+/* omap3 opps */
+#include "opp3xxx_data.c"
+
diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
new file mode 100644
index 0000000..46cbd53
--- /dev/null
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -0,0 +1,98 @@
+/*
+ * OMAP3 OPP table definitions.
+ *
+ * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
+ *	Nishanth Menon
+ *	Kevin Hilman
+ * Copyright (C) 2010 Nokia Corporation.
+ *      Eduardo Valentin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
+	/* MPU OPP1 */
+	OPP_INITIALIZER("mpu", true, 125000000, 975000),
+	/* MPU OPP2 */
+	OPP_INITIALIZER("mpu", true, 250000000, 1075000),
+	/* MPU OPP3 */
+	OPP_INITIALIZER("mpu", true, 500000000, 1200000),
+	/* MPU OPP4 */
+	OPP_INITIALIZER("mpu", true, 550000000, 1270000),
+	/* MPU OPP5 */
+	OPP_INITIALIZER("mpu", true, 600000000, 1350000),
+
+	/*
+	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
+	 * almost the same than the one at 83MHz thus providing very little
+	 * gain for the power point of view. In term of energy it will even
+	 * increase the consumption due to the very negative performance
+	 * impact that frequency will do to the MPU and the whole system in
+	 * general.
+	 */
+	OPP_INITIALIZER("l3_main", false, 41500000, 975000),
+	/* L3 OPP2 */
+	OPP_INITIALIZER("l3_main", true, 83000000, 1050000),
+	/* L3 OPP3 */
+	OPP_INITIALIZER("l3_main", true, 166000000, 1150000),
+
+
+	/* DSP OPP1 */
+	OPP_INITIALIZER("iva", true, 90000000, 975000),
+	/* DSP OPP2 */
+	OPP_INITIALIZER("iva", true, 180000000, 1075000),
+	/* DSP OPP3 */
+	OPP_INITIALIZER("iva", true, 360000000, 1200000),
+	/* DSP OPP4 */
+	OPP_INITIALIZER("iva", true, 400000000, 1270000),
+	/* DSP OPP5 */
+	OPP_INITIALIZER("iva", true, 430000000, 1350000),
+};
+
+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
+	/* MPU OPP1 - OPP50 */
+	OPP_INITIALIZER("mpu", true,  300000000, 1012500),
+	/* MPU OPP2 - OPP100 */
+	OPP_INITIALIZER("mpu", true,  600000000, 1200000),
+	/* MPU OPP3 - OPP-Turbo */
+	OPP_INITIALIZER("mpu", false, 800000000, 1325000),
+	/* MPU OPP4 - OPP-SB */
+	OPP_INITIALIZER("mpu", false, 1000000000, 1375000),
+
+	/* L3 OPP1 - OPP50 */
+	OPP_INITIALIZER("l3_main", true, 100000000, 1000000),
+	/* L3 OPP2 - OPP100, OPP-Turbo, OPP-SB */
+	OPP_INITIALIZER("l3_main", true, 200000000, 1200000),
+
+	/* DSP OPP1 - OPP50 */
+	OPP_INITIALIZER("iva", true,  260000000, 1012500),
+	/* DSP OPP2 - OPP100 */
+	OPP_INITIALIZER("iva", true,  520000000, 1200000),
+	/* DSP OPP3 - OPP-Turbo */
+	OPP_INITIALIZER("iva", false, 660000000, 1325000),
+	/* DSP OPP4 - OPP-SB */
+	OPP_INITIALIZER("iva", false, 800000000, 1375000),
+};
+
+/**
+ * omap3_opp_init() - initialize omap3 opp table
+ */
+int __init omap3_opp_init(void)
+{
+	int r = -ENODEV;
+
+	if (!cpu_is_omap34xx())
+		return r;
+
+	if (cpu_is_omap3630())
+		r = omap_init_opp_table(omap36xx_opp_def_list,
+			ARRAY_SIZE(omap36xx_opp_def_list));
+	else
+		r = omap_init_opp_table(omap34xx_opp_def_list,
+			ARRAY_SIZE(omap34xx_opp_def_list));
+
+	return r;
+}
+device_initcall(omap3_opp_init);
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 0d75bfd..2031f15 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -20,6 +20,15 @@ extern int omap3_can_sleep(void);
 extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
 extern int omap3_idle_init(void);
 
+#if defined(CONFIG_PM_OPP)
+extern int omap3_opp_init(void);
+#else
+static inline int omap3_opp_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 struct cpuidle_params {
 	u8  valid;
 	u32 sleep_latency;
-- 
1.6.3.3

[PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

This patch adds OPP tables for OMAP4. New file has been added to keep
the OMAP4 opp tables and the registration of these tables with the
generic opp framework by OMAP SoC OPP interface.

Based on:
http://dev.omapzoom.org/?p=santosh/kernel-omap4-base.git;a=blob;f=arch/arm/mach-omap2/opp44xx_data.c;h=252e3d0cb6050a64f390b9311c1c4977d74f762a;hb=refs/heads/omap4_next

Signed-off-by: Thara Gopinath <thara@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Warning: http://lkml.org/lkml/2010/11/9/389
Introduces ARCH_HAS_OPP which needs to be enabled as well
for OMAP4 in Kconfig

V4: switched data to .c file
v3: http://marc.info/?l=linux-omap&m=128984926712794&w=2
	* switched to using device_initcall to autoinitialize the
	  opp tables
v2: https://patchwork.kernel.org/patch/266921/

 arch/arm/mach-omap2/Kconfig        |    1 +
 arch/arm/mach-omap2/opp.c          |    3 +-
 arch/arm/mach-omap2/opp4xxx_data.c |   49 ++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h           |    5 +++
 4 files changed, 57 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp4xxx_data.c

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 93a91ff..0f1855a 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -45,6 +45,7 @@ config ARCH_OMAP4
 	select ARM_GIC
 	select PL310_ERRATA_588369
 	select ARM_ERRATA_720789
+ 	select PM_OPP if PM
 
 comment "OMAP Core Type"
 	depends on ARCH_OMAP2
diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
index 66e12be..48a553f 100644
--- a/arch/arm/mach-omap2/opp.c
+++ b/arch/arm/mach-omap2/opp.c
@@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
 
 /* omap3 opps */
 #include "opp3xxx_data.c"
-
+/* omap4 opps */
+#include "opp4xxx_data.c"
diff --git a/arch/arm/mach-omap2/opp4xxx_data.c b/arch/arm/mach-omap2/opp4xxx_data.c
new file mode 100644
index 0000000..6271774
--- /dev/null
+++ b/arch/arm/mach-omap2/opp4xxx_data.c
@@ -0,0 +1,49 @@
+/*
+ * OMAP4 OPP table definitions.
+ *
+ * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
+ *	Nishanth Menon
+ * Copyright (C) 2009 - 2010 Deep Root Systems, LLC.
+ *	Kevin Hilman
+ * Copyright (C) 2010 Nokia Corporation.
+ *      Eduardo Valentin
+ * Copyright (C) 2010 Texas Instruments Incorporated.
+ *	Thara Gopinath
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+static struct omap_opp_def __initdata omap44xx_opp_def_list[] = {
+	/* MPU OPP1 - OPP50 */
+	OPP_INITIALIZER("mpu", true, 300000000, 1100000),
+	/* MPU OPP2 - OPP100 */
+	OPP_INITIALIZER("mpu", true, 600000000, 1200000),
+	/* MPU OPP3 - OPP-Turbo */
+	OPP_INITIALIZER("mpu", false, 800000000, 1260000),
+	/* MPU OPP4 - OPP-SB */
+	OPP_INITIALIZER("mpu", false, 1008000000, 1350000),
+	/* L3 OPP1 - OPP50 */
+	OPP_INITIALIZER("l3_main_1", true, 100000000, 930000),
+	/* L3 OPP2 - OPP100, OPP-Turbo, OPP-SB */
+	OPP_INITIALIZER("l3_main_1", true, 200000000, 1100000),
+	/* TODO: add IVA, DSP, aess, fdif, gpu */
+};
+
+/**
+ * omap4_opp_init() - initialize omap4 opp table
+ */
+int __init omap4_opp_init(void)
+{
+	int r = -ENODEV;
+
+	if (!cpu_is_omap44xx())
+		return r;
+
+	r = omap_init_opp_table(omap44xx_opp_def_list,
+			ARRAY_SIZE(omap44xx_opp_def_list));
+
+	return r;
+}
+device_initcall(omap4_opp_init);
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 2031f15..a43e069 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -22,11 +22,16 @@ extern int omap3_idle_init(void);
 
 #if defined(CONFIG_PM_OPP)
 extern int omap3_opp_init(void);
+extern int omap4_opp_init(void);
 #else
 static inline int omap3_opp_init(void)
 {
 	return -EINVAL;
 }
+static inline int omap4_opp_init(void)
+{
+	return -EINVAL;
+}
 #endif
 
 struct cpuidle_params {
-- 
1.6.3.3

[PATCH v3 3/3] OMAP3: remove OPP interfaces from OMAP PM layer

[Nishanth Menon]

From: Kevin Hilman <khilman@deeprootsystems.com>

With new OPP layer, OPP users will access OPP API directly instead of
using OMAP PM layer, so remove all notions of OPPs from the OMAP PM
layer.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---

posted for completeness sake - no change involved
v3: no longer explicitly calling the init_table, instead
    depending on the device_initcall to initialize as needed

v2: https://patchwork.kernel.org/patch/266931/

 arch/arm/mach-omap2/io.c                  |    3 +-
 arch/arm/plat-omap/include/plat/omap-pm.h |   31 +++++++++-------------------
 arch/arm/plat-omap/omap-pm-noop.c         |   11 +---------
 3 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 40562dd..2fe4e02 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -327,8 +327,7 @@ void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
 	else if (cpu_is_omap44xx())
 		omap44xx_hwmod_init();
 
-	/* The OPP tables have to be registered before a clk init */
-	omap_pm_if_early_init(mpu_opps, dsp_opps, l3_opps);
+	omap_pm_if_early_init();
 
 	if (cpu_is_omap2420())
 		omap2420_clk_init();
diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
index 728fbb9..62c3fe9 100644
--- a/arch/arm/plat-omap/include/plat/omap-pm.h
+++ b/arch/arm/plat-omap/include/plat/omap-pm.h
@@ -17,27 +17,10 @@
 #include <linux/device.h>
 #include <linux/cpufreq.h>
 #include <linux/clk.h>
+#include <linux/opp.h>
 
 #include "powerdomain.h"
 
-/**
- * struct omap_opp - clock frequency-to-OPP ID table for DSP, MPU
- * @rate: target clock rate
- * @opp_id: OPP ID
- * @min_vdd: minimum VDD1 voltage (in millivolts) for this OPP
- *
- * Operating performance point data.  Can vary by OMAP chip and board.
- */
-struct omap_opp {
-	unsigned long rate;
-	u8 opp_id;
-	u16 min_vdd;
-};
-
-extern struct omap_opp *mpu_opps;
-extern struct omap_opp *dsp_opps;
-extern struct omap_opp *l3_opps;
-
 /*
  * agent_id values for use with omap_pm_set_min_bus_tput():
  *
@@ -59,9 +42,11 @@ extern struct omap_opp *l3_opps;
  * framework starts.  The "_if_" is to avoid name collisions with the
  * PM idle-loop code.
  */
-int __init omap_pm_if_early_init(struct omap_opp *mpu_opp_table,
-				 struct omap_opp *dsp_opp_table,
-				 struct omap_opp *l3_opp_table);
+#ifdef CONFIG_OMAP_PM_NONE
+#define omap_pm_if_early_init() 0
+#else
+int __init omap_pm_if_early_init(void);
+#endif
 
 /**
  * omap_pm_if_init - OMAP PM init code called after clock fw init
@@ -69,7 +54,11 @@ int __init omap_pm_if_early_init(struct omap_opp *mpu_opp_table,
  * The main initialization code.  OPP tables are passed in here.  The
  * "_if_" is to avoid name collisions with the PM idle-loop code.
  */
+#ifdef CONFIG_OMAP_PM_NONE
+#define omap_pm_if_init() 0
+#else
 int __init omap_pm_if_init(void);
+#endif
 
 /**
  * omap_pm_if_exit - OMAP PM exit code
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index e129ce8..ca75abb 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -26,10 +26,6 @@
 
 #include <plat/powerdomain.h>
 
-struct omap_opp *dsp_opps;
-struct omap_opp *mpu_opps;
-struct omap_opp *l3_opps;
-
 /*
  * Device-driver-originated constraints (via board-*.c files)
  */
@@ -308,13 +304,8 @@ int omap_pm_get_dev_context_loss_count(struct device *dev)
 
 
 /* Should be called before clk framework init */
-int __init omap_pm_if_early_init(struct omap_opp *mpu_opp_table,
-				 struct omap_opp *dsp_opp_table,
-				 struct omap_opp *l3_opp_table)
+int __init omap_pm_if_early_init(void)
 {
-	mpu_opps = mpu_opp_table;
-	dsp_opps = dsp_opp_table;
-	l3_opps = l3_opp_table;
 	return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH v4 1/3] omap: opp: add OMAP3 OPP table data and common init

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Add OPP data for OMAP34xx and OMAP36xx and initialization functions
> to populate OPP tables based on current SoC.
> introduce an OMAP generic opp initialization routine which OMAP3
> and OMAP4+ SoCs can use to register their OPP definitions.
>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

Some minor comments below...

> ---
> Warning: http://lkml.org/lkml/2010/11/9/389
> Introduces ARCH_HAS_OPP which needs to be enabled as well
> for OMAP3 in the Kconfig
> v4:
> 	Comments from Thomas addressed:
> 	* Data switched to .c file and the c file included in opp.c
> 	* init_table will fail with -EEXIST if already called
> 	* minor comment improvements
> 	Not addressed:
> 	* request for board files to explicitly call init table
> 	  as discussed http://marc.info/?l=linux-omap&m=128992417530385&w=2
>
> v3: http://marc.info/?t=128984939100006&r=1&w=2
> 	* added documentation for custom opp modification
> 	  by board files
> 	* switched to using device_initcall to autoinitialize the
> 	  opp tables
> v2: https://patchwork.kernel.org/patch/266911/
>
>  Documentation/arm/OMAP/omap_pm     |   26 +++++++
>  arch/arm/mach-omap2/Kconfig        |    1 +
>  arch/arm/mach-omap2/Makefile       |    2 +
>  arch/arm/mach-omap2/opp.c          |  134 ++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/opp3xxx_data.c |   98 ++++++++++++++++++++++++++
>  arch/arm/mach-omap2/pm.h           |    9 +++
>  6 files changed, 270 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/opp.c
>  create mode 100644 arch/arm/mach-omap2/opp3xxx_data.c
>
> diff --git a/Documentation/arm/OMAP/omap_pm b/Documentation/arm/OMAP/omap_pm
> index 5389440..88341f0 100644
> --- a/Documentation/arm/OMAP/omap_pm
> +++ b/Documentation/arm/OMAP/omap_pm
> @@ -127,3 +127,29 @@ implementation needs:
>  10. (*pdata->cpu_set_freq)(unsigned long f)
>  
>  11. (*pdata->cpu_get_freq)(void)
> +
> +Customizing OPP for platform
> +============================
> +Defining CONFIG_PM should enable OPP layer for the silicon
> +and the registration of OPP table should take place automatically.
> +However, in special cases, the default OPP table may need to be
> +tweaked, for e.g.:
> + * enable default OPPs which are disabled by default, but which
> +   could be enabled on a platform
> + * Disable an unsupported OPP on the platform
> + * Define and add a custom opp table entry
> +in these cases, the board file needs to do additional steps as follows:
> +arch/arm/mach-omapx/board-xyz.c
> +	#include "pm.h"
> +	....
> +	static void __init omap_xyz_init_irq(void)
> +	{
> +		....
> +		/* Initialize the default table */
> +		omapx_opp_init();
> +		/* Do customization to the defaults */
> +		....
> +	}
> +NOTE: omapx_opp_init will be omap3_opp_init or as required
> +based on the omap family.
> +

new blank line at EOF (reported by git-apply)

> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index ab784bf..93a91ff 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -35,6 +35,7 @@ config ARCH_OMAP3
>  	select CPU_V7
>  	select USB_ARCH_HAS_EHCI
>  	select ARM_L1_CACHE_SHIFT_6 if !ARCH_OMAP4
> + 	select PM_OPP if PM

spaces before tab here (reported by git-apply)

>  
>  config ARCH_OMAP4
>  	bool "TI OMAP4"
> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
> index 60e51bc..1650a62 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -64,6 +64,8 @@ endif
>  
>  endif
>  
> +obj-$(CONFIG_PM_OPP)		+= opp.o
> +
>  # PRCM
>  obj-$(CONFIG_ARCH_OMAP2)		+= cm.o
>  obj-$(CONFIG_ARCH_OMAP3)		+= cm.o
> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
> new file mode 100644
> index 0000000..66e12be
> --- /dev/null
> +++ b/arch/arm/mach-omap2/opp.c
> @@ -0,0 +1,134 @@
> +/*
> + *  OMAP SoC specific OPP wrapper function
> + *
> + * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
> + *	Nishanth Menon
> + * Copyright (C) 2009 - 2010 Deep Root Systems, LLC.
> + *	Kevin Hilman
> + * Copyright (C) 2010 Nokia Corporation.
> + *      Eduardo Valentin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/opp.h>
> +
> +#include <plat/cpu.h>
> +#include <plat/omap_device.h>
> +
> +#include "pm.h"
> +
> +/**
> + * struct omap_opp_def - OMAP OPP Definition
> + * @hwmod_name:	Name of the hwmod for this domain
> + * @freq:	Frequency in hertz corresponding to this OPP
> + * @u_volt:	Nominal voltage in microvolts corresponding to this OPP
> + * @enabled:	True/false - is this OPP enabled/disabled by default
> + *
> + * OMAP SOCs have a standard set of tuples consisting of frequency and voltage
> + * pairs that the device will support per voltage domain. This is called
> + * Operating Points or OPP. The actual definitions of OMAP Operating Points
> + * varies over silicon within the same family of devices. For a specific
> + * domain, you can have a set of {frequency, voltage} pairs and this is denoted
> + * by an array of omap_opp_def. As the kernel boots and more information is
> + * available, a set of these are activated based on the precise nature of
> + * device the kernel boots up on. It is interesting to remember that each IP
> + * which belongs to a voltage domain may define their own set of OPPs on top
> + * of this - but this is handled by the appropriate driver.
> + */
> +struct omap_opp_def {
> +	char *hwmod_name;
> +
> +	unsigned long freq;
> +	unsigned long u_volt;
> +
> +	bool default_available;
> +};
> +
> +/*
> + * Initialization wrapper used to define an OPP for OMAP variants.
> + */
> +#define OPP_INITIALIZER(_hwmod_name, _enabled, _freq, _uv)	\
> +{								\
> +	.hwmod_name	= _hwmod_name,				\
> +	.default_available	= _enabled,			\
> +	.freq		= _freq,				\
> +	.u_volt		= _uv,					\
> +}
> +
> +/* Temp variable to allow multiple calls */
> +static u8 __initdata omap_table_init;
> +
> +/**
> + * omap_init_opp_table() - Initialize opp table as per the CPU type
> + * @opp_def:		opp default list for this silicon
> + * @opp_def_size:	number of opp entries for this silicon
> + *
> + * Register the initial OPP table with the OPP library based on the CPU
> + * type.
> + */
> +static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
> +		u32 opp_def_size)
> +{
> +	int i, r;
> +
> +	if (!opp_def || !opp_def_size) {
> +		pr_err("%s: invalid params!\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Initialize only if not already initialized even if the previous
> +	 * call failed, because, no reason we'd succeed again.
> +	 */
> +	if (omap_table_init)
> +		return -EEXIST;
> +	omap_table_init = 1;
> +
> +	/* Lets now register with OPP library */
> +	for (i = 0; i < opp_def_size; i++) {
> +		struct omap_hwmod *oh;
> +		struct device *dev;
> +
> +		if (!opp_def->hwmod_name) {
> +			pr_err("%s: NULL name of omap_hwmod, failing [%d].\n",
> +				__func__, i);
> +			return -EINVAL;
> +		}
> +		oh = omap_hwmod_lookup(opp_def->hwmod_name);
> +		if (!oh || !oh->od) {
> +			pr_warn("%s: no hwmod or odev for %s, [%d] "
> +				"cannot add OPPs.\n", __func__,
> +				opp_def->hwmod_name, i);
> +			return -EINVAL;
> +		}
> +		dev = &oh->od->pdev.dev;
> +
> +		r = opp_add(dev, opp_def->freq, opp_def->u_volt);
> +		if (r) {
> +			dev_err(dev, "%s: add OPP %ld failed for %s [%d] "
> +				"result=%d\n",
> +			       __func__, opp_def->freq,
> +			       opp_def->hwmod_name, i, r);
> +		} else {
> +			if (!opp_def->default_available)
> +				r = opp_disable(dev, opp_def->freq);
> +			if (r)
> +				dev_err(dev, "%s: disable %ld failed for %s "
> +					"[%d] result=%d\n",
> +					__func__, opp_def->freq,
> +					opp_def->hwmod_name, i, r);
> +		}
> +		opp_def++;
> +	}
> +
> +	return 0;
> +}
> +
> +/* omap3 opps */
> +#include "opp3xxx_data.c"
> +

extra blank line at EOF

> diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
> new file mode 100644
> index 0000000..46cbd53
> --- /dev/null
> +++ b/arch/arm/mach-omap2/opp3xxx_data.c
> @@ -0,0 +1,98 @@
> +/*
> + * OMAP3 OPP table definitions.
> + *
> + * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
> + *	Nishanth Menon
> + *	Kevin Hilman
> + * Copyright (C) 2010 Nokia Corporation.
> + *      Eduardo Valentin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
> +	/* MPU OPP1 */
> +	OPP_INITIALIZER("mpu", true, 125000000, 975000),
> +	/* MPU OPP2 */
> +	OPP_INITIALIZER("mpu", true, 250000000, 1075000),
> +	/* MPU OPP3 */
> +	OPP_INITIALIZER("mpu", true, 500000000, 1200000),
> +	/* MPU OPP4 */
> +	OPP_INITIALIZER("mpu", true, 550000000, 1270000),
> +	/* MPU OPP5 */
> +	OPP_INITIALIZER("mpu", true, 600000000, 1350000),
> +
> +	/*
> +	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
> +	 * almost the same than the one at 83MHz thus providing very little
> +	 * gain for the power point of view. In term of energy it will even
> +	 * increase the consumption due to the very negative performance
> +	 * impact that frequency will do to the MPU and the whole system in
> +	 * general.
> +	 */
> +	OPP_INITIALIZER("l3_main", false, 41500000, 975000),
> +	/* L3 OPP2 */
> +	OPP_INITIALIZER("l3_main", true, 83000000, 1050000),
> +	/* L3 OPP3 */
> +	OPP_INITIALIZER("l3_main", true, 166000000, 1150000),
> +
> +
> +	/* DSP OPP1 */
> +	OPP_INITIALIZER("iva", true, 90000000, 975000),
> +	/* DSP OPP2 */
> +	OPP_INITIALIZER("iva", true, 180000000, 1075000),
> +	/* DSP OPP3 */
> +	OPP_INITIALIZER("iva", true, 360000000, 1200000),
> +	/* DSP OPP4 */
> +	OPP_INITIALIZER("iva", true, 400000000, 1270000),
> +	/* DSP OPP5 */
> +	OPP_INITIALIZER("iva", true, 430000000, 1350000),
> +};
> +
> +static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
> +	/* MPU OPP1 - OPP50 */
> +	OPP_INITIALIZER("mpu", true,  300000000, 1012500),
> +	/* MPU OPP2 - OPP100 */
> +	OPP_INITIALIZER("mpu", true,  600000000, 1200000),
> +	/* MPU OPP3 - OPP-Turbo */
> +	OPP_INITIALIZER("mpu", false, 800000000, 1325000),
> +	/* MPU OPP4 - OPP-SB */
> +	OPP_INITIALIZER("mpu", false, 1000000000, 1375000),
> +
> +	/* L3 OPP1 - OPP50 */
> +	OPP_INITIALIZER("l3_main", true, 100000000, 1000000),
> +	/* L3 OPP2 - OPP100, OPP-Turbo, OPP-SB */
> +	OPP_INITIALIZER("l3_main", true, 200000000, 1200000),
> +
> +	/* DSP OPP1 - OPP50 */
> +	OPP_INITIALIZER("iva", true,  260000000, 1012500),
> +	/* DSP OPP2 - OPP100 */
> +	OPP_INITIALIZER("iva", true,  520000000, 1200000),
> +	/* DSP OPP3 - OPP-Turbo */
> +	OPP_INITIALIZER("iva", false, 660000000, 1325000),
> +	/* DSP OPP4 - OPP-SB */
> +	OPP_INITIALIZER("iva", false, 800000000, 1375000),
> +};
> +
> +/**
> + * omap3_opp_init() - initialize omap3 opp table
> + */
> +int __init omap3_opp_init(void)
> +{
> +	int r = -ENODEV;
> +
> +	if (!cpu_is_omap34xx())
> +		return r;
> +
> +	if (cpu_is_omap3630())
> +		r = omap_init_opp_table(omap36xx_opp_def_list,
> +			ARRAY_SIZE(omap36xx_opp_def_list));
> +	else
> +		r = omap_init_opp_table(omap34xx_opp_def_list,
> +			ARRAY_SIZE(omap34xx_opp_def_list));
> +
> +	return r;
> +}
> +device_initcall(omap3_opp_init);
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 0d75bfd..2031f15 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -20,6 +20,15 @@ extern int omap3_can_sleep(void);
>  extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
>  extern int omap3_idle_init(void);
>  
> +#if defined(CONFIG_PM_OPP)
> +extern int omap3_opp_init(void);
> +#else
> +static inline int omap3_opp_init(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  struct cpuidle_params {
>  	u8  valid;
>  	u32 sleep_latency;

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> This patch adds OPP tables for OMAP4. New file has been added to keep
> the OMAP4 opp tables and the registration of these tables with the
> generic opp framework by OMAP SoC OPP interface.
>
> Based on:
> http://dev.omapzoom.org/?p=santosh/kernel-omap4-base.git;a=blob;f=arch/arm/mach-omap2/opp44xx_data.c;h=252e3d0cb6050a64f390b9311c1c4977d74f762a;hb=refs/heads/omap4_next
>
> Signed-off-by: Thara Gopinath <thara@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Warning: http://lkml.org/lkml/2010/11/9/389
> Introduces ARCH_HAS_OPP which needs to be enabled as well
> for OMAP4 in Kconfig
>
> V4: switched data to .c file
> v3: http://marc.info/?l=linux-omap&m=128984926712794&w=2
> 	* switched to using device_initcall to autoinitialize the
> 	  opp tables
> v2: https://patchwork.kernel.org/patch/266921/
>
>  arch/arm/mach-omap2/Kconfig        |    1 +
>  arch/arm/mach-omap2/opp.c          |    3 +-
>  arch/arm/mach-omap2/opp4xxx_data.c |   49 ++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/pm.h           |    5 +++
>  4 files changed, 57 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/opp4xxx_data.c
>
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index 93a91ff..0f1855a 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -45,6 +45,7 @@ config ARCH_OMAP4
>  	select ARM_GIC
>  	select PL310_ERRATA_588369
>  	select ARM_ERRATA_720789
> + 	select PM_OPP if PM

spaces before tab (reported by git-apply)

>  comment "OMAP Core Type"
>  	depends on ARCH_OMAP2
> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
> index 66e12be..48a553f 100644
> --- a/arch/arm/mach-omap2/opp.c
> +++ b/arch/arm/mach-omap2/opp.c
> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>  
>  /* omap3 opps */
>  #include "opp3xxx_data.c"
> -
> +/* omap4 opps */
> +#include "opp4xxx_data.c"
> diff --git a/arch/arm/mach-omap2/opp4xxx_data.c b/arch/arm/mach-omap2/opp4xxx_data.c
> new file mode 100644
> index 0000000..6271774
> --- /dev/null
> +++ b/arch/arm/mach-omap2/opp4xxx_data.c
> @@ -0,0 +1,49 @@
> +/*
> + * OMAP4 OPP table definitions.
> + *
> + * Copyright (C) 2009 - 2010 Texas Instruments Incorporated.
> + *	Nishanth Menon
> + * Copyright (C) 2009 - 2010 Deep Root Systems, LLC.
> + *	Kevin Hilman
> + * Copyright (C) 2010 Nokia Corporation.
> + *      Eduardo Valentin
> + * Copyright (C) 2010 Texas Instruments Incorporated.
> + *	Thara Gopinath
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +static struct omap_opp_def __initdata omap44xx_opp_def_list[] = {
> +	/* MPU OPP1 - OPP50 */
> +	OPP_INITIALIZER("mpu", true, 300000000, 1100000),
> +	/* MPU OPP2 - OPP100 */
> +	OPP_INITIALIZER("mpu", true, 600000000, 1200000),
> +	/* MPU OPP3 - OPP-Turbo */
> +	OPP_INITIALIZER("mpu", false, 800000000, 1260000),
> +	/* MPU OPP4 - OPP-SB */
> +	OPP_INITIALIZER("mpu", false, 1008000000, 1350000),
> +	/* L3 OPP1 - OPP50 */
> +	OPP_INITIALIZER("l3_main_1", true, 100000000, 930000),
> +	/* L3 OPP2 - OPP100, OPP-Turbo, OPP-SB */
> +	OPP_INITIALIZER("l3_main_1", true, 200000000, 1100000),
> +	/* TODO: add IVA, DSP, aess, fdif, gpu */
> +};
> +
> +/**
> + * omap4_opp_init() - initialize omap4 opp table
> + */
> +int __init omap4_opp_init(void)
> +{
> +	int r = -ENODEV;
> +
> +	if (!cpu_is_omap44xx())
> +		return r;
> +
> +	r = omap_init_opp_table(omap44xx_opp_def_list,
> +			ARRAY_SIZE(omap44xx_opp_def_list));
> +
> +	return r;
> +}
> +device_initcall(omap4_opp_init);
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 2031f15..a43e069 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -22,11 +22,16 @@ extern int omap3_idle_init(void);
>  
>  #if defined(CONFIG_PM_OPP)
>  extern int omap3_opp_init(void);
> +extern int omap4_opp_init(void);
>  #else
>  static inline int omap3_opp_init(void)
>  {
>  	return -EINVAL;
>  }
> +static inline int omap4_opp_init(void)
> +{
> +	return -EINVAL;
> +}
>  #endif

Now that these are device_initcalls, seems like they can be made static
and not callable externally.

Kevin

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

Kevin Hilman wrote, on 11/22/2010 04:45 PM:
>>   static inline int omap3_opp_init(void)
>> >    {
>> >    	return -EINVAL;
>> >    }
>> >  +static inline int omap4_opp_init(void)
>> >  +{
>> >  +	return -EINVAL;
>> >  +}
>> >    #endif
> Now that these are device_initcalls, seems like they can be made static
> and not callable externally.
how do we handle boards that need custom opp table modifications?

-- 
Regards,
Nishanth Menon

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman wrote, on 11/22/2010 04:45 PM:
>>>   static inline int omap3_opp_init(void)
>>> >    {
>>> >    	return -EINVAL;
>>> >    }
>>> >  +static inline int omap4_opp_init(void)
>>> >  +{
>>> >  +	return -EINVAL;
>>> >  +}
>>> >    #endif
>> Now that these are device_initcalls, seems like they can be made static
>> and not callable externally.
>
> how do we handle boards that need custom opp table modifications?

um..., er... ok, I was just testing to see if you were awake today.  :/

Clearly I'm not remembering even my own feature suggestions.  Sorry for
the noise.  Guess I'm trying to review too many different series today.

Kevin

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> This patch adds OPP tables for OMAP4. New file has been added to keep
> the OMAP4 opp tables and the registration of these tables with the
> generic opp framework by OMAP SoC OPP interface.

[...]

> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
> index 66e12be..48a553f 100644
> --- a/arch/arm/mach-omap2/opp.c
> +++ b/arch/arm/mach-omap2/opp.c
> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>  
>  /* omap3 opps */
>  #include "opp3xxx_data.c"
> -
> +/* omap4 opps */
> +#include "opp4xxx_data.c"

I'm not sure I like the including of C files.  Any reason you prefer
this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
compiled in via Makefile and these two are included.

I vote for consistency.

Kevin

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

Kevin Hilman wrote, on 11/22/2010 05:19 PM:
> Nishanth Menon<nm@ti.com>  writes:
>
>> This patch adds OPP tables for OMAP4. New file has been added to keep
>> the OMAP4 opp tables and the registration of these tables with the
>> generic opp framework by OMAP SoC OPP interface.
>
> [...]
>
>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>> index 66e12be..48a553f 100644
>> --- a/arch/arm/mach-omap2/opp.c
>> +++ b/arch/arm/mach-omap2/opp.c
>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>>
>>   /* omap3 opps */
>>   #include "opp3xxx_data.c"
>> -
>> +/* omap4 opps */
>> +#include "opp4xxx_data.c"
>
> I'm not sure I like the including of C files.  Any reason you prefer
> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
> compiled in via Makefile and these two are included.

I dont buy it. I am seeing us go around in circles for this:
http://marc.info/?l=linux-omap&m=128986880406272&w=2
a) we dont want others to use specifics implemented in opp.c in other 
files (e.g. board files)
b) we have many similar usage in linux kernel - so this usage is not 
first time.
c) opp2xx usage is very different from opp3/4 usage

-- 
Regards,
Nishanth Menon

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>> Nishanth Menon<nm@ti.com>  writes:
>>
>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>> the OMAP4 opp tables and the registration of these tables with the
>>> generic opp framework by OMAP SoC OPP interface.
>>
>> [...]
>>
>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>> index 66e12be..48a553f 100644
>>> --- a/arch/arm/mach-omap2/opp.c
>>> +++ b/arch/arm/mach-omap2/opp.c
>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>>>
>>>   /* omap3 opps */
>>>   #include "opp3xxx_data.c"
>>> -
>>> +/* omap4 opps */
>>> +#include "opp4xxx_data.c"
>>
>> I'm not sure I like the including of C files.  Any reason you prefer
>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>> compiled in via Makefile and these two are included.
>
> I dont buy it. I am seeing us go around in circles for this:
> http://marc.info/?l=linux-omap&m=128986880406272&w=2
> a) we dont want others to use specifics implemented in opp.c in other
> files (e.g. board files)

not sure how this is prevented.

> b) we have many similar usage in linux kernel - so this usage is not
> first time.
> c) opp2xx usage is very different from opp3/4 usage

I'm not going to insist on one way or the other, just stating my
preference for not including C files from C files without good
justification.  You've stated your reasons, I guess Tony can decide.

Kevin

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>> Nishanth Menon<nm@ti.com>  writes:
>>>
>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>> the OMAP4 opp tables and the registration of these tables with the
>>>> generic opp framework by OMAP SoC OPP interface.
>>> [...]
>>>
>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>> index 66e12be..48a553f 100644
>>>> --- a/arch/arm/mach-omap2/opp.c
>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>>>>
>>>>   /* omap3 opps */
>>>>   #include "opp3xxx_data.c"
>>>> -
>>>> +/* omap4 opps */
>>>> +#include "opp4xxx_data.c"
>>> I'm not sure I like the including of C files.  Any reason you prefer
>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>> compiled in via Makefile and these two are included.
>> I dont buy it. I am seeing us go around in circles for this:
>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>> a) we dont want others to use specifics implemented in opp.c in other
>> files (e.g. board files)
> 
> not sure how this is prevented.
The approach is as follows:
the #defines and struct definitions are in opp.c
opp{3,4}xxx_data.c use the same - by itself wont build because they need 
the defines in opp.c
that way, there is no possibility of clean hacks possible except to 
follow the current mechanism for a future omap silicon

The main objective was to restrict the potential of board developers 
from hacking the default OPP table - which would be possible by exposing 
the headers for board files - which as Thomas rightly pointed out in the 
  thread I pointed out,  opens up a possibility for board files to 
include them as well and re-instantiate the table again..

> 
>> b) we have many similar usage in linux kernel - so this usage is not
>> first time.
>> c) opp2xx usage is very different from opp3/4 usage
> 
> I'm not going to insist on one way or the other, just stating my
> preference for not including C files from C files without good
> justification.  You've stated your reasons, I guess Tony can decide.
okay by me - will post a v5 after seeing the final decision on that front :)

-- 
Regards,
Nishanth Menon

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>>> Nishanth Menon<nm@ti.com>  writes:
>>>>
>>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>>> the OMAP4 opp tables and the registration of these tables with the
>>>>> generic opp framework by OMAP SoC OPP interface.
>>>> [...]
>>>>
>>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>>> index 66e12be..48a553f 100644
>>>>> --- a/arch/arm/mach-omap2/opp.c
>>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>>>>>
>>>>>   /* omap3 opps */
>>>>>   #include "opp3xxx_data.c"
>>>>> -
>>>>> +/* omap4 opps */
>>>>> +#include "opp4xxx_data.c"
>>>> I'm not sure I like the including of C files.  Any reason you prefer
>>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>>> compiled in via Makefile and these two are included.
>>> I dont buy it. I am seeing us go around in circles for this:
>>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>>> a) we dont want others to use specifics implemented in opp.c in other
>>> files (e.g. board files)
>>
>> not sure how this is prevented.
> The approach is as follows:
> the #defines and struct definitions are in opp.c
> opp{3,4}xxx_data.c use the same - by itself wont build because they
> need the defines in opp.c
> that way, there is no possibility of clean hacks possible except to
> follow the current mechanism for a future omap silicon
>
> The main objective was to restrict the potential of board developers
> from hacking the default OPP table - which would be possible by
> exposing the headers for board files - which as Thomas rightly pointed
> out in the thread I pointed out,  opens up a possibility for board
> files to include them as well and re-instantiate the table again..

OK, then this falls into the "trick to make ugly hacks *really* ugly"
category.   I can live with that. :)

Kevin

>>
>>> b) we have many similar usage in linux kernel - so this usage is not
>>> first time.
>>> c) opp2xx usage is very different from opp3/4 usage
>>
>> I'm not going to insist on one way or the other, just stating my
>> preference for not including C files from C files without good
>> justification.  You've stated your reasons, I guess Tony can decide.
> okay by me - will post a v5 after seeing the final decision on that front :)

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Cousson, Benoit]

Hi Nishanth,

On 11/23/2010 4:19 PM, Menon, Nishanth wrote:
> Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
>> Nishanth Menon<nm@ti.com>  writes:
>>
>>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>>> Nishanth Menon<nm@ti.com>   writes:
>>>>
>>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>>> the OMAP4 opp tables and the registration of these tables with the
>>>>> generic opp framework by OMAP SoC OPP interface.
>>>> [...]
>>>>
>>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>>> index 66e12be..48a553f 100644
>>>>> --- a/arch/arm/mach-omap2/opp.c
>>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct omap_opp_def *opp_def,
>>>>>
>>>>>    /* omap3 opps */
>>>>>    #include "opp3xxx_data.c"
>>>>> -
>>>>> +/* omap4 opps */
>>>>> +#include "opp4xxx_data.c"
>>>> I'm not sure I like the including of C files.  Any reason you prefer
>>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>>> compiled in via Makefile and these two are included.
>>> I dont buy it. I am seeing us go around in circles for this:
>>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>>> a) we dont want others to use specifics implemented in opp.c in other
>>> files (e.g. board files)
>>
>> not sure how this is prevented.
> The approach is as follows:
> the #defines and struct definitions are in opp.c
> opp{3,4}xxx_data.c use the same - by itself wont build because they need
> the defines in opp.c
> that way, there is no possibility of clean hacks possible except to
> follow the current mechanism for a future omap silicon
>
> The main objective was to restrict the potential of board developers
> from hacking the default OPP table - which would be possible by exposing
> the headers for board files - which as Thomas rightly pointed out in the
>    thread I pointed out,  opens up a possibility for board files to
> include them as well and re-instantiate the table again..
>
>>
>>> b) we have many similar usage in linux kernel - so this usage is not
>>> first time.
>>> c) opp2xx usage is very different from opp3/4 usage
>>
>> I'm not going to insist on one way or the other, just stating my
>> preference for not including C files from C files without good
>> justification.  You've stated your reasons, I guess Tony can decide.
> okay by me - will post a v5 after seeing the final decision on that front :)

FYI, Paul changed the clock and hwmod data files from include to C 
files, because Russell didn't like at all the include of data file.

Why cannot we handle the opp data the same way we are managing clock, 
hwmod or even pad mux data for various Soc?

Regards,
Benoit

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

Cousson, Benoit had written, on 11/23/2010 04:33 PM, the following:
> Hi Nishanth,
> 
> On 11/23/2010 4:19 PM, Menon, Nishanth wrote:
>> Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
>>> Nishanth Menon<nm@ti.com>  writes:
>>>
>>>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>>>> Nishanth Menon<nm@ti.com>   writes:
>>>>>
>>>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>>>> the OMAP4 opp tables and the registration of these tables with the
>>>>>> generic opp framework by OMAP SoC OPP interface.
>>>>> [...]
>>>>>
>>>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>>>> index 66e12be..48a553f 100644
>>>>>> --- a/arch/arm/mach-omap2/opp.c
>>>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct 
>>>>>> omap_opp_def *opp_def,
>>>>>>
>>>>>>    /* omap3 opps */
>>>>>>    #include "opp3xxx_data.c"
>>>>>> -
>>>>>> +/* omap4 opps */
>>>>>> +#include "opp4xxx_data.c"
>>>>> I'm not sure I like the including of C files.  Any reason you prefer
>>>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>>>> compiled in via Makefile and these two are included.
>>>> I dont buy it. I am seeing us go around in circles for this:
>>>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>>>> a) we dont want others to use specifics implemented in opp.c in other
>>>> files (e.g. board files)
>>>
>>> not sure how this is prevented.
>> The approach is as follows:
>> the #defines and struct definitions are in opp.c
>> opp{3,4}xxx_data.c use the same - by itself wont build because they need
>> the defines in opp.c
>> that way, there is no possibility of clean hacks possible except to
>> follow the current mechanism for a future omap silicon
>>
>> The main objective was to restrict the potential of board developers
>> from hacking the default OPP table - which would be possible by exposing
>> the headers for board files - which as Thomas rightly pointed out in the
>>    thread I pointed out,  opens up a possibility for board files to
>> include them as well and re-instantiate the table again..
[...]
> 
> FYI, Paul changed the clock and hwmod data files from include to C 
> files, because Russell didn't like at all the include of data file.
> 
> Why cannot we handle the opp data the same way we are managing clock, 
> hwmod or even pad mux data for various Soc?

Objective:
I do not want folks to start creating their own custom "default" OPP 
tables for silicon and registering with OPP framework. intent is to have 
the default OPP tables in one place - here.

Solution I followed:
make the following:
a) struct omap_opp_def
b) OPP_INITIALIZER
c) omap_init_opp_table
not available for anyone other than opp.c - they are defined in opp.c 
and not available externally.

If these were in a header, I am inviting failure of my objective. yes, I 
could review it when posted to l-o and NAK it, but wont prevent private 
trees from basically(IMHO) misusing the framework

Options available:
a) v1,2,3 of the series basically used a header opp{3,4}xxx_data .h
in v3, I received a comment (IMHO rightly so) that a header defining a 
static array implies capability for some other file to include the 
header and misuse it (basically headers are meant to share data structs 
between x and y - if you dont intend to do that, then put it in a c file)
b) in v4, I converted the data header to a .c and included the data.c in 
opp.c.

NOTE:
a) padconf, hwmod, clock are designed to be reused by rest of 
mach-omap2. opp defines are meant to be isolated. I dont really have a 
preference c or header file for the data, just that I want to, by design 
"make any hacks attempts - *really ugly*"
b) Inclusion of c in c files are not uniquely introduced here.
$ find . -iname "*.c"|xargs grep "#include"| grep -v "\.h"|grep 
":#include"|wc -l
558


Hope this clarifies.

Regards,
Nishanth Menon

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Cousson, Benoit]

On 11/23/2010 11:56 PM, Menon, Nishanth wrote:
> Cousson, Benoit had written, on 11/23/2010 04:33 PM, the following:
>> Hi Nishanth,
>>
>> On 11/23/2010 4:19 PM, Menon, Nishanth wrote:
>>> Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
>>>> Nishanth Menon<nm@ti.com>   writes:
>>>>
>>>>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>>>>> Nishanth Menon<nm@ti.com>    writes:
>>>>>>
>>>>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>>>>> the OMAP4 opp tables and the registration of these tables with the
>>>>>>> generic opp framework by OMAP SoC OPP interface.
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>>>>> index 66e12be..48a553f 100644
>>>>>>> --- a/arch/arm/mach-omap2/opp.c
>>>>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct
>>>>>>> omap_opp_def *opp_def,
>>>>>>>
>>>>>>>     /* omap3 opps */
>>>>>>>     #include "opp3xxx_data.c"
>>>>>>> -
>>>>>>> +/* omap4 opps */
>>>>>>> +#include "opp4xxx_data.c"
>>>>>> I'm not sure I like the including of C files.  Any reason you prefer
>>>>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>>>>> compiled in via Makefile and these two are included.
>>>>> I dont buy it. I am seeing us go around in circles for this:
>>>>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>>>>> a) we dont want others to use specifics implemented in opp.c in other
>>>>> files (e.g. board files)
>>>>
>>>> not sure how this is prevented.
>>> The approach is as follows:
>>> the #defines and struct definitions are in opp.c
>>> opp{3,4}xxx_data.c use the same - by itself wont build because they need
>>> the defines in opp.c
>>> that way, there is no possibility of clean hacks possible except to
>>> follow the current mechanism for a future omap silicon
>>>
>>> The main objective was to restrict the potential of board developers
>>> from hacking the default OPP table - which would be possible by exposing
>>> the headers for board files - which as Thomas rightly pointed out in the
>>>     thread I pointed out,  opens up a possibility for board files to
>>> include them as well and re-instantiate the table again..
> [...]
>>
>> FYI, Paul changed the clock and hwmod data files from include to C
>> files, because Russell didn't like at all the include of data file.
>>
>> Why cannot we handle the opp data the same way we are managing clock,
>> hwmod or even pad mux data for various Soc?
>
> Objective:
> I do not want folks to start creating their own custom "default" OPP
> tables for silicon and registering with OPP framework. intent is to have
> the default OPP tables in one place - here.
>
> Solution I followed:
> make the following:
> a) struct omap_opp_def
> b) OPP_INITIALIZER
> c) omap_init_opp_table
> not available for anyone other than opp.c - they are defined in opp.c
> and not available externally.
>
> If these were in a header, I am inviting failure of my objective. yes, I
> could review it when posted to l-o and NAK it, but wont prevent private
> trees from basically(IMHO) misusing the framework
>
> Options available:
> a) v1,2,3 of the series basically used a header opp{3,4}xxx_data .h
> in v3, I received a comment (IMHO rightly so) that a header defining a
> static array implies capability for some other file to include the
> header and misuse it (basically headers are meant to share data structs
> between x and y - if you dont intend to do that, then put it in a c file)
> b) in v4, I converted the data header to a .c and included the data.c in
> opp.c.
>
> NOTE:
> a) padconf, hwmod, clock are designed to be reused by rest of
> mach-omap2. opp defines are meant to be isolated. I dont really have a
> preference c or header file for the data, just that I want to, by design
> "make any hacks attempts - *really ugly*"

Mmm, I'm not convince that this will prevent anybody to do dirty stuff 
anyway.
Doing "dirty" stuff to avoid even more dirty stuff is kind of a weird 
approach... for my point of view at least...

And no, this is not that different than the hwmod data or clock data... 
These data are never manipulated directly by anybody but the core code.

> b) Inclusion of c in c files are not uniquely introduced here.
> $ find . -iname "*.c"|xargs grep "#include"| grep -v "\.h"|grep
> ":#include"|wc -l
> 558

That does not mean that we have to keep doing it.
There are probably much more lines of code that does not stick to the 
CodingStyle ;-)

> Hope this clarifies.
Yes, but that still does not justify that for my point of view.

Benoit

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Kevin Hilman]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> On 11/23/2010 11:56 PM, Menon, Nishanth wrote:
>> Cousson, Benoit had written, on 11/23/2010 04:33 PM, the following:
>>> Hi Nishanth,
>>>
>>> On 11/23/2010 4:19 PM, Menon, Nishanth wrote:
>>>> Kevin Hilman had written, on 11/22/2010 06:09 PM, the following:
>>>>> Nishanth Menon<nm@ti.com>   writes:
>>>>>
>>>>>> Kevin Hilman wrote, on 11/22/2010 05:19 PM:
>>>>>>> Nishanth Menon<nm@ti.com>    writes:
>>>>>>>
>>>>>>>> This patch adds OPP tables for OMAP4. New file has been added to keep
>>>>>>>> the OMAP4 opp tables and the registration of these tables with the
>>>>>>>> generic opp framework by OMAP SoC OPP interface.
>>>>>>> [...]
>>>>>>>
>>>>>>>> diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c
>>>>>>>> index 66e12be..48a553f 100644
>>>>>>>> --- a/arch/arm/mach-omap2/opp.c
>>>>>>>> +++ b/arch/arm/mach-omap2/opp.c
>>>>>>>> @@ -131,4 +131,5 @@ static int __init omap_init_opp_table(struct
>>>>>>>> omap_opp_def *opp_def,
>>>>>>>>
>>>>>>>>     /* omap3 opps */
>>>>>>>>     #include "opp3xxx_data.c"
>>>>>>>> -
>>>>>>>> +/* omap4 opps */
>>>>>>>> +#include "opp4xxx_data.c"
>>>>>>> I'm not sure I like the including of C files.  Any reason you prefer
>>>>>>> this to just adding them to the Makefile?  e.g. opp24xx_dta.c are
>>>>>>> compiled in via Makefile and these two are included.
>>>>>> I dont buy it. I am seeing us go around in circles for this:
>>>>>> http://marc.info/?l=linux-omap&m=128986880406272&w=2
>>>>>> a) we dont want others to use specifics implemented in opp.c in other
>>>>>> files (e.g. board files)
>>>>>
>>>>> not sure how this is prevented.
>>>> The approach is as follows:
>>>> the #defines and struct definitions are in opp.c
>>>> opp{3,4}xxx_data.c use the same - by itself wont build because they need
>>>> the defines in opp.c
>>>> that way, there is no possibility of clean hacks possible except to
>>>> follow the current mechanism for a future omap silicon
>>>>
>>>> The main objective was to restrict the potential of board developers
>>>> from hacking the default OPP table - which would be possible by exposing
>>>> the headers for board files - which as Thomas rightly pointed out in the
>>>>     thread I pointed out,  opens up a possibility for board files to
>>>> include them as well and re-instantiate the table again..
>> [...]
>>>
>>> FYI, Paul changed the clock and hwmod data files from include to C
>>> files, because Russell didn't like at all the include of data file.
>>>
>>> Why cannot we handle the opp data the same way we are managing clock,
>>> hwmod or even pad mux data for various Soc?
>>
>> Objective:
>> I do not want folks to start creating their own custom "default" OPP
>> tables for silicon and registering with OPP framework. intent is to have
>> the default OPP tables in one place - here.
>>
>> Solution I followed:
>> make the following:
>> a) struct omap_opp_def
>> b) OPP_INITIALIZER
>> c) omap_init_opp_table
>> not available for anyone other than opp.c - they are defined in opp.c
>> and not available externally.
>>
>> If these were in a header, I am inviting failure of my objective. yes, I
>> could review it when posted to l-o and NAK it, but wont prevent private
>> trees from basically(IMHO) misusing the framework
>>
>> Options available:
>> a) v1,2,3 of the series basically used a header opp{3,4}xxx_data .h
>> in v3, I received a comment (IMHO rightly so) that a header defining a
>> static array implies capability for some other file to include the
>> header and misuse it (basically headers are meant to share data structs
>> between x and y - if you dont intend to do that, then put it in a c file)
>> b) in v4, I converted the data header to a .c and included the data.c in
>> opp.c.
>>
>> NOTE:
>> a) padconf, hwmod, clock are designed to be reused by rest of
>> mach-omap2. opp defines are meant to be isolated. I dont really have a
>> preference c or header file for the data, just that I want to, by design
>> "make any hacks attempts - *really ugly*"
>
> Mmm, I'm not convince that this will prevent anybody to do dirty stuff
> anyway.
> Doing "dirty" stuff to avoid even more dirty stuff is kind of a weird
> approach... for my point of view at least...
>
> And no, this is not that different than the hwmod data or clock
> data... These data are never manipulated directly by anybody but the
> core code.
>
>> b) Inclusion of c in c files are not uniquely introduced here.
>> $ find . -iname "*.c"|xargs grep "#include"| grep -v "\.h"|grep
>> ":#include"|wc -l
>> 558
>
> That does not mean that we have to keep doing it.
> There are probably much more lines of code that does not stick to the
> CodingStyle ;-)
>
>> Hope this clarifies.
> Yes, but that still does not justify that for my point of view.

OK, let's settle this.

Nishanth, as stated by Benoit, this kind of thing has been NAK'd in the
past upstream, so let's learn from that and not repeat it.

We don't need to take extreme measures to prevent folks from doing dirty
hacks, especially when such measures are controversial by standard
coding practices and convention.

Let's please just make this behave like clock & hwmod data and move on.

Kevin

Re: [PATCH v4 2/3] omap4: opp: add OPP table data

[Nishanth Menon]

Kevin Hilman wrote, on 11/23/2010 06:16 PM:
[..]
> Let's please just make this behave like clock&  hwmod data and move on.

I suppose we could do that in the interest of world peace and global 
warming ;) hopefully the v5 will the last set.

-- 
Regards,
Nishanth Menon