drivers/qcom: add RPMH communication support

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +12 -0

+               label = "apps_rsc";
+               compatible = "qcom,rpmh-rsc";
+               reg = <0x179c0000 0x10000>, <0x179d0000 0x10000>,
+                     <0x179e0000 0x10000>, <0xd00 0>;
+               reg-names = "drv-0", "drv-1", "drv-2", "tcs-offset";
+               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+               qcom,drv-id = <2>;
+               qcom,tcs-config = <SLEEP_TCS   3>,
+                                 <WAKE_TCS    3>,
+                                 <ACTIVE_TCS  2>,
+                                 <CONTROL_TCS 1>;
+       };

Hi,

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt line 99

Quoted 3 lines · Lina Iyer

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +3 -0

+TCS-OFFSET: 0xD00
+
+       apps_rsc: rsc@179e000 {

This label is wrong in two ways:

1. I believe it's supposed to be the first reg listed, so 179c0000

2. There's only 3 0's at the end.

Thus, it should be:

apps_rsc: rsc@179c0000

Quoted 12 lines

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +12 -0

+               label = "apps_rsc";
+               compatible = "qcom,rpmh-rsc";
+               reg = <0x179c0000 0x10000>, <0x179d0000 0x10000>,
+                     <0x179e0000 0x10000>, <0xd00 0>;
+               reg-names = "drv-0", "drv-1", "drv-2", "tcs-offset";
+               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+               qcom,drv-id = <2>;
+               qcom,tcs-config = <SLEEP_TCS   3>,
+                                 <WAKE_TCS    3>,
+                                 <ACTIVE_TCS  2>,
+                                 <CONTROL_TCS 1>;
+       };

Sending RPMH requests and waiting for response from the controller through a callback is common functionality across all platform drivers. To simplify drivers, add a library functions to create RPMH client and send resource state requests.

rpmh_write() is a synchronous blocking call that can be used to send active state requests.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- replace rpmh_client with device
- inline wait_for_tx_done()

Changes in v4:
- use const struct tcs_cmd in API
- remove wait count from this patch
- changed -EFAULT to -EINVAL

---
 drivers/soc/qcom/Makefile        |   4 +-
 drivers/soc/qcom/rpmh-internal.h |   6 ++
 drivers/soc/qcom/rpmh-rsc.c      |   8 ++
 drivers/soc/qcom/rpmh.c          | 168 +++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h          |  25 +++++
 5 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/rpmh.c
 create mode 100644 include/soc/qcom/rpmh.h

drivers/soc/qcom/Makefile +3 -1

index cb6300f6a8e9..bb395c3202ca 100644
@@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
 obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
-obj-$(CONFIG_QCOM_RPMH)	+=	rpmh-rsc.o
+obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
+qcom_rpmh-y			+= rpmh-rsc.o
+qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o

drivers/soc/qcom/rpmh-internal.h +2 -0

index cc29176f1303..d9a21726e568 100644
@@ -14,6 +14,7 @@
 #define MAX_CMDS_PER_TCS		16
 #define MAX_TCS_PER_TYPE		3
 #define MAX_TCS_NR			(MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+#define RPMH_MAX_CTRLR			2
 
 struct rsc_drv;
 

