[PM-WIP-OPP][PATCH 0/4] few opp layer cleanups

[PM-WIP-OPP][PATCH 0/4] few opp layer cleanups

[Nishanth Menon]

Opp layer cleanup series - tries the following:
a) remove the BUG_ON being used on functional statements
b) improve the voltage conversion routine a bit
c) introduce a per-opp data storage mechanism
d) remove the hardcoded vdd1-vdd2 thruput dependency using the generic
   opp data storage mechanism

Basic sanity tested on SDP3630

Nishanth Menon (3):
  omap3: pm: cpufreq: BUG_ON cleanup
  omap: pm: opp: add ability to store data per opp
  omap3: srf: remove hardcoded opp dependency

Phil Carmody (1):
  omap: pm: opp: twl: use DIV_ROUND_UP

 arch/arm/mach-omap2/cpufreq34xx.c     |   21 +++++++++-
 arch/arm/mach-omap2/resource34xx.c    |   17 ++++----
 arch/arm/plat-omap/include/plat/opp.h |   56 ++++++++++++++++++++++++++++
 arch/arm/plat-omap/opp.c              |   66 +++++++++++++++++++++++++++++++++
 arch/arm/plat-omap/opp_twl_tps.c      |    2 +-
 5 files changed, 150 insertions(+), 12 deletions(-)

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Regards,
Nishanth Menon

[PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

BUG_ON should not ideally contain a functional code. Remove it out.

Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/cpufreq34xx.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
index c453ec5..f0ed3ae 100644
--- a/arch/arm/mach-omap2/cpufreq34xx.c
+++ b/arch/arm/mach-omap2/cpufreq34xx.c
@@ -111,6 +111,7 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
 
 void __init omap3_pm_init_opp_table(void)
 {
+	int r;
 	struct omap_opp_def **omap3_opp_def_list;
 	struct omap_opp_def *omap34xx_opp_def_list[] = {
 		omap34xx_mpu_rate_table,
@@ -126,8 +127,9 @@ void __init omap3_pm_init_opp_table(void)
 	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
 				omap34xx_opp_def_list;
 
-	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
-	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
-	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
+	r = opp_init_list(OPP_MPU, omap3_opp_def_list[0]);
+	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
+	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
+	BUG_ON(r);
 }
 
-- 
1.6.3.3

[PM-WIP-OPP][PATCH 2/4] omap: pm: opp: twl: use DIV_ROUND_UP

[Nishanth Menon]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

kernel.h provides a neat function for DIV_ROUND_UP which should
be used instead of replicating it in code

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 arch/arm/plat-omap/opp_twl_tps.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/opp_twl_tps.c b/arch/arm/plat-omap/opp_twl_tps.c
index 468fb97..112f106 100644
--- a/arch/arm/plat-omap/opp_twl_tps.c
+++ b/arch/arm/plat-omap/opp_twl_tps.c
@@ -37,5 +37,5 @@ unsigned long omap_twl_vsel_to_uv(const u8 vsel)
 u8 omap_twl_uv_to_vsel(unsigned long uv)
 {
 	/* Round up to higher voltage */
-	return (((uv + 99) / 100 - 6000) + 124) / 125;
+	return DIV_ROUND_UP(uv - 600000, 12500);
 }
-- 
1.6.3.3

[PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Many modules seem to need some sort of flexible storage mechanism
which is corresponds to a specific opp. To cater to this need, we
provide store, restore and removal apis.

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/plat-omap/include/plat/opp.h |   56 ++++++++++++++++++++++++++++
 arch/arm/plat-omap/opp.c              |   66 +++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
index dc9a0d9..666c514 100644
--- a/arch/arm/plat-omap/include/plat/opp.h
+++ b/arch/arm/plat-omap/include/plat/opp.h
@@ -231,6 +231,47 @@ struct omap_opp * __deprecated opp_find_by_opp_id(enum opp_t opp_type,
 						  u8 opp_id);
 u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
 
+/**
+ * opp_store_data() - Store a data corresponding to an opp
+ * @opp: opp where to store
+ * @name: unique string to identify the type of data stored
+ * @data: The pointer which is used to the actual data
+ *
+ * Many scenarios require a custom data to be stored corresponding to a
+ * specific OPP which may need to be retrieved for operations. The actual
+ * type of data might be very specific to a CPU, allowing opp layer to store
+ * any type or mixture of types of data to be adequately retrieved
+ * by corresponding modules which consume that data.
+ * typical examples are Smart reflex nTarget values, L3 threshold dependencies
+ *
+ * Returns 0 if successful or a corresponding error value if failed.
+ */
+int opp_store_data(struct omap_opp *opp, char *name, void *data);
+
+/**
+ * opp_get_data() - get a stored data corresponding to an opp
+ * @opp: pointer to opp
+ * @name: unique string to identify the type of data stored
+ *
+ * Retrieve a stored data identified by the name allowing usage
+ * accross modules on a need basis
+ *
+ * Returns ERR_PTRs and should be checked with IS_ERR() macros
+ */
+void *opp_get_data(struct omap_opp *opp, char *name);
+
+/**
+ * opp_remove_data() - remove the stored data corresponding to an opp
+ * @opp: pointer to opp
+ * @name: unique string to identify the type of data stored
+ *
+ * Remove a stored data identified by the name allowing replacing
+ * old values with new or removing the information altogether if needed
+ *
+ * Returns 0 if successfully removed, else returns corresponding error value
+ */
+int opp_remove_data(struct omap_opp *opp, char *name);
+
 void opp_init_cpufreq_table(enum opp_t opp_type,
 			    struct cpufreq_frequency_table **table);
 #else
@@ -300,6 +341,21 @@ static inline u8 __deprecated opp_get_opp_id(struct omap_opp *opp)
 	return 0;
 }
 
+int opp_store_data(struct omap_opp *opp, char *name, void *data)
+{
+	return -EINVAL;
+}
+
+void *opp_get_data(struct omap_opp *opp, char *name)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+int opp_remove_data(struct omap_opp *opp, char *name)
+{
+	return -EINVAL;
+}
+
 static inline void opp_init_cpufreq_table(struct omap_opp *opps,
 			    struct cpufreq_frequency_table **table)
 {
diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index bb8120e..15f6f7c 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -14,12 +14,19 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/cpufreq.h>
 
 #include <plat/opp_twl_tps.h>
 #include <plat/opp.h>
 
+struct omap_opp_data {
+	char *name;
+	void *data;
+	struct list_head list;
+};
+
 /**
  * struct omap_opp - OMAP OPP description structure
  * @enabled:	true/false - marking this OPP as enabled/disabled
@@ -37,6 +44,7 @@ struct omap_opp {
 	unsigned long rate;
 	unsigned long u_volt;
 	u8 opp_id;
+	struct list_head data_list;
 };
 
 /*
@@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
 	opp->rate = opp_def->freq;
 	opp->enabled = opp_def->enabled;
 	opp->u_volt = opp_def->u_volt;
+	INIT_LIST_HEAD(&opp->data_list);
 }
 
 int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
@@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
 	return 0;
 }
 
+void *opp_get_data(struct omap_opp *opp, char *name)
+{
+	void *data = ERR_PTR(-EINVAL);
+	struct omap_opp_data *tmp;
+
+	if (unlikely(!opp || !name))
+		return ERR_PTR(-EINVAL);
+
+	list_for_each_entry(tmp, &opp->data_list, list)
+		if (!strcmp(name, tmp->name)) {
+			data = tmp->data;
+			break;
+		}
+	return data;
+}
+
+int opp_store_data(struct omap_opp *opp, char *name, void *data)
+{
+	struct omap_opp_data *new;
+	if (unlikely(!opp || !name))
+		return -EINVAL;
+	/* NAK to double registration */
+	if (unlikely(!IS_ERR(opp_get_data(opp, name))))
+		return -EINVAL;
+
+	new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+	new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!new->name) {
+		kfree(new);
+		return -ENOMEM;
+	}
+	new->data = data;
+	strcpy(new->name, name);
+	INIT_LIST_HEAD(&new->list);
+	list_add(&new->list, &opp->data_list);
+	return 0;
+}
+
+int opp_remove_data(struct omap_opp *opp, char *name)
+{
+	struct omap_opp_data *tmp;
+
+	if (unlikely(!opp || !name))
+		return -EINVAL;
+
+	list_for_each_entry(tmp, &opp->data_list, list)
+		if (!strcmp(name, tmp->name)) {
+			list_del(&tmp->list);
+			kfree(tmp->name);
+			kfree(tmp);
+			return 0;
+		}
+	return -EINVAL;
+}
+
 /* XXX document */
 void opp_init_cpufreq_table(enum opp_t opp_type,
 			    struct cpufreq_frequency_table **table)
-- 
1.6.3.3

[PM-WIP-OPP][PATCH 4/4] omap3: srf: remove hardcoded opp dependency

[Nishanth Menon]

referencing l3 threshold based on OPP ID is hardcoded for OMAP3430
This is wrong when we consider OMAP3630 and beyond. use the store
and restore of data to handle this information

Cc: Ambresh K <ambresh@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Tero Kristo <tero.kristo@nokia.com>
Cc: Thara Gopinath <thara@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/cpufreq34xx.c  |   13 +++++++++++++
 arch/arm/mach-omap2/resource34xx.c |   17 +++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
index f0ed3ae..b0a03fe 100644
--- a/arch/arm/mach-omap2/cpufreq34xx.c
+++ b/arch/arm/mach-omap2/cpufreq34xx.c
@@ -112,6 +112,8 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
 void __init omap3_pm_init_opp_table(void)
 {
 	int r;
+	unsigned long freq, l3_thresh;
+	struct omap_opp *opp;
 	struct omap_opp_def **omap3_opp_def_list;
 	struct omap_opp_def *omap34xx_opp_def_list[] = {
 		omap34xx_mpu_rate_table,
@@ -131,5 +133,16 @@ void __init omap3_pm_init_opp_table(void)
 	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
 	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
 	BUG_ON(r);
+
+	/* First get the l3 thresh from highest l3 opp */
+	freq = ULONG_MAX;
+	opp = opp_find_freq_floor(OPP_L3, &freq);
+	l3_thresh = freq * 4 / 1000;
+	/* Now setup the L3 bandwidth restrictions for right mpu freqs */
+	freq = cpu_is_omap3630() ? 500000000 : 600000000;
+	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq))) {
+		opp_store_data(opp, "l3thresh", (void *) l3_thresh);
+		freq++;
+	}
 }
 
diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index c6cce8b..b17ce90 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -448,17 +448,18 @@ int set_opp(struct shared_resource *resp, u32 target_level)
 	int ret = -EINVAL;
 
 	if (resp == vdd1_resp) {
-		if (target_level < 3)
+		struct omap_opp *opp;
+		void *data;
+		opp = opp_find_by_opp_id(OPP_MPU, target_level);
+		data = opp_get_data(opp, "l3thresh");
+		if (IS_ERR(data))
 			resource_release("vdd2_opp", &vdd2_dev);
 
 		ret = resource_set_opp_level(VDD1_OPP, target_level, 0);
-		/*
-		 * For VDD1 OPP3 and above, make sure the interconnect
-		 * is at 100Mhz or above.
-		 * throughput in KiB/s for 100 Mhz = 100 * 1000 * 4.
-		 */
-		if (target_level >= 3)
-			resource_request("vdd2_opp", &vdd2_dev, 400000);
+
+		if (!IS_ERR(data))
+			resource_request("vdd2_opp", &vdd2_dev,
+					(unsigned long) data);
 
 	} else if (resp == vdd2_resp) {
 		unsigned long req_l3_freq;
-- 
1.6.3.3

Re: [PM-WIP-OPP][PATCH 4/4] omap3: srf: remove hardcoded opp dependency

[Felipe Balbi]

On Thu, Mar 18, 2010 at 07:44:51PM +0100, ext Nishanth Menon wrote:
>@@ -131,5 +133,16 @@ void __init omap3_pm_init_opp_table(void)
> 	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
> 	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
> 	BUG_ON(r);
>+
>+	/* First get the l3 thresh from highest l3 opp */
>+	freq = ULONG_MAX;
>+	opp = opp_find_freq_floor(OPP_L3, &freq);
>+	l3_thresh = freq * 4 / 1000;
>+	/* Now setup the L3 bandwidth restrictions for right mpu freqs */
>+	freq = cpu_is_omap3630() ? 500000000 : 600000000;

I also don't like this. Don't you have somewhere else you could pick ?

>+	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq))) {
>+		opp_store_data(opp, "l3thresh", (void *) l3_thresh);
>+		freq++;
>+	}

this is a good example of what I mean. Instead of saving l3_thresh, why 
don't you group all those data in a structure (which could even be 
defined per-cpu really, you already have cpufreq34xx, so you could have 
cpufreq24xx 36xx 44xx, etc) and just store that in a void * ??

-- 
balbi

Re: [PM-WIP-OPP][PATCH 4/4] omap3: srf: remove hardcoded opp dependency

[Nishanth Menon]

Felipe Balbi had written, on 03/19/2010 09:47 AM, the following:
> On Thu, Mar 18, 2010 at 07:44:51PM +0100, ext Nishanth Menon wrote:
>> @@ -131,5 +133,16 @@ void __init omap3_pm_init_opp_table(void)
>> 	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
>> 	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
>> 	BUG_ON(r);
>> +
>> +	/* First get the l3 thresh from highest l3 opp */
>> +	freq = ULONG_MAX;
>> +	opp = opp_find_freq_floor(OPP_L3, &freq);
>> +	l3_thresh = freq * 4 / 1000;
>> +	/* Now setup the L3 bandwidth restrictions for right mpu freqs */
>> +	freq = cpu_is_omap3630() ? 500000000 : 600000000;
> 
> I also don't like this. Don't you have somewhere else you could pick ?

yeah..:) I was hoping someone would comment on it. for multiple reasons 
why i dont like this, but did put it in as a seperate api call - but I 
did not like adding an API call when there is no reason to do that.

a) Essentially the opp bandwidth limit is specific to each cpu - but the 
bandwidth itself is more of a resourceframework implementation -  this 
is the cause of my dislike.
b) if someone were to do an opp_add to l3, the logic I added is crappy! 
ok, I think the code that adds l3 should also ensure to re-store new 
bandwidth data.. (but anyways opp layer's next limitation comes here -> 
lack of a callback mechanism to trigger re-computation etc..).. but that 
is another topic..

I am not entirely sure where to put this registration if not here.. in a 
module independent manner.

> 
>> +	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq))) {
>> +		opp_store_data(opp, "l3thresh", (void *) l3_thresh);
>> +		freq++;
>> +	}
> 
> this is a good example of what I mean. Instead of saving l3_thresh, why 
> don't you group all those data in a structure (which could even be 
> defined per-cpu really, you already have cpufreq34xx, so you could have 
> cpufreq24xx 36xx 44xx, etc) and just store that in a void * ??
> 
exactly the reason as pointed out in my reply to your email - it is not 
flexible enough to do it as SR and similar modules would be common 
drivers for all silicon - so u'd restrict their implementation by having 
a common structure :).


-- 
Regards,
Nishanth Menon

RE: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Cousson, Benoit]

Hi Nishanth,

>From: Menon, Nishanth
>Sent: Thursday, March 18, 2010 7:45 PM
>
>Many modules seem to need some sort of flexible storage mechanism
>which is corresponds to a specific opp. To cater to this need, we
>provide store, restore and removal apis.

Do we really need that level of flexibility for the moment?
The type of information that belong to an OPP are kind of static for a dedicated SoC (i.e. SR gain, Ntarget, ABB).
Cannot a simple pointer to a static struct do the job?

That's just my two cents.

Regards,
Benoit


>Cc: Ambresh K <ambresh@ti.com>
>Cc: Benoit Cousson <b-cousson@ti.com>
>Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
>Cc: Sanjeev Premi <premi@ti.com>
>Cc: Tero Kristo <tero.kristo@nokia.com>
>Cc: Thara Gopinath <thara@ti.com>
>
>Signed-off-by: Nishanth Menon <nm@ti.com>
>---
> arch/arm/plat-omap/include/plat/opp.h |   56 ++++++++++++++++++++++++++++
> arch/arm/plat-omap/opp.c              |   66
>+++++++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+), 0 deletions(-)
>
>diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-
>omap/include/plat/opp.h
>index dc9a0d9..666c514 100644
>--- a/arch/arm/plat-omap/include/plat/opp.h
>+++ b/arch/arm/plat-omap/include/plat/opp.h
>@@ -231,6 +231,47 @@ struct omap_opp * __deprecated opp_find_by_opp_id(enum
>opp_t opp_type,
>                                                 u8 opp_id);
> u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
>
>+/**
>+ * opp_store_data() - Store a data corresponding to an opp
>+ * @opp: opp where to store
>+ * @name: unique string to identify the type of data stored
>+ * @data: The pointer which is used to the actual data
>+ *
>+ * Many scenarios require a custom data to be stored corresponding to a
>+ * specific OPP which may need to be retrieved for operations. The actual
>+ * type of data might be very specific to a CPU, allowing opp layer to
>store
>+ * any type or mixture of types of data to be adequately retrieved
>+ * by corresponding modules which consume that data.
>+ * typical examples are Smart reflex nTarget values, L3 threshold
>dependencies
>+ *
>+ * Returns 0 if successful or a corresponding error value if failed.
>+ */
>+int opp_store_data(struct omap_opp *opp, char *name, void *data);
>+
>+/**
>+ * opp_get_data() - get a stored data corresponding to an opp
>+ * @opp: pointer to opp
>+ * @name: unique string to identify the type of data stored
>+ *
>+ * Retrieve a stored data identified by the name allowing usage
>+ * accross modules on a need basis
>+ *
>+ * Returns ERR_PTRs and should be checked with IS_ERR() macros
>+ */
>+void *opp_get_data(struct omap_opp *opp, char *name);
>+
>+/**
>+ * opp_remove_data() - remove the stored data corresponding to an opp
>+ * @opp: pointer to opp
>+ * @name: unique string to identify the type of data stored
>+ *
>+ * Remove a stored data identified by the name allowing replacing
>+ * old values with new or removing the information altogether if needed
>+ *
>+ * Returns 0 if successfully removed, else returns corresponding error
>value
>+ */
>+int opp_remove_data(struct omap_opp *opp, char *name);
>+
> void opp_init_cpufreq_table(enum opp_t opp_type,
>                           struct cpufreq_frequency_table **table);
> #else
>@@ -300,6 +341,21 @@ static inline u8 __deprecated opp_get_opp_id(struct
>omap_opp *opp)
>       return 0;
> }
>
>+int opp_store_data(struct omap_opp *opp, char *name, void *data)
>+{
>+      return -EINVAL;
>+}
>+
>+void *opp_get_data(struct omap_opp *opp, char *name)
>+{
>+      return ERR_PTR(-EINVAL);
>+}
>+
>+int opp_remove_data(struct omap_opp *opp, char *name)
>+{
>+      return -EINVAL;
>+}
>+
> static inline void opp_init_cpufreq_table(struct omap_opp *opps,
>                           struct cpufreq_frequency_table **table)
> {
>diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>index bb8120e..15f6f7c 100644
>--- a/arch/arm/plat-omap/opp.c
>+++ b/arch/arm/plat-omap/opp.c
>@@ -14,12 +14,19 @@
> #include <linux/errno.h>
> #include <linux/err.h>
> #include <linux/init.h>
>+#include <linux/list.h>
> #include <linux/slab.h>
> #include <linux/cpufreq.h>
>
> #include <plat/opp_twl_tps.h>
> #include <plat/opp.h>
>
>+struct omap_opp_data {
>+      char *name;
>+      void *data;
>+      struct list_head list;
>+};
>+
> /**
>  * struct omap_opp - OMAP OPP description structure
>  * @enabled:  true/false - marking this OPP as enabled/disabled
>@@ -37,6 +44,7 @@ struct omap_opp {
>       unsigned long rate;
>       unsigned long u_volt;
>       u8 opp_id;
>+      struct list_head data_list;
> };
>
> /*
>@@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
>       opp->rate = opp_def->freq;
>       opp->enabled = opp_def->enabled;
>       opp->u_volt = opp_def->u_volt;
>+      INIT_LIST_HEAD(&opp->data_list);
> }
>
> int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
>@@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
>       return 0;
> }
>
>+void *opp_get_data(struct omap_opp *opp, char *name)
>+{
>+      void *data = ERR_PTR(-EINVAL);
>+      struct omap_opp_data *tmp;
>+
>+      if (unlikely(!opp || !name))
>+              return ERR_PTR(-EINVAL);
>+
>+      list_for_each_entry(tmp, &opp->data_list, list)
>+              if (!strcmp(name, tmp->name)) {
>+                      data = tmp->data;
>+                      break;
>+              }
>+      return data;
>+}
>+
>+int opp_store_data(struct omap_opp *opp, char *name, void *data)
>+{
>+      struct omap_opp_data *new;
>+      if (unlikely(!opp || !name))
>+              return -EINVAL;
>+      /* NAK to double registration */
>+      if (unlikely(!IS_ERR(opp_get_data(opp, name))))
>+              return -EINVAL;
>+
>+      new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
>+      if (!new)
>+              return -ENOMEM;
>+      new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
>+      if (!new->name) {
>+              kfree(new);
>+              return -ENOMEM;
>+      }
>+      new->data = data;
>+      strcpy(new->name, name);
>+      INIT_LIST_HEAD(&new->list);
>+      list_add(&new->list, &opp->data_list);
>+      return 0;
>+}
>+
>+int opp_remove_data(struct omap_opp *opp, char *name)
>+{
>+      struct omap_opp_data *tmp;
>+
>+      if (unlikely(!opp || !name))
>+              return -EINVAL;
>+
>+      list_for_each_entry(tmp, &opp->data_list, list)
>+              if (!strcmp(name, tmp->name)) {
>+                      list_del(&tmp->list);
>+                      kfree(tmp->name);
>+                      kfree(tmp);
>+                      return 0;
>+              }
>+      return -EINVAL;
>+}
>+
> /* XXX document */
> void opp_init_cpufreq_table(enum opp_t opp_type,
>                           struct cpufreq_frequency_table **table)
>--
>1.6.3.3


Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Cousson, Benoit had written, on 03/19/2010 05:14 AM, the following:
> Hi Nishanth,
> 
>> From: Menon, Nishanth
>> Sent: Thursday, March 18, 2010 7:45 PM
>>
>> Many modules seem to need some sort of flexible storage mechanism
>> which is corresponds to a specific opp. To cater to this need, we
>> provide store, restore and removal apis.
> 
> Do we really need that level of flexibility for the moment?
> The type of information that belong to an OPP are kind of static for a dedicated SoC (i.e. SR gain, Ntarget, ABB).
> Cannot a simple pointer to a static struct do the job?
> 
> That's just my two cents.
The follow on patch shows why that flexibility is needed. this is 
important IMHO from a multiple-OMAP boot perspective -> the data is 
static for *each* dedicated SOC, but the nature of the data and the type 
of information stored will vary between SOCs themselves. as an example - 
There is no need to store NTarget on OMAP2/OMAP1 platform, while OMAP4 
or beyond may have additional data to store.

