RE: [PATCHv4 3/8] ARM: OMAP3+: voltage: add support for voltagedomain usecounts

On Tue, 2012-07-24 at 15:58 -0500, Vishwanath Sripathy wrote:

Tero,

quoted

-----Original Message-----
From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
owner@vger.kernel.org] On Behalf Of Tero Kristo
Sent: Friday, July 13, 2012 9:20 AM
To: linux-omap@vger.kernel.org; paul@pwsan.com; khilman@ti.com
Cc: linux-arm-kernel@lists.infradead.org
Subject: [PATCHv4 3/8] ARM: OMAP3+: voltage: add support for
voltagedomain usecounts

These are updated based on powerdomain usecounts. Also added support
for voltdm->sleep and voltdm->wakeup calls that will be invoked once
voltagedomain enters sleep or wakes up based on usecount numbers.
These
will be used for controlling voltage scaling functionality.

Signed-off-by: Tero Kristo <redacted>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <redacted>
---
 arch/arm/mach-omap2/powerdomain.c |    6 +++-
 arch/arm/mach-omap2/voltage.c     |   56
+++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/voltage.h     |   11 +++++++
 3 files changed, 72 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-

omap2/powerdomain.c
index 68bdf36..3b4b15d 100644

--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c

@@ -994,7 +994,8 @@ void pwrdm_clkdm_enable(struct powerdomain

*pwrdm)
 	if (!pwrdm)
 		return;

-	atomic_inc(&pwrdm->usecount);
+	if (atomic_inc_return(&pwrdm->usecount) == 1)
+		voltdm_pwrdm_enable(pwrdm->voltdm.ptr);
 }

 /**

@@ -1013,6 +1014,9 @@ void pwrdm_clkdm_disable(struct powerdomain

*pwrdm)

 	val = atomic_dec_return(&pwrdm->usecount);

+	if (!val)
+		voltdm_pwrdm_disable(pwrdm->voltdm.ptr);
+
 	BUG_ON(val < 0);
 }

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-

omap2/voltage.c
index 4dc60e8..8c6439b 100644

--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c

@@ -38,6 +38,7 @@

 #include "voltage.h"
 #include "powerdomain.h"
+#include "smartreflex.h"

 #include "vc.h"
 #include "vp.h"

@@ -340,6 +341,61 @@ int voltdm_add_pwrdm(struct voltagedomain

*voltdm, struct powerdomain *pwrdm)
 }

 /**
+ * voltdm_pwrdm_enable - increase usecount for a voltagedomain
+ * @voltdm: struct voltagedomain * to increase count for
+ *
+ * Increases usecount for a given voltagedomain. If the usecount
reaches
+ * 1, the domain is awakened from idle and the function will call
the
+ * voltagedomain->wakeup callback for this domain.
+ */
+void voltdm_pwrdm_enable(struct voltagedomain *voltdm)
+{
+	if (!voltdm)
+		return;
+
+	if (atomic_inc_return(&voltdm->usecount) == 1) {
+		if (voltdm->wakeup)
+			voltdm->wakeup(voltdm);
+	}
+}

I think these usecounting functions are prone to race conditions and need
to be protected using spin_lock or something like that (not just the
usecounter). Otherwise right sequence will not be followed when multiple
modules try to idle and enable simultaneously. You can see this issue when
2 cpus enter idle independently.

Yes, you are right... something like this needs to be added for SMP once
individual cpuidle is in place. I'll add this kind of fix for the next
version.

-Tero