@@ -52,6 +53,7 @@ struct tcs_group {
  * @tcs:        TCS groups
  * @tcs_in_use: s/w state of the TCS
  * @lock:       synchronize state of the controller
+ * @list:       element in list of drv
  */
 struct rsc_drv {
 	const char *name;

@@ -61,9 +63,13 @@ struct rsc_drv {
 	struct tcs_group tcs[TCS_TYPE_NR];

drivers/soc/qcom/rpmh-internal.h +13 -0

+static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
+{
+	int i;
+	struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
+	struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
+
+	if (!drv)
+		return ctrlr;
+
+	for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+		if (rpmh_rsc[i].drv == drv) {
+			ctrlr = &rpmh_rsc[i];
+			return ctrlr;

+/**
+ * rpmh_write: Write a set of RPMH commands and block until response
+ *
+ * @rc: The RPMh handle got from rpmh_get_client

Hi Lina,

Quoted 73 lines · Lina Iyer

Sending RPMH requests and waiting for response from the controller through a callback is common functionality across all platform drivers. To simplify drivers, add a library functions to create RPMH client and send resource state requests.

rpmh_write() is a synchronous blocking call that can be used to send active state requests.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- replace rpmh_client with device
- inline wait_for_tx_done()

Changes in v4:
- use const struct tcs_cmd in API
- remove wait count from this patch
- changed -EFAULT to -EINVAL

---
 drivers/soc/qcom/Makefile        |   4 +-
 drivers/soc/qcom/rpmh-internal.h |   6 ++
 drivers/soc/qcom/rpmh-rsc.c      |   8 ++
 drivers/soc/qcom/rpmh.c          | 168 +++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h          |  25 +++++
 5 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/rpmh.c
 create mode 100644 include/soc/qcom/rpmh.h

drivers/soc/qcom/Makefile +3 -1

index cb6300f6a8e9..bb395c3202ca 100644
@@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
 obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
-obj-$(CONFIG_QCOM_RPMH)	+=	rpmh-rsc.o
+obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
+qcom_rpmh-y			+= rpmh-rsc.o
+qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o

drivers/soc/qcom/rpmh-internal.h +4 -0

index cc29176f1303..d9a21726e568 100644
@@ -14,6 +14,7 @@
 #define MAX_CMDS_PER_TCS		16
 #define MAX_TCS_PER_TYPE		3
 #define MAX_TCS_NR			(MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+#define RPMH_MAX_CTRLR			2
 
 struct rsc_drv;
 

@@ -52,6 +53,7 @@ struct tcs_group {
  * @tcs:        TCS groups
  * @tcs_in_use: s/w state of the TCS
  * @lock:       synchronize state of the controller
+ * @list:       element in list of drv
  */
 struct rsc_drv {
 	const char *name;

@@ -61,9 +63,13 @@ struct rsc_drv {
 	struct tcs_group tcs[TCS_TYPE_NR];
 	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
 	spinlock_t lock;
+	struct list_head list;
 };
 
+extern struct list_head rsc_drv_list;
 

drivers/soc/qcom/rpmh-internal.h struct rsc_drv { · line 73

Quoted 3 lines

drivers/soc/qcom/rpmh-internal.h +1 -0

 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
 
+void rpmh_tx_done(const struct tcs_request *msg, int r);

                                                       ~

nit: consider using a more expressive name, like status, rc, res or err.

<snip>

Quoted 19 lines

drivers/soc/qcom/rpmh-internal.h +19 -0

+static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
+{
+	int i;
+	struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
+	struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
+
+	if (!drv)
+		return ctrlr;
+
+	for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+		if (rpmh_rsc[i].drv == drv) {
+			ctrlr = &rpmh_rsc[i];
+			return ctrlr;
+		}
+	}
+
+	list_for_each_entry(p, &rsc_drv_list, list) {
+		if (drv == p) {
+			for (i = 0; i < RPMH_MAX_CTRLR; i++) {

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (!rpmh_rsc[i].drv)
+					break;
+			}
+			rpmh_rsc[i].drv = drv;

There is a race condition, get_rpmh_ctrlr() could be executed simulatenously in different contexts, and select the same controller instance for different DRVs.

It's probably an unlikely case, but to be safe you'd have to do something like this:

retry:

for (i = 0; i < RPMH_MAX_CTRLR; i++) {
	if (!rpmh_rsc[i].drv)
		break;
}

spin_lock(&rpmh_rsc[i].lock);

if (!rpmh_rsc[i].drv) {
	rpmh_rsc[i].drv = drv;
	ctrlr = &rpmh_rsc[i];
} else {
	spin_unlock(&rpmh_rsc[i].lock);
	goto retry;
}

spin_unlock(&rpmh_rsc[i].lock);

The above code doesn't address another potential error case, where #DRV > RPMH_MAX_CTRLR. In this case we'd access memory beyond rpmh_rsc. This would be a configuration error, not sure if it's strictly necessary to handle it.

<snip>

Quoted 4 lines

+/**
+ * rpmh_write: Write a set of RPMH commands and block until response
+ *
+ * @rc: The RPMh handle got from rpmh_get_client

              ~~~~

nit: Other than this the driver consistently uses 'RPMH'

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

drivers/soc/qcom/rpmh-internal.h +13 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");
+				return -EINVAL;
+			} else if (j == len - 1) {
+				return i;
+			}
+		}
+	}
+
+	return -ENODATA;
+}

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+				return -EINVAL;
+			} else if (j == len - 1) {
+				return i;
+			}
+		}
+	}
+
+	return -ENODATA;
+}

Quoted 27 lines · Matthias Kaehlcke

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

Quoted 12 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Here, we are trying to find the first address from the request and its position 'i' in the cmd_cache.

Quoted 5 lines

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

We now check for the sequence using the iterator 'j' only after we have found 'i' (the beginning of our request).

I hope that helps clear the concern.

Quoted 48 lines · Lina Iyer

Quoted 27 lines · Matthias Kaehlcke

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

Quoted 12 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Here, we are trying to find the first address from the request and its position 'i' in the cmd_cache.

Quoted 5 lines

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

Quoted 8 lines

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

We now check for the sequence using the iterator 'j' only after we have found 'i' (the beginning of our request).

I hope that helps clear the concern.

It doesn't, maybe I'm just confused, the driver has a certain complexity and I don't claim to have a comprehensive understanding :)

If I understand correctly find_match() is used to find a sequence of commands of length 'len' in the command cache. If that is correct I would expect it to do the following:

1. iterate through the commands in the command cache and find a command that matches the first command in the sequence

2. verify that the (len - 1) subsequent commands match those in the sequence, otherwise bail out

If I'm not mistaken the current version of find_match() only checks that the first command exists. After that it happily increases the command index, but doesn't perform any checks (after finding the first command 'tcs->cmd_cache[i] != cmd[0].addr' remains false for the subsequent values of j). When j reaches (len - 1) the function returns the index of the first command in the cache, regardless of whether the other commands match or not.

Quoted 71 lines · Matthias Kaehlcke

Quoted 48 lines · Lina Iyer

Quoted 27 lines · Matthias Kaehlcke

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

Quoted 12 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Here, we are trying to find the first address from the request and its position 'i' in the cmd_cache.

Quoted 5 lines

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

Quoted 8 lines

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

We now check for the sequence using the iterator 'j' only after we have found 'i' (the beginning of our request).

I hope that helps clear the concern.

It doesn't, maybe I'm just confused, the driver has a certain complexity and I don't claim to have a comprehensive understanding :)

If I understand correctly find_match() is used to find a sequence of commands of length 'len' in the command cache. If that is correct I would expect it to do the following:

1. iterate through the commands in the command cache and find a command that matches the first command in the sequence

2. verify that the (len - 1) subsequent commands match those in the sequence, otherwise bail out

Quoted 8 lines

If I'm not mistaken the current version of find_match() only checks that the first command exists. After that it happily increases the command index, but doesn't perform any checks (after finding the first command 'tcs->cmd_cache[i] != cmd[0].addr' remains false for the subsequent values of j). When j reaches (len - 1) the function returns the index of the first command in the cache, regardless of whether the other commands match or not.

Did you miss the check inside the WARN?
WARN(tcs->cmd_cache[i + j] != cmd[j].addr,

Quoted 80 lines · Lina Iyer

Quoted 71 lines · Matthias Kaehlcke

Quoted 48 lines · Lina Iyer

Quoted 27 lines · Matthias Kaehlcke

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

Quoted 12 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Here, we are trying to find the first address from the request and its position 'i' in the cmd_cache.

Quoted 5 lines

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

Quoted 8 lines

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

We now check for the sequence using the iterator 'j' only after we have found 'i' (the beginning of our request).

I hope that helps clear the concern.

It doesn't, maybe I'm just confused, the driver has a certain complexity and I don't claim to have a comprehensive understanding :)

If I understand correctly find_match() is used to find a sequence of commands of length 'len' in the command cache. If that is correct I would expect it to do the following:

1. iterate through the commands in the command cache and find a command that matches the first command in the sequence

2. verify that the (len - 1) subsequent commands match those in the sequence, otherwise bail out

Quoted 8 lines

If I'm not mistaken the current version of find_match() only checks that the first command exists. After that it happily increases the command index, but doesn't perform any checks (after finding the first command 'tcs->cmd_cache[i] != cmd[0].addr' remains false for the subsequent values of j). When j reaches (len - 1) the function returns the index of the first command in the cache, regardless of whether the other commands match or not.

Quoted 2 lines

Did you miss the check inside the WARN?
WARN(tcs->cmd_cache[i + j] != cmd[j].addr,

My point is that this code is never reached, also regardless of the condition, the branch would always return -EINVAL.

for (j = 0; j < len; j++) {
	if (tcs->cmd_cache[i] != cmd[0].addr) {
		if (j == 0)
			break;
		WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
		     "Message does not match previous sequence.\n");
		return -EINVAL;
	} else if (j == len - 1) {
		return i;
	}
}

Let's single step through this, assuming the sequence of len=3 is in the cache:

1. j=0
2. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed (good, this is the first
command we are looking for)
3. (j == len - 1): false

4. j=1
5. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed
6. (j == len - 1): false

7. j=2
8. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed
9. (j == len - 1): true
=> return i

Am I getting something wrong here?

Quoted 119 lines · Matthias Kaehlcke

Quoted 80 lines · Lina Iyer

Quoted 71 lines · Matthias Kaehlcke

Quoted 48 lines · Lina Iyer

Quoted 27 lines · Matthias Kaehlcke

Quoted 23 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 120 +++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

drivers/soc/qcom/rpmh-internal.h +0 -0

index d9a21726e568..6e19fe458c31 100644

<snip>

Quoted 12 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 9 lines

drivers/soc/qcom/rpmh-internal.h +9 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i] != cmd[0].addr) {

Shouldn't the condition be 'tcs->cmd_cache[i + j] != cmd[j].addr'?

Here, we are trying to find the first address from the request and its position 'i' in the cmd_cache.

Quoted 5 lines

Otherwise the code below the following if branch will never be executed. Either the 'tcs->cmd_cache[i] != cmd[0].addr' branch isn't entered because the addresses match, or the addresses don't match and the inner loop is aborted after the first iteration.

Quoted 8 lines

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h

Quoted 4 lines

drivers/soc/qcom/rpmh-internal.h +4 -0

+				if (j == 0)
+					break;
+				WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
+				     "Message does not match previous sequence.\n");

We now check for the sequence using the iterator 'j' only after we have found 'i' (the beginning of our request).

I hope that helps clear the concern.

It doesn't, maybe I'm just confused, the driver has a certain complexity and I don't claim to have a comprehensive understanding :)

If I understand correctly find_match() is used to find a sequence of commands of length 'len' in the command cache. If that is correct I would expect it to do the following:

1. iterate through the commands in the command cache and find a command that matches the first command in the sequence

2. verify that the (len - 1) subsequent commands match those in the sequence, otherwise bail out

Quoted 8 lines

If I'm not mistaken the current version of find_match() only checks that the first command exists. After that it happily increases the command index, but doesn't perform any checks (after finding the first command 'tcs->cmd_cache[i] != cmd[0].addr' remains false for the subsequent values of j). When j reaches (len - 1) the function returns the index of the first command in the cache, regardless of whether the other commands match or not.

Quoted 2 lines

Did you miss the check inside the WARN?
WARN(tcs->cmd_cache[i + j] != cmd[j].addr,

My point is that this code is never reached, also regardless of the condition, the branch would always return -EINVAL.

for (j = 0; j < len; j++) {
	if (tcs->cmd_cache[i] != cmd[0].addr) {
		if (j == 0)
			break;
		WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
		     "Message does not match previous sequence.\n");
		return -EINVAL;
	} else if (j == len - 1) {
		return i;
	}
}

Let's single step through this, assuming the sequence of len=3 is in the cache:

1. j=0
2. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed (good, this is the first
command we are looking for)
3. (j == len - 1): false

4. j=1
5. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed
6. (j == len - 1): false

7. j=2
8. (tcs->cmd_cache[i] != cmd[0].addr): false
=> branch with WARN + EINVAL not executed
9. (j == len - 1): true
=> return i

Quoted 1 line

Am I getting something wrong here?

The for_each_set_bit() should increment the 'i' and we would attempt to compare the first address in the request with the next command in the TCS cache. If they don't match we repeat the process again. If it does, then we loop through 'j' to find if the sequence matches.

Did I miss something?

Quoted 8 lines

Quoted 1 line

Am I getting something wrong here?

The for_each_set_bit() should increment the 'i' and we would attempt to compare the first address in the request with the next command in the TCS cache. If they don't match we repeat the process again. If it does, then we loop through 'j' to find if the sequence matches.

Did I miss something?

One of us is clearly in need of more caffeine or ready for the weekend, it might be me :) Maybe another pair of eyeballs could help to resolve this deadlock ...

My single stepping above assumes that tcs->cmd_cache[i] matches cmd[0].addr, i.e. we either found the start of the sequence we are looking for or another sequence that starts with the same address. My claim is that the code returns i in either case, whether the subsequent addresses match or not.

Hi,

Quoted 11 lines · Matthias Kaehlcke

Quoted 8 lines

Quoted 1 line

Am I getting something wrong here?

The for_each_set_bit() should increment the 'i' and we would attempt to compare the first address in the request with the next command in the TCS cache. If they don't match we repeat the process again. If it does, then we loop through 'j' to find if the sequence matches.

Did I miss something?

One of us is clearly in need of more caffeine or ready for the weekend, it might be me :) Maybe another pair of eyeballs could help

Quoted 7 lines

to resolve this deadlock ...

My single stepping above assumes that tcs->cmd_cache[i] matches cmd[0].addr, i.e. we either found the start of the sequence we are looking for or another sequence that starts with the same address. My claim is that the code returns i in either case, whether the subsequent addresses match or not.

I haven't reviewed this patch in detail, but I attempted to be another pair of eyes here. Something is definitely wrong with the "for (j = 0; j < len; j++)" loop. I believe the code that's written right now is equivalent to this much shorter function:

(quoted diff) +13 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+                     int len)
+{
+       int i, j;
+
+       /* Check for already cached commands */
+       for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+               if (tcs->cmd_cache[i] == cmd[0].addr)
+                       return i;
+       }
+
+       return -ENODATA;
+}

Specifically the test "if (tcs->cmd_cache[i] != cmd[0].addr)" does not take "j" into account. Thus if it was false when "j == 0" it will continue to be false for "j == 1", "j == 2", etc. Eventually you'll hit the "else if (j == len - 1)" and return.

I believe that's what Matthias has been saying. I personally haven't looked at the rest of the patch to see how things out to be fixed, but I'm quite convinced that the function either has a bug or should be written as the shorter version I've written above.

Yes, this is incorrect in its current form. This is what it should be -

static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
int len)

{
       int i, j;

       /* Check for already cached commands */
       for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
               if (tcs->cmd_cache[i] != cmd[0].addr)
                       continue;
               for (j = 0; j < len; j++) {
                       WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
                            "Message does not match previous sequence.\n");
                       return -EINVAL;
               }

               if (j == len - 1)
                       return i;

Hi,

Quoted 5 lines · Lina Iyer

Yes, this is incorrect in its current form. This is what it should be -

static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
int len)

Quoted 12 lines

{
       int i, j;

       /* Check for already cached commands */
       for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
               if (tcs->cmd_cache[i] != cmd[0].addr)
                       continue;
               for (j = 0; j < len; j++) {
                       WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
                            "Message does not match previous sequence.\n");
                       return -EINVAL;
               }

If len > 0, won't the above always return -EINVAL?

Quoted 2 lines

               if (j == len - 1)
                       return i;

Care to explain how you could get here and the test "if (j == len - 1)" could be false? ;-P

Quoted 14 lines · Doug Anderson

Hi,

Quoted 11 lines · Matthias Kaehlcke

Quoted 8 lines

Quoted 1 line

Am I getting something wrong here?

The for_each_set_bit() should increment the 'i' and we would attempt to compare the first address in the request with the next command in the TCS cache. If they don't match we repeat the process again. If it does, then we loop through 'j' to find if the sequence matches.

Did I miss something?

One of us is clearly in need of more caffeine or ready for the weekend, it might be me :) Maybe another pair of eyeballs could help

I need them both. Sorry about the back and forth. I understand what the problem is. The code doesnt look right. I seem to have messed it up. Thanks Matthias for being patient and going through this.

Quoted 32 lines

Quoted 7 lines

to resolve this deadlock ...

My single stepping above assumes that tcs->cmd_cache[i] matches cmd[0].addr, i.e. we either found the start of the sequence we are looking for or another sequence that starts with the same address. My claim is that the code returns i in either case, whether the subsequent addresses match or not.

I haven't reviewed this patch in detail, but I attempted to be another pair of eyes here. Something is definitely wrong with the "for (j = 0; j < len; j++)" loop. I believe the code that's written right now is equivalent to this much shorter function:

(quoted diff) +13 -0

+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+                     int len)
+{
+       int i, j;
+
+       /* Check for already cached commands */
+       for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+               if (tcs->cmd_cache[i] == cmd[0].addr)
+                       return i;
+       }
+
+       return -ENODATA;
+}

Specifically the test "if (tcs->cmd_cache[i] != cmd[0].addr)" does not take "j" into account. Thus if it was false when "j == 0" it will continue to be false for "j == 1", "j == 2", etc. Eventually you'll hit the "else if (j == len - 1)" and return.

Quoted 5 lines

I believe that's what Matthias has been saying. I personally haven't looked at the rest of the patch to see how things out to be fixed, but I'm quite convinced that the function either has a bug or should be written as the shorter version I've written above.

Yes, this is incorrect in its current form. This is what it should be -

static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
int len)

{
       int i, j;

       /* Check for already cached commands */
       for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
               if (tcs->cmd_cache[i] != cmd[0].addr)
                       continue;

               for (j = 0; j < len; j++) {
                       WARN(tcs->cmd_cache[i + j] != cmd[j].addr,
                            "Message does not match previous sequence.\n");
                       return -EINVAL;
               }

               if (j == len - 1)
                       return i;
       }

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +12 -0

index d169f7da5422..170d4a10275d 100644
@@ -101,6 +101,51 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 
+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;
+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);

