[PATCHv2 1/1] OMAP3: PM: Functions to get freq from opp and vice-versa

[PATCHv2 1/1] OMAP3: PM: Functions to get freq from opp and vice-versa

[Sanjeev Premi]

There is no generic function to get OPP corresponding to frequency
and vice versa.

Function get_opp() - in resource34xx.c - is included only when
CONFIG_OMAP_PM_SRF is enabled. Moreover, it is always called with
an explicit addition of MAX_VDDn_OPP e.g.
  opp = get_opp(mpu_opps + MAX_VDD1_OPP, clk->rate);
  opp = get_opp(l3_opps + MAX_VDD2_OPP, clk->rate);

This is 'addition' is required as there is no encapsulation of
MIN and MAX VDDs associated to the table.

This patch fixes the issue by creating a 'table' object that
encapsulates the MIN and MAX values and the existing rate table.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/omap3-opp.h           |   42 +++------------------
 arch/arm/mach-omap2/pm.c                  |   49 +++++++++++++++++++++++++
 arch/arm/mach-omap2/pm34xx.c              |   57 +++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/mach/omap-pm.h |   22 +++++++++++
 4 files changed, 134 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index 3c9a582..52deb4a 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -21,42 +21,12 @@
 #define S83M    83000000
 #define S166M   166000000
 
-static struct omap_opp omap3_mpu_rate_table[] = {
-	{0, 0, 0},
-	/*OPP1*/
-	{S125M, VDD1_OPP1, 0x1E},
-	/*OPP2*/
-	{S250M, VDD1_OPP2, 0x26},
-	/*OPP3*/
-	{S500M, VDD1_OPP3, 0x30},
-	/*OPP4*/
-	{S550M, VDD1_OPP4, 0x36},
-	/*OPP5*/
-	{S600M, VDD1_OPP5, 0x3C},
-};
+extern struct omap_opp omap3_mpu_rate_table[];
+extern struct omap_opp omap3_dsp_rate_table[];
+extern struct omap_opp omap3_l3_rate_table[];
 
-static struct omap_opp omap3_l3_rate_table[] = {
-	{0, 0, 0},
-	/*OPP1*/
-	{0, VDD2_OPP1, 0x1E},
-	/*OPP2*/
-	{S83M, VDD2_OPP2, 0x24},
-	/*OPP3*/
-	{S166M, VDD2_OPP3, 0x2C},
-};
-
-static struct omap_opp omap3_dsp_rate_table[] = {
-	{0, 0, 0},
-	/*OPP1*/
-	{S90M, VDD1_OPP1, 0x1E},
-	/*OPP2*/
-	{S180M, VDD1_OPP2, 0x26},
-	/*OPP3*/
-	{S360M, VDD1_OPP3, 0x30},
-	/*OPP4*/
-	{S400M, VDD1_OPP4, 0x36},
-	/*OPP5*/
-	{S430M, VDD1_OPP5, 0x3C},
-};
+extern const struct omap_opp_table omap3_mpu_opp_table;
+extern const struct omap_opp_table omap3_dsp_opp_table;
+extern const struct omap_opp_table omap3_l3_opp_table;
 
 #endif
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index fec7d00..2bbe63f 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -33,6 +33,7 @@
 #include <mach/powerdomain.h>
 #include <mach/resource.h>
 #include <mach/omap34xx.h>
+#include <mach/omap-pm.h>
 
 #include "prm-regbits-34xx.h"
 #include "pm.h"
@@ -281,3 +282,51 @@ static int __init omap_pm_init(void)
 	return error;
 }
 late_initcall(omap_pm_init);
+
+/*
+ * Get frequency corresponding to an OPP
+ */
+int opp_to_freq(unsigned long* freq, const struct omap_opp_table* table, u8 opp)
+{
+        int i, found=0;
+
+        if (table && table->opps) {
+		for (i = table->min; i <= table->max; i++) {
+			if (table->opps[i].opp_id == opp) {
+				found = 1;
+				break;
+			}
+		}
+
+		if (found) {
+			*freq = table->opps[i].rate;
+			return 1;
+		}
+	}
+
+        return 0;
+}
+
+/*
+ * Get OPP corresponding to a frequency
+ */
+int freq_to_opp(u8* opp, const struct omap_opp_table* table, unsigned long freq)
+{
+        int i, found=0;
+
+        if (table && table->opps) {
+		for (i = table->min; i <= table->max; i++) {
+			if (table->opps[i].rate == freq) {
+				found = 1;
+				break;
+			}
+		}
+
+		if (found) {
+			*opp = table->opps[i].opp_id;
+			return 1;
+		}
+	}
+
+        return 0;
+}
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index a9eda25..dc966ae 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -50,6 +50,7 @@
 #include "prm.h"
 #include "pm.h"
 #include "sdrc.h"
+#include "omap3-opp.h"
 
 static int regset_save_on_suspend;
 
@@ -93,6 +94,62 @@ static struct prm_setup_vc prm_setup = {
 	.vdd1_off = 0x00,	/* 0.6v */
 };
 
+struct omap_opp omap3_mpu_rate_table[] = {
+	{0, 0, 0},
+	/*OPP1*/
+	{S125M, VDD1_OPP1, 0x1E},
+	/*OPP2*/
+	{S250M, VDD1_OPP2, 0x26},
+	/*OPP3*/
+	{S500M, VDD1_OPP3, 0x30},
+	/*OPP4*/
+	{S550M, VDD1_OPP4, 0x36},
+	/*OPP5*/
+	{S600M, VDD1_OPP5, 0x3C},
+};
+
+struct omap_opp omap3_l3_rate_table[] = {
+	{0, 0, 0},
+	/*OPP1*/
+	{0, VDD2_OPP1, 0x1E},
+	/*OPP2*/
+	{S83M, VDD2_OPP2, 0x24},
+	/*OPP3*/
+	{S166M, VDD2_OPP3, 0x2C},
+};
+
+struct omap_opp omap3_dsp_rate_table[] = {
+	{0, 0, 0},
+	/*OPP1*/
+	{S90M, VDD1_OPP1, 0x1E},
+	/*OPP2*/
+	{S180M, VDD1_OPP2, 0x26},
+	/*OPP3*/
+	{S360M, VDD1_OPP3, 0x30},
+	/*OPP4*/
+	{S400M, VDD1_OPP4, 0x36},
+	/*OPP5*/
+	{S430M, VDD1_OPP5, 0x3C},
+};
+
+const struct omap_opp_table omap3_mpu_opp_table = {
+	VDD1_OPP1,
+	VDD1_OPP5,
+	omap3_mpu_rate_table
+};
+
+const struct omap_opp_table omap3_dsp_opp_table = {
+        VDD1_OPP1,
+        VDD1_OPP5,
+        omap3_dsp_rate_table
+};
+
+const struct omap_opp_table omap3_l3_opp_table = {
+        VDD2_OPP1,
+        VDD2_OPP3,
+        omap3_l3_rate_table
+};
+
 static inline void omap3_per_save_context(void)
 {
 	omap3_gpio_save_context();
diff --git a/arch/arm/plat-omap/include/mach/omap-pm.h b/arch/arm/plat-omap/include/mach/omap-pm.h
index 583e540..51216cf 100644
--- a/arch/arm/plat-omap/include/mach/omap-pm.h
+++ b/arch/arm/plat-omap/include/mach/omap-pm.h
@@ -33,6 +33,20 @@ struct omap_opp {
 	u16 vsel;
 };
 
+/* struct omap_opp_table - View OPP table as an object
+ * @min: Minimum OPP id
+ * @max: Maximim OPP id
+ * @opps: Pointer to array defining the OPPs.
+ *
+ * An OPP table has varied length. Knowing minimum and maximum
+ * OPP ids allow easy traversal.
+ */
+struct omap_opp_table {
+	u8	min;
+	u8	max;
+	struct omap_opp* opps;
+};
+
 extern struct omap_opp *mpu_opps;
 extern struct omap_opp *dsp_opps;
 extern struct omap_opp *l3_opps;
@@ -48,6 +62,14 @@ extern struct omap_opp *l3_opps;
 #define OCP_TARGET_AGENT		1
 #define OCP_INITIATOR_AGENT		2
 
+int opp_to_freq(unsigned long* freq,
+		const struct omap_opp_table* table,
+		u8 opp);
+
+int freq_to_opp(u8* opp,
+		const struct omap_opp_table* table,
+		unsigned long freq);
+
 /**
  * omap_pm_if_early_init - OMAP PM init code called before clock fw init
  * @mpu_opp_table: array ptr to struct omap_opp for MPU
-- 
1.6.2.2

Question on OPP table handling

[Nishanth Menon]

Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> +struct omap_opp omap3_mpu_rate_table[] = {
> +	{0, 0, 0},
> +	/*OPP1*/
> +	{S125M, VDD1_OPP1, 0x1E},
> +	/*OPP2*/
> +	{S250M, VDD1_OPP2, 0x26},
> +	/*OPP3*/
> +	{S500M, VDD1_OPP3, 0x30},
> +	/*OPP4*/
> +	{S550M, VDD1_OPP4, 0x36},
> +	/*OPP5*/
> +	{S600M, VDD1_OPP5, 0x3C},
> +};
>   
For those involved,
if we wanted to convert omap3_mpu_table[] into *omap3_mpu_table so that
we dynamically initialize it based on cpu type - what would be the
recommendations?
Regards,
Nishanth Menon

