Re: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
On 8/4/2010 6:31 AM, Gopinath, Thara wrote: From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, June 25, 2010 11:56 PM Thara Gopinathth...@ti.com writes: This patch removes the usage of vdd and sr id alltogether. This is achieved by introducing a separte voltage domain per VDD and hooking this up with the voltage and smartreflex internal info structure. Any user of voltage or smartreflex layer should call into omap_volt_domain_get to get the voltage domain handle and make use of this to call into the various exported API's. Great, I'm glad to see those gone. Minor comment on naming: In current code, we currently have struct clockdomain *clkdm; struct powerdomain *pwrdm; so, for consistency, I'd suggest using struct voltagedomain *voltdm; instead of this: struct omap_volt_domain *volt_domain; Also, it looks like your 'struct omap_vdd_info' is the real struct that represents a voltage domain. Maybe you're planning this already, but I suggest you get rid of omap_vdd_info and just move all that stuff into the voltagedomain. Again, that will probably create a diff with a ton of renames, so this should just be part of your V2 series. Are you sure? Because omap_vdd_info contains all the internal details about the voltage domains. Do we really want to expose it? IMHO omap_vdd_info should remain as internal structure instead of exposing it out. I think as well that struct voltagedomain should be a soc generic representation of the voltage domain, whereas omap_vdd_info will contain all the soc specific details. The issue with voltage domain, is that the compared to clockdomain and powerdomain, the details are quite different between OMAP2, 3 and 4. OMAP2 does not have any VP, VC, SR, so in this case, the voltagedomain should be almost empty whereas OMAP3 will contain much more stuff. OMAP4 should be pretty similar. That being said, since we have only 1 scalable voltage domain on OMAP2, we can still manage the overhead. Benoit -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
Cousson, Benoit b-cous...@ti.com writes: On 8/4/2010 6:31 AM, Gopinath, Thara wrote: From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, June 25, 2010 11:56 PM Thara Gopinathth...@ti.com writes: This patch removes the usage of vdd and sr id alltogether. This is achieved by introducing a separte voltage domain per VDD and hooking this up with the voltage and smartreflex internal info structure. Any user of voltage or smartreflex layer should call into omap_volt_domain_get to get the voltage domain handle and make use of this to call into the various exported API's. Great, I'm glad to see those gone. Minor comment on naming: In current code, we currently have struct clockdomain *clkdm; struct powerdomain *pwrdm; so, for consistency, I'd suggest using struct voltagedomain *voltdm; instead of this: struct omap_volt_domain *volt_domain; Also, it looks like your 'struct omap_vdd_info' is the real struct that represents a voltage domain. Maybe you're planning this already, but I suggest you get rid of omap_vdd_info and just move all that stuff into the voltagedomain. Again, that will probably create a diff with a ton of renames, so this should just be part of your V2 series. Are you sure? Because omap_vdd_info contains all the internal details about the voltage domains. Do we really want to expose it? IMHO omap_vdd_info should remain as internal structure instead of exposing it out. I think as well that struct voltagedomain should be a soc generic representation of the voltage domain, whereas omap_vdd_info will contain all the soc specific details. OK. Kevin The issue with voltage domain, is that the compared to clockdomain and powerdomain, the details are quite different between OMAP2, 3 and 4. OMAP2 does not have any VP, VC, SR, so in this case, the voltagedomain should be almost empty whereas OMAP3 will contain much more stuff. OMAP4 should be pretty similar. That being said, since we have only 1 scalable voltage domain on OMAP2, we can still manage the overhead. Benoit -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
-Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Friday, June 25, 2010 11:56 PM To: Gopinath, Thara Cc: linux-omap@vger.kernel.org; p...@pwsan.com; Cousson, Benoit; Sripathy, Vishwanath; Sawant, Anand Subject: Re: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's. Thara Gopinath th...@ti.com writes: This patch removes the usage of vdd and sr id alltogether. This is achieved by introducing a separte voltage domain per VDD and hooking this up with the voltage and smartreflex internal info structure. Any user of voltage or smartreflex layer should call into omap_volt_domain_get to get the voltage domain handle and make use of this to call into the various exported API's. Great, I'm glad to see those gone. Minor comment on naming: In current code, we currently have struct clockdomain *clkdm; struct powerdomain *pwrdm; so, for consistency, I'd suggest using struct voltagedomain *voltdm; instead of this: struct omap_volt_domain *volt_domain; Also, it looks like your 'struct omap_vdd_info' is the real struct that represents a voltage domain. Maybe you're planning this already, but I suggest you get rid of omap_vdd_info and just move all that stuff into the voltagedomain. Again, that will probably create a diff with a ton of renames, so this should just be part of your V2 series. Are you sure? Because omap_vdd_info contains all the internal details about the voltage domains. Do we really want to expose it? IMHO omap_vdd_info should remain as internal structure instead of exposing it out. Regards Thara -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
[Reply 1/2]
Gopinath, Thara had written, on 06/24/2010 10:02 AM, the following:
This patch removes the usage of vdd and sr id alltogether.
good.. thanks for doing this :). /me had enough of them ;)
This is achieved by introducing a separte voltage domain per
VDD and hooking this up with the voltage and smartreflex
internal info structure. Any user of voltage or smartreflex layer
should call into omap_volt_domain_get to get the voltage
domain handle and make use of this to call into the various
exported API's.
These changes should be part of V2 of the sr/voltage series
instead of being a separate patch in itself.
Apologies on the spam, but it does look like the reply never reached l-o due to
size! I will split the reply into two parts if possible
This series seems to do a lot more than sr id, vdd id removal.. mebbe
splitting this series was easier for review - after spending an hr of
cross refering the humungous patch to code, i am all set to have a break ;).
I know the series will get squashed anyways, but helps review. anyways,
my attempt at giving some sane comments below.
Signed-off-by: Thara Gopinath th...@ti.com
---
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c|4 +
arch/arm/mach-omap2/pm-debug.c|2 +-
arch/arm/mach-omap2/smartreflex-class3.c | 26 +-
arch/arm/mach-omap2/smartreflex.c | 115 +++--
arch/arm/mach-omap2/sr_device.c | 14 +-
arch/arm/mach-omap2/voltage.c | 697
+
arch/arm/mach-omap2/voltage.h | 126 -
arch/arm/plat-omap/include/plat/smartreflex.h | 41 +-
arch/arm/plat-omap/include/plat/voltage.h | 137 +
9 files changed, 617 insertions(+), 545 deletions(-)
delete mode 100644 arch/arm/mach-omap2/voltage.h
create mode 100644 arch/arm/plat-omap/include/plat/voltage.h
what is the motivation for plat/voltage.h? are we expecting external
users other than voltage related layers to use this? are all APIs
required to be exposed?
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 074347f..c4f9abc 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -265,6 +265,7 @@ static struct omap_sr_dev_data omap34xx_sr1_dev_attr = {
.test_sennenable= 0x3,
.test_senpenable= 0x3,
.test_nvalues = omap34xx_sr1_test_nvalues,
+ .vdd_name = mpu,
};
static struct omap_hwmod omap34xx_sr1_hwmod = {
@@ -294,6 +295,7 @@ static struct omap_sr_dev_data omap36xx_sr1_dev_attr = {
.test_sennenable= 0x1,
.test_senpenable= 0x1,
.test_nvalues = omap36xx_sr1_test_nvalues,
+ .vdd_name = mpu,
};
static struct omap_hwmod omap36xx_sr1_hwmod = {
@@ -328,6 +330,7 @@ static struct omap_sr_dev_data omap34xx_sr2_dev_attr = {
.test_sennenable= 0x3,
.test_senpenable= 0x3,
.test_nvalues = omap34xx_sr2_test_nvalues,
+ .vdd_name = core,
};
static struct omap_hwmod omap34xx_sr2_hwmod = {
@@ -356,6 +359,7 @@ static struct omap_sr_dev_data omap36xx_sr2_dev_attr = {
.test_sennenable= 0x1,
.test_senpenable= 0x1,
.test_nvalues = omap36xx_sr2_test_nvalues,
+ .vdd_name = core,
minor nitpick - we should standardize on l3 or core - I like core as
it is a little more precise but i see usage mixed up in
arch/arm/mach-omap2/*.[ch] - just a side note..
};
static struct omap_hwmod omap36xx_sr2_hwmod = {
diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 9ed7146..82bb032 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -31,11 +31,11 @@
#include plat/board.h
#include plat/powerdomain.h
#include plat/clockdomain.h
+#include plat/voltage.h
#include prm.h
#include cm.h
#include pm.h
-#include voltage.h
int omap2_pm_debug;
diff --git a/arch/arm/mach-omap2/smartreflex-class3.c
b/arch/arm/mach-omap2/smartreflex-class3.c
index f3b766f..0b5c824 100644
--- a/arch/arm/mach-omap2/smartreflex-class3.c
+++ b/arch/arm/mach-omap2/smartreflex-class3.c
@@ -14,36 +14,36 @@
#include plat/smartreflex.h
#include smartreflex-class3.h
-#include voltage.h
-static int sr_class3_enable(int id)
+static int sr_class3_enable(struct omap_volt_domain *volt_domain)
{
unsigned long volt = 0;
- volt = get_curr_voltage(id);
+ volt = get_curr_voltage(volt_domain);
if (!volt) {
- pr_warning(%s: Current voltage unknown.Cannot enable SR%d\n,
- __func__, id);
+ pr_warning(%s: Current voltage unknown.Cannot enable
sr_%s\n,
I am going to be a nitpick ;)- space after
that '.'
Re: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
Thara Gopinath th...@ti.com writes: This patch removes the usage of vdd and sr id alltogether. This is achieved by introducing a separte voltage domain per VDD and hooking this up with the voltage and smartreflex internal info structure. Any user of voltage or smartreflex layer should call into omap_volt_domain_get to get the voltage domain handle and make use of this to call into the various exported API's. Great, I'm glad to see those gone. Minor comment on naming: In current code, we currently have struct clockdomain *clkdm; struct powerdomain *pwrdm; so, for consistency, I'd suggest using struct voltagedomain *voltdm; instead of this: struct omap_volt_domain *volt_domain; Also, it looks like your 'struct omap_vdd_info' is the real struct that represents a voltage domain. Maybe you're planning this already, but I suggest you get rid of omap_vdd_info and just move all that stuff into the voltagedomain. Again, that will probably create a diff with a ton of renames, so this should just be part of your V2 series. These changes should be part of V2 of the sr/voltage series instead of being a separate patch in itself. Agreed. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
Thara Gopinath th...@ti.com writes:
This patch removes the usage of vdd and sr id alltogether.
This is achieved by introducing a separte voltage domain per
VDD and hooking this up with the voltage and smartreflex
internal info structure. Any user of voltage or smartreflex layer
should call into omap_volt_domain_get to get the voltage
domain handle and make use of this to call into the various
exported API's.
These changes should be part of V2 of the sr/voltage series
instead of being a separate patch in itself.
Signed-off-by: Thara Gopinath th...@ti.com
[...]
-static struct omap_sr *_sr_lookup(int srid)
+static struct omap_sr *_sr_lookup(struct omap_volt_domain *volt_domain)
{
struct omap_sr *sr_info, *temp_sr_info;
sr_info = NULL;
list_for_each_entry(temp_sr_info, sr_list, node) {
- if (srid == temp_sr_info-srid) {
+ if (volt_domain == temp_sr_info-volt_domain) {
sr_info = temp_sr_info;
break;
Do we still need an _sr_lookup() function? Isn't there a single SR
instance per voltage domain?
At init time, the sr_info should be linked to the voltage domain, and
then the code can simply do:
struct omap_sr *sr_info = voltdm-sr_info;
instead of _sr_lookup.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PM-SR] [PATCH] OMAP: PM: Remove the usage of vdd id's.
From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
Sent: Friday, June 25, 2010 8:49 PM
Thara Gopinath th...@ti.com writes:
This patch removes the usage of vdd and sr id alltogether.
This is achieved by introducing a separte voltage domain per
VDD and hooking this up with the voltage and smartreflex
internal info structure. Any user of voltage or smartreflex layer
should call into omap_volt_domain_get to get the voltage
domain handle and make use of this to call into the various
exported API's.
These changes should be part of V2 of the sr/voltage series
instead of being a separate patch in itself.
Signed-off-by: Thara Gopinath th...@ti.com
[...]
-static struct omap_sr *_sr_lookup(int srid)
+static struct omap_sr *_sr_lookup(struct omap_volt_domain
*volt_domain)
{
struct omap_sr *sr_info, *temp_sr_info;
sr_info = NULL;
list_for_each_entry(temp_sr_info, sr_list, node) {
-if (srid == temp_sr_info-srid) {
+if (volt_domain == temp_sr_info-volt_domain) {
sr_info = temp_sr_info;
break;
Do we still need an _sr_lookup() function? Isn't there a single SR
instance per voltage domain?
Yep, it must be a one to one mapping. But you still need to get the smartreflex
instance that belong to a certain voltage domain. A voltage domain does not know
if it has a SR that can control it.
Benoit
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet.
036 420 040 R.C.S Antibes. Capital de EUR 753.920
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
