[PATCH 0/9] PM / Domains: Add support for multi PM domains per device

[PATCH 0/9] PM / Domains: Add support for multi PM domains per device

[Ulf Hansson]

There are devices that are partitioned across multiple PM domains. Currently
these can't be supported well by the available PM infrastructures we have in
the kernel. This series is an attempt to address this.

The interesting parts happens from patch 5 an onwards, including a minor DT
update to the existing power-domain bindings, the 4 earlier are just trivial
clean-ups of some related code in genpd, which I happened to stumble over.

Some additional background:

One existing case where devices are partitioned across multiple PM domains, is
the Nvida Tegra 124/210 X-USB subsystem. A while ago Jon Hunter (Nvidia) sent a
series, trying to address these issues, however this is a new approach, while
it re-uses the same concepts from DT point of view.

The Tegra 124/210 X-USB subsystem contains of a host controller and a device
controller. Each controller have its own independent PM domain, but are being
partitioned across another shared PM domain for the USB super-speed logic.

Currently to make the drivers work, either the related PM domains needs to stay
powered on always or the PM domain topology needs to be in-correctly modelled
through sub-domains. In both cases PM domains may be powered on while they
don't need to be, so in the end this means - wasting power -.

As stated above, this series intends to address these problem from a PM
infrastructure point of view. More details are available in each changelog.

It should be noted that this series has been tested on HW, however only by using
a home-cooked test PM domain driver for genpd and together with a test driver.
This allowed me to play with PM domain (genpd), runtime PM and device links.

Any further deployment for real use cases are greatly appreciated. I am happy to
to help, if needed!

Kind regards
Ulf Hansson


Ulf Hansson (9):
  PM / Domains: Drop extern declarations of functions in pm_domain.h
  PM / Domains: Drop __pm_genpd_add_device()
  PM / Domains: Drop genpd as in-param for pm_genpd_remove_device()
  PM / Domains: Drop unused parameter in genpd_allocate_dev_data()
  PM / Domains: dt: Allow power-domain property to be a list of phandles
  PM / Domains: Don't attach devices in genpd with multi PM domains
  PM / Domains: Split genpd_dev_pm_attach()
  PM / Domains: Add support for multi PM domains per device to genpd
  PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM
    domains

 .../devicetree/bindings/power/power_domain.txt     |  18 ++-
 drivers/base/power/common.c                        |  33 ++++-
 drivers/base/power/domain.c                        | 154 ++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c            |   2 +-
 include/linux/pm_domain.h                          |  79 +++++------
 5 files changed, 208 insertions(+), 78 deletions(-)

-- 
2.7.4

[PATCH 1/9] PM / Domains: Drop extern declarations of functions in pm_domain.h

[Ulf Hansson]

Using "extern" to declare a function in a public header file is somewhat
pointless, but also doesn't hurt. However, to make all the function
declarations in pm_domain.h to be consistent, let's drop the use of
"extern".

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm_domain.h | 51 +++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 4e57640..c847e9a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -143,21 +143,17 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 	return to_gpd_data(dev->power.subsys_data->domain_data);
 }
 
-extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
-				 struct device *dev,
-				 struct gpd_timing_data *td);
-
-extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
-				  struct device *dev);
-extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
-				  struct generic_pm_domain *new_subdomain);
-extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
-				     struct generic_pm_domain *target);
-extern int pm_genpd_init(struct generic_pm_domain *genpd,
-			 struct dev_power_governor *gov, bool is_off);
-extern int pm_genpd_remove(struct generic_pm_domain *genpd);
-extern int dev_pm_genpd_set_performance_state(struct device *dev,
-					      unsigned int state);
+int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+			  struct gpd_timing_data *td);
+int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
+int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
+			   struct generic_pm_domain *new_subdomain);
+int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
+			      struct generic_pm_domain *target);
+int pm_genpd_init(struct generic_pm_domain *genpd,
+		  struct dev_power_governor *gov, bool is_off);
+int pm_genpd_remove(struct generic_pm_domain *genpd);
+int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -215,8 +211,8 @@ static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
 }
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_SLEEP
-extern void pm_genpd_syscore_poweroff(struct device *dev);
-extern void pm_genpd_syscore_poweron(struct device *dev);
+void pm_genpd_syscore_poweroff(struct device *dev);
+void pm_genpd_syscore_poweron(struct device *dev);
 #else
 static inline void pm_genpd_syscore_poweroff(struct device *dev) {}
 static inline void pm_genpd_syscore_poweron(struct device *dev) {}
@@ -240,14 +236,13 @@ int of_genpd_add_provider_simple(struct device_node *np,
 int of_genpd_add_provider_onecell(struct device_node *np,
 				  struct genpd_onecell_data *data);
 void of_genpd_del_provider(struct device_node *np);
-extern int of_genpd_add_device(struct of_phandle_args *args,
-			       struct device *dev);
-extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
-				  struct of_phandle_args *new_subdomain);
-extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
-extern int of_genpd_parse_idle_states(struct device_node *dn,
-			struct genpd_power_state **states, int *n);
-extern unsigned int of_genpd_opp_to_performance_state(struct device *dev,
+int of_genpd_add_device(struct of_phandle_args *args, struct device *dev);
+int of_genpd_add_subdomain(struct of_phandle_args *parent,
+			   struct of_phandle_args *new_subdomain);
+struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
+int of_genpd_parse_idle_states(struct device_node *dn,
+			       struct genpd_power_state **states, int *n);
+unsigned int of_genpd_opp_to_performance_state(struct device *dev,
 				struct device_node *opp_node);
 
 int genpd_dev_pm_attach(struct device *dev);
@@ -304,9 +299,9 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 #ifdef CONFIG_PM
-extern int dev_pm_domain_attach(struct device *dev, bool power_on);
-extern void dev_pm_domain_detach(struct device *dev, bool power_off);
-extern void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
+int dev_pm_domain_attach(struct device *dev, bool power_on);
+void dev_pm_domain_detach(struct device *dev, bool power_off);
+void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
-- 
2.7.4

[PATCH 2/9] PM / Domains: Drop __pm_genpd_add_device()

[Ulf Hansson]

There are still a few non-DT existing users of genpd, however neither of
them uses __pm_genpd_add_device(), hence let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 10 ++++------
 include/linux/pm_domain.h   | 14 +++-----------
 2 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index da6c886..d58aee3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1414,23 +1414,21 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 }
 
 /**
- * __pm_genpd_add_device - Add a device to an I/O PM domain.
+ * pm_genpd_add_device - Add a device to an I/O PM domain.
  * @genpd: PM domain to add the device to.
  * @dev: Device to be added.
- * @td: Set of PM QoS timing parameters to attach to the device.
  */
-int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
-			  struct gpd_timing_data *td)
+int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
 {
 	int ret;
 
 	mutex_lock(&gpd_list_lock);
-	ret = genpd_add_device(genpd, dev, td);
+	ret = genpd_add_device(genpd, dev, NULL);
 	mutex_unlock(&gpd_list_lock);
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
+EXPORT_SYMBOL_GPL(pm_genpd_add_device);
 
 static int genpd_remove_device(struct generic_pm_domain *genpd,
 			       struct device *dev)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index c847e9a..79888fb 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -143,8 +143,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 	return to_gpd_data(dev->power.subsys_data->domain_data);
 }
 
-int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
-			  struct gpd_timing_data *td);
+int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
 int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *new_subdomain);
@@ -163,9 +162,8 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 {
 	return ERR_PTR(-ENOSYS);
 }
-static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
-					struct device *dev,
-					struct gpd_timing_data *td)
+static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
+				      struct device *dev)
 {
 	return -ENOSYS;
 }
@@ -204,12 +202,6 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
 #define pm_domain_always_on_gov		(*(struct dev_power_governor *)(NULL))
 #endif
 
-static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-				      struct device *dev)
-{
-	return __pm_genpd_add_device(genpd, dev, NULL);
-}
-
 #ifdef CONFIG_PM_GENERIC_DOMAINS_SLEEP
 void pm_genpd_syscore_poweroff(struct device *dev);
 void pm_genpd_syscore_poweron(struct device *dev);
-- 
2.7.4

[PATCH 3/9] PM / Domains: Drop genpd as in-param for pm_genpd_remove_device()

[Ulf Hansson]

There is no need to pass a genpd struct to pm_genpd_remove_device(), as we
already have the information about the PM domain (genpd) through the device
structure.

Additionally, we don't allow to remove a PM domain from a device, other
than the one it may have assigned to it, so really it does not make sense
to have a separate in-param for it.

For these reason, drop it and update the current only call to
pm_genpd_remove_device() from amdgpu_acp.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: David (ChunMing) Zhou <David1.Zhou@amd.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c             | 8 ++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
 include/linux/pm_domain.h               | 5 ++---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d58aee3..f08fa15 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1475,13 +1475,13 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
 /**
  * pm_genpd_remove_device - Remove a device from an I/O PM domain.
- * @genpd: PM domain to remove the device from.
  * @dev: Device to be removed.
  */
-int pm_genpd_remove_device(struct generic_pm_domain *genpd,
-			   struct device *dev)
+int pm_genpd_remove_device(struct device *dev)
 {
-	if (!genpd || genpd != genpd_lookup_dev(dev))
+	struct generic_pm_domain *genpd = genpd_lookup_dev(dev);
+
+	if (!genpd)
 		return -EINVAL;
 
 	return genpd_remove_device(genpd, dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index a29362f..1255804 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -513,7 +513,7 @@ static int acp_hw_fini(void *handle)
 	if (adev->acp.acp_genpd) {
 		for (i = 0; i < ACP_DEVS ; i++) {
 			dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i);
-			ret = pm_genpd_remove_device(&adev->acp.acp_genpd->gpd, dev);
+			ret = pm_genpd_remove_device(dev);
 			/* If removal fails, dont giveup and try rest */
 			if (ret)
 				dev_err(dev, "remove dev from genpd failed\n");
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 79888fb..42e0d64 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -144,7 +144,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 }
 
 int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
-int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
+int pm_genpd_remove_device(struct device *dev);
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *new_subdomain);
 int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
@@ -167,8 +167,7 @@ static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
 {
 	return -ENOSYS;
 }
-static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
-					 struct device *dev)
+static inline int pm_genpd_remove_device(struct device *dev)
 {
 	return -ENOSYS;
 }
