[PATCH 0/4] Add RSC power domain support

[PATCH 0/4] Add RSC power domain support

[Maulik Shah]

Resource State Coordinator (RSC) is responsible for powering off/lowering
the requirements from CPU subsystem for the associated hardware like buses,
clocks, and regulators when all CPUs and cluster is powered down.

RSC power domain uses last-man activities provided by genpd framework based on
Ulf Hansoon's patch series[1], when the cluster of CPUs enter deepest idle
states. As a part of domain poweroff, RSC can lower resource state requirements
by flushing the cached sleep and wake state votes for resources.

Dependencies:

[1] https://lkml.org/lkml/2019/5/13/839

Maulik Shah (4):
  drivers: qcom: rpmh: fix macro to accept NULL argument
  drivers: qcom: rpmh: remove rpmh_flush export
  dt-bindings: soc: qcom: Add RSC power domain specifier
  drivers: qcom: rpmh-rsc: Add RSC power domain support

 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt |  7 ++
 drivers/soc/qcom/rpmh-internal.h              |  3 +
 drivers/soc/qcom/rpmh-rsc.c                   | 96 +++++++++++++++++++
 drivers/soc/qcom/rpmh.c                       | 22 ++---
 include/soc/qcom/rpmh.h                       |  5 -
 5 files changed, 116 insertions(+), 17 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH 1/4] drivers: qcom: rpmh: fix macro to accept NULL argument

[Maulik Shah]

Device argument matches with dev variable declared in RPMH message.
Compiler reports error when the argument is NULL since the argument
matches the name of the property. Rename dev argument to device to
fix this.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 035091fd44b8..3a4579d056a4 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -23,7 +23,7 @@
 
 #define RPMH_TIMEOUT_MS			msecs_to_jiffies(10000)
 
-#define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name)	\
+#define DEFINE_RPMH_MSG_ONSTACK(device, s, q, name)	\
 	struct rpmh_request name = {			\
 		.msg = {				\
 			.state = s,			\
@@ -33,7 +33,7 @@
 		},					\
 		.cmd = { { 0 } },			\
 		.completion = q,			\
-		.dev = dev,				\
+		.dev = device,				\
 		.needs_free = false,				\
 	}
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/4] drivers: qcom: rpmh: fix macro to accept NULL argument

[Stephen Boyd]

Quoting Maulik Shah (2019-08-13 01:24:39)
> Device argument matches with dev variable declared in RPMH message.
> Compiler reports error when the argument is NULL since the argument
> matches the name of the property. Rename dev argument to device to
> fix this.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Maybe this should have a Fixes tag? And the subject could be something
like "avoid shadowing local variables in macro"?

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH 2/4] drivers: qcom: rpmh: remove rpmh_flush export

[Maulik Shah]

rpmh_flush() was exported with the idea that an external entity
operation during CPU idle would know when to flush the sleep and wake
TCS. Since, this is not the case when defining a power domain for the
RSC. Remove the function export and instead allow the function to be
called internally.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh-internal.h |  1 +
 drivers/soc/qcom/rpmh.c          | 18 ++++++++----------
 include/soc/qcom/rpmh.h          |  5 -----
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a7bbbb67991c..6eec32b97f83 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -110,5 +110,6 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
 int rpmh_rsc_invalidate(struct rsc_drv *drv);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);
+int rpmh_flush(struct rpmh_ctrlr *ctrlr);
 
 #endif /* __RPM_INTERNAL_H__ */
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 3a4579d056a4..eb0ded059d2e 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -427,11 +427,10 @@ static int is_req_valid(struct cache_req *req)
 		req->sleep_val != req->wake_val);
 }
 