Quoted 36 lines · Lina Iyer

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +12 -0

index d169f7da5422..170d4a10275d 100644
@@ -101,6 +101,51 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 
+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;
+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 125

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+		return 0;
+	}
+
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			spin_unlock(&tcs->lock);
+			return -EAGAIN;
+		}
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
+		bitmap_zero(tcs->slots, MAX_TCS_SLOTS);

Shouldn't this only clear the slots corresponding to 'm'?

If resetting the entire bitmap is really intended it could be done once after the loop, though the bitmap might be left in an inconsistent state if a TCS is still in use.

Quoted 50 lines · Matthias Kaehlcke

Quoted 36 lines · Lina Iyer

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +12 -0

index d169f7da5422..170d4a10275d 100644
@@ -101,6 +101,51 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 
+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;
+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 125

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+		return 0;
+	}
+
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			spin_unlock(&tcs->lock);
+			return -EAGAIN;
+		}
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
+		bitmap_zero(tcs->slots, MAX_TCS_SLOTS);

Shouldn't this only clear the slots corresponding to 'm'?

Quoted 3 lines

If resetting the entire bitmap is really intended it could be done once after the loop, though the bitmap might be left in an inconsistent state if a TCS is still in use.

The TCS will not be in use, the check tcs_is_free() will ensure that. I will move the bitmap_zero() outside the for().