This allows an unified store and retrival capability for OPP specific 
data without knowing or constraining the exact type of data stored.

consider(hypothetically) how u'd implement dependency using resource 
framework across OMAPs in a uniform way -> we'd need custom apis 
get_vdd1_threshold and get_vdd2_threshold etc.. which are pretty much 
custom api implementation

in the case of 3630 vs 3430 the nTargets are different. for 3430 ABB 
data is probably irrelevant - for that matter nTarget may or maynot be 
programmed for a specific OPP (hypothetically again)..

But in short, there is some custom SOC data tied down to a specific OPP 
- this data may or maynot be present based on SOC and OPPs - today's OPP 
layer lacks this flexibility and with it, we can better optimize the 
logic we write. hence this introduction to it.

[...]
-- 
Regards,
Nishanth Menon

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Felipe Balbi]

hi,

On Thu, Mar 18, 2010 at 07:44:50PM +0100, ext Nishanth Menon wrote:
>+int opp_store_data(struct omap_opp *opp, char *name, void *data)
>+{
>+	return -EINVAL;

Would -ENODATA fit better ??

>diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>index bb8120e..15f6f7c 100644
>--- a/arch/arm/plat-omap/opp.c
>+++ b/arch/arm/plat-omap/opp.c
>@@ -14,12 +14,19 @@
> #include <linux/errno.h>
> #include <linux/err.h>
> #include <linux/init.h>
>+#include <linux/list.h>
> #include <linux/slab.h>
> #include <linux/cpufreq.h>
>
> #include <plat/opp_twl_tps.h>
> #include <plat/opp.h>
>
>+struct omap_opp_data {
>+	char *name;
>+	void *data;
>+	struct list_head list;
>+};
>+
> /**
>  * struct omap_opp - OMAP OPP description structure
>  * @enabled:	true/false - marking this OPP as enabled/disabled
>@@ -37,6 +44,7 @@ struct omap_opp {
> 	unsigned long rate;
> 	unsigned long u_volt;
> 	u8 opp_id;
>+	struct list_head data_list;
> };
>
> /*
>@@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
> 	opp->rate = opp_def->freq;
> 	opp->enabled = opp_def->enabled;
> 	opp->u_volt = opp_def->u_volt;
>+	INIT_LIST_HEAD(&opp->data_list);
> }
>
> int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
>@@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
> 	return 0;
> }
>
>+void *opp_get_data(struct omap_opp *opp, char *name)
>+{
>+	void *data = ERR_PTR(-EINVAL);
>+	struct omap_opp_data *tmp;
>+
>+	if (unlikely(!opp || !name))
>+		return ERR_PTR(-EINVAL);
>+
>+	list_for_each_entry(tmp, &opp->data_list, list)
>+		if (!strcmp(name, tmp->name)) {
>+			data = tmp->data;
>+			break;
>+		}
>+	return data;
>+}
>+
>+int opp_store_data(struct omap_opp *opp, char *name, void *data)
>+{
>+	struct omap_opp_data *new;
>+	if (unlikely(!opp || !name))
>+		return -EINVAL;
>+	/* NAK to double registration */
>+	if (unlikely(!IS_ERR(opp_get_data(opp, name))))
>+		return -EINVAL;
>+
>+	new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
>+	if (!new)
>+		return -ENOMEM;
>+	new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
>+	if (!new->name) {
>+		kfree(new);
>+		return -ENOMEM;
>+	}
>+	new->data = data;
>+	strcpy(new->name, name);
>+	INIT_LIST_HEAD(&new->list);
>+	list_add(&new->list, &opp->data_list);
>+	return 0;
>+}

will you really want to add several data entries there ? I don't see the 
point. Maybe I'm missing something.

I like the idea of having a void * to allow you to store anything there. 
But generally that's a 1 - 1 relation:

1 device/bus/(in this case) opp - 1 data.

by doing that, the API will look like 
dev_set_drvdata()/dev_get_drvdata() and could be implemented simply as:

void opp_set_data(struct omap_opp *opp, void *data)
{
	opp->data = data;

	return 0;
}

void *opp_get_data(struct omap_opp *opp)
{
	return opp->data;
}

-- 
balbi

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Felipe Balbi had written, on 03/19/2010 09:43 AM, the following:
> hi,
> 
> On Thu, Mar 18, 2010 at 07:44:50PM +0100, ext Nishanth Menon wrote:
>> +int opp_store_data(struct omap_opp *opp, char *name, void *data)
>> +{
>> +	return -EINVAL;
> 
> Would -ENODATA fit better ??
wondered later on if 0 is better here and get_data with -ENODATA - for 
the actual code also..

> 
>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>> index bb8120e..15f6f7c 100644
>> --- a/arch/arm/plat-omap/opp.c
>> +++ b/arch/arm/plat-omap/opp.c
>> @@ -14,12 +14,19 @@
>> #include <linux/errno.h>
>> #include <linux/err.h>
>> #include <linux/init.h>
>> +#include <linux/list.h>
>> #include <linux/slab.h>
>> #include <linux/cpufreq.h>
>>
>> #include <plat/opp_twl_tps.h>
>> #include <plat/opp.h>
>>
>> +struct omap_opp_data {
>> +	char *name;
>> +	void *data;
>> +	struct list_head list;
>> +};
>> +
>> /**
>>  * struct omap_opp - OMAP OPP description structure
>>  * @enabled:	true/false - marking this OPP as enabled/disabled
>> @@ -37,6 +44,7 @@ struct omap_opp {
>> 	unsigned long rate;
>> 	unsigned long u_volt;
>> 	u8 opp_id;
>> +	struct list_head data_list;
>> };
>>
>> /*
>> @@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
>> 	opp->rate = opp_def->freq;
>> 	opp->enabled = opp_def->enabled;
>> 	opp->u_volt = opp_def->u_volt;
>> +	INIT_LIST_HEAD(&opp->data_list);
>> }
>>
>> int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
>> @@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
>> 	return 0;
>> }
>>
>> +void *opp_get_data(struct omap_opp *opp, char *name)
>> +{
>> +	void *data = ERR_PTR(-EINVAL);
>> +	struct omap_opp_data *tmp;
>> +
>> +	if (unlikely(!opp || !name))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	list_for_each_entry(tmp, &opp->data_list, list)
>> +		if (!strcmp(name, tmp->name)) {
>> +			data = tmp->data;
>> +			break;
>> +		}
>> +	return data;
>> +}
>> +
>> +int opp_store_data(struct omap_opp *opp, char *name, void *data)
>> +{
>> +	struct omap_opp_data *new;
>> +	if (unlikely(!opp || !name))
>> +		return -EINVAL;
>> +	/* NAK to double registration */
>> +	if (unlikely(!IS_ERR(opp_get_data(opp, name))))
>> +		return -EINVAL;
>> +
>> +	new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
>> +	if (!new)
>> +		return -ENOMEM;
>> +	new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
>> +	if (!new->name) {
>> +		kfree(new);
>> +		return -ENOMEM;
>> +	}
>> +	new->data = data;
>> +	strcpy(new->name, name);
>> +	INIT_LIST_HEAD(&new->list);
>> +	list_add(&new->list, &opp->data_list);
>> +	return 0;
>> +}
> 
> will you really want to add several data entries there ? I don't see the 
> point. Maybe I'm missing something.
> 
> I like the idea of having a void * to allow you to store anything there. 
> But generally that's a 1 - 1 relation:
> 
> 1 device/bus/(in this case) opp - 1 data.

I think I am lost here -> why device/bus/opp? this is a 1-many 
relationship. here is an example of it:
OPP by definition is a tuple (voltage,frequency).

for each OPP, we may have additional information which is custom to a 
specific SOC ->
OMAP2 and OMAP1 have no SR module -> so I dont need to store SR nTarget 
value.

data type 1: OMAP34xx and OMAP3630 have SR, and nTarget is a *per chip* 
calibration value specific to an OPP -however, we have 5 OPPs (or upto 7 
in the case of 3440 etc.) in omap34xx while omap3630 has upto 4 OPPs.
in omap3630, we use ABB data which is in addition to nTarget (if my 
meager understanding of this is right)..

data type 2: The dependency of vdd1 OPP to vdd2 opp is variant based on SOC.

the types of data which is stored per OPP is changing all the time and 
in future we will have varied data again.

Now, based on your proposal as I understand,
struct omap2_data {
blah_o21
blah_o22
};

struct omap3_data {
blah_o31
blah_o32
blah_o33
}

struct omap4_data {
blah_o41
blah_o42
blah_o31
blah_o32
}

and so on (remember we may have shared or similar data between various 
SOCs at times)

redundancy, maintenance are the problems i see other than ability to 
have a module which uses blah_o31 to be generic and not know the 
difference between struct omap3_data and omap4_data
the resultant module code will look like:
if (cpu_is_omap3430()) {
	my_blaho31 = ((struct omap3_data *) get_opp_data(opp))->blah_o31;
} else if cpu_is...() {
..
}

now in the approach I took,
you could have:
struct sr_ntarget_type{
	unsigned long nTarget;
	something else if needed
}

And in SR driver, the module doesnot need to care which silicon it is 
running on.. it just does opp_get_data(opp,"sr_ntarget") and gets the 
correct data for that silicon on that OPP. It is much simpler and 
similar to the manner implemented in many other frameworks such as clock 
etc..

[...]

-- 
Regards,
Nishanth Menon

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Felipe Balbi]

Hi,

On Fri, Mar 19, 2010 at 10:25:18AM -0500, Nishanth Menon wrote:
> Now, based on your proposal as I understand,
> struct omap2_data {
> blah_o21
> blah_o22
> };
> 
> struct omap3_data {
> blah_o31
> blah_o32
> blah_o33
> }
> 
> struct omap4_data {
> blah_o41
> blah_o42
> blah_o31
> blah_o32
> }
> 
> and so on (remember we may have shared or similar data between various 
> SOCs at times)
> 
> redundancy, maintenance are the problems i see other than ability to 
> have a module which uses blah_o31 to be generic and not know the 
> difference between struct omap3_data and omap4_data
> the resultant module code will look like:
> if (cpu_is_omap3430()) {
> 	my_blaho31 = ((struct omap3_data *) get_opp_data(opp))->blah_o31;
> } else if cpu_is...() {
> ..
> }

ok, I get your point.

> now in the approach I took,
> you could have:
> struct sr_ntarget_type{
> 	unsigned long nTarget;
> 	something else if needed
> }
> 
> And in SR driver, the module doesnot need to care which silicon it is 
> running on.. it just does opp_get_data(opp,"sr_ntarget") and gets the 
> correct data for that silicon on that OPP. It is much simpler and 
> similar to the manner implemented in many other frameworks such as clock 
> etc..

the thing is that ideally it would simply:

opp_get_data(opp);

and that should be done so that it fits all possibilities, but if it's
that different per-SoC, then I guess there's nothing to do.

-- 
balbi

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Felipe Balbi had written, on 03/19/2010 12:47 PM, the following:
[...]