-- 
2.7.4

[PATCH 4/9] PM / Domains: Drop unused parameter in genpd_allocate_dev_data()

[Ulf Hansson]

The in-parameter struct generic_pm_domain *genpd to
genpd_allocate_dev_data() is unused, so let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@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 f08fa15..050ce07 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1316,7 +1316,6 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 #endif /* CONFIG_PM_SLEEP */
 
 static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
-					struct generic_pm_domain *genpd,
 					struct gpd_timing_data *td)
 {
 	struct generic_pm_domain_data *gpd_data;
@@ -1385,7 +1384,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
-	gpd_data = genpd_alloc_dev_data(dev, genpd, td);
+	gpd_data = genpd_alloc_dev_data(dev, td);
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
-- 
2.7.4

[PATCH 5/9] PM / Domains: dt: Allow power-domain property to be a list of phandles

[Ulf Hansson]

To be able to describe topologies where devices are partitioned across
multiple power domains, let's extend the power-domain property to allow
being a list of phandles.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Suggested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 .../devicetree/bindings/power/power_domain.txt         | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 4733f76..b27ae7f 100644
--- a/Documentation/devicetree/bindings/power/power_domain.txt
+++ b/Documentation/devicetree/bindings/power/power_domain.txt
@@ -111,8 +111,9 @@ Example 3:
 ==PM domain consumers==
 
 Required properties:
- - power-domains : A phandle and PM domain specifier as defined by bindings of
-                   the power controller specified by phandle.
+ - power-domains : A phandle and PM domain specifier, or a list of phandles, as
+		   defined by bindings of the power controller specified by
+		   phandle.
 
 Example:
 
@@ -122,9 +123,16 @@ Example:
 		power-domains = <&power 0>;
 	};
 
-The node above defines a typical PM domain consumer device, which is located
-inside a PM domain with index 0 of a power controller represented by a node
-with the label "power".
+	leaky-device@12350000 {
+		compatible = "foo,i-leak-current";
+		reg = <0x12350000 0x1000>;
+		power-domains = <&power0 0>, <&power1 0> ;
+	};
+
+The first example above defines a typical PM domain consumer device, which is
+located inside a PM domain with index 0 of a power controller represented by a
+node with the label "power". In the second example the consumer device are
+partitioned across two PM domains.
 
 Optional properties:
 - required-opps: This contains phandle to an OPP node in another device's OPP
-- 
2.7.4

[PATCH 6/9] PM / Domains: Don't attach devices in genpd with multi PM domains

[Ulf Hansson]

The power-domain DT property may now contain a list of phandles, which
represents that a device are partitioned across multiple PM domains. This
leads to a new situation in genpd_dev_pm_attach(), as only one PM domain
can be attached per device.

To remain things simple for the most common configuration, when a single PM
domain is used, let's treat the multiple PM domain case as being specific.

In other words, let's change genpd_dev_pm_attach() to check for multiple PM
domains and prevent it from attach any PM domain for this case. Instead,
leave this to be managed separately, from following changes to genpd.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Suggested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 050ce07..4597e1c 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2229,10 +2229,10 @@ static void genpd_dev_pm_sync(struct device *dev)
  * attaches the device to retrieved pm_domain ops.
  *
  * Returns 1 on successfully attached PM domain, 0 when the device don't need a
- * PM domain or a negative error code in case of failures. Note that if a
- * power-domain exists for the device, but it cannot be found or turned on,
- * then return -EPROBE_DEFER to ensure that the device is not probed and to
- * re-try again later.
+ * PM domain or when multiple power-domains exists for it, else a negative error
+ * code. Note that if a power-domain exists for the device, but it cannot be
+ * found or turned on, then return -EPROBE_DEFER to ensure that the device is
+ * not probed and to re-try again later.
  */
 int genpd_dev_pm_attach(struct device *dev)
 {
@@ -2243,10 +2243,18 @@ int genpd_dev_pm_attach(struct device *dev)
 	if (!dev->of_node)
 		return 0;
 
+	/*
+	 * Devices with multiple PM domains must be attached separately, as we
+	 * can only attach one PM domain per device.
+	 */
+	if (of_count_phandle_with_args(dev->of_node, "power-domains",
+				       "#power-domain-cells") != 1)
+		return 0;
+
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
 					"#power-domain-cells", 0, &pd_args);
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	mutex_lock(&gpd_list_lock);
 	pd = genpd_get_from_provider(&pd_args);
-- 
2.7.4

[PATCH 7/9] PM / Domains: Split genpd_dev_pm_attach()

[Ulf Hansson]

To extend genpd to deal with allowing multiple PM domains per device, some
of the code in genpd_dev_pm_attach() can be re-used. Let's prepare for this
by moving some of the code into a sub-function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 60 +++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4597e1c..d538640 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2221,38 +2221,15 @@ static void genpd_dev_pm_sync(struct device *dev)
 	genpd_queue_power_off_work(pd);
 }
 
-/**
- * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
- * @dev: Device to attach.
- *
- * Parse device's OF node to find a PM domain specifier. If such is found,
- * attaches the device to retrieved pm_domain ops.
- *
- * Returns 1 on successfully attached PM domain, 0 when the device don't need a
- * PM domain or when multiple power-domains exists for it, else a negative error
- * code. Note that if a power-domain exists for the device, but it cannot be
- * found or turned on, then return -EPROBE_DEFER to ensure that the device is
- * not probed and to re-try again later.
- */
-int genpd_dev_pm_attach(struct device *dev)
+static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
+				 unsigned int index)
 {
 	struct of_phandle_args pd_args;
 	struct generic_pm_domain *pd;
 	int ret;
 
-	if (!dev->of_node)
-		return 0;
-
-	/*
-	 * Devices with multiple PM domains must be attached separately, as we
-	 * can only attach one PM domain per device.
-	 */
-	if (of_count_phandle_with_args(dev->of_node, "power-domains",
-				       "#power-domain-cells") != 1)
-		return 0;
-
-	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
-					"#power-domain-cells", 0, &pd_args);
+	ret = of_parse_phandle_with_args(np, "power-domains",
+				"#power-domain-cells", index, &pd_args);
 	if (ret < 0)
 		return ret;
 
@@ -2290,6 +2267,35 @@ int genpd_dev_pm_attach(struct device *dev)
 
 	return ret ? -EPROBE_DEFER : 1;
 }
+
+/**
+ * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
+ * @dev: Device to attach.
+ *
+ * Parse device's OF node to find a PM domain specifier. If such is found,
+ * attaches the device to retrieved pm_domain ops.
+ *
+ * Returns 1 on successfully attached PM domain, 0 when the device don't need a
+ * PM domain or when multiple power-domains exists for it, else a negative error
+ * code. Note that if a power-domain exists for the device, but it cannot be
+ * found or turned on, then return -EPROBE_DEFER to ensure that the device is
+ * not probed and to re-try again later.
+ */
+int genpd_dev_pm_attach(struct device *dev)
+{
+	if (!dev->of_node)
+		return 0;
+
+	/*
+	 * Devices with multiple PM domains must be attached separately, as we
+	 * can only attach one PM domain per device.
+	 */
+	if (of_count_phandle_with_args(dev->of_node, "power-domains",
+				       "#power-domain-cells") != 1)
+		return 0;
+
+	return __genpd_dev_pm_attach(dev, dev->of_node, 0);
+}
 EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 
 static const struct of_device_id idle_state_match[] = {
-- 
2.7.4

[PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

To support devices being partitioned across multiple PM domains, let's
start by extending genpd to cope with these configurations.

More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
similar to genpd_dev_pm_attach(), but the new function also allows the
caller to provide an index to what PM domain it wants to attach.

Furthermore, let genpd register a new virtual struct device via calling
device_register() and attach it to the corresponding PM domain, which is
looked up via calling the existing genpd OF functions. Note that the new
device is needed, because only one PM domain can be attached per device. At
successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
allowing the caller to operate on it to deal with power management.

To deal with detaching of a PM domain for multiple PM domain case, we can
still re-use the existing genpd_dev_pm_detach() function, although we need
to extend it to cover cleanup of the earlier registered device, via calling
device_unregister().

An important note, genpd_dev_pm_attach_by_id() shall only be called by the
driver core / PM core, similar to how genpd_dev_pm_attach() is used.
Following changes deploys this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 +++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index d538640..ffeb6ea 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_genpd_remove_last);
 
+static void genpd_release_dev(struct device *dev)
+{
+	kfree(dev);
+}
+
+static struct bus_type genpd_bus_type = {
+	.name		= "genpd",
+};
+
 /**
  * genpd_dev_pm_detach - Detach a device from its PM domain.
  * @dev: Device to detach.
@@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 
 	/* Check if PM domain can be powered off after removing this device. */
 	genpd_queue_power_off_work(pd);
+
+	/* Unregister the device if it was created by genpd. */
+	if (dev->bus == &genpd_bus_type)
+		device_unregister(dev);
 }
 
 static void genpd_dev_pm_sync(struct device *dev)
@@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
 
+/**
+ * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
+ * @dev: Device to attach.
+ * @index: The index of the PM domain.
+ *
+ * Parse device's OF node to find a PM domain specifier at the provided @index.
+ * If such is found, allocates a new device and attaches it to retrieved
+ * pm_domain ops.
+ *
+ * Returns the allocated device if successfully attached PM domain, NULL when
+ * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
+ * in case of failures. Note that if a power-domain exists for the device, but
+ * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
+ * that the device is not probed and to re-try again later.
+ */
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+					 unsigned int index)
+{
+	struct device *genpd_dev;
+	int num_domains;
+	int ret;
+
+	if (!dev->of_node)
+		return NULL;
+
+	/* Deal only with devices using multiple PM domains. */
+	num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
+						 "#power-domain-cells");
+	if (num_domains < 2 || index >= num_domains)
+		return NULL;
+
+	/* Allocate and register device on the genpd bus. */
+	genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
+	if (!genpd_dev)
+		return ERR_PTR(-ENOMEM);
+
+	dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
+	genpd_dev->bus = &genpd_bus_type;
+	genpd_dev->release = genpd_release_dev;
+
+	ret = device_register(genpd_dev);
+	if (ret) {
+		kfree(genpd_dev);
+		return ERR_PTR(ret);
+	}
+
+	/* Try to attach the device to the PM domain at the specified index. */
+	ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
+	if (ret < 1) {
+		device_unregister(genpd_dev);
+		return ret ? ERR_PTR(ret) : NULL;
+	}
+
+	pm_runtime_set_active(genpd_dev);
+	pm_runtime_enable(genpd_dev);
+
+	return genpd_dev;
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
+
 static const struct of_device_id idle_state_match[] = {
 	{ .compatible = "domain-idle-state", },
 	{ }
@@ -2456,6 +2529,12 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(of_genpd_opp_to_performance_state);
 
+static int __init genpd_bus_init(void)
+{
+	return bus_register(&genpd_bus_type);
+}
+core_initcall(genpd_bus_init);
+
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 42e0d64..82458e8 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -237,6 +237,8 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev,
 				struct device_node *opp_node);
 
 int genpd_dev_pm_attach(struct device *dev);
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+					 unsigned int index);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
 static inline int of_genpd_add_provider_simple(struct device_node *np,
 					struct generic_pm_domain *genpd)