-static int send_single(const struct device *dev, enum rpmh_state state,
+static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
 		       u32 addr, u32 data)
 {
-	DEFINE_RPMH_MSG_ONSTACK(dev, state, NULL, rpm_msg);
-	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	DEFINE_RPMH_MSG_ONSTACK(NULL, state, NULL, rpm_msg);
 
 	/* Wake sets are always complete and sleep sets are not */
 	rpm_msg.msg.wait_for_compl = (state == RPMH_WAKE_ONLY_STATE);
@@ -445,7 +444,7 @@ static int send_single(const struct device *dev, enum rpmh_state state,
 /**
  * rpmh_flush: Flushes the buffered active and sleep sets to TCS
  *
- * @dev: The device making the request
+ * @ctrlr: controller making request to flush cached data
  *
  * Return: -EBUSY if the controller is busy, probably waiting on a response
  * to a RPMH request sent earlier.
@@ -454,10 +453,9 @@ static int send_single(const struct device *dev, enum rpmh_state state,
  * that is powering down the entire system. Since no other RPMH API would be
  * executing at this time, it is safe to run lockless.
  */
-int rpmh_flush(const struct device *dev)
+int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 {
 	struct cache_req *p;
-	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
 	int ret;
 
 	if (!ctrlr->dirty) {
@@ -480,11 +478,12 @@ int rpmh_flush(const struct device *dev)
 				 __func__, p->addr, p->sleep_val, p->wake_val);
 			continue;
 		}
-		ret = send_single(dev, RPMH_SLEEP_STATE, p->addr, p->sleep_val);
+		ret = send_single(ctrlr, RPMH_SLEEP_STATE, p->addr,
+				  p->sleep_val);
 		if (ret)
 			return ret;
-		ret = send_single(dev, RPMH_WAKE_ONLY_STATE,
-				  p->addr, p->wake_val);
+		ret = send_single(ctrlr, RPMH_WAKE_ONLY_STATE, p->addr,
+				  p->wake_val);
 		if (ret)
 			return ret;
 	}
@@ -493,7 +492,6 @@ int rpmh_flush(const struct device *dev)
 
 	return 0;
 }