>> now in the approach I took,
>> you could have:
>> struct sr_ntarget_type{
>> 	unsigned long nTarget;
>> 	something else if needed
>> }
>>
>> And in SR driver, the module doesnot need to care which silicon it is 
>> running on.. it just does opp_get_data(opp,"sr_ntarget") and gets the 
>> correct data for that silicon on that OPP. It is much simpler and 
>> similar to the manner implemented in many other frameworks such as clock 
>> etc..
> 
> the thing is that ideally it would simply:
> 
> opp_get_data(opp);
> 
> and that should be done so that it fits all possibilities, but if it's
> that different per-SoC, then I guess there's nothing to do.
> 
but consider this:
data type1,2,3.. may have been stored w.r.t an opp.. now the module 
who'd query it wants it's own data and does not care about anything 
else... opp_get_data(opp) alone does not help, you'd need 
opp_get_data(opp,some_identfier) :).. hence the implementation..

-- 
Regards,
Nishanth Menon

RE: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Cousson, Benoit]

Hi Nishanth,

>From: Menon, Nishanth
>Sent: Friday, March 19, 2010 4:25 PM
>
>Felipe Balbi had written, on 03/19/2010 09:43 AM, the following:
>> hi,
>>
>> On Thu, Mar 18, 2010 at 07:44:50PM +0100, ext Nishanth Menon wrote:
>>> +int opp_store_data(struct omap_opp *opp, char *name, void *data)
>>> +{
>>> +   return -EINVAL;
>>
>> Would -ENODATA fit better ??
>wondered later on if 0 is better here and get_data with -ENODATA - for
>the actual code also..
>
>>
>>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>>> index bb8120e..15f6f7c 100644
>>> --- a/arch/arm/plat-omap/opp.c
>>> +++ b/arch/arm/plat-omap/opp.c
>>> @@ -14,12 +14,19 @@
>>> #include <linux/errno.h>
>>> #include <linux/err.h>
>>> #include <linux/init.h>
>>> +#include <linux/list.h>
>>> #include <linux/slab.h>
>>> #include <linux/cpufreq.h>
>>>
>>> #include <plat/opp_twl_tps.h>
>>> #include <plat/opp.h>
>>>
>>> +struct omap_opp_data {
>>> +   char *name;
>>> +   void *data;
>>> +   struct list_head list;
>>> +};
>>> +
>>> /**
>>>  * struct omap_opp - OMAP OPP description structure
>>>  * @enabled:        true/false - marking this OPP as enabled/disabled
>>> @@ -37,6 +44,7 @@ struct omap_opp {
>>>     unsigned long rate;
>>>     unsigned long u_volt;
>>>     u8 opp_id;
>>> +   struct list_head data_list;
>>> };
>>>
>>> /*
>>> @@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
>>>     opp->rate = opp_def->freq;
>>>     opp->enabled = opp_def->enabled;
>>>     opp->u_volt = opp_def->u_volt;
>>> +   INIT_LIST_HEAD(&opp->data_list);
>>> }
>>>
>>> int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
>>> @@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
>>>     return 0;
>>> }
>>>
>>> +void *opp_get_data(struct omap_opp *opp, char *name)
>>> +{
>>> +   void *data = ERR_PTR(-EINVAL);
>>> +   struct omap_opp_data *tmp;
>>> +
>>> +   if (unlikely(!opp || !name))
>>> +           return ERR_PTR(-EINVAL);
>>> +
>>> +   list_for_each_entry(tmp, &opp->data_list, list)
>>> +           if (!strcmp(name, tmp->name)) {
>>> +                   data = tmp->data;
>>> +                   break;
>>> +           }
>>> +   return data;
>>> +}
>>> +
>>> +int opp_store_data(struct omap_opp *opp, char *name, void *data)
>>> +{
>>> +   struct omap_opp_data *new;
>>> +   if (unlikely(!opp || !name))
>>> +           return -EINVAL;
>>> +   /* NAK to double registration */
>>> +   if (unlikely(!IS_ERR(opp_get_data(opp, name))))
>>> +           return -EINVAL;
>>> +
>>> +   new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
>>> +   if (!new)
>>> +           return -ENOMEM;
>>> +   new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
>>> +   if (!new->name) {
>>> +           kfree(new);
>>> +           return -ENOMEM;
>>> +   }
>>> +   new->data = data;
>>> +   strcpy(new->name, name);
>>> +   INIT_LIST_HEAD(&new->list);
>>> +   list_add(&new->list, &opp->data_list);
>>> +   return 0;
>>> +}
>>
>> will you really want to add several data entries there ? I don't see the
>> point. Maybe I'm missing something.
>>
>> I like the idea of having a void * to allow you to store anything there.
>> But generally that's a 1 - 1 relation:
>>
>> 1 device/bus/(in this case) opp - 1 data.
>
>I think I am lost here -> why device/bus/opp? this is a 1-many
>relationship. here is an example of it:
>OPP by definition is a tuple (voltage,frequency).
>
>for each OPP, we may have additional information which is custom to a
>specific SOC ->
>OMAP2 and OMAP1 have no SR module -> so I dont need to store SR nTarget
>value.
>
>data type 1: OMAP34xx and OMAP3630 have SR, and nTarget is a *per chip*
>calibration value specific to an OPP -however, we have 5 OPPs (or upto 7
>in the case of 3440 etc.) in omap34xx while omap3630 has upto 4 OPPs.
>in omap3630, we use ABB data which is in addition to nTarget (if my
>meager understanding of this is right)..
>
>data type 2: The dependency of vdd1 OPP to vdd2 opp is variant based on SOC.
>
>the types of data which is stored per OPP is changing all the time and
>in future we will have varied data again.
>
>Now, based on your proposal as I understand,
>struct omap2_data {
>blah_o21
>blah_o22
>};
>
>struct omap3_data {
>blah_o31
>blah_o32
>blah_o33
>}
>
>struct omap4_data {
>blah_o41
>blah_o42
>blah_o31
>blah_o32
>}
>
>and so on (remember we may have shared or similar data between various
>SOCs at times)
>
>redundancy, maintenance are the problems i see other than ability to
>have a module which uses blah_o31 to be generic and not know the
>difference between struct omap3_data and omap4_data
>the resultant module code will look like:
>if (cpu_is_omap3430()) {
>       my_blaho31 = ((struct omap3_data *) get_opp_data(opp))->blah_o31;
>} else if cpu_is...() {
>..
>}
>
>now in the approach I took,
>you could have:
>struct sr_ntarget_type{
>       unsigned long nTarget;
>       something else if needed
>}

I'm still not convinced...

It appears that there is still some confusion with what OPP is or should be.

The OPP layer, for my point of view was done to handle the freq -> voltage association per IP. This layer should be flexible because we can add or remove dynamically any OPP.

All the data related to SR and ABB belong to the characterized voltages that belong to a certain OPP for a voltage rail. These informations are not configurable at all and not related to any IP. They are well defined for a particular SoC techno (65nm, 45nm) for a voltage domain.
So they should belong to a SoC specific file, and should be hard coded for a voltage domain.
The point is that you should not tie SR Ntarget, ABB... to the OPP itself but to the voltage. Hence the need to have other voltage -> SR, ABB table in the voltage management code only.
We should not clutter the OPP layer with something that is voltage related.

The other point is that up to now, all voltage related data are a super set of the previous SoC data, so you can define only one voltage_data structure that work for all SoC. If the data is useless for previous platform, just put 0 or a flag to disable that parameter.


Regarding the link between voltage domains, there are two different types that should not be necessarily handled by the OPP layer.

- The first case is the one we have today to handle the MPU -> L3 dependency: In that case it is a pure policy dependency, that can or not be applied depending of the need and of the customer.
Is it the OPP layer responsibility to handle policy? If it is the case, we should add much more stuff to expose that to user mode and allow a certain amount of flexibility.
- The second case is due to HW limitation like on OMAP4, where we cannot have the highest OPP on MPU or IVA with the lowest one on the CORE.
In that case, we probably have to handle that in the low level voltage management part and not in the OPP layer.


Bottom line is I still think this is a little bit over-engineered for the real need, and on the other side that does not fully solve the needs.

Regards,
Benoit

Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Cousson, Benoit had written, on 03/21/2010 04:50 PM, the following:
[...]
>>
>> now in the approach I took,
>> you could have:
>> struct sr_ntarget_type{
>>       unsigned long nTarget;
>>       something else if needed
>> }
> 
> I'm still not convinced...
> 
> It appears that there is still some confusion with what OPP is or should be.

Errr.. Overall, the idea is to provide an infrastructure to store 
information per opp, OPP layer does not make policy decision - that is 
left to the caller (a.k.a user modules).

> 
> The OPP layer, for my point of view was done to handle the freq -> voltage association per IP. This layer should be flexible because we can add or remove dynamically any OPP.
yes, this is the basic SOC generic definition of OPP - i agree.
> 
> All the data related to SR and ABB belong to the characterized voltages that belong to a certain OPP for a voltage rail. These informations are not configurable at all and not related to any IP. They are well defined for a particular SoC techno (65nm, 45nm) for a voltage domain.
exactly the reason why this patch is relevant nominal voltage and 
frequencies per OPP is not considered dynamic. OPP layer is a data store 
layer - it stores information in a centralized manner allowing other 
dependent module to query, store and operate on that information.

the dependency as you rightly pointed out is:
voltage -> SR,ABB information.

but as you already know,
OPP = (freq,voltage)

hence there is a indirect dependency of SR, ABB information per OPP.

> So they should belong to a SoC specific file, and should be hard coded for a voltage domain.
ACK - the question is how would you do it? try to answer these questions:
a) where do you store SR ntarget values which is per OPP in a SOC 
generic way?
b) how do you allow SR ntarget value to be queried in a SOC generic way?

> The point is that you should not tie SR Ntarget, ABB... to the OPP itself but to the voltage. Hence the need to have other voltage -> SR, ABB table in the voltage management code only.
err.. that is exactly how it is done.

> We should not clutter the OPP layer with something that is voltage related.
I dont see your point here. OPP layer is just providing an 
infrastructure to store OPP related data.

> 
> The other point is that up to now, all voltage related data are a super set of the previous SoC data, so you can define only one voltage_data structure that work for all SoC. If the data is useless for previous platform, just put 0 or a flag to disable that parameter.
welcome to redundant data. How do you plan to support 35xx devices which 
do not have nTarget data? by putting 0s for those fields? you need to 
store this data at the end of the day in some data structure, you dont 
have a choice. the solution as I pointed in the previous mailchain is to 
provide an SOC generic manner which ties the data corresponding to a 
OPP(freq/voltage) to the OPP itself.

if you look at the implementation from Thara's patches today, see:
See:
https://patchwork.kernel.org/patch/86642/
you will endup with a code as follows:
arch/arm/mach-omap2/srdevice.c:

> +/* Read EFUSE values from control registers for OMAP3430 */
> +static void __init omap34xx_sr_read_efuse(struct omap_smartreflex_data *sr_data,
> +						int sr_id)
> +{
> +	if (sr_id == SR1) {
> +		sr_data->no_opp = opp_get_opp_count(OPP_MPU);
> +		sr_data->sr_nvalue = kzalloc(sizeof(sr_data->sr_nvalue) *
> +					sr_data->no_opp , GFP_KERNEL);
> +		if (WARN_ON(!sr_data->sr_nvalue))
> +			return;
> +
> +		sr_data->senn_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR) &
> +					OMAP343X_SR1_SENNENABLE_MASK) >>
> +					OMAP343X_SR1_SENNENABLE_SHIFT;
> +		sr_data->senp_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR) &
> +					OMAP343X_SR1_SENPENABLE_MASK) >>
> +					OMAP343X_SR1_SENPENABLE_SHIFT;
> +		sr_data->sr_nvalue[4] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP5_VDD1);
> +		sr_data->sr_nvalue[3] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP4_VDD1);
> +		sr_data->sr_nvalue[2] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP3_VDD1);
> +		sr_data->sr_nvalue[1] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP2_VDD1);
> +		sr_data->sr_nvalue[0] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP1_VDD1);
> +	} else if (sr_id == SR2) {
> +		sr_data->no_opp = 3;
> +		sr_data->sr_nvalue = kzalloc(sizeof(sr_data->sr_nvalue) *
> +					sr_data->no_opp , GFP_KERNEL);
> +		if (WARN_ON(!sr_data->sr_nvalue))
> +			return;
> +
> +		sr_data->senn_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR) &
> +					OMAP343X_SR2_SENNENABLE_MASK) >>
> +					OMAP343X_SR2_SENNENABLE_SHIFT;
> +		sr_data->senp_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR) &
> +					OMAP343X_SR2_SENPENABLE_MASK) >>
> +					OMAP343X_SR2_SENPENABLE_SHIFT;
> +		sr_data->sr_nvalue[2] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP3_VDD2);
> +		sr_data->sr_nvalue[1] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP2_VDD2);
> +		sr_data->sr_nvalue[0] = omap_ctrl_readl(
> +					OMAP343X_CONTROL_FUSE_OPP1_VDD2);
> +	}
> +}
well.. that is what you get if you decide that ONLY the module should 
store the data-> it will have to have the concept of OPP ID as a result!

Sorry, I disagree with this.

> 
> 
> Regarding the link between voltage domains, there are two different types that should not be necessarily handled by the OPP layer.
> 
> - The first case is the one we have today to handle the MPU -> L3 dependency: In that case it is a pure policy dependency, that can or not be applied depending of the need and of the customer.
> Is it the OPP layer responsibility to handle policy? If it is the case, we should add much more stuff to expose that to user mode and allow a certain amount of flexibility.
Errr... you missed the purpose of the patch I suspect - OPP layer did 
not implement policy - it just provided an uniform mechanism to store 
data, the policy of dependency is implemented in resource34xx.c.


> - The second case is due to HW limitation like on OMAP4, where we cannot have the highest OPP on MPU or IVA with the lowest one on the CORE.
> In that case, we probably have to handle that in the low level voltage management part and not in the OPP layer.
And how would you implement it? you need to describe this information 
some place without hardcoding it.. OPP layer allows you that flexibility.

> 
> 
> Bottom line is I still think this is a little bit over-engineered for the real need, and on the other side that does not fully solve the needs.
I strongly disagree with your opinion. I think the OPP layer's job is to 
store OPP relevant information which can be queried and operated upon by 
layers that implement policy and functionality - how and when they do it 
is up to them, OPP layer should provide a mechanism for data storage and 
this is introduced in this patchset.


-- 
Regards,
Nishanth Menon

RE: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Cousson, Benoit]

>From: Menon, Nishanth
>Sent: Monday, March 22, 2010 2:30 PM
>
>Cousson, Benoit had written, on 03/21/2010 04:50 PM, the following:
>[...]
>>>
>>> now in the approach I took,
>>> you could have:
>>> struct sr_ntarget_type{
>>>       unsigned long nTarget;
>>>       something else if needed
>>> }
>>
>> I'm still not convinced...
>>
>> It appears that there is still some confusion with what OPP is or should
>be.
>
>Errr.. Overall, the idea is to provide an infrastructure to store
>information per opp, OPP layer does not make policy decision - that is
>left to the caller (a.k.a user modules).
>
>>
>> The OPP layer, for my point of view was done to handle the freq ->
>voltage association per IP. This layer should be flexible because we can
>add or remove dynamically any OPP.
>yes, this is the basic SOC generic definition of OPP - i agree.
>>
>> All the data related to SR and ABB belong to the characterized voltages
>that belong to a certain OPP for a voltage rail. These informations are not
>configurable at all and not related to any IP. They are well defined for a
>particular SoC techno (65nm, 45nm) for a voltage domain.
>exactly the reason why this patch is relevant nominal voltage and
>frequencies per OPP is not considered dynamic. OPP layer is a data store
>layer - it stores information in a centralized manner allowing other
>dependent module to query, store and operate on that information.
>
>the dependency as you rightly pointed out is:
>voltage -> SR,ABB information.
>
>but as you already know,
>OPP = (freq,voltage)
>
>hence there is a indirect dependency of SR, ABB information per OPP.

Not at all, you missed the point of storing information per IP vs. information per voltage domain.
You first translate the freq to voltage for all IPs that belong to that voltage domain. If you have 10 IPs in the same voltage domain, you will not duplicate 10 times the SR informations.

>> So they should belong to a SoC specific file, and should be hard coded
>for a voltage domain.
>ACK - the question is how would you do it? try to answer these questions:
>a) where do you store SR ntarget values which is per OPP in a SOC
>generic way?
>b) how do you allow SR ntarget value to be queried in a SOC generic way?

By adding a voltage management API similar to the OPP management API, but dedicated to voltage domains.

>> The point is that you should not tie SR Ntarget, ABB... to the OPP itself
>but to the voltage. Hence the need to have other voltage -> SR, ABB table
>in the voltage management code only.
>err.. that is exactly how it is done.

Not really, it uses the OPP ID for the moment.

>> We should not clutter the OPP layer with something that is voltage
>related.
>I dont see your point here. OPP layer is just providing an
>infrastructure to store OPP related data.

This is the point... OPP layer is a freq/voltage tuple management layer not a voltage control layer. There is not necessarily a one to one mapping between OPP and voltage.

>>
>> The other point is that up to now, all voltage related data are a super
>set of the previous SoC data, so you can define only one voltage_data
>structure that work for all SoC. If the data is useless for previous
>platform, just put 0 or a flag to disable that parameter.
>welcome to redundant data. How do you plan to support 35xx devices which
>do not have nTarget data? by putting 0s for those fields? you need to
>store this data at the end of the day in some data structure, you dont
>have a choice. the solution as I pointed in the previous mailchain is to
>provide an SOC generic manner which ties the data corresponding to a
>OPP(freq/voltage) to the OPP itself.

Welcome to complex data management for a couple of 32 bits data :-)
If you look at the size of the infrastructure you have to put in place, I'm not even sure that you will save one bit.

>if you look at the implementation from Thara's patches today, see:
>See:
>https://patchwork.kernel.org/patch/86642/
>you will endup with a code as follows:
>arch/arm/mach-omap2/srdevice.c:
>
>> +/* Read EFUSE values from control registers for OMAP3430 */
>> +static void __init omap34xx_sr_read_efuse(struct omap_smartreflex_data
>*sr_data,
>> +                                            int sr_id)
>> +{
>> +    if (sr_id == SR1) {
>> +            sr_data->no_opp = opp_get_opp_count(OPP_MPU);
>> +            sr_data->sr_nvalue = kzalloc(sizeof(sr_data->sr_nvalue) *
>> +                                    sr_data->no_opp , GFP_KERNEL);
>> +            if (WARN_ON(!sr_data->sr_nvalue))
>> +                    return;
>> +
>> +            sr_data->senn_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR)
>&
>> +                                    OMAP343X_SR1_SENNENABLE_MASK) >>
>> +                                    OMAP343X_SR1_SENNENABLE_SHIFT;
>> +            sr_data->senp_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR)
>&
>> +                                    OMAP343X_SR1_SENPENABLE_MASK) >>
>> +                                    OMAP343X_SR1_SENPENABLE_SHIFT;
>> +            sr_data->sr_nvalue[4] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP5_VDD1);
>> +            sr_data->sr_nvalue[3] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP4_VDD1);
>> +            sr_data->sr_nvalue[2] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP3_VDD1);
>> +            sr_data->sr_nvalue[1] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP2_VDD1);
>> +            sr_data->sr_nvalue[0] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP1_VDD1);
>> +    } else if (sr_id == SR2) {
>> +            sr_data->no_opp = 3;
>> +            sr_data->sr_nvalue = kzalloc(sizeof(sr_data->sr_nvalue) *
>> +                                    sr_data->no_opp , GFP_KERNEL);
>> +            if (WARN_ON(!sr_data->sr_nvalue))
>> +                    return;
>> +
>> +            sr_data->senn_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR)
>&
>> +                                    OMAP343X_SR2_SENNENABLE_MASK) >>
>> +                                    OMAP343X_SR2_SENNENABLE_SHIFT;
>> +            sr_data->senp_mod = (omap_ctrl_readl(OMAP343X_CONTROL_FUSE_SR)
>&
>> +                                    OMAP343X_SR2_SENPENABLE_MASK) >>
>> +                                    OMAP343X_SR2_SENPENABLE_SHIFT;
>> +            sr_data->sr_nvalue[2] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP3_VDD2);
>> +            sr_data->sr_nvalue[1] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP2_VDD2);
>> +            sr_data->sr_nvalue[0] = omap_ctrl_readl(
>> +                                    OMAP343X_CONTROL_FUSE_OPP1_VDD2);
>> +    }
>> +}
>well.. that is what you get if you decide that ONLY the module should
>store the data-> it will have to have the concept of OPP ID as a result!
>
>Sorry, I disagree with this.

That, I agree (with you). We need to get rid of the OPP ID stuff.

>>
>> Regarding the link between voltage domains, there are two different types
>that should not be necessarily handled by the OPP layer.
>>
>> - The first case is the one we have today to handle the MPU -> L3
>dependency: In that case it is a pure policy dependency, that can or not be
>applied depending of the need and of the customer.
>> Is it the OPP layer responsibility to handle policy? If it is the case,
>we should add much more stuff to expose that to user mode and allow a
>certain amount of flexibility.
>Errr... you missed the purpose of the patch I suspect - OPP layer did
>not implement policy - it just provided an uniform mechanism to store
>data, the policy of dependency is implemented in resource34xx.c.

Not at all, that's why I'm asking you if you'd like to do that.
If you want to add the storage in the OPP layer, just add an explicit attribute in the OPP structure. No need to have a dynamic stuff to do that.

>> - The second case is due to HW limitation like on OMAP4, where we cannot
>have the highest OPP on MPU or IVA with the lowest one on the CORE.
>> In that case, we probably have to handle that in the low level voltage
>management part and not in the OPP layer.
>And how would you implement it? you need to describe this information
>some place without hardcoding it.. OPP layer allows you that flexibility.

In fact that case should be hardcoded. It will be SoC dependant.

>> Bottom line is I still think this is a little bit over-engineered for the
>real need, and on the other side that does not fully solve the needs.
>I strongly disagree with your opinion. I think the OPP layer's job is to
>store OPP relevant information which can be queried and operated upon by
>layers that implement policy and functionality - how and when they do it
>is up to them, OPP layer should provide a mechanism for data storage and
>this is introduced in this patchset.

Part of the problem I think, is due to the confusion between an OPP layer and a voltage control layer.
Other part of the problem, is the abuse of "void *" to store any kind of stuff that should be well typed attributes in the OPP structure.

Otherwise the primary idea to remove OPP ID is good, and the way to go.