@@ -282,6 +284,12 @@ static inline int genpd_dev_pm_attach(struct device *dev)
 	return 0;
 }
 
+static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+						       unsigned int index)
+{
+	return NULL;
+}
+
 static inline
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 {
-- 
2.7.4

[PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Ulf Hansson]

The existing dev_pm_domain_attach() function, allows a single PM domain to
be attached per device. To be able to support devices that are partitioned
across multiple PM domains, let's introduce a new interface,
dev_pm_domain_attach_by_id().

The dev_pm_domain_attach_by_id() returns a new allocated struct device with
the corresponding attached PM domain. This enables for example a driver to
operate on the new device from a power management point of view. The driver
may then also benefit from using the received device, to set up so called
device-links towards its original device. Depending on the situation, these
links may then be dynamically changed.

The new interface is typically called by drivers during their probe phase,
in case they manages devices which uses multiple PM domains. If that is the
case, the driver also becomes responsible of managing the detaching of the
PM domains, which typically should be done at the remove phase. Detaching
is done by calling the existing dev_pm_domain_detach() function and for
each of the received devices from dev_pm_domain_attach_by_id().

Note, currently its only genpd that supports multiple PM domains per
device, but dev_pm_domain_attach_by_id() can easily by extended to cover
other PM domain types, if/when needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
 include/linux/pm_domain.h   |  7 +++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 7ae62b6..d3db974 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
 EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
 
 /**
+ * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
+ * @index: The index of the PM domain.
+ * @dev: Device to attach.
+ *
+ * As @dev may only be attached to a single PM domain, the backend PM domain
+ * provider should create a virtual device to attach instead. As attachment
+ * succeeds, the ->detach() callback in the struct dev_pm_domain should be
+ * assigned by the corresponding backend attach function.
+ *
+ * This function should typically be invoked from drivers during probe phase.
+ * Especially for those that manages devices which requires power management
+ * through more than one PM domain.
+ *
+ * Callers must ensure proper synchronization of this function with power
+ * management callbacks.
+ *
+ * Returns the virtual attached device in case successfully attached PM domain,
+ * NULL in case @dev don't need a PM domain, else a PTR_ERR().
+ */
+struct device *dev_pm_domain_attach_by_id(struct device *dev,
+					  unsigned int index)
+{
+	if (dev->pm_domain)
+		return NULL;
+
+	return genpd_dev_pm_attach_by_id(dev, index);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_id);
+
+/**
  * dev_pm_domain_detach - Detach a device from its PM domain.
  * @dev: Device to detach.
  * @power_off: Used to indicate whether we should power off the device.
  *
  * This functions will reverse the actions from dev_pm_domain_attach() and thus
  * try to detach the @dev from its PM domain. Typically it should be invoked
- * from subsystem level code during the remove phase.
+ * during the remove phase, either from subsystem level code or from drivers in
+ * case attaching was done through dev_pm_domain_attach_by_id.
  *
  * Callers must ensure proper synchronization of this function with power
  * management callbacks.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 82458e8..493ce67 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -299,6 +299,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 
 #ifdef CONFIG_PM
 int dev_pm_domain_attach(struct device *dev, bool power_on);
+struct device *dev_pm_domain_attach_by_id(struct device *dev,
+					  unsigned int index);
 void dev_pm_domain_detach(struct device *dev, bool power_off);
 void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
 #else
@@ -306,6 +308,11 @@ static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }
+static inline struct device *dev_pm_domain_attach_by_id(struct device *dev,
+							unsigned int index);
+{
+	return NULL;
+}
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
 static inline void dev_pm_domain_set(struct device *dev,
 				     struct dev_pm_domain *pd) {}
-- 
2.7.4

Re: [PATCH 5/9] PM / Domains: dt: Allow power-domain property to be a list of phandles

[Geert Uytterhoeven]

Hi Ulf,

On Fri, May 18, 2018 at 12:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> To be able to describe topologies where devices are partitioned across
> multiple power domains, let's extend the power-domain property to allow
> being a list of phandles.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Suggested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -111,8 +111,9 @@ Example 3:
>  ==PM domain consumers==
>
>  Required properties:
> - - power-domains : A phandle and PM domain specifier as defined by bindings of
> -                   the power controller specified by phandle.
> + - power-domains : A phandle and PM domain specifier, or a list of phandles, as

A list of PM domain specifiers?

(A PM domain specifier consists of a phandle, and zero or more indices)

> +                  defined by bindings of the power controller specified by
> +                  phandle.
>
>  Example:
>
> @@ -122,9 +123,16 @@ Example:
>                 power-domains = <&power 0>;
>         };
>
> -The node above defines a typical PM domain consumer device, which is located
> -inside a PM domain with index 0 of a power controller represented by a node
> -with the label "power".
> +       leaky-device@12350000 {

I know it's just an example, but this uses the same unit-address and
reg property
as the device node above.

There's a similar issue with the two other examples below, but they
use different
node names (leaky-device0 and leaky-device1).

> +               compatible = "foo,i-leak-current";
> +               reg = <0x12350000 0x1000>;
> +               power-domains = <&power0 0>, <&power1 0> ;
> +       };
> +
> +The first example above defines a typical PM domain consumer device, which is
> +located inside a PM domain with index 0 of a power controller represented by a
> +node with the label "power". In the second example the consumer device are
> +partitioned across two PM domains.
>
>  Optional properties:
>  - required-opps: This contains phandle to an OPP node in another device's OPP

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

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

Hi Ulf,

On 18/05/18 11:31, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> start by extending genpd to cope with these configurations.
> 
> More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
> similar to genpd_dev_pm_attach(), but the new function also allows the
> caller to provide an index to what PM domain it wants to attach.
> 
> Furthermore, let genpd register a new virtual struct device via calling
> device_register() and attach it to the corresponding PM domain, which is
> looked up via calling the existing genpd OF functions. Note that the new
> device is needed, because only one PM domain can be attached per device. At
> successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
> allowing the caller to operate on it to deal with power management.
> 
> To deal with detaching of a PM domain for multiple PM domain case, we can
> still re-use the existing genpd_dev_pm_detach() function, although we need
> to extend it to cover cleanup of the earlier registered device, via calling
> device_unregister().
> 
> An important note, genpd_dev_pm_attach_by_id() shall only be called by the
> driver core / PM core, similar to how genpd_dev_pm_attach() is used.
> Following changes deploys this.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/base/power/domain.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pm_domain.h   |  8 +++++
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d538640..ffeb6ea 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
>   }
>   EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>   
> +static void genpd_release_dev(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> +	.name		= "genpd",
> +};
> +
>   /**
>    * genpd_dev_pm_detach - Detach a device from its PM domain.
>    * @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>   
>   	/* Check if PM domain can be powered off after removing this device. */
>   	genpd_queue_power_off_work(pd);
> +
> +	/* Unregister the device if it was created by genpd. */
> +	if (dev->bus == &genpd_bus_type)
> +		device_unregister(dev);
>   }
>   
>   static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>   
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL when
> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the device, but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> +					 unsigned int index)
> +{
> +	struct device *genpd_dev;
> +	int num_domains;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	/* Deal only with devices using multiple PM domains. */
> +	num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> +						 "#power-domain-cells");
> +	if (num_domains < 2 || index >= num_domains)
> +		return NULL;
> +
> +	/* Allocate and register device on the genpd bus. */
> +	genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> +	if (!genpd_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> +	genpd_dev->bus = &genpd_bus_type;
> +	genpd_dev->release = genpd_release_dev;
> +
> +	ret = device_register(genpd_dev);
> +	if (ret) {
> +		kfree(genpd_dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* Try to attach the device to the PM domain at the specified index. */
> +	ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> +	if (ret < 1) {
> +		device_unregister(genpd_dev);
> +		return ret ? ERR_PTR(ret) : NULL;
> +	}
> +
> +	pm_runtime_set_active(genpd_dev);
> +	pm_runtime_enable(genpd_dev);
> +
> +	return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);

Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?

If so, I can see that that would work, but it does not seem to fit the RPM model
where currently for something like device clocks, the RPM callbacks can handle
all clocks at once.

I was wondering why we could not add a list of genpd domains to the
dev_pm_domain struct for the device? For example ...

diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d8357..a11d6db3c077 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
  * subsystem-level and driver-level callbacks.
  */
 struct dev_pm_domain {
+       struct list_head        genpd_list;
        struct dev_pm_ops       ops;
        void (*detach)(struct device *dev, bool power_off);
        int (*activate)(struct device *dev);
@@ -666,6 +667,11 @@ struct dev_pm_domain {
        void (*dismiss)(struct device *dev);
 };

+struct dev_pm_domain_link {
+       struct generic_pm_domain *genpd;
+       struct list_head node;
+};
+
 /*
  * The PM_EVENT_ messages are also used by drivers implementing the legacy
  * suspend framework, based on the ->suspend() and ->resume() callbacks common
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index e61b5cd79fe2..019593804670 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -51,7 +51,6 @@ struct dev_pm_opp;

 struct generic_pm_domain {
        struct device dev;
-       struct dev_pm_domain domain;    /* PM domain operations */
        struct list_head gpd_list_node; /* Node in the global PM domains list */
        struct list_head master_links;  /* Links with PM domain as a master */
        struct list_head slave_links;   /* Links with PM domain as a slave */
@@ -99,11 +98,6 @@ struct generic_pm_domain {

 };

-static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-{
-       return container_of(pd, struct generic_pm_domain, domain);
-}
-

Obviously the above will not compile but the intent would be to allocate a
dev_pm_domain_link struct per power-domain that the device needs and add
to the genpd_list for the device. It would be a much bigger change in
having to iterate through all the power-domains when turning devices on
and off, however, it would simplify the client driver. 

Cheers
Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>
>> +/**
>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> + * @dev: Device to attach.
>> + * @index: The index of the PM domain.
>> + *
>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>> + * If such is found, allocates a new device and attaches it to retrieved
>> + * pm_domain ops.
>> + *
>> + * Returns the allocated device if successfully attached PM domain, NULL when
>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>> + * in case of failures. Note that if a power-domain exists for the device, but
>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>> + * that the device is not probed and to re-try again later.
>> + */
>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>> +                                      unsigned int index)
>> +{
>> +     struct device *genpd_dev;
>> +     int num_domains;
>> +     int ret;
>> +
>> +     if (!dev->of_node)
>> +             return NULL;
>> +
>> +     /* Deal only with devices using multiple PM domains. */
>> +     num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>> +                                              "#power-domain-cells");
>> +     if (num_domains < 2 || index >= num_domains)
>> +             return NULL;
>> +
>> +     /* Allocate and register device on the genpd bus. */
>> +     genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>> +     if (!genpd_dev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>> +     genpd_dev->bus = &genpd_bus_type;
>> +     genpd_dev->release = genpd_release_dev;
>> +
>> +     ret = device_register(genpd_dev);
>> +     if (ret) {
>> +             kfree(genpd_dev);
>> +             return ERR_PTR(ret);
>> +     }
>> +
>> +     /* Try to attach the device to the PM domain at the specified index. */
>> +     ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>> +     if (ret < 1) {
>> +             device_unregister(genpd_dev);
>> +             return ret ? ERR_PTR(ret) : NULL;
>> +     }
>> +
>> +     pm_runtime_set_active(genpd_dev);
>> +     pm_runtime_enable(genpd_dev);
>> +
>> +     return genpd_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>
> Thanks for sending this. Believe it or not this has still been on my to-do list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains exposed as devices
> to the client device. So I assume that this means that the drivers for devices
> with multiple power-domains will need to call RPM APIs for each of these
> additional power-domains. Is that correct?

They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.

>
> If so, I can see that that would work, but it does not seem to fit the RPM model
> where currently for something like device clocks, the RPM callbacks can handle
> all clocks at once.
>
> I was wondering why we could not add a list of genpd domains to the
> dev_pm_domain struct for the device? For example ...

See above answer, hopefully that explains it.

FYI, that's why I also discovered the bug around using device links
with runtime PM during probe.
https://patchwork.kernel.org/patch/10408825/

>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..a11d6db3c077 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
>   * subsystem-level and driver-level callbacks.
>   */
>  struct dev_pm_domain {
> +       struct list_head        genpd_list;
>         struct dev_pm_ops       ops;
>         void (*detach)(struct device *dev, bool power_off);
>         int (*activate)(struct device *dev);
> @@ -666,6 +667,11 @@ struct dev_pm_domain {
>         void (*dismiss)(struct device *dev);
>  };
>
> +struct dev_pm_domain_link {
> +       struct generic_pm_domain *genpd;
> +       struct list_head node;
> +};
> +
>  /*
>   * The PM_EVENT_ messages are also used by drivers implementing the legacy
>   * suspend framework, based on the ->suspend() and ->resume() callbacks common
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e61b5cd79fe2..019593804670 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,7 +51,6 @@ struct dev_pm_opp;
>
>  struct generic_pm_domain {
>         struct device dev;
> -       struct dev_pm_domain domain;    /* PM domain operations */
>         struct list_head gpd_list_node; /* Node in the global PM domains list */
>         struct list_head master_links;  /* Links with PM domain as a master */
>         struct list_head slave_links;   /* Links with PM domain as a slave */
> @@ -99,11 +98,6 @@ struct generic_pm_domain {
>
>  };
>
> -static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
> -{
> -       return container_of(pd, struct generic_pm_domain, domain);
> -}
> -
>
> Obviously the above will not compile but the intent would be to allocate a
> dev_pm_domain_link struct per power-domain that the device needs and add
> to the genpd_list for the device. It would be a much bigger change in
> having to iterate through all the power-domains when turning devices on
> and off, however, it would simplify the client driver.
>
> Cheers
> Jon
>
> --
> nvpublic

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 22/05/18 15:47, Ulf Hansson wrote:
> [...]
> 
>>>
>>> +/**
>>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> + * @dev: Device to attach.
>>> + * @index: The index of the PM domain.
>>> + *
>>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>>> + * If such is found, allocates a new device and attaches it to retrieved
>>> + * pm_domain ops.
>>> + *
>>> + * Returns the allocated device if successfully attached PM domain, NULL when
>>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>>> + * in case of failures. Note that if a power-domain exists for the device, but
>>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>>> + * that the device is not probed and to re-try again later.
>>> + */
>>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>>> +                                      unsigned int index)
>>> +{
>>> +     struct device *genpd_dev;
>>> +     int num_domains;
>>> +     int ret;
>>> +
>>> +     if (!dev->of_node)
>>> +             return NULL;
>>> +
>>> +     /* Deal only with devices using multiple PM domains. */
>>> +     num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>>> +                                              "#power-domain-cells");
>>> +     if (num_domains < 2 || index >= num_domains)
>>> +             return NULL;
>>> +
>>> +     /* Allocate and register device on the genpd bus. */
>>> +     genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>>> +     if (!genpd_dev)
>>> +             return ERR_PTR(-ENOMEM);
>>> +
>>> +     dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>>> +     genpd_dev->bus = &genpd_bus_type;
>>> +     genpd_dev->release = genpd_release_dev;
>>> +
>>> +     ret = device_register(genpd_dev);
>>> +     if (ret) {
>>> +             kfree(genpd_dev);
>>> +             return ERR_PTR(ret);
>>> +     }
>>> +
>>> +     /* Try to attach the device to the PM domain at the specified index. */
>>> +     ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>>> +     if (ret < 1) {
>>> +             device_unregister(genpd_dev);
>>> +             return ret ? ERR_PTR(ret) : NULL;
>>> +     }
>>> +
>>> +     pm_runtime_set_active(genpd_dev);
>>> +     pm_runtime_enable(genpd_dev);
>>> +
>>> +     return genpd_dev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>
>> Thanks for sending this. Believe it or not this has still been on my to-do list
>> and so we definitely need a solution for Tegra.
>>
>> Looking at the above it appears that additional power-domains exposed as devices
>> to the client device. So I assume that this means that the drivers for devices
>> with multiple power-domains will need to call RPM APIs for each of these
>> additional power-domains. Is that correct?
> 
> They can, but should not!
> 
> Instead, the driver shall use device_link_add() and device_link_del(),
> dynamically, depending on what PM domain that their original device
> needs for the current running use case.
> 
> In that way, they keep existing runtime PM deployment, operating on
> its original device.

OK, sounds good. Any reason why the linking cannot be handled by the 
above API? Is there a use-case where you would not want it linked?

Thanks
Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:25 AM, Jon Hunter wrote:
> 
> On 22/05/18 15:47, Ulf Hansson wrote:
>> [...]
>>
>>>>
>>>> +/**
>>>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>>> + * @dev: Device to attach.
>>>> + * @index: The index of the PM domain.
>>>> + *
>>>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>>>> + * If such is found, allocates a new device and attaches it to retrieved
>>>> + * pm_domain ops.
>>>> + *
>>>> + * Returns the allocated device if successfully attached PM domain, NULL when
>>>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>>>> + * in case of failures. Note that if a power-domain exists for the device, but
>>>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>>>> + * that the device is not probed and to re-try again later.
>>>> + */
>>>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>>>> +                                      unsigned int index)
>>>> +{
>>>> +     struct device *genpd_dev;
>>>> +     int num_domains;
>>>> +     int ret;
>>>> +
>>>> +     if (!dev->of_node)
>>>> +             return NULL;
>>>> +
>>>> +     /* Deal only with devices using multiple PM domains. */
>>>> +     num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>>>> +                                              "#power-domain-cells");
>>>> +     if (num_domains < 2 || index >= num_domains)
>>>> +             return NULL;
>>>> +
>>>> +     /* Allocate and register device on the genpd bus. */
>>>> +     genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>>>> +     if (!genpd_dev)
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +
>>>> +     dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>>>> +     genpd_dev->bus = &genpd_bus_type;
>>>> +     genpd_dev->release = genpd_release_dev;
>>>> +
>>>> +     ret = device_register(genpd_dev);
>>>> +     if (ret) {
>>>> +             kfree(genpd_dev);
>>>> +             return ERR_PTR(ret);
>>>> +     }
>>>> +
>>>> +     /* Try to attach the device to the PM domain at the specified index. */
>>>> +     ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>>>> +     if (ret < 1) {
>>>> +             device_unregister(genpd_dev);
>>>> +             return ret ? ERR_PTR(ret) : NULL;
>>>> +     }
>>>> +
>>>> +     pm_runtime_set_active(genpd_dev);
>>>> +     pm_runtime_enable(genpd_dev);
>>>> +
>>>> +     return genpd_dev;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>>
>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>> and so we definitely need a solution for Tegra.
>>>
>>> Looking at the above it appears that additional power-domains exposed as devices
>>> to the client device. So I assume that this means that the drivers for devices
>>> with multiple power-domains will need to call RPM APIs for each of these
>>> additional power-domains. Is that correct?
>>
>> They can, but should not!
>>
>> Instead, the driver shall use device_link_add() and device_link_del(),
>> dynamically, depending on what PM domain that their original device
>> needs for the current running use case.
>>
>> In that way, they keep existing runtime PM deployment, operating on
>> its original device.
> 
> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?

I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

Rajendra, Jon,

On 23 May 2018 at 06:51, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 05/23/2018 02:25 AM, Jon Hunter wrote:
>>
>> On 22/05/18 15:47, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>
>>>>> +/**
>>>>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>>>> + * @dev: Device to attach.
>>>>> + * @index: The index of the PM domain.
>>>>> + *
>>>>> + * Parse device's OF node to find a PM domain specifier at the provided @index.
>>>>> + * If such is found, allocates a new device and attaches it to retrieved
>>>>> + * pm_domain ops.
>>>>> + *
>>>>> + * Returns the allocated device if successfully attached PM domain, NULL when
>>>>> + * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
>>>>> + * in case of failures. Note that if a power-domain exists for the device, but
>>>>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
>>>>> + * that the device is not probed and to re-try again later.
>>>>> + */
>>>>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>>>>> +                                      unsigned int index)
>>>>> +{
>>>>> +     struct device *genpd_dev;
>>>>> +     int num_domains;
>>>>> +     int ret;
>>>>> +
>>>>> +     if (!dev->of_node)
>>>>> +             return NULL;
>>>>> +
>>>>> +     /* Deal only with devices using multiple PM domains. */
>>>>> +     num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>>>>> +                                              "#power-domain-cells");
>>>>> +     if (num_domains < 2 || index >= num_domains)
>>>>> +             return NULL;
>>>>> +
>>>>> +     /* Allocate and register device on the genpd bus. */
>>>>> +     genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>>>>> +     if (!genpd_dev)
>>>>> +             return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +     dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>>>>> +     genpd_dev->bus = &genpd_bus_type;
>>>>> +     genpd_dev->release = genpd_release_dev;
>>>>> +
>>>>> +     ret = device_register(genpd_dev);
>>>>> +     if (ret) {
>>>>> +             kfree(genpd_dev);
>>>>> +             return ERR_PTR(ret);
>>>>> +     }
>>>>> +
>>>>> +     /* Try to attach the device to the PM domain at the specified index. */
>>>>> +     ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>>>>> +     if (ret < 1) {
>>>>> +             device_unregister(genpd_dev);
>>>>> +             return ret ? ERR_PTR(ret) : NULL;
>>>>> +     }
>>>>> +
>>>>> +     pm_runtime_set_active(genpd_dev);
>>>>> +     pm_runtime_enable(genpd_dev);
>>>>> +
>>>>> +     return genpd_dev;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>>>
>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>> and so we definitely need a solution for Tegra.
>>>>
>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>> to the client device. So I assume that this means that the drivers for devices
>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>> additional power-domains. Is that correct?
>>>
>>> They can, but should not!
>>>
>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>> dynamically, depending on what PM domain that their original device
>>> needs for the current running use case.
>>>
>>> In that way, they keep existing runtime PM deployment, operating on
>>> its original device.
>>
>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>
> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
> then a default linking of all would help.

First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 07:12, Ulf Hansson wrote:

...

>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>> and so we definitely need a solution for Tegra.
>>>>>
>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>> additional power-domains. Is that correct?
>>>>
>>>> They can, but should not!
>>>>
>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>> dynamically, depending on what PM domain that their original device
>>>> needs for the current running use case.
>>>>
>>>> In that way, they keep existing runtime PM deployment, operating on
>>>> its original device.
>>>
>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>
>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>> then a default linking of all would help.
> 
> First, I think we need to decide on *where* the linking should be
> done, not at both places, as that would just mess up synchronization
> of who is responsible for calling the device_link_del() at detach.
> 
> Second, It would in principle be fine to call device_link_add() and
> device_link_del() as a part of the attach/detach APIs. However, there
> is a downside to such solution, which would be that the driver then
> needs call the detach API, just to do device_link_del(). Of course
> then it would also needs to call the attach API later if/when needed.
> Doing this adds unnecessary overhead - comparing to just let the
> driver call device_link_add|del() when needed. On the upside, yes, it
> would put less burden on the drivers as it then only needs to care
> about using one set of functions.
> 
> Which solution do you prefer?

Any reason why we could not add a 'boolean' argument to the API to 
indicate whether the new device should be linked? I think that I prefer 
the API handles it, but I can see there could be instances where drivers 
may wish to handle it themselves.

Rajendra, do you have a use-case right now where the driver would want 
to handle the linking?

Cheers
Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:37 PM, Jon Hunter wrote:
> 
> On 23/05/18 07:12, Ulf Hansson wrote:
> 
> ...
> 
>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>> and so we definitely need a solution for Tegra.
>>>>>>
>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>> additional power-domains. Is that correct?
>>>>>
>>>>> They can, but should not!
>>>>>
>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>> dynamically, depending on what PM domain that their original device
>>>>> needs for the current running use case.
>>>>>
>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>> its original device.
>>>>
>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>
>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
> 
> Any reason why we could not add a 'boolean' argument to the API to indicate whether the new device should be linked? I think that I prefer the API handles it, but I can see there could be instances where drivers may wish to handle it themselves.
> 
> Rajendra, do you have a use-case right now where the driver would want to handle the linking?

So if I understand this right, any driver which does want to control individual powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?

I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or both together if we
really need very high resolution using both vfe modules. 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>
>> On 23/05/18 07:12, Ulf Hansson wrote:
>>
>> ...
>>
>>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>
>>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>>> additional power-domains. Is that correct?
>>>>>>
>>>>>> They can, but should not!
>>>>>>
>>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>>> dynamically, depending on what PM domain that their original device
>>>>>> needs for the current running use case.
>>>>>>
>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>> its original device.
>>>>>
>>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>>
>>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>>> then a default linking of all would help.
>>>
>>> First, I think we need to decide on *where* the linking should be
>>> done, not at both places, as that would just mess up synchronization
>>> of who is responsible for calling the device_link_del() at detach.
>>>
>>> Second, It would in principle be fine to call device_link_add() and
>>> device_link_del() as a part of the attach/detach APIs. However, there
>>> is a downside to such solution, which would be that the driver then
>>> needs call the detach API, just to do device_link_del(). Of course
>>> then it would also needs to call the attach API later if/when needed.
>>> Doing this adds unnecessary overhead - comparing to just let the
>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>> would put less burden on the drivers as it then only needs to care
>>> about using one set of functions.
>>>
>>> Which solution do you prefer?
>>
>> Any reason why we could not add a 'boolean' argument to the API to indicate whether the new device should be linked? I think that I prefer the API handles it, but I can see there could be instances where drivers may wish to handle it themselves.
>>
>> Rajendra, do you have a use-case right now where the driver would want to handle the linking?
>
> So if I understand this right, any driver which does want to control individual powerdomain state would
> need to do the linking itself right?
>
> What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
> then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
> I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?

Correct!

>
> I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
> turned on one at a time (depending on what resolution needs to be supported) or both together if we
> really need very high resolution using both vfe modules.

I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:33, Ulf Hansson wrote:
> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>>
>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>
>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>
>>> ...
>>>
>>>>>>>> Thanks for sending this. Believe it or not this has still been on my to-do list
>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>
>>>>>>>> Looking at the above it appears that additional power-domains exposed as devices
>>>>>>>> to the client device. So I assume that this means that the drivers for devices
>>>>>>>> with multiple power-domains will need to call RPM APIs for each of these
>>>>>>>> additional power-domains. Is that correct?
>>>>>>>
>>>>>>> They can, but should not!
>>>>>>>
>>>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>> needs for the current running use case.
>>>>>>>
>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>> its original device.
>>>>>>
>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the above API? Is there a use-case where you would not want it linked?
>>>>>
>>>>> I am guessing the linking is what would give the driver the ability to decide which subset of powerdomains it actually wants to control
>>>>> at any point using runtime PM. If we have cases wherein the driver would want to turn on/off _all_ its associated powerdomains _always_
>>>>> then a default linking of all would help.
>>>>
>>>> First, I think we need to decide on *where* the linking should be
>>>> done, not at both places, as that would just mess up synchronization
>>>> of who is responsible for calling the device_link_del() at detach.
>>>>
>>>> Second, It would in principle be fine to call device_link_add() and
>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>> is a downside to such solution, which would be that the driver then
>>>> needs call the detach API, just to do device_link_del(). Of course
>>>> then it would also needs to call the attach API later if/when needed.
>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>> would put less burden on the drivers as it then only needs to care
>>>> about using one set of functions.
>>>>
>>>> Which solution do you prefer?
>>>
>>> Any reason why we could not add a 'boolean' argument to the API to indicate whether the new device should be linked? I think that I prefer the API handles it, but I can see there could be instances where drivers may wish to handle it themselves.
>>>
>>> Rajendra, do you have a use-case right now where the driver would want to handle the linking?
>>
>> So if I understand this right, any driver which does want to control individual powerdomain state would
>> need to do the linking itself right?
>>
>> What I am saying is, if I have device A, with powerdomains X and Y, and if I want to turn on only X,
>> then I would want only X to be linked with A, and at a later point if I want both X and Y to be turned on,
>> I would then go ahead and link both X and Y to A? Is that correct or did I get it all wrong?
> 
> Correct!
> 
>>
>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe powerdoamins, which can be
>> turned on one at a time (depending on what resolution needs to be supported) or both together if we
>> really need very high resolution using both vfe modules.
> 
> I think this is also the case for the Tegra XUSB subsystem.
> 
> The usb device is always attached to one PM domain, but depending on
> if super-speed mode is used, another PM domain for that logic needs to
> be powered on as well.
> 
> Jon, please correct me if I am wrong!

Yes this is technically correct, however, in reality I think we are 
always going to enable the superspeed domain if either the host or 
device domain is enabled. So we would probably always link the 
superspeed with the host and device devices.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:45, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 23/05/18 10:33, Ulf Hansson wrote:
>>
>> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>
>>>
>>>
>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>>
>>>>
>>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>>
>>>> ...
>>>>
>>>>>>>>> Thanks for sending this. Believe it or not this has still been on
>>>>>>>>> my to-do list
>>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>>
>>>>>>>>> Looking at the above it appears that additional power-domains
>>>>>>>>> exposed as devices
>>>>>>>>> to the client device. So I assume that this means that the drivers
>>>>>>>>> for devices
>>>>>>>>> with multiple power-domains will need to call RPM APIs for each of
>>>>>>>>> these
>>>>>>>>> additional power-domains. Is that correct?
>>>>>>>>
>>>>>>>>
>>>>>>>> They can, but should not!
>>>>>>>>
>>>>>>>> Instead, the driver shall use device_link_add() and
>>>>>>>> device_link_del(),
>>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>>> needs for the current running use case.
>>>>>>>>
>>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>>> its original device.
>>>>>>>
>>>>>>>
>>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>>>>>> above API? Is there a use-case where you would not want it linked?
>>>>>>
>>>>>>
>>>>>> I am guessing the linking is what would give the driver the ability to
>>>>>> decide which subset of powerdomains it actually wants to control
>>>>>> at any point using runtime PM. If we have cases wherein the driver
>>>>>> would want to turn on/off _all_ its associated powerdomains _always_
>>>>>> then a default linking of all would help.
>>>>>
>>>>>
>>>>> First, I think we need to decide on *where* the linking should be
>>>>> done, not at both places, as that would just mess up synchronization
>>>>> of who is responsible for calling the device_link_del() at detach.
>>>>>
>>>>> Second, It would in principle be fine to call device_link_add() and
>>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>>> is a downside to such solution, which would be that the driver then
>>>>> needs call the detach API, just to do device_link_del(). Of course
>>>>> then it would also needs to call the attach API later if/when needed.
>>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>>> would put less burden on the drivers as it then only needs to care
>>>>> about using one set of functions.
>>>>>
>>>>> Which solution do you prefer?
>>>>
>>>>
>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>> indicate whether the new device should be linked? I think that I prefer the
>>>> API handles it, but I can see there could be instances where drivers may
>>>> wish to handle it themselves.
>>>>
>>>> Rajendra, do you have a use-case right now where the driver would want
>>>> to handle the linking?
>>>
>>>
>>> So if I understand this right, any driver which does want to control
>>> individual powerdomain state would
>>> need to do the linking itself right?
>>>
>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>> if I want to turn on only X,
>>> then I would want only X to be linked with A, and at a later point if I
>>> want both X and Y to be turned on,
>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>> I get it all wrong?
>>
>>
>> Correct!
>>
>>>
>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>> powerdoamins, which can be
>>> turned on one at a time (depending on what resolution needs to be
>>> supported) or both together if we
>>> really need very high resolution using both vfe modules.
>>
>>
>> I think this is also the case for the Tegra XUSB subsystem.
>>
>> The usb device is always attached to one PM domain, but depending on
>> if super-speed mode is used, another PM domain for that logic needs to
>> be powered on as well.
>>
>> Jon, please correct me if I am wrong!
>
>
> Yes this is technically correct, however, in reality I think we are always
> going to enable the superspeed domain if either the host or device domain is
> enabled. So we would probably always link the superspeed with the host and
> device devices.

Why? Wouldn't that waste power if the superspeed mode isn't used?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:47, Ulf Hansson wrote:
> On 23 May 2018 at 11:45, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 23/05/18 10:33, Ulf Hansson wrote:
>>>
>>> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>>>
>>>>>
>>>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>>>> Thanks for sending this. Believe it or not this has still been on
>>>>>>>>>> my to-do list
>>>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>>>
>>>>>>>>>> Looking at the above it appears that additional power-domains
>>>>>>>>>> exposed as devices
>>>>>>>>>> to the client device. So I assume that this means that the drivers
>>>>>>>>>> for devices
>>>>>>>>>> with multiple power-domains will need to call RPM APIs for each of
>>>>>>>>>> these
>>>>>>>>>> additional power-domains. Is that correct?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> They can, but should not!
>>>>>>>>>
>>>>>>>>> Instead, the driver shall use device_link_add() and
>>>>>>>>> device_link_del(),
>>>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>>>> needs for the current running use case.
>>>>>>>>>
>>>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>>>> its original device.
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>>>>>>> above API? Is there a use-case where you would not want it linked?
>>>>>>>
>>>>>>>
>>>>>>> I am guessing the linking is what would give the driver the ability to
>>>>>>> decide which subset of powerdomains it actually wants to control
>>>>>>> at any point using runtime PM. If we have cases wherein the driver
>>>>>>> would want to turn on/off _all_ its associated powerdomains _always_
>>>>>>> then a default linking of all would help.
>>>>>>
>>>>>>
>>>>>> First, I think we need to decide on *where* the linking should be
>>>>>> done, not at both places, as that would just mess up synchronization
>>>>>> of who is responsible for calling the device_link_del() at detach.
>>>>>>
>>>>>> Second, It would in principle be fine to call device_link_add() and
>>>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>>>> is a downside to such solution, which would be that the driver then
>>>>>> needs call the detach API, just to do device_link_del(). Of course
>>>>>> then it would also needs to call the attach API later if/when needed.
>>>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>>>> would put less burden on the drivers as it then only needs to care
>>>>>> about using one set of functions.
>>>>>>
>>>>>> Which solution do you prefer?
>>>>>
>>>>>
>>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>>> indicate whether the new device should be linked? I think that I prefer the
>>>>> API handles it, but I can see there could be instances where drivers may
>>>>> wish to handle it themselves.
>>>>>
>>>>> Rajendra, do you have a use-case right now where the driver would want
>>>>> to handle the linking?
>>>>
>>>>
>>>> So if I understand this right, any driver which does want to control
>>>> individual powerdomain state would
>>>> need to do the linking itself right?
>>>>
>>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>>> if I want to turn on only X,
>>>> then I would want only X to be linked with A, and at a later point if I
>>>> want both X and Y to be turned on,
>>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>>> I get it all wrong?
>>>
>>>
>>> Correct!
>>>
>>>>
>>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>>> powerdoamins, which can be
>>>> turned on one at a time (depending on what resolution needs to be
>>>> supported) or both together if we
>>>> really need very high resolution using both vfe modules.
>>>
>>>
>>> I think this is also the case for the Tegra XUSB subsystem.
>>>
>>> The usb device is always attached to one PM domain, but depending on
>>> if super-speed mode is used, another PM domain for that logic needs to
>>> be powered on as well.
>>>
>>> Jon, please correct me if I am wrong!
>>
>>
>> Yes this is technically correct, however, in reality I think we are always
>> going to enable the superspeed domain if either the host or device domain is
>> enabled. So we would probably always link the superspeed with the host and
>> device devices.
> 
> Why? Wouldn't that waste power if the superspeed mode isn't used?

Simply to reduce complexity.

Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:07, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 23/05/18 07:12, Ulf Hansson wrote:
>
> ...
>
>
>>>>>> Thanks for sending this. Believe it or not this has still been on my
>>>>>> to-do list
>>>>>> and so we definitely need a solution for Tegra.
>>>>>>
>>>>>> Looking at the above it appears that additional power-domains exposed
>>>>>> as devices
>>>>>> to the client device. So I assume that this means that the drivers for
>>>>>> devices
>>>>>> with multiple power-domains will need to call RPM APIs for each of
>>>>>> these
>>>>>> additional power-domains. Is that correct?
>>>>>
>>>>>
>>>>> They can, but should not!
>>>>>
>>>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>>>> dynamically, depending on what PM domain that their original device
>>>>> needs for the current running use case.
>>>>>
>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>> its original device.
>>>>
>>>>
>>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>>> above API? Is there a use-case where you would not want it linked?
>>>
>>>
>>> I am guessing the linking is what would give the driver the ability to
>>> decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would
>>> want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
>
>
> Any reason why we could not add a 'boolean' argument to the API to indicate
> whether the new device should be linked? I think that I prefer the API
> handles it, but I can see there could be instances where drivers may wish to
> handle it themselves.

Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?

[...]

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 08:04, Ulf Hansson wrote:

...

>> Any reason why we could not add a 'boolean' argument to the API to indicate
>> whether the new device should be linked? I think that I prefer the API
>> handles it, but I can see there could be instances where drivers may wish to
>> handle it themselves.
> 
> Coming back to this question. Both Tegra XUSB and Qcom Camera use
> case, would benefit from doing the linking themselves, as it needs
> different PM domains to be powered on depending on the current use
> case - as to avoid wasting power.
> 
> However, I can understand that you prefer some simplicity over
> optimizations, as you told us. Then, does it mean that you are
> insisting on extending the APIs with a boolean for linking, or are you
> fine with the driver to call device_link_add()?

I am fine with the driver calling device_link_add(), but I just wonder 
if we will find a several drivers doing this and then we will end up 
doing this later anyway.

The current API is called ...

* genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
* @dev: Device to attach.
* @index: The index of the PM domain.

This naming and description is a bit misleading, because really it is 
not attaching the device that is passed, but creating a new device to 
attach a PM domain to. So we should consider renaming and changing the 
description and indicate that users need to link the device.

Finally, how is a PM domain attached via calling 
genpd_dev_pm_attach_by_id() detached?

Cheers
Jon

-- 
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 24/05/18 08:04, Ulf Hansson wrote:
>
> ...
>
>>> Any reason why we could not add a 'boolean' argument to the API to
>>> indicate
>>> whether the new device should be linked? I think that I prefer the API
>>> handles it, but I can see there could be instances where drivers may wish
>>> to
>>> handle it themselves.
>>
>>
>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>> case, would benefit from doing the linking themselves, as it needs
>> different PM domains to be powered on depending on the current use
>> case - as to avoid wasting power.
>>
>> However, I can understand that you prefer some simplicity over
>> optimizations, as you told us. Then, does it mean that you are
>> insisting on extending the APIs with a boolean for linking, or are you
>> fine with the driver to call device_link_add()?
>
>
> I am fine with the driver calling device_link_add(), but I just wonder if we
> will find a several drivers doing this and then we will end up doing this
> later anyway.

Okay.

>
> The current API is called ...
>
> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> * @dev: Device to attach.
> * @index: The index of the PM domain.
>
> This naming and description is a bit misleading, because really it is not
> attaching the device that is passed, but creating a new device to attach a
> PM domain to. So we should consider renaming and changing the description
> and indicate that users need to link the device.

I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?

I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.

However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.

In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.

>
> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
> detached?

Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 13:17, Ulf Hansson wrote:
> On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 24/05/18 08:04, Ulf Hansson wrote:
>>
>> ...
>>
>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>> indicate
>>>> whether the new device should be linked? I think that I prefer the API
>>>> handles it, but I can see there could be instances where drivers may wish
>>>> to
>>>> handle it themselves.
>>>
>>>
>>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>>> case, would benefit from doing the linking themselves, as it needs
>>> different PM domains to be powered on depending on the current use
>>> case - as to avoid wasting power.
>>>
>>> However, I can understand that you prefer some simplicity over
>>> optimizations, as you told us. Then, does it mean that you are
>>> insisting on extending the APIs with a boolean for linking, or are you
>>> fine with the driver to call device_link_add()?
>>
>>
>> I am fine with the driver calling device_link_add(), but I just wonder if we
>> will find a several drivers doing this and then we will end up doing this
>> later anyway.
> 
> Okay.
> 
>>
>> The current API is called ...
>>
>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> * @dev: Device to attach.
>> * @index: The index of the PM domain.
>>
>> This naming and description is a bit misleading, because really it is not
>> attaching the device that is passed, but creating a new device to attach a
>> PM domain to. So we should consider renaming and changing the description
>> and indicate that users need to link the device.
> 
> I picked the name to be consistent with the existing
> genpd_dev_pm_attach(). Do you have a better suggestion?

Well, it appears to get more of a 'get' function and so I don't see why 
we could not have 'genpd_dev_get_by_id()' and then we could have a 
genpd_dev_put() as well (which would call genpd_dev_pm_detach).

> I agree, some details is missing to the description, let me try to
> improve it. Actually, I was trying to follow existing descriptions
> from genpd_dev_pm_attach(), so perhaps that also needs a little
> update.
> 
> However, do note that, neither genpd_dev_pm_attach() or
> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
> rather only by the driver core. So description may not be so
> important.
> 
> In regards to good descriptions, for sure the API added in patch9,
> dev_pm_domain_attach_by_id(), needs a good one, as this is what
> drivers should be using.

OK. Same appears to apply here to the description as I mentioned above. 
Still seems to be more of a 'get' than an attach. So I wonder if it 
should be dev_pm_domain_get_by_id() instead?

>> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
>> detached?
> 
> Via the existing genpd_dev_pm_detach(), according to what I have
> described in the change log. I clarify the description in regards to
> this as well.

OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that 
you said 'although we need to extend it to cover cleanup of the earlier 
registered device, via calling device_unregister().' So if we do this 
then that would be fine.

Cheers!
Jon

-- 
nvpublic

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Jon Hunter]