RE: Question on OPP table handling

[Premi, Sanjeev]

> -----Original Message-----
> From: Nishanth Menon [mailto:menon.nishanth@gmail.com] 
> Sent: Saturday, October 03, 2009 8:35 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Question on OPP table handling
> 
> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> > +struct omap_opp omap3_mpu_rate_table[] = {
> > +	{0, 0, 0},
> > +	/*OPP1*/
> > +	{S125M, VDD1_OPP1, 0x1E},
> > +	/*OPP2*/
> > +	{S250M, VDD1_OPP2, 0x26},
> > +	/*OPP3*/
> > +	{S500M, VDD1_OPP3, 0x30},
> > +	/*OPP4*/
> > +	{S550M, VDD1_OPP4, 0x36},
> > +	/*OPP5*/
> > +	{S600M, VDD1_OPP5, 0x3C},
> > +};
> >   
> For those involved,
> if we wanted to convert omap3_mpu_table[] into 
> *omap3_mpu_table so that
> we dynamically initialize it based on cpu type - what would be the
> recommendations?

Nishanth,

Good idea! 

As a table, previous patch enables it (not as intent, but due to syntax):

  >  +/* struct omap_opp_table - View OPP table as an object
  > + * @min: Minimum OPP id
  > + * @max: Maximim OPP id
  > + * @opps: Pointer to array defining the OPPs.
  > + *
  > + * An OPP table has varied length. Knowing minimum and maximum
  > + * OPP ids allow easy traversal.
  > + */
  > +struct omap_opp_table {
  > +       u8      min;
  > +       u8      max;
  > +       struct omap_opp* opps;
  > +};

But now, I think it would be good to have an API that can fill an opp_table:

int add_opp_definition(u8 id, u32 freq, u16 vsel);

...and, if an array is preferred, length can be set as:
int set_opp_table_length (u8 max);

If I were to extend the discussion from other thread on toggling the valid OPPs
based on "enable_off_mode", these could be handy.

int set_opp_valid(bool flag);
bool is_opp_valid(void);

Best regards,
Sanjeev


> Regards,
> Nishanth Menon
> 
> 
> 

Re: Question on OPP table handling

[Kevin Hilman]

"Premi, Sanjeev" <premi@ti.com> writes:

>> -----Original Message-----
>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com] 
>> Sent: Saturday, October 03, 2009 8:35 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Question on OPP table handling
>> 
>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
>> > +struct omap_opp omap3_mpu_rate_table[] = {
>> > +	{0, 0, 0},
>> > +	/*OPP1*/
>> > +	{S125M, VDD1_OPP1, 0x1E},
>> > +	/*OPP2*/
>> > +	{S250M, VDD1_OPP2, 0x26},
>> > +	/*OPP3*/
>> > +	{S500M, VDD1_OPP3, 0x30},
>> > +	/*OPP4*/
>> > +	{S550M, VDD1_OPP4, 0x36},
>> > +	/*OPP5*/
>> > +	{S600M, VDD1_OPP5, 0x3C},
>> > +};
>> >   
>> For those involved,
>> if we wanted to convert omap3_mpu_table[] into 
>> *omap3_mpu_table so that
>> we dynamically initialize it based on cpu type - what would be the
>> recommendations?
>
> Nishanth,
>
> Good idea! 
>
> As a table, previous patch enables it (not as intent, but due to syntax):
>
>   >  +/* struct omap_opp_table - View OPP table as an object
>   > + * @min: Minimum OPP id
>   > + * @max: Maximim OPP id
>   > + * @opps: Pointer to array defining the OPPs.
>   > + *
>   > + * An OPP table has varied length. Knowing minimum and maximum
>   > + * OPP ids allow easy traversal.
>   > + */
>   > +struct omap_opp_table {
>   > +       u8      min;
>   > +       u8      max;
>   > +       struct omap_opp* opps;
>   > +};
>
> But now, I think it would be good to have an API that can fill an opp_table:
>
> int add_opp_definition(u8 id, u32 freq, u16 vsel);
>
> ...and, if an array is preferred, length can be set as:
> int set_opp_table_length (u8 max);

I'm all for dynamic OPP setting, but not as an array.  A list should
probably be used.

> If I were to extend the discussion from other thread on toggling the valid OPPs
> based on "enable_off_mode", these could be handy.
>
> int set_opp_valid(bool flag);
> bool is_opp_valid(void); 
>

Yes, we need a concept of a valid OPP, not just for OFF mode, but for
OSWR and possibly for a full constraint framework as well.

Kevin


>> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question on OPP table handling

[Nishanth Menon]

Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> "Premi, Sanjeev" <premi@ti.com> writes:
> 
>>> -----Original Message-----
>>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com] 
>>> Sent: Saturday, October 03, 2009 8:35 PM
>>> To: Premi, Sanjeev
>>> Cc: linux-omap@vger.kernel.org
>>> Subject: Question on OPP table handling
>>>
>>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
>>>> +struct omap_opp omap3_mpu_rate_table[] = {
>>>> +	{0, 0, 0},
>>>> +	/*OPP1*/
>>>> +	{S125M, VDD1_OPP1, 0x1E},
>>>> +	/*OPP2*/
>>>> +	{S250M, VDD1_OPP2, 0x26},
>>>> +	/*OPP3*/
>>>> +	{S500M, VDD1_OPP3, 0x30},
>>>> +	/*OPP4*/
>>>> +	{S550M, VDD1_OPP4, 0x36},
>>>> +	/*OPP5*/
>>>> +	{S600M, VDD1_OPP5, 0x3C},
>>>> +};
>>>>   
>>> For those involved,
>>> if we wanted to convert omap3_mpu_table[] into 
>>> *omap3_mpu_table so that
>>> we dynamically initialize it based on cpu type - what would be the
>>> recommendations?
>> Nishanth,
>>
>> Good idea! 
>>
>> As a table, previous patch enables it (not as intent, but due to syntax):
>>
>>   >  +/* struct omap_opp_table - View OPP table as an object
>>   > + * @min: Minimum OPP id
>>   > + * @max: Maximim OPP id
>>   > + * @opps: Pointer to array defining the OPPs.
>>   > + *
>>   > + * An OPP table has varied length. Knowing minimum and maximum
>>   > + * OPP ids allow easy traversal.
>>   > + */
>>   > +struct omap_opp_table {
>>   > +       u8      min;
>>   > +       u8      max;
>>   > +       struct omap_opp* opps;
>>   > +};
>>
>> But now, I think it would be good to have an API that can fill an opp_table:
>>
>> int add_opp_definition(u8 id, u32 freq, u16 vsel);
>>
>> ...and, if an array is preferred, length can be set as:
>> int set_opp_table_length (u8 max);
> 
> I'm all for dynamic OPP setting, but not as an array.  A list should
> probably be used.