-EXPORT_SYMBOL(rpmh_flush);
 
 /**
  * rpmh_invalidate: Invalidate all sleep and active sets
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index 619e07c75da9..f9ec353d24a5 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -20,8 +20,6 @@ int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 		     const struct tcs_cmd *cmd, u32 *n);
 
-int rpmh_flush(const struct device *dev);
-
 int rpmh_invalidate(const struct device *dev);
 
 #else
@@ -40,9 +38,6 @@ static inline int rpmh_write_batch(const struct device *dev,
 				   const struct tcs_cmd *cmd, u32 *n)
 { return -ENODEV; }
 
-static inline int rpmh_flush(const struct device *dev)
-{ return -ENODEV; }
-
 static inline int rpmh_invalidate(const struct device *dev)
 { return -ENODEV; }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 2/4] drivers: qcom: rpmh: remove rpmh_flush export

[Stephen Boyd]

Quoting Maulik Shah (2019-08-13 01:24:40)
> rpmh_flush() was exported with the idea that an external entity
> operation during CPU idle would know when to flush the sleep and wake
> TCS. Since, this is not the case when defining a power domain for the
> RSC. Remove the function export and instead allow the function to be
> called internally.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH 3/4] dt-bindings: soc: qcom: Add RSC power domain specifier

[Maulik Shah]

In addition to transmitting resource state requests to the remote
processor, the RSC is responsible for powering off/lowering the
requirements from CPUs subsystem for the associated hardware like
buses, clocks, and regulators when all CPUs and cluster is powered down.

The power domain is configured to a low power state and when all the
CPUs are powered down, the RSC can lower resource state requirements
and power down the rails that power the CPUs.

Add PM domain specifier property for RSC controller.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
index 9b86d1eff219..d0ab6e9b6745 100644
--- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
@@ -83,6 +83,13 @@ Properties:
 	Value type: <string>
 	Definition: Name for the RSC. The name would be used in trace logs.
 
+- #power-domain-cells:
+	Usage: optional
+	Value type: <u32>
+	Definition: Number of cells in power domain specifier. Optional for
+		    controllers that may be in 'solver' state where they can
+		    be in autonomous mode executing low power modes.
+
 Drivers that want to use the RSC to communicate with RPMH must specify their
 bindings as child nodes of the RSC controllers they wish to communicate with.
 
@@ -112,6 +119,7 @@ TCS-OFFSET: 0xD00
 				  <SLEEP_TCS   3>,
 				  <WAKE_TCS    3>,
 				  <CONTROL_TCS 1>;
+		#power-domain-cells = <0>;
 	};
 
 Example 2:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 3/4] dt-bindings: soc: qcom: Add RSC power domain specifier

[Stephen Boyd]

Quoting Maulik Shah (2019-08-13 01:24:41)
> In addition to transmitting resource state requests to the remote
> processor, the RSC is responsible for powering off/lowering the
> requirements from CPUs subsystem for the associated hardware like
> buses, clocks, and regulators when all CPUs and cluster is powered down.
> 
> The power domain is configured to a low power state and when all the
> CPUs are powered down, the RSC can lower resource state requirements
> and power down the rails that power the CPUs.
> 
> Add PM domain specifier property for RSC controller.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH 4/4] drivers: qcom: rpmh-rsc: Add RSC power domain support

[Maulik Shah]

Add RSC power domain support. RSC is top level power domain in
hireachical CPU LPM modes. Once the rsc domain is down flush all
cached sleep and wake votes from controller.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh-internal.h |  2 +
 drivers/soc/qcom/rpmh-rsc.c      | 96 ++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 6eec32b97f83..3736c148effc 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -8,6 +8,7 @@
 #define __RPM_INTERNAL_H__
 
 #include <linux/bitmap.h>
+#include <linux/pm_domain.h>
 #include <soc/qcom/tcs.h>
 
 #define TCS_TYPE_NR			4
@@ -102,6 +103,7 @@ struct rsc_drv {
 	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
 	spinlock_t lock;
 	struct rpmh_ctrlr client;
+	struct generic_pm_domain rsc_pd;
 };
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..bd8e9f1a43b4 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	return ret;
 }
 
+/**
+ *  rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy.
+ *
+ *  @drv: The controller
+ *
+ *  Returns false if the TCSes are engaged in handling requests,
+ *  True if controller is idle.
+ */
+static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv)
+{
+	int m;
+	struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
+	bool ret = true;
+
+	spin_lock(&drv->lock);
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			ret = false;
+			break;
+		}
+	}
+	spin_unlock(&drv->lock);
+
+	return ret;
+}
+
 /**
  * rpmh_rsc_write_ctrl_data: Write request to the controller
  *
@@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return tcs_ctrl_write(drv, msg);
 }
 
+int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
+{
+	struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
+	int ret = 0;
+
+	/*
+	 * RPMh domain can not be powered off when there is pending ACK for
+	 * ACTIVE_TCS request. Exit when controller is busy.
+	 */
+
+	ret = rpmh_rsc_ctrlr_is_idle(drv);
+	if (!ret)
+		goto exit;
+
+	ret = rpmh_flush(&drv->client);
+	if (ret)
+		goto exit;
+
+exit:
+	return ret;
+}
+
+static int rpmh_probe_power_domain(struct platform_device *pdev,
+				   struct rsc_drv *drv)
+{
+	int ret = -ENOMEM;
+	struct generic_pm_domain *rsc_pd = &drv->rsc_pd;
+	struct device_node *dn = pdev->dev.of_node;
+
+	rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name);
+	if (!rsc_pd->name)
+		goto exit;
+
+	rsc_pd->name = kbasename(rsc_pd->name);
+	rsc_pd->power_off = rpmh_domain_power_off;
+	rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE;
+
+	ret = pm_genpd_init(rsc_pd, NULL, false);
+	if (ret)
+		goto free_name;
+
+	ret = of_genpd_add_provider_simple(dn, rsc_pd);
+	if (ret)
+		goto remove_pd;
+
+	pr_debug("init PM domain %s\n", rsc_pd->name);
+
+	return ret;
+
+remove_pd:
+	pm_genpd_remove(rsc_pd);
+
+free_name:
+	kfree(rsc_pd->name);
+
+exit:
+	return ret;
+}
+
 static int rpmh_probe_tcs_config(struct platform_device *pdev,
 				 struct rsc_drv *drv)
 {
@@ -650,6 +735,17 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Power domain is not required for controllers that support 'solver'
+	 * mode where they can be in autonomous mode executing low power mode
+	 * to power down.
+	 */
+	if (of_property_read_bool(dn, "#power-domain-cells")) {
+		ret = rpmh_probe_power_domain(pdev, drv);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_init(&drv->lock);
 	bitmap_zero(drv->tcs_in_use, MAX_TCS_NR);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 4/4] drivers: qcom: rpmh-rsc: Add RSC power domain support

[Stephen Boyd]

Quoting Maulik Shah (2019-08-13 01:24:42)
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..bd8e9f1a43b4 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>         return ret;
>  }
>  
> +/**
> + *  rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy.
> + *
> + *  @drv: The controller
> + *
> + *  Returns false if the TCSes are engaged in handling requests,
> + *  True if controller is idle.
> + */
> +static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv)
> +{
> +       int m;
> +       struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
> +       bool ret = true;
> +
> +       spin_lock(&drv->lock);
> +       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> +               if (!tcs_is_free(drv, m)) {

Isn't this a copy of an existing function in the rpmh driver?

> +                       ret = false;
> +                       break;
> +               }
> +       }
> +       spin_unlock(&drv->lock);
> +
> +       return ret;
> +}
> +
>  /**
>   * rpmh_rsc_write_ctrl_data: Write request to the controller
>   *
> @@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>         return tcs_ctrl_write(drv, msg);
>  }
>  
> +int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
> +{
> +       struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
> +       int ret = 0;
> +
> +       /*
> +        * RPMh domain can not be powered off when there is pending ACK for
> +        * ACTIVE_TCS request. Exit when controller is busy.
> +        */
> +
> +       ret = rpmh_rsc_ctrlr_is_idle(drv);
> +       if (!ret)
> +               goto exit;