On 18/05/18 11:31, Ulf Hansson wrote:
> The existing dev_pm_domain_attach() function, allows a single PM domain to
> be attached per device. To be able to support devices that are partitioned
> across multiple PM domains, let's introduce a new interface,
> dev_pm_domain_attach_by_id().
> 
> The dev_pm_domain_attach_by_id() returns a new allocated struct device with
> the corresponding attached PM domain. This enables for example a driver to
> operate on the new device from a power management point of view. The driver
> may then also benefit from using the received device, to set up so called
> device-links towards its original device. Depending on the situation, these
> links may then be dynamically changed.
> 
> The new interface is typically called by drivers during their probe phase,
> in case they manages devices which uses multiple PM domains. If that is the
> case, the driver also becomes responsible of managing the detaching of the
> PM domains, which typically should be done at the remove phase. Detaching
> is done by calling the existing dev_pm_domain_detach() function and for
> each of the received devices from dev_pm_domain_attach_by_id().
> 
> Note, currently its only genpd that supports multiple PM domains per
> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
> other PM domain types, if/when needed.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>   include/linux/pm_domain.h   |  7 +++++++
>   2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 7ae62b6..d3db974 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>   EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>   
>   /**
> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.

Isn't this more of a 'get'?

> + * @index: The index of the PM domain.
> + * @dev: Device to attach.

Isn't this just the device associated with the PM domain we are getting?

> + *
> + * As @dev may only be attached to a single PM domain, the backend PM domain
> + * provider should create a virtual device to attach instead. As attachment
> + * succeeds, the ->detach() callback in the struct dev_pm_domain should be
> + * assigned by the corresponding backend attach function.
> + *
> + * This function should typically be invoked from drivers during probe phase.
> + * Especially for those that manages devices which requires power management
> + * through more than one PM domain.
> + *
> + * Callers must ensure proper synchronization of this function with power
> + * management callbacks.
> + *
> + * Returns the virtual attached device in case successfully attached PM domain,
> + * NULL in case @dev don't need a PM domain, else a PTR_ERR().

Should this be 'NULL in the case where the @dev already has a power-domain'?

> + */
> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
> +					  unsigned int index)
> +{
> +	if (dev->pm_domain)

I wonder if this is worthy of a ...

	if (WARN_ON(dev->pm_domain))

> +		return NULL;

Don't we consider this an error case? I wonder why not return PTR_ERR 
here as well? This would be consistent with dev_pm_domain_attach().

Cheers
Jon

-- 
nvpublic

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Ulf Hansson]

