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

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

[Nishanth Menon]

Series version 3 incorporating changes for the comments
for using device_initcall to initialize the opp layer

patches based on kernel.org 2.6.37-rc1

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                 |  172 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.h        |   82 ++++++++++++++
 arch/arm/mach-omap2/opp4xxx_data.h        |   37 ++++++
 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, 347 insertions(+), 33 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp.c
 create mode 100644 arch/arm/mach-omap2/opp3xxx_data.h
 create mode 100644 arch/arm/mach-omap2/opp4xxx_data.h

 Regards,
 Nishanth Menon

[PATCH v3 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.

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
v3:
	* 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          |  154 ++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.h |   82 +++++++++++++++++++
 arch/arm/mach-omap2/pm.h           |    9 ++
 6 files changed, 274 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp.c
 create mode 100644 arch/arm/mach-omap2/opp3xxx_data.h

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..79c711c
--- /dev/null
+++ b/arch/arm/mach-omap2/opp.c
@@ -0,0 +1,154 @@
+/*
+ *  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;
+	}
+
+	/*
+	 * Allow multiple calls, but initialize only if not already initalized
+	 * even if the previous call failed, coz, no reason we'd succeed again
+	 */
+	if (omap_table_init)
+		return 0;
+	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.h"
+/**
+ * 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/opp3xxx_data.h b/arch/arm/mach-omap2/opp3xxx_data.h
new file mode 100644
index 0000000..2ce36a2
--- /dev/null
+++ b/arch/arm/mach-omap2/opp3xxx_data.h
@@ -0,0 +1,82 @@
+/*
+ * 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.
+ */
+
+#ifndef __ASM_PLAT_OPP_OMAP3XXX_H__
+#define __ASM_PLAT_OPP_OMAP3XXX_H__
+
+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),
+};
+
+#endif		/* __ASM_PLAT_OPP_OMAP3XXX_H__ */
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

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

[Thomas Petazzoni]

Hello,

On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon <nm@ti.com> wrote:

> +++ b/arch/arm/mach-omap2/opp3xxx_data.h
> +
> +static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
> +
> +static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {

Do we really want to have structure definitions in an header file ?
Unless I'm wrong, this means that if the opp3xxx_data.h file is
included in two different C files, the structures will be present twice.

As far as I could see, most of the kernel instantiate structure in C
files instead.

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Nishanth Menon]

Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
> Hello,
> 
> On Mon, 15 Nov 2010 13:27:39 -0600
> Nishanth Menon <nm@ti.com> wrote:
> 
>> +++ b/arch/arm/mach-omap2/opp3xxx_data.h
>> +
>> +static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
>> +
>> +static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
> 
> Do we really want to have structure definitions in an header file ?
> Unless I'm wrong, this means that if the opp3xxx_data.h file is
> included in two different C files, the structures will be present twice.
The intent here - DONT DO precisely THAT!
> 
> As far as I could see, most of the kernel instantiate structure in C
> files instead.
The intent here though was that opp3xx.h and opp4xx.h are private to 
just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
find drivers/ -iname "*.c"|xargs grep "#include"| grep -v "\.h"
shows numerous examples of .c files being included in c files. I dont 
have an issue of renaming these headers as .c file instead (I had 
carried them over as .h from old implementation, but we can change it), 
main point being, I just dont want folks mucking around and hacking 
stuff with the defines.

-- 
Regards,
Nishanth Menon

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

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [101115 16:43]:
> Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
> >Hello,
> >
> >On Mon, 15 Nov 2010 13:27:39 -0600
> >Nishanth Menon <nm@ti.com> wrote:
> >
> >>+++ b/arch/arm/mach-omap2/opp3xxx_data.h
> >>+
> >>+static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
> >>+
> >>+static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
> >
> >Do we really want to have structure definitions in an header file ?
> >Unless I'm wrong, this means that if the opp3xxx_data.h file is
> >included in two different C files, the structures will be present twice.
> The intent here - DONT DO precisely THAT!
> >
> >As far as I could see, most of the kernel instantiate structure in C
> >files instead.
> The intent here though was that opp3xx.h and opp4xx.h are private to
> just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
> find drivers/ -iname "*.c"|xargs grep "#include"| grep -v "\.h"
> shows numerous examples of .c files being included in c files. I
> dont have an issue of renaming these headers as .c file instead (I
> had carried them over as .h from old implementation, but we can
> change it), main point being, I just dont want folks mucking around
> and hacking stuff with the defines.

