Skip to contentSkip to conversation

Invoke rpmh_flush for non OSI targets

PATCH Superseded Maulik Shah 2 participants 17 messages Mar 26, 2020 – Mar 31, 2020 1 message not in the archive

Subsystems (4)

v14 of 17 Superseded Latest is v17 →

lore
Share & tools
lore
#0 lore raw

v14 cover Invoke rpmh_flush for non OSI targets

Changes in v14:
- Address Doug's comments on change 3 from v13
- Drop new APIs for start and end transaction from change 4 in v13
- Update change 4 to use cpu pm notifications instead
- Add [5] as change 5 to enable use of WAKE TCS when ACTIVE TCS count is 0
- Add change 6 to Allow multiple WAKE TCS to be used as ACTIVE TCSes
- First 4 changes can be merged even without change 5 and 6.

Changes in v13:
- Address Stephen's comment to maintain COMPILE_TEST
- Address Doug's comments and add new APIs for start and end transaction

Changes in v12:
- Kconfig change to remove COMPILE_TEST was dropped in v11, reinclude it.

Changes in v11:
- Address Doug's comments on change 2 and 3
- Include change to invalidate TCSes before flush from [4]

Changes in v10:
- Address Evan's comments to update commit message on change 2
- Add Evan's Reviewed by on change 2
- Remove comment from rpmh_flush() related to last CPU invoking it
- Rebase all changes on top of next-20200302

Changes in v9:
- Keep rpmh_flush() to invoke from within cache_lock
- Remove comments related to only last cpu invoking rpmh_flush()

Changes in v8:
- Address Stephen's comments on changes 2 and 3
- Add Reviewed by from Stephen on change 1

Changes in v7:
- Address Srinivas's comments to update commit text
- Add Reviewed by from Srinivas

Changes in v6:
- Drop 1 & 2 changes from v5 as they already landed in maintainer tree
- Drop 3 & 4 changes from v5 as no user at present for power domain in rsc
- Rename subject to appropriate since power domain changes are dropped
- Rebase other changes on top of next-20200221

Changes in v5:
- Add Rob's Acked by on dt-bindings change
- Drop firmware psci change
- Update cpuidle stats in dtsi to follow PC mode
- Include change to update dirty flag when data is updated from [4]
- Add change to invoke rpmh_flush when caches are dirty

Changes in v4:
- Add change to allow hierarchical topology in PC mode
- Drop hierarchical domain idle states converter from v3
- Address Merge sc7180 dtsi change to add low power modes

Changes in v3:
- Address Rob's comment on dt property value
- Address Stephen's comments on rpmh-rsc driver change
- Include sc7180 cpuidle low power mode changes from [1]
- Include hierarchical domain idle states converter change from [2]

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[3], 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 various resources.

[1] https://patchwork.kernel.org/patch/11218965
[2] https://patchwork.kernel.org/patch/10941671
[3] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=222355
[4] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=236503
[5] https://patchwork.kernel.org/patch/10818129

Maulik Shah (5):
arm64: dts: qcom: sc7180: Add cpuidle low power states
soc: qcom: rpmh: Update dirty flag only when data changes
soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new
data
soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches
soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

Raju P.L.S.S.S.N (1):
soc: qcom: rpmh-rsc: Clear active mode configuration for wake TCS

 arch/arm64/boot/dts/qcom/sc7180.dtsi |  78 ++++++++++++++
 drivers/soc/qcom/rpmh-internal.h     |   8 ++
 drivers/soc/qcom/rpmh-rsc.c          | 203 ++++++++++++++++++++++++++++-------
 drivers/soc/qcom/rpmh.c              |  71 ++++++------
 4 files changed, 280 insertions(+), 80 deletions(-)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#1 lore raw

PATCH v14 1/6 arm64: dts: qcom: sc7180: Add cpuidle low power states

Add device bindings for cpuidle states for cpu devices.

---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
arch/arm64/boot/dts/qcom/sc7180.dtsi +78 -0
index 253274d..f5c08ce 100644
@@ -94,6 +94,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_0>;
@@ -113,6 +116,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_100>;
@@ -129,6 +135,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_200>;
@@ -145,6 +154,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_300>;
@@ -161,6 +173,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x400>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_400>;
@@ -177,6 +192,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x500>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
 			next-level-cache = <&L2_500>;
@@ -193,6 +211,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x600>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0
+					   &BIG_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1740>;
 			dynamic-power-coefficient = <405>;
 			next-level-cache = <&L2_600>;
@@ -209,6 +230,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x700>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0
+					   &BIG_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			capacity-dmips-mhz = <1740>;
 			dynamic-power-coefficient = <405>;
 			next-level-cache = <&L2_700>;
@@ -255,6 +279,60 @@
 				};
 			};
 		};