return 0? Shouldn't it return some negative value?

> +
> +       ret = rpmh_flush(&drv->client);
> +       if (ret)
> +               goto exit;

Why not just return rpmh_flush(...)?

The usage of goto in this function is entirely unnecessary.

> +
> +exit:
> +       return ret;
> +}
> +
> +static int rpmh_probe_power_domain(struct platform_device *pdev,
> +                                  struct rsc_drv *drv)
> +{
> +       int ret = -ENOMEM;
> +       struct generic_pm_domain *rsc_pd = &drv->rsc_pd;
> +       struct device_node *dn = pdev->dev.of_node;
> +
> +       rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name);
> +       if (!rsc_pd->name)
> +               goto exit;

return -ENOMEM;

> +
> +       rsc_pd->name = kbasename(rsc_pd->name);
> +       rsc_pd->power_off = rpmh_domain_power_off;
> +       rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE;
> +
> +       ret = pm_genpd_init(rsc_pd, NULL, false);
> +       if (ret)
> +               goto free_name;
> +
> +       ret = of_genpd_add_provider_simple(dn, rsc_pd);
> +       if (ret)
> +               goto remove_pd;
> +
> +       pr_debug("init PM domain %s\n", rsc_pd->name);
> +
> +       return ret;

	ret = of_genpd_add_provider_simple(...)
	if (!ret)
		return 0;

Drop the pr_debug(), it's not useful.

> +
> +remove_pd:
> +       pm_genpd_remove(rsc_pd);
> +
> +free_name:
> +       kfree(rsc_pd->name);
> +
> +exit:
> +       return ret;

Please remove newlines between labels above.

Re: [PATCH 4/4] drivers: qcom: rpmh-rsc: Add RSC power domain support

[Maulik Shah]