Won't a list implementation cause more than necessary overhead? I agree 
that something like set_opp_table_length probably might be redundant in 
that case.


>> If I were to extend the discussion from other thread on toggling the valid OPPs
>> based on "enable_off_mode", these could be handy.
>>
>> int set_opp_valid(bool flag);
>> bool is_opp_valid(void); 
>>
> 
> Yes, we need a concept of a valid OPP, not just for OFF mode, but for
> OSWR and possibly for a full constraint framework as well.
Ack.
-- 
Regards,
Nishanth Menon

RE: Question on OPP table handling

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Nishanth Menon
> Sent: Monday, October 05, 2009 10:49 PM
> To: linux-omap@vger.kernel.org
> Cc: Premi, Sanjeev; Kevin H
> Subject: Re: Question on OPP table handling
> 
> Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> > "Premi, Sanjeev" <premi@ti.com> writes:
> >
> >>> -----Original Message-----
> >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> >>> Sent: Saturday, October 03, 2009 8:35 PM
> >>> To: Premi, Sanjeev
> >>> Cc: linux-omap@vger.kernel.org
> >>> Subject: Question on OPP table handling
> >>>
> >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> >>>> +struct omap_opp omap3_mpu_rate_table[] = {
> >>>> +	{0, 0, 0},
> >>>> +	/*OPP1*/
> >>>> +	{S125M, VDD1_OPP1, 0x1E},
> >>>> +	/*OPP2*/
> >>>> +	{S250M, VDD1_OPP2, 0x26},
> >>>> +	/*OPP3*/
> >>>> +	{S500M, VDD1_OPP3, 0x30},
> >>>> +	/*OPP4*/
> >>>> +	{S550M, VDD1_OPP4, 0x36},
> >>>> +	/*OPP5*/
> >>>> +	{S600M, VDD1_OPP5, 0x3C},
> >>>> +};
> >>>>
> >>> For those involved,
> >>> if we wanted to convert omap3_mpu_table[] into
> >>> *omap3_mpu_table so that
> >>> we dynamically initialize it based on cpu type - what would be the
> >>> recommendations?
> >> Nishanth,
> >>
> >> Good idea!
> >>
> >> As a table, previous patch enables it (not as intent, but due to
> syntax):
> >>
> >>   >  +/* struct omap_opp_table - View OPP table as an object
> >>   > + * @min: Minimum OPP id
> >>   > + * @max: Maximim OPP id
> >>   > + * @opps: Pointer to array defining the OPPs.
> >>   > + *
> >>   > + * An OPP table has varied length. Knowing minimum and maximum
> >>   > + * OPP ids allow easy traversal.
> >>   > + */
> >>   > +struct omap_opp_table {
> >>   > +       u8      min;
> >>   > +       u8      max;
> >>   > +       struct omap_opp* opps;
> >>   > +};
> >>
> >> But now, I think it would be good to have an API that can fill an
> opp_table:
> >>
> >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
> >>
> >> ...and, if an array is preferred, length can be set as:
> >> int set_opp_table_length (u8 max);
> >
> > I'm all for dynamic OPP setting, but not as an array.  A list should
> > probably be used.
> 
> Won't a list implementation cause more than necessary overhead? I agree
> that something like set_opp_table_length probably might be redundant in
> that case.
> 
> 
> >> If I were to extend the discussion from other thread on toggling the
> valid OPPs
> >> based on "enable_off_mode", these could be handy.
> >>
> >> int set_opp_valid(bool flag);
> >> bool is_opp_valid(void);
> >>
> >
> > Yes, we need a concept of a valid OPP, not just for OFF mode, but for
> > OSWR and possibly for a full constraint framework as well.
> Ack.
Even though above approach is possibly better a simple fix could be just adding a flag in the structure (OPP valid/invalid) and populating this flag run time using CPU type.


Regards,
Santosh

RE: Question on OPP table handling

[Cousson, Benoit]

Hi Kevin,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Nishanth Menon
> Sent: Monday, October 05, 2009 7:19 PM
> To: linux-omap@vger.kernel.org
> Cc: Premi, Sanjeev; Kevin H
> Subject: Re: Question on OPP table handling
> 
> Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> > "Premi, Sanjeev" <premi@ti.com> writes:
> >
> >>> -----Original Message-----
> >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> >>> Sent: Saturday, October 03, 2009 8:35 PM
> >>> To: Premi, Sanjeev
> >>> Cc: linux-omap@vger.kernel.org
> >>> Subject: Question on OPP table handling
> >>>
> >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> >>>> +struct omap_opp omap3_mpu_rate_table[] = {
> >>>> +	{0, 0, 0},
> >>>> +	/*OPP1*/
> >>>> +	{S125M, VDD1_OPP1, 0x1E},
> >>>> +	/*OPP2*/
> >>>> +	{S250M, VDD1_OPP2, 0x26},
> >>>> +	/*OPP3*/
> >>>> +	{S500M, VDD1_OPP3, 0x30},
> >>>> +	/*OPP4*/
> >>>> +	{S550M, VDD1_OPP4, 0x36},
> >>>> +	/*OPP5*/
> >>>> +	{S600M, VDD1_OPP5, 0x3C},
> >>>> +};
> >>>>
> >>> For those involved,
> >>> if we wanted to convert omap3_mpu_table[] into
> >>> *omap3_mpu_table so that
> >>> we dynamically initialize it based on cpu type - what would be the
> >>> recommendations?
> >> Nishanth,
> >>
> >> Good idea!
> >>
> >> As a table, previous patch enables it (not as intent, but due to
> syntax):
> >>
> >>   >  +/* struct omap_opp_table - View OPP table as an object
> >>   > + * @min: Minimum OPP id
> >>   > + * @max: Maximim OPP id
> >>   > + * @opps: Pointer to array defining the OPPs.
> >>   > + *
> >>   > + * An OPP table has varied length. Knowing minimum and maximum
> >>   > + * OPP ids allow easy traversal.
> >>   > + */
> >>   > +struct omap_opp_table {
> >>   > +       u8      min;
> >>   > +       u8      max;
> >>   > +       struct omap_opp* opps;
> >>   > +};
> >>
> >> But now, I think it would be good to have an API that can fill an
> opp_table:
> >>
> >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
> >>
> >> ...and, if an array is preferred, length can be set as:
> >> int set_opp_table_length (u8 max);
> >
> > I'm all for dynamic OPP setting, but not as an array.  A list should
> > probably be used.
> 
> Won't a list implementation cause more than necessary overhead? I agree
> that something like set_opp_table_length probably might be redundant in
> that case.