+
+		idle-states {
+			entry-method = "psci";
+
+			LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <549>;
+				exit-latency-us = <901>;
+				min-residency-us = <1774>;
+				local-timer-stop;
+			};
+
+			LITTLE_CPU_SLEEP_1: cpu-sleep-0-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-rail-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <702>;
+				exit-latency-us = <915>;
+				min-residency-us = <4001>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <523>;
+				exit-latency-us = <1244>;
+				min-residency-us = <2207>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_1: cpu-sleep-1-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-rail-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <526>;
+				exit-latency-us = <1854>;
+				min-residency-us = <5555>;
+				local-timer-stop;
+			};
+
+			CLUSTER_SLEEP_0: cluster-sleep-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "cluster-power-down";
+				arm,psci-suspend-param = <0x40003444>;
+				entry-latency-us = <3263>;
+				exit-latency-us = <6562>;
+				min-residency-us = <9926>;
+				local-timer-stop;
+			};
+		};
 	};
 
 	memory@80000000 {
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#2 lore raw

PATCH v14 2/6 soc: qcom: rpmh: Update dirty flag only when data changes

In reply to Maulik Shah

Currently rpmh ctrlr dirty flag is set for all cases regardless of data is really changed or not. Add changes to update dirty flag when data is changed to newer values. Update dirty flag everytime when data in batch cache is updated since rpmh_flush() may get invoked from any CPU instead of only last CPU going to low power mode.

Also move dirty flag updates to happen from within cache_lock and remove unnecessary INIT_LIST_HEAD() call and a default case from switch.

Fixes 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests")
---
 drivers/soc/qcom/rpmh.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
drivers/soc/qcom/rpmh.c +11 -8
index eb0ded0..03630ae 100644
@@ -119,6 +119,7 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
 {
 	struct cache_req *req;
 	unsigned long flags;
+	u32 old_sleep_val, old_wake_val;
 
 	spin_lock_irqsave(&ctrlr->cache_lock, flags);
 	req = __find_req(ctrlr, cmd->addr);
@@ -133,26 +134,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
 
 	req->addr = cmd->addr;
 	req->sleep_val = req->wake_val = UINT_MAX;
-	INIT_LIST_HEAD(&req->list);
 	list_add_tail(&req->list, &ctrlr->cache);
 
 existing:
+	old_sleep_val = req->sleep_val;
+	old_wake_val = req->wake_val;
+
 	switch (state) {
 	case RPMH_ACTIVE_ONLY_STATE:
-		if (req->sleep_val != UINT_MAX)
-			req->wake_val = cmd->data;
-		break;
 	case RPMH_WAKE_ONLY_STATE:
 		req->wake_val = cmd->data;
 		break;
 	case RPMH_SLEEP_STATE:
 		req->sleep_val = cmd->data;
 		break;
-	default:
-		break;
 	}
 
-	ctrlr->dirty = true;
+	ctrlr->dirty = (req->sleep_val != old_sleep_val ||
+			req->wake_val != old_wake_val) &&
+			req->sleep_val != UINT_MAX &&
+			req->wake_val != UINT_MAX;
+
 unlock:
 	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 
@@ -287,6 +289,7 @@ static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
 
 	spin_lock_irqsave(&ctrlr->cache_lock, flags);
 	list_add_tail(&req->list, &ctrlr->batch_cache);
+	ctrlr->dirty = true;
 	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 }
 
@@ -323,6 +326,7 @@ static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
 	list_for_each_entry_safe(req, tmp, &ctrlr->batch_cache, list)
 		kfree(req);
 	INIT_LIST_HEAD(&ctrlr->batch_cache);
+	ctrlr->dirty = true;
 	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 }
 
@@ -507,7 +511,6 @@ int rpmh_invalidate(const struct device *dev)
 	int ret;
 
 	invalidate_batch(ctrlr);
-	ctrlr->dirty = true;
 
 	do {
 		ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#3 lore raw

PATCH v14 3/6 soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new data

In reply to Maulik Shah

TCSes have previously programmed data when rpmh_flush is called. This can cause old data to trigger along with newly flushed.

Fix this by cleaning SLEEP and WAKE TCSes before new data is flushed.

With this there is no need to invoke rpmh_rsc_invalidate() call from rpmh_invalidate().

Simplify rpmh_invalidate() by moving invalidate_batch() inside.

Fixes 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests")
---
 drivers/soc/qcom/rpmh.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)
drivers/soc/qcom/rpmh.c +18 -23
index 03630ae..5147d82 100644
@@ -317,19 +317,6 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
 	return ret;
 }
 
-static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
-{
-	struct batch_cache_req *req, *tmp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ctrlr->cache_lock, flags);
-	list_for_each_entry_safe(req, tmp, &ctrlr->batch_cache, list)
-		kfree(req);
-	INIT_LIST_HEAD(&ctrlr->batch_cache);
-	ctrlr->dirty = true;
-	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
-}
-
 /**
  * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
  * batch to finish.
@@ -467,6 +454,13 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 		return 0;
 	}
 
+	/* Invalidate the TCSes first to avoid stale data */
+	do {
+		ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
+	} while (ret == -EAGAIN);
+	if (ret)
+		return ret;
+
 	/* First flush the cached batch requests */
 	ret = flush_batch(ctrlr);
 	if (ret)