On 8/14/2019 11:55 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-13 01:24:42)
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index e278fc11fe5c..bd8e9f1a43b4 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>>          return ret;
>>   }
>>   
>> +/**
>> + *  rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy.
>> + *
>> + *  @drv: The controller
>> + *
>> + *  Returns false if the TCSes are engaged in handling requests,
>> + *  True if controller is idle.
>> + */
>> +static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv)
>> +{
>> +       int m;
>> +       struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
>> +       bool ret = true;
>> +
>> +       spin_lock(&drv->lock);
>> +       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>> +               if (!tcs_is_free(drv, m)) {
> Isn't this a copy of an existing function in the rpmh driver?
No.  The changes to add this were previously posted but did not went it.
>> +                       ret = false;
>> +                       break;
>> +               }
>> +       }
>> +       spin_unlock(&drv->lock);
>> +
>> +       return ret;
>> +}
>> +
>>   /**
>>    * rpmh_rsc_write_ctrl_data: Write request to the controller
>>    *
>> @@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>>          return tcs_ctrl_write(drv, msg);
>>   }
>>   
>> +int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
>> +{
>> +       struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
>> +       int ret = 0;
>> +
>> +       /*
>> +        * RPMh domain can not be powered off when there is pending ACK for
>> +        * ACTIVE_TCS request. Exit when controller is busy.
>> +        */
>> +
>> +       ret = rpmh_rsc_ctrlr_is_idle(drv);
>> +       if (!ret)
>> +               goto exit;
> return 0? Shouldn't it return some negative value?
Done.
>> +
>> +       ret = rpmh_flush(&drv->client);
>> +       if (ret)
>> +               goto exit;
> Why not just return rpmh_flush(...)?
>
> The usage of goto in this function is entirely unnecessary.
Done.
>> +
>> +exit:
>> +       return ret;
>> +}
>> +
>> +static int rpmh_probe_power_domain(struct platform_device *pdev,
>> +                                  struct rsc_drv *drv)
>> +{
>> +       int ret = -ENOMEM;
>> +       struct generic_pm_domain *rsc_pd = &drv->rsc_pd;
>> +       struct device_node *dn = pdev->dev.of_node;
>> +
>> +       rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name);
>> +       if (!rsc_pd->name)
>> +               goto exit;
> return -ENOMEM;
Done.
>> +
>> +       rsc_pd->name = kbasename(rsc_pd->name);
>> +       rsc_pd->power_off = rpmh_domain_power_off;
>> +       rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE;
>> +
>> +       ret = pm_genpd_init(rsc_pd, NULL, false);
>> +       if (ret)
>> +               goto free_name;
>> +
>> +       ret = of_genpd_add_provider_simple(dn, rsc_pd);
>> +       if (ret)
>> +               goto remove_pd;
>> +
>> +       pr_debug("init PM domain %s\n", rsc_pd->name);
>> +
>> +       return ret;
> 	ret = of_genpd_add_provider_simple(...)
> 	if (!ret)
> 		return 0;
>
> Drop the pr_debug(), it's not useful.
Done.
>> +
>> +remove_pd:
>> +       pm_genpd_remove(rsc_pd);
>> +
>> +free_name:
>> +       kfree(rsc_pd->name);
>> +
>> +exit:
>> +       return ret;
> Please remove newlines between labels above.
Done.

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

Re: [PATCH 0/4] Add RSC power domain support

[Stephen Boyd]

Quoting Maulik Shah (2019-08-13 01:24:38)
> Resource State Coordinator (RSC) is responsible for powering off/lowering
> the requirements from CPU subsystem for the associated hardware like buses,
> clocks, and regulators when all CPUs and cluster is powered down.
> 
> RSC power domain uses last-man activities provided by genpd framework based on
> Ulf Hansoon's patch series[1], when the cluster of CPUs enter deepest idle
> states. As a part of domain poweroff, RSC can lower resource state requirements
> by flushing the cached sleep and wake state votes for resources.

This series looks like half the solution. Is there a full set of patches
that connects the RPMh power domain to cpuidle and genpds?

Re: [PATCH 0/4] Add RSC power domain support

[Maulik Shah]

On 8/14/2019 11:49 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-13 01:24:38)
>> Resource State Coordinator (RSC) is responsible for powering off/lowering
>> the requirements from CPU subsystem for the associated hardware like buses,
>> clocks, and regulators when all CPUs and cluster is powered down.
>>
>> RSC power domain uses last-man activities provided by genpd framework based on
>> Ulf Hansoon's patch series[1], when the cluster of CPUs enter deepest idle
>> states. As a part of domain poweroff, RSC can lower resource state requirements
>> by flushing the cached sleep and wake state votes for resources.
> This series looks like half the solution. Is there a full set of patches
> that connects the RPMh power domain to cpuidle and genpds?
Yes, i will include in next version.

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