Regards,
Benoit

Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Cousson, Benoit had written, on 03/22/2010 12:46 PM, the following:
>> From: Menon, Nishanth
>> Sent: Monday, March 22, 2010 2:30 PM
>>
>> Cousson, Benoit had written, on 03/21/2010 04:50 PM, the following:
>> [...]
>>>> now in the approach I took,
>>>> you could have:
>>>> struct sr_ntarget_type{
>>>>       unsigned long nTarget;
>>>>       something else if needed
>>>> }
>>> I'm still not convinced...
>>>
>>> It appears that there is still some confusion with what OPP is or should
>> be.
>>
>> Errr.. Overall, the idea is to provide an infrastructure to store
>> information per opp, OPP layer does not make policy decision - that is
>> left to the caller (a.k.a user modules).
>>
>>> The OPP layer, for my point of view was done to handle the freq ->
>> voltage association per IP. This layer should be flexible because we can
>> add or remove dynamically any OPP.
>> yes, this is the basic SOC generic definition of OPP - i agree.
>>> All the data related to SR and ABB belong to the characterized voltages
>> that belong to a certain OPP for a voltage rail. These informations are not
>> configurable at all and not related to any IP. They are well defined for a
>> particular SoC techno (65nm, 45nm) for a voltage domain.
>> exactly the reason why this patch is relevant nominal voltage and
>> frequencies per OPP is not considered dynamic. OPP layer is a data store
>> layer - it stores information in a centralized manner allowing other
>> dependent module to query, store and operate on that information.
>>
>> the dependency as you rightly pointed out is:
>> voltage -> SR,ABB information.
>>
>> but as you already know,
>> OPP = (freq,voltage)
>>
>> hence there is a indirect dependency of SR, ABB information per OPP.
> 
> Not at all, you missed the point of storing information per IP vs. information per voltage domain.
> You first translate the freq to voltage for all IPs that belong to that voltage domain. If you have 10 IPs in the same voltage domain, you will not duplicate 10 times the SR informations.
I dont think you will. if you register with opp layer 10 times different 
data, well.. you'd do it.. but anyways.. that is a minor point.

> 
>>> So they should belong to a SoC specific file, and should be hard coded
>> for a voltage domain.
>> ACK - the question is how would you do it? try to answer these questions:
>> a) where do you store SR ntarget values which is per OPP in a SOC
>> generic way?
>> b) how do you allow SR ntarget value to be queried in a SOC generic way?
> 
> By adding a voltage management API similar to the OPP management API, but dedicated to voltage domains.

I like that approach - do we have anything that does it today? nope. the 
intent of doing SR changes was to introduce these changes, but seems to 
have been missed somewhere down the pipe. shrug.

> 
>>> The point is that you should not tie SR Ntarget, ABB... to the OPP itself
>> but to the voltage. Hence the need to have other voltage -> SR, ABB table
>> in the voltage management code only.
>> err.. that is exactly how it is done.
> 
> Not really, it uses the OPP ID for the moment.
Seriously - that is not an explanation - it is a bad implementation.

> 
>>> We should not clutter the OPP layer with something that is voltage
>> related.
>> I dont see your point here. OPP layer is just providing an
>> infrastructure to store OPP related data.
> 
> This is the point... OPP layer is a freq/voltage tuple management layer not a voltage control layer. There is not necessarily a one to one mapping between OPP and voltage.
I agree from a theoretical perspective. the user of voltage management 
layer needs the opp layer to know the voltages. The question is where do 
you want to store the voltage related information?

> 
>>> The other point is that up to now, all voltage related data are a super
>> set of the previous SoC data, so you can define only one voltage_data
>> structure that work for all SoC. If the data is useless for previous
>> platform, just put 0 or a flag to disable that parameter.
>> welcome to redundant data. How do you plan to support 35xx devices which
>> do not have nTarget data? by putting 0s for those fields? you need to
>> store this data at the end of the day in some data structure, you dont
>> have a choice. the solution as I pointed in the previous mailchain is to
>> provide an SOC generic manner which ties the data corresponding to a
>> OPP(freq/voltage) to the OPP itself.
> 
> Welcome to complex data management for a couple of 32 bits data :-)
:)
> If you look at the size of the infrastructure you have to put in place, I'm not even sure that you will save one bit.
I dont claim so.. but it stores only relevant information.. lists are 
not exactly the most optimal logic, it is just the most flexible :)

> 
>> if you look at the implementation from Thara's patches today, see:
>> See:
>> https://patchwork.kernel.org/patch/86642/
>> you will endup with a code as follows:
>> arch/arm/mach-omap2/srdevice.c:
>>
>>> +/* Read EFUSE values from control registers for OMAP3430 */
>>> +static void __init omap34xx_sr_read_efuse(struct omap_smartreflex_data
>> *sr_data,
>>> +                                            int sr_id)
>>> +{
>>> +    if (sr_id == SR1) {
[...]
>>> +            sr_data->sr_nvalue[0] = omap_ctrl_readl(
>>> +                                    OMAP343X_CONTROL_FUSE_OPP1_VDD2);
>>> +    }
>>> +}
>> well.. that is what you get if you decide that ONLY the module should
>> store the data-> it will have to have the concept of OPP ID as a result!
>>
>> Sorry, I disagree with this.
> 
> That, I agree (with you). We need to get rid of the OPP ID stuff.

[...] [I am not about to repeat everything I stated in previous 
threads.. so  snipping the discussion down.]

> 
> Otherwise the primary idea to remove OPP ID is good, and the way to go.

good. So lets NAK that SR series and see how else we can implement it 
without OPP ID. I am open to any clean mechanisms you may propose 
without using OPP ID referencing :).

-- 
Regards,
Nishanth Menon

RE: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Gopinath, Thara]

>>
>>[...] [I am not about to repeat everything I stated in previous
>>threads.. so  snipping the discussion down.]
>>
>>>
>>> Otherwise the primary idea to remove OPP ID is good, and the way to go.
>>
>>good. So lets NAK that SR series and see how else we can implement it
>>without OPP ID. I am open to any clean mechanisms you may propose
>>without using OPP ID referencing :).

Ok guys.. In the current V2 series , I have linked N targets with voltage.. Only it does not come from voltage layer but from smartreflex devices layer. The smartreflex driver does not use opp ids at all.. Also whether you call it by opp ids or by any other name, we need to know the number of different voltages supported by VDD1 and VDD2 and form the table.. That is exactly what I am doing in smartreflex device layer. I am just creating a table with different voltages and N target values associated with those voltages. To create this table I need to know there should be 5 voltages for VDD1 and 3 voltages for VDD2 which unfortunately coincides with the number of different OPP's defined in OMAP3430 today.. 
I also think it is an excellent idea to NAK a series of 19 patches for which everybody has been shouting for, for this reason.

Regards
Thara  

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Gopinath, Thara had written, on 03/23/2010 12:06 AM, the following:
> 
>>> [...] [I am not about to repeat everything I stated in previous
>>> threads.. so  snipping the discussion down.]
>>>
>>>> Otherwise the primary idea to remove OPP ID is good, and the way to go.
>>> good. So lets NAK that SR series and see how else we can implement it
>>> without OPP ID. I am open to any clean mechanisms you may propose
>>> without using OPP ID referencing :).
> 
> Ok guys.. In the current V2 series , I have linked N targets with voltage..
 > Only it does not come from voltage layer but from smartreflex devices 
layer.
 > The smartreflex driver does not use opp ids at all.. Also whether you 
call
 >it by opp ids or by any other name, we need to know the number of 
different
 > voltages supported by VDD1 and VDD2 and form the table.. That is exactly
 >what I am doing in smartreflex device layer. I am just creating a 
table with
 >different voltages and N target values associated with those voltages.
 > To create this table I need to know there should be 5 voltages for VDD1
 > and 3 voltages for VDD2 which unfortunately coincides with the number of
 > different OPP's defined in OMAP3430 today..
SR patches should ideally be discussed in SR patch series context 
anyways, since we are looking at the fundamentals of OPP:
3430: VDD1: 5 voltages+frequencies, VDD2: 3 voltages and frequencies
3630: VDD1: 4 voltages+frequencies, VDD2: 2 voltages and frequencies
are there cases where the number of discrete voltage != number of 
supported frequencies?

Agreed that for a voltage the characteristic data is unique. but since a 
voltage is tied to a frequency, does'nt it make sense to tie it to an 
OPP (my initial point in the first place)?

look at the amount of redundant data:
  static void __init omap34xx_sr_volt_details(struct omap_smartreflex_data
                                                  *sr_data, int srid)
  {
          if (srid == SR1) {
                  sr_data->no_opp = opp_get_opp_count(OPP_MPU);
                  sr_data->sr_volt_data = 
kzalloc(sizeof(sr_data->sr_volt_data) *
                                  sr_data->no_opp , GFP_KERNEL);
                  WARN_ON(!sr_data->sr_volt_data);
                  sr_data->sr_volt_data[0].voltage = 975000;
                  sr_data->sr_volt_data[1].voltage = 1075000;
                  sr_data->sr_volt_data[2].voltage = 1200000;
                  sr_data->sr_volt_data[3].voltage = 1270000;
                  sr_data->sr_volt_data[4].voltage = 1350000;
          } else if (srid == SR2) {
                  sr_data->no_opp = 3;
                  sr_data->sr_volt_data = 
kzalloc(sizeof(sr_data->sr_volt_data) *
                                  sr_data->no_opp , GFP_KERNEL);
                  WARN_ON(!sr_data->sr_volt_data);
                  sr_data->sr_volt_data[0].voltage = 975000;
                  sr_data->sr_volt_data[1].voltage = 1050000;
                  sr_data->sr_volt_data[2].voltage = 1150000;
          }
  }

If you link sr_volt_data with OPP, you have a simple scalable soln 
without having to manage voltage in multiple places etc.

the implementation is definitely based on number of OPPs :). and it is 
not exactly the most scalable implementation currently present.
> I also think it is an excellent idea to NAK a series of 19 patches for
 > which everybody has been shouting for, for this reason.
NAK was for SRID usage and voltage indexing which makes scaling across 
silicon variants troublesome.

-- 
Regards,
Nishanth Menon

RE: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Cousson, Benoit]

>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner@vger.kernel.org] On Behalf Of Menon, Nishanth
>Sent: Tuesday, March 23, 2010 2:00 PM
>
>Gopinath, Thara had written, on 03/23/2010 12:06 AM, the following:
>>
>>>> [...] [I am not about to repeat everything I stated in previous
>>>> threads.. so  snipping the discussion down.]
>>>>
>>>>> Otherwise the primary idea to remove OPP ID is good, and the way to go.
>>>> good. So lets NAK that SR series and see how else we can implement it
>>>> without OPP ID. I am open to any clean mechanisms you may propose
>>>> without using OPP ID referencing :).
>>
>> Ok guys.. In the current V2 series , I have linked N targets with
>voltage..
> > Only it does not come from voltage layer but from smartreflex devices
>layer.
> > The smartreflex driver does not use opp ids at all.. Also whether you
>call
> >it by opp ids or by any other name, we need to know the number of
>different
> > voltages supported by VDD1 and VDD2 and form the table.. That is exactly
> >what I am doing in smartreflex device layer. I am just creating a
>table with
> >different voltages and N target values associated with those voltages.
> > To create this table I need to know there should be 5 voltages for VDD1
> > and 3 voltages for VDD2 which unfortunately coincides with the number of
> > different OPP's defined in OMAP3430 today..
>SR patches should ideally be discussed in SR patch series context
>anyways, since we are looking at the fundamentals of OPP:
>3430: VDD1: 5 voltages+frequencies, VDD2: 3 voltages and frequencies
>3630: VDD1: 4 voltages+frequencies, VDD2: 2 voltages and frequencies
>are there cases where the number of discrete voltage != number of
>supported frequencies?

