[PATCH v2 0/6] Add RSC power domain support

[PATCH v2 0/6] Add RSC power domain support

[Maulik Shah]

Changes in v2:
- Add Stephen's Reviewed-By to the first three patches
- Addressed Stephen's comments on fourth patch
- Include changes to connect rpmh domain to cpuidle and genpds

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 (6):
  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
  arm64: dts: Convert to the hierarchical CPU topology layout for sdm845
  arm64: dts: Add rsc power domain for sdm845

 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt |   8 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 105 +++++++++++++-----
 drivers/soc/qcom/rpmh-internal.h              |   3 +
 drivers/soc/qcom/rpmh-rsc.c                   |  84 ++++++++++++++
 drivers/soc/qcom/rpmh.c                       |  22 ++--
 include/soc/qcom/rpmh.h                       |   5 -
 6 files changed, 185 insertions(+), 42 deletions(-)

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

[PATCH v2 1/6] 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>
Reviewed-by: Stephen Boyd <swboyd@chromium.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.

[PATCH v2 2/6] 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>
Reviewed-by: Stephen Boyd <swboyd@chromium.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.

[PATCH v2 3/6] 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>
Reviewed-by: Stephen Boyd <swboyd@chromium.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 v2 3/6] dt-bindings: soc: qcom: Add RSC power domain specifier

[Rob Herring]

On Fri, Aug 23, 2019 at 01:47:00PM +0530, Maulik Shah wrote:
> 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>
> ---
>  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.

What's the value? It's always 0?

> +
>  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 v2 3/6] dt-bindings: soc: qcom: Add RSC power domain specifier

[Maulik Shah]

On 8/28/2019 4:02 AM, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 01:47:00PM +0530, Maulik Shah wrote:
>> 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>
>> ---
>>   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.
> What's the value? It's always 0?

yes. its value is always 0. i will update definition to mention this in 
next version.

>> +
>>   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.
>>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 4/6] 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      | 84 ++++++++++++++++++++++++++++++++
 2 files changed, 86 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..884b39599e8f 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,53 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return tcs_ctrl_write(drv, msg);
 }
 
+static int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
+{
+	struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
+
+	/*
+	 * RPMh domain can not be powered off when there is pending ACK for
+	 * ACTIVE_TCS request. Exit when controller is busy.
+	 */
+
+	if (!rpmh_rsc_ctrlr_is_idle(drv))
+		return -EBUSY;
+
+	return rpmh_flush(&drv->client);
+}
+
+static int rpmh_probe_power_domain(struct platform_device *pdev,
+				   struct rsc_drv *drv)
+{
+	int ret;
+	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)
+		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;
+
+	return ret;
+
+remove_pd:
+	pm_genpd_remove(rsc_pd);
+free_name:
+	kfree(rsc_pd->name);
+	return ret;
+}
+
 static int rpmh_probe_tcs_config(struct platform_device *pdev,
 				 struct rsc_drv *drv)
 {
@@ -650,6 +723,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 v2 4/6] drivers: qcom: rpmh-rsc: Add RSC power domain support

[Stephen Boyd]

Quoting Maulik Shah (2019-08-23 01:17:01)
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..884b39599e8f 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,

Please use kernel-doc style for returns here.

> + *  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);

I think these need to be irqsave/restore still.

> +       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> +               if (!tcs_is_free(drv, m)) {

This snippet is from tcs_invalidate(). Please collapse it into some sort
of function or macro like for_each_tcs().

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

Nitpick: Remove this extra newline.

> +       if (!rpmh_rsc_ctrlr_is_idle(drv))
> +               return -EBUSY;
> +
> +       return rpmh_flush(&drv->client);
> +}
> +
> +static int rpmh_probe_power_domain(struct platform_device *pdev,
> +                                  struct rsc_drv *drv)
> +{
> +       int ret;
> +       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);

Maybe use devm_kasprintf?

> +       if (!rsc_pd->name)
> +               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;
> +
> +       return ret;
> +
> +remove_pd:
> +       pm_genpd_remove(rsc_pd);
> +free_name:
> +       kfree(rsc_pd->name);

And then drop this one?