I'm aligned with Nishanth. I think a static table with the possibility to disable some entry is good enough to deal with most of the OPPs we have on OMAP3 and we will have to handle on OMAP4.

OPPs are defined during silicon characterization, and should not be changed dynamically (in theory).

Do you have something in mind that might justify a dynamic management?

Regards,
Benoit
 
> >> If I were to extend the discussion from other thread on toggling the
> valid OPPs
> >> based on "enable_off_mode", these could be handy.
> >>
> >> int set_opp_valid(bool flag);
> >> bool is_opp_valid(void);
> >>
> >
> > Yes, we need a concept of a valid OPP, not just for OFF mode, but for
> > OSWR and possibly for a full constraint framework as well.
> Ack.
> --
> 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

RE: Question on OPP table handling

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Tuesday, October 06, 2009 1:36 PM
> To: Menon, Nishanth; linux-omap@vger.kernel.org
> Cc: Premi, Sanjeev; Kevin H
> Subject: RE: Question on OPP table handling
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Nishanth Menon
> > Sent: Monday, October 05, 2009 10:49 PM
> > To: linux-omap@vger.kernel.org
> > Cc: Premi, Sanjeev; Kevin H
> > Subject: Re: Question on OPP table handling
> > 
> > Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> > > "Premi, Sanjeev" <premi@ti.com> writes:
> > >
> > >>> -----Original Message-----
> > >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> > >>> Sent: Saturday, October 03, 2009 8:35 PM
> > >>> To: Premi, Sanjeev
> > >>> Cc: linux-omap@vger.kernel.org
> > >>> Subject: Question on OPP table handling
> > >>>
> > >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> > >>>> +struct omap_opp omap3_mpu_rate_table[] = {
> > >>>> +	{0, 0, 0},
> > >>>> +	/*OPP1*/
> > >>>> +	{S125M, VDD1_OPP1, 0x1E},
> > >>>> +	/*OPP2*/
> > >>>> +	{S250M, VDD1_OPP2, 0x26},
> > >>>> +	/*OPP3*/
> > >>>> +	{S500M, VDD1_OPP3, 0x30},
> > >>>> +	/*OPP4*/
> > >>>> +	{S550M, VDD1_OPP4, 0x36},
> > >>>> +	/*OPP5*/
> > >>>> +	{S600M, VDD1_OPP5, 0x3C},
> > >>>> +};
> > >>>>
> > >>> For those involved,
> > >>> if we wanted to convert omap3_mpu_table[] into
> > >>> *omap3_mpu_table so that
> > >>> we dynamically initialize it based on cpu type - what 
> would be the
> > >>> recommendations?
> > >> Nishanth,
> > >>
> > >> Good idea!
> > >>
> > >> As a table, previous patch enables it (not as intent, but due to
> > syntax):
> > >>
> > >>   >  +/* struct omap_opp_table - View OPP table as an object
> > >>   > + * @min: Minimum OPP id
> > >>   > + * @max: Maximim OPP id
> > >>   > + * @opps: Pointer to array defining the OPPs.
> > >>   > + *
> > >>   > + * An OPP table has varied length. Knowing minimum 
> and maximum
> > >>   > + * OPP ids allow easy traversal.
> > >>   > + */
> > >>   > +struct omap_opp_table {
> > >>   > +       u8      min;
> > >>   > +       u8      max;
> > >>   > +       struct omap_opp* opps;
> > >>   > +};
> > >>
> > >> But now, I think it would be good to have an API that can fill an
> > opp_table:
> > >>
> > >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
> > >>
> > >> ...and, if an array is preferred, length can be set as:
> > >> int set_opp_table_length (u8 max);
> > >
> > > I'm all for dynamic OPP setting, but not as an array.  A 
> list should
> > > probably be used.
> > 
> > Won't a list implementation cause more than necessary 
> overhead? I agree
> > that something like set_opp_table_length probably might be 
> redundant in
> > that case.
> > 
> > 
> > >> If I were to extend the discussion from other thread on 
> toggling the
> > valid OPPs
> > >> based on "enable_off_mode", these could be handy.
> > >>
> > >> int set_opp_valid(bool flag);
> > >> bool is_opp_valid(void);
> > >>
> > >
> > > Yes, we need a concept of a valid OPP, not just for OFF 
> mode, but for
> > > OSWR and possibly for a full constraint framework as well.
> > Ack.
> Even though above approach is possibly better a simple fix 
> could be just adding a flag in the structure (OPP 
> valid/invalid) and populating this flag run time using CPU type.
> 

[sp] This is exactly in the original patch.

Best regards,
Sanjeev

> 
> Regards,
> Santosh
> 

RE: Question on OPP table handling

[Premi, Sanjeev]

> -----Original Message-----
> From: Cousson, Benoit 
> Sent: Tuesday, October 06, 2009 2:12 PM
> To: Kevin H; linux-omap@vger.kernel.org
> Cc: Premi, Sanjeev; Menon, Nishanth
> Subject: RE: Question on OPP table handling
> 
> Hi Kevin,
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Nishanth Menon
> > Sent: Monday, October 05, 2009 7:19 PM
> > To: linux-omap@vger.kernel.org
> > Cc: Premi, Sanjeev; Kevin H
> > Subject: Re: Question on OPP table handling
> > 
> > Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> > > "Premi, Sanjeev" <premi@ti.com> writes:
> > >
> > >>> -----Original Message-----
> > >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> > >>> Sent: Saturday, October 03, 2009 8:35 PM
> > >>> To: Premi, Sanjeev
> > >>> Cc: linux-omap@vger.kernel.org
> > >>> Subject: Question on OPP table handling
> > >>>
> > >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> > >>>> +struct omap_opp omap3_mpu_rate_table[] = {
> > >>>> +	{0, 0, 0},
> > >>>> +	/*OPP1*/
> > >>>> +	{S125M, VDD1_OPP1, 0x1E},
> > >>>> +	/*OPP2*/
> > >>>> +	{S250M, VDD1_OPP2, 0x26},
> > >>>> +	/*OPP3*/
> > >>>> +	{S500M, VDD1_OPP3, 0x30},
> > >>>> +	/*OPP4*/
> > >>>> +	{S550M, VDD1_OPP4, 0x36},
> > >>>> +	/*OPP5*/
> > >>>> +	{S600M, VDD1_OPP5, 0x3C},
> > >>>> +};
> > >>>>
> > >>> For those involved,
> > >>> if we wanted to convert omap3_mpu_table[] into
> > >>> *omap3_mpu_table so that
> > >>> we dynamically initialize it based on cpu type - what 
> would be the
> > >>> recommendations?
> > >> Nishanth,
> > >>
> > >> Good idea!
> > >>
> > >> As a table, previous patch enables it (not as intent, but due to
> > syntax):
> > >>
> > >>   >  +/* struct omap_opp_table - View OPP table as an object
> > >>   > + * @min: Minimum OPP id
> > >>   > + * @max: Maximim OPP id
> > >>   > + * @opps: Pointer to array defining the OPPs.
> > >>   > + *
> > >>   > + * An OPP table has varied length. Knowing minimum 
> and maximum
> > >>   > + * OPP ids allow easy traversal.
> > >>   > + */
> > >>   > +struct omap_opp_table {
> > >>   > +       u8      min;
> > >>   > +       u8      max;
> > >>   > +       struct omap_opp* opps;
> > >>   > +};
> > >>
> > >> But now, I think it would be good to have an API that can fill an
> > opp_table:
> > >>
> > >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
> > >>
> > >> ...and, if an array is preferred, length can be set as:
> > >> int set_opp_table_length (u8 max);
> > >
> > > I'm all for dynamic OPP setting, but not as an array.  A 
> list should
> > > probably be used.
> > 
> > Won't a list implementation cause more than necessary 
> overhead? I agree
> > that something like set_opp_table_length probably might be 
> redundant in
> > that case.
> 
> I'm aligned with Nishanth. I think a static table with the 
> possibility to disable some entry is good enough to deal with 
> most of the OPPs we have on OMAP3 and we will have to handle on OMAP4.
> 
> OPPs are defined during silicon characterization, and should 
> not be changed dynamically (in theory).