On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 18/05/18 11:31, Ulf Hansson wrote:
>>
>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>> be attached per device. To be able to support devices that are partitioned
>> across multiple PM domains, let's introduce a new interface,
>> dev_pm_domain_attach_by_id().
>>
>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>> with
>> the corresponding attached PM domain. This enables for example a driver to
>> operate on the new device from a power management point of view. The
>> driver
>> may then also benefit from using the received device, to set up so called
>> device-links towards its original device. Depending on the situation,
>> these
>> links may then be dynamically changed.
>>
>> The new interface is typically called by drivers during their probe phase,
>> in case they manages devices which uses multiple PM domains. If that is
>> the
>> case, the driver also becomes responsible of managing the detaching of the
>> PM domains, which typically should be done at the remove phase. Detaching
>> is done by calling the existing dev_pm_domain_detach() function and for
>> each of the received devices from dev_pm_domain_attach_by_id().
>>
>> Note, currently its only genpd that supports multiple PM domains per
>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>> other PM domain types, if/when needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>   drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>>   include/linux/pm_domain.h   |  7 +++++++
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 7ae62b6..d3db974 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>> power_on)
>>   EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>     /**
>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>
>
> Isn't this more of a 'get'?

I don't think so, at least according to the common understanding of
how we use get and put APIs.

For example, clk_get() returns a cookie to a clk, which you then can
do a hole bunch of different clk specific operations on.

This is different, there are no specific PM domain operations the
caller can or should do. Instead the idea is to keep drivers more or
less transparent, still using runtime PM as before. The only care the
caller need to take is to use device links, which BTW isn't a PM
domain specific thing.

>
>> + * @index: The index of the PM domain.
>> + * @dev: Device to attach.
>
>
> Isn't this just the device associated with the PM domain we are getting?

Correct, but please note, as stated above, I don't think it's a good
idea to return a special PM domain cookie, because we don't want the
caller to run special PM domain operations.

>
>> + *
>> + * As @dev may only be attached to a single PM domain, the backend PM
>> domain
>> + * provider should create a virtual device to attach instead. As
>> attachment
>> + * succeeds, the ->detach() callback in the struct dev_pm_domain should
>> be
>> + * assigned by the corresponding backend attach function.
>> + *
>> + * This function should typically be invoked from drivers during probe
>> phase.
>> + * Especially for those that manages devices which requires power
>> management
>> + * through more than one PM domain.
>> + *
>> + * Callers must ensure proper synchronization of this function with power
>> + * management callbacks.
>> + *
>> + * Returns the virtual attached device in case successfully attached PM
>> domain,
>> + * NULL in case @dev don't need a PM domain, else a PTR_ERR().
>
>
> Should this be 'NULL in the case where the @dev already has a power-domain'?

Yes, I think so. The intent is to be consistent with how
dev_pm_domain_attach() works and according to the latest changes I did
for it. It's queued for 4.18, please have a look in Rafael's tree and
you will see.

>
>> + */
>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>> +                                         unsigned int index)
>> +{
>> +       if (dev->pm_domain)
>
>
> I wonder if this is worthy of a ...
>
>         if (WARN_ON(dev->pm_domain))
>
>> +               return NULL;
>
>
> Don't we consider this an error case? I wonder why not return PTR_ERR here
> as well? This would be consistent with dev_pm_domain_attach().

Please see above comment.

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>>
>>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> * @dev: Device to attach.
>>> * @index: The index of the PM domain.
>>>
>>> This naming and description is a bit misleading, because really it is not
>>> attaching the device that is passed, but creating a new device to attach
>>> a
>>> PM domain to. So we should consider renaming and changing the description
>>> and indicate that users need to link the device.
>>
>>
>> I picked the name to be consistent with the existing
>> genpd_dev_pm_attach(). Do you have a better suggestion?
>
>
> Well, it appears to get more of a 'get' function and so I don't see why we
> could not have 'genpd_dev_get_by_id()' and then we could have a
> genpd_dev_put() as well (which would call genpd_dev_pm_detach).
>
>> I agree, some details is missing to the description, let me try to
>> improve it. Actually, I was trying to follow existing descriptions
>> from genpd_dev_pm_attach(), so perhaps that also needs a little
>> update.
>>
>> However, do note that, neither genpd_dev_pm_attach() or
>> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
>> rather only by the driver core. So description may not be so
>> important.
>>
>> In regards to good descriptions, for sure the API added in patch9,
>> dev_pm_domain_attach_by_id(), needs a good one, as this is what
>> drivers should be using.
>
>
> OK. Same appears to apply here to the description as I mentioned above.
> Still seems to be more of a 'get' than an attach. So I wonder if it should
> be dev_pm_domain_get_by_id() instead?

Regarding "get" vs "attach", I suggest we continue to discuss that in
patch 9. Whatever is decided, $subject patch needs to follow.

>
>>> Finally, how is a PM domain attached via calling
>>> genpd_dev_pm_attach_by_id()
>>> detached?
>>
>>
>> Via the existing genpd_dev_pm_detach(), according to what I have
>> described in the change log. I clarify the description in regards to
>> this as well.
>
>
> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
> said 'although we need to extend it to cover cleanup of the earlier
> registered device, via calling device_unregister().' So if we do this then
> that would be fine.

Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.

So I guess we are in agreement that we don't need another API to deal
with detach?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 22:21, Ulf Hansson wrote:

...

>> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
>> said 'although we need to extend it to cover cleanup of the earlier
>> registered device, via calling device_unregister().' So if we do this then
>> that would be fine.
> 
> Let me clarify the changelog. It's not a to-do, as it's already done
> as part of $subject patch.

Yes I see it now. OK, then that's fine.

Jon

-- 
nvpublic

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Jon Hunter]