What usually works best is to have common opp.c, then have opp34xx.c
that has initcall that registers the data in opp.c.

That leaves out if cpu_is_omapxxx else if stuff in opp.c and then
adding support for new omaps is just a matter of doing oppxxxx.c.

Regards,

Tony

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

[Nishanth Menon]

Tony Lindgren had written, on 11/16/2010 02:35 PM, the following:
> * Nishanth Menon <nm@ti.com> [101115 16:43]:
>> Thomas Petazzoni had written, on 11/15/2010 04:51 PM, the following:
>>> Hello,
>>>
>>> On Mon, 15 Nov 2010 13:27:39 -0600
>>> Nishanth Menon <nm@ti.com> wrote:
>>>
>>>> +++ b/arch/arm/mach-omap2/opp3xxx_data.h
>>>> +
>>>> +static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
>>>> +
>>>> +static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
>>> Do we really want to have structure definitions in an header file ?
>>> Unless I'm wrong, this means that if the opp3xxx_data.h file is
>>> included in two different C files, the structures will be present twice.
>> The intent here - DONT DO precisely THAT!
>>> As far as I could see, most of the kernel instantiate structure in C
>>> files instead.
>> The intent here though was that opp3xx.h and opp4xx.h are private to
>> just opp.c to prevent misuse elsewhere. hmm.. thinking a bit,
>> find drivers/ -iname "*.c"|xargs grep "#include"| grep -v "\.h"
>> shows numerous examples of .c files being included in c files. I
>> dont have an issue of renaming these headers as .c file instead (I
>> had carried them over as .h from old implementation, but we can
>> change it), main point being, I just dont want folks mucking around
>> and hacking stuff with the defines.
> 
> What usually works best is to have common opp.c, then have opp34xx.c
> that has initcall that registers the data in opp.c.
> 
> That leaves out if cpu_is_omapxxx else if stuff in opp.c and then
> adding support for new omaps is just a matter of doing oppxxxx.c.

Series rev4 already posted here:
http://marc.info/?l=linux-omap&m=128993367212640&w=2
I believe I have taken care of the comments there - do let me know if I 
screwed anything up here..
-- 
Regards,
Nishanth Menon

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

[Thomas Petazzoni]

Hello,

On Mon, 15 Nov 2010 13:27:39 -0600
Nishanth Menon <nm@ti.com> wrote:

> +	/*
> +	 * Allow multiple calls, but initialize only if not already initalized

Minor: s/initalized/initialized/.

> +	 * even if the previous call failed, coz, no reason we'd succeed again
> +	 */
> +	if (omap_table_init)
> +		return 0;
> +	omap_table_init = 1;

Do we really need this ? I personaly don't really like this quite of
"Hey, I'm already initialized, let's do nothing silently then". Unless
there are strong reasons for which this function could be called twice,
I'd rather not have this, or turn this into a BUG_ON(omap_table_init ==
1).

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Nishanth Menon]

Thomas Petazzoni wrote, on 11/16/2010 05:21 AM:
> Hello,
>
> On Mon, 15 Nov 2010 13:27:39 -0600
> Nishanth Menon<nm@ti.com>  wrote:
>
>> +	/*
>> +	 * Allow multiple calls, but initialize only if not already initalized
>
> Minor: s/initalized/initialized/.
aah thanks :)

>
>> +	 * even if the previous call failed, coz, no reason we'd succeed again
>> +	 */
>> +	if (omap_table_init)
>> +		return 0;
>> +	omap_table_init = 1;
>
> Do we really need this ? I personaly don't really like this quite of
> "Hey, I'm already initialized, let's do nothing silently then". Unless
> there are strong reasons for which this function could be called twice,
> I'd rather not have this, or turn this into a BUG_ON(omap_table_init ==
> 1).
Yes, it is needed. The intent here is different. See the documentation 
that I put along with this patch - At times, board files may need to do 
customization to opps - like enable 1GHz on that platform alone -> it 
can do it *only if* the defaults are registered, following which it can 
call opp_enable. when device_initcall follows this at a later point, it 
is still valid.

btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok 
fine, system can still survive without cpufreq, no need to stop system 
operations because of that.


-- 
Regards,
Nishanth Menon

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

[Thomas Petazzoni]

Hello,

On Tue, 16 Nov 2010 05:54:50 -0600
Nishanth Menon <nm@ti.com> wrote:

> > Do we really need this ? I personaly don't really like this quite of
> > "Hey, I'm already initialized, let's do nothing silently then". Unless
> > there are strong reasons for which this function could be called twice,
> > I'd rather not have this, or turn this into a BUG_ON(omap_table_init ==
> > 1).
> Yes, it is needed. The intent here is different. See the documentation 
> that I put along with this patch - At times, board files may need to do 
> customization to opps - like enable 1GHz on that platform alone -> it 
> can do it *only if* the defaults are registered, following which it can 
> call opp_enable. when device_initcall follows this at a later point, it 
> is still valid.

Ah, right, I didn't catch that omapX_init_opp() could be called first
from the board init function, and then later on as a device_initcall()
callback.

But, sorry, I find this even uglier than I thought it was :) What about
adding the obligation to boards file to call the omapX_init_opp()
function and then do their customization (if needed), then no call to
omapX_init_opp() would be needed as a device_initcall() callback. Or,
add a mechanism that allows the board file to register its
customizations, that are later taken into account by the
omapX_init_opp() function.

Maybe it's just a matter of personal taste, but I really don't like
this kind of "let's call this function once, do some stuff, then call
it again, since it'll know that it shouldn't do anything".

> btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok 
> fine, system can still survive without cpufreq, no need to stop system 
> operations because of that.

I don't see why replacing:

+	if (omap_table_init)
+		return 0;
+	omap_table_init = 1;

by

	BUG_ON(omap_table_init == 1);
	omap_table_init = 1;

would prevent you from having no OPP table (the case where a NULL OPP
table is passed is tested *before* in omapX_init_opp()).

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Nishanth Menon]

Thomas Petazzoni wrote, on 11/16/2010 06:42 AM:
> Hello,
>
> On Tue, 16 Nov 2010 05:54:50 -0600
> Nishanth Menon<nm@ti.com>  wrote:
>
>>> Do we really need this ? I personaly don't really like this quite of
>>> "Hey, I'm already initialized, let's do nothing silently then". Unless
>>> there are strong reasons for which this function could be called twice,
>>> I'd rather not have this, or turn this into a BUG_ON(omap_table_init ==
>>> 1).
>> Yes, it is needed. The intent here is different. See the documentation
>> that I put along with this patch - At times, board files may need to do
>> customization to opps - like enable 1GHz on that platform alone ->  it
>> can do it *only if* the defaults are registered, following which it can
>> call opp_enable. when device_initcall follows this at a later point, it
>> is still valid.
>
> Ah, right, I didn't catch that omapX_init_opp() could be called first
> from the board init function, and then later on as a device_initcall()
> callback.
>
> But, sorry, I find this even uglier than I thought it was :) What about
> adding the obligation to boards file to call the omapX_init_opp()
> function and then do their customization (if needed), then no call to
> omapX_init_opp() would be needed as a device_initcall() callback. Or,
> add a mechanism that allows the board file to register its
> customizations, that are later taken into account by the
> omapX_init_opp() function.
>
> Maybe it's just a matter of personal taste, but I really don't like
> this kind of "let's call this function once, do some stuff, then call
> it again, since it'll know that it shouldn't do anything".,

I feel you may have misunderstood the code, we DONOT oblige all boards 
to *have* to call omapX_init_opp. It is a device_initcall - so for the 
boards that dont call it, device_initcall will trigger and initialzie 
it. the hooks for the customization of the OPPs is in OPP layer itself.

the need we satisfy is this: if you need to support two sets of boards:
a) boards that are happy with the defaults - most of the boards - dont 
do anything in the board file. (device_init_call with auto register the 
defaults)
b) boards that need customization - these guys need to call 
omapX_init_opp(to register the defaults) before customizing the defaults.

Does this explain the code and reason for the logic? if you do have a 
better mechanism, lets know.