@@ -498,24 +492,25 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 }
 
 /**
- * rpmh_invalidate: Invalidate all sleep and active sets
- * sets.
+ * rpmh_invalidate: Invalidate sleep and active sets in batch_cache
  *
  * @dev: The device making the request
  *
- * Invalidate the sleep and active values in the TCS blocks.
+ * Invalidate the sleep and wake values in batch_cache.
  */
 int rpmh_invalidate(const struct device *dev)
 {
 	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
-	int ret;
-
-	invalidate_batch(ctrlr);
+	struct batch_cache_req *req, *tmp;
+	unsigned long flags;
 
-	do {
-		ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
-	} while (ret == -EAGAIN);
+	spin_lock_irqsave(&ctrlr->cache_lock, flags);
+	list_for_each_entry_safe(req, tmp, &ctrlr->batch_cache, list)
+		kfree(req);
+	INIT_LIST_HEAD(&ctrlr->batch_cache);
+	ctrlr->dirty = true;
+	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(rpmh_invalidate);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#4 lore raw

Re: [PATCH v14 3/6] soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new data

Hi,

Quoted 5 lines · Maulik Shah

/**
- * rpmh_invalidate: Invalidate all sleep and active sets
- * sets.
+ * rpmh_invalidate: Invalidate sleep and active sets in batch_cache

s/and active/and wake/

...with that, feel free to add my Reviewed-by tag.

-Doug

#5 lore raw

Re: [PATCH v14 3/6] soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new data

Hi,

Quoted 7 lines · Doug Anderson

Hi,

Quoted 4 lines · Maulik Shah

/**
- * rpmh_invalidate: Invalidate all sleep and active sets
- * sets.
+ * rpmh_invalidate: Invalidate sleep and active sets in batch_cache

Quoted 5 lines

s/and active/and wake/

...with that, feel free to add my Reviewed-by tag.

-Doug

Thankd Doug for the review. I will update this in v15.

Thanks,
Maulik

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#6 lore raw

PATCH v14 4/6 soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches

In reply to Maulik Shah

Add changes to invoke rpmh flush() from cpu pm notification. This is done when last cpu is entering power collapse and controller is not busy.

Controller that do have 'HW solver' mode do not need to register for cpu pm notification. They may be in autonomous mode executing low power mode do not require rpmh_flush() to happen from CPU PM notification.

---
 drivers/soc/qcom/rpmh-internal.h |  8 ++++
 drivers/soc/qcom/rpmh-rsc.c      | 99 +++++++++++++++++++++++++++++++++++++---
 drivers/soc/qcom/rpmh.c          | 15 ++----
 3 files changed, 103 insertions(+), 19 deletions(-)
drivers/soc/qcom/rpmh-internal.h +8 -0
index 6eec32b..33ee50b 100644
@@ -85,22 +85,30 @@ struct rpmh_ctrlr {
  * Resource State Coordinator controller (RSC)
  *
  * @name:       controller identifier
+ * @base:       start address of the RSC's DRV registers
  * @tcs_base:   start address of the TCS registers in this controller
  * @id:         instance id in the controller (Direct Resource Voter)
  * @num_tcs:    number of TCSes in this DRV
+ * @rsc_pm:     CPU PM notifier for controller
+ * @cpus_in_pc: CPU mask for cpus in idle power collapse
  * @tcs:        TCS groups
  * @tcs_in_use: s/w state of the TCS
  * @lock:       synchronize state of the controller
+ * @pm_lock:    synchronize during PM notifications
  * @client:     handle to the DRV's client.
  */
 struct rsc_drv {
 	const char *name;
+	void __iomem *base;
 	void __iomem *tcs_base;
 	int id;
 	int num_tcs;
+	struct notifier_block rsc_pm;
+	struct cpumask cpus_in_pc;
 	struct tcs_group tcs[TCS_TYPE_NR];
 	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
 	spinlock_t lock;
+	spinlock_t pm_lock;
 	struct rpmh_ctrlr client;
 };
 
drivers/soc/qcom/rpmh-rsc.c +92 -7

144 lines not on this page · raw message

drivers/soc/qcom/rpmh.c +3 -12
index 5147d82..565b6e4 100644
@@ -297,12 +297,10 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
 {
 	struct batch_cache_req *req;
 	const struct rpmh_request *rpm_msg;
-	unsigned long flags;
 	int ret = 0;
 	int i;
 
 	/* Send Sleep/Wake requests to the controller, expect no response */
-	spin_lock_irqsave(&ctrlr->cache_lock, flags);
 	list_for_each_entry(req, &ctrlr->batch_cache, list) {
 		for (i = 0; i < req->count; i++) {
 			rpm_msg = req->rpm_msgs + i;
@@ -312,7 +310,6 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
 				break;
 		}
 	}
-	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 
 	return ret;
 }
@@ -437,12 +434,10 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
  *
  * @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.
+ * Return: 0 on success, error number otherwise.
  *
- * This function is always called from the sleep code from the last CPU
- * that is powering down the entire system. Since no other RPMH API would be
- * executing at this time, it is safe to run lockless.
+ * This function can either be called from sleep code on the last CPU
+ * (thus no spinlock needed) or with the ctrlr->cache_lock already held.
  */
 int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 {
@@ -466,10 +461,6 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 	if (ret)
 		return ret;
 
-	/*
-	 * Nobody else should be calling this function other than system PM,
-	 * hence we can run without locks.
-	 */
 	list_for_each_entry(p, &ctrlr->cache, list) {
 		if (!is_req_valid(p)) {
 			pr_debug("%s: skipping RPMH req: a:%#x s:%#x w:%#x",
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#7 lore raw

Re: [PATCH v14 4/6] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches

Hi,

Quoted 3 lines · Maulik Shah

Add changes to invoke rpmh flush() from cpu pm notification. This is done when last cpu is entering power collapse and

s/when last/when the last/

Quoted 1 line

controller is not busy.

A few overall comments:

* CPU_PM certainly seems interesting and I wasn't aware of it.

* Your series is predicated on nobody calling any of the RPMH functions after the last CPU enters low power mode with CPU_PM_ENTER and before some CPU calls CPU_PM_EXIT. Is it worth adding a check for that to the code? This is super important for the 'zero-active-TCS' case but even for the 'some-active-TCS' it's still important that nobody tries to update the sleep/wake values after you flush.

* I think your series is focused on idle, right? What about suspend/resume? You need a flush before suspend, right? Do you need to register for something to handle suspend, or do the pm callbacks all get called for full system suspend? If so, how late do they get called?

Quoted 1 line

Controller that do have 'HW solver' mode do not need to register

s/Controller/Controllers

drivers/soc/qcom/rpmh-internal.h struct rpmh_ctrlr { · line 93

Quoted 10 lines
drivers/soc/qcom/rpmh-internal.h +3 -0
@@ -85,22 +85,30 @@ struct rpmh_ctrlr {
  * Resource State Coordinator controller (RSC)
  *
  * @name:       controller identifier
+ * @base:       start address of the RSC's DRV registers
  * @tcs_base:   start address of the TCS registers in this controller
  * @id:         instance id in the controller (Direct Resource Voter)
  * @num_tcs:    number of TCSes in this DRV
+ * @rsc_pm:     CPU PM notifier for controller
+ * @cpus_in_pc: CPU mask for cpus in idle power collapse

Name "cpus_entered_pm" to match Linux naming? Also the comment should clearly say that this is only used in non-solver mode.

drivers/soc/qcom/rpmh-internal.h struct rpmh_ctrlr { · line 97

Quoted 4 lines
drivers/soc/qcom/rpmh-internal.h +1 -0
  * @tcs:        TCS groups
  * @tcs_in_use: s/w state of the TCS
  * @lock:       synchronize state of the controller
+ * @pm_lock:    synchronize during PM notifications

Comment should say that this is only for non-solver mode.

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 535

Quoted 10 lines
drivers/soc/qcom/rpmh-rsc.c +6 -0
@@ -521,6 +527,71 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
        return tcs_ctrl_write(drv, msg);
 }
 
+/**
+ * rpmh_rsc_ctrlr_is_busy: Check if any of the AMCs are busy.
+ *
+ * @drv: The controller
+ *
+ * Returns True if the TCSes are engaged in handling requests, False otherwise.

This is almost, but not quite in kernel-doc syntax. Why not make it match the example in "Documentation/doc-guide/kernel-doc.rst"? Specifically trying to follow that style:

* rpmh_rsc_ctrlr_is_busy() - Check if any of the AMCs are busy.
* @drv: The controller
*
* Return: true if TCSes are engaged in handling requests; else false

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 542

Quoted 6 lines
drivers/soc/qcom/rpmh-rsc.c +6 -0
+static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
+{
+       int m;
+       struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
+
+       /**

nit: why double star in "/**"? That's for kerneldoc comments. I don't think it makes sense here...

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 551

Quoted 9 lines
drivers/soc/qcom/rpmh-rsc.c +9 -0
+        * If we made an active request on a RSC that does not have a
+        * dedicated TCS for active state use, then re-purposed wake TCSes
+        * should be checked for not busy
+        */
+       if (!tcs->num_tcs)
+               tcs = get_tcs_of_type(drv, WAKE_TCS);
+
+       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+               if (!tcs_is_free(drv, m))

As per my documentation patch, tcs_is_free():

* Must be called with the drv->lock held or the tcs_lock for the TCS being
* tested.

...I think you're OK calling without the lock since you're running on the last CPU (and you're blocking any other CPUs from coming online) and interrupts are disabled, but please document.

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 565

Quoted 8 lines
drivers/soc/qcom/rpmh-rsc.c +8 -0
+static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *v)
+{
+       struct rsc_drv *drv = container_of(nfb, struct rsc_drv, rsc_pm);
+       unsigned long flags;
+       int ret = NOTIFY_OK;
+
+       spin_lock_irqsave(&drv->pm_lock, flags);

nit: you can get away with just spin_lock(). cpu_pm_enter() is documented to be called with interrupts already disabled so the "irqsave" and "irqrestore" is useless.

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 571

Quoted 5 lines
drivers/soc/qcom/rpmh-rsc.c +5 -0
+       switch (action) {
+       case CPU_PM_ENTER:
+               cpumask_set_cpu(raw_smp_processor_id(), &drv->cpus_in_pc);
+
+               if (!cpumask_equal(&drv->cpus_in_pc, cpu_online_mask))

Is it ever possible to offline a CPU after doing "CPU_PM_ENTER" on it?
If so you'll never be "equal" to the online mask again. This seems
safer:

!cpumask_subset(cpu_online_mask, &drv->cpus_in_pc)

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 584

Quoted 13 lines
drivers/soc/qcom/rpmh-rsc.c +13 -0
+                       goto exit;
+               break;
+       case CPU_PM_ENTER_FAILED:
+       case CPU_PM_EXIT:
+               cpumask_clear_cpu(raw_smp_processor_id(), &drv->cpus_in_pc);
+               goto exit;
+       }
+
+       ret = rpmh_rsc_ctrlr_is_busy(drv);
+       if (ret) {
+               ret = NOTIFY_BAD;
+               goto exit;
+       }

Are you sure you can't just skip the call to rpmh_rsc_ctrlr_is_busy()?
Specifically, I'm not sure why you need to block until the last
active mode transaction is done.

a) If the active mode transaction was on its own TCS, I'd hope that the RPMH hardware would know to let the active transitions happen before triggering the sleep (please confirm)

b) If the active mode transaction was borrowing the wake TCS the flush should notice and return NOTIFY_BAD anyway.

Oh! ...but looking closer at case b) points out a problem anyway. You need to go into rpmh_flush() and stop having it loop on 'while (ret == -EAGAIN)". As per my documentation series (please review) when -EAGAIN is returned it's important that the caller enable interrupts for a little while before trying again. You're not doing that here. Maybe if you fix it then you can get rid of your special case rpmh_rsc_ctrlr_is_busy()?

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 586

Quoted 1 line
drivers/soc/qcom/rpmh-rsc.c +1 -0
+       ret = rpmh_flush(&drv->client);

Back when RPMH first landed we had a whole argument about whether it should be one file or two. Given that it's currently two files, I think the abstraction is that rpmh-rsc generally doesn't call into rpmh except for the very special rpmh_tx_done().

...so seems like the whole pm_callback should move into rpmh.c and not in rpmh-rsc.c?

Quoted 5 lines
drivers/soc/qcom/rpmh-rsc.c +0 -1
@@ -533,21 +604,20 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
        int i, ret, n, st = 0;
        struct tcs_group *tcs;
        struct resource *res;
-       void __iomem *base;

drivers/soc/qcom/rpmh-rsc.c static int rpmh_probe_tcs_config(struct platform_device *pdev, · line 613

Quoted 10 lines
drivers/soc/qcom/rpmh-rsc.c +3 -3
        char drv_id[10] = {0};
 
        snprintf(drv_id, ARRAY_SIZE(drv_id), "drv-%d", drv->id);
        res = platform_get_resource_byname(pdev, IORESOURCE_MEM, drv_id);
-       base = devm_ioremap_resource(&pdev->dev, res);
-       if (IS_ERR(base))
-               return PTR_ERR(base);
+       drv->base = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(drv->base))
+               return PTR_ERR(drv->base);

I don't understand this part of the change. Why move "base" into the driver data. It's not needed after probe. Please undo this. If you really feel like base needs to be separate please move to its own patch.

Quoted 6 lines
drivers/soc/qcom/rpmh-rsc.c +2 -0
@@ -663,6 +734,20 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
        if (ret)
                return ret;
 
+       /*
+        * CPU PM notification are not required for controllers that support

drivers/soc/qcom/rpmh-rsc.c static int rpmh_rsc_probe(struct platform_device *pdev) · line 748

Quoted 10 lines
drivers/soc/qcom/rpmh-rsc.c +10 -0
+        * 'HW solver' mode where they can be in autonomous mode executing low
+        * power mode to power down.
+        */
+       solver_config = readl_relaxed(drv->base + DRV_SOLVER_CONFIG);
+       solver_config &= DRV_HW_SOLVER_MASK << DRV_HW_SOLVER_SHIFT;
+       solver_config = solver_config >> DRV_HW_SOLVER_SHIFT;
+       if (!solver_config) {
+               drv->rsc_pm.notifier_call = rpmh_rsc_cpu_pm_callback;
+               cpu_pm_register_notifier(&drv->rsc_pm);
+               spin_lock_init(&drv->pm_lock);

nit: init the spin lock before registering instead of after.

drivers/soc/qcom/rpmh.c static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state, · line 440

Quoted 11 lines
drivers/soc/qcom/rpmh.c +3 -5
  * @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.
+ * Return: 0 on success, error number otherwise.
  *
- * This function is always called from the sleep code from the last CPU
- * that is powering down the entire system. Since no other RPMH API would be
- * executing at this time, it is safe to run lockless.
+ * This function can either be called from sleep code on the last CPU
+ * (thus no spinlock needed) or with the ctrlr->cache_lock already held.

Now you can remove the "or with the ctrlr->cache_lock already held" since it's no longer true.

-Doug

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

Re: [PATCH v14 4/6] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches

Hi,

Quoted 13 lines · Maulik Shah

* @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.
+ * Return: 0 on success, error number otherwise.
*
- * This function is always called from the sleep code from the last CPU
- * that is powering down the entire system. Since no other RPMH API would be
- * executing at this time, it is safe to run lockless.
+ * This function can either be called from sleep code on the last CPU
+ * (thus no spinlock needed) or with the ctrlr->cache_lock already held.

Quoted 4 lines

Now you can remove the "or with the ctrlr->cache_lock already held" since it's no longer true.

It can be true for other RSCs, so i kept as it is.

I don't really understand this. The cache_lock is only a concept in "rpmh.c". How could another RSC grab the cache lock? If nothing else, can you remove this comment until support for those other RSCs are added and we can evaluate then?

-Doug

#10 lore raw

Re: [PATCH v14 4/6] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches

Hi,

Quoted 18 lines · Doug Anderson

Hi,

Quoted 15 lines · Maulik Shah

* @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.
+ * Return: 0 on success, error number otherwise.
*
- * This function is always called from the sleep code from the last CPU
- * that is powering down the entire system. Since no other RPMH API would be
- * executing at this time, it is safe to run lockless.
+ * This function can either be called from sleep code on the last CPU
+ * (thus no spinlock needed) or with the ctrlr->cache_lock already held.

Now you can remove the "or with the ctrlr->cache_lock already held" since it's no longer true.

Quoted 7 lines
Quoted 1 line

It can be true for other RSCs, so i kept as it is.

I don't really understand this. The cache_lock is only a concept in "rpmh.c". How could another RSC grab the cache lock? If nothing else, can you remove this comment until support for those other RSCs are added and we can evaluate then?

-Doug

Okay i will remove this comment until support for other RSCs are added.

Thanks,
Maulik

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#11 lore raw

PATCH v14 5/6 soc: qcom: rpmh-rsc: Clear active mode configuration for wake TCS

In reply to Maulik Shah

From: "Raju P.L.S.S.S.N" <[email protected]>

For RSCs that have sleep & wake TCS but no dedicated active TCS, wake TCS can be re-purposed to send active requests. Once the active requests are sent and response is received, the active mode configuration needs to be cleared so that controller can use wake TCS for sending wake requests.

Introduce enable_tcs_irq() to enable completion IRQ for repurposed TCSes.

Fixes 2de4b8d33eab (drivers: qcom: rpmh-rsc: allow active requests from wake TCS)
[mkshah: call enable_tcs_irq() within drv->lock, update commit message]
---
 drivers/soc/qcom/rpmh-rsc.c | 77 +++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 23 deletions(-)
drivers/soc/qcom/rpmh-rsc.c +54 -23
index ee052cc..8fa70b4 100644
@@ -207,6 +207,42 @@ static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
 	return NULL;
 }
 
+static void __tcs_trigger(struct rsc_drv *drv, int tcs_id, bool trigger)
+{
+	u32 enable;
+
+	/*
+	 * HW req: Clear the DRV_CONTROL and enable TCS again
+	 * While clearing ensure that the AMC mode trigger is cleared
+	 * and then the mode enable is cleared.
+	 */
+	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0);
+	enable &= ~TCS_AMC_MODE_TRIGGER;
+	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+	enable &= ~TCS_AMC_MODE_ENABLE;
+	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+
+	if (trigger) {
+		/* Enable the AMC mode on the TCS and then trigger the TCS */
+		enable = TCS_AMC_MODE_ENABLE;
+		write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+		enable |= TCS_AMC_MODE_TRIGGER;
+		write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+	}
+}
+
+static void enable_tcs_irq(struct rsc_drv *drv, int tcs_id, bool enable)
+{
+	u32 data;
+
+	data = read_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0);
+	if (enable)
+		data |= BIT(tcs_id);
+	else
+		data &= ~BIT(tcs_id);
+	write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, data);
+}
+
 /**
  * tcs_tx_done: TX Done interrupt handler
  */
@@ -243,6 +279,14 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 		}
 
 		trace_rpmh_tx_done(drv, i, req, err);
+
+		/*
+		 * If wake tcs was re-purposed for sending active
+		 * votes, clear AMC trigger & enable modes and
+		 * disable interrupt for this TCS
+		 */
+		if (!drv->tcs[ACTIVE_TCS].num_tcs)
+			__tcs_trigger(drv, i, false);
 skip:
 		/* Reclaim the TCS */
 		write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
@@ -250,6 +294,13 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 		write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i));
 		spin_lock(&drv->lock);
 		clear_bit(i, drv->tcs_in_use);