> +       return ret;
> +}
> +
>  static int rpmh_probe_tcs_config(struct platform_device *pdev,
>                                  struct rsc_drv *drv)
>  {
> @@ -650,6 +723,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);

What happens if it fails later on? The genpd provider is still sitting
around and needs to be removed on probe failure later on in this
function. It would be nicer if there wasn't another function to probe
the power domain and it was just inlined here actually. That way we
don't have to wonder about what's going on across two blocks of code.

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

[Maulik Shah]

On 9/5/2019 11:02 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-23 01:17:01)
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index e278fc11fe5c..884b39599e8f 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,
> Please use kernel-doc style for returns here.
Done.
>> + *  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);
> I think these need to be irqsave/restore still.
Done.
>> +       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>> +               if (!tcs_is_free(drv, m)) {
> This snippet is from tcs_invalidate(). Please collapse it into some sort
> of function or macro like for_each_tcs().
Keeping it as it is, the snippet is actually little different from 
tcs_invalidate.
>> +                       ret = false;
>> +                       break;
>> +               }
>> +       }
>> +       spin_unlock(&drv->lock);
>> +
>> +       return ret;
>> +}
>> +
>>   /**
>>    * rpmh_rsc_write_ctrl_data: Write request to the controller
>>    *
>> @@ -521,6 +547,53 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>>          return tcs_ctrl_write(drv, msg);
>>   }
>>   
>> +static int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
>> +{
>> +       struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
>> +
>> +       /*
>> +        * RPMh domain can not be powered off when there is pending ACK for
>> +        * ACTIVE_TCS request. Exit when controller is busy.
>> +        */
>> +
> Nitpick: Remove this extra newline.
Done.
>> +       if (!rpmh_rsc_ctrlr_is_idle(drv))
>> +               return -EBUSY;
>> +
>> +       return rpmh_flush(&drv->client);
>> +}
>> +
>> +static int rpmh_probe_power_domain(struct platform_device *pdev,
>> +                                  struct rsc_drv *drv)
>> +{
>> +       int ret;
>> +       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);
> Maybe use devm_kasprintf?
Done.
>> +       if (!rsc_pd->name)
>> +               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;
>> +
>> +       return ret;
>> +
>> +remove_pd:
>> +       pm_genpd_remove(rsc_pd);
>> +free_name:
>> +       kfree(rsc_pd->name);
> And then drop this one?
Done.
>> +       return ret;
>> +}
>> +
>>   static int rpmh_probe_tcs_config(struct platform_device *pdev,
>>                                   struct rsc_drv *drv)
>>   {
>> @@ -650,6 +723,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);
> What happens if it fails later on? The genpd provider is still sitting
> around and needs to be removed on probe failure later on in this
> function. It would be nicer if there wasn't another function to probe
> the power domain and it was just inlined here actually. That way we
> don't have to wonder about what's going on across two blocks of code.

Thanks for pointing this.  Moved it at the end of probe to avoid this.

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

[PATCH v2 5/6] arm64: dts: Convert to the hierarchical CPU topology layout for sdm845

[Maulik Shah]

In the hierarchical layout, we are creating power domains around each CPU
and describes the idle states for them inside the power domain provider
node. Note that, the CPU's idle states still needs to be compatible with
"arm,idle-state".

Furthermore, represent the CPU cluster as a separate master power domain,
powering the CPU's power domains. The cluster node, contains the idle
states for the cluster and each idle state needs to be compatible with
the "domain-idle-state".

If the running platform is using a PSCI FW that supports the OS initiated
CPU suspend mode, which likely should be the case unless the PSCI FW is
very old, this change triggers the PSCI driver to enable it.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 103 ++++++++++++++++++++-------
 1 file changed, 78 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 4babff5f19b5..0e7f36d2a7d9 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -190,9 +190,8 @@
 			compatible = "qcom,kryo385";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 			capacity-dmips-mhz = <607>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
@@ -211,9 +210,8 @@
 			compatible = "qcom,kryo385";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 			capacity-dmips-mhz = <607>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
@@ -229,9 +227,8 @@
 			compatible = "qcom,kryo385";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 			capacity-dmips-mhz = <607>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