>
>> btw, BUG_ON is a strict NO NO for me here - if I dont have OPP table, ok
>> fine, system can still survive without cpufreq, no need to stop system
>> operations because of that.
>
> I don't see why replacing:
>
> +	if (omap_table_init)
> +		return 0;
> +	omap_table_init = 1;
>
> by
>
> 	BUG_ON(omap_table_init == 1);
> 	omap_table_init = 1;
>
> would prevent you from having no OPP table (the case where a NULL OPP
> table is passed is tested *before* in omapX_init_opp()).
HUH?? NULL table to a static function - what code are you talking 
about?? why are you so behind BUG_ON, when there are valid reasons for 
reentry into code.

-- 
Regards,
Nishanth Menon

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

[Thomas Petazzoni]

Hello,

On Tue, 16 Nov 2010 07:10:36 -0600
Nishanth Menon <nm@ti.com> wrote:

> I feel you may have misunderstood the code, we DONOT oblige all boards 
> to *have* to call omapX_init_opp. It is a device_initcall - so for the 
> boards that dont call it, device_initcall will trigger and initialzie 
> it. the hooks for the customization of the OPPs is in OPP layer itself.

This is exactly what I have understood.

> the need we satisfy is this: if you need to support two sets of boards:
> a) boards that are happy with the defaults - most of the boards - dont 
> do anything in the board file. (device_init_call with auto register the 
> defaults)
> b) boards that need customization - these guys need to call 
> omapX_init_opp(to register the defaults) before customizing the defaults.
> 
> Does this explain the code and reason for the logic? if you do have a 
> better mechanism, lets know.

Yes, it explains the code and reason for the logic, but still doesn't
make it pretty :-)

> > would prevent you from having no OPP table (the case where a NULL OPP
> > table is passed is tested *before* in omapX_init_opp()).
> HUH?? NULL table to a static function - what code are you talking 
> about?? why are you so behind BUG_ON, when there are valid reasons for 
> reentry into code.

In the current design, yes, there are indeed valid reasons for reentry
into the omapX_init_opp() function, and that's exactly the point I'm
critizicing here.

Regards!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Nishanth Menon]

Thomas Petazzoni had written, on 11/16/2010 07:20 AM, the following:
>>> would prevent you from having no OPP table (the case where a NULL OPP
>>> table is passed is tested *before* in omapX_init_opp()).
>> HUH?? NULL table to a static function - what code are you talking 
>> about?? why are you so behind BUG_ON, when there are valid reasons for 
>> reentry into code.
> 
> In the current design, yes, there are indeed valid reasons for reentry
> into the omapX_init_opp() function, and that's exactly the point I'm
> critizicing here.
how about:
	if (omap_table_init)
		return -EEXIST;
does that make it better? it still retains the ability to handle both 
kinds of platforms as well as not BUG out.

-- 
Regards,
Nishanth Menon

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

[Nishanth Menon]

Thomas Petazzoni had written, on 11/16/2010 06:42 AM, the following:

> But, sorry, I find this even uglier than I thought it was :) What about
> adding the obligation to boards file to call the omapX_init_opp()
> function and then do their customization (if needed), then no call to
I knew I had discussed this before! Apologies, I should have dug this 
thread up earlier in the discussion.

my initial implementation had forced board files to call the 
opp_init_table, then changed that here:
http://marc.info/?l=linux-omap&m=127431810922704&w=2
Kevin seemed happy with the change here:
http://marc.info/?l=linux-omap&m=127507237109393&w=2

-- 
Regards,
Nishanth Menon

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

[Thomas Petazzoni]

On Tue, 16 Nov 2010 09:23:06 -0600
Nishanth Menon <nm@ti.com> wrote:

> my initial implementation had forced board files to call the 
> opp_init_table, then changed that here:
> http://marc.info/?l=linux-omap&m=127431810922704&w=2
> Kevin seemed happy with the change here:
> http://marc.info/?l=linux-omap&m=127507237109393&w=2

Ok, if Kevin is happy with this solution, fair enough. Sorry for the
noise, and thanks for your answers.

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Nishanth Menon]

Thomas Petazzoni had written, on 11/16/2010 09:50 AM, the following:
> On Tue, 16 Nov 2010 09:23:06 -0600
> Nishanth Menon <nm@ti.com> wrote:
> 
>> my initial implementation had forced board files to call the 
>> opp_init_table, then changed that here:
>> http://marc.info/?l=linux-omap&m=127431810922704&w=2
>> Kevin seemed happy with the change here:
>> http://marc.info/?l=linux-omap&m=127507237109393&w=2
> 
> Ok, if Kevin is happy with this solution, fair enough. Sorry for the
> noise, and thanks for your answers.
Thanks for taking the time to review and the comments. It is always a 
good idea to evolve to a better solution.