+		/*
+		 * Disable interrupt for WAKE TCS to avoid being
+		 * spammed with interrupts coming when the solver
+		 * sends its wake votes.
+		 */
+		if (!drv->tcs[ACTIVE_TCS].num_tcs)
+			enable_tcs_irq(drv, i, false);
 		spin_unlock(&drv->lock);
 		if (req)
 			rpmh_tx_done(req, err);
@@ -291,28 +342,6 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
 }
 
-static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
-{
-	u32 enable;
-
-	/*
-	 * HW req: Clear the DRV_CONTROL and enable TCS again
-	 * While clearing ensure that the AMC mode trigger is cleared
-	 * and then the mode enable is cleared.
-	 */
-	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0);
-	enable &= ~TCS_AMC_MODE_TRIGGER;
-	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
-	enable &= ~TCS_AMC_MODE_ENABLE;
-	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
-
-	/* Enable the AMC mode on the TCS and then trigger the TCS */
-	enable = TCS_AMC_MODE_ENABLE;
-	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
-	enable |= TCS_AMC_MODE_TRIGGER;
-	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
-}
-
 static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 				  const struct tcs_request *msg)
 {
@@ -383,10 +412,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 
 	tcs->req[tcs_id - tcs->offset] = msg;
 	set_bit(tcs_id, drv->tcs_in_use);
+	if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS)
+		enable_tcs_irq(drv, tcs_id, true);
 	spin_unlock(&drv->lock);
 
 	__tcs_buffer_write(drv, tcs_id, 0, msg);