@@ -247,9 +244,8 @@
 			compatible = "qcom,kryo385";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 			capacity-dmips-mhz = <607>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
@@ -266,9 +262,8 @@
 			reg = <0x0 0x400>;
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD4>;
+			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_400>;
@@ -284,9 +279,8 @@
 			reg = <0x0 0x500>;
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD5>;
+			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_500>;
@@ -302,9 +296,8 @@
 			reg = <0x0 0x600>;
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD6>;
+			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_600>;
@@ -320,9 +313,8 @@
 			reg = <0x0 0x700>;
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD7>;
+			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_700>;
@@ -412,7 +404,7 @@
 			};
 
 			CLUSTER_SLEEP_0: cluster-sleep-0 {
-				compatible = "arm,idle-state";
+				compatible = "domain-idle-state";
 				idle-state-name = "cluster-power-down";
 				arm,psci-suspend-param = <0x400000F4>;
 				entry-latency-us = <3263>;
@@ -618,6 +610,67 @@
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: cpu-pd0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+						<&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD1: cpu-pd1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+						<&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD2: cpu-pd2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+						<&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD3: cpu-pd3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+						<&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD4: cpu-pd4 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+						<&BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD5: cpu-pd5 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+						<&BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD6: cpu-pd6 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+						<&BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD7: cpu-pd7 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+						<&BIG_CPU_SLEEP_1>;
+		};
+
+		CLUSTER_PD: cluster-pd {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
+		};
 	};
 
 	soc: soc {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH v2 6/6] arm64: dts: Add rsc power domain for sdm845

[Maulik Shah]

Add rsc power domain to enable sending sleep and wake votes
using generic power domain infrastructure.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0e7f36d2a7d9..1ea61464e666 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -669,6 +669,7 @@
 
 		CLUSTER_PD: cluster-pd {
 			#power-domain-cells = <0>;
+			power-domains = <&apps_rsc>;
 			domain-idle-states = <&CLUSTER_SLEEP_0>;
 		};
 	};
@@ -2587,6 +2588,7 @@
 					  <SLEEP_TCS   3>,
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
+			#power-domain-cells = <0>;
 
 			rpmhcc: clock-controller {
 				compatible = "qcom,sdm845-rpmh-clk";
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH v2 6/6] arm64: dts: Add rsc power domain for sdm845

[Stephen Boyd]

Quoting Maulik Shah (2019-08-23 01:17:03)
> Add rsc power domain to enable sending sleep and wake votes
> using generic power domain infrastructure.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Can this be combined with the previous patch?

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

[Matthias Kaehlcke]

Hi Maulik,

What is the status of this series? It seems it hasn't been updated since
you sent it in August last year. Do you plan to send a v3 in the near future
to address the outstanding comments?

Thanks

Matthias

On Fri, Aug 23, 2019 at 01:46:57PM +0530, Maulik Shah wrote:
> Changes in v2:
> - Add Stephen's Reviewed-By to the first three patches
> - Addressed Stephen's comments on fourth patch
> - Include changes to connect rpmh domain to cpuidle and genpds
> 
> 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 (6):
>   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
>   arm64: dts: Convert to the hierarchical CPU topology layout for sdm845
>   arm64: dts: Add rsc power domain for sdm845
> 
>  .../devicetree/bindings/soc/qcom/rpmh-rsc.txt |   8 ++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          | 105 +++++++++++++-----
>  drivers/soc/qcom/rpmh-internal.h              |   3 +
>  drivers/soc/qcom/rpmh-rsc.c                   |  84 ++++++++++++++
>  drivers/soc/qcom/rpmh.c                       |  22 ++--
>  include/soc/qcom/rpmh.h                       |   5 -
>  6 files changed, 185 insertions(+), 42 deletions(-)
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
> 

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

[Maulik Shah]

Hi Matthias,

Yes i will soon post a v3 series addressing outstanding comments.

Thanks,

Maulik

On 1/22/2020 12:35 AM, Matthias Kaehlcke wrote:
> Hi Maulik,
>
> What is the status of this series? It seems it hasn't been updated since
> you sent it in August last year. Do you plan to send a v3 in the near future
> to address the outstanding comments?

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