If you'd like to support DPLL bypass or several frequencies for thermal management purpose it can happen. And believe me; it will happen soon, at least on OMAP4.

>Agreed that for a voltage the characteristic data is unique. but since a
>voltage is tied to a frequency, does'nt it make sense to tie it to an
>OPP (my initial point in the first place)?

Frequency is an IP characteristic, not a voltage one, hence the need to separate them.

Regards,
Benoit

>look at the amount of redundant data:
>  static void __init omap34xx_sr_volt_details(struct omap_smartreflex_data
>                                                  *sr_data, int srid)
>  {
>          if (srid == SR1) {
>                  sr_data->no_opp = opp_get_opp_count(OPP_MPU);
>                  sr_data->sr_volt_data =
>kzalloc(sizeof(sr_data->sr_volt_data) *
>                                  sr_data->no_opp , GFP_KERNEL);
>                  WARN_ON(!sr_data->sr_volt_data);
>                  sr_data->sr_volt_data[0].voltage = 975000;
>                  sr_data->sr_volt_data[1].voltage = 1075000;
>                  sr_data->sr_volt_data[2].voltage = 1200000;
>                  sr_data->sr_volt_data[3].voltage = 1270000;
>                  sr_data->sr_volt_data[4].voltage = 1350000;
>          } else if (srid == SR2) {
>                  sr_data->no_opp = 3;
>                  sr_data->sr_volt_data =
>kzalloc(sizeof(sr_data->sr_volt_data) *
>                                  sr_data->no_opp , GFP_KERNEL);
>                  WARN_ON(!sr_data->sr_volt_data);
>                  sr_data->sr_volt_data[0].voltage = 975000;
>                  sr_data->sr_volt_data[1].voltage = 1050000;
>                  sr_data->sr_volt_data[2].voltage = 1150000;
>          }
>  }
>
>If you link sr_volt_data with OPP, you have a simple scalable soln
>without having to manage voltage in multiple places etc.
>
>the implementation is definitely based on number of OPPs :). and it is
>not exactly the most scalable implementation currently present.
>> I also think it is an excellent idea to NAK a series of 19 patches for
> > which everybody has been shouting for, for this reason.
>NAK was for SRID usage and voltage indexing which makes scaling across
>silicon variants troublesome.
>
>--
>Regards,
>Nishanth Menon
>--
>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PM-WIP-OPP][PATCH 3/4] omap: pm: opp: add ability to store data per opp

[Nishanth Menon]

Cousson, Benoit had written, on 03/23/2010 11:12 AM, the following:
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Menon, Nishanth
>> Sent: Tuesday, March 23, 2010 2:00 PM
>>
>> Gopinath, Thara had written, on 03/23/2010 12:06 AM, the following:
>>>>> [...] [I am not about to repeat everything I stated in previous
>>>>> threads.. so  snipping the discussion down.]
>>>>>
>>>>>> Otherwise the primary idea to remove OPP ID is good, and the way to go.
>>>>> good. So lets NAK that SR series and see how else we can implement it
>>>>> without OPP ID. I am open to any clean mechanisms you may propose
>>>>> without using OPP ID referencing :).
>>> Ok guys.. In the current V2 series , I have linked N targets with
>> voltage..
>>> Only it does not come from voltage layer but from smartreflex devices
>> layer.
>>> The smartreflex driver does not use opp ids at all.. Also whether you
>> call
>>> it by opp ids or by any other name, we need to know the number of
>> different
>>> voltages supported by VDD1 and VDD2 and form the table.. That is exactly
>>> what I am doing in smartreflex device layer. I am just creating a
>> table with
>>> different voltages and N target values associated with those voltages.
>>> To create this table I need to know there should be 5 voltages for VDD1
>>> and 3 voltages for VDD2 which unfortunately coincides with the number of
>>> different OPP's defined in OMAP3430 today..
>> SR patches should ideally be discussed in SR patch series context
>> anyways, since we are looking at the fundamentals of OPP:
>> 3430: VDD1: 5 voltages+frequencies, VDD2: 3 voltages and frequencies
>> 3630: VDD1: 4 voltages+frequencies, VDD2: 2 voltages and frequencies
>> are there cases where the number of discrete voltage != number of
>> supported frequencies?
> 
> If you'd like to support DPLL bypass or several frequencies for thermal management purpose it can happen. And believe me; it will happen soon, at least on OMAP4.
> 
>> Agreed that for a voltage the characteristic data is unique. but since a
>> voltage is tied to a frequency, does'nt it make sense to tie it to an
>> OPP (my initial point in the first place)?
> 
> Frequency is an IP characteristic, not a voltage one, hence the need to separate them.
> 
[..]

Sigh.. quickly becoming an SR thread, but what the heck..

Trying to brainstorm here: Can we consider an inverse relationship? As 
in: For a given voltage to a voltage rail, there exists a supported 
range of frequencies for specific IP modules? does'nt this make sense? I 
mean for a given voltage to the module, there is only so much range of 
frequencies you can do on the module that sinks that voltage?

In the case of OMAP3, then we will have a 1-1 relation ship, on OMAP4, 
you'd potentially have 1-many relationship..

If this is the representation, then storing that information will still 
make sense.

The v2 patches implement a logic as follows:
sr_device.c: arch_initcall:
	omap34xx_sr_volt_details
this currently stores voltages (essentially replicates OPP layer 
information from cpufreq34xx.c other than using OPP indices). The NAK 
for this approach is still valid from maintainence and redundancy 
perspective.

What other alternatives do we have?

Here is what I can imagine from the few mins I spend thinking about it 
(just my 2 cents):

int omap_voltage_notify(omap_device *volt_dev, struct omap_voltage_data 
*new, struct omap_voltage_data *old, enum voltage_notify_type);

enum voltage_notify_type {
	VOLTAGE_PRE
	VOLTAGE_ACTIVATE
	VOLTAGE_POST
	VOLTAGE_PAUSE
	VOLTAGE_RESUME
}
or split them up into 4/5 functions..

I will not need to store SRID or any similar stuff inside various files 
NOR will i have to replicate another OPP layer in SR/vc codebase.

volt_device will contain platform_data similar to what 
omap_smartreflex_data has today + additional register baseaddress and 
offset information. it will not contain sr_nvalue, nor sr_volt_data

struct omap_sr_volt_data will be omap_voltage_data be similar to what we 
provide today.

The caller has the responsibility of providing the correct voltage_data 
to the voltage subsystem.

So for today (resource34xx.c or pm34xx.c which ever is triggering the 
chage) the caller will provide the trigger to the voltage layer- for 
OMAP3, store the voltage_data against each OPP. For OMAP4 and elsewhere 
the data could be somewhere else (hwmod itself perhaps if it triggers 
the transitions eventually?). It seems to scale to me + all those hacks 
such as get_curr_vdd[12]_voltage() in the current voltage.c can be avoided.


-- 
Regards,
Nishanth Menon

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> BUG_ON should not ideally contain a functional code. Remove it out.

True.  But this code should not be using BUG_ON() in the first place.

We should not crash the whole kernel in this case, just fail
with a warning.

If you're cleaning this up, can you make it fail more gracefully.

Kevin


> Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2
>
> Cc: Ambresh K <ambresh@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Tero Kristo <tero.kristo@nokia.com>
> Cc: Thara Gopinath <thara@ti.com>
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/cpufreq34xx.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
> index c453ec5..f0ed3ae 100644
> --- a/arch/arm/mach-omap2/cpufreq34xx.c
> +++ b/arch/arm/mach-omap2/cpufreq34xx.c
> @@ -111,6 +111,7 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
>  
>  void __init omap3_pm_init_opp_table(void)
>  {
> +	int r;
>  	struct omap_opp_def **omap3_opp_def_list;
>  	struct omap_opp_def *omap34xx_opp_def_list[] = {
>  		omap34xx_mpu_rate_table,
> @@ -126,8 +127,9 @@ void __init omap3_pm_init_opp_table(void)
>  	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>  				omap34xx_opp_def_list;
>  
> -	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
> -	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
> -	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
> +	r = opp_init_list(OPP_MPU, omap3_opp_def_list[0]);
> +	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
> +	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
> +	BUG_ON(r);
>  }
>  
> -- 
> 1.6.3.3

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

Kevin Hilman had written, on 03/18/2010 05:49 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> BUG_ON should not ideally contain a functional code. Remove it out.
> 
> True.  But this code should not be using BUG_ON() in the first place.
> 
> We should not crash the whole kernel in this case, just fail
> with a warning.
> 
> If you're cleaning this up, can you make it fail more gracefully.
I agree  if this was a preipheral driver or a non-critical path. but in 
this case:

a) we are speaking of a core description of the h/w - OPPs frequencies 
and voltages which out which the functionality of the system is at 
stake. I am not speaking of just having a basic kernel boot up to shell 
prompt - we need the kernel to do much better than that.

b) Is there any reason why the registration could fail - if it did fail 
at this point, there is something catastrophic happening - some other 
driver is going beserk OR Opp layer is by itself screwed up - why 
continue if we can warn the system and force a fix of the code?

c) is there a recovery mechanism to put the system back in a usable mode 
with dvfs etc? I might prefer to get some ideas on it..

> 
> Kevin
> 
> 
>> Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2
>>
>> Cc: Ambresh K <ambresh@ti.com>
>> Cc: Benoit Cousson <b-cousson@ti.com>
>> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Cc: Phil Carmody <ext-phil.2.carmody@nokia.com>
>> Cc: Sanjeev Premi <premi@ti.com>
>> Cc: Tero Kristo <tero.kristo@nokia.com>
>> Cc: Thara Gopinath <thara@ti.com>
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>>  arch/arm/mach-omap2/cpufreq34xx.c |    8 +++++---
>>  1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c
>> index c453ec5..f0ed3ae 100644
>> --- a/arch/arm/mach-omap2/cpufreq34xx.c
>> +++ b/arch/arm/mach-omap2/cpufreq34xx.c
>> @@ -111,6 +111,7 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = {
>>  
>>  void __init omap3_pm_init_opp_table(void)
>>  {
>> +	int r;
>>  	struct omap_opp_def **omap3_opp_def_list;
>>  	struct omap_opp_def *omap34xx_opp_def_list[] = {
>>  		omap34xx_mpu_rate_table,
>> @@ -126,8 +127,9 @@ void __init omap3_pm_init_opp_table(void)
>>  	omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>>  				omap34xx_opp_def_list;
>>  
>> -	BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0]));
>> -	BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1]));
>> -	BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2]));
>> +	r = opp_init_list(OPP_MPU, omap3_opp_def_list[0]);
>> +	r |= opp_init_list(OPP_L3, omap3_opp_def_list[1]);
>> +	r |= opp_init_list(OPP_DSP, omap3_opp_def_list[2]);
>> +	BUG_ON(r);
>>  }
>>  
>> -- 
>> 1.6.3.3


-- 
Regards,
Nishanth Menon

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Felipe Balbi]