On 24/05/18 22:11, Ulf Hansson wrote:
> On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 18/05/18 11:31, Ulf Hansson wrote:
>>>
>>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>>> be attached per device. To be able to support devices that are partitioned
>>> across multiple PM domains, let's introduce a new interface,
>>> dev_pm_domain_attach_by_id().
>>>
>>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>>> with
>>> the corresponding attached PM domain. This enables for example a driver to
>>> operate on the new device from a power management point of view. The
>>> driver
>>> may then also benefit from using the received device, to set up so called
>>> device-links towards its original device. Depending on the situation,
>>> these
>>> links may then be dynamically changed.
>>>
>>> The new interface is typically called by drivers during their probe phase,
>>> in case they manages devices which uses multiple PM domains. If that is
>>> the
>>> case, the driver also becomes responsible of managing the detaching of the
>>> PM domains, which typically should be done at the remove phase. Detaching
>>> is done by calling the existing dev_pm_domain_detach() function and for
>>> each of the received devices from dev_pm_domain_attach_by_id().
>>>
>>> Note, currently its only genpd that supports multiple PM domains per
>>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>>> other PM domain types, if/when needed.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>    drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>>>    include/linux/pm_domain.h   |  7 +++++++
>>>    2 files changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>>> index 7ae62b6..d3db974 100644
>>> --- a/drivers/base/power/common.c
>>> +++ b/drivers/base/power/common.c
>>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>>> power_on)
>>>    EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>>      /**
>>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>>
>>
>> Isn't this more of a 'get'?
> 
> I don't think so, at least according to the common understanding of
> how we use get and put APIs.
> 
> For example, clk_get() returns a cookie to a clk, which you then can
> do a hole bunch of different clk specific operations on.
> 
> This is different, there are no specific PM domain operations the
> caller can or should do. Instead the idea is to keep drivers more or
> less transparent, still using runtime PM as before. The only care the
> caller need to take is to use device links, which BTW isn't a PM
> domain specific thing.

Yes, but a user can still call pm_runtime_get/put with the device 
returned if they wish to, right?

>>
>>> + * @index: The index of the PM domain.
>>> + * @dev: Device to attach.
>>
>>
>> Isn't this just the device associated with the PM domain we are getting?
> 
> Correct, but please note, as stated above, I don't think it's a good
> idea to return a special PM domain cookie, because we don't want the
> caller to run special PM domain operations.
> 
>>
>>> + *
>>> + * As @dev may only be attached to a single PM domain, the backend PM
>>> domain
>>> + * provider should create a virtual device to attach instead. As
>>> attachment
>>> + * succeeds, the ->detach() callback in the struct dev_pm_domain should
>>> be
>>> + * assigned by the corresponding backend attach function.
>>> + *
>>> + * This function should typically be invoked from drivers during probe
>>> phase.
>>> + * Especially for those that manages devices which requires power
>>> management
>>> + * through more than one PM domain.
>>> + *
>>> + * Callers must ensure proper synchronization of this function with power
>>> + * management callbacks.
>>> + *
>>> + * Returns the virtual attached device in case successfully attached PM
>>> domain,
>>> + * NULL in case @dev don't need a PM domain, else a PTR_ERR().
>>
>>
>> Should this be 'NULL in the case where the @dev already has a power-domain'?
> 
> Yes, I think so. The intent is to be consistent with how
> dev_pm_domain_attach() works and according to the latest changes I did
> for it. It's queued for 4.18, please have a look in Rafael's tree and
> you will see.

Ah, I see your change now.

>>
>>> + */
>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>> +                                         unsigned int index)
>>> +{
>>> +       if (dev->pm_domain)
>>
>>
>> I wonder if this is worthy of a ...
>>
>>          if (WARN_ON(dev->pm_domain))
>>
>>> +               return NULL;
>>
>>
>> Don't we consider this an error case? I wonder why not return PTR_ERR here
>> as well? This would be consistent with dev_pm_domain_attach().
> 
> Please see above comment.

Right, but this case still seems like an error. My understanding is that 
only drivers will use this API directly and it will not be used by the 
device driver core (unlike dev_pm_domain_attach), so if anyone calls 
this attempting to attach another PM domain when one is already 
attached, they are doing something wrong.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Ulf Hansson]