[sp] The intent of 'dynamic' is not with respect to changing the
     OPPs but manitaining OPPs in an array or a list.

     This is to take care of possibility that an OPP is not
     applicable for specific devices. E.g. OPP5 was earlier
     considered 'overdrive'; and the code had a small 'hack'
     to prevent this OPP being used during cpufreq.

     Marking the OPP 'disabled/invalid' in the table would have
     been a better solution.

     In a 'list' implementation, the node corresponding to such
     OPPs can be removed from the 'active' list.

Best regards,
Sanjeev

> 
> Do you have something in mind that might justify a dynamic management?
> 


> Regards,
> Benoit
>  
> > >> If I were to extend the discussion from other thread on 
> toggling the
> > valid OPPs
> > >> based on "enable_off_mode", these could be handy.
> > >>
> > >> int set_opp_valid(bool flag);
> > >> bool is_opp_valid(void);
> > >>
> > >
> > > Yes, we need a concept of a valid OPP, not just for OFF 
> mode, but for
> > > OSWR and possibly for a full constraint framework as well.
> > Ack.
> > --
> > 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
> 
> 

RE: Question on OPP table handling

[Shilimkar, Santosh]

> -----Original Message-----
> From: Premi, Sanjeev
> Sent: Tuesday, October 06, 2009 2:14 PM
> To: Shilimkar, Santosh; Menon, Nishanth; linux-omap@vger.kernel.org
> Cc: Kevin H
> Subject: RE: Question on OPP table handling
> 
> 
> > -----Original Message-----
> > From: Shilimkar, Santosh
> > Sent: Tuesday, October 06, 2009 1:36 PM
> > To: Menon, Nishanth; linux-omap@vger.kernel.org
> > Cc: Premi, Sanjeev; Kevin H
> > Subject: RE: Question on OPP table handling
> >
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > > owner@vger.kernel.org] On Behalf Of Nishanth Menon
> > > Sent: Monday, October 05, 2009 10:49 PM
> > > To: linux-omap@vger.kernel.org
> > > Cc: Premi, Sanjeev; Kevin H
> > > Subject: Re: Question on OPP table handling
> > >
> > > Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
> > > > "Premi, Sanjeev" <premi@ti.com> writes:
> > > >
> > > >>> -----Original Message-----
> > > >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
> > > >>> Sent: Saturday, October 03, 2009 8:35 PM
> > > >>> To: Premi, Sanjeev
> > > >>> Cc: linux-omap@vger.kernel.org
> > > >>> Subject: Question on OPP table handling
> > > >>>
> > > >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
> > > >>>> +struct omap_opp omap3_mpu_rate_table[] = {
> > > >>>> +	{0, 0, 0},
> > > >>>> +	/*OPP1*/
> > > >>>> +	{S125M, VDD1_OPP1, 0x1E},
> > > >>>> +	/*OPP2*/
> > > >>>> +	{S250M, VDD1_OPP2, 0x26},
> > > >>>> +	/*OPP3*/
> > > >>>> +	{S500M, VDD1_OPP3, 0x30},
> > > >>>> +	/*OPP4*/
> > > >>>> +	{S550M, VDD1_OPP4, 0x36},
> > > >>>> +	/*OPP5*/
> > > >>>> +	{S600M, VDD1_OPP5, 0x3C},
> > > >>>> +};
> > > >>>>
> > > >>> For those involved,
> > > >>> if we wanted to convert omap3_mpu_table[] into
> > > >>> *omap3_mpu_table so that
> > > >>> we dynamically initialize it based on cpu type - what
> > would be the
> > > >>> recommendations?
> > > >> Nishanth,
> > > >>
> > > >> Good idea!
> > > >>
> > > >> As a table, previous patch enables it (not as intent, but due to
> > > syntax):
> > > >>
> > > >>   >  +/* struct omap_opp_table - View OPP table as an object
> > > >>   > + * @min: Minimum OPP id
> > > >>   > + * @max: Maximim OPP id
> > > >>   > + * @opps: Pointer to array defining the OPPs.
> > > >>   > + *
> > > >>   > + * An OPP table has varied length. Knowing minimum
> > and maximum
> > > >>   > + * OPP ids allow easy traversal.
> > > >>   > + */
> > > >>   > +struct omap_opp_table {
> > > >>   > +       u8      min;
> > > >>   > +       u8      max;
> > > >>   > +       struct omap_opp* opps;
> > > >>   > +};
> > > >>
> > > >> But now, I think it would be good to have an API that can fill an
> > > opp_table:
> > > >>
> > > >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
> > > >>
> > > >> ...and, if an array is preferred, length can be set as:
> > > >> int set_opp_table_length (u8 max);
> > > >
> > > > I'm all for dynamic OPP setting, but not as an array.  A
> > list should
> > > > probably be used.
> > >
> > > Won't a list implementation cause more than necessary
> > overhead? I agree
> > > that something like set_opp_table_length probably might be
> > redundant in
> > > that case.
> > >
> > >
> > > >> If I were to extend the discussion from other thread on
> > toggling the
> > > valid OPPs
> > > >> based on "enable_off_mode", these could be handy.
> > > >>
> > > >> int set_opp_valid(bool flag);
> > > >> bool is_opp_valid(void);
> > > >>
> > > >
> > > > Yes, we need a concept of a valid OPP, not just for OFF
> > mode, but for
> > > > OSWR and possibly for a full constraint framework as well.
> > > Ack.
> > Even though above approach is possibly better a simple fix
> > could be just adding a flag in the structure (OPP
> > valid/invalid) and populating this flag run time using CPU type.
> >
> 
> [sp] This is exactly in the original patch.
OK

Regards,
Santosh

Re: Question on OPP table handling

[Nishanth Menon]