If there are no further comments, I will post a v4 later today 
incorporating comments:
a) return error instead of 0 if opp table is already initialized
b) change the .h to .c

-- 
Regards,
Nishanth Menon

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

[Kevin Hilman]

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

> On Tue, 16 Nov 2010 09:23:06 -0600
> Nishanth Menon <nm@ti.com> wrote:
>
>> my initial implementation had forced board files to call the 
>> opp_init_table, then changed that here:
>> http://marc.info/?l=linux-omap&m=127431810922704&w=2
>> Kevin seemed happy with the change here:
>> http://marc.info/?l=linux-omap&m=127507237109393&w=2
>
> Ok, if Kevin is happy with this solution, fair enough. Sorry for the
> noise, and thanks for your answers.

Yes, I'm not a big fan of the init function called multiple times
either, but I really want to minimize what board files have to do.

Historically, we tend to add all the init functions to every board file,
and this is getting cumbersome to understand and maintain.  What we need
is for common code to take care of sensible defaults for all boards, and
then only boards with non-default behavior have to have custom code.

Kevin

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

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [101116 08:06]:
> Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:
> 
> > On Tue, 16 Nov 2010 09:23:06 -0600
> > Nishanth Menon <nm@ti.com> wrote:
> >
> >> my initial implementation had forced board files to call the 
> >> opp_init_table, then changed that here:
> >> http://marc.info/?l=linux-omap&m=127431810922704&w=2
> >> Kevin seemed happy with the change here:
> >> http://marc.info/?l=linux-omap&m=127507237109393&w=2
> >
> > Ok, if Kevin is happy with this solution, fair enough. Sorry for the
> > noise, and thanks for your answers.
> 
> Yes, I'm not a big fan of the init function called multiple times
> either, but I really want to minimize what board files have to do.
> 
> Historically, we tend to add all the init functions to every board file,
> and this is getting cumbersome to understand and maintain.  What we need
> is for common code to take care of sensible defaults for all boards, and
> then only boards with non-default behavior have to have custom code.

Yeah. The initial comment from Thomas with data in .h file should
be fixed though.

Tony

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

[Thomas Petazzoni]

On Tue, 16 Nov 2010 08:16:07 -0800
Kevin Hilman <khilman@deeprootsystems.com> wrote:

> Yes, I'm not a big fan of the init function called multiple times
> either, but I really want to minimize what board files have to do.
> 
> Historically, we tend to add all the init functions to every board
> file, and this is getting cumbersome to understand and maintain.
> What we need is for common code to take care of sensible defaults for
> all boards, and then only boards with non-default behavior have to
> have custom code.

The other way is to have the board code "register" its customization
into the OPP subsystem, and then when the OPP subsystem is initialized,
it takes those customizations into account. But in that specific case,
it's not clear how those customizations could easily be expressed, so
maybe that multiple call strategy is the simplest solution.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 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
v3:
	* 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          |   18 +++++++++++++++++
 arch/arm/mach-omap2/opp4xxx_data.h |   37 ++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h           |    5 ++++
 4 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/opp4xxx_data.h

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 79c711c..d1012bd 100644
--- a/arch/arm/mach-omap2/opp.c
+++ b/arch/arm/mach-omap2/opp.c
@@ -152,3 +152,21 @@ int __init omap3_opp_init(void)
 }
 device_initcall(omap3_opp_init);
 
+/* omap4 opps */
+#include "opp4xxx_data.h"
+/**
+ * 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/opp4xxx_data.h b/arch/arm/mach-omap2/opp4xxx_data.h
new file mode 100644
index 0000000..2e5fe67
--- /dev/null
+++ b/arch/arm/mach-omap2/opp4xxx_data.h
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ */
+
+#ifndef __ASM_PLAT_OPP_OMAP4XXX_H__
+#define __ASM_PLAT_OPP_OMAP4XXX_H__
+
+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 */
+};
+
+#endif		/*  __ASM_PLAT_OPP_OMAP4XXX_H__ */
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>
---
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