-	__tcs_trigger(drv, tcs_id);
+	__tcs_trigger(drv, tcs_id, true);
 
 done_write:
 	spin_unlock_irqrestore(&tcs->lock, flags);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#12 lore raw

Re: [PATCH v14 5/6] soc: qcom: rpmh-rsc: Clear active mode configuration for wake TCS

Hi,

Quoted 11 lines · Maulik Shah

From: "Raju P.L.S.S.S.N" <[email protected]>

For RSCs that have sleep & wake TCS but no dedicated active TCS, wake TCS can be re-purposed to send active requests. Once the active requests are sent and response is received, the active mode configuration needs to be cleared so that controller can use wake TCS for sending wake requests.

Introduce enable_tcs_irq() to enable completion IRQ for repurposed TCSes.

Quoted 7 lines
Fixes 2de4b8d33eab (drivers: qcom: rpmh-rsc: allow active requests from wake TCS)
[mkshah: call enable_tcs_irq() within drv->lock, update commit message]
---
 drivers/soc/qcom/rpmh-rsc.c | 77 +++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 23 deletions(-)

I was writing my review of v1 at the same time you sent this. Looks like your patch does address the most important piece of feedback I had (adjusting the interrupt enable under spinlock), but some of the other feedback might be nice to incorporate:

https://lore.kernel.org/r/CAD=FV=XmBQb8yfx14T-tMQ68F-h=3UHog744b3X3JZViu15+4g@mail.gmail.com

-Doug

#13 lore raw

PATCH v14 6/6 soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

In reply to Maulik Shah

When there are more than one WAKE TCS available and there is no dedicated ACTIVE TCS available, invalidating all WAKE TCSes and waiting for current transfer to complete in first WAKE TCS blocks using another free WAKE TCS to complete current request.

Remove rpmh_rsc_invalidate() to happen from tcs_write() when WAKE TCSes is re-purposed to be used for Active mode. Clear only currently used WAKE TCS's register configuration.

Mark the caches as dirty so next time when rpmh_flush() is invoked it can invalidate and program cached sleep and wake sets again.

Fixes 2de4b8d33eab (drivers: qcom: rpmh-rsc: allow active requests from wake TCS)
---
 drivers/soc/qcom/rpmh-rsc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
drivers/soc/qcom/rpmh-rsc.c +19 -10
index 8fa70b4..c0513af 100644
@@ -154,8 +154,9 @@ int rpmh_rsc_invalidate(struct rsc_drv *drv)
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 					 const struct tcs_request *msg)
 {
-	int type, ret;
+	int type;
 	struct tcs_group *tcs;
+	unsigned long flags;
 
 	switch (msg->state) {
 	case RPMH_ACTIVE_ONLY_STATE:
@@ -175,18 +176,18 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 	 * If we are making an active request on a RSC that does not have a
 	 * dedicated TCS for active state use, then re-purpose a wake TCS to
 	 * send active votes.
-	 * NOTE: The driver must be aware that this RSC does not have a
-	 * dedicated AMC, and therefore would invalidate the sleep and wake
-	 * TCSes before making an active state request.
+	 *
+	 * NOTE: Mark caches as dirty here since existing data in wake TCS will
+	 * be lost. rpmh_flush() will processed for dirty caches to restore
+	 * data.
 	 */
 	tcs = get_tcs_of_type(drv, type);
 	if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs) {
 		tcs = get_tcs_of_type(drv, WAKE_TCS);
-		if (tcs->num_tcs) {
-			ret = rpmh_rsc_invalidate(drv);
-			if (ret)
-				return ERR_PTR(ret);
-		}
+
+		spin_lock_irqsave(&drv->client.cache_lock, flags);
+		drv->client.dirty = true;
+		spin_unlock_irqrestore(&drv->client.cache_lock, flags);
 	}
 
 	return tcs;
@@ -412,8 +413,16 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 
 	tcs->req[tcs_id - tcs->offset] = msg;
 	set_bit(tcs_id, drv->tcs_in_use);
-	if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS)
+	if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS) {
+		/*
+		 * Clear previously programmed WAKE commands in selected
+		 * repurposed TCS to avoid triggering them. tcs->slots will be
+		 * cleaned from rpmh_flush() by invoking rpmh_rsc_invalidate()
+		 */
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
 		enable_tcs_irq(drv, tcs_id, true);
+	}
 	spin_unlock(&drv->lock);
 
 	__tcs_buffer_write(drv, tcs_id, 0, msg);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
#14 lore raw

Re: [PATCH v14 6/6] soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

Hi,

Quoted 48 lines · Maulik Shah