Premi, Sanjeev had written, on 10/06/2009 03:52 AM, the following:
>>>>>>> +	{0, 0, 0},
>>>>>>> +	/*OPP1*/
>>>>>>> +	{S125M, VDD1_OPP1, 0x1E},
>>>>>>> +	/*OPP2*/
>>>>>>> +	{S250M, VDD1_OPP2, 0x26},
>>>>>>> +	/*OPP3*/
>>>>>>> +	{S500M, VDD1_OPP3, 0x30},
>>>>>>> +	/*OPP4*/
>>>>>>> +	{S550M, VDD1_OPP4, 0x36},
>>>>>>> +	/*OPP5*/
>>>>>>> +	{S600M, VDD1_OPP5, 0x3C},
>>>>>>> +};
>>>>>>>
>>>>>> For those involved,
>>>>>> if we wanted to convert omap3_mpu_table[] into
>>>>>> *omap3_mpu_table so that
>>>>>> we dynamically initialize it based on cpu type - what 
>> would be the
>>>>>> recommendations?
>>>>> Nishanth,
>>>>>
>>>>> Good idea!
>>>>>
>>>>> As a table, previous patch enables it (not as intent, but due to
>>> syntax):
>>>>>   >  +/* struct omap_opp_table - View OPP table as an object
>>>>>   > + * @min: Minimum OPP id
>>>>>   > + * @max: Maximim OPP id
>>>>>   > + * @opps: Pointer to array defining the OPPs.
>>>>>   > + *
>>>>>   > + * An OPP table has varied length. Knowing minimum 
>> and maximum
>>>>>   > + * OPP ids allow easy traversal.
>>>>>   > + */
>>>>>   > +struct omap_opp_table {
>>>>>   > +       u8      min;
>>>>>   > +       u8      max;
>>>>>   > +       struct omap_opp* opps;
>>>>>   > +};
>>>>>
>>>>> But now, I think it would be good to have an API that can fill an
>>> opp_table:
>>>>> int add_opp_definition(u8 id, u32 freq, u16 vsel);
>>>>>
>>>>> ...and, if an array is preferred, length can be set as:
>>>>> int set_opp_table_length (u8 max);
>>>> I'm all for dynamic OPP setting, but not as an array.  A 
>> list should
>>>> probably be used.
>>> Won't a list implementation cause more than necessary 
>> overhead? I agree
>>> that something like set_opp_table_length probably might be 
>> redundant in
>>> that case.
>> I'm aligned with Nishanth. I think a static table with the 
>> possibility to disable some entry is good enough to deal with 
>> most of the OPPs we have on OMAP3 and we will have to handle on OMAP4.
>>
>> OPPs are defined during silicon characterization, and should 
>> not be changed dynamically (in theory).
> 
> [sp] The intent of 'dynamic' is not with respect to changing the
>      OPPs but manitaining OPPs in an array or a list.
> 
>      This is to take care of possibility that an OPP is not
>      applicable for specific devices. E.g. OPP5 was earlier
>      considered 'overdrive'; and the code had a small 'hack'
>      to prevent this OPP being used during cpufreq.
> 
>      Marking the OPP 'disabled/invalid' in the table would have
>      been a better solution.
> 
>      In a 'list' implementation, the node corresponding to such
>      OPPs can be removed from the 'active' list.

Couple of opinions on why a list with disabled/invalid marker might not 
make sense as a grand unified OPP table:
a) it is no better than a list implementation
b) it is a waste of memory.
c) search Algo overheads

Recommendation a.k.a hybrid approach:
* Each silicon has it's own static OPP table
* Each table has a invalid marker which is disabled for silicon variants 
which dont need specific OPPs
* OPPs table be stored in a hash table a.k.a how do we optimize search 
for OPP params?

-- 
Regards,
Nishanth Menon

RE: Question on OPP table handling

[Dasgupta, Romit]

>
>Couple of opinions on why a list with disabled/invalid marker might not
>make sense as a grand unified OPP table:
>a) it is no better than a list implementation
>b) it is a waste of memory.
[Romit] Put all the OPP tables for different OPPs in __initdata. Copy the runtime detected CPU's OPP table in memory. Others get discarded!

Re: Question on OPP table handling

[Nishanth Menon]

Dasgupta, Romit had written, on 10/06/2009 07:00 AM, the following:
>> Couple of opinions on why a list with disabled/invalid marker might not
>> make sense as a grand unified OPP table:
>> a) it is no better than a list implementation
>> b) it is a waste of memory.
> [Romit] Put all the OPP tables for different OPPs in __initdata. Copy the runtime detected CPU's OPP table in memory. Others get discarded!
> 
I like this approach.. takers?

-- 
Regards,
Nishanth Menon

RE: Question on OPP table handling

[Cousson, Benoit]

> -----Original Message-----
> From: Menon, Nishanth
> Sent: Tuesday, October 06, 2009 2:04 PM
> To: Dasgupta, Romit
> Cc: Premi, Sanjeev; Cousson, Benoit; Kevin H; linux-omap@vger.kernel.org
> Subject: Re: Question on OPP table handling
> 
> Dasgupta, Romit had written, on 10/06/2009 07:00 AM, the following:
> >> Couple of opinions on why a list with disabled/invalid marker might not
> >> make sense as a grand unified OPP table:
> >> a) it is no better than a list implementation
> >> b) it is a waste of memory.
> > [Romit] Put all the OPP tables for different OPPs in __initdata. Copy
> the runtime detected CPU's OPP table in memory. Others get discarded!
> >
> I like this approach.. takers?
> 

I think it is not enough. Some OPPs will be selected based on runtime CPU detection, but some OPPs might be disabled dynamically for some usecase depending of external parameters.

Regards,
Benoit

RE: Question on OPP table handling

[Dasgupta, Romit]

>> Dasgupta, Romit had written, on 10/06/2009 07:00 AM, the following:
>> >> Couple of opinions on why a list with disabled/invalid marker might not
>> >> make sense as a grand unified OPP table:
>> >> a) it is no better than a list implementation
>> >> b) it is a waste of memory.
>> > [Romit] Put all the OPP tables for different OPPs in __initdata. Copy
>> the runtime detected CPU's OPP table in memory. Others get discarded!
>> >
>> I like this approach.. takers?
>>
>
>I think it is not enough. Some OPPs will be selected based on runtime CPU
>detection, but some OPPs might be disabled dynamically for some usecase
>depending of external parameters.

[Romit] For a given SoC and type you can have only one OPP table. Isn't that true? 

RE: Question on OPP table handling

[Dasgupta, Romit]

Typo I meant Put all the OPP tables for different CPUs (instead ofOPPs) in __initdata. Copy the run time detected CPU's OPP table in memory. Others get discarded!"

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Tuesday, October 06, 2009 5:34 PM
>To: Dasgupta, Romit
>Cc: Premi, Sanjeev; Cousson, Benoit; Kevin H; linux-omap@vger.kernel.org
>Subject: Re: Question on OPP table handling
>
>Dasgupta, Romit had written, on 10/06/2009 07:00 AM, the following:
>>> Couple of opinions on why a list with disabled/invalid marker might not
>>> make sense as a grand unified OPP table:
>>> a) it is no better than a list implementation
>>> b) it is a waste of memory.
>> [Romit] Put all the OPP tables for different OPPs in __initdata. Copy the runtime
>detected CPU's OPP table in memory. Others get discarded!
>>
>I like this approach.. takers?
>
>--
>Regards,
>Nishanth Menon

RE: Question on OPP table handling

[Shilimkar, Santosh]

 -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Dasgupta, Romit
> Sent: Tuesday, October 06, 2009 5:51 PM
> To: Menon, Nishanth
> Cc: Premi, Sanjeev; Cousson, Benoit; Kevin H; linux-omap@vger.kernel.org
> Subject: RE: Question on OPP table handling
> 
> Typo I meant Put all the OPP tables for different CPUs (instead ofOPPs) in
> __initdata. Copy the run time detected CPU's OPP table in memory. Others
> get discarded!"
This is a good idea. Or if we want to save the _Copy_ effort, then make all these opp table structures as 'const' so that it get moved to *.RO section. In that case all data is still available.

RE: Question on OPP table handling

[Cousson, Benoit]

