[PATCH RFC v2 00/16] PM / Domains: Generic PM domains for CPUs

[PATCH RFC v2 00/16] PM / Domains: Generic PM domains for CPUs

[Lina Iyer]

Changes since v1 [1]:

- Address review comments on v1.
- Incorporate Kevin's arch/arm/domain.c changes
- Drop drivers/base/power/cpu_domain.c 
- Rebase on top of linux-next (to date)
- Reference implemantation added.

This patchset fashions CPU clusters as generic PM domains. CPUs in most new
SOCs are grouped together as a cluster, along with some supporting hardware,
GIC, L2 caches etc. When all these devices in a cluster is powered off, the
power domain may also be powered off. 

GenPD framework provides the necessary backend to build a cluster hierarchy
through devices, domains and nested domains. When devices and sub-domains of a
genpd are suspended, the genpd may also be suspended and resumed before the
first of the devices resumes. This works well for most devices and domains.
However, not all devices operate in a process context. Devices that suspend and
resume when IRQs are disabled, are defined as IRQ safe devices and domains that
have IRQ safe devices are never powered off. The reason being, genpd uses
mutexes to lock domains and IRQ safe devices may operate in atomic contexts,
therefore cannot sleep. So even when all the devices are suspended, the domain
remains powered on.

Since CPUIdle frameworks calls the CPU's idle functions with IRQs disabled, the
CPU devices have to be IRQ safe. A cluster fashioned as genpd be attached to
IRQ safe CPU devices and possibly other devices that may or may not be IRQ
safe. However, even when all these devices are suspended, the genpd would never
be powered off. This is a significant part of the cluster's idle power
consumption.

These patches attempt to save some runtime idle power of CPU clusters by
allowing the genpd to suspend even when the attached devices are IRQ safe. The
approach here is simple, allow genpd domains to specify the type of locking the
domain would use. A genpd that can suspend and resume in an IRQ safe context,
would initialize a spinlock as the genpd lock instead of a mutex. Therefore,
IRQ safe devices may initiate a genpd suspend when the last active device goes
idle. In a CPU domain, the last CPU powering down, may now program the domain
hardware to suspend, when the CPU enters idle. Thus when all the CPUs, caches,
peripheral cluster hardware are in idle, the cluster domain would also be in a
low power state.

These patches also bring in a generalized way to represent CPU clusters on an
ARM platform. Platform drivers specify the CPU domain providers in their DT and
a genpd is initialized for every domain provider specified for the CPU.
Optionally, the platform may register their callback to perform SoC specific
functions when the domains are suspended/resumed. Currently, only CPU devices
are attached to the CPU genpd by default. Devices representing L2, GIC,
peripheral hardware that depend on the PM domain, may also be attached to the
genpd in the future.

This RFC is based on Ulf's clean up of genpd intermediate states [2] and
Kevin's WIP on generic PM domains for ARM CPU cluster. CPU domain definition
for the QCOM DB platform is provided as reference (patch 11/16 onwards).

Thanks,
Lina

[1]. http://www.spinics.net/lists/arm-kernel/msg423430.html
[2]. https://patches.linaro.org/50047/


Kevin Hilman (1):
  WIP: ARM: PM domains for CPUs/clusters

Lina Iyer (15):
  PM / Domains: Allocate memory outside domain locks
  PM / Domains: Remove dev->driver check for runtime PM
  PM / Domains: Support IRQ safe PM domains
  arm: domain: Remove hack to add dev->driver.
  arm: domain: Make CPU genpd IRQ safe
  arm: domain: Synchronize CPU device runtime PM usage with idle state
  arm: domain: Handle CPU online reference counting
  arm: domain: Add platform callbacks for domain power on/off
  drivers: cpuidle: Add runtime PM support for CPUIdle
  drivers: qcom: spm: Support cache and coherency SPMs
  drivers: qcom: spm: Enable runtime suspend/resume of CPU PM domain
  drivers: qcom: spm: Add 8084 L2 SPM register data
  arm: dts: Add L2 power-controller device bindings for APQ8084
  arm: dts: Add power domain device bindings for APQ8084
  drivers: qcom: Enable genpd on selecting QCOM_PM

 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |   1 +
 Documentation/power/devices.txt                    |  11 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |  18 ++
 arch/arm/boot/dts/qcom-apq8084.dtsi                |  10 +-
 arch/arm/include/asm/cpu.h                         |   1 -
 arch/arm/include/asm/pm_domain.h                   |  27 +++
 arch/arm/kernel/Makefile                           |   1 +
 arch/arm/kernel/domains.c                          | 243 +++++++++++++++++++++
 drivers/base/power/domain.c                        | 230 ++++++++++++++-----
 drivers/cpuidle/cpuidle-arm.c                      |  11 +
 drivers/soc/qcom/Kconfig                           |   3 +
 drivers/soc/qcom/spm.c                             | 156 ++++++++++++-
 include/linux/pm_domain.h                          |  11 +-
 13 files changed, 649 insertions(+), 74 deletions(-)
 create mode 100644 arch/arm/include/asm/pm_domain.h
 create mode 100644 arch/arm/kernel/domains.c

-- 
2.1.4

[PATCH RFC v2 00/16] PM / Domains: Generic PM domains for CPUs

[Lina Iyer]

Changes since v1 [1]:

- Address review comments on v1.
- Incorporate Kevin's arch/arm/domain.c changes
- Drop drivers/base/power/cpu_domain.c 
- Rebase on top of linux-next (to date)
- Reference implemantation added.

This patchset fashions CPU clusters as generic PM domains. CPUs in most new
SOCs are grouped together as a cluster, along with some supporting hardware,
GIC, L2 caches etc. When all these devices in a cluster is powered off, the
power domain may also be powered off. 

GenPD framework provides the necessary backend to build a cluster hierarchy
through devices, domains and nested domains. When devices and sub-domains of a
genpd are suspended, the genpd may also be suspended and resumed before the
first of the devices resumes. This works well for most devices and domains.
However, not all devices operate in a process context. Devices that suspend and
resume when IRQs are disabled, are defined as IRQ safe devices and domains that
have IRQ safe devices are never powered off. The reason being, genpd uses
mutexes to lock domains and IRQ safe devices may operate in atomic contexts,
therefore cannot sleep. So even when all the devices are suspended, the domain
remains powered on.

Since CPUIdle frameworks calls the CPU's idle functions with IRQs disabled, the
CPU devices have to be IRQ safe. A cluster fashioned as genpd be attached to
IRQ safe CPU devices and possibly other devices that may or may not be IRQ
safe. However, even when all these devices are suspended, the genpd would never
be powered off. This is a significant part of the cluster's idle power
consumption.

These patches attempt to save some runtime idle power of CPU clusters by
allowing the genpd to suspend even when the attached devices are IRQ safe. The
approach here is simple, allow genpd domains to specify the type of locking the
domain would use. A genpd that can suspend and resume in an IRQ safe context,
would initialize a spinlock as the genpd lock instead of a mutex. Therefore,
IRQ safe devices may initiate a genpd suspend when the last active device goes
idle. In a CPU domain, the last CPU powering down, may now program the domain
hardware to suspend, when the CPU enters idle. Thus when all the CPUs, caches,
peripheral cluster hardware are in idle, the cluster domain would also be in a
low power state.

These patches also bring in a generalized way to represent CPU clusters on an
ARM platform. Platform drivers specify the CPU domain providers in their DT and
a genpd is initialized for every domain provider specified for the CPU.
Optionally, the platform may register their callback to perform SoC specific
functions when the domains are suspended/resumed. Currently, only CPU devices
are attached to the CPU genpd by default. Devices representing L2, GIC,
peripheral hardware that depend on the PM domain, may also be attached to the
genpd in the future.

This RFC is based on Ulf's clean up of genpd intermediate states [2] and
Kevin's WIP on generic PM domains for ARM CPU cluster. CPU domain definition
for the QCOM DB platform is provided as reference (patch 11/16 onwards).

Thanks,
Lina

[1]. http://www.spinics.net/lists/arm-kernel/msg423430.html
[2]. https://patches.linaro.org/50047/


Kevin Hilman (1):
  WIP: ARM: PM domains for CPUs/clusters

Lina Iyer (15):
  PM / Domains: Allocate memory outside domain locks
  PM / Domains: Remove dev->driver check for runtime PM
  PM / Domains: Support IRQ safe PM domains
  arm: domain: Remove hack to add dev->driver.
  arm: domain: Make CPU genpd IRQ safe
  arm: domain: Synchronize CPU device runtime PM usage with idle state
  arm: domain: Handle CPU online reference counting
  arm: domain: Add platform callbacks for domain power on/off
  drivers: cpuidle: Add runtime PM support for CPUIdle
  drivers: qcom: spm: Support cache and coherency SPMs
  drivers: qcom: spm: Enable runtime suspend/resume of CPU PM domain
  drivers: qcom: spm: Add 8084 L2 SPM register data
  arm: dts: Add L2 power-controller device bindings for APQ8084
  arm: dts: Add power domain device bindings for APQ8084
  drivers: qcom: Enable genpd on selecting QCOM_PM

 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |   1 +
 Documentation/power/devices.txt                    |  11 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |  18 ++
 arch/arm/boot/dts/qcom-apq8084.dtsi                |  10 +-
 arch/arm/include/asm/cpu.h                         |   1 -
 arch/arm/include/asm/pm_domain.h                   |  27 +++
 arch/arm/kernel/Makefile                           |   1 +
 arch/arm/kernel/domains.c                          | 243 +++++++++++++++++++++
 drivers/base/power/domain.c                        | 230 ++++++++++++++-----
 drivers/cpuidle/cpuidle-arm.c                      |  11 +
 drivers/soc/qcom/Kconfig                           |   3 +
 drivers/soc/qcom/spm.c                             | 156 ++++++++++++-
 include/linux/pm_domain.h                          |  11 +-
 13 files changed, 649 insertions(+), 74 deletions(-)
 create mode 100644 arch/arm/include/asm/pm_domain.h
 create mode 100644 arch/arm/kernel/domains.c

-- 
2.1.4

[PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Lina Iyer]

In preparation for supporting IRQ-safe domains, allocate domain data
outside the domain locks. These functions are not called in an atomic
context, so we can always allocate memory using GFP_KERNEL. By
allocating memory before the locks, we can safely lock the domain using
spinlocks instead of mutexes.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes in v2:
	- Modify commit text
---
 drivers/base/power/domain.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 87d405a..44af889 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *subdomain)
 {
-	struct gpd_link *link;
+	struct gpd_link *link, *itr;
 	int ret = 0;
 
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
 
@@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		goto out;
 	}
 
-	list_for_each_entry(link, &genpd->master_links, master_node) {
-		if (link->slave == subdomain && link->master == genpd) {
+	list_for_each_entry(itr, &genpd->master_links, master_node) {
+		if (itr->slave == subdomain && itr->master == genpd) {
 			ret = -EINVAL;
 			goto out;
 		}
 	}
 
-	link = kzalloc(sizeof(*link), GFP_KERNEL);
-	if (!link) {
-		ret = -ENOMEM;
-		goto out;
-	}
 	link->master = genpd;
 	list_add_tail(&link->master_node, &genpd->master_links);
 	link->slave = subdomain;
@@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
+	if (!cpuidle_data)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
-		goto out;
-	}
-	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-	if (!cpuidle_data) {
-		ret = -ENOMEM;
-		goto out;
+		goto err_drv;
 	}
+
 	cpuidle_drv = cpuidle_driver_ref();
 	if (!cpuidle_drv) {
 		ret = -ENODEV;
-- 
2.1.4

[PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Lina Iyer]

In preparation for supporting IRQ-safe domains, allocate domain data
outside the domain locks. These functions are not called in an atomic
context, so we can always allocate memory using GFP_KERNEL. By
allocating memory before the locks, we can safely lock the domain using
spinlocks instead of mutexes.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes in v2:
	- Modify commit text
---
 drivers/base/power/domain.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 87d405a..44af889 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *subdomain)
 {
-	struct gpd_link *link;
+	struct gpd_link *link, *itr;
 	int ret = 0;
 
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
 
@@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		goto out;
 	}
 
-	list_for_each_entry(link, &genpd->master_links, master_node) {
-		if (link->slave == subdomain && link->master == genpd) {
+	list_for_each_entry(itr, &genpd->master_links, master_node) {
+		if (itr->slave == subdomain && itr->master == genpd) {
 			ret = -EINVAL;
 			goto out;
 		}
 	}
 
-	link = kzalloc(sizeof(*link), GFP_KERNEL);
-	if (!link) {
-		ret = -ENOMEM;
-		goto out;
-	}
 	link->master = genpd;
 	list_add_tail(&link->master_node, &genpd->master_links);
 	link->slave = subdomain;
@@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
+	if (!cpuidle_data)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
-		goto out;
-	}
-	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-	if (!cpuidle_data) {
-		ret = -ENOMEM;
-		goto out;
+		goto err_drv;
 	}
+
 	cpuidle_drv = cpuidle_driver_ref();
 	if (!cpuidle_drv) {
 		ret = -ENODEV;
-- 
2.1.4

[PATCH RFC v2 02/16] PM / Domains: Remove dev->driver check for runtime PM

[Lina Iyer]

Remove check for driver of a device, for runtime PM. Device may be
suspended without an explicit driver. This check seems to be vestigial
and incorrect in the current context.

Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 44af889..e9b7cfb 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -391,8 +391,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		    || pdd->dev->power.irq_safe))
+		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
 			not_suspended++;
 	}
 
-- 
2.1.4

[PATCH RFC v2 02/16] PM / Domains: Remove dev->driver check for runtime PM

[Lina Iyer]

Remove check for driver of a device, for runtime PM. Device may be
suspended without an explicit driver. This check seems to be vestigial
and incorrect in the current context.

Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 44af889..e9b7cfb 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -391,8 +391,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		    || pdd->dev->power.irq_safe))
+		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
 			not_suspended++;
 	}
 
-- 
2.1.4

[PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Lina Iyer]

Generic Power Domains currently support turning on/off only in process
context. This prevents the usage of PM domains for domains that could be
powered on/off in a context where IRQs are disabled. Many such domains
exist today and do not get powered off, when the IRQ safe devices in
that domain are powered off, because of this limitation.

However, not all domains can operate in IRQ safe contexts. Genpd
therefore, has to support both cases where the domain may or may not
operate in IRQ safe contexts. Configuring genpd to use an appropriate
lock for that domain, would allow domains that have IRQ safe devices to
runtime suspend and resume, in atomic context.

To achieve domain specific locking, set the domain's ->flag to
GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
should use a spinlock instead of a mutex for locking the domain. Locking
is abstracted through genpd_lock() and genpd_unlock() functions that use
the flag to determine the appropriate lock to be used for that domain.
Domains that have lower latency to suspend and resume and can operate
with IRQs disabled may now be able to save power, when the component
devices and sub-domains are idle at runtime.

The restriction this imposes on the domain hierarchy is that sub-domains
and all devices in the IRQ safe domain's hierarchy also have to be IRQ
safe, so that we dont try to lock a mutex, while holding a spinlock.
Non-IRQ safe domains may continue to have devices and sub-domains that
may or may not be IRQ safe.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes in v2:
	- Modify commit text
	- Fix an issue with IRQ safe check
	- Rename genpd_lock_domain() and genpd_unlock_domain names
	- Inlined functions instead of macros
	- Rebase on top of V4 of Ulf's patch [1].

[1]. https://patches.linaro.org/50047/
---
 Documentation/power/devices.txt |  11 ++-
 drivers/base/power/domain.c     | 201 +++++++++++++++++++++++++++++++---------
 include/linux/pm_domain.h       |  11 ++-
 3 files changed, 178 insertions(+), 45 deletions(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index d172bce..67b74ae 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -600,7 +600,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
 into a low-power state together at the same time by turning off the shared
 power resource.  Of course, they also need to be put into the full-power state
 together, by turning the shared power resource on.  A set of devices with this
-property is often referred to as a power domain.
+property is often referred to as a power domain. A power domain may also be
+nested inside another domain.
+
+Devices, by default, operate in process context and if a device can operate in
+IRQ safe context, has to be explicitly set as IRQ safe. PM domains by default,
+operate in process context but could have devices that are IRQ safe. Such PM
+domains cannot be powered on/off during runtime PM. On the other hand, an IRQ
+safe PM domain that can be powered on/off and suspend or resumed in an atomic
+context, may contain IRQ safe devices. Such domain may only contain only IRQ
+safe devices or IRQ safe sub-domains.
 
 Support for power domains is provided through the pm_domain field of struct
 device.  This field is a pointer to an object of type struct dev_pm_domain,
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e9b7cfb..42ffb8b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -50,6 +50,74 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	if (unlikely(subclass > 0))
+		spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+	else
+		spin_lock_irqsave(&genpd->slock, flags);
+
+	genpd->lock_flags = flags;
+	return 0;
+}
+
+static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+}
+
+static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->mlock)
+{
+	if (unlikely(subclass > 0))
+		mutex_lock_nested(&genpd->mlock, subclass);
+	else
+		mutex_lock(&genpd->mlock);
+	return 0;
+}
+
+static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
+	__acquires(&genpd->mlock)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
+	__releases(&genpd->mlock)
+{
+	mutex_unlock(&genpd->mlock);
+}
+
+static inline int genpd_lock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+			: genpd_lock_irq(genpd, 0);
+}
+
+static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
+			: genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
+}
+
+static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+			: genpd_lock_interruptible_irq(genpd);
+}
+
+static inline void genpd_unlock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_unlock_noirq(genpd)
+			: genpd_unlock_irq(genpd);
+}
+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
@@ -270,9 +338,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
 	int ret;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -332,9 +400,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 		spin_unlock_irq(&dev->power.lock);
 
 		if (!IS_ERR(genpd)) {
-			mutex_lock(&genpd->lock);
+			genpd_lock(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock(genpd);
 		}
 
 		dev = dev->parent;
@@ -391,7 +459,13 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
+		/*
+		 * We do not want to power off the domain if the device is
+		 * not suspended or an IRQ safe device is part of this
+		 * non-IRQ safe domain.
+		 */
+		if (!pm_runtime_suspended(pdd->dev) ||
+			(pdd->dev->power.irq_safe && !genpd->irq_safe))
 			not_suspended++;
 	}
 
@@ -457,9 +531,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	pm_genpd_poweroff(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 }
 
 /**
@@ -497,17 +571,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	}
 
 	/*
-	 * If power.irq_safe is set, this routine will be run with interrupts
-	 * off, so it can't use mutexes.
+	 * If power.irq_safe is set, this routine may be run with
+	 * IRQ disabled, so suspend only if the power domain is
+	 * irq_safe.
 	 */
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return 0;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
@@ -532,15 +608,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/* If power.irq_safe, the PM domain is never powered off. */
-	if (dev->power.irq_safe) {
+	/*
+	 * As we dont power off a non IRQ safe domain, which holds a
+	 * an IRQ safe device, we dont need to restore power to it.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe) {
 		timed = false;
 		goto out;
 	}
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		return ret;
@@ -740,14 +819,14 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count++ == 0) {
 		genpd->suspended_count = 0;
 		genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (genpd->suspend_power_off) {
 		pm_runtime_put_noidle(dev);
@@ -765,12 +844,12 @@ static int pm_genpd_prepare(struct device *dev)
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock(genpd);
 
 		if (--genpd->prepared_count == 0)
 			genpd->suspend_power_off = false;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 		pm_runtime_enable(dev);
 	}
 
@@ -1127,13 +1206,13 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	run_complete = !genpd->suspend_power_off;
 	if (--genpd->prepared_count == 0)
 		genpd->suspend_power_off = false;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (run_complete) {
 		pm_generic_complete(dev);
@@ -1277,11 +1356,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
+	/* Devices in an IRQ safe PM domain have to be IRQ safe too */
+	if (genpd->irq_safe && !dev->power.irq_safe) {
+		dev_err(dev,
+			"Devices in an IRQ safe domain have to be IRQ safe.\n");
+		return -EINVAL;
+	}
+
 	gpd_data = genpd_alloc_dev_data(dev, genpd, td);
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1298,7 +1384,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
@@ -1342,7 +1428,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	gpd_data = to_gpd_data(pdd);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1357,14 +1443,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	list_del_init(&pdd->list_node);
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
@@ -1385,12 +1471,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	/*
+	 * If the domain can be powered on/off in an IRQ safe
+	 * context, ensure that the subdomain can also be
+	 * powered on/off in that context.
+	 */
+	if (genpd->irq_safe && !subdomain->irq_safe) {
+		WARN("Incompatible sub-domain %s of IRQ safe domain %s\n",
+				subdomain->name, genpd->name);
+		return -EINVAL;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
-	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+	genpd_lock(genpd);
+	genpd_lock_nested(subdomain);
 
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    &&  subdomain->status != GPD_STATE_POWER_OFF) {
@@ -1413,8 +1510,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		genpd_sd_counter_inc(genpd);
 
  out:
-	mutex_unlock(&subdomain->lock);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(subdomain);
+	genpd_unlock(genpd);
 
 	return ret;
 }
@@ -1462,13 +1559,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	list_for_each_entry(link, &genpd->master_links, master_node) {
 		if (link->slave != subdomain)
 			continue;
 
-		mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+		genpd_lock_nested(subdomain);
 
 		list_del(&link->master_node);
 		list_del(&link->slave_node);
@@ -1476,13 +1573,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 		if (subdomain->status != GPD_STATE_POWER_OFF)
 			genpd_sd_counter_dec(genpd);
 
-		mutex_unlock(&subdomain->lock);
+		genpd_unlock(subdomain);
 
 		ret = 0;
 		break;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return ret;
 }
@@ -1506,11 +1603,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	if (genpd->irq_safe) {
+		WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
+				genpd->name);
+		return -EINVAL;
+	}
+
 	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
 	if (!cpuidle_data)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
@@ -1537,7 +1640,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	genpd_recalc_cpu_exit_latency(genpd);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 
  err:
@@ -1574,7 +1677,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	cpuidle_data = genpd->cpuidle_data;
 	if (!cpuidle_data) {
@@ -1592,7 +1695,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	kfree(cpuidle_data);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -1653,6 +1756,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
 	return cb ? cb(dev) : 0;
 }
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->irq_safe = true;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->irq_safe = false;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1668,7 +1782,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
-	mutex_init(&genpd->lock);
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	genpd->in_progress = 0;
@@ -2053,7 +2167,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	struct gpd_link *link;
 	int ret;
 
-	ret = mutex_lock_interruptible(&genpd->lock);
+	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
@@ -2073,7 +2187,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	}
 
 	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+		kobj_path = kobject_get_path(&pm_data->dev->kobj,
+				genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
 		if (kobj_path == NULL)
 			continue;
 
@@ -2084,7 +2199,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	seq_puts(s, "\n");
 exit:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b2725e6..dc7cb53 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -16,9 +16,11 @@
 #include <linux/of.h>
 #include <linux/notifier.h>
 #include <linux/cpuidle.h>
+#include <linux/spinlock.h>
 
 /* Defines used for the flags field in the struct generic_pm_domain */
 #define GENPD_FLAG_PM_CLK	(1U << 0) /* PM domain uses PM clk */
+#define GENPD_FLAG_IRQ_SAFE	(1U << 1) /* PM domain operates in atomic */
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
@@ -49,7 +51,6 @@ struct generic_pm_domain {
 	struct list_head master_links;	/* Links with PM domain as a master */
 	struct list_head slave_links;	/* Links with PM domain as a slave */
 	struct list_head dev_list;	/* List of devices */
-	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
 	const char *name;
@@ -74,6 +75,14 @@ struct generic_pm_domain {
 	void (*detach_dev)(struct generic_pm_domain *domain,
 			   struct device *dev);
 	unsigned int flags;		/* Bit field of configs for genpd */
+	bool irq_safe;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-- 
2.1.4

[PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Lina Iyer]

Generic Power Domains currently support turning on/off only in process
context. This prevents the usage of PM domains for domains that could be
powered on/off in a context where IRQs are disabled. Many such domains
exist today and do not get powered off, when the IRQ safe devices in
that domain are powered off, because of this limitation.

However, not all domains can operate in IRQ safe contexts. Genpd
therefore, has to support both cases where the domain may or may not
operate in IRQ safe contexts. Configuring genpd to use an appropriate
lock for that domain, would allow domains that have IRQ safe devices to
runtime suspend and resume, in atomic context.

To achieve domain specific locking, set the domain's ->flag to
GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
should use a spinlock instead of a mutex for locking the domain. Locking
is abstracted through genpd_lock() and genpd_unlock() functions that use
the flag to determine the appropriate lock to be used for that domain.
Domains that have lower latency to suspend and resume and can operate
with IRQs disabled may now be able to save power, when the component
devices and sub-domains are idle at runtime.

The restriction this imposes on the domain hierarchy is that sub-domains
and all devices in the IRQ safe domain's hierarchy also have to be IRQ
safe, so that we dont try to lock a mutex, while holding a spinlock.
Non-IRQ safe domains may continue to have devices and sub-domains that
may or may not be IRQ safe.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes in v2:
	- Modify commit text
	- Fix an issue with IRQ safe check
	- Rename genpd_lock_domain() and genpd_unlock_domain names
	- Inlined functions instead of macros
	- Rebase on top of V4 of Ulf's patch [1].

[1]. https://patches.linaro.org/50047/
---
 Documentation/power/devices.txt |  11 ++-
 drivers/base/power/domain.c     | 201 +++++++++++++++++++++++++++++++---------
 include/linux/pm_domain.h       |  11 ++-
 3 files changed, 178 insertions(+), 45 deletions(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index d172bce..67b74ae 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -600,7 +600,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
 into a low-power state together at the same time by turning off the shared
 power resource.  Of course, they also need to be put into the full-power state
 together, by turning the shared power resource on.  A set of devices with this
-property is often referred to as a power domain.
+property is often referred to as a power domain. A power domain may also be
+nested inside another domain.
+
+Devices, by default, operate in process context and if a device can operate in
+IRQ safe context, has to be explicitly set as IRQ safe. PM domains by default,
+operate in process context but could have devices that are IRQ safe. Such PM
+domains cannot be powered on/off during runtime PM. On the other hand, an IRQ
+safe PM domain that can be powered on/off and suspend or resumed in an atomic
+context, may contain IRQ safe devices. Such domain may only contain only IRQ
+safe devices or IRQ safe sub-domains.
 
 Support for power domains is provided through the pm_domain field of struct
 device.  This field is a pointer to an object of type struct dev_pm_domain,
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e9b7cfb..42ffb8b 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -50,6 +50,74 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	if (unlikely(subclass > 0))
+		spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+	else
+		spin_lock_irqsave(&genpd->slock, flags);
+
+	genpd->lock_flags = flags;
+	return 0;
+}
+
+static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+}
+
+static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->mlock)
+{
+	if (unlikely(subclass > 0))
+		mutex_lock_nested(&genpd->mlock, subclass);
+	else
+		mutex_lock(&genpd->mlock);
+	return 0;
+}
+
+static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
+	__acquires(&genpd->mlock)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
+	__releases(&genpd->mlock)
+{
+	mutex_unlock(&genpd->mlock);
+}
+
+static inline int genpd_lock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+			: genpd_lock_irq(genpd, 0);
+}
+
+static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
+			: genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
+}
+
+static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+			: genpd_lock_interruptible_irq(genpd);
+}
+
+static inline void genpd_unlock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_unlock_noirq(genpd)
+			: genpd_unlock_irq(genpd);
+}
+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
@@ -270,9 +338,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
 	int ret;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -332,9 +400,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 		spin_unlock_irq(&dev->power.lock);
 
 		if (!IS_ERR(genpd)) {
-			mutex_lock(&genpd->lock);
+			genpd_lock(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock(genpd);
 		}
 
 		dev = dev->parent;
@@ -391,7 +459,13 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
+		/*
+		 * We do not want to power off the domain if the device is
+		 * not suspended or an IRQ safe device is part of this
+		 * non-IRQ safe domain.
+		 */
+		if (!pm_runtime_suspended(pdd->dev) ||
+			(pdd->dev->power.irq_safe && !genpd->irq_safe))
 			not_suspended++;
 	}
 
@@ -457,9 +531,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	pm_genpd_poweroff(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 }
 
 /**
@@ -497,17 +571,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	}
 
 	/*
-	 * If power.irq_safe is set, this routine will be run with interrupts
-	 * off, so it can't use mutexes.
+	 * If power.irq_safe is set, this routine may be run with
+	 * IRQ disabled, so suspend only if the power domain is
+	 * irq_safe.
 	 */
-	if (dev->power.irq_safe)
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return 0;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
@@ -532,15 +608,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/* If power.irq_safe, the PM domain is never powered off. */
-	if (dev->power.irq_safe) {
+	/*
+	 * As we dont power off a non IRQ safe domain, which holds a
+	 * an IRQ safe device, we dont need to restore power to it.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe) {
 		timed = false;
 		goto out;
 	}
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		return ret;
@@ -740,14 +819,14 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count++ == 0) {
 		genpd->suspended_count = 0;
 		genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (genpd->suspend_power_off) {
 		pm_runtime_put_noidle(dev);
@@ -765,12 +844,12 @@ static int pm_genpd_prepare(struct device *dev)
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock(genpd);
 
 		if (--genpd->prepared_count == 0)
 			genpd->suspend_power_off = false;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 		pm_runtime_enable(dev);
 	}
 
@@ -1127,13 +1206,13 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	run_complete = !genpd->suspend_power_off;
 	if (--genpd->prepared_count == 0)
 		genpd->suspend_power_off = false;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (run_complete) {
 		pm_generic_complete(dev);
@@ -1277,11 +1356,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
+	/* Devices in an IRQ safe PM domain have to be IRQ safe too */
+	if (genpd->irq_safe && !dev->power.irq_safe) {
+		dev_err(dev,
+			"Devices in an IRQ safe domain have to be IRQ safe.\n");
+		return -EINVAL;
+	}
+
 	gpd_data = genpd_alloc_dev_data(dev, genpd, td);
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1298,7 +1384,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
@@ -1342,7 +1428,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	gpd_data = to_gpd_data(pdd);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1357,14 +1443,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	list_del_init(&pdd->list_node);
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
@@ -1385,12 +1471,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	/*
+	 * If the domain can be powered on/off in an IRQ safe
+	 * context, ensure that the subdomain can also be
+	 * powered on/off in that context.
+	 */
+	if (genpd->irq_safe && !subdomain->irq_safe) {
+		WARN("Incompatible sub-domain %s of IRQ safe domain %s\n",
+				subdomain->name, genpd->name);
+		return -EINVAL;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
-	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+	genpd_lock(genpd);
+	genpd_lock_nested(subdomain);
 
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    &&  subdomain->status != GPD_STATE_POWER_OFF) {
@@ -1413,8 +1510,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		genpd_sd_counter_inc(genpd);
 
  out:
-	mutex_unlock(&subdomain->lock);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(subdomain);
+	genpd_unlock(genpd);
 
 	return ret;
 }
@@ -1462,13 +1559,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	list_for_each_entry(link, &genpd->master_links, master_node) {
 		if (link->slave != subdomain)
 			continue;
 
-		mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+		genpd_lock_nested(subdomain);
 
 		list_del(&link->master_node);
 		list_del(&link->slave_node);
@@ -1476,13 +1573,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 		if (subdomain->status != GPD_STATE_POWER_OFF)
 			genpd_sd_counter_dec(genpd);
 
-		mutex_unlock(&subdomain->lock);
+		genpd_unlock(subdomain);
 
 		ret = 0;
 		break;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return ret;
 }
@@ -1506,11 +1603,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	if (genpd->irq_safe) {
+		WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
+				genpd->name);
+		return -EINVAL;
+	}
+
 	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
 	if (!cpuidle_data)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
@@ -1537,7 +1640,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	genpd_recalc_cpu_exit_latency(genpd);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 
  err:
@@ -1574,7 +1677,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	cpuidle_data = genpd->cpuidle_data;
 	if (!cpuidle_data) {
@@ -1592,7 +1695,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	kfree(cpuidle_data);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -1653,6 +1756,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
 	return cb ? cb(dev) : 0;
 }
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->irq_safe = true;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->irq_safe = false;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1668,7 +1782,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
-	mutex_init(&genpd->lock);
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	genpd->in_progress = 0;
@@ -2053,7 +2167,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	struct gpd_link *link;
 	int ret;
 