On 25 May 2018 at 10:31, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 24/05/18 22:11, Ulf Hansson wrote:
>>
>> On 24 May 2018 at 17:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>>
>>> On 18/05/18 11:31, Ulf Hansson wrote:
>>>>
>>>>
>>>> The existing dev_pm_domain_attach() function, allows a single PM domain
>>>> to
>>>> be attached per device. To be able to support devices that are
>>>> partitioned
>>>> across multiple PM domains, let's introduce a new interface,
>>>> dev_pm_domain_attach_by_id().
>>>>
>>>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>>>> with
>>>> the corresponding attached PM domain. This enables for example a driver
>>>> to
>>>> operate on the new device from a power management point of view. The
>>>> driver
>>>> may then also benefit from using the received device, to set up so
>>>> called
>>>> device-links towards its original device. Depending on the situation,
>>>> these
>>>> links may then be dynamically changed.
>>>>
>>>> The new interface is typically called by drivers during their probe
>>>> phase,
>>>> in case they manages devices which uses multiple PM domains. If that is
>>>> the
>>>> case, the driver also becomes responsible of managing the detaching of
>>>> the
>>>> PM domains, which typically should be done at the remove phase.
>>>> Detaching
>>>> is done by calling the existing dev_pm_domain_detach() function and for
>>>> each of the received devices from dev_pm_domain_attach_by_id().
>>>>
>>>> Note, currently its only genpd that supports multiple PM domains per
>>>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>>>> other PM domain types, if/when needed.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>    drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>>>>    include/linux/pm_domain.h   |  7 +++++++
>>>>    2 files changed, 39 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>>>> index 7ae62b6..d3db974 100644
>>>> --- a/drivers/base/power/common.c
>>>> +++ b/drivers/base/power/common.c
>>>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>>>> power_on)
>>>>    EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>>>      /**
>>>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM
>>>> domains.
>>>
>>>
>>>
>>> Isn't this more of a 'get'?
>>
>>
>> I don't think so, at least according to the common understanding of
>> how we use get and put APIs.
>>
>> For example, clk_get() returns a cookie to a clk, which you then can
>> do a hole bunch of different clk specific operations on.
>>
>> This is different, there are no specific PM domain operations the
>> caller can or should do. Instead the idea is to keep drivers more or
>> less transparent, still using runtime PM as before. The only care the
>> caller need to take is to use device links, which BTW isn't a PM
>> domain specific thing.
>
>
> Yes, but a user can still call pm_runtime_get/put with the device returned
> if they wish to, right?

Correct.

[...]

>>>> + */
>>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>>> +                                         unsigned int index)
>>>> +{
>>>> +       if (dev->pm_domain)
>>>
>>>
>>>
>>> I wonder if this is worthy of a ...
>>>
>>>          if (WARN_ON(dev->pm_domain))
>>>
>>>> +               return NULL;
>>>
>>>
>>>
>>> Don't we consider this an error case? I wonder why not return PTR_ERR
>>> here
>>> as well? This would be consistent with dev_pm_domain_attach().
>>
>>
>> Please see above comment.
>
>
> Right, but this case still seems like an error. My understanding is that
> only drivers will use this API directly and it will not be used by the
> device driver core (unlike dev_pm_domain_attach), so if anyone calls this
> attempting to attach another PM domain when one is already attached, they
> are doing something wrong.

[...]

You may be right!

What I was thinking of is whether multiple PM domains may be optional
in some cases, but instead a PM domain have already been attached by
dev_pm_domain_attach(), prior the driver starts to probe.

Then, assuming we return an error for this case, that means the caller
then need to check the dev->pm_domain pointer, prior calling
dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear
though?

Kind regards
Uffe

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Jon Hunter]