> -----Original Message-----
> From: Dasgupta, Romit
> Sent: Tuesday, October 06, 2009 2:14 PM
> To: Cousson, Benoit; Menon, Nishanth
> Cc: Premi, Sanjeev; Kevin H; linux-omap@vger.kernel.org
> Subject: RE: Question on OPP table handling
> 
> >> Dasgupta, Romit had written, on 10/06/2009 07:00 AM, the following:
> >> >> Couple of opinions on why a list with disabled/invalid marker might
> not
> >> >> make sense as a grand unified OPP table:
> >> >> a) it is no better than a list implementation
> >> >> b) it is a waste of memory.
> >> > [Romit] Put all the OPP tables for different OPPs in __initdata. Copy
> >> the runtime detected CPU's OPP table in memory. Others get discarded!
> >> >
> >> I like this approach.. takers?
> >>
> >
> >I think it is not enough. Some OPPs will be selected based on runtime CPU
> >detection, but some OPPs might be disabled dynamically for some usecase
> >depending of external parameters.
> 
> [Romit] For a given SoC and type you can have only one OPP table. Isn't
> that true?

Yes, it is true but you might have to disable dynamically some OPP (like OPP5 and OPP4) for thermal management reason. The way the resource is handled today, you cannot force the reduction of the frequency in case of thermal issue.
I agree that there might be more elegant way to deal with that, but we can take advantage of disabling dynamically any OPP to do it.

Re: Question on OPP table handling

[Kevin Hilman]

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

> Hi Kevin,
>
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Nishanth Menon
>> Sent: Monday, October 05, 2009 7:19 PM
>> To: linux-omap@vger.kernel.org
>> Cc: Premi, Sanjeev; Kevin H
>> Subject: Re: Question on OPP table handling
>> 
>> Kevin Hilman had written, on 10/05/2009 11:56 AM, the following:
>> > "Premi, Sanjeev" <premi@ti.com> writes:
>> >
>> >>> -----Original Message-----
>> >>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
>> >>> Sent: Saturday, October 03, 2009 8:35 PM
>> >>> To: Premi, Sanjeev
>> >>> Cc: linux-omap@vger.kernel.org
>> >>> Subject: Question on OPP table handling
>> >>>
>> >>> Sanjeev Premi said the following on 10/01/2009 01:03 PM:
>> >>>> +struct omap_opp omap3_mpu_rate_table[] = {
>> >>>> +	{0, 0, 0},
>> >>>> +	/*OPP1*/
>> >>>> +	{S125M, VDD1_OPP1, 0x1E},
>> >>>> +	/*OPP2*/
>> >>>> +	{S250M, VDD1_OPP2, 0x26},
>> >>>> +	/*OPP3*/
>> >>>> +	{S500M, VDD1_OPP3, 0x30},
>> >>>> +	/*OPP4*/
>> >>>> +	{S550M, VDD1_OPP4, 0x36},
>> >>>> +	/*OPP5*/
>> >>>> +	{S600M, VDD1_OPP5, 0x3C},
>> >>>> +};
>> >>>>
>> >>> For those involved,
>> >>> if we wanted to convert omap3_mpu_table[] into
>> >>> *omap3_mpu_table so that
>> >>> we dynamically initialize it based on cpu type - what would be the
>> >>> recommendations?
>> >> Nishanth,
>> >>
>> >> Good idea!
>> >>
>> >> As a table, previous patch enables it (not as intent, but due to
>> syntax):
>> >>
>> >>   >  +/* struct omap_opp_table - View OPP table as an object
>> >>   > + * @min: Minimum OPP id
>> >>   > + * @max: Maximim OPP id
>> >>   > + * @opps: Pointer to array defining the OPPs.
>> >>   > + *
>> >>   > + * An OPP table has varied length. Knowing minimum and maximum
>> >>   > + * OPP ids allow easy traversal.
>> >>   > + */
>> >>   > +struct omap_opp_table {
>> >>   > +       u8      min;
>> >>   > +       u8      max;
>> >>   > +       struct omap_opp* opps;
>> >>   > +};
>> >>
>> >> But now, I think it would be good to have an API that can fill an
>> opp_table:
>> >>
>> >> int add_opp_definition(u8 id, u32 freq, u16 vsel);
>> >>
>> >> ...and, if an array is preferred, length can be set as:
>> >> int set_opp_table_length (u8 max);
>> >
>> > I'm all for dynamic OPP setting, but not as an array.  A list should
>> > probably be used.
>> 
>> Won't a list implementation cause more than necessary overhead? I agree
>> that something like set_opp_table_length probably might be redundant in
>> that case.
>
> I'm aligned with Nishanth. I think a static table with the possibility to disable some entry is good enough to deal with most of the OPPs we have on OMAP3 and we will have to handle on OMAP4.
>
> OPPs are defined during silicon characterization, and should not be changed dynamically (in theory).
>
> Do you have something in mind that might justify a dynamic management?
>

Ultimately, I don't really care what the data structure used is.

My primary concern is that at run-time (not just boot time) OPPs can
be disabled/invalidated so that they are not available to DVFS.

There are many reasons for this

- chip bugs
- CPUfreq policies can disable/enable certain OPPs
- speed-rated silicon
- thermal management
- etc.

Kevin

Re: Question on OPP table handling

[Nishanth Menon]

Sanjeev, All,
Cousson, Benoit had written, on 10/06/2009 07:52 AM, the following:
> Yes, it is true but you might have to disable dynamically some OPP (like OPP5 and OPP4) for thermal management reason. The way the resource is handled today, you cannot force the reduction of the frequency in case of thermal issue.
> I agree that there might be more elegant way to deal with that, but we can take advantage of disabling dynamically any OPP to do it.
I would like to introduce OPP tables for 3630, but I will be dependent 
on  http://marc.info/?l=linux-omap&m=125442020209267&w=2

Let me know how I can introduce the following into l-o + l-o pm. ( I 
suppose l-o first…):
http://dev.omapzoom.org/?p=integration/kernel-omap3.git;a=blob;f=arch/arm/mach-omap2/omap3-opp.h;h=ac3ae6f219b37760de00e8e469ae3911d02f0304;hb=refs/heads/sync_wake-up3630#l74

Not sure of the status and plan on this. Could we sync on this?

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

Re: [PATCHv2 1/1] OMAP3: PM: Functions to get freq from opp and vice-versa

[Kevin Hilman]

Sanjeev Premi <premi@ti.com> writes:

> There is no generic function to get OPP corresponding to frequency
> and vice versa.
>
> Function get_opp() - in resource34xx.c - is included only when
> CONFIG_OMAP_PM_SRF is enabled. Moreover, it is always called with
> an explicit addition of MAX_VDDn_OPP e.g.
>   opp = get_opp(mpu_opps + MAX_VDD1_OPP, clk->rate);
>   opp = get_opp(l3_opps + MAX_VDD2_OPP, clk->rate);
>
> This is 'addition' is required as there is no encapsulation of
> MIN and MAX VDDs associated to the table.
>
> This patch fixes the issue by creating a 'table' object that
> encapsulates the MIN and MAX values and the existing rate table.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>

Thanks, pulling into PM branch.

Kevin