-	ret = mutex_lock_interruptible(&genpd->lock);
+	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
@@ -2073,7 +2187,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	}
 
 	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+		kobj_path = kobject_get_path(&pm_data->dev->kobj,
+				genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
 		if (kobj_path == NULL)
 			continue;
 
@@ -2084,7 +2199,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	seq_puts(s, "\n");
 exit:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b2725e6..dc7cb53 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -16,9 +16,11 @@
 #include <linux/of.h>
 #include <linux/notifier.h>
 #include <linux/cpuidle.h>
+#include <linux/spinlock.h>
 
 /* Defines used for the flags field in the struct generic_pm_domain */
 #define GENPD_FLAG_PM_CLK	(1U << 0) /* PM domain uses PM clk */
+#define GENPD_FLAG_IRQ_SAFE	(1U << 1) /* PM domain operates in atomic */
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
@@ -49,7 +51,6 @@ struct generic_pm_domain {
 	struct list_head master_links;	/* Links with PM domain as a master */
 	struct list_head slave_links;	/* Links with PM domain as a slave */
 	struct list_head dev_list;	/* List of devices */
-	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
 	const char *name;
@@ -74,6 +75,14 @@ struct generic_pm_domain {
 	void (*detach_dev)(struct generic_pm_domain *domain,
 			   struct device *dev);
 	unsigned int flags;		/* Bit field of configs for genpd */
+	bool irq_safe;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-- 
2.1.4

[PATCH RFC v2 04/16] WIP: ARM: PM domains for CPUs/clusters

[Lina Iyer]

From: Kevin Hilman <khilman@linaro.org>

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 534f27c..fc59876 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -61,6 +61,7 @@
 			reg = <0x0>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu1: cpu@1 {
@@ -69,6 +70,7 @@
 			reg = <0x1>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu2: cpu@2 {
@@ -77,6 +79,7 @@
 			reg = <0x2>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu3: cpu@3 {
@@ -85,6 +88,7 @@
 			reg = <0x3>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu4: cpu@100 {
@@ -93,6 +97,7 @@
 			reg = <0x100>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu5: cpu@101 {
@@ -101,6 +106,7 @@
 			reg = <0x101>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu6: cpu@102 {
@@ -109,6 +115,7 @@
 			reg = <0x102>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu7: cpu@103 {
@@ -117,6 +124,7 @@
 			reg = <0x103>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 	};
 
@@ -249,6 +257,16 @@
 		};
 	};
 
+	big_cluster_pd: big_cluster {
+		compatible = "arm,pd";
+		#power-domain-cells = <0>;
+	};
+
+	little_cluster_pd: little_cluster {
+		compatible = "arm,pd";
+ 		#power-domain-cells = <0>;
+	};
+
 	gsc_pd: power-domain@10044000 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044000 0x20>;
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index e69f7a1..98ce19c 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -78,6 +78,7 @@ CFLAGS_pj4-cp0.o		:= -marm
 AFLAGS_iwmmxt.o			:= -Wa,-mcpu=iwmmxt
 obj-$(CONFIG_ARM_CPU_TOPOLOGY)  += topology.o
 obj-$(CONFIG_VDSO)		+= vdso.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += domains.o
 
 ifneq ($(CONFIG_ARCH_EBSA110),y)
   obj-y		+= io.o
diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
new file mode 100644
index 0000000..8388f54
--- /dev/null
+++ b/arch/arm/kernel/domains.c
@@ -0,0 +1,122 @@
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/of_platform.h>
+#include <linux/device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define NAME_MAX 16
+
+struct arm_pm_domain {
+	struct generic_pm_domain genpd;
+};
+
+static inline
+struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
+{
+	return container_of(d, struct arm_pm_domain, genpd);
+}
+
+static int arm_pd_power_down(struct generic_pm_domain *genpd)
+{
+	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
+	return 0;
+}
+
+static int arm_pd_power_up(struct generic_pm_domain *genpd)
+{
+	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
+	return 0;
+}
+
+static int arm_domain_cpu_init(void)
+{
+	int cpuid, ret = 0;
+
+	/* Find any CPU nodes with a phandle to this power domain */
+	for_each_possible_cpu(cpuid) {
+		struct device *cpu_dev;
+
+		/* FIXME: this is open-coding of_cpu_device_node_get(), but I want handle to cpu_dev */
+		cpu_dev = get_cpu_device(cpuid);
+		if (!cpu_dev) {
+ 			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
+			return -ENODEV;
+		}
+
+		/* 
+		 * HACK: genpd checks if devices are runtime_suspended
+		 * before doing a poweroff of the domain.  However, that check
+		 * assumes that that device has a driver.  Since CPU devices don't
+		 * have a driver, genpd assumes that the device is runtime_suspended
+		 * and will power off the domain as soon as the any device 
+		 * in the domain does a runtime_suspend.
+		 *
+		 * c.f. the following code in pm_genpd_poweroff():
+		 *
+		 * if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
+		 *      || pdd->dev->power.irq_safe))
+		 *  	not_suspended++;
+		 *
+		 * Just removing the pdd->dev->driver check would also work, 
+		 * but not sure if that's right in the general case.
+		 */
+		cpu_dev->driver = kzalloc(sizeof(struct device_driver), GFP_KERNEL);
+		WARN_ON(!cpu_dev->driver);
+
+		if (cpu_online(cpuid)) {
+			pm_runtime_set_active(cpu_dev);
+			pm_runtime_get_noresume(cpu_dev);
+		} else {
+			pm_runtime_set_suspended(cpu_dev);
+		}
+		pm_runtime_irq_safe(cpu_dev);
+		pm_runtime_enable(cpu_dev);
+
+		ret = genpd_dev_pm_attach(cpu_dev);
+		if (ret) {
+			dev_warn(cpu_dev, "%s: Unable to attach to power-domain: %d\n", __func__, ret);
+			pm_runtime_disable(cpu_dev);
+		}
+	}
+
+	return 0;
+}
+device_initcall(arm_domain_cpu_init);
+
+static int arm_domain_init(void)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "arm,pd") {
+		struct arm_pm_domain *pd;
+		struct device *dev;
+
+		pdev = of_find_device_by_node(np);
+		dev = &pdev->dev;
+
+		pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+		if (!pd) {
+			pr_err("%s: failed to allocate memory for domain\n",
+					__func__);
+			return -ENOMEM;
+		}
+
+		pd->genpd.name = kstrdup(np->name, GFP_KERNEL);
+		pd->genpd.power_off = arm_pd_power_down;
+		pd->genpd.power_on = arm_pd_power_up;
+		platform_set_drvdata(pdev, pd);
+
+		dev_dbg(dev, "adding as generic power domain.\n");
+		pm_genpd_init(&pd->genpd, &simple_qos_governor, false);
+		of_genpd_add_provider_simple(np, &pd->genpd);
+	}
+
+	return 0;
+}
+arch_initcall(arm_domain_init);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 42ffb8b..02140e6 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -5,6 +5,7 @@
  *
  * This file is released under the GPLv2.
  */
+#define DEBUG
 
 #include <linux/kernel.h>
 #include <linux/io.h>
-- 
2.1.4

[PATCH RFC v2 04/16] WIP: ARM: PM domains for CPUs/clusters

[Lina Iyer]

From: Kevin Hilman <khilman@linaro.org>

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 534f27c..fc59876 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -61,6 +61,7 @@
 			reg = <0x0>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu1: cpu at 1 {
@@ -69,6 +70,7 @@
 			reg = <0x1>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu2: cpu at 2 {
@@ -77,6 +79,7 @@
 			reg = <0x2>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu3: cpu at 3 {
@@ -85,6 +88,7 @@
 			reg = <0x3>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			power-domains = <&big_cluster_pd>;
 		};
 
 		cpu4: cpu at 100 {
@@ -93,6 +97,7 @@
 			reg = <0x100>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu5: cpu at 101 {
@@ -101,6 +106,7 @@
 			reg = <0x101>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu6: cpu at 102 {
@@ -109,6 +115,7 @@
 			reg = <0x102>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 
 		cpu7: cpu at 103 {
@@ -117,6 +124,7 @@
 			reg = <0x103>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			power-domains = <&little_cluster_pd>;
 		};
 	};
 
@@ -249,6 +257,16 @@
 		};
 	};
 
+	big_cluster_pd: big_cluster {
+		compatible = "arm,pd";
+		#power-domain-cells = <0>;
+	};
+
+	little_cluster_pd: little_cluster {
+		compatible = "arm,pd";
+ 		#power-domain-cells = <0>;
+	};
+
 	gsc_pd: power-domain at 10044000 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044000 0x20>;
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index e69f7a1..98ce19c 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -78,6 +78,7 @@ CFLAGS_pj4-cp0.o		:= -marm
 AFLAGS_iwmmxt.o			:= -Wa,-mcpu=iwmmxt
 obj-$(CONFIG_ARM_CPU_TOPOLOGY)  += topology.o
 obj-$(CONFIG_VDSO)		+= vdso.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += domains.o
 
 ifneq ($(CONFIG_ARCH_EBSA110),y)
   obj-y		+= io.o
diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
new file mode 100644
index 0000000..8388f54
--- /dev/null
+++ b/arch/arm/kernel/domains.c
@@ -0,0 +1,122 @@
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/of_platform.h>
+#include <linux/device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define NAME_MAX 16
+
+struct arm_pm_domain {
+	struct generic_pm_domain genpd;
+};
+
+static inline
+struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
+{
+	return container_of(d, struct arm_pm_domain, genpd);
+}
+
+static int arm_pd_power_down(struct generic_pm_domain *genpd)
+{
+	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
+	return 0;
+}
+
+static int arm_pd_power_up(struct generic_pm_domain *genpd)
+{
+	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
+	return 0;
+}
+
+static int arm_domain_cpu_init(void)
+{
+	int cpuid, ret = 0;
+
+	/* Find any CPU nodes with a phandle to this power domain */
+	for_each_possible_cpu(cpuid) {
+		struct device *cpu_dev;
+
+		/* FIXME: this is open-coding of_cpu_device_node_get(), but I want handle to cpu_dev */
+		cpu_dev = get_cpu_device(cpuid);
+		if (!cpu_dev) {
+ 			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
+			return -ENODEV;
+		}
+
+		/* 
+		 * HACK: genpd checks if devices are runtime_suspended
+		 * before doing a poweroff of the domain.  However, that check
+		 * assumes that that device has a driver.  Since CPU devices don't
+		 * have a driver, genpd assumes that the device is runtime_suspended
+		 * and will power off the domain as soon as the any device 
+		 * in the domain does a runtime_suspend.
+		 *
+		 * c.f. the following code in pm_genpd_poweroff():
+		 *
+		 * if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
+		 *      || pdd->dev->power.irq_safe))
+		 *  	not_suspended++;
+		 *
+		 * Just removing the pdd->dev->driver check would also work, 
+		 * but not sure if that's right in the general case.
+		 */
+		cpu_dev->driver = kzalloc(sizeof(struct device_driver), GFP_KERNEL);
+		WARN_ON(!cpu_dev->driver);
+
+		if (cpu_online(cpuid)) {
+			pm_runtime_set_active(cpu_dev);
+			pm_runtime_get_noresume(cpu_dev);
+		} else {
+			pm_runtime_set_suspended(cpu_dev);
+		}
+		pm_runtime_irq_safe(cpu_dev);
+		pm_runtime_enable(cpu_dev);
+
+		ret = genpd_dev_pm_attach(cpu_dev);
+		if (ret) {
+			dev_warn(cpu_dev, "%s: Unable to attach to power-domain: %d\n", __func__, ret);
+			pm_runtime_disable(cpu_dev);
+		}
+	}
+
+	return 0;
+}
+device_initcall(arm_domain_cpu_init);
+
+static int arm_domain_init(void)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "arm,pd") {
+		struct arm_pm_domain *pd;
+		struct device *dev;
+
+		pdev = of_find_device_by_node(np);
+		dev = &pdev->dev;
+
+		pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+		if (!pd) {
+			pr_err("%s: failed to allocate memory for domain\n",
+					__func__);
+			return -ENOMEM;
+		}
+
+		pd->genpd.name = kstrdup(np->name, GFP_KERNEL);
+		pd->genpd.power_off = arm_pd_power_down;
+		pd->genpd.power_on = arm_pd_power_up;
+		platform_set_drvdata(pdev, pd);
+
+		dev_dbg(dev, "adding as generic power domain.\n");
+		pm_genpd_init(&pd->genpd, &simple_qos_governor, false);
+		of_genpd_add_provider_simple(np, &pd->genpd);
+	}
+
+	return 0;
+}
+arch_initcall(arm_domain_init);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 42ffb8b..02140e6 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -5,6 +5,7 @@
  *
  * This file is released under the GPLv2.
  */
+#define DEBUG
 
 #include <linux/kernel.h>
 #include <linux/io.h>
-- 
2.1.4

[PATCH RFC v2 05/16] arm: domain: Remove hack to add dev->driver.

[Lina Iyer]

Fixed in Generic PM domains framework.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 8388f54..3248409 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -48,26 +48,6 @@ static int arm_domain_cpu_init(void)
 			return -ENODEV;
 		}
 
-		/* 
-		 * HACK: genpd checks if devices are runtime_suspended
-		 * before doing a poweroff of the domain.  However, that check
-		 * assumes that that device has a driver.  Since CPU devices don't
-		 * have a driver, genpd assumes that the device is runtime_suspended
-		 * and will power off the domain as soon as the any device 
-		 * in the domain does a runtime_suspend.
-		 *
-		 * c.f. the following code in pm_genpd_poweroff():
-		 *
-		 * if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		 *      || pdd->dev->power.irq_safe))
-		 *  	not_suspended++;
-		 *
-		 * Just removing the pdd->dev->driver check would also work, 
-		 * but not sure if that's right in the general case.
-		 */
-		cpu_dev->driver = kzalloc(sizeof(struct device_driver), GFP_KERNEL);
-		WARN_ON(!cpu_dev->driver);
-
 		if (cpu_online(cpuid)) {
 			pm_runtime_set_active(cpu_dev);
 			pm_runtime_get_noresume(cpu_dev);
-- 
2.1.4

[PATCH RFC v2 05/16] arm: domain: Remove hack to add dev->driver.

[Lina Iyer]

Fixed in Generic PM domains framework.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 8388f54..3248409 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -48,26 +48,6 @@ static int arm_domain_cpu_init(void)
 			return -ENODEV;
 		}
 
-		/* 
-		 * HACK: genpd checks if devices are runtime_suspended
-		 * before doing a poweroff of the domain.  However, that check
-		 * assumes that that device has a driver.  Since CPU devices don't
-		 * have a driver, genpd assumes that the device is runtime_suspended
-		 * and will power off the domain as soon as the any device 
-		 * in the domain does a runtime_suspend.
-		 *
-		 * c.f. the following code in pm_genpd_poweroff():
-		 *
-		 * if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		 *      || pdd->dev->power.irq_safe))
-		 *  	not_suspended++;
-		 *
-		 * Just removing the pdd->dev->driver check would also work, 
-		 * but not sure if that's right in the general case.
-		 */
-		cpu_dev->driver = kzalloc(sizeof(struct device_driver), GFP_KERNEL);
-		WARN_ON(!cpu_dev->driver);
-
 		if (cpu_online(cpuid)) {
 			pm_runtime_set_active(cpu_dev);
 			pm_runtime_get_noresume(cpu_dev);
-- 
2.1.4

[PATCH RFC v2 06/16] arm: domain: Make CPU genpd IRQ safe

[Lina Iyer]

Generic PM domains that support runtime suspend/resume in IRQ safe
contexts should define GENPD_FLAG_IRQ_SAFE flag for the genpd.

CPUIdle powers off the CPUs in an IRQ safe context and therefore CPU
domains that use CPUIlde for runtime suspend/resume would need to
operate as IRQ safe domains. Define GENPD_FLAG_IRQ_SAFE for CPU domains.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 3248409..a27f825 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -90,6 +90,7 @@ static int arm_domain_init(void)
 		pd->genpd.name = kstrdup(np->name, GFP_KERNEL);
 		pd->genpd.power_off = arm_pd_power_down;
 		pd->genpd.power_on = arm_pd_power_up;
+		pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
 		platform_set_drvdata(pdev, pd);
 
 		dev_dbg(dev, "adding as generic power domain.\n");
-- 
2.1.4

[PATCH RFC v2 06/16] arm: domain: Make CPU genpd IRQ safe

[Lina Iyer]

Generic PM domains that support runtime suspend/resume in IRQ safe
contexts should define GENPD_FLAG_IRQ_SAFE flag for the genpd.

CPUIdle powers off the CPUs in an IRQ safe context and therefore CPU
domains that use CPUIlde for runtime suspend/resume would need to
operate as IRQ safe domains. Define GENPD_FLAG_IRQ_SAFE for CPU domains.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 3248409..a27f825 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -90,6 +90,7 @@ static int arm_domain_init(void)
 		pd->genpd.name = kstrdup(np->name, GFP_KERNEL);
 		pd->genpd.power_off = arm_pd_power_down;
 		pd->genpd.power_on = arm_pd_power_up;
+		pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
 		platform_set_drvdata(pdev, pd);
 
 		dev_dbg(dev, "adding as generic power domain.\n");
-- 
2.1.4

[PATCH RFC v2 07/16] arm: domain: Synchronize CPU device runtime PM usage with idle state

[Lina Iyer]

When a CPU is running, the runtime PM usage count should be incremented
and decremented when the CPU is powers down. To ensure that this happens
correctly, increment the runtime usage from that CPU.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index a27f825..59b0180 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -33,6 +33,14 @@ static int arm_pd_power_up(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static void run_cpu(void *unused)
+{
+	struct device *cpu_dev = get_cpu_device(smp_processor_id());
+
+	/* We are running, increment the usage count */
+	pm_runtime_get_noresume(cpu_dev);
+}
+
 static int arm_domain_cpu_init(void)
 {
 	int cpuid, ret = 0;
@@ -50,13 +58,27 @@ static int arm_domain_cpu_init(void)
 
 		if (cpu_online(cpuid)) {
 			pm_runtime_set_active(cpu_dev);
-			pm_runtime_get_noresume(cpu_dev);
+			/*
+			 * Execute the below on that 'cpu' to ensure that the reference
+			 * counting is correct. Its possible that while this code is
+			 * executing, the 'cpu' may be powerd down, but we may incorrectly
+			 * increment the usage. By executing the get_cpu on the 'cpu',
+			 * we can ensure that the 'cpu' and its usage count are matched.
+			 */
+			smp_call_function_single(cpuid, run_cpu, NULL, true);
 		} else {
 			pm_runtime_set_suspended(cpu_dev);
 		}
 		pm_runtime_irq_safe(cpu_dev);
 		pm_runtime_enable(cpu_dev);
 
+		/*
+		 * We attempt to attach the device to genpd again. We would
+		 * have failed in our earlier attempt to attach to the domain
+		 * provider as the CPU device would not have been IRQ safe,
+		 * while the domain is defined as IRQ safe. IRQ safe domains
+		 * can only have IRQ safe devices.
+		 */
 		ret = genpd_dev_pm_attach(cpu_dev);
 		if (ret) {
 			dev_warn(cpu_dev, "%s: Unable to attach to power-domain: %d\n", __func__, ret);
-- 
2.1.4

[PATCH RFC v2 07/16] arm: domain: Synchronize CPU device runtime PM usage with idle state

[Lina Iyer]

When a CPU is running, the runtime PM usage count should be incremented
and decremented when the CPU is powers down. To ensure that this happens
correctly, increment the runtime usage from that CPU.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index a27f825..59b0180 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -33,6 +33,14 @@ static int arm_pd_power_up(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static void run_cpu(void *unused)
+{
+	struct device *cpu_dev = get_cpu_device(smp_processor_id());
+
+	/* We are running, increment the usage count */
+	pm_runtime_get_noresume(cpu_dev);
+}
+
 static int arm_domain_cpu_init(void)
 {
 	int cpuid, ret = 0;
@@ -50,13 +58,27 @@ static int arm_domain_cpu_init(void)
 
 		if (cpu_online(cpuid)) {
 			pm_runtime_set_active(cpu_dev);
-			pm_runtime_get_noresume(cpu_dev);
+			/*
+			 * Execute the below on that 'cpu' to ensure that the reference
+			 * counting is correct. Its possible that while this code is
+			 * executing, the 'cpu' may be powerd down, but we may incorrectly
+			 * increment the usage. By executing the get_cpu on the 'cpu',
+			 * we can ensure that the 'cpu' and its usage count are matched.
+			 */
+			smp_call_function_single(cpuid, run_cpu, NULL, true);
 		} else {
 			pm_runtime_set_suspended(cpu_dev);
 		}
 		pm_runtime_irq_safe(cpu_dev);
 		pm_runtime_enable(cpu_dev);
 
+		/*
+		 * We attempt to attach the device to genpd again. We would
+		 * have failed in our earlier attempt to attach to the domain
+		 * provider as the CPU device would not have been IRQ safe,
+		 * while the domain is defined as IRQ safe. IRQ safe domains
+		 * can only have IRQ safe devices.
+		 */
 		ret = genpd_dev_pm_attach(cpu_dev);
 		if (ret) {
 			dev_warn(cpu_dev, "%s: Unable to attach to power-domain: %d\n", __func__, ret);
-- 
2.1.4

[PATCH RFC v2 08/16] arm: domain: Handle CPU online reference counting

[Lina Iyer]

Ensure that the CPU device usage count is incremented for a CPU that
just came online. CPUs coming online do not follow the same code path as
CPUs warm booting back to CPUIdle. Register for hotplug notifier and
ensure the reference count is incremented correctly.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 59b0180..680c3fb 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -41,10 +41,49 @@ static void run_cpu(void *unused)
 	pm_runtime_get_noresume(cpu_dev);
 }
 
+static int cpu_online_notifier(struct notifier_block *n,
+		unsigned long action, void *hcpu)
+{
+	int cpu = (unsigned long)hcpu;
+	struct device *dev = get_cpu_device(cpu);
+
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		/*
+		 * Attach the cpu to its domain if the cpu is coming up
+		 * for the first time.
+		 * Called from the cpu that is coming up.
+		 */
+		pm_runtime_set_active(dev);
+		pm_runtime_get_noresume(dev);
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block hotplug_notifier = {
+	.notifier_call = cpu_online_notifier,
+};
+
 static int arm_domain_cpu_init(void)
 {
 	int cpuid, ret = 0;
 
+	/*
+	 * Register for hotplug notifier, because not all CPUs
+	 * may be online at this time. Also, hotplug entry path is not
+	 * the same as warm boot. So the reference counting would have
+	 * to be adjusted here accordingly.
+	 * Register early, incase, a core comes online,
+	 * while we are executing this.
+	 */
+	register_cpu_notifier(&hotplug_notifier);
+
 	/* Find any CPU nodes with a phandle to this power domain */
 	for_each_possible_cpu(cpuid) {
 		struct device *cpu_dev;
-- 
2.1.4

[PATCH RFC v2 08/16] arm: domain: Handle CPU online reference counting

[Lina Iyer]

Ensure that the CPU device usage count is incremented for a CPU that
just came online. CPUs coming online do not follow the same code path as
CPUs warm booting back to CPUIdle. Register for hotplug notifier and
ensure the reference count is incremented correctly.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/domains.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 59b0180..680c3fb 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -41,10 +41,49 @@ static void run_cpu(void *unused)
 	pm_runtime_get_noresume(cpu_dev);
 }
 
+static int cpu_online_notifier(struct notifier_block *n,
+		unsigned long action, void *hcpu)
+{
+	int cpu = (unsigned long)hcpu;
+	struct device *dev = get_cpu_device(cpu);
+
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		/*
+		 * Attach the cpu to its domain if the cpu is coming up
+		 * for the first time.
+		 * Called from the cpu that is coming up.
+		 */
+		pm_runtime_set_active(dev);
+		pm_runtime_get_noresume(dev);
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block hotplug_notifier = {
+	.notifier_call = cpu_online_notifier,
+};
+
 static int arm_domain_cpu_init(void)
 {
 	int cpuid, ret = 0;
 
+	/*
+	 * Register for hotplug notifier, because not all CPUs
+	 * may be online at this time. Also, hotplug entry path is not
+	 * the same as warm boot. So the reference counting would have
+	 * to be adjusted here accordingly.
+	 * Register early, incase, a core comes online,
+	 * while we are executing this.
+	 */
+	register_cpu_notifier(&hotplug_notifier);
+
 	/* Find any CPU nodes with a phandle to this power domain */
 	for_each_possible_cpu(cpuid) {
 		struct device *cpu_dev;
-- 
2.1.4

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

Platform drivers may have additional setup inorder before the domain can
be powered off. Allow, platform drivers to register power on/off
callbacks against a domain provider.

While registering the callback ensure that the domain is neither in
power on/off state. The domain should be active. To ensure that the
platform callback registration doesntrace with genpd power on/off,
execute the registration from a CPU on that domain.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/include/asm/cpu.h       |  1 -
 arch/arm/include/asm/pm_domain.h | 27 ++++++++++++++
 arch/arm/kernel/domains.c        | 81 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 107 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/include/asm/pm_domain.h

diff --git a/arch/arm/include/asm/cpu.h b/arch/arm/include/asm/cpu.h
index 2744f06..4a7e346 100644
--- a/arch/arm/include/asm/cpu.h
+++ b/arch/arm/include/asm/cpu.h
@@ -22,5 +22,4 @@ struct cpuinfo_arm {
 };
 
 DECLARE_PER_CPU(struct cpuinfo_arm, cpu_data);
-
 #endif
diff --git a/arch/arm/include/asm/pm_domain.h b/arch/arm/include/asm/pm_domain.h
new file mode 100644
index 0000000..d13c291
--- /dev/null
+++ b/arch/arm/include/asm/pm_domain.h
@@ -0,0 +1,27 @@
+/*
+ *  arch/arm/include/asm/pm_domain.h
+ *
+ *  Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __ASM_ARM_PM_DOMAIN_H
+#define __ASM_ARM_PM_DOMAIN_H
+
+#include <linux/pm_domain.h>
+
+#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
+extern int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*pd_down)(struct generic_pm_domain *),
+		int (*pd_up)(struct generic_pm_domain *));
+#else
+static inline
+int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*pd_down)(struct generic_pm_domain *),
+		int (*pd_up)(struct generic_pm_domain *))
+{ return -ENODEV; }
+#endif
+
+#endif
diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 680c3fb..2f5ba3f 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -9,10 +9,19 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
+#include <asm/pm_domain.h>
+
 #define NAME_MAX 16
 
+struct platform_cb {
+	int (*power_off)(struct generic_pm_domain *);
+	int (*power_on)(struct generic_pm_domain *);
+};
+
 struct arm_pm_domain {
 	struct generic_pm_domain genpd;
+	struct platform_cb plat_handler;
+	struct spinlock_t lock;
 };
 
 static inline
@@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
 
 static int arm_pd_power_down(struct generic_pm_domain *genpd)
 {
+	struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
+
+	if (arm_pd->plat_handler.power_off)
+		return arm_pd->plat_handler.power_off(genpd);
+
 	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
 	return 0;
 }
 
 static int arm_pd_power_up(struct generic_pm_domain *genpd)
 {
+	struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
+
+	if (arm_pd->plat_handler.power_on)
+		return arm_pd->plat_handler.power_on(genpd);
+
 	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
 	return 0;
 }
 
+static void __register_platform_domain_handlers(void *data)
+{
+	struct device *cpu_dev = get_cpu_device(smp_processor_id());
+	struct generic_pm_domain *genpd;
+	struct arm_pm_domain *arm_pd;
+	unsigned long flags;
+	struct platform_cb *handler = data;
+
+	genpd = pd_to_genpd(cpu_dev->pm_domain);
+	BUG_ON(IS_ERR_OR_NULL(genpd));
+	arm_pd = to_arm_pd(genpd);
+
+	/*
+	 * Lock to avoid race with other CPUs in the same domain
+	 * trying to set the function pointers.
+	 */
+	spin_lock_irqsave(&arm_pd->lock, flags);
+	arm_pd->plat_handler.power_on = handler->power_on;
+	arm_pd->plat_handler.power_off = handler->power_off;
+	spin_unlock_irqrestore(&arm_pd->lock, flags);
+}
+
+int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*power_on)(struct generic_pm_domain *),
+		int (*power_off)(struct generic_pm_domain *))
+{
+	struct platform_cb handler;
+	struct generic_pm_domain *genpd, *p;
+	struct device *cpu_dev;
+	int cpu;
+
+	handler.power_on = power_on;
+	handler.power_off = power_off;
+
+	genpd = of_genpd_get_from_provider(args);
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		cpu_dev = get_cpu_device(cpu);
+		p = pm_genpd_lookup_dev(cpu_dev);
+		/*
+		 * Execute the registration on the requested 'cpu', so we dont
+		 * race with the domain->power_off or ->power_on calls.
+		 * By executing on that 'cpu', we are assured that the 'cpu' is
+		 * awake, therefore the domain. This will avoid a spin lock
+		 * between power on/off calls and this function.
+		 */
+		if (p == genpd) {
+			smp_call_function_single(cpu,
+				__register_platform_domain_handlers,
+				&handler, true);
+			break;
+		}
+	}
+	put_online_cpus();
+
+	return 0;
+}
+EXPORT_SYMBOL(register_platform_domain_handlers);
+
 static void run_cpu(void *unused)
 {
 	struct device *cpu_dev = get_cpu_device(smp_processor_id());
@@ -91,7 +169,7 @@ static int arm_domain_cpu_init(void)
 		/* FIXME: this is open-coding of_cpu_device_node_get(), but I want handle to cpu_dev */
 		cpu_dev = get_cpu_device(cpuid);
 		if (!cpu_dev) {
- 			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
+			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
 			return -ENODEV;
 		}
 
@@ -152,6 +230,7 @@ static int arm_domain_init(void)
 		pd->genpd.power_off = arm_pd_power_down;
 		pd->genpd.power_on = arm_pd_power_up;
 		pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
+		spin_lock_init(&pd->lock);
 		platform_set_drvdata(pdev, pd);
 
 		dev_dbg(dev, "adding as generic power domain.\n");
-- 
2.1.4

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

Platform drivers may have additional setup inorder before the domain can
be powered off. Allow, platform drivers to register power on/off
callbacks against a domain provider.

While registering the callback ensure that the domain is neither in
power on/off state. The domain should be active. To ensure that the
platform callback registration doesntrace with genpd power on/off,
execute the registration from a CPU on that domain.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/include/asm/cpu.h       |  1 -
 arch/arm/include/asm/pm_domain.h | 27 ++++++++++++++
 arch/arm/kernel/domains.c        | 81 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 107 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/include/asm/pm_domain.h

diff --git a/arch/arm/include/asm/cpu.h b/arch/arm/include/asm/cpu.h
index 2744f06..4a7e346 100644
--- a/arch/arm/include/asm/cpu.h
+++ b/arch/arm/include/asm/cpu.h
@@ -22,5 +22,4 @@ struct cpuinfo_arm {
 };
 
 DECLARE_PER_CPU(struct cpuinfo_arm, cpu_data);
-
 #endif
diff --git a/arch/arm/include/asm/pm_domain.h b/arch/arm/include/asm/pm_domain.h
new file mode 100644
index 0000000..d13c291
--- /dev/null
+++ b/arch/arm/include/asm/pm_domain.h
@@ -0,0 +1,27 @@
+/*
+ *  arch/arm/include/asm/pm_domain.h
+ *
+ *  Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __ASM_ARM_PM_DOMAIN_H
+#define __ASM_ARM_PM_DOMAIN_H
+
+#include <linux/pm_domain.h>
+
+#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
+extern int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*pd_down)(struct generic_pm_domain *),
+		int (*pd_up)(struct generic_pm_domain *));
+#else
+static inline
+int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*pd_down)(struct generic_pm_domain *),
+		int (*pd_up)(struct generic_pm_domain *))
+{ return -ENODEV; }
+#endif
+
+#endif
diff --git a/arch/arm/kernel/domains.c b/arch/arm/kernel/domains.c
index 680c3fb..2f5ba3f 100644
--- a/arch/arm/kernel/domains.c
+++ b/arch/arm/kernel/domains.c
@@ -9,10 +9,19 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
+#include <asm/pm_domain.h>
+
 #define NAME_MAX 16
 
+struct platform_cb {
+	int (*power_off)(struct generic_pm_domain *);
+	int (*power_on)(struct generic_pm_domain *);
+};
+
 struct arm_pm_domain {
 	struct generic_pm_domain genpd;
+	struct platform_cb plat_handler;
+	struct spinlock_t lock;
 };
 
 static inline
@@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
 
 static int arm_pd_power_down(struct generic_pm_domain *genpd)
 {
+	struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
+
+	if (arm_pd->plat_handler.power_off)
+		return arm_pd->plat_handler.power_off(genpd);
+
 	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
 	return 0;
 }
 
 static int arm_pd_power_up(struct generic_pm_domain *genpd)
 {
+	struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
+
+	if (arm_pd->plat_handler.power_on)
+		return arm_pd->plat_handler.power_on(genpd);
+
 	/* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
 	return 0;
 }
 
+static void __register_platform_domain_handlers(void *data)
+{
+	struct device *cpu_dev = get_cpu_device(smp_processor_id());
+	struct generic_pm_domain *genpd;
+	struct arm_pm_domain *arm_pd;
+	unsigned long flags;
+	struct platform_cb *handler = data;
+
+	genpd = pd_to_genpd(cpu_dev->pm_domain);
+	BUG_ON(IS_ERR_OR_NULL(genpd));
+	arm_pd = to_arm_pd(genpd);
+
+	/*
+	 * Lock to avoid race with other CPUs in the same domain
+	 * trying to set the function pointers.
+	 */
+	spin_lock_irqsave(&arm_pd->lock, flags);
+	arm_pd->plat_handler.power_on = handler->power_on;
+	arm_pd->plat_handler.power_off = handler->power_off;
+	spin_unlock_irqrestore(&arm_pd->lock, flags);
+}
+
+int register_platform_domain_handlers(struct of_phandle_args *args,
+		int (*power_on)(struct generic_pm_domain *),
+		int (*power_off)(struct generic_pm_domain *))
+{
+	struct platform_cb handler;
+	struct generic_pm_domain *genpd, *p;
+	struct device *cpu_dev;
+	int cpu;
+
+	handler.power_on = power_on;
+	handler.power_off = power_off;
+
+	genpd = of_genpd_get_from_provider(args);
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		cpu_dev = get_cpu_device(cpu);
+		p = pm_genpd_lookup_dev(cpu_dev);
+		/*
+		 * Execute the registration on the requested 'cpu', so we dont
+		 * race with the domain->power_off or ->power_on calls.
+		 * By executing on that 'cpu', we are assured that the 'cpu' is
+		 * awake, therefore the domain. This will avoid a spin lock
+		 * between power on/off calls and this function.
+		 */
+		if (p == genpd) {
+			smp_call_function_single(cpu,
+				__register_platform_domain_handlers,
+				&handler, true);
+			break;
+		}
+	}
+	put_online_cpus();
+
+	return 0;
+}
+EXPORT_SYMBOL(register_platform_domain_handlers);
+
 static void run_cpu(void *unused)
 {
 	struct device *cpu_dev = get_cpu_device(smp_processor_id());
@@ -91,7 +169,7 @@ static int arm_domain_cpu_init(void)
 		/* FIXME: this is open-coding of_cpu_device_node_get(), but I want handle to cpu_dev */
 		cpu_dev = get_cpu_device(cpuid);
 		if (!cpu_dev) {
- 			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
+			pr_warn("%s: Unable to get device for CPU%d\n", __func__, cpuid);
 			return -ENODEV;
 		}
 
@@ -152,6 +230,7 @@ static int arm_domain_init(void)
 		pd->genpd.power_off = arm_pd_power_down;
 		pd->genpd.power_on = arm_pd_power_up;
 		pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
+		spin_lock_init(&pd->lock);
 		platform_set_drvdata(pdev, pd);
 
 		dev_dbg(dev, "adding as generic power domain.\n");
-- 
2.1.4

[PATCH RFC v2 10/16] drivers: cpuidle: Add runtime PM support for CPUIdle

[Lina Iyer]

Notify runtime PM when the CPU is going to be powered off in the idle
state. This allows for runtime PM suspend/resume of the CPU's PM domain.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/cpuidle/cpuidle-arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..77fd673 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -11,12 +11,14 @@
 
 #define pr_fmt(fmt) "CPUidle arm: " fmt
 
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <asm/cpuidle.h>
@@ -45,12 +47,20 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 
 	ret = cpu_pm_enter();
 	if (!ret) {
+		struct device *cpu_dev = get_cpu_device(dev->cpu);
+
+		/*
+		 * Notify runtime PM as well of this cpu powering down
+		 * TODO: Merge CPU_PM and runtime PM.
+		 */
+		pm_runtime_put_sync(cpu_dev);
 		/*
 		 * Pass idle state index to cpu_suspend which in turn will
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
 		arm_cpuidle_suspend(idx);
+		pm_runtime_get_sync(cpu_dev);
 
 		cpu_pm_exit();
 	}
@@ -146,6 +156,7 @@ static int __init arm_idle_init(void)
 			kfree(dev);
 			goto out_fail;
 		}
+
 	}
 
 	return 0;
-- 
2.1.4

[PATCH RFC v2 10/16] drivers: cpuidle: Add runtime PM support for CPUIdle

[Lina Iyer]

Notify runtime PM when the CPU is going to be powered off in the idle
state. This allows for runtime PM suspend/resume of the CPU's PM domain.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/cpuidle/cpuidle-arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..77fd673 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -11,12 +11,14 @@
 
 #define pr_fmt(fmt) "CPUidle arm: " fmt
 
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <asm/cpuidle.h>
@@ -45,12 +47,20 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 
 	ret = cpu_pm_enter();
 	if (!ret) {
+		struct device *cpu_dev = get_cpu_device(dev->cpu);
+
+		/*
+		 * Notify runtime PM as well of this cpu powering down
+		 * TODO: Merge CPU_PM and runtime PM.
+		 */
+		pm_runtime_put_sync(cpu_dev);
 		/*
 		 * Pass idle state index to cpu_suspend which in turn will
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
 		arm_cpuidle_suspend(idx);
+		pm_runtime_get_sync(cpu_dev);
 
 		cpu_pm_exit();
 	}
@@ -146,6 +156,7 @@ static int __init arm_idle_init(void)
 			kfree(dev);
 			goto out_fail;
 		}
+
 	}
 
 	return 0;
-- 
2.1.4

[PATCH RFC v2 11/16] drivers: qcom: spm: Support cache and coherency SPMs

[Lina Iyer]

Recognize non-CPU SPM devices defined in the DT and configure the
corresponding SPM hardware. SPM controllers for L2 controls the cache's
idle low power state and may also be used to turn off the cluster's
power rail.

On multi-cluster SoCs, each cluster would have an SPM for the cache and
an additional coherency level SPM. The coherency SPM turns off or puts
the coherency hardware in idle and any caches present at that level.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/spm.c | 75 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index b04b05a..bef2dfe1 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -68,9 +68,17 @@ struct spm_reg_data {
 	u8 start_index[PM_SLEEP_MODE_NR];
 };
 
+struct cluster;
+
 struct spm_driver_data {
 	void __iomem *reg_base;
 	const struct spm_reg_data *reg_data;
+	struct cluster *domain;
+};
+
+/* Group for domain entities */
+struct cluster {
+	struct spm_driver_data *domain_spm;
 };
 
 static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
@@ -116,6 +124,9 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
 
 static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
 
+/* 3 instances: little, big and coherency (cluster of clusters) */
+static struct cluster cpu_domain[3];
+
 typedef int (*idle_fn)(int);
 static DEFINE_PER_CPU(idle_fn*, qcom_idle_ops);
 
@@ -282,14 +293,26 @@ static struct cpuidle_ops qcom_cpuidle_ops __initdata = {
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
 
+/* Match L2 SPM with their affinity level */
+static const struct of_device_id cache_spm_table[] = {
+	{ },
+};
+
 static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
-		int *spm_cpu)
+		int *index, bool *is_domain_spm)
 {
 	struct spm_driver_data *drv = NULL;
-	struct device_node *cpu_node, *saw_node;
+	struct device_node *cpu_node, *saw_node, *cache_node;
 	int cpu;
 	bool found;
+	const struct of_device_id *match_id;
+	int idx;
 
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return ERR_PTR(-ENOMEM);
+
+	/* Check for a CPU SPM, if found we are done */
 	for_each_possible_cpu(cpu) {
 		cpu_node = of_cpu_device_node_get(cpu);
 		if (!cpu_node)
@@ -302,10 +325,37 @@ static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
 			break;
 	}
 
+	/* 
+	 * If found, we have a CPU SPM, if not,
+	 * check if we have a cache SPM
+	 */
 	if (found) {
-		drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
-		if (drv)
-			*spm_cpu = cpu;
+		/*
+		 * Now that we have our CPU, find the associated L2
+		 * SAW and bind the CPU with the domain that contains
+		 * the L2 SAW.
+		 */
+		cache_node = of_parse_phandle(cpu_node,
+				"next-level-cache", 0);
+		saw_node = of_parse_phandle(cache_node, "qcom,saw", 0);
+		match_id = of_match_node(cache_spm_table, saw_node);
+		if (match_id) {
+			idx = (int) match_id->data;
+			drv->domain = &cpu_domain[idx];
+		}
+		of_node_put(saw_node);
+		of_node_put(cache_node);
+		*index = cpu;
+		*is_domain_spm = false;
+	} else {
+		/* Check if this is a cache SPM */
+		match_id = of_match_node(cache_spm_table, pdev->dev.of_node);
+		if (!match_id) {
+			devm_kfree(&pdev->dev, drv);
+			return ERR_PTR(-ENODEV);
+		}
+		*index = (int) match_id->data;
+		*is_domain_spm = true;
 	}
 
 	return drv;
@@ -327,11 +377,12 @@ static int spm_dev_probe(struct platform_device *pdev)
 	struct resource *res;
 	const struct of_device_id *match_id;
 	void __iomem *addr;
-	int cpu;
+	int index;
+	bool is_domain_spm;
 
-	drv = spm_get_drv(pdev, &cpu);
-	if (!drv)
-		return -EINVAL;
+	drv = spm_get_drv(pdev, &index, &is_domain_spm);
+	if (IS_ERR(drv))
+		return PTR_ERR(drv);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
@@ -366,7 +417,11 @@ static int spm_dev_probe(struct platform_device *pdev)
 	/* Set up Standby as the default low power mode */
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_STBY);
 
-	per_cpu(cpu_spm_drv, cpu) = drv;
+	/* We are ready to use the CPU/Cache SPM. */
+	if (is_domain_spm)
+		cpu_domain[index].domain_spm = drv;
+	else
+		per_cpu(cpu_spm_drv, index) = drv;
 
 	return 0;
 }
-- 
2.1.4

[PATCH RFC v2 11/16] drivers: qcom: spm: Support cache and coherency SPMs

[Lina Iyer]

Recognize non-CPU SPM devices defined in the DT and configure the
corresponding SPM hardware. SPM controllers for L2 controls the cache's
idle low power state and may also be used to turn off the cluster's
power rail.

On multi-cluster SoCs, each cluster would have an SPM for the cache and
an additional coherency level SPM. The coherency SPM turns off or puts
the coherency hardware in idle and any caches present at that level.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/spm.c | 75 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index b04b05a..bef2dfe1 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -68,9 +68,17 @@ struct spm_reg_data {
 	u8 start_index[PM_SLEEP_MODE_NR];
 };
 
+struct cluster;
+
 struct spm_driver_data {
 	void __iomem *reg_base;
 	const struct spm_reg_data *reg_data;
+	struct cluster *domain;
+};
+
+/* Group for domain entities */
+struct cluster {
+	struct spm_driver_data *domain_spm;
 };
 
 static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
@@ -116,6 +124,9 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
 
 static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
 
+/* 3 instances: little, big and coherency (cluster of clusters) */
+static struct cluster cpu_domain[3];
+
 typedef int (*idle_fn)(int);
 static DEFINE_PER_CPU(idle_fn*, qcom_idle_ops);
 
@@ -282,14 +293,26 @@ static struct cpuidle_ops qcom_cpuidle_ops __initdata = {
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
 
+/* Match L2 SPM with their affinity level */
+static const struct of_device_id cache_spm_table[] = {
+	{ },
+};
+
 static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
-		int *spm_cpu)
+		int *index, bool *is_domain_spm)
 {
 	struct spm_driver_data *drv = NULL;
-	struct device_node *cpu_node, *saw_node;
+	struct device_node *cpu_node, *saw_node, *cache_node;
 	int cpu;
 	bool found;
+	const struct of_device_id *match_id;
+	int idx;
 
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return ERR_PTR(-ENOMEM);
+
+	/* Check for a CPU SPM, if found we are done */
 	for_each_possible_cpu(cpu) {
 		cpu_node = of_cpu_device_node_get(cpu);
 		if (!cpu_node)
@@ -302,10 +325,37 @@ static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
 			break;
 	}
 
+	/* 
+	 * If found, we have a CPU SPM, if not,
+	 * check if we have a cache SPM
+	 */
 	if (found) {
-		drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
-		if (drv)
-			*spm_cpu = cpu;
+		/*
+		 * Now that we have our CPU, find the associated L2
+		 * SAW and bind the CPU with the domain that contains
+		 * the L2 SAW.
+		 */
+		cache_node = of_parse_phandle(cpu_node,
+				"next-level-cache", 0);
+		saw_node = of_parse_phandle(cache_node, "qcom,saw", 0);
+		match_id = of_match_node(cache_spm_table, saw_node);
+		if (match_id) {
+			idx = (int) match_id->data;
+			drv->domain = &cpu_domain[idx];
+		}
+		of_node_put(saw_node);
+		of_node_put(cache_node);
+		*index = cpu;
+		*is_domain_spm = false;
+	} else {
+		/* Check if this is a cache SPM */
+		match_id = of_match_node(cache_spm_table, pdev->dev.of_node);
+		if (!match_id) {
+			devm_kfree(&pdev->dev, drv);
+			return ERR_PTR(-ENODEV);
+		}
+		*index = (int) match_id->data;
+		*is_domain_spm = true;
 	}
 
 	return drv;
@@ -327,11 +377,12 @@ static int spm_dev_probe(struct platform_device *pdev)
 	struct resource *res;
 	const struct of_device_id *match_id;
 	void __iomem *addr;
-	int cpu;
+	int index;
+	bool is_domain_spm;
 
-	drv = spm_get_drv(pdev, &cpu);
-	if (!drv)
-		return -EINVAL;
+	drv = spm_get_drv(pdev, &index, &is_domain_spm);
+	if (IS_ERR(drv))
+		return PTR_ERR(drv);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
@@ -366,7 +417,11 @@ static int spm_dev_probe(struct platform_device *pdev)
 	/* Set up Standby as the default low power mode */
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_STBY);
 
-	per_cpu(cpu_spm_drv, cpu) = drv;
+	/* We are ready to use the CPU/Cache SPM. */
+	if (is_domain_spm)
+		cpu_domain[index].domain_spm = drv;
+	else
+		per_cpu(cpu_spm_drv, index) = drv;
 
 	return 0;
 }
-- 
2.1.4

[PATCH RFC v2 12/16] drivers: qcom: spm: Enable runtime suspend/resume of CPU PM domain

[Lina Iyer]

On APQ8084 QCOM SoC's, the CPUs are powered by a single rail controlled
by the L2 cache power controller (L2 SPM). The L2 power domain supplies
power to all the CPUs and L2. It is safe to power down the domain when
all the CPUs and the L2 are powered down.

Powering down of the domain is done through the finite state machine on
the L2 SAW. The L2 SPM can be configured to enter an idle state, when
all CPUs enter their idle state. The L2 SPM state machine would turn off
the cache and possibly power off the power domain as well. The SPM also
guarantees that the h/w is ready for the CPU to resume, when woken up by
an interrupt.

Define a cluster that holds the SPM and possibly other common cluster
elements. The L2 SAW is also the genpd domain provider and the CPUs are
the devices attached to the domain. When CPUIdle powers down each CPU,
the ARM domain framework would callback to notify that the domain may be
powered off. Configure the L2 SPM at that time to flush the L2 cache and
turn off the CPU power rail.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/spm.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index bef2dfe1..ad1498e 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -24,9 +24,12 @@
 #include <linux/platform_device.h>
 #include <linux/cpuidle.h>
 #include <linux/cpu_pm.h>
+#include <linux/pm_domain.h>
 #include <linux/qcom_scm.h>
 
+#include <asm/cacheflush.h>
 #include <asm/cpuidle.h>
+#include <asm/pm_domain.h>
 #include <asm/proc-fns.h>
 #include <asm/suspend.h>
 
@@ -79,6 +82,7 @@ struct spm_driver_data {
 /* Group for domain entities */
 struct cluster {
 	struct spm_driver_data *domain_spm;
+	bool domain_off;
 };
 
 static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
@@ -181,7 +185,23 @@ static void spm_set_low_power_mode(struct spm_driver_data *drv,
 
 static int qcom_pm_collapse(unsigned long int unused)
 {
-	qcom_scm_cpu_power_down(QCOM_SCM_CPU_PWR_DOWN_L2_ON);
+	int flags = QCOM_SCM_CPU_PWR_DOWN_L2_ON;
+	int cpu = smp_processor_id();
+	bool domain_off;
+
+	/*
+	 * Check if the CPU domain will power off after this CPU
+	 * enters idle.
+	 * L2 cache may be turned off when the domain powers off,
+	 * flush the non-secure cache before calling into secure.
+	 */
+	domain_off = per_cpu(cpu_spm_drv, cpu)->domain->domain_off;
+	if (domain_off) {
+		flags = QCOM_SCM_CPU_PWR_DOWN_L2_OFF;
+		flush_cache_all();
+	}
+
+	qcom_scm_cpu_power_down(flags);
 
 	/*
 	 * Returns here only if there was a pending interrupt and we did not
@@ -293,6 +313,35 @@ static struct cpuidle_ops qcom_cpuidle_ops __initdata = {
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
 
+static int pd_power_on(struct generic_pm_domain *domain)
+{
+	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, smp_processor_id());
+	struct cluster *pmd = drv->domain;
+
+	if (!pmd || !pmd->domain_spm)
+		return 0;
+
+	pmd->domain_off = false;
+	spm_set_low_power_mode(pmd->domain_spm, PM_SLEEP_MODE_STBY);
+
+	return 0;
+}
+
+static int pd_power_off(struct generic_pm_domain *domain)
+{
+	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, smp_processor_id());
+	struct cluster *pmd = drv->domain;
+
+	if (!pmd || !pmd->domain_spm)
+		return 0;
+
+	pmd->domain_off = true;
+	spm_set_low_power_mode(pmd->domain_spm, PM_SLEEP_MODE_SPC);
+
+	return 0;
+}
+
+
 /* Match L2 SPM with their affinity level */
 static const struct of_device_id cache_spm_table[] = {
 	{ },
@@ -418,8 +467,18 @@ static int spm_dev_probe(struct platform_device *pdev)
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_STBY);
 
 	/* We are ready to use the CPU/Cache SPM. */
-	if (is_domain_spm)
+	if (is_domain_spm) {
+		struct of_phandle_args args;
+		int ret;
+
+		args.np = pdev->dev.of_node;
+		args.args_count = 0;
+		ret = register_platform_domain_handlers(&args,
+				pd_power_on, pd_power_off);
+		if (ret)
+			dev_dbg(&pdev->dev, "Domain callback not registered\n");
 		cpu_domain[index].domain_spm = drv;
+	}
 	else
 		per_cpu(cpu_spm_drv, index) = drv;
 
-- 
2.1.4

[PATCH RFC v2 12/16] drivers: qcom: spm: Enable runtime suspend/resume of CPU PM domain

[Lina Iyer]

On APQ8084 QCOM SoC's, the CPUs are powered by a single rail controlled
by the L2 cache power controller (L2 SPM). The L2 power domain supplies
power to all the CPUs and L2. It is safe to power down the domain when
all the CPUs and the L2 are powered down.

Powering down of the domain is done through the finite state machine on
the L2 SAW. The L2 SPM can be configured to enter an idle state, when
all CPUs enter their idle state. The L2 SPM state machine would turn off
the cache and possibly power off the power domain as well. The SPM also
guarantees that the h/w is ready for the CPU to resume, when woken up by
an interrupt.

Define a cluster that holds the SPM and possibly other common cluster
elements. The L2 SAW is also the genpd domain provider and the CPUs are
the devices attached to the domain. When CPUIdle powers down each CPU,
the ARM domain framework would callback to notify that the domain may be
powered off. Configure the L2 SPM at that time to flush the L2 cache and
turn off the CPU power rail.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/spm.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index bef2dfe1..ad1498e 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -24,9 +24,12 @@
 #include <linux/platform_device.h>
 #include <linux/cpuidle.h>
 #include <linux/cpu_pm.h>
+#include <linux/pm_domain.h>
 #include <linux/qcom_scm.h>
 
+#include <asm/cacheflush.h>
 #include <asm/cpuidle.h>
+#include <asm/pm_domain.h>
 #include <asm/proc-fns.h>
 #include <asm/suspend.h>
 
@@ -79,6 +82,7 @@ struct spm_driver_data {
 /* Group for domain entities */
 struct cluster {
 	struct spm_driver_data *domain_spm;
+	bool domain_off;
 };
 
 static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
@@ -181,7 +185,23 @@ static void spm_set_low_power_mode(struct spm_driver_data *drv,
 
 static int qcom_pm_collapse(unsigned long int unused)
 {
-	qcom_scm_cpu_power_down(QCOM_SCM_CPU_PWR_DOWN_L2_ON);
+	int flags = QCOM_SCM_CPU_PWR_DOWN_L2_ON;
+	int cpu = smp_processor_id();
+	bool domain_off;
+
+	/*
+	 * Check if the CPU domain will power off after this CPU
+	 * enters idle.
+	 * L2 cache may be turned off when the domain powers off,
+	 * flush the non-secure cache before calling into secure.
+	 */
+	domain_off = per_cpu(cpu_spm_drv, cpu)->domain->domain_off;
+	if (domain_off) {
+		flags = QCOM_SCM_CPU_PWR_DOWN_L2_OFF;
+		flush_cache_all();
+	}
+
+	qcom_scm_cpu_power_down(flags);
 
 	/*
 	 * Returns here only if there was a pending interrupt and we did not
@@ -293,6 +313,35 @@ static struct cpuidle_ops qcom_cpuidle_ops __initdata = {
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
 CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
 
+static int pd_power_on(struct generic_pm_domain *domain)
+{
+	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, smp_processor_id());
+	struct cluster *pmd = drv->domain;
+
+	if (!pmd || !pmd->domain_spm)
+		return 0;
+
+	pmd->domain_off = false;
+	spm_set_low_power_mode(pmd->domain_spm, PM_SLEEP_MODE_STBY);
+
+	return 0;
+}
+
+static int pd_power_off(struct generic_pm_domain *domain)
+{
+	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, smp_processor_id());
+	struct cluster *pmd = drv->domain;
+
+	if (!pmd || !pmd->domain_spm)
+		return 0;
+
+	pmd->domain_off = true;
+	spm_set_low_power_mode(pmd->domain_spm, PM_SLEEP_MODE_SPC);
+
+	return 0;
+}
+
+
 /* Match L2 SPM with their affinity level */
 static const struct of_device_id cache_spm_table[] = {
 	{ },
@@ -418,8 +467,18 @@ static int spm_dev_probe(struct platform_device *pdev)
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_STBY);
 
 	/* We are ready to use the CPU/Cache SPM. */
-	if (is_domain_spm)
+	if (is_domain_spm) {
+		struct of_phandle_args args;
+		int ret;
+
+		args.np = pdev->dev.of_node;
+		args.args_count = 0;
+		ret = register_platform_domain_handlers(&args,
+				pd_power_on, pd_power_off);
+		if (ret)
+			dev_dbg(&pdev->dev, "Domain callback not registered\n");
 		cpu_domain[index].domain_spm = drv;
+	}
 	else
 		per_cpu(cpu_spm_drv, index) = drv;
 
-- 
2.1.4

[PATCH RFC v2 13/16] drivers: qcom: spm: Add 8084 L2 SPM register data

[Lina Iyer]

Add register data and configure L2 SAW to support voltage control and L2
idle states for QCOM APQ8084 SoC.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 .../devicetree/bindings/arm/msm/qcom,saw2.txt        |  1 +
 drivers/soc/qcom/spm.c                               | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
index ae4afc6..91430ff 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
@@ -27,6 +27,7 @@ PROPERTIES
 			"qcom,apq8064-saw2-v1.1-cpu"
 			"qcom,msm8974-saw2-v2.1-cpu"
 			"qcom,apq8084-saw2-v2.1-cpu"
+			"qcom,apq8084-saw2-v2.1-l2"
 
 - reg:
 	Usage: required
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index ad1498e..c54de31 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -90,6 +90,8 @@ static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
 	[SPM_REG_SPM_CTL]	= 0x30,
 	[SPM_REG_DLY]		= 0x34,
 	[SPM_REG_SEQ_ENTRY]	= 0x80,
+	[SPM_REG_PMIC_DATA_0]	= 0x40,
+	[SPM_REG_PMIC_DATA_1]	= 0x44,
 };
 
 /* SPM register data for 8974, 8084 */
@@ -104,6 +106,20 @@ static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
 	.start_index[PM_SLEEP_MODE_SPC] = 3,
 };
 
+static const struct spm_reg_data spm_reg_8084_l2  = {
+	.reg_offset = spm_reg_offset_v2_1,
+	.spm_cfg = 0x14,
+	.spm_dly = 0x3C102800,
+	.pmic_data[0] = 0x02030080,
+	.pmic_data[1] = 0x00030000,
+	.seq = { 0x1F, 0x00, 0x20, 0x03, 0x22, 0x00, 0x0F, 0x00, 0x10, 0x22,
+		0x12, 0x32, 0x60, 0x70, 0x80, 0xB0, 0x11, 0x42, 0x03, 0x01,
+		0xB0, 0x78, 0x80, 0x12, 0x22, 0x44, 0x50, 0x3B, 0x60, 0x02,
+		0x32, 0x50, 0x0F },
+	.start_index[PM_SLEEP_MODE_STBY] = 0,
+	.start_index[PM_SLEEP_MODE_SPC] = 7,
+};
+
 static const u8 spm_reg_offset_v1_1[SPM_REG_NR] = {
 	[SPM_REG_CFG]		= 0x08,
 	[SPM_REG_SPM_CTL]	= 0x20,
@@ -216,6 +232,7 @@ static int qcom_cpu_spc(int cpu)
 	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
 
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
+
 	ret = cpu_suspend(0, qcom_pm_collapse);
 	/*
 	 * ARM common code executes WFI without calling into our driver and
@@ -344,6 +361,7 @@ static int pd_power_off(struct generic_pm_domain *domain)
 
 /* Match L2 SPM with their affinity level */
 static const struct of_device_id cache_spm_table[] = {
+	{ .compatible = "qcom,apq8084-saw2-v2.1-l2", .data = (void *)0 },
 	{ },
 };
 
@@ -417,6 +435,8 @@ static const struct of_device_id spm_match_table[] = {
 	  .data = &spm_reg_8974_8084_cpu },
 	{ .compatible = "qcom,apq8064-saw2-v1.1-cpu",
 	  .data = &spm_reg_8064_cpu },
+	{ .compatible = "qcom,apq8084-saw2-v2.1-l2",
+	  .data = &spm_reg_8084_l2 },
 	{ },
 };
 
-- 
2.1.4

[PATCH RFC v2 13/16] drivers: qcom: spm: Add 8084 L2 SPM register data

[Lina Iyer]

Add register data and configure L2 SAW to support voltage control and L2
idle states for QCOM APQ8084 SoC.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 .../devicetree/bindings/arm/msm/qcom,saw2.txt        |  1 +
 drivers/soc/qcom/spm.c                               | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
index ae4afc6..91430ff 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
@@ -27,6 +27,7 @@ PROPERTIES
 			"qcom,apq8064-saw2-v1.1-cpu"
 			"qcom,msm8974-saw2-v2.1-cpu"
 			"qcom,apq8084-saw2-v2.1-cpu"
+			"qcom,apq8084-saw2-v2.1-l2"
 
 - reg:
 	Usage: required
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index ad1498e..c54de31 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -90,6 +90,8 @@ static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
 	[SPM_REG_SPM_CTL]	= 0x30,
 	[SPM_REG_DLY]		= 0x34,
 	[SPM_REG_SEQ_ENTRY]	= 0x80,
+	[SPM_REG_PMIC_DATA_0]	= 0x40,
+	[SPM_REG_PMIC_DATA_1]	= 0x44,
 };
 
 /* SPM register data for 8974, 8084 */
@@ -104,6 +106,20 @@ static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
 	.start_index[PM_SLEEP_MODE_SPC] = 3,
 };
 
+static const struct spm_reg_data spm_reg_8084_l2  = {
+	.reg_offset = spm_reg_offset_v2_1,
+	.spm_cfg = 0x14,
+	.spm_dly = 0x3C102800,
+	.pmic_data[0] = 0x02030080,
+	.pmic_data[1] = 0x00030000,
+	.seq = { 0x1F, 0x00, 0x20, 0x03, 0x22, 0x00, 0x0F, 0x00, 0x10, 0x22,
+		0x12, 0x32, 0x60, 0x70, 0x80, 0xB0, 0x11, 0x42, 0x03, 0x01,
+		0xB0, 0x78, 0x80, 0x12, 0x22, 0x44, 0x50, 0x3B, 0x60, 0x02,
+		0x32, 0x50, 0x0F },
+	.start_index[PM_SLEEP_MODE_STBY] = 0,
+	.start_index[PM_SLEEP_MODE_SPC] = 7,
+};
+
 static const u8 spm_reg_offset_v1_1[SPM_REG_NR] = {
 	[SPM_REG_CFG]		= 0x08,
 	[SPM_REG_SPM_CTL]	= 0x20,
@@ -216,6 +232,7 @@ static int qcom_cpu_spc(int cpu)
 	struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
 
 	spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
+
 	ret = cpu_suspend(0, qcom_pm_collapse);
 	/*
 	 * ARM common code executes WFI without calling into our driver and
@@ -344,6 +361,7 @@ static int pd_power_off(struct generic_pm_domain *domain)
 
 /* Match L2 SPM with their affinity level */
 static const struct of_device_id cache_spm_table[] = {
+	{ .compatible = "qcom,apq8084-saw2-v2.1-l2", .data = (void *)0 },
 	{ },
 };
 
@@ -417,6 +435,8 @@ static const struct of_device_id spm_match_table[] = {
 	  .data = &spm_reg_8974_8084_cpu },
 	{ .compatible = "qcom,apq8064-saw2-v1.1-cpu",
 	  .data = &spm_reg_8064_cpu },
+	{ .compatible = "qcom,apq8084-saw2-v2.1-l2",
+	  .data = &spm_reg_8084_l2 },
 	{ },
 };
 
-- 
2.1.4

[PATCH RFC v2 14/16] arm: dts: Add L2 power-controller device bindings for APQ8084

[Lina Iyer]

Add power controller (SAW) device nodes for L2 caches. L2 SAW enable L2
to enter idle states and be powered off. Also, on 8084 the L2 SAW may be
used to regulate the active voltage for the cpu and L2.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 7084010..900ef1f 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -59,7 +59,7 @@
 		};
 
 		L2: l2-cache {
-			compatible = "qcom,arch-cache";
+			compatible = "cache";
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
@@ -183,7 +183,7 @@
 		};
 
 		saw_l2: power-controller@f9012000 {
-			compatible = "qcom,saw2";
+			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
 		};
-- 
2.1.4

[PATCH RFC v2 14/16] arm: dts: Add L2 power-controller device bindings for APQ8084

[Lina Iyer]

Add power controller (SAW) device nodes for L2 caches. L2 SAW enable L2
to enter idle states and be powered off. Also, on 8084 the L2 SAW may be
used to regulate the active voltage for the cpu and L2.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 7084010..900ef1f 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -59,7 +59,7 @@
 		};
 
 		L2: l2-cache {
-			compatible = "qcom,arch-cache";
+			compatible = "cache";
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
@@ -183,7 +183,7 @@
 		};
 
 		saw_l2: power-controller at f9012000 {
-			compatible = "qcom,saw2";
+			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
 		};
-- 
2.1.4

[PATCH RFC v2 15/16] arm: dts: Add power domain device bindings for APQ8084

[Lina Iyer]

Define L2 SAW as the power domain provider and individual cpus are the
power domain consumers.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 900ef1f..4a61f8d 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -23,6 +23,7 @@
 			qcom,acc = <&acc0>;
 			qcom,saw = <&saw0>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu@1 {
@@ -34,6 +35,7 @@
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu@2 {
@@ -45,6 +47,7 @@
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu@3 {
@@ -56,6 +59,7 @@
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		L2: l2-cache {
@@ -183,9 +187,11 @@
 		};
 
 		saw_l2: power-controller@f9012000 {
-			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2";
+			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2",
+					 "arm,pd";
 			reg = <0xf9012000 0x1000>;
 			regulator;
+			#power-domain-cells = <0>;
 		};
 
 		acc0: clock-controller@f9088000 {
-- 
2.1.4

[PATCH RFC v2 15/16] arm: dts: Add power domain device bindings for APQ8084

[Lina Iyer]

Define L2 SAW as the power domain provider and individual cpus are the
power domain consumers.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 900ef1f..4a61f8d 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -23,6 +23,7 @@
 			qcom,acc = <&acc0>;
 			qcom,saw = <&saw0>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu at 1 {
@@ -34,6 +35,7 @@
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu at 2 {
@@ -45,6 +47,7 @@
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		cpu at 3 {
@@ -56,6 +59,7 @@
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
 			cpu-idle-states = <&CPU_SPC>;
+			power-domains = <&saw_l2>;
 		};
 
 		L2: l2-cache {
@@ -183,9 +187,11 @@
 		};
 
 		saw_l2: power-controller at f9012000 {
-			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2";
+			compatible = "qcom,apq8084-saw2-v2.1-l2", "qcom,saw2",
+					 "arm,pd";
 			reg = <0xf9012000 0x1000>;
 			regulator;
+			#power-domain-cells = <0>;
 		};
 
 		acc0: clock-controller at f9088000 {
-- 
2.1.4

[PATCH RFC v2 16/16] drivers: qcom: Enable genpd on selecting QCOM_PM

[Lina Iyer]

Enable PM_CPU_DOMAIN and its PM_GENERIC_DOMAINS dependenciesd to provide
cpu domain support for QCOM SoCs.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 01aa2fd..9564a35 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -15,6 +15,9 @@ config QCOM_PM
 	depends on ARCH_QCOM && !ARM64
 	select QCOM_SCM
 	select ARM_CPU_SUSPEND
+	select PM_GENERIC_DOMAINS
+	select PM_GENERIC_DOMAINS_SLEEP
+	select PM_GENERIC_DOMAINS_OF
 	help
 	  QCOM Platform specific power driver to manage cores and L2 low power
 	  modes. It interface with various system drivers to put the cores in
-- 
2.1.4

[PATCH RFC v2 16/16] drivers: qcom: Enable genpd on selecting QCOM_PM

[Lina Iyer]

Enable PM_CPU_DOMAIN and its PM_GENERIC_DOMAINS dependenciesd to provide
cpu domain support for QCOM SoCs.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 01aa2fd..9564a35 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -15,6 +15,9 @@ config QCOM_PM
 	depends on ARCH_QCOM && !ARM64
 	select QCOM_SCM
 	select ARM_CPU_SUSPEND
+	select PM_GENERIC_DOMAINS
+	select PM_GENERIC_DOMAINS_SLEEP
+	select PM_GENERIC_DOMAINS_OF
 	help
 	  QCOM Platform specific power driver to manage cores and L2 low power
 	  modes. It interface with various system drivers to put the cores in
-- 
2.1.4

Re: [PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Krzysztof Kozlowski]

On 27.06.2015 12:02, Lina Iyer wrote:
> In preparation for supporting IRQ-safe domains, allocate domain data
> outside the domain locks. These functions are not called in an atomic
> context, so we can always allocate memory using GFP_KERNEL. By
> allocating memory before the locks, we can safely lock the domain using
> spinlocks instead of mutexes.
> 
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> Changes in v2:
> 	- Modify commit text
> ---
>  drivers/base/power/domain.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 87d405a..44af889 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>  			   struct generic_pm_domain *subdomain)
>  {
> -	struct gpd_link *link;
> +	struct gpd_link *link, *itr;
>  	int ret = 0;
>  
>  	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
>  	    || genpd == subdomain)
>  		return -EINVAL;
>  
> +	link = kzalloc(sizeof(*link), GFP_KERNEL);
> +	if (!link)
> +		return -ENOMEM;
> +
>  	mutex_lock(&genpd->lock);
>  	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>  
> @@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>  		goto out;
>  	}
>  
> -	list_for_each_entry(link, &genpd->master_links, master_node) {
> -		if (link->slave == subdomain && link->master == genpd) {
> +	list_for_each_entry(itr, &genpd->master_links, master_node) {
> +		if (itr->slave == subdomain && itr->master == genpd) {
>  			ret = -EINVAL;
>  			goto out;

The same comment as in previous version - where is the kfree()? You
moved the allocation so all error paths have to be checked and adjusted.
Not just one that I pointed in previous review.

>  		}
>  	}
>  
> -	link = kzalloc(sizeof(*link), GFP_KERNEL);
> -	if (!link) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
>  	link->master = genpd;
>  	list_add_tail(&link->master_node, &genpd->master_links);
>  	link->slave = subdomain;
> @@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>  	if (IS_ERR_OR_NULL(genpd) || state < 0)
>  		return -EINVAL;
>  
> +	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> +	if (!cpuidle_data)
> +		return -ENOMEM;
> +
>  	mutex_lock(&genpd->lock);
>  
>  	if (genpd->cpuidle_data) {
>  		ret = -EEXIST;
> -		goto out;
> -	}
> -	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> -	if (!cpuidle_data) {
> -		ret = -ENOMEM;
> -		goto out;
> +		goto err_drv;

Is the mutex freed in this exit path?

Best regards,
Krzysztof

>  	}
> +
>  	cpuidle_drv = cpuidle_driver_ref();
>  	if (!cpuidle_drv) {
>  		ret = -ENODEV;
> 

[PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Krzysztof Kozlowski]

On 27.06.2015 12:02, Lina Iyer wrote:
> In preparation for supporting IRQ-safe domains, allocate domain data
> outside the domain locks. These functions are not called in an atomic
> context, so we can always allocate memory using GFP_KERNEL. By
> allocating memory before the locks, we can safely lock the domain using
> spinlocks instead of mutexes.
> 
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> Changes in v2:
> 	- Modify commit text
> ---
>  drivers/base/power/domain.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 87d405a..44af889 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>  			   struct generic_pm_domain *subdomain)
>  {
> -	struct gpd_link *link;
> +	struct gpd_link *link, *itr;
>  	int ret = 0;
>  
>  	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
>  	    || genpd == subdomain)
>  		return -EINVAL;
>  
> +	link = kzalloc(sizeof(*link), GFP_KERNEL);
> +	if (!link)
> +		return -ENOMEM;
> +
>  	mutex_lock(&genpd->lock);
>  	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>  
> @@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>  		goto out;
>  	}
>  
> -	list_for_each_entry(link, &genpd->master_links, master_node) {
> -		if (link->slave == subdomain && link->master == genpd) {
> +	list_for_each_entry(itr, &genpd->master_links, master_node) {
> +		if (itr->slave == subdomain && itr->master == genpd) {
>  			ret = -EINVAL;
>  			goto out;

The same comment as in previous version - where is the kfree()? You
moved the allocation so all error paths have to be checked and adjusted.
Not just one that I pointed in previous review.

>  		}
>  	}
>  
> -	link = kzalloc(sizeof(*link), GFP_KERNEL);
> -	if (!link) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
>  	link->master = genpd;
>  	list_add_tail(&link->master_node, &genpd->master_links);
>  	link->slave = subdomain;
> @@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>  	if (IS_ERR_OR_NULL(genpd) || state < 0)
>  		return -EINVAL;
>  
> +	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> +	if (!cpuidle_data)
> +		return -ENOMEM;
> +
>  	mutex_lock(&genpd->lock);
>  
>  	if (genpd->cpuidle_data) {
>  		ret = -EEXIST;
> -		goto out;
> -	}
> -	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> -	if (!cpuidle_data) {
> -		ret = -ENOMEM;
> -		goto out;
> +		goto err_drv;

Is the mutex freed in this exit path?

Best regards,
Krzysztof

>  	}
> +
>  	cpuidle_drv = cpuidle_driver_ref();
>  	if (!cpuidle_drv) {
>  		ret = -ENODEV;
> 

Re: [PATCH RFC v2 02/16] PM / Domains: Remove dev->driver check for runtime PM

[Krzysztof Kozlowski]

On 27.06.2015 12:02, Lina Iyer wrote:
> Remove check for driver of a device, for runtime PM. Device may be
> suspended without an explicit driver. This check seems to be vestigial
> and incorrect in the current context.
> 
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)


Makes sense,
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 44af889..e9b7cfb 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -391,8 +391,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>  		if (stat > PM_QOS_FLAGS_NONE)
>  			return -EBUSY;
>  
> -		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
> -		    || pdd->dev->power.irq_safe))
> +		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
>  			not_suspended++;
>  	}
>  
> 

[PATCH RFC v2 02/16] PM / Domains: Remove dev->driver check for runtime PM

[Krzysztof Kozlowski]

On 27.06.2015 12:02, Lina Iyer wrote:
> Remove check for driver of a device, for runtime PM. Device may be
> suspended without an explicit driver. This check seems to be vestigial
> and incorrect in the current context.
> 
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)


Makes sense,
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 44af889..e9b7cfb 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -391,8 +391,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>  		if (stat > PM_QOS_FLAGS_NONE)
>  			return -EBUSY;
>  
> -		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
> -		    || pdd->dev->power.irq_safe))
> +		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
>  			not_suspended++;
>  	}
>  
> 

Re: [PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Hi Lina,

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.

[...]

Thanks for your patch!

> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -600,7 +600,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
>  into a low-power state together at the same time by turning off the shared
>  power resource.  Of course, they also need to be put into the full-power state
>  together, by turning the shared power resource on.  A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another domain.

"... another power domain"?

> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. PM domains by default,

This isn't exactly your fault, but this section is the only one inside this
document that talks about "power domains". All other sections talk about
"PM domains".
You're adding a paragraph about "PM domains" inside this section.

> +operate in process context but could have devices that are IRQ safe. Such PM
> +domains cannot be powered on/off during runtime PM. On the other hand, an IRQ
> +safe PM domain that can be powered on/off and suspend or resumed in an atomic
> +context, may contain IRQ safe devices. Such domain may only contain only IRQ
> +safe devices or IRQ safe sub-domains.
>
>  Support for power domains is provided through the pm_domain field of struct
>  device.  This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e9b7cfb..42ffb8b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -50,6 +50,74 @@
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       if (unlikely(subclass > 0))

In general, using "unlikely" is frowned upon.
And in this case, the compiler already knows the exact value of subclass,
as it's passed as a constant from one of the functions below.

> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->mlock)
> +{
> +       if (unlikely(subclass > 0))

Same here.

> @@ -1277,11 +1356,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>                 return -EINVAL;
>
> +       /* Devices in an IRQ safe PM domain have to be IRQ safe too */

Do you really need this comment? It's identical to the error message below.

> +       if (genpd->irq_safe && !dev->power.irq_safe) {
> +               dev_err(dev,
> +                       "Devices in an IRQ safe domain have to be IRQ safe.\n");

> @@ -1385,12 +1471,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>             || genpd == subdomain)
>                 return -EINVAL;
>
> +       /*
> +        * If the domain can be powered on/off in an IRQ safe
> +        * context, ensure that the subdomain can also be
> +        * powered on/off in that context.
> +        */
> +       if (genpd->irq_safe && !subdomain->irq_safe) {
> +               WARN("Incompatible sub-domain %s of IRQ safe domain %s\n",
> +                               subdomain->name, genpd->name);

If you change the error message to "Subdomains in an IRQ safe domain have
to be IRQ safe", it's (a) consistent with the one for devices, and (b) you
can remove the comment.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Hi Lina,

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.

[...]

Thanks for your patch!

> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -600,7 +600,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
>  into a low-power state together at the same time by turning off the shared
>  power resource.  Of course, they also need to be put into the full-power state
>  together, by turning the shared power resource on.  A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another domain.

"... another power domain"?

> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. PM domains by default,

This isn't exactly your fault, but this section is the only one inside this
document that talks about "power domains". All other sections talk about
"PM domains".
You're adding a paragraph about "PM domains" inside this section.

> +operate in process context but could have devices that are IRQ safe. Such PM
> +domains cannot be powered on/off during runtime PM. On the other hand, an IRQ
> +safe PM domain that can be powered on/off and suspend or resumed in an atomic
> +context, may contain IRQ safe devices. Such domain may only contain only IRQ
> +safe devices or IRQ safe sub-domains.
>
>  Support for power domains is provided through the pm_domain field of struct
>  device.  This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e9b7cfb..42ffb8b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -50,6 +50,74 @@
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       if (unlikely(subclass > 0))

In general, using "unlikely" is frowned upon.
And in this case, the compiler already knows the exact value of subclass,
as it's passed as a constant from one of the functions below.

> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->mlock)
> +{
> +       if (unlikely(subclass > 0))

Same here.

> @@ -1277,11 +1356,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>                 return -EINVAL;
>
> +       /* Devices in an IRQ safe PM domain have to be IRQ safe too */

Do you really need this comment? It's identical to the error message below.

> +       if (genpd->irq_safe && !dev->power.irq_safe) {
> +               dev_err(dev,
> +                       "Devices in an IRQ safe domain have to be IRQ safe.\n");

> @@ -1385,12 +1471,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>             || genpd == subdomain)
>                 return -EINVAL;
>
> +       /*
> +        * If the domain can be powered on/off in an IRQ safe
> +        * context, ensure that the subdomain can also be
> +        * powered on/off in that context.
> +        */
> +       if (genpd->irq_safe && !subdomain->irq_safe) {
> +               WARN("Incompatible sub-domain %s of IRQ safe domain %s\n",
> +                               subdomain->name, genpd->name);

If you change the error message to "Subdomains in an IRQ safe domain have
to be IRQ safe", it's (a) consistent with the one for devices, and (b) you
can remove the comment.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Sorry, I forgot a few comments...

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt

> +context, may contain IRQ safe devices. Such domain may only contain only IRQ

Double "only".

> +safe devices or IRQ safe sub-domains.

> @@ -532,15 +608,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       /* If power.irq_safe, the PM domain is never powered off. */
> -       if (dev->power.irq_safe) {
> +       /*
> +        * As we dont power off a non IRQ safe domain, which holds a

Bogus "a" at the end.

> +        * an IRQ safe device, we dont need to restore power to it.
> +        */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 03/16] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Sorry, I forgot a few comments...

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt

> +context, may contain IRQ safe devices. Such domain may only contain only IRQ

Double "only".

> +safe devices or IRQ safe sub-domains.

> @@ -532,15 +608,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       /* If power.irq_safe, the PM domain is never powered off. */
> -       if (dev->power.irq_safe) {
> +       /*
> +        * As we dont power off a non IRQ safe domain, which holds a

Bogus "a" at the end.

> +        * an IRQ safe device, we dont need to restore power to it.
> +        */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 07/16] arm: domain: Synchronize CPU device runtime PM usage with idle state

[Geert Uytterhoeven]

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> When a CPU is running, the runtime PM usage count should be incremented
> and decremented when the CPU is powers down. To ensure that this happens

powered down

> correctly, increment the runtime usage from that CPU.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 07/16] arm: domain: Synchronize CPU device runtime PM usage with idle state

[Geert Uytterhoeven]

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> When a CPU is running, the runtime PM usage count should be incremented
> and decremented when the CPU is powers down. To ensure that this happens

powered down

> correctly, increment the runtime usage from that CPU.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina, Kevin,

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Platform drivers may have additional setup inorder before the domain can
> be powered off. Allow, platform drivers to register power on/off

Bogus comma.

> callbacks against a domain provider.
>
> While registering the callback ensure that the domain is neither in
> power on/off state. The domain should be active. To ensure that the
> platform callback registration doesntrace with genpd power on/off,

doesn't race

> execute the registration from a CPU on that domain.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

> --- /dev/null
> +++ b/arch/arm/include/asm/pm_domain.h
> @@ -0,0 +1,27 @@
> +/*
> + *  arch/arm/include/asm/pm_domain.h
> + *
> + *  Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __ASM_ARM_PM_DOMAIN_H
> +#define __ASM_ARM_PM_DOMAIN_H
> +
> +#include <linux/pm_domain.h>
> +
> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
> +extern int register_platform_domain_handlers(struct of_phandle_args *args,
> +               int (*pd_down)(struct generic_pm_domain *),
> +               int (*pd_up)(struct generic_pm_domain *));

This looks a bit convoluted to me...

> --- a/arch/arm/kernel/domains.c
> +++ b/arch/arm/kernel/domains.c
> @@ -9,10 +9,19 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>
> +#include <asm/pm_domain.h>
> +
>  #define NAME_MAX 16
>
> +struct platform_cb {
> +       int (*power_off)(struct generic_pm_domain *);
> +       int (*power_on)(struct generic_pm_domain *);
> +};
> +
>  struct arm_pm_domain {
>         struct generic_pm_domain genpd;
> +       struct platform_cb plat_handler;
> +       struct spinlock_t lock;
>  };
>
>  static inline
> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
>
>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>  {
> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
> +
> +       if (arm_pd->plat_handler.power_off)
> +               return arm_pd->plat_handler.power_off(genpd);
> +
>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>         return 0;
>  }
>
>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>  {
> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
> +
> +       if (arm_pd->plat_handler.power_on)
> +               return arm_pd->plat_handler.power_on(genpd);
> +
>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>         return 0;
>  }

> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>                 pd->genpd.power_off = arm_pd_power_down;
>                 pd->genpd.power_on = arm_pd_power_up;

Shouldn't these .power_off() and .power_on() be set up from platform code
instead, in the platform-specific code that creates the PM domain?

The PM Domain containing the CPU may contain other devices, in which
case it's already set up from platform-specific code, which would conflict
with arm_domain_init()?

Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
(both for devices and CPUs) on R-Mobile, and
arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.

R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
with separate PM Domains for L2/SCU and sub-domains for the CPUs.
Unfortunately we don't have SMP support for it, so currently dtsi describes
the first cpu core only. The full structure should look like this

        cpus {
                cpu0: cpu@0 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu1: cpu@1 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu2: cpu@2 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu3: cpu@3 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu4: cpu@4 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu5: cpu@5 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu6: cpu@6 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu7: cpu@7 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };
        };

        L2_CA15: cache-controller@0 {
                compatible = "cache";
                power-domains = <&pd_a3sm>;
        };

        L2_CA7: cache-controller@1 {
                compatible = "cache";
                power-domains = <&pd_a3km>;
        };

And the PM Domain part (which is complete in upstream):

        pd_c4: c4@0 {
                #power-domain-cells = <0>;

                pd_a3sm: a3sm@20 {
                        reg = <20>;
                        #power-domain-cells = <0>;

                        pd_a2sl: a2sl@21 {
                                reg = <21>;
                                #power-domain-cells = <0>;
                        };
                };

                pd_a3km: a3km@22 {
                        reg = <22>;
                        #size-cells = <0>;
                        #power-domain-cells = <0>;

                        pd_a2kl: a2kl@23 {
                                reg = <23>;
                                #power-domain-cells = <0>;
                        };
                };
        };

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina, Kevin,

On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Platform drivers may have additional setup inorder before the domain can
> be powered off. Allow, platform drivers to register power on/off

Bogus comma.

> callbacks against a domain provider.
>
> While registering the callback ensure that the domain is neither in
> power on/off state. The domain should be active. To ensure that the
> platform callback registration doesntrace with genpd power on/off,

doesn't race

> execute the registration from a CPU on that domain.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

> --- /dev/null
> +++ b/arch/arm/include/asm/pm_domain.h
> @@ -0,0 +1,27 @@
> +/*
> + *  arch/arm/include/asm/pm_domain.h
> + *
> + *  Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __ASM_ARM_PM_DOMAIN_H
> +#define __ASM_ARM_PM_DOMAIN_H
> +
> +#include <linux/pm_domain.h>
> +
> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
> +extern int register_platform_domain_handlers(struct of_phandle_args *args,
> +               int (*pd_down)(struct generic_pm_domain *),
> +               int (*pd_up)(struct generic_pm_domain *));

This looks a bit convoluted to me...

> --- a/arch/arm/kernel/domains.c
> +++ b/arch/arm/kernel/domains.c
> @@ -9,10 +9,19 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>
> +#include <asm/pm_domain.h>
> +
>  #define NAME_MAX 16
>
> +struct platform_cb {
> +       int (*power_off)(struct generic_pm_domain *);
> +       int (*power_on)(struct generic_pm_domain *);
> +};
> +
>  struct arm_pm_domain {
>         struct generic_pm_domain genpd;
> +       struct platform_cb plat_handler;
> +       struct spinlock_t lock;
>  };
>
>  static inline
> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
>
>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>  {
> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
> +
> +       if (arm_pd->plat_handler.power_off)
> +               return arm_pd->plat_handler.power_off(genpd);
> +
>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>         return 0;
>  }
>
>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>  {
> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
> +
> +       if (arm_pd->plat_handler.power_on)
> +               return arm_pd->plat_handler.power_on(genpd);
> +
>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>         return 0;
>  }

> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>                 pd->genpd.power_off = arm_pd_power_down;
>                 pd->genpd.power_on = arm_pd_power_up;

Shouldn't these .power_off() and .power_on() be set up from platform code
instead, in the platform-specific code that creates the PM domain?

The PM Domain containing the CPU may contain other devices, in which
case it's already set up from platform-specific code, which would conflict
with arm_domain_init()?

Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
(both for devices and CPUs) on R-Mobile, and
arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.

R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
with separate PM Domains for L2/SCU and sub-domains for the CPUs.
Unfortunately we don't have SMP support for it, so currently dtsi describes
the first cpu core only. The full structure should look like this

        cpus {
                cpu0: cpu at 0 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu1: cpu at 1 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu2: cpu at 2 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu3: cpu at 3 {
                        compatible = "arm,cortex-a15";
                        power-domains = <&pd_a2sl>;
                        next-level-cache = <&L2_CA15>;
                };

                cpu4: cpu at 4 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu5: cpu at 5 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu6: cpu at 6 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };

                cpu7: cpu at 7 {
                        compatible = "arm,cortex-a7";
                        power-domains = <&pd_a2kl>;
                        next-level-cache = <&L2_CA7>;
                };
        };

        L2_CA15: cache-controller at 0 {
                compatible = "cache";
                power-domains = <&pd_a3sm>;
        };

        L2_CA7: cache-controller at 1 {
                compatible = "cache";
                power-domains = <&pd_a3km>;
        };

And the PM Domain part (which is complete in upstream):

        pd_c4: c4 at 0 {
                #power-domain-cells = <0>;

                pd_a3sm: a3sm at 20 {
                        reg = <20>;
                        #power-domain-cells = <0>;

                        pd_a2sl: a2sl at 21 {
                                reg = <21>;
                                #power-domain-cells = <0>;
                        };
                };

                pd_a3km: a3km at 22 {
                        reg = <22>;
                        #size-cells = <0>;
                        #power-domain-cells = <0>;

                        pd_a2kl: a2kl at 23 {
                                reg = <23>;
                                #power-domain-cells = <0>;
                        };
                };
        };

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>Hi Lina, Kevin,
>
>On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Platform drivers may have additional setup inorder before the domain can
>> be powered off. Allow, platform drivers to register power on/off
>
>Bogus comma.
>
Will fix.

>> callbacks against a domain provider.
>>
>> While registering the callback ensure that the domain is neither in
>> power on/off state. The domain should be active. To ensure that the
>> platform callback registration doesntrace with genpd power on/off,
>
>doesn't race
>
Argh, thanks.

>> execute the registration from a CPU on that domain.
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
>> --- /dev/null
>> +++ b/arch/arm/include/asm/pm_domain.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + *  arch/arm/include/asm/pm_domain.h
>> + *
>> + *  Copyright (C) 2015 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef __ASM_ARM_PM_DOMAIN_H
>> +#define __ASM_ARM_PM_DOMAIN_H
>> +
>> +#include <linux/pm_domain.h>
>> +
>> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
>> +extern int register_platform_domain_handlers(struct of_phandle_args *args,
>> +               int (*pd_down)(struct generic_pm_domain *),
>> +               int (*pd_up)(struct generic_pm_domain *));
>
>This looks a bit convoluted to me...
>
>> --- a/arch/arm/kernel/domains.c
>> +++ b/arch/arm/kernel/domains.c
>> @@ -9,10 +9,19 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>
>> +#include <asm/pm_domain.h>
>> +
>>  #define NAME_MAX 16
>>
>> +struct platform_cb {
>> +       int (*power_off)(struct generic_pm_domain *);
>> +       int (*power_on)(struct generic_pm_domain *);
>> +};
>> +
>>  struct arm_pm_domain {
>>         struct generic_pm_domain genpd;
>> +       struct platform_cb plat_handler;
>> +       struct spinlock_t lock;
>>  };
>>
>>  static inline
>> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
>>
>>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>>  {
>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>> +
>> +       if (arm_pd->plat_handler.power_off)
>> +               return arm_pd->plat_handler.power_off(genpd);
>> +
>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>         return 0;
>>  }
>>
>>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>>  {
>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>> +
>> +       if (arm_pd->plat_handler.power_on)
>> +               return arm_pd->plat_handler.power_on(genpd);
>> +
>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>         return 0;
>>  }
>
>> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>>                 pd->genpd.power_off = arm_pd_power_down;
>>                 pd->genpd.power_on = arm_pd_power_up;
>
>Shouldn't these .power_off() and .power_on() be set up from platform code
>instead, in the platform-specific code that creates the PM domain?
>
>The PM Domain containing the CPU may contain other devices, in which
>case it's already set up from platform-specific code, which would conflict
>with arm_domain_init()?
>
In my first RFC, the platform code was creating a domain. In this RFC,
we are flaunting an idea that a generic code could setup the domains
without any intervention from platform code. It will do everything
common on most ARM platforms. Architectures that do not have anything
specific in their domain, would benefit from such an initialization.

If we were to export this genpd and then platform code could attach more
devices to the genpd, or make it a sub of another genpd, would that
work?

>Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
>(both for devices and CPUs) on R-Mobile, and
>arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.
>
>R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
>with separate PM Domains for L2/SCU and sub-domains for the CPUs.

If you could get the CPU genpd from the ARM common code, you could embed
that in the L2 domain.

On QCOM's big.LITTLE SoC, there would a PM domain for each of the CPUs
and then there would be a coherency PM domain that would contain the big
and LITTLE cluster domains. In that case, the platform code would create
and initialize the coherency domain and make the CPU domains a sub of
the coherency domain. Would something like that work?

>Unfortunately we don't have SMP support for it, so currently dtsi describes
>the first cpu core only. The full structure should look like this
>
>        cpus {
>                cpu0: cpu@0 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu1: cpu@1 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu2: cpu@2 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu3: cpu@3 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu4: cpu@4 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu5: cpu@5 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu6: cpu@6 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu7: cpu@7 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>        };
>
>        L2_CA15: cache-controller@0 {
>                compatible = "cache";
>                power-domains = <&pd_a3sm>;
>        };
>
>        L2_CA7: cache-controller@1 {
>                compatible = "cache";
>                power-domains = <&pd_a3km>;
>        };
>
>And the PM Domain part (which is complete in upstream):
>
>        pd_c4: c4@0 {
>                #power-domain-cells = <0>;
>
>                pd_a3sm: a3sm@20 {
>                        reg = <20>;
>                        #power-domain-cells = <0>;
>
>                        pd_a2sl: a2sl@21 {
>                                reg = <21>;
>                                #power-domain-cells = <0>;
>                        };
>                };
>
>                pd_a3km: a3km@22 {
>                        reg = <22>;
>                        #size-cells = <0>;
>                        #power-domain-cells = <0>;
>
>                        pd_a2kl: a2kl@23 {
>                                reg = <23>;
>                                #power-domain-cells = <0>;
>                        };
>                };
>        };
>
Thanks for the example. Would it work, if the platform code initalizes
the pd_a3sm, pd_a3km and pd_c4 and set up the hierarchy and to add the
CPU domains, you could query the ARM and then add the domains to the
pd_a3sm and pd_a3km?

Thanks for looking through the patch.

-- Lina

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>Hi Lina, Kevin,
>
>On Sat, Jun 27, 2015 at 5:02 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Platform drivers may have additional setup inorder before the domain can
>> be powered off. Allow, platform drivers to register power on/off
>
>Bogus comma.
>
Will fix.

>> callbacks against a domain provider.
>>
>> While registering the callback ensure that the domain is neither in
>> power on/off state. The domain should be active. To ensure that the
>> platform callback registration doesntrace with genpd power on/off,
>
>doesn't race
>
Argh, thanks.

>> execute the registration from a CPU on that domain.
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
>> --- /dev/null
>> +++ b/arch/arm/include/asm/pm_domain.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + *  arch/arm/include/asm/pm_domain.h
>> + *
>> + *  Copyright (C) 2015 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef __ASM_ARM_PM_DOMAIN_H
>> +#define __ASM_ARM_PM_DOMAIN_H
>> +
>> +#include <linux/pm_domain.h>
>> +
>> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
>> +extern int register_platform_domain_handlers(struct of_phandle_args *args,
>> +               int (*pd_down)(struct generic_pm_domain *),
>> +               int (*pd_up)(struct generic_pm_domain *));
>
>This looks a bit convoluted to me...
>
>> --- a/arch/arm/kernel/domains.c
>> +++ b/arch/arm/kernel/domains.c
>> @@ -9,10 +9,19 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>
>> +#include <asm/pm_domain.h>
>> +
>>  #define NAME_MAX 16
>>
>> +struct platform_cb {
>> +       int (*power_off)(struct generic_pm_domain *);
>> +       int (*power_on)(struct generic_pm_domain *);
>> +};
>> +
>>  struct arm_pm_domain {
>>         struct generic_pm_domain genpd;
>> +       struct platform_cb plat_handler;
>> +       struct spinlock_t lock;
>>  };
>>
>>  static inline
>> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
>>
>>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>>  {
>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>> +
>> +       if (arm_pd->plat_handler.power_off)
>> +               return arm_pd->plat_handler.power_off(genpd);
>> +
>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>         return 0;
>>  }
>>
>>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>>  {
>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>> +
>> +       if (arm_pd->plat_handler.power_on)
>> +               return arm_pd->plat_handler.power_on(genpd);
>> +
>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>         return 0;
>>  }
>
>> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>>                 pd->genpd.power_off = arm_pd_power_down;
>>                 pd->genpd.power_on = arm_pd_power_up;
>
>Shouldn't these .power_off() and .power_on() be set up from platform code
>instead, in the platform-specific code that creates the PM domain?
>
>The PM Domain containing the CPU may contain other devices, in which
>case it's already set up from platform-specific code, which would conflict
>with arm_domain_init()?
>
In my first RFC, the platform code was creating a domain. In this RFC,
we are flaunting an idea that a generic code could setup the domains
without any intervention from platform code. It will do everything
common on most ARM platforms. Architectures that do not have anything
specific in their domain, would benefit from such an initialization.

If we were to export this genpd and then platform code could attach more
devices to the genpd, or make it a sub of another genpd, would that
work?

>Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
>(both for devices and CPUs) on R-Mobile, and
>arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.
>
>R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
>with separate PM Domains for L2/SCU and sub-domains for the CPUs.

If you could get the CPU genpd from the ARM common code, you could embed
that in the L2 domain.

On QCOM's big.LITTLE SoC, there would a PM domain for each of the CPUs
and then there would be a coherency PM domain that would contain the big
and LITTLE cluster domains. In that case, the platform code would create
and initialize the coherency domain and make the CPU domains a sub of
the coherency domain. Would something like that work?

>Unfortunately we don't have SMP support for it, so currently dtsi describes
>the first cpu core only. The full structure should look like this
>
>        cpus {
>                cpu0: cpu at 0 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu1: cpu at 1 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu2: cpu at 2 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu3: cpu at 3 {
>                        compatible = "arm,cortex-a15";
>                        power-domains = <&pd_a2sl>;
>                        next-level-cache = <&L2_CA15>;
>                };
>
>                cpu4: cpu at 4 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu5: cpu at 5 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu6: cpu at 6 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>
>                cpu7: cpu at 7 {
>                        compatible = "arm,cortex-a7";
>                        power-domains = <&pd_a2kl>;
>                        next-level-cache = <&L2_CA7>;
>                };
>        };
>
>        L2_CA15: cache-controller at 0 {
>                compatible = "cache";
>                power-domains = <&pd_a3sm>;
>        };
>
>        L2_CA7: cache-controller at 1 {
>                compatible = "cache";
>                power-domains = <&pd_a3km>;
>        };
>
>And the PM Domain part (which is complete in upstream):
>
>        pd_c4: c4 at 0 {
>                #power-domain-cells = <0>;
>
>                pd_a3sm: a3sm at 20 {
>                        reg = <20>;
>                        #power-domain-cells = <0>;
>
>                        pd_a2sl: a2sl at 21 {
>                                reg = <21>;
>                                #power-domain-cells = <0>;
>                        };
>                };
>
>                pd_a3km: a3km at 22 {
>                        reg = <22>;
>                        #size-cells = <0>;
>                        #power-domain-cells = <0>;
>
>                        pd_a2kl: a2kl at 23 {
>                                reg = <23>;
>                                #power-domain-cells = <0>;
>                        };
>                };
>        };
>
Thanks for the example. Would it work, if the platform code initalizes
the pd_a3sm, pd_a3km and pd_c4 and set up the hierarchy and to add the
CPU domains, you could query the ARM and then add the domains to the
pd_a3sm and pd_a3km?

Thanks for looking through the patch.

-- Lina

Re: [PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Lina Iyer]

On Mon, Jun 29 2015 at 18:26 -0600, Krzysztof Kozlowski wrote:
>On 27.06.2015 12:02, Lina Iyer wrote:
>> In preparation for supporting IRQ-safe domains, allocate domain data
>> outside the domain locks. These functions are not called in an atomic
>> context, so we can always allocate memory using GFP_KERNEL. By
>> allocating memory before the locks, we can safely lock the domain using
>> spinlocks instead of mutexes.
>>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> Changes in v2:
>> 	- Modify commit text
>> ---
>>  drivers/base/power/domain.c | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 87d405a..44af889 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>  			   struct generic_pm_domain *subdomain)
>>  {
>> -	struct gpd_link *link;
>> +	struct gpd_link *link, *itr;
>>  	int ret = 0;
>>
>>  	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
>>  	    || genpd == subdomain)
>>  		return -EINVAL;
>>
>> +	link = kzalloc(sizeof(*link), GFP_KERNEL);
>> +	if (!link)
>> +		return -ENOMEM;
>> +
>>  	mutex_lock(&genpd->lock);
>>  	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>>
>> @@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>  		goto out;
>>  	}
>>
>> -	list_for_each_entry(link, &genpd->master_links, master_node) {
>> -		if (link->slave == subdomain && link->master == genpd) {
>> +	list_for_each_entry(itr, &genpd->master_links, master_node) {
>> +		if (itr->slave == subdomain && itr->master == genpd) {
>>  			ret = -EINVAL;
>>  			goto out;
>
>The same comment as in previous version - where is the kfree()? You
>moved the allocation so all error paths have to be checked and adjusted.
>Not just one that I pointed in previous review.
>
Sorry, didnt notice that, will fix this. I seem to have addressed the
other fone.
>>  		}
>>  	}
>>
>> -	link = kzalloc(sizeof(*link), GFP_KERNEL);
>> -	if (!link) {
>> -		ret = -ENOMEM;
>> -		goto out;
>> -	}
>>  	link->master = genpd;
>>  	list_add_tail(&link->master_node, &genpd->master_links);
>>  	link->slave = subdomain;
>> @@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>  	if (IS_ERR_OR_NULL(genpd) || state < 0)
>>  		return -EINVAL;
>>
>> +	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>> +	if (!cpuidle_data)
>> +		return -ENOMEM;
>> +
>>  	mutex_lock(&genpd->lock);
>>
>>  	if (genpd->cpuidle_data) {
>>  		ret = -EEXIST;
>> -		goto out;
>> -	}
>> -	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>> -	if (!cpuidle_data) {
>> -		ret = -ENOMEM;
>> -		goto out;
>> +		goto err_drv;
>
>Is the mutex freed in this exit path?
>
Yes, err_drv jumps to out, which frees the mutex.

Thank you so much for the review.

-- Lina

>>  	}
>> +
>>  	cpuidle_drv = cpuidle_driver_ref();
>>  	if (!cpuidle_drv) {
>>  		ret = -ENODEV;
>>
>

[PATCH RFC v2 01/16] PM / Domains: Allocate memory outside domain locks

[Lina Iyer]

On Mon, Jun 29 2015 at 18:26 -0600, Krzysztof Kozlowski wrote:
>On 27.06.2015 12:02, Lina Iyer wrote:
>> In preparation for supporting IRQ-safe domains, allocate domain data
>> outside the domain locks. These functions are not called in an atomic
>> context, so we can always allocate memory using GFP_KERNEL. By
>> allocating memory before the locks, we can safely lock the domain using
>> spinlocks instead of mutexes.
>>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> Changes in v2:
>> 	- Modify commit text
>> ---
>>  drivers/base/power/domain.c | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 87d405a..44af889 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1379,13 +1379,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>  			   struct generic_pm_domain *subdomain)
>>  {
>> -	struct gpd_link *link;
>> +	struct gpd_link *link, *itr;
>>  	int ret = 0;
>>
>>  	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
>>  	    || genpd == subdomain)
>>  		return -EINVAL;
>>
>> +	link = kzalloc(sizeof(*link), GFP_KERNEL);
>> +	if (!link)
>> +		return -ENOMEM;
>> +
>>  	mutex_lock(&genpd->lock);
>>  	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>>
>> @@ -1395,18 +1399,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>  		goto out;
>>  	}
>>
>> -	list_for_each_entry(link, &genpd->master_links, master_node) {
>> -		if (link->slave == subdomain && link->master == genpd) {
>> +	list_for_each_entry(itr, &genpd->master_links, master_node) {
>> +		if (itr->slave == subdomain && itr->master == genpd) {
>>  			ret = -EINVAL;
>>  			goto out;
>
>The same comment as in previous version - where is the kfree()? You
>moved the allocation so all error paths have to be checked and adjusted.
>Not just one that I pointed in previous review.
>
Sorry, didnt notice that, will fix this. I seem to have addressed the
other fone.
>>  		}
>>  	}
>>
>> -	link = kzalloc(sizeof(*link), GFP_KERNEL);
>> -	if (!link) {
>> -		ret = -ENOMEM;
>> -		goto out;
>> -	}
>>  	link->master = genpd;
>>  	list_add_tail(&link->master_node, &genpd->master_links);
>>  	link->slave = subdomain;
>> @@ -1508,17 +1507,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>  	if (IS_ERR_OR_NULL(genpd) || state < 0)
>>  		return -EINVAL;
>>
>> +	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>> +	if (!cpuidle_data)
>> +		return -ENOMEM;
>> +
>>  	mutex_lock(&genpd->lock);
>>
>>  	if (genpd->cpuidle_data) {
>>  		ret = -EEXIST;
>> -		goto out;
>> -	}
>> -	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>> -	if (!cpuidle_data) {
>> -		ret = -ENOMEM;
>> -		goto out;
>> +		goto err_drv;
>
>Is the mutex freed in this exit path?
>
Yes, err_drv jumps to out, which frees the mutex.

Thank you so much for the review.

-- Lina

>>  	}
>> +
>>  	cpuidle_drv = cpuidle_driver_ref();
>>  	if (!cpuidle_drv) {
>>  		ret = -ENODEV;
>>
>

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina,

On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/pm_domain.h
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + *  arch/arm/include/asm/pm_domain.h
>>> + *
>>> + *  Copyright (C) 2015 Linaro Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +#ifndef __ASM_ARM_PM_DOMAIN_H
>>> +#define __ASM_ARM_PM_DOMAIN_H
>>> +
>>> +#include <linux/pm_domain.h>
>>> +
>>> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
>>> +extern int register_platform_domain_handlers(struct of_phandle_args
>>> *args,
>>> +               int (*pd_down)(struct generic_pm_domain *),
>>> +               int (*pd_up)(struct generic_pm_domain *));
>>
>>
>> This looks a bit convoluted to me...
>>
>>> --- a/arch/arm/kernel/domains.c
>>> +++ b/arch/arm/kernel/domains.c
>>> @@ -9,10 +9,19 @@
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>>
>>> +#include <asm/pm_domain.h>
>>> +
>>>  #define NAME_MAX 16
>>>
>>> +struct platform_cb {
>>> +       int (*power_off)(struct generic_pm_domain *);
>>> +       int (*power_on)(struct generic_pm_domain *);
>>> +};
>>> +
>>>  struct arm_pm_domain {
>>>         struct generic_pm_domain genpd;
>>> +       struct platform_cb plat_handler;
>>> +       struct spinlock_t lock;
>>>  };
>>>
>>>  static inline
>>> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct
>>> generic_pm_domain *d)
>>>
>>>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>>>  {
>>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>>> +
>>> +       if (arm_pd->plat_handler.power_off)
>>> +               return arm_pd->plat_handler.power_off(genpd);
>>> +
>>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>>         return 0;
>>>  }
>>>
>>>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>>>  {
>>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>>> +
>>> +       if (arm_pd->plat_handler.power_on)
>>> +               return arm_pd->plat_handler.power_on(genpd);
>>> +
>>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>>         return 0;
>>>  }
>>
>>
>>> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>>>                 pd->genpd.power_off = arm_pd_power_down;
>>>                 pd->genpd.power_on = arm_pd_power_up;
>>
>>
>> Shouldn't these .power_off() and .power_on() be set up from platform code
>> instead, in the platform-specific code that creates the PM domain?
>>
>> The PM Domain containing the CPU may contain other devices, in which
>> case it's already set up from platform-specific code, which would conflict
>> with arm_domain_init()?
>>
> In my first RFC, the platform code was creating a domain. In this RFC,
> we are flaunting an idea that a generic code could setup the domains
> without any intervention from platform code. It will do everything
> common on most ARM platforms. Architectures that do not have anything
> specific in their domain, would benefit from such an initialization.
>
> If we were to export this genpd and then platform code could attach more
> devices to the genpd, or make it a sub of another genpd, would that
> work?
>
>> Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
>> (both for devices and CPUs) on R-Mobile, and
>> arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.
>>
>> R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
>> with separate PM Domains for L2/SCU and sub-domains for the CPUs.
>
>
> If you could get the CPU genpd from the ARM common code, you could embed
> that in the L2 domain.
>
> On QCOM's big.LITTLE SoC, there would a PM domain for each of the CPUs
> and then there would be a coherency PM domain that would contain the big
> and LITTLE cluster domains. In that case, the platform code would create
> and initialize the coherency domain and make the CPU domains a sub of
> the coherency domain. Would something like that work?
>
>
>> Unfortunately we don't have SMP support for it, so currently dtsi
>> describes
>> the first cpu core only. The full structure should look like this
>>
>>        cpus {
>>                cpu0: cpu@0 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu1: cpu@1 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu2: cpu@2 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu3: cpu@3 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu4: cpu@4 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu5: cpu@5 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu6: cpu@6 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu7: cpu@7 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>        };
>>
>>        L2_CA15: cache-controller@0 {
>>                compatible = "cache";
>>                power-domains = <&pd_a3sm>;
>>        };
>>
>>        L2_CA7: cache-controller@1 {
>>                compatible = "cache";
>>                power-domains = <&pd_a3km>;
>>        };
>>
>> And the PM Domain part (which is complete in upstream):
>>
>>        pd_c4: c4@0 {
>>                #power-domain-cells = <0>;
>>
>>                pd_a3sm: a3sm@20 {
>>                        reg = <20>;
>>                        #power-domain-cells = <0>;
>>
>>                        pd_a2sl: a2sl@21 {
>>                                reg = <21>;
>>                                #power-domain-cells = <0>;
>>                        };
>>                };
>>
>>                pd_a3km: a3km@22 {
>>                        reg = <22>;
>>                        #size-cells = <0>;
>>                        #power-domain-cells = <0>;
>>
>>                        pd_a2kl: a2kl@23 {
>>                                reg = <23>;
>>                                #power-domain-cells = <0>;
>>                        };
>>                };
>>        };
>>
> Thanks for the example. Would it work, if the platform code initalizes
> the pd_a3sm, pd_a3km and pd_c4 and set up the hierarchy and to add the
> CPU domains, you could query the ARM and then add the domains to the
> pd_a3sm and pd_a3km?

Yes, it could work. But then I have to add _more_ code to ignore the a2sl
and a2kl code in pm-rmobile.c, and call register_platform_domain_handlers(),
as in the end it's pm-rmobile that knows how to power up those PM domains.

I'm a bit reluctant to split responsibility across two drivers: a
platform-specific
one handling PM domains with non-cpu devices, and a generic one handling
PM domains with cpu-devices.

Perhaps the generic one can be optional, and provide helpers for common
CPU operations? Then the platform-specific driver can handle all PM domains,
and delegate to the generic CPU helpers where appropriate.

Does that make sense?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina,

On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/pm_domain.h
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + *  arch/arm/include/asm/pm_domain.h
>>> + *
>>> + *  Copyright (C) 2015 Linaro Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +#ifndef __ASM_ARM_PM_DOMAIN_H
>>> +#define __ASM_ARM_PM_DOMAIN_H
>>> +
>>> +#include <linux/pm_domain.h>
>>> +
>>> +#if IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)
>>> +extern int register_platform_domain_handlers(struct of_phandle_args
>>> *args,
>>> +               int (*pd_down)(struct generic_pm_domain *),
>>> +               int (*pd_up)(struct generic_pm_domain *));
>>
>>
>> This looks a bit convoluted to me...
>>
>>> --- a/arch/arm/kernel/domains.c
>>> +++ b/arch/arm/kernel/domains.c
>>> @@ -9,10 +9,19 @@
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>>
>>> +#include <asm/pm_domain.h>
>>> +
>>>  #define NAME_MAX 16
>>>
>>> +struct platform_cb {
>>> +       int (*power_off)(struct generic_pm_domain *);
>>> +       int (*power_on)(struct generic_pm_domain *);
>>> +};
>>> +
>>>  struct arm_pm_domain {
>>>         struct generic_pm_domain genpd;
>>> +       struct platform_cb plat_handler;
>>> +       struct spinlock_t lock;
>>>  };
>>>
>>>  static inline
>>> @@ -23,16 +32,85 @@ struct arm_pm_domain *to_arm_pd(struct
>>> generic_pm_domain *d)
>>>
>>>  static int arm_pd_power_down(struct generic_pm_domain *genpd)
>>>  {
>>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>>> +
>>> +       if (arm_pd->plat_handler.power_off)
>>> +               return arm_pd->plat_handler.power_off(genpd);
>>> +
>>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>>         return 0;
>>>  }
>>>
>>>  static int arm_pd_power_up(struct generic_pm_domain *genpd)
>>>  {
>>> +       struct arm_pm_domain *arm_pd = to_arm_pd(genpd);
>>> +
>>> +       if (arm_pd->plat_handler.power_on)
>>> +               return arm_pd->plat_handler.power_on(genpd);
>>> +
>>>         /* pr_info("KJH: %s: %s\n", __func__, genpd->name); */
>>>         return 0;
>>>  }
>>
>>
>>> @@ -152,6 +230,7 @@ static int arm_domain_init(void)
>>>                 pd->genpd.power_off = arm_pd_power_down;
>>>                 pd->genpd.power_on = arm_pd_power_up;
>>
>>
>> Shouldn't these .power_off() and .power_on() be set up from platform code
>> instead, in the platform-specific code that creates the PM domain?
>>
>> The PM Domain containing the CPU may contain other devices, in which
>> case it's already set up from platform-specific code, which would conflict
>> with arm_domain_init()?
>>
> In my first RFC, the platform code was creating a domain. In this RFC,
> we are flaunting an idea that a generic code could setup the domains
> without any intervention from platform code. It will do everything
> common on most ARM platforms. Architectures that do not have anything
> specific in their domain, would benefit from such an initialization.
>
> If we were to export this genpd and then platform code could attach more
> devices to the genpd, or make it a sub of another genpd, would that
> work?
>
>> Cfr. arch/arm/mach-shmobile/pm-rmobile.c, which handles all PM domains
>> (both for devices and CPUs) on R-Mobile, and
>> arch/arm/boot/dts/{r8a73a4,r8a7740,sh73a0}.dtsi.
>>
>> R8a73a4 is the most advanced of these three: it has 2 big.LITTLE clusters,
>> with separate PM Domains for L2/SCU and sub-domains for the CPUs.
>
>
> If you could get the CPU genpd from the ARM common code, you could embed
> that in the L2 domain.
>
> On QCOM's big.LITTLE SoC, there would a PM domain for each of the CPUs
> and then there would be a coherency PM domain that would contain the big
> and LITTLE cluster domains. In that case, the platform code would create
> and initialize the coherency domain and make the CPU domains a sub of
> the coherency domain. Would something like that work?
>
>
>> Unfortunately we don't have SMP support for it, so currently dtsi
>> describes
>> the first cpu core only. The full structure should look like this
>>
>>        cpus {
>>                cpu0: cpu at 0 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu1: cpu at 1 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu2: cpu at 2 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu3: cpu at 3 {
>>                        compatible = "arm,cortex-a15";
>>                        power-domains = <&pd_a2sl>;
>>                        next-level-cache = <&L2_CA15>;
>>                };
>>
>>                cpu4: cpu at 4 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu5: cpu at 5 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu6: cpu at 6 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>
>>                cpu7: cpu at 7 {
>>                        compatible = "arm,cortex-a7";
>>                        power-domains = <&pd_a2kl>;
>>                        next-level-cache = <&L2_CA7>;
>>                };
>>        };
>>
>>        L2_CA15: cache-controller at 0 {
>>                compatible = "cache";
>>                power-domains = <&pd_a3sm>;
>>        };
>>
>>        L2_CA7: cache-controller at 1 {
>>                compatible = "cache";
>>                power-domains = <&pd_a3km>;
>>        };
>>
>> And the PM Domain part (which is complete in upstream):
>>
>>        pd_c4: c4 at 0 {
>>                #power-domain-cells = <0>;
>>
>>                pd_a3sm: a3sm at 20 {
>>                        reg = <20>;
>>                        #power-domain-cells = <0>;
>>
>>                        pd_a2sl: a2sl at 21 {
>>                                reg = <21>;
>>                                #power-domain-cells = <0>;
>>                        };
>>                };
>>
>>                pd_a3km: a3km at 22 {
>>                        reg = <22>;
>>                        #size-cells = <0>;
>>                        #power-domain-cells = <0>;
>>
>>                        pd_a2kl: a2kl at 23 {
>>                                reg = <23>;
>>                                #power-domain-cells = <0>;
>>                        };
>>                };
>>        };
>>
> Thanks for the example. Would it work, if the platform code initalizes
> the pd_a3sm, pd_a3km and pd_c4 and set up the hierarchy and to add the
> CPU domains, you could query the ARM and then add the domains to the
> pd_a3sm and pd_a3km?

Yes, it could work. But then I have to add _more_ code to ignore the a2sl
and a2kl code in pm-rmobile.c, and call register_platform_domain_handlers(),
as in the end it's pm-rmobile that knows how to power up those PM domains.

I'm a bit reluctant to split responsibility across two drivers: a
platform-specific
one handling PM domains with non-cpu devices, and a generic one handling
PM domains with cpu-devices.

Perhaps the generic one can be optional, and provide helpers for common
CPU operations? Then the platform-specific driver can handle all PM domains,
and delegate to the generic CPU helpers where appropriate.

Does that make sense?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:

>Perhaps the generic one can be optional, and provide helpers for common
>CPU operations? Then the platform-specific driver can handle all PM domains,
>and delegate to the generic CPU helpers where appropriate.
>
>Does that make sense?
>
Well it does. Thanks Geert. My RFC v1 [1] did exacly that. But, it didnt
fit into the big picture of things well.

Here is where we wanted to head towards, in the long run. Today, we have
CPU_PM for CPU runtime and we have runtime PM for others, we want to
unify and move to a generic runtime PM for the CPUs as well. To that
effort, we want to bring in generic code all into the fold of CPU
runtime PM and CPU domain runtime PM. A generic CPU PM domain with its
own genpd callback for ->power_on() and ->power_off() would help handle
the common power on/off stuff there and possibly call into GIC and
others that currently use CPU_PM from there.  So the common node needs
to be the handle of power on/off callbacks from the genpd, when all the
CPUs are entering idle or resuming.

With what you have suggested, the platform driver creates the genpd and
would pass the CPU genpd to the common code for common operations. (This
was what was done in [1]). The platform driver would set the power_on()
and power_off() callbacks and that would have to be overriden in order
handle common CPU domain suspend/resume activities. Overwriting members
of an object allocated by the platform driver, is something we should
avoid.

Or instead of allocating the memory in your platform driver for the CPU
genpd, you could query and get the genpd and then add your additions on
top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
overwrite the power on/off callbacks in the genpd. They still have to be
registered separately like in this patch. Again, not every elegant, IMO.

Another option, that might be cleaner, is that we could have a PM domain
for CPUs that would set up the compatibility flag to "arm,pd" and you
could nest that domain inside pd_a2sl and pd_a2kl.

pd_c4: c4@0 {
	[...]
	pd_a3sm: a3sm@20 {
		[...]
		pd_a2sl: a2sl@21 {
			reg = <21>;
			#power-domain-cells = <0>;
			pd_cpu_sl: pd1 { <-- Virtual PM domain
				#power-domain-cells = <0>;
			};
		};
	};
};

cpus {
	cpu0: cpu@0 {
		compatible = "arm,cortex-a15";
		power-domains = <&pd_cpu_sl>; <-- here we refer to the
						  virtual PM domain
		next-level-cache = <&L2_CA15>;
        };
	[...]
};

This the common code would get its own callbacks and when that genpd
powers off, the platform genpd would power down. But no new code is
needed in your platform driver. The benefit is that platform code and
generic CPU domain code may exist and act in parallel and may only be
related if specified in the DT and the problem with that approach is
that this virtual PM domain is not a h/w domain, hence specifying in DT
is questionable.

What do you think?

Thanks,
Lina

[1]. http://www.spinics.net/lists/arm-kernel/msg423430.html

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:

>Perhaps the generic one can be optional, and provide helpers for common
>CPU operations? Then the platform-specific driver can handle all PM domains,
>and delegate to the generic CPU helpers where appropriate.
>
>Does that make sense?
>
Well it does. Thanks Geert. My RFC v1 [1] did exacly that. But, it didnt
fit into the big picture of things well.

Here is where we wanted to head towards, in the long run. Today, we have
CPU_PM for CPU runtime and we have runtime PM for others, we want to
unify and move to a generic runtime PM for the CPUs as well. To that
effort, we want to bring in generic code all into the fold of CPU
runtime PM and CPU domain runtime PM. A generic CPU PM domain with its
own genpd callback for ->power_on() and ->power_off() would help handle
the common power on/off stuff there and possibly call into GIC and
others that currently use CPU_PM from there.  So the common node needs
to be the handle of power on/off callbacks from the genpd, when all the
CPUs are entering idle or resuming.

With what you have suggested, the platform driver creates the genpd and
would pass the CPU genpd to the common code for common operations. (This
was what was done in [1]). The platform driver would set the power_on()
and power_off() callbacks and that would have to be overriden in order
handle common CPU domain suspend/resume activities. Overwriting members
of an object allocated by the platform driver, is something we should
avoid.

Or instead of allocating the memory in your platform driver for the CPU
genpd, you could query and get the genpd and then add your additions on
top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
overwrite the power on/off callbacks in the genpd. They still have to be
registered separately like in this patch. Again, not every elegant, IMO.

Another option, that might be cleaner, is that we could have a PM domain
for CPUs that would set up the compatibility flag to "arm,pd" and you
could nest that domain inside pd_a2sl and pd_a2kl.

pd_c4: c4 at 0 {
	[...]
	pd_a3sm: a3sm at 20 {
		[...]
		pd_a2sl: a2sl at 21 {
			reg = <21>;
			#power-domain-cells = <0>;
			pd_cpu_sl: pd1 { <-- Virtual PM domain
				#power-domain-cells = <0>;
			};
		};
	};
};

cpus {
	cpu0: cpu at 0 {
		compatible = "arm,cortex-a15";
		power-domains = <&pd_cpu_sl>; <-- here we refer to the
						  virtual PM domain
		next-level-cache = <&L2_CA15>;
        };
	[...]
};

This the common code would get its own callbacks and when that genpd
powers off, the platform genpd would power down. But no new code is
needed in your platform driver. The benefit is that platform code and
generic CPU domain code may exist and act in parallel and may only be
related if specified in the DT and the problem with that approach is
that this virtual PM domain is not a h/w domain, hence specifying in DT
is questionable.

What do you think?

Thanks,
Lina

[1]. http://www.spinics.net/lists/arm-kernel/msg423430.html

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina,

On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>
>> Perhaps the generic one can be optional, and provide helpers for common
>> CPU operations? Then the platform-specific driver can handle all PM
>> domains,
>> and delegate to the generic CPU helpers where appropriate.
>>
>> Does that make sense?
>>
> Well it does. Thanks Geert. My RFC v1 [1] did exacly that. But, it didnt
> fit into the big picture of things well.
>
> Here is where we wanted to head towards, in the long run. Today, we have
> CPU_PM for CPU runtime and we have runtime PM for others, we want to
> unify and move to a generic runtime PM for the CPUs as well. To that
> effort, we want to bring in generic code all into the fold of CPU
> runtime PM and CPU domain runtime PM. A generic CPU PM domain with its
> own genpd callback for ->power_on() and ->power_off() would help handle
> the common power on/off stuff there and possibly call into GIC and
> others that currently use CPU_PM from there.  So the common node needs
> to be the handle of power on/off callbacks from the genpd, when all the
> CPUs are entering idle or resuming.

Agreed.

> With what you have suggested, the platform driver creates the genpd and
> would pass the CPU genpd to the common code for common operations. (This
> was what was done in [1]). The platform driver would set the power_on()
> and power_off() callbacks and that would have to be overriden in order
> handle common CPU domain suspend/resume activities. Overwriting members
> of an object allocated by the platform driver, is something we should
> avoid.

Instead of letting the generic code override the .power_{on,off}() callbacks,
the platform code could call the generic CPU-related methods from its own
.power_{on,off}() callbacks?

struct rmobile_pm_domain already has .suspend() and .resume() methods.
The former is used to e.g. prevent the PM domains containing CPUs to be
powered down (in the absence of cpuidle integration). That requires scanning
the DT for CPUs, and it would indeed be good to have that scanning support
in generic code.
The latter became unused after the removal of sh7372 support, which did have
some cpuidle integration.

> Or instead of allocating the memory in your platform driver for the CPU
> genpd, you could query and get the genpd and then add your additions on
> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
> overwrite the power on/off callbacks in the genpd. They still have to be
> registered separately like in this patch. Again, not every elegant, IMO.

Just wondering, can I set up the .attach_dev() and .detach_dev()?

Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
needed for devices with MSTP clocks. CPU and L2 cache don't have these,
and there are no other devices in e.g. a3sm and a2sl.

The GIC has an MSTP clock, but it's part of a different power domain.

> Another option, that might be cleaner, is that we could have a PM domain
> for CPUs that would set up the compatibility flag to "arm,pd" and you
> could nest that domain inside pd_a2sl and pd_a2kl.
>
> pd_c4: c4@0 {
>         [...]
>         pd_a3sm: a3sm@20 {
>                 [...]
>                 pd_a2sl: a2sl@21 {
>                         reg = <21>;
>                         #power-domain-cells = <0>;
>                         pd_cpu_sl: pd1 { <-- Virtual PM domain
>                                 #power-domain-cells = <0>;
>                         };
>                 };
>         };
> };
>
> cpus {
>         cpu0: cpu@0 {
>                 compatible = "arm,cortex-a15";
>                 power-domains = <&pd_cpu_sl>; <-- here we refer to the
>                                                   virtual PM domain
>                 next-level-cache = <&L2_CA15>;
>        };
>         [...]
> };
>
> This the common code would get its own callbacks and when that genpd
> powers off, the platform genpd would power down. But no new code is
> needed in your platform driver. The benefit is that platform code and
> generic CPU domain code may exist and act in parallel and may only be
> related if specified in the DT and the problem with that approach is
> that this virtual PM domain is not a h/w domain, hence specifying in DT
> is questionable.

Indeed, I don't like this option, as the DT would no longer describe HW,
but the Linux implementation.

So let's continue with your approach, and see how it turns out. We can
always change and improvide code, while changing DT is more complicated.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Geert Uytterhoeven]

Hi Lina,

On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>
>> Perhaps the generic one can be optional, and provide helpers for common
>> CPU operations? Then the platform-specific driver can handle all PM
>> domains,
>> and delegate to the generic CPU helpers where appropriate.
>>
>> Does that make sense?
>>
> Well it does. Thanks Geert. My RFC v1 [1] did exacly that. But, it didnt
> fit into the big picture of things well.
>
> Here is where we wanted to head towards, in the long run. Today, we have
> CPU_PM for CPU runtime and we have runtime PM for others, we want to
> unify and move to a generic runtime PM for the CPUs as well. To that
> effort, we want to bring in generic code all into the fold of CPU
> runtime PM and CPU domain runtime PM. A generic CPU PM domain with its
> own genpd callback for ->power_on() and ->power_off() would help handle
> the common power on/off stuff there and possibly call into GIC and
> others that currently use CPU_PM from there.  So the common node needs
> to be the handle of power on/off callbacks from the genpd, when all the
> CPUs are entering idle or resuming.

Agreed.

> With what you have suggested, the platform driver creates the genpd and
> would pass the CPU genpd to the common code for common operations. (This
> was what was done in [1]). The platform driver would set the power_on()
> and power_off() callbacks and that would have to be overriden in order
> handle common CPU domain suspend/resume activities. Overwriting members
> of an object allocated by the platform driver, is something we should
> avoid.

Instead of letting the generic code override the .power_{on,off}() callbacks,
the platform code could call the generic CPU-related methods from its own
.power_{on,off}() callbacks?

struct rmobile_pm_domain already has .suspend() and .resume() methods.
The former is used to e.g. prevent the PM domains containing CPUs to be
powered down (in the absence of cpuidle integration). That requires scanning
the DT for CPUs, and it would indeed be good to have that scanning support
in generic code.
The latter became unused after the removal of sh7372 support, which did have
some cpuidle integration.

> Or instead of allocating the memory in your platform driver for the CPU
> genpd, you could query and get the genpd and then add your additions on
> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
> overwrite the power on/off callbacks in the genpd. They still have to be
> registered separately like in this patch. Again, not every elegant, IMO.

Just wondering, can I set up the .attach_dev() and .detach_dev()?

Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
needed for devices with MSTP clocks. CPU and L2 cache don't have these,
and there are no other devices in e.g. a3sm and a2sl.

The GIC has an MSTP clock, but it's part of a different power domain.

> Another option, that might be cleaner, is that we could have a PM domain
> for CPUs that would set up the compatibility flag to "arm,pd" and you
> could nest that domain inside pd_a2sl and pd_a2kl.
>
> pd_c4: c4 at 0 {
>         [...]
>         pd_a3sm: a3sm at 20 {
>                 [...]
>                 pd_a2sl: a2sl at 21 {
>                         reg = <21>;
>                         #power-domain-cells = <0>;
>                         pd_cpu_sl: pd1 { <-- Virtual PM domain
>                                 #power-domain-cells = <0>;
>                         };
>                 };
>         };
> };
>
> cpus {
>         cpu0: cpu at 0 {
>                 compatible = "arm,cortex-a15";
>                 power-domains = <&pd_cpu_sl>; <-- here we refer to the
>                                                   virtual PM domain
>                 next-level-cache = <&L2_CA15>;
>        };
>         [...]
> };
>
> This the common code would get its own callbacks and when that genpd
> powers off, the platform genpd would power down. But no new code is
> needed in your platform driver. The benefit is that platform code and
> generic CPU domain code may exist and act in parallel and may only be
> related if specified in the DT and the problem with that approach is
> that this virtual PM domain is not a h/w domain, hence specifying in DT
> is questionable.

Indeed, I don't like this option, as the DT would no longer describe HW,
but the Linux implementation.

So let's continue with your approach, and see how it turns out. We can
always change and improvide code, while changing DT is more complicated.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:

[...]

>> With what you have suggested, the platform driver creates the genpd and
>> would pass the CPU genpd to the common code for common operations. (This
>> was what was done in [1]). The platform driver would set the power_on()
>> and power_off() callbacks and that would have to be overriden in order
>> handle common CPU domain suspend/resume activities. Overwriting members
>> of an object allocated by the platform driver, is something we should
>> avoid.
>
>Instead of letting the generic code override the .power_{on,off}() callbacks,
>the platform code could call the generic CPU-related methods from its own
>.power_{on,off}() callbacks?
>
We dont want to impose on platform drivers to call back into the CPU
domain core code to do common domain power down activities. On a generic
ARM CPU domain, we may not even need a platform specific domain
callbacks. IMHO, this is not desirable.

>struct rmobile_pm_domain already has .suspend() and .resume() methods.
>The former is used to e.g. prevent the PM domains containing CPUs to be
>powered down (in the absence of cpuidle integration). That requires scanning
>the DT for CPUs, and it would indeed be good to have that scanning support
>in generic code.
>The latter became unused after the removal of sh7372 support, which did have
>some cpuidle integration.
>
The CPUs may still be attached to domain, even if they are not powered
off by CPUIdle. The code increases the reference count for every online
CPU and any CPUs that may come online. The reference count, only goes
down when the CPU powers down through CPUIdle or hotplug. So, if a CPU
is not integrated with CPUIdle, it would remain in use (unless powered
off by hotplug) and the domain would not be powered down.

>> Or instead of allocating the memory in your platform driver for the CPU
>> genpd, you could query and get the genpd and then add your additions on
>> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
>> overwrite the power on/off callbacks in the genpd. They still have to be
>> registered separately like in this patch. Again, not every elegant, IMO.
>
>Just wondering, can I set up the .attach_dev() and .detach_dev()?
>
I am presuming, you are concerned with the fore mentioned case of CPUs
not participating in CPUIdle. With pm_runtime_put_sync() not happening
for CPUs that do not power down, you should not have to worry about the
domain being powered down.

Please correct me if I misunderstood your point.

>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
>needed for devices with MSTP clocks. CPU and L2 cache don't have these,
>and there are no other devices in e.g. a3sm and a2sl.
>
>The GIC has an MSTP clock, but it's part of a different power domain.
>
Okay.

[...]

>> This the common code would get its own callbacks and when that genpd
>> powers off, the platform genpd would power down. But no new code is
>> needed in your platform driver. The benefit is that platform code and
>> generic CPU domain code may exist and act in parallel and may only be
>> related if specified in the DT and the problem with that approach is
>> that this virtual PM domain is not a h/w domain, hence specifying in DT
>> is questionable.
>
>Indeed, I don't like this option, as the DT would no longer describe HW,
>but the Linux implementation.
>
>So let's continue with your approach, and see how it turns out. We can
>always change and improvide code, while changing DT is more complicated.
>
Agreed.

Thanks for your time, Geert.

-- Lina

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:

[...]

>> With what you have suggested, the platform driver creates the genpd and
>> would pass the CPU genpd to the common code for common operations. (This
>> was what was done in [1]). The platform driver would set the power_on()
>> and power_off() callbacks and that would have to be overriden in order
>> handle common CPU domain suspend/resume activities. Overwriting members
>> of an object allocated by the platform driver, is something we should
>> avoid.
>
>Instead of letting the generic code override the .power_{on,off}() callbacks,
>the platform code could call the generic CPU-related methods from its own
>.power_{on,off}() callbacks?
>
We dont want to impose on platform drivers to call back into the CPU
domain core code to do common domain power down activities. On a generic
ARM CPU domain, we may not even need a platform specific domain
callbacks. IMHO, this is not desirable.

>struct rmobile_pm_domain already has .suspend() and .resume() methods.
>The former is used to e.g. prevent the PM domains containing CPUs to be
>powered down (in the absence of cpuidle integration). That requires scanning
>the DT for CPUs, and it would indeed be good to have that scanning support
>in generic code.
>The latter became unused after the removal of sh7372 support, which did have
>some cpuidle integration.
>
The CPUs may still be attached to domain, even if they are not powered
off by CPUIdle. The code increases the reference count for every online
CPU and any CPUs that may come online. The reference count, only goes
down when the CPU powers down through CPUIdle or hotplug. So, if a CPU
is not integrated with CPUIdle, it would remain in use (unless powered
off by hotplug) and the domain would not be powered down.

>> Or instead of allocating the memory in your platform driver for the CPU
>> genpd, you could query and get the genpd and then add your additions on
>> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
>> overwrite the power on/off callbacks in the genpd. They still have to be
>> registered separately like in this patch. Again, not every elegant, IMO.
>
>Just wondering, can I set up the .attach_dev() and .detach_dev()?
>
I am presuming, you are concerned with the fore mentioned case of CPUs
not participating in CPUIdle. With pm_runtime_put_sync() not happening
for CPUs that do not power down, you should not have to worry about the
domain being powered down.

Please correct me if I misunderstood your point.

>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
>needed for devices with MSTP clocks. CPU and L2 cache don't have these,
>and there are no other devices in e.g. a3sm and a2sl.
>
>The GIC has an MSTP clock, but it's part of a different power domain.
>
Okay.

[...]

>> This the common code would get its own callbacks and when that genpd
>> powers off, the platform genpd would power down. But no new code is
>> needed in your platform driver. The benefit is that platform code and
>> generic CPU domain code may exist and act in parallel and may only be
>> related if specified in the DT and the problem with that approach is
>> that this virtual PM domain is not a h/w domain, hence specifying in DT
>> is questionable.
>
>Indeed, I don't like this option, as the DT would no longer describe HW,
>but the Linux implementation.
>
>So let's continue with your approach, and see how it turns out. We can
>always change and improvide code, while changing DT is more complicated.
>
Agreed.

Thanks for your time, Geert.

-- Lina

Re: [PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Mon, Jul 06 2015 at 09:18 -0600, Lina Iyer wrote:
>On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote:
>>Hi Lina,
>>
>>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>>>>On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>>On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>
>[...]
>
>>>With what you have suggested, the platform driver creates the genpd and
>>>would pass the CPU genpd to the common code for common operations. (This
>>>was what was done in [1]). The platform driver would set the power_on()
>>>and power_off() callbacks and that would have to be overriden in order
>>>handle common CPU domain suspend/resume activities. Overwriting members
>>>of an object allocated by the platform driver, is something we should
>>>avoid.
>>
>>Instead of letting the generic code override the .power_{on,off}() callbacks,
>>the platform code could call the generic CPU-related methods from its own
>>.power_{on,off}() callbacks?
>>
>We dont want to impose on platform drivers to call back into the CPU
>domain core code to do common domain power down activities. On a generic
>ARM CPU domain, we may not even need a platform specific domain
>callbacks. IMHO, this is not desirable.
>
Any thoughts on this?

>>struct rmobile_pm_domain already has .suspend() and .resume() methods.
>>The former is used to e.g. prevent the PM domains containing CPUs to be
>>powered down (in the absence of cpuidle integration). That requires scanning
>>the DT for CPUs, and it would indeed be good to have that scanning support
>>in generic code.
>>The latter became unused after the removal of sh7372 support, which did have
>>some cpuidle integration.
>>
>The CPUs may still be attached to domain, even if they are not powered
>off by CPUIdle. The code increases the reference count for every online
>CPU and any CPUs that may come online. The reference count, only goes
>down when the CPU powers down through CPUIdle or hotplug. So, if a CPU
>is not integrated with CPUIdle, it would remain in use (unless powered
>off by hotplug) and the domain would not be powered down.
>
>>>Or instead of allocating the memory in your platform driver for the CPU
>>>genpd, you could query and get the genpd and then add your additions on
>>>top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
>>>overwrite the power on/off callbacks in the genpd. They still have to be
>>>registered separately like in this patch. Again, not every elegant, IMO.
>>
>>Just wondering, can I set up the .attach_dev() and .detach_dev()?
>>
>I am presuming, you are concerned with the fore mentioned case of CPUs
>not participating in CPUIdle. With pm_runtime_put_sync() not happening
>for CPUs that do not power down, you should not have to worry about the
>domain being powered down.
>
>Please correct me if I misunderstood your point.
>
>>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
>>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
>>needed for devices with MSTP clocks. CPU and L2 cache don't have these,
>>and there are no other devices in e.g. a3sm and a2sl.
>>
>>The GIC has an MSTP clock, but it's part of a different power domain.
>>
>Okay.
>
>[...]
>
>>>This the common code would get its own callbacks and when that genpd
>>>powers off, the platform genpd would power down. But no new code is
>>>needed in your platform driver. The benefit is that platform code and
>>>generic CPU domain code may exist and act in parallel and may only be
>>>related if specified in the DT and the problem with that approach is
>>>that this virtual PM domain is not a h/w domain, hence specifying in DT
>>>is questionable.
>>
>>Indeed, I don't like this option, as the DT would no longer describe HW,
>>but the Linux implementation.
>>
>>So let's continue with your approach, and see how it turns out. We can
>>always change and improvide code, while changing DT is more complicated.
>>
>Agreed.
>
>Thanks for your time, Geert.
>
>-- Lina

[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[Lina Iyer]

On Mon, Jul 06 2015 at 09:18 -0600, Lina Iyer wrote:
>On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote:
>>Hi Lina,
>>
>>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>>>>On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>>On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:
>
>[...]
>
>>>With what you have suggested, the platform driver creates the genpd and
>>>would pass the CPU genpd to the common code for common operations. (This
>>>was what was done in [1]). The platform driver would set the power_on()
>>>and power_off() callbacks and that would have to be overriden in order
>>>handle common CPU domain suspend/resume activities. Overwriting members
>>>of an object allocated by the platform driver, is something we should
>>>avoid.
>>
>>Instead of letting the generic code override the .power_{on,off}() callbacks,
>>the platform code could call the generic CPU-related methods from its own
>>.power_{on,off}() callbacks?
>>
>We dont want to impose on platform drivers to call back into the CPU
>domain core code to do common domain power down activities. On a generic
>ARM CPU domain, we may not even need a platform specific domain
>callbacks. IMHO, this is not desirable.
>
Any thoughts on this?

>>struct rmobile_pm_domain already has .suspend() and .resume() methods.
>>The former is used to e.g. prevent the PM domains containing CPUs to be
>>powered down (in the absence of cpuidle integration). That requires scanning
>>the DT for CPUs, and it would indeed be good to have that scanning support
>>in generic code.
>>The latter became unused after the removal of sh7372 support, which did have
>>some cpuidle integration.
>>
>The CPUs may still be attached to domain, even if they are not powered
>off by CPUIdle. The code increases the reference count for every online
>CPU and any CPUs that may come online. The reference count, only goes
>down when the CPU powers down through CPUIdle or hotplug. So, if a CPU
>is not integrated with CPUIdle, it would remain in use (unless powered
>off by hotplug) and the domain would not be powered down.
>
>>>Or instead of allocating the memory in your platform driver for the CPU
>>>genpd, you could query and get the genpd and then add your additions on
>>>top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
>>>overwrite the power on/off callbacks in the genpd. They still have to be
>>>registered separately like in this patch. Again, not every elegant, IMO.
>>
>>Just wondering, can I set up the .attach_dev() and .detach_dev()?
>>
>I am presuming, you are concerned with the fore mentioned case of CPUs
>not participating in CPUIdle. With pm_runtime_put_sync() not happening
>for CPUs that do not power down, you should not have to worry about the
>domain being powered down.
>
>Please correct me if I misunderstood your point.
>
>>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
>>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
>>needed for devices with MSTP clocks. CPU and L2 cache don't have these,
>>and there are no other devices in e.g. a3sm and a2sl.
>>
>>The GIC has an MSTP clock, but it's part of a different power domain.
>>
>Okay.
>
>[...]
>
>>>This the common code would get its own callbacks and when that genpd
>>>powers off, the platform genpd would power down. But no new code is
>>>needed in your platform driver. The benefit is that platform code and
>>>generic CPU domain code may exist and act in parallel and may only be
>>>related if specified in the DT and the problem with that approach is
>>>that this virtual PM domain is not a h/w domain, hence specifying in DT
>>>is questionable.
>>
>>Indeed, I don't like this option, as the DT would no longer describe HW,
>>but the Linux implementation.
>>
>>So let's continue with your approach, and see how it turns out. We can
>>always change and improvide code, while changing DT is more complicated.
>>
>Agreed.
>
>Thanks for your time, Geert.
>
>-- Lina