On Fri, Mar 19, 2010 at 03:21:34PM +0100, ext Nishanth Menon wrote:
>Kevin Hilman had written, on 03/18/2010 05:49 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> BUG_ON should not ideally contain a functional code. Remove it out.
>>
>> True.  But this code should not be using BUG_ON() in the first place.
>>
>> We should not crash the whole kernel in this case, just fail
>> with a warning.
>>
>> If you're cleaning this up, can you make it fail more gracefully.
>I agree  if this was a preipheral driver or a non-critical path. but in
>this case:
>
>a) we are speaking of a core description of the h/w - OPPs frequencies
>and voltages which out which the functionality of the system is at
>stake. I am not speaking of just having a basic kernel boot up to shell
>prompt - we need the kernel to do much better than that.
>
>b) Is there any reason why the registration could fail - if it did fail
>at this point, there is something catastrophic happening - some other
>driver is going beserk OR Opp layer is by itself screwed up - why
>continue if we can warn the system and force a fix of the code?

I agree with Nishanth here. If it fails at this point, we deserve the 
BUG().

-- 
balbi

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 03/18/2010 05:49 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> BUG_ON should not ideally contain a functional code. Remove it out.
>>
>> True.  But this code should not be using BUG_ON() in the first place.
>>
>> We should not crash the whole kernel in this case, just fail
>> with a warning.
>>
>> If you're cleaning this up, can you make it fail more gracefully.
> I agree  if this was a preipheral driver or a non-critical path. but
> in this case:
>
> a) we are speaking of a core description of the h/w - OPPs frequencies
> and voltages which out which the functionality of the system is at
> stake. I am not speaking of just having a basic kernel boot up to
> shell prompt - we need the kernel to do much better than that.

A system can boot fine without OPPs/DVFS.  OPPs will not be
registered with CPUfreq, and no DVFS atempts will be made.  

> b) Is there any reason why the registration could fail - if it did
> fail at this point, there is something catastrophic happening - some
> other driver is going beserk OR Opp layer is by itself screwed up -
> why continue if we can warn the system and force a fix of the code?

Using WARN() will produce a nice loud message that alert users, get
reported and get fixed as well.

> c) is there a recovery mechanism to put the system back in a usable
> mode with dvfs etc? I might prefer to get some ideas on it..

What is to recover from?  While not optimal in power/performance, a
kernel can boot and work just fine without OPPs/DVFS.  If this call
fails, no DVFS will be available but the system is still quite usable.

The bigger problem is that everyone things that their
feature/subsystem is so crucial that any problems should hang the
system, when things could actually continue just fine without it in
most cases.

IMO, Using BUG* macros usually indicates improper or incomplete error
handling rather than a real catastrophic system failure.

Kevin

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Felipe Balbi]

On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
> IMO, Using BUG* macros usually indicates improper or incomplete error
> handling rather than a real catastrophic system failure.

on the other hand a kernel oops and system hang will always get noted. Rather
than a WARN() which simply sits in the log buffer. Remember that not all
users will be looking at console. If there's a kernel hang, we get a
core dump which is saved in the mtdoops (if you're using it) and can
send later to some central server for analysis.

-- 
balbi

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Kevin Hilman]

Felipe Balbi <me@felipebalbi.com> writes:

> On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
>> IMO, Using BUG* macros usually indicates improper or incomplete error
>> handling rather than a real catastrophic system failure.
>
> on the other hand a kernel oops and system hang will always get
> noted. Rather than a WARN() which simply sits in the log buffer.

Of course, but what I'm trying to avoid is making other people deal
with a BUG inserted by a developer when proper error checking and
recovery is what is really needed.

Kevin

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

Kevin Hilman had written, on 03/19/2010 01:42 PM, the following:
> Felipe Balbi <me@felipebalbi.com> writes:
> 
>> On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
>>> IMO, Using BUG* macros usually indicates improper or incomplete error
>>> handling rather than a real catastrophic system failure.
>> on the other hand a kernel oops and system hang will always get
>> noted. Rather than a WARN() which simply sits in the log buffer.
> 
> Of course, but what I'm trying to avoid is making other people deal
> with a BUG inserted by a developer when proper error checking and
> recovery is what is really needed.
> 
I respect your views. but a few moments of thoughts:
how would the recovery look like? I can think of 2 options here.. do 
share your views:

Option 1:
if (opp_init_list(OPP_MPU, omap3_opp_def_list[0])) {
	WARN("dsp OPP table registration failed");
	return;
}
if (opp_init_list(OPP_L3, omap3_opp_def_list[1])) {
	WARN("dsp OPP table registration failed");
	return;
}
if (opp_init_list(OPP_DSP, omap3_opp_def_list[2])) {
	WARN("dsp OPP table registration failed");
	return;
}

Option 2:
	if (opp_init_list(OPP_MPU, omap3_opp_def_list[0]))
		return;
	if (opp_init_list(OPP_L3, omap3_opp_def_list[1]))
		goto mpu_disable;
	if (opp_init_list(OPP_DSP, omap3_opp_def_list[2]))
		goto l3_disable;
	return;

l3_disable:
	freq = 0;
	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_L3, &freq)) {
		opp_disable(opp);
		freq++;
	}
mpu_disable:
	freq = 0;
	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq)) {
		opp_disable(opp);
		freq++;
	}
	WARN("Registration of OPP tables failed!!");
	return;

Option 1 is a bad idea as it leaves the system in an invalid state
Option 2 is the better idea as we dont have a opp_delete option(not 
required usually).

All that code for something that will almost never happen?	
-- 
Regards,
Nishanth Menon

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman had written, on 03/19/2010 01:42 PM, the following:
>> Felipe Balbi <me@felipebalbi.com> writes:
>>
>>> On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
>>>> IMO, Using BUG* macros usually indicates improper or incomplete error
>>>> handling rather than a real catastrophic system failure.
>>> on the other hand a kernel oops and system hang will always get
>>> noted. Rather than a WARN() which simply sits in the log buffer.
>>
>> Of course, but what I'm trying to avoid is making other people deal
>> with a BUG inserted by a developer when proper error checking and
>> recovery is what is really needed.
>>
> I respect your views. but a few moments of thoughts:
> how would the recovery look like? I can think of 2 options here.. do
> share your views:
>
> Option 1:
> if (opp_init_list(OPP_MPU, omap3_opp_def_list[0])) {
> 	WARN("dsp OPP table registration failed");
> 	return;
> }
> if (opp_init_list(OPP_L3, omap3_opp_def_list[1])) {
> 	WARN("dsp OPP table registration failed");
> 	return;
> }
> if (opp_init_list(OPP_DSP, omap3_opp_def_list[2])) {
> 	WARN("dsp OPP table registration failed");
> 	return;
> }
>
> Option 2:
> 	if (opp_init_list(OPP_MPU, omap3_opp_def_list[0]))
> 		return;
> 	if (opp_init_list(OPP_L3, omap3_opp_def_list[1]))
> 		goto mpu_disable;
> 	if (opp_init_list(OPP_DSP, omap3_opp_def_list[2]))
> 		goto l3_disable;
> 	return;
>
> l3_disable:
> 	freq = 0;
> 	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_L3, &freq)) {
> 		opp_disable(opp);
> 		freq++;
> 	}
> mpu_disable:
> 	freq = 0;
> 	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq)) {
> 		opp_disable(opp);
> 		freq++;
> 	}
> 	WARN("Registration of OPP tables failed!!");
> 	return;
>
> Option 1 is a bad idea as it leaves the system in an invalid state
> Option 2 is the better idea as we dont have a opp_delete option(not
> required usually).

I'm OK with either actually, as either is better than BUG_ON()
instead of error checking.

With option 1, the system is not really in an invalid state, just and
untested state.  What will happen is users of the OPP API will error
codes when trying to get OPPs and they should fail gracefully as well.

Once again, this is about proper error checking, and robustness, not
about causing a panic() when something (relatively) minor happens.

> All that code for something that will almost never happen?	

Yes.  That's what error checking is all about.

Kevin

P.S. I can't find the ref atm, but I mentioned not liking the BUG_ON
     early in the review of the OPP layer, and it would need to be
     cleaned up before upstream merge.

Re: [PM-WIP-OPP][PATCH 1/4] omap3: pm: cpufreq: BUG_ON cleanup

[Nishanth Menon]

Kevin Hilman had written, on 03/19/2010 03:49 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Kevin Hilman had written, on 03/19/2010 01:42 PM, the following:
>>> Felipe Balbi <me@felipebalbi.com> writes:
>>>
>>>> On Fri, Mar 19, 2010 at 10:46:54AM -0700, Kevin Hilman wrote:
>>>>> IMO, Using BUG* macros usually indicates improper or incomplete error
>>>>> handling rather than a real catastrophic system failure.
>>>> on the other hand a kernel oops and system hang will always get
>>>> noted. Rather than a WARN() which simply sits in the log buffer.
>>> Of course, but what I'm trying to avoid is making other people deal
>>> with a BUG inserted by a developer when proper error checking and
>>> recovery is what is really needed.
>>>
>> I respect your views. but a few moments of thoughts:
>> how would the recovery look like? I can think of 2 options here.. do
>> share your views:
>>
>> Option 1:
>> if (opp_init_list(OPP_MPU, omap3_opp_def_list[0])) {
>> 	WARN("dsp OPP table registration failed");
>> 	return;
>> }
>> if (opp_init_list(OPP_L3, omap3_opp_def_list[1])) {
>> 	WARN("dsp OPP table registration failed");
>> 	return;
>> }
>> if (opp_init_list(OPP_DSP, omap3_opp_def_list[2])) {
>> 	WARN("dsp OPP table registration failed");
>> 	return;
>> }
>>
>> Option 2:
>> 	if (opp_init_list(OPP_MPU, omap3_opp_def_list[0]))
>> 		return;
>> 	if (opp_init_list(OPP_L3, omap3_opp_def_list[1]))
>> 		goto mpu_disable;
>> 	if (opp_init_list(OPP_DSP, omap3_opp_def_list[2]))
>> 		goto l3_disable;
>> 	return;
>>
>> l3_disable:
>> 	freq = 0;
>> 	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_L3, &freq)) {
>> 		opp_disable(opp);
>> 		freq++;
>> 	}
>> mpu_disable:
>> 	freq = 0;
>> 	while (!IS_ERR(opp = opp_find_freq_ceil(OPP_MPU, &freq)) {
>> 		opp_disable(opp);
>> 		freq++;
>> 	}
>> 	WARN("Registration of OPP tables failed!!");
>> 	return;
>>
>> Option 1 is a bad idea as it leaves the system in an invalid state
>> Option 2 is the better idea as we dont have a opp_delete option(not
>> required usually).
> 
> I'm OK with either actually, as either is better than BUG_ON()
> instead of error checking.
> 
> With option 1, the system is not really in an invalid state, just and
> untested state.  What will happen is users of the OPP API will error
> codes when trying to get OPPs and they should fail gracefully as well.
except if L3/DSP reg fails, you allow MPU freqs configured by no DSP or 
L3 freq.. prefer the option 2 in that respect.. it is binary - you get 
all the configurations done right OR you dont.
> 
> Once again, this is about proper error checking, and robustness, not
> about causing a panic() when something (relatively) minor happens.
> 
>> All that code for something that will almost never happen?	
> 
> Yes.  That's what error checking is all about.
> 
> Kevin
> 
> P.S. I can't find the ref atm, but I mentioned not liking the BUG_ON
>      early in the review of the OPP layer, and it would need to be
>      cleaned up before upstream merge.
alrite alrite.. i am black and blue with the beatings ;). I am going 
with option 2 if there are no other objections.. will send out the patch 
after collating other comments.

-- 
Regards,
Nishanth Menon