> ---
>  arch/arm/mach-omap2/omap3-opp.h           |   42 +++------------------
>  arch/arm/mach-omap2/pm.c                  |   49 +++++++++++++++++++++++++
>  arch/arm/mach-omap2/pm34xx.c              |   57 +++++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/mach/omap-pm.h |   22 +++++++++++
>  4 files changed, 134 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
> index 3c9a582..52deb4a 100644
> --- a/arch/arm/mach-omap2/omap3-opp.h
> +++ b/arch/arm/mach-omap2/omap3-opp.h
> @@ -21,42 +21,12 @@
>  #define S83M    83000000
>  #define S166M   166000000
>  
> -static struct omap_opp omap3_mpu_rate_table[] = {
> -	{0, 0, 0},
> -	/*OPP1*/
> -	{S125M, VDD1_OPP1, 0x1E},
> -	/*OPP2*/
> -	{S250M, VDD1_OPP2, 0x26},
> -	/*OPP3*/
> -	{S500M, VDD1_OPP3, 0x30},
> -	/*OPP4*/
> -	{S550M, VDD1_OPP4, 0x36},
> -	/*OPP5*/
> -	{S600M, VDD1_OPP5, 0x3C},
> -};
> +extern struct omap_opp omap3_mpu_rate_table[];
> +extern struct omap_opp omap3_dsp_rate_table[];
> +extern struct omap_opp omap3_l3_rate_table[];
>  
> -static struct omap_opp omap3_l3_rate_table[] = {
> -	{0, 0, 0},
> -	/*OPP1*/
> -	{0, VDD2_OPP1, 0x1E},
> -	/*OPP2*/
> -	{S83M, VDD2_OPP2, 0x24},
> -	/*OPP3*/
> -	{S166M, VDD2_OPP3, 0x2C},
> -};
> -
> -static struct omap_opp omap3_dsp_rate_table[] = {
> -	{0, 0, 0},
> -	/*OPP1*/
> -	{S90M, VDD1_OPP1, 0x1E},
> -	/*OPP2*/
> -	{S180M, VDD1_OPP2, 0x26},
> -	/*OPP3*/
> -	{S360M, VDD1_OPP3, 0x30},
> -	/*OPP4*/
> -	{S400M, VDD1_OPP4, 0x36},
> -	/*OPP5*/
> -	{S430M, VDD1_OPP5, 0x3C},
> -};
> +extern const struct omap_opp_table omap3_mpu_opp_table;
> +extern const struct omap_opp_table omap3_dsp_opp_table;
> +extern const struct omap_opp_table omap3_l3_opp_table;
>  
>  #endif
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index fec7d00..2bbe63f 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -33,6 +33,7 @@
>  #include <mach/powerdomain.h>
>  #include <mach/resource.h>
>  #include <mach/omap34xx.h>
> +#include <mach/omap-pm.h>
>  
>  #include "prm-regbits-34xx.h"
>  #include "pm.h"
> @@ -281,3 +282,51 @@ static int __init omap_pm_init(void)
>  	return error;
>  }
>  late_initcall(omap_pm_init);
> +
> +/*
> + * Get frequency corresponding to an OPP
> + */
> +int opp_to_freq(unsigned long* freq, const struct omap_opp_table* table, u8 opp)
> +{
> +        int i, found=0;
> +
> +        if (table && table->opps) {
> +		for (i = table->min; i <= table->max; i++) {
> +			if (table->opps[i].opp_id == opp) {
> +				found = 1;
> +				break;
> +			}
> +		}
> +
> +		if (found) {
> +			*freq = table->opps[i].rate;
> +			return 1;
> +		}
> +	}
> +
> +        return 0;
> +}
> +
> +/*
> + * Get OPP corresponding to a frequency
> + */
> +int freq_to_opp(u8* opp, const struct omap_opp_table* table, unsigned long freq)
> +{
> +        int i, found=0;
> +
> +        if (table && table->opps) {
> +		for (i = table->min; i <= table->max; i++) {
> +			if (table->opps[i].rate == freq) {
> +				found = 1;
> +				break;
> +			}
> +		}
> +
> +		if (found) {
> +			*opp = table->opps[i].opp_id;
> +			return 1;
> +		}
> +	}
> +
> +        return 0;
> +}
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index a9eda25..dc966ae 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -50,6 +50,7 @@
>  #include "prm.h"
>  #include "pm.h"
>  #include "sdrc.h"
> +#include "omap3-opp.h"
>  
>  static int regset_save_on_suspend;
>  
> @@ -93,6 +94,62 @@ static struct prm_setup_vc prm_setup = {
>  	.vdd1_off = 0x00,	/* 0.6v */
>  };
>  
> +struct omap_opp omap3_mpu_rate_table[] = {
> +	{0, 0, 0},
> +	/*OPP1*/
> +	{S125M, VDD1_OPP1, 0x1E},
> +	/*OPP2*/
> +	{S250M, VDD1_OPP2, 0x26},
> +	/*OPP3*/
> +	{S500M, VDD1_OPP3, 0x30},
> +	/*OPP4*/
> +	{S550M, VDD1_OPP4, 0x36},
> +	/*OPP5*/
> +	{S600M, VDD1_OPP5, 0x3C},
> +};
> +
> +struct omap_opp omap3_l3_rate_table[] = {
> +	{0, 0, 0},
> +	/*OPP1*/
> +	{0, VDD2_OPP1, 0x1E},
> +	/*OPP2*/
> +	{S83M, VDD2_OPP2, 0x24},
> +	/*OPP3*/
> +	{S166M, VDD2_OPP3, 0x2C},
> +};
> +
> +struct omap_opp omap3_dsp_rate_table[] = {
> +	{0, 0, 0},
> +	/*OPP1*/
> +	{S90M, VDD1_OPP1, 0x1E},
> +	/*OPP2*/
> +	{S180M, VDD1_OPP2, 0x26},
> +	/*OPP3*/
> +	{S360M, VDD1_OPP3, 0x30},
> +	/*OPP4*/
> +	{S400M, VDD1_OPP4, 0x36},
> +	/*OPP5*/
> +	{S430M, VDD1_OPP5, 0x3C},
> +};
> +
> +const struct omap_opp_table omap3_mpu_opp_table = {
> +	VDD1_OPP1,
> +	VDD1_OPP5,
> +	omap3_mpu_rate_table
> +};
> +
> +const struct omap_opp_table omap3_dsp_opp_table = {
> +        VDD1_OPP1,
> +        VDD1_OPP5,
> +        omap3_dsp_rate_table
> +};
> +
> +const struct omap_opp_table omap3_l3_opp_table = {
> +        VDD2_OPP1,
> +        VDD2_OPP3,
> +        omap3_l3_rate_table
> +};
> +
>  static inline void omap3_per_save_context(void)
>  {
>  	omap3_gpio_save_context();
> diff --git a/arch/arm/plat-omap/include/mach/omap-pm.h b/arch/arm/plat-omap/include/mach/omap-pm.h
> index 583e540..51216cf 100644
> --- a/arch/arm/plat-omap/include/mach/omap-pm.h
> +++ b/arch/arm/plat-omap/include/mach/omap-pm.h
> @@ -33,6 +33,20 @@ struct omap_opp {
>  	u16 vsel;
>  };
>  
> +/* struct omap_opp_table - View OPP table as an object
> + * @min: Minimum OPP id
> + * @max: Maximim OPP id
> + * @opps: Pointer to array defining the OPPs.
> + *
> + * An OPP table has varied length. Knowing minimum and maximum
> + * OPP ids allow easy traversal.
> + */
> +struct omap_opp_table {
> +	u8	min;
> +	u8	max;
> +	struct omap_opp* opps;
> +};
> +
>  extern struct omap_opp *mpu_opps;
>  extern struct omap_opp *dsp_opps;
>  extern struct omap_opp *l3_opps;
> @@ -48,6 +62,14 @@ extern struct omap_opp *l3_opps;
>  #define OCP_TARGET_AGENT		1
>  #define OCP_INITIATOR_AGENT		2
>  
> +int opp_to_freq(unsigned long* freq,
> +		const struct omap_opp_table* table,
> +		u8 opp);
> +
> +int freq_to_opp(u8* opp,
> +		const struct omap_opp_table* table,
> +		unsigned long freq);
> +
>  /**
>   * omap_pm_if_early_init - OMAP PM init code called before clock fw init
>   * @mpu_opp_table: array ptr to struct omap_opp for MPU
> -- 
> 1.6.2.2
>
> --
> 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