When there are more than one WAKE TCS available and there is no dedicated ACTIVE TCS available, invalidating all WAKE TCSes and waiting for current transfer to complete in first WAKE TCS blocks using another free WAKE TCS to complete current request.

Remove rpmh_rsc_invalidate() to happen from tcs_write() when WAKE TCSes is re-purposed to be used for Active mode. Clear only currently used WAKE TCS's register configuration.

Mark the caches as dirty so next time when rpmh_flush() is invoked it can invalidate and program cached sleep and wake sets again.

Fixes 2de4b8d33eab (drivers: qcom: rpmh-rsc: allow active requests from wake TCS)
---
 drivers/soc/qcom/rpmh-rsc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
drivers/soc/qcom/rpmh-rsc.c +6 -4
index 8fa70b4..c0513af 100644
@@ -154,8 +154,9 @@ int rpmh_rsc_invalidate(struct rsc_drv *drv)
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                                         const struct tcs_request *msg)
 {
-       int type, ret;
+       int type;
        struct tcs_group *tcs;
+       unsigned long flags;
 
        switch (msg->state) {
        case RPMH_ACTIVE_ONLY_STATE:
@@ -175,18 +176,18 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
         * If we are making an active request on a RSC that does not have a
         * dedicated TCS for active state use, then re-purpose a wake TCS to
         * send active votes.
-        * NOTE: The driver must be aware that this RSC does not have a
-        * dedicated AMC, and therefore would invalidate the sleep and wake
-        * TCSes before making an active state request.
+        *
+        * NOTE: Mark caches as dirty here since existing data in wake TCS will
+        * be lost. rpmh_flush() will processed for dirty caches to restore
+        * data.
         */
        tcs = get_tcs_of_type(drv, type);
        if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs) {

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv, · line 190

Quoted 10 lines
drivers/soc/qcom/rpmh-rsc.c +4 -5
                tcs = get_tcs_of_type(drv, WAKE_TCS);
-               if (tcs->num_tcs) {
-                       ret = rpmh_rsc_invalidate(drv);
-                       if (ret)
-                               return ERR_PTR(ret);
-               }
+
+               spin_lock_irqsave(&drv->client.cache_lock, flags);
+               drv->client.dirty = true;
+               spin_unlock_irqrestore(&drv->client.cache_lock, flags);

This seems like a huge abstraction violation. Why can't rpmh_write() / rpmh_write_async() / rpmh_write_batch() just always unconditionally mark the cache dirty? Are there really lots of cases when those calls are made and they do nothing?

Other than that this patch seems sane to me and addresses one of the comments I had in:

https://lore.kernel.org/r/CAD=FV=XmBQb8yfx14T-tMQ68F-h=3UHog744b3X3JZViu15+4g@mail.gmail.com

...interestingly after your patch I guess now I guess tcs_invalidate() no longer needs spinlocks since it's only ever called from PM code on the last CPU. ...if you agree, I can always do it in my cleanup series. See:

https://lore.kernel.org/r/CAD=FV=Xp1o68HnC2-hMnffDDsi+jjgc9pNrdNuypjQZbS5K4nQ@mail.gmail.com

-Doug

#15 lore raw

Re: [PATCH v14 6/6] soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

Hi,

Quoted 53 lines · Doug Anderson

Hi,

Quoted 50 lines · Maulik Shah

When there are more than one WAKE TCS available and there is no dedicated ACTIVE TCS available, invalidating all WAKE TCSes and waiting for current transfer to complete in first WAKE TCS blocks using another free WAKE TCS to complete current request.

Remove rpmh_rsc_invalidate() to happen from tcs_write() when WAKE TCSes is re-purposed to be used for Active mode. Clear only currently used WAKE TCS's register configuration.

Mark the caches as dirty so next time when rpmh_flush() is invoked it can invalidate and program cached sleep and wake sets again.

Fixes 2de4b8d33eab (drivers: qcom: rpmh-rsc: allow active requests from wake TCS)
---
 drivers/soc/qcom/rpmh-rsc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
drivers/soc/qcom/rpmh-rsc.c +6 -6
index 8fa70b4..c0513af 100644
@@ -154,8 +154,9 @@ int rpmh_rsc_invalidate(struct rsc_drv *drv)
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                                         const struct tcs_request *msg)
 {
-       int type, ret;
+       int type;
        struct tcs_group *tcs;
+       unsigned long flags;
 
        switch (msg->state) {
        case RPMH_ACTIVE_ONLY_STATE:
@@ -175,18 +176,18 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
         * If we are making an active request on a RSC that does not have a
         * dedicated TCS for active state use, then re-purpose a wake TCS to
         * send active votes.
-        * NOTE: The driver must be aware that this RSC does not have a
-        * dedicated AMC, and therefore would invalidate the sleep and wake
-        * TCSes before making an active state request.
+        *
+        * NOTE: Mark caches as dirty here since existing data in wake TCS will
+        * be lost. rpmh_flush() will processed for dirty caches to restore
+        * data.
         */
        tcs = get_tcs_of_type(drv, type);
        if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs) {
                tcs = get_tcs_of_type(drv, WAKE_TCS);
-               if (tcs->num_tcs) {
-                       ret = rpmh_rsc_invalidate(drv);
Quoted 8 lines

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv, · line 190

Quoted 7 lines
drivers/soc/qcom/rpmh-rsc.c +4 -3
-                       if (ret)
-                               return ERR_PTR(ret);
-               }
+
+               spin_lock_irqsave(&drv->client.cache_lock, flags);
+               drv->client.dirty = true;
+               spin_unlock_irqrestore(&drv->client.cache_lock, flags);

This seems like a huge abstraction violation.

Agree that cache_lock and dirty flag are used in rpmh.c

I will address this to either notify rpmh.c to mark it dirty or think of other solution.

Quoted 4 lines

Why can't rpmh_write()
/ rpmh_write_async() / rpmh_write_batch() just always unconditionally mark the cache dirty? Are there really lots of cases when those calls are made and they do nothing?

At rpmh.c, it doesn't know that rpmh-rsc.c worked on borrowed TCS to finish the request.

We should not blindly mark caches dirty everytime.

Quoted 7 lines

Other than that this patch seems sane to me and addresses one of the comments I had in:

https://lore.kernel.org/r/CAD=FV=XmBQb8yfx14T-tMQ68F-h=3UHog744b3X3JZViu15+4g@mail.gmail.com

Quoted 8 lines

...interestingly after your patch I guess now I guess tcs_invalidate() no longer needs spinlocks since it's only ever called from PM code on the last CPU. ...if you agree, I can always do it in my cleanup series. See:

https://lore.kernel.org/r/CAD=FV=Xp1o68HnC2-hMnffDDsi+jjgc9pNrdNuypjQZbS5K4nQ@mail.gmail.com

-Doug

There are other RSCs which use same driver, so lets keep spinlock.

I still didn't get chance to validate your patch (i will have update sometime next week), just to update I have never seen any issue internally

using spin_lock even on nosmp case, that might require it to change to _irq_save/restore variant.

Thanks,
Maulik

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#16 lore raw

Re: [PATCH v14 6/6] soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

Hi,

Quoted 9 lines · Maulik Shah
Quoted 5 lines

Why can't rpmh_write()
/ rpmh_write_async() / rpmh_write_batch() just always unconditionally mark the cache dirty? Are there really lots of cases when those calls are made and they do nothing?

At rpmh.c, it doesn't know that rpmh-rsc.c worked on borrowed TCS to finish the request.

We should not blindly mark caches dirty everytime.

In message ID "[email protected]"

Quoted 1 line · you

yes we should trust callers not to send duplicate data

...you can see some reference to it in my reply:

https://lore.kernel.org/r/CAD=FV=VPSahhK71k_D+nfL1=5QE5sKMQT=6zzyEF7+JWMcTxsg@mail.gmail.com/

If callers are trusted to never send duplicate data then ever call to rpmh_write() will make a change. ...and thus the cache should always be marked dirty, no? Also note that since rpmh_write() to "active" also counts as a write to "wake" even those will dirty the cache.

Which case are you expecting a rpmh_write() call to not dirty the cache?

Quoted 10 lines
Quoted 8 lines

...interestingly after your patch I guess now I guess tcs_invalidate() no longer needs spinlocks since it's only ever called from PM code on the last CPU. ...if you agree, I can always do it in my cleanup series. See:

https://lore.kernel.org/r/CAD=FV=Xp1o68HnC2-hMnffDDsi+jjgc9pNrdNuypjQZbS5K4nQ@mail.gmail.com

-Doug

There are other RSCs which use same driver, so lets keep spinlock.

It is really hard to try to write keeping in mind these "other RSCs" for which there is no code upstream. IMO we should write the code keeping in mind what is supported upstream and then when those "other RSCs" get added we can evaluate their needs.

Specifically when reasoning about rpmh.c and rpmh-rsc.c I can only look at the code that's there now and decide whether it is race free or there are races. Back when I was analyzing the proposal to do rpmh_flush() all the time (not from PM code) it felt like there were a bunch of races, especially in the zero-active-TCS case. Most of the races go away when you assume that rpmh_flush() is only ever called from the PM code when nobody could be in the middle of an active transfer.

If we are ever planning to call rpmh_flush() from another place we need to re-look at all those races.

-Doug

#17 lore raw

Re: [PATCH v14 6/6] soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request

Hi,

Quoted 16 lines · Doug Anderson

Hi,

Quoted 7 lines · Maulik Shah
Quoted 4 lines

Why can't rpmh_write()
/ rpmh_write_async() / rpmh_write_batch() just always unconditionally mark the cache dirty? Are there really lots of cases when those calls are made and they do nothing?

At rpmh.c, it doesn't know that rpmh-rsc.c worked on borrowed TCS to finish the request.

We should not blindly mark caches dirty everytime.

In message ID "[email protected]"

Quoted 1 line · you

yes we should trust callers not to send duplicate data

...you can see some reference to it in my reply:

Quoted 8 lines

https://lore.kernel.org/r/CAD=FV=VPSahhK71k_D+nfL1=5QE5sKMQT=6zzyEF7+JWMcTxsg@mail.gmail.com/

If callers are trusted to never send duplicate data then ever call to rpmh_write() will make a change. ...and thus the cache should always be marked dirty, no? Also note that since rpmh_write() to "active" also counts as a write to "wake" even those will dirty the cache.

Which case are you expecting a rpmh_write() call to not dirty the cache?

Ok, i will remove marking cache dirty here.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

...interestingly after your patch I guess now I guess tcs_invalidate() no longer needs spinlocks since it's only ever called from PM code on the last CPU. ...if you agree, I can always do it in my cleanup series. See:

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

https://lore.kernel.org/r/CAD=FV=Xp1o68HnC2-hMnffDDsi+jjgc9pNrdNuypjQZbS5K4nQ@mail.gmail.com

-Doug

There are other RSCs which use same driver, so lets keep spinlock.

It is really hard to try to write keeping in mind these "other RSCs" for which there is no code upstream. IMO we should write the code keeping in mind what is supported upstream and then when those "other RSCs" get added we can evaluate their needs.

Agree but i would insist not remove locks in your cleanup/documentation series which are already there.

These will be again need to be added.

The locks don't cause any issue being there since only last cpu is invoking rpmh_flush() at present.

Adding support for other RSCs is in my to do list, and when that is being done we can re-evaluate and

remove if not required.

Quoted 10 lines

Specifically when reasoning about rpmh.c and rpmh-rsc.c I can only look at the code that's there now and decide whether it is race free or there are races. Back when I was analyzing the proposal to do rpmh_flush() all the time (not from PM code) it felt like there were a bunch of races, especially in the zero-active-TCS case. Most of the races go away when you assume that rpmh_flush() is only ever called from the PM code when nobody could be in the middle of an active transfer.

Quoted 2 lines

If we are ever planning to call rpmh_flush() from another place we need to re-look at all those races.

Sure. we can re-look all cases.

Quoted 3 lines

-Doug

Thanks,
Maulik

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