On 25/05/18 11:45, Ulf Hansson wrote:

...

>> Right, but this case still seems like an error. My understanding is that
>> only drivers will use this API directly and it will not be used by the
>> device driver core (unlike dev_pm_domain_attach), so if anyone calls this
>> attempting to attach another PM domain when one is already attached, they
>> are doing something wrong.
> 
> [...]
> 
> You may be right!
> 
> What I was thinking of is whether multiple PM domains may be optional
> in some cases, but instead a PM domain have already been attached by
> dev_pm_domain_attach(), prior the driver starts to probe.
> 
> Then, assuming we return an error for this case, that means the caller
> then need to check the dev->pm_domain pointer, prior calling
> dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear
> though?

IMO the driver should know whether is needs multiple power-domains or 
not and if it needs multiple then it should just call 
dev_pm_domain_attach_by_id() N times without needing to checking 
dev->pm_domain first. If it fails then either the PM domain core did 
something wrong or power-domains are missing from DT, but either way 
there is an error, so let it fail.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains

[Ulf Hansson]

On 25 May 2018 at 13:07, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 25/05/18 11:45, Ulf Hansson wrote:
>
> ...
>
>>> Right, but this case still seems like an error. My understanding is that
>>> only drivers will use this API directly and it will not be used by the
>>> device driver core (unlike dev_pm_domain_attach), so if anyone calls this
>>> attempting to attach another PM domain when one is already attached, they
>>> are doing something wrong.
>>
>>
>> [...]
>>
>> You may be right!
>>
>> What I was thinking of is whether multiple PM domains may be optional
>> in some cases, but instead a PM domain have already been attached by
>> dev_pm_domain_attach(), prior the driver starts to probe.
>>
>> Then, assuming we return an error for this case, that means the caller
>> then need to check the dev->pm_domain pointer, prior calling
>> dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear
>> though?
>
>
> IMO the driver should know whether is needs multiple power-domains or not
> and if it needs multiple then it should just call
> dev_pm_domain_attach_by_id() N times without needing to checking
> dev->pm_domain first. If it fails then either the PM domain core did
> something wrong or power-domains are missing from DT, but either way there
> is an error, so let it fail.

Right, sounds reasonable!

Kind regards
Uffe