Platform drivers need make a lot of resource state requests at the same time, say, at the start or end of an usecase. It can be quite inefficient to send each request separately. Instead they can give the RPMH library a batch of requests to be sent and wait on the whole transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple RPMH command sets. Each RPMH command set is set asynchronously and the API blocks until all the command sets are complete and receive their tx_done callbacks.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- replace rpmh_client with device *
Changes in v4:
- reorganize rpmh_write_batch()
- introduce wait_count here, instead of patch#4

---
 drivers/soc/qcom/rpmh.c | 155 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 161 insertions(+), 2 deletions(-)

drivers/soc/qcom/rpmh.c +6 -0

index b56445a05e23..c5962c692aac 100644
@@ -21,6 +21,7 @@
 #include "rpmh-internal.h"
 
 #define RPMH_TIMEOUT_MS			msecs_to_jiffies(10000)
+#define RPMH_MAX_REQ_IN_BATCH		10
 
 #define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name)	\
 	struct rpmh_request name = {			\

@@ -34,6 +35,7 @@
 		.completion = q,			\
 		.dev = dev,				\
 		.free = NULL,				\
+		.wait_count = NULL,			\
 	}
 
 /**

@@ -60,6 +62,7 @@ struct cache_req {
  * @dev: the device making the request
  * @err: err return from the controller
  * @free: the request object to be freed at tx_done
+ * @wait_count: count of waiters for this completion
  */
 struct rpmh_request {
 	struct tcs_request msg;

@@ -68,6 +71,7 @@ struct rpmh_request {
 	const struct device *dev;
 	int err;
 	struct rpmh_request *free;
+	atomic_t *wait_count;
 };
 
 /**

@@ -77,12 +81,14 @@ struct rpmh_request {
  * @cache: the list of cached requests
  * @lock: synchronize access to the controller data
  * @dirty: was the cache updated since flush
+ * @batch_cache: Cache sleep and wake requests sent as batch
  */
 struct rpmh_ctrlr {
 	struct rsc_drv *drv;
 	struct list_head cache;
 	spinlock_t lock;
 	bool dirty;
+	const struct rpmh_request *batch_cache[2 * RPMH_MAX_REQ_IN_BATCH];
 };
 

drivers/soc/qcom/rpmh.c +4 -0

+	for (i = 0; i < index; i++) {
+		kfree(ctrlr->batch_cache[i]->free);
+		ctrlr->batch_cache[i] = NULL;
+	}

+/**
+ * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
+ * batch to finish.
+ *
+ * @dev: the device making the request
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The array of count of elements in each batch, 0 terminated.

+ * Write a request to the RSC controller without caching. If the request
+ * state is ACTIVE, then the requests are treated as completion request
+ * and sent to the controller immediately. The function waits until all the
+ * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
+ * request is sent as fire-n-forget and no ack is expected.
+ *
+ * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
+ */

drivers/soc/qcom/rpmh.c +56 -0

+int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 *n)
+{
+	struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL };
+	DECLARE_COMPLETION_ONSTACK(compl);
+	atomic_t wait_count = ATOMIC_INIT(0);
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	int count = 0;
+	int ret, i;
+
+	if (IS_ERR(ctrlr) || !cmd || !n)
+		return -EINVAL;
+
+	while (n[count++] > 0)
+		;
+	count--;
+	if (!count || count > RPMH_MAX_REQ_IN_BATCH)
+		return -EINVAL;
+
+	for (i = 0; i < count; i++) {
+		rpm_msg[i] = __get_rpmh_msg_async(state, cmd, n[i]);
+		if (IS_ERR_OR_NULL(rpm_msg[i])) {
+			ret = PTR_ERR(rpm_msg[i]);
+			for (; i >= 0; i--)
+				kfree(rpm_msg[i]->free);
+			return ret;
+		}
+		cmd += n[i];
+	}
+
+	if (state != RPMH_ACTIVE_ONLY_STATE)
+		return cache_batch(ctrlr, rpm_msg, count);
+
+	atomic_set(&wait_count, count);
+
+	for (i = 0; i < count; i++) {
+		rpm_msg[i]->completion = &compl;
+		rpm_msg[i]->wait_count = &wait_count;
+		ret = rpmh_rsc_send_data(ctrlr->drv, &rpm_msg[i]->msg);
+		if (ret) {
+			int j;
+
+			pr_err("Error(%d) sending RPMH message addr=%#x\n",
+			       ret, rpm_msg[i]->msg.cmds[0].addr);
+			for (j = i; j < count; j++)
+				rpmh_tx_done(&rpm_msg[j]->msg, ret);
+			break;
+		}
+	}
+
+	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+
+}
+EXPORT_SYMBOL(rpmh_write_batch);
+
 static int is_req_valid(struct cache_req *req)
 {
 	return (req->sleep_val != UINT_MAX &&

Quoted 24 lines · Lina Iyer

Platform drivers need make a lot of resource state requests at the same time, say, at the start or end of an usecase. It can be quite inefficient to send each request separately. Instead they can give the RPMH library a batch of requests to be sent and wait on the whole transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple RPMH command sets. Each RPMH command set is set asynchronously and the API blocks until all the command sets are complete and receive their tx_done callbacks.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v6:
- replace rpmh_client with device *
Changes in v4:
- reorganize rpmh_write_batch()
- introduce wait_count here, instead of patch#4

---
 drivers/soc/qcom/rpmh.c | 155 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 161 insertions(+), 2 deletions(-)

<snip>

drivers/soc/qcom/rpmh.c int rpmh_write(const struct device *dev, enum rpmh_state state, · line 349

Quoted 11 lines

drivers/soc/qcom/rpmh.c +11 -0

+static int cache_batch(struct rpmh_ctrlr *ctrlr,
+		       struct rpmh_request **rpm_msg, int count)
+{
+	unsigned long flags;
+	int ret = 0;
+	int index = 0;
+	int i;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	while (ctrlr->batch_cache[index])
+		index++;

This will access memory beyond 'batch_cache' when the cache is full.

<snip>

drivers/soc/qcom/rpmh.c int rpmh_write(const struct device *dev, enum rpmh_state state, · line 391

Quoted 9 lines

drivers/soc/qcom/rpmh.c +9 -0

+static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
+{
+	unsigned long flags;
+	int index = 0;
+	int i;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	while (ctrlr->batch_cache[index])
+		index++;

Same as above. Also, why loop twice?

Quoted 4 lines

drivers/soc/qcom/rpmh.c +4 -0

+	for (i = 0; i < index; i++) {
+		kfree(ctrlr->batch_cache[i]->free);
+		ctrlr->batch_cache[i] = NULL;
+	}

<snip>

Quoted 8 lines

+/**
+ * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
+ * batch to finish.
+ *
+ * @dev: the device making the request
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The array of count of elements in each batch, 0 terminated.

nit: in this driver 'n' is usually associated with the command offset within a TCS. Since it isn't an overly descriptive name it may already cost the reader a while to commit that to his/her memory, and now we are overloading 'n' with a different meaning (I also noticed this in another patch of this series, but didn't comment).

drivers/soc/qcom/rpmh.c int rpmh_flush(const struct device *dev) · line 529

Quoted 2 lines

drivers/soc/qcom/rpmh.c +0 -0

 	/*
 	 * Nobody else should be calling this function other than system PM,,

                                                                           ~

Remove extra comma.

Some RSCs may only have sleep and wake TCS, i.e, there is no dedicated TCS for active mode request, but drivers may still want to make active requests from these RSCs. In such cases re-purpose the wake TCS to send active state requests.

The requirement for this is that the driver is aware that the wake TCS is being repurposed to send active request, hence the sleep and wake TCSes be invalidated before the active request is sent.

Signed-off-by Lina Iyer <[email protected]>