drivers/qcom: add RPMH communication support

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

+int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+       int ret;
+
+       if (!msg || !msg->cmds || !msg->num_cmds ||
+           msg->num_cmds > MAX_RPMH_PAYLOAD) {
+               WARN_ON(1);
+               return -EINVAL;
+       }
+
+       do {
+               ret = tcs_write(drv, msg);

Hi,

Quoted 12 lines · Lina Iyer

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

+int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+       int ret;
+
+       if (!msg || !msg->cmds || !msg->num_cmds ||
+           msg->num_cmds > MAX_RPMH_PAYLOAD) {
+               WARN_ON(1);
+               return -EINVAL;
+       }
+
+       do {
+               ret = tcs_write(drv, msg);

drivers/soc/qcom/rpmh-rsc.c line 341

Quoted 10 lines

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

+               if (ret == -EBUSY) {
+                       pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=%#x\n",
+                                           msg->cmds[0].addr);
+                       udelay(10);
+               }
+       } while (ret == -EBUSY);
+
+       return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_send_data);

Here and elsewhere in this series: why EXPORT_SYMBOL in this case? This is only exported to rpmh.c, right? You don't need EXPORT_SYMBOL for that. The Makefile puts rpmh.c and rpmh-rsc.c together in the same "qcom_rpmh.o", and then even further the KConfig lists this as bool so both are builtin to the kernel.

-Doug

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Log sent RPMH requests and interrupt responses in FTRACE.

Cc Steven Rostedt <[email protected]>

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

---

Changes in v7:
- varible name changes and white space

Changes in v6:
- struct tcs_response was removed. Fix in trace as well.
Changes in v4:
- fix compilation issues, use __assign_str
- use %#x instead of 0x%08x
Changes in v3:
- Use __string() instead of char *
- fix TRACE_INCLUDE_PATH
---

Quoted 1 line · On Wed, 9 May 2018 11:01:52 -0600 Lina Iyer <[email protected]> wrote:

Log sent RPMH requests and interrupt responses in FTRACE.

Has this changed since the last time I reviewed it? If not, please add
the: Reviewed-by: Steven Rostedt (VMware) <[email protected]> to you patch queue.

-- Steve

Quoted 17 lines

Cc Steven Rostedt <[email protected]>

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

---

Changes in v7:
- varible name changes and white space

Changes in v6:
- struct tcs_response was removed. Fix in trace as well.
Changes in v4:
- fix compilation issues, use __assign_str
- use %#x instead of 0x%08x
Changes in v3:
- Use __string() instead of char *
- fix TRACE_INCLUDE_PATH
---

drivers/soc/qcom/rpmh.c new file +8 -0

+int rpmh_write(const struct device *dev, enum rpmh_state state,
+              const struct tcs_cmd *cmd, u32 n)
+{
+       DECLARE_COMPLETION_ONSTACK(compl);
+       DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+       int ret;
+
+       if (!cmd || !n || n > MAX_RPMH_PAYLOAD)

Hi,

Quoted 8 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +8 -0

+int rpmh_write(const struct device *dev, enum rpmh_state state,
+              const struct tcs_cmd *cmd, u32 n)
+{
+       DECLARE_COMPLETION_ONSTACK(compl);
+       DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+       int ret;
+
+       if (!cmd || !n || n > MAX_RPMH_PAYLOAD)

drivers/soc/qcom/rpmh.c line 173

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+               return -EINVAL;
+
+       memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
+       rpm_msg.msg.num_cmds = n;
+
+       ret = __rpmh_write(dev, state, &rpm_msg);
+       if (ret)
+               return ret;
+
+       ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);

IMO it's almost never a good idea to use wait_for_completion_timeout() together with a completion that's declared on the stack. If you somehow insist that this is a good idea then I need to see incredibly clear and obvious code/comments that say why it's impossible that the process might somehow try to signal the completion _after_ RPMH_TIMEOUT_MS has expired.

Specifically if the timeout happens but the process could still signal a completion later then they will access random data on the stack of a function that has already returned. This causes ridiculously difficult-to-debug crashes.

NOTE: You've got timeout set to 10 seconds here. Is that really even useful? IMO just call wait_for_completion() without a timeout. It's much better to have a nice clean hang than a random stack corruption.

Quoted 38 lines · Doug Anderson

Hi,

Quoted 9 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +8 -0

+int rpmh_write(const struct device *dev, enum rpmh_state state,
+              const struct tcs_cmd *cmd, u32 n)
+{
+       DECLARE_COMPLETION_ONSTACK(compl);
+       DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+       int ret;
+
+       if (!cmd || !n || n > MAX_RPMH_PAYLOAD)

drivers/soc/qcom/rpmh.c line 173

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+               return -EINVAL;
+
+       memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
+       rpm_msg.msg.num_cmds = n;
+
+       ret = __rpmh_write(dev, state, &rpm_msg);
+       if (ret)
+               return ret;
+
+       ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);

IMO it's almost never a good idea to use wait_for_completion_timeout() together with a completion that's declared on the stack. If you somehow insist that this is a good idea then I need to see incredibly clear and obvious code/comments that say why it's impossible that the process might somehow try to signal the completion _after_ RPMH_TIMEOUT_MS has expired.

Specifically if the timeout happens but the process could still signal a completion later then they will access random data on the stack of a function that has already returned. This causes ridiculously difficult-to-debug crashes.

Quoted 5 lines

NOTE: You've got timeout set to 10 seconds here. Is that really even useful? IMO just call wait_for_completion() without a timeout. It's much better to have a nice clean hang than a random stack corruption.

The 10 sec timeout will guarantee that we will not get a response at all anymore for the request. Usually requests can be considered failed if there is no response in a few tens of microseconds. 10 sec is just an arbitarily large number.

The reason we use timeout is that once the timeout happens, we know we have failed, we could trigger a watchdog or crash the system. This is very important for our productization in debugging RPMH failures. A hang would not always trigger a watchdog and the failure would be silent and possibly fatal but hard to debug.

Hi,

Quoted 49 lines · Lina Iyer

Quoted 38 lines · Doug Anderson

Hi,

Quoted 9 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +8 -0

+int rpmh_write(const struct device *dev, enum rpmh_state state,
+              const struct tcs_cmd *cmd, u32 n)
+{
+       DECLARE_COMPLETION_ONSTACK(compl);
+       DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+       int ret;
+
+       if (!cmd || !n || n > MAX_RPMH_PAYLOAD)

drivers/soc/qcom/rpmh.c line 173

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+               return -EINVAL;
+
+       memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
+       rpm_msg.msg.num_cmds = n;
+
+       ret = __rpmh_write(dev, state, &rpm_msg);
+       if (ret)
+               return ret;
+
+       ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);

IMO it's almost never a good idea to use wait_for_completion_timeout() together with a completion that's declared on the stack. If you somehow insist that this is a good idea then I need to see incredibly clear and obvious code/comments that say why it's impossible that the process might somehow try to signal the completion _after_ RPMH_TIMEOUT_MS has expired.

Specifically if the timeout happens but the process could still signal a completion later then they will access random data on the stack of a function that has already returned. This causes ridiculously difficult-to-debug crashes.

Quoted 5 lines

NOTE: You've got timeout set to 10 seconds here. Is that really even useful? IMO just call wait_for_completion() without a timeout. It's much better to have a nice clean hang than a random stack corruption.

The 10 sec timeout will guarantee that we will not get a response at all anymore for the request. Usually requests can be considered failed if there is no response in a few tens of microseconds. 10 sec is just an arbitarily large number.

Quoted 5 lines

The reason we use timeout is that once the timeout happens, we know we have failed, we could trigger a watchdog or crash the system. This is very important for our productization in debugging RPMH failures. A hang would not always trigger a watchdog and the failure would be silent and possibly fatal but hard to debug.

If you intend the system to crash when this timeout happens then IMHO add a BUG_ON. Then I won't worry about something coming around later and clobbering the stack.

-Doug

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-rsc.c +1 -0

index c0edf3850147..b5894b001ae1 100644
 
<snip>
 
+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,

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

+static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
+		      int *tcs_id, int *cmd_id)
+{
+	int slot, offset;
+	int i = 0;
+
+	/* Find if we already have the msg in our TCS */

Hi Lina,

Quoted 24 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-rsc.c +0 -0

index c0edf3850147..b5894b001ae1 100644
 
<snip>

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

Quoted 11 lines

drivers/soc/qcom/rpmh-rsc.c +11 -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)
+			continue;
+		if (i + len >= MAX_TCS_SLOTS)
+			goto seq_err;

The command cache can have less than MAX_TCS_SLOTS slot:

static int rpmh_probe_tcs_config(struct platform_device *pdev,
struct rsc_drv *drv)

{
	...
	tcs->cmd_cache = devm_kcalloc(&pdev->dev,
				      tcs->num_tcs * ncpt, sizeof(u32),
				      GFP_KERNEL);
	...
}

So the condition needs to be:

if (i + len >= tcs->num_tcs * tcs->ncpt)

Quoted 7 lines

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

+static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
+		      int *tcs_id, int *cmd_id)
+{
+	int slot, offset;
+	int i = 0;
+
+	/* Find if we already have the msg in our TCS */

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

Quoted 10 lines

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

+	slot = find_match(tcs, msg->cmds, msg->num_cmds);
+	if (slot >= 0)
+		goto copy_data;
+
+	/* Do over, until we can fit the full payload in a TCS */
+	do {
+		slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
+						  i, msg->num_cmds, 0);
+		if (slot == MAX_TCS_SLOTS)
+			return -ENOMEM;

Like above, use 'tcs->num_tcs * tcs->ncpt' as maximum instead of MAX_TCS_SLOTS.

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

Quoted 13 lines

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

+static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+	struct tcs_group *tcs;
+	int tcs_id = 0, cmd_id = 0;
+	unsigned long flags;
+	int ret;
+
+	tcs = get_tcs_for_msg(drv, msg);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock_irqsave(&tcs->lock, flags);
+	/* find the m-th TCS and the n-th position in the TCS to write to */

The comment still refers to the old names 'm' and 'n'.

Hi,

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 75

Quoted 9 lines · Lina Iyer

/**
* struct rpmh_request: the message to be sent to rpmh-rsc *

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

@@ -54,9 +71,15 @@ struct rpmh_request {
   * struct rpmh_ctrlr: our representation of the controller
   *
   * @drv: the controller instance
+ * @cache: the list of cached requests
+ * @lock: synchronize access to the controller data

nit: this makes it sound like this lock will be grabbed for all calls into rpmh-rsc. In fact, it only protects access to the cache. Ideally name it cache_lock and document that it's for protecting the cache.

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

Quoted 9 lines

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

+/**
+ * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ *
+ * @dev: The device making the request
+ *
+ * Return: -EBUSY if the controller is busy, probably waiting on a response
+ * to a RPMH request sent earlier.
+ *
+ * This function is generally called from the sleep code from the last CPU

"is generally" implies that sometimes it's not called from the sleep code. Change to "is always". If "is generally" is more correct, you can't run lockless right?

-Doug

Platform drivers that want to send a request but do not want to block until the RPMH request completes have now a new API - rpmh_write_async().

The API allocates memory and send the requests and returns the control back to the platform driver. The tx_done callback from the controller is handled in the context of the controller's thread and frees the allocated memory. This API allows RPMH requests from atomic contexts as well.

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

Hi,

drivers/soc/qcom/rpmh.c void rpmh_tx_done(const struct tcs_request *msg, int r) · line 144

Quoted 7 lines · Lina Iyer

/**

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

@@ -137,6 +140,8 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
                 dev_err(rpm_msg->dev, "RPMH TX fail in msg addr=%#x, err=%d\n",
                         rpm_msg->msg.cmds[0].addr, r);
 
+       kfree(rpm_msg->free);
+

The way the code is written makes it seem like you could free memory _and_ have a completion but you can't. Specifically:

* The only plausible thing that "rpm_msg->free" could point to is "rpm_msg".

* The complete(compl) would then be accessing freed memory.

I believe the kfree() should be at the end of the function. Personally I'd make it more obvious that this is just a boolean value and change to:

if (rpm_msg->needs_free)
kgree(rpm_msg)

...then the reader of the code doesn't need to go figure out what you're trying to free.

-Doug

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 v7:
- Check for loop out of bounds

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 | 154 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 160 insertions(+), 2 deletions(-)

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

index 1bb62876795c..a0e277b4b846 100644
 
...
 

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

+		rpm_msg = ctrlr->batch_cache[i];
+		ret = rpmh_rsc_write_ctrl_data(ctrlr->drv, &rpm_msg->msg);
+		if (ret)
+			break;
+	}
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+

Quoted 34 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 v7:
- Check for loop out of bounds

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 | 154 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 160 insertions(+), 2 deletions(-)

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

index 1bb62876795c..a0e277b4b846 100644
 
...
 

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

Quoted 10 lines

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

+static int flush_batch(struct rpmh_ctrlr *ctrlr)
+{
+	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->lock, flags);
+	for (i = 0; ctrlr->batch_cache[i]; i++) {

I missed this earlier: the loop goes beyond ctrlr->batch_cache when the batch cache is full.

Quoted 12 lines

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

+		rpm_msg = ctrlr->batch_cache[i];
+		ret = rpmh_rsc_write_ctrl_data(ctrlr->drv, &rpm_msg->msg);
+		if (ret)
+			break;
+	}
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+	return ret;
+}
+
+static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
+{

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

Quoted 5 lines

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

+	unsigned long flags;
+	int index = 0;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	while (ctrlr->batch_cache[index]) {

This still goes beyond ctrlr->batch_cache when the batch cache is full.

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

 };
 
 static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];

@@ -133,6 +140,7 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
        struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
                                                    msg);
        struct completion *compl = rpm_msg->completion;
+       atomic_t *wc = rpm_msg->wait_count;
 
        rpm_msg->err = r;
 

drivers/soc/qcom/rpmh.c +9 -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);

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

+/**
+ * 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.
+ */
+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;

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

+
+       atomic_set(&wait_count, count);
+
+       for (i = 0; i < count; i++) {

+ break;

Hi,

Quoted 9 lines · Lina Iyer

/**

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

@@ -77,12 +82,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;

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 4 lines

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

        struct list_head cache;
        spinlock_t lock;
        bool dirty;
+       const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Quoted 12 lines

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

 };
 
 static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];

@@ -133,6 +140,7 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
        struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
                                                    msg);
        struct completion *compl = rpm_msg->completion;
+       atomic_t *wc = rpm_msg->wait_count;
 
        rpm_msg->err = r;
 

drivers/soc/qcom/rpmh.c void rpmh_tx_done(const struct tcs_request *msg, int r) · line 155

Quoted 7 lines

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

@@ -143,8 +151,13 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
        kfree(rpm_msg->free);
 
        /* Signal the blocking thread we are done */
-       if (compl)
-               complete(compl);
+       if (!compl)
+               return;

The comment above this "if" block no longer applies to the line next to it after your patch. ...but below I suggest you get rid of "wait_count", so maybe this part of the patch will go away.

Quoted 9 lines

drivers/soc/qcom/rpmh.c +9 -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);

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

Quoted 10 lines

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

+       while (index < RPMH_MAX_BATCH_CACHE && ctrlr->batch_cache[index])
+               index++;
+       if (index + count >= RPMH_MAX_BATCH_CACHE) {
+               ret = -ENOMEM;
+               goto fail;
+       }
+
+       for (i = 0; i < count; i++)
+               ctrlr->batch_cache[index + i] = rpm_msg[i];
+fail:

Nit: this label is for both failure and normal exit, so call it "exit".

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 29 lines

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

+/**
+ * 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.
+ */
+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;

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

Quoted 10 lines

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

+
+       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])) {

Just "IS_ERR". It's never NULL.

...also add a i-- somewhere in here or you're going to be kfree()ing your error value, aren't you?

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

Quoted 10 lines

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

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

Don't you need to free rpm_msg items in this case?

Quoted 4 lines

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

+
+       atomic_set(&wait_count, count);
+
+       for (i = 0; i < count; i++) {

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

Quoted 10 lines

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

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

You're just using rpmh_tx_done() to free memory? Note that you'll probably do your error handling in this function a favor if you rename __get_rpmh_msg_async() to __fill_rpmh_msg() and remove the memory allocation from there. Then you can do one big allocation of the whole array in rpmh_write_batch() and then you'll only have one free at the end...

Quoted 1 line

+ break;

"break" seems wrong here. You'll end up waiting for the completion, then I guess timing out, then returning -ETIMEDOUT?

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

Quoted 4 lines

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

+               }
+       }
+
+       ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);

The "wait_count" abstraction is confusing and I believe it's not needed. I think you can remove it and change the above to this (untested) code:

time_left = RPMH_TIMEOUT_MS;
for (i = 0; i < count; i++) {
time_left = wait_for_completion_timeout(&compl, time_left); if (!time_left)
return -ETIMEDOUT;
}

...specifically completions are additive, so just wait "count" times and then the reader doesn't need to learn your new wait_count abstraction and try to reason about it.

...and, actually, I argue in other replies that this should't use a timeout, so even cleaner:

for (i = 0; i < count; i++)
wait_for_completion(&compl);

Once you do that, you can also get rid of the need to pre-count "n", so all your loops turn into:

for (i = 0; n[i]; i++)

I suppose you might want to get rid of "RPMH_MAX_REQ_IN_BATCH" and dynamically allocate your array too, but that seems sane. As per above it seems like you should just dynamically allocate a whole array of "struct rpmh_request" items at once anyway.

---

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

Quoted 4 lines

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

+       return (ret > 0) ? 0 : -ETIMEDOUT;
+
+}
+EXPORT_SYMBOL(rpmh_write_batch);

Perhaps an even simpler thing than taking all my advice above: can't you just add a optional completion to rpmh_write_async()? That would just be stuffed into rpm_msg.

Now your batch code would just be a bunch of calls to rpmh_write_async() with an equal number of wait_for_completion() calls at the end. Is there a reason that wouldn't work? You'd get rid of _a lot_ of code.

-Doug

Quoted 10 lines

Quoted 10 lines

/**

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

@@ -77,12 +82,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;

Quoted 9 lines

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 3 lines

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

        spinlock_t lock;
        bool dirty;
+       const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Due to a hardware limitation, requests made by bus drivers must be set up in the sleep and wake TCS first before setting up the requests from other drivers. Bus drivers use batch mode for any and all RPMH communication. Hence their request are the only ones in the batch_cache.

Quoted 6 lines

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

Quoted 8 lines

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

flush_batch does write to the controller using
rpmh_rsc_write_ctrl_data()

Hi,

Quoted 19 lines · Lina Iyer

Quoted 10 lines

Quoted 10 lines

/**

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

@@ -77,12 +82,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;

Quoted 9 lines

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 3 lines

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

        spinlock_t lock;
        bool dirty;
+       const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Quoted 4 lines

Due to a hardware limitation, requests made by bus drivers must be set up in the sleep and wake TCS first before setting up the requests from other drivers. Bus drivers use batch mode for any and all RPMH communication. Hence their request are the only ones in the batch_cache.

This is totally not obvious and not commented. Why not rename to "priority" instead of "batch"?

If the only requirement is that these come first, that's still no reason to use totally separate code paths and mechanisms. These requests can still use the same data structures / functions and just be ordered to come first, can't they? ...or given a boolean "priority" and you can do two passes through your queue: one to do the priority ones and one to do the secondary ones.

Quoted 14 lines

Quoted 6 lines

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

Quoted 8 lines

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 2 lines

flush_batch does write to the controller using
rpmh_rsc_write_ctrl_data()

My confusion is that they happen at different times. As I understand it:

* For the normal case, you immediately calls rpmh_rsc_write_ctrl_data() and then later do the rest of the work.

* For the batch case, you call both later.

Is there a good reason for this, or is it just an arbitrary difference? If there's a good reason, it should be documented.

Quoted 34 lines · Doug Anderson

Hi,

Quoted 20 lines · Lina Iyer

Quoted 11 lines

Quoted 11 lines

/**

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

@@ -77,12 +82,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;

Quoted 9 lines

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 2 lines

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

        bool dirty;
+       const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Quoted 4 lines

Due to a hardware limitation, requests made by bus drivers must be set up in the sleep and wake TCS first before setting up the requests from other drivers. Bus drivers use batch mode for any and all RPMH communication. Hence their request are the only ones in the batch_cache.

This is totally not obvious and not commented. Why not rename to "priority" instead of "batch"?

Quoted 8 lines

If the only requirement is that these come first, that's still no reason to use totally separate code paths and mechanisms. These requests can still use the same data structures / functions and just be ordered to come first, can't they? ...or given a boolean "priority" and you can do two passes through your queue: one to do the priority ones and one to do the secondary ones.

The bus requests have a certain order and cannot be mutated by the RPMH driver. It has to be maintained in the TCS in the same order.

Also, the bus requests have quite a churn during the course of an usecase. They are setup and invalidated often.
It is faster to have them separate and invalidate the whole lot of the batch_cache instead of intertwining them with requests from other drivers and then figuring out which all must be invalidated and rebuilt when the next batch requests comes in. Remember, that requests may come from any driver any time and therefore will be mangled if we use the same data structure. So to be faster and to avoid having mangled requests in the TCS, I have kept the data structures separate.

Quoted 26 lines

Quoted 15 lines

Quoted 7 lines

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

Quoted 8 lines

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 2 lines

flush_batch does write to the controller using
rpmh_rsc_write_ctrl_data()

My confusion is that they happen at different times. As I understand it:

* For the normal case, you immediately calls rpmh_rsc_write_ctrl_data() and then later do the rest of the work.

* For the batch case, you call both later.

Quoted 4 lines

Is there a good reason for this, or is it just an arbitrary difference? If there's a good reason, it should be documented.

In both the cases, the requests are cached in the rpmh library and are only sent to the controller only when the flushed. I am not sure the work is any different. The rpmh_flush() flushes out batch requests and then the requests from other drivers.

Hi,

Quoted 43 lines · Lina Iyer

Quoted 42 lines · Doug Anderson

Hi,

Quoted 20 lines · Lina Iyer

Quoted 11 lines

Quoted 11 lines

/**

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

@@ -77,12 +82,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;

Quoted 9 lines

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 2 lines

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

        bool dirty;
+       const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Quoted 4 lines

Due to a hardware limitation, requests made by bus drivers must be set up in the sleep and wake TCS first before setting up the requests from other drivers. Bus drivers use batch mode for any and all RPMH communication. Hence their request are the only ones in the batch_cache.

This is totally not obvious and not commented. Why not rename to "priority" instead of "batch"?

If the only requirement is that these come first, that's still no reason to use totally separate code paths and mechanisms. These requests can still use the same data structures / functions and just be ordered to come first, can't they? ...or given a boolean "priority" and you can do two passes through your queue: one to do the priority ones and one to do the secondary ones.

Quoted 2 lines

The bus requests have a certain order and cannot be mutated by the RPMH driver. It has to be maintained in the TCS in the same order.

Please document this requirement in the code.

Quoted 9 lines

Also, the bus requests have quite a churn during the course of an usecase. They are setup and invalidated often.
It is faster to have them separate and invalidate the whole lot of the batch_cache instead of intertwining them with requests from other drivers and then figuring out which all must be invalidated and rebuilt when the next batch requests comes in. Remember, that requests may come from any driver any time and therefore will be mangled if we use the same data structure. So to be faster and to avoid having mangled requests in the TCS, I have kept the data structures separate.

If you need to find a certain group of requests then can't you just tag them and it's easy to find them? I'm still not seeing the need for a whole separate code path and data structure.

Quoted 26 lines

Quoted 26 lines

Quoted 15 lines

Quoted 7 lines

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

Quoted 8 lines

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 2 lines

flush_batch does write to the controller using
rpmh_rsc_write_ctrl_data()

My confusion is that they happen at different times. As I understand it:

* For the normal case, you immediately calls rpmh_rsc_write_ctrl_data() and then later do the rest of the work.

* For the batch case, you call both later.

Quoted 8 lines

Quoted 4 lines

Is there a good reason for this, or is it just an arbitrary difference? If there's a good reason, it should be documented.

In both the cases, the requests are cached in the rpmh library and are only sent to the controller only when the flushed. I am not sure the work is any different. The rpmh_flush() flushes out batch requests and then the requests from other drivers.

OK then, here are the code paths I see:

rpmh_write
=> __rpmh_write
=> cache_rpm_request()
=> (state != RPMH_ACTIVE_ONLY_STATE): rpmh_rsc_send_data()

rpmh_write_batch
=> (state != RPMH_ACTIVE_ONLY_STATE): cache_batch()
=> No call to rpmh_rsc_send_data()

Said another way:

* if you call rpmh_write() for something you're going to defer you will still call cache_rpm_request() _before_ rpmh_write() returns.

* if you call rpmh_write_batch() for something you're going to defer then you _don't_ call cache_rpm_request() before rpmh_write_batch() returns.

Do you find a fault in my analysis of the current code? If you see a fault then please point it out. If you don't see a fault, please say why the behaviors are different. I certainly understand that eventually you will call cache_rpm_request() for both cases. It's just that in one case the call happens right away and the other case it is deferred.

Quoted 11 lines

Quoted 9 lines

Also, the bus requests have quite a churn during the course of an usecase. They are setup and invalidated often.
It is faster to have them separate and invalidate the whole lot of the batch_cache instead of intertwining them with requests from other drivers and then figuring out which all must be invalidated and rebuilt when the next batch requests comes in. Remember, that requests may come from any driver any time and therefore will be mangled if we use the same data structure. So to be faster and to avoid having mangled requests in the TCS, I have kept the data structures separate.

Quoted 4 lines

If you need to find a certain group of requests then can't you just tag them and it's easy to find them? I'm still not seeing the need for a whole separate code path and data structure.

Could be done but it will be slower and involve a lot more code than separate data structures.

Quoted 61 lines

Quoted 29 lines

Quoted 29 lines

Quoted 16 lines

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

Quoted 8 lines

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 2 lines

flush_batch does write to the controller using
rpmh_rsc_write_ctrl_data()

My confusion is that they happen at different times. As I understand it:

* For the normal case, you immediately calls rpmh_rsc_write_ctrl_data() and then later do the rest of the work.

* For the batch case, you call both later.

Quoted 8 lines

Quoted 4 lines

Is there a good reason for this, or is it just an arbitrary difference? If there's a good reason, it should be documented.

In both the cases, the requests are cached in the rpmh library and are only sent to the controller only when the flushed. I am not sure the work is any different. The rpmh_flush() flushes out batch requests and then the requests from other drivers.

OK then, here are the code paths I see:

rpmh_write
=> __rpmh_write
=> cache_rpm_request()
=> (state != RPMH_ACTIVE_ONLY_STATE): rpmh_rsc_send_data()

rpmh_write_batch
=> (state != RPMH_ACTIVE_ONLY_STATE): cache_batch()
=> No call to rpmh_rsc_send_data()

Said another way:

* if you call rpmh_write() for something you're going to defer you will still call cache_rpm_request() _before_ rpmh_write() returns.

* if you call rpmh_write_batch() for something you're going to defer then you _don't_ call cache_rpm_request() before rpmh_write_batch() returns.

Quoted 6 lines

Do you find a fault in my analysis of the current code? If you see a fault then please point it out. If you don't see a fault, please say why the behaviors are different. I certainly understand that eventually you will call cache_rpm_request() for both cases. It's just that in one case the call happens right away and the other case it is deferred.

True. I see where your confusion is. It is because of an optimization and our existential need for saving power at every opportunity.

For rpmh_write path -
The reason being for the disparity is that, we can vote for a system low power mode (modes where we flush the caches and put the entire silicon is a low power state) at any point when all the CPUs are idle. If a driver has updated its vote for a system resource, it needs to be updated in the cache and thats what cache_rpm_request() does. Its possible that we may enter a system low power mode immediately after that. The rpmh_flush() would be called by the idle driver and we would flush the cached values and enter the idle state. By writing immediately to the TCS, we avoid increasing the latency of entering an idle state.

For the rpmh_write_batch() path -
The Bus driver would invalidate the TCS and the batch_cache. The use case fills up the batch_cache with values as needed by the use case. While during the usecase, the CPUs can go idle and the idle drivers would call rpmh_flush(). At that time, we would write the batch_cache into the already invalidated TCSes and then write the rest of the cached requests from ->cache. We then enter the low power modes.

The optimization of writing the sleep/wake votes in the same context of the driver making the request, helps us avoid writing some extra registers in the critical idle path.

Hi,

drivers/soc/qcom/rpmh.c struct rpmh_request { · line 92

Quoted 13 lines · Lina Iyer

/**

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

@@ -77,12 +82,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[RPMH_MAX_BATCH_CACHE];

I'm pretty confused about why the "batch_cache" is separate from the normal cache. As far as I can tell the purpose of the two is the same but you have two totally separate code paths and data structures.

Quoted 12 lines

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

  };
 
  static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];

@@ -133,6 +140,7 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
         struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
                                                     msg);
         struct completion *compl = rpm_msg->completion;
+       atomic_t *wc = rpm_msg->wait_count;
 
         rpm_msg->err = r;
 

drivers/soc/qcom/rpmh.c void rpmh_tx_done(const struct tcs_request *msg, int r) · line 155

Quoted 7 lines

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

@@ -143,8 +151,13 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
         kfree(rpm_msg->free);
 
         /* Signal the blocking thread we are done */
-       if (compl)
-               complete(compl);
+       if (!compl)
+               return;

The comment above this "if" block no longer applies to the line next to it after your patch. ...but below I suggest you get rid of "wait_count", so maybe this part of the patch will go away.

Quoted 9 lines

drivers/soc/qcom/rpmh.c +9 -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);

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

Quoted 10 lines

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

+       while (index < RPMH_MAX_BATCH_CACHE && ctrlr->batch_cache[index])
+               index++;
+       if (index + count >= RPMH_MAX_BATCH_CACHE) {
+               ret = -ENOMEM;
+               goto fail;
+       }
+
+       for (i = 0; i < count; i++)
+               ctrlr->batch_cache[index + i] = rpm_msg[i];
+fail:

Nit: this label is for both failure and normal exit, so call it "exit".

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

Quoted 4 lines

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

+       spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+       return ret;
+}

As part of my overall confusion about why the batch cache is different than the normal one: for the normal use case you still call rpmh_rsc_write_ctrl_data() for things you put in your cache, but you don't for the batch cache. I still haven't totally figured out what rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't do it for the batch cache but you do for the other one.

Quoted 35 lines

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

+/**
+ * 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.
+ */
+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;

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

Quoted 4 lines

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

+
+       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])) {

Just "IS_ERR". It's never NULL.
...also add a i-- somewhere in here or you're going to be kfree()ing your error value, aren't you?

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

Quoted 12 lines

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

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

Don't you need to free rpm_msg items in this case?

Quoted 4 lines

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

+
+       atomic_set(&wait_count, count);
+
+       for (i = 0; i < count; i++) {

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

Quoted 10 lines

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

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

You're just using rpmh_tx_done() to free memory? Note that you'll probably do your error handling in this function a favor if you rename __get_rpmh_msg_async() to __fill_rpmh_msg() and remove the memory allocation from there. Then you can do one big allocation of the whole array in rpmh_write_batch() and then you'll only have one free at the end...

Quoted 1 line

+ break;

"break" seems wrong here. You'll end up waiting for the completion, then I guess timing out, then returning -ETIMEDOUT?

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

Quoted 6 lines

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

+               }
+       }
+
+       ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);

The "wait_count" abstraction is confusing and I believe it's not needed. I think you can remove it and change the above to this (untested) code:

time_left = RPMH_TIMEOUT_MS;

for (i = 0; i < count; i++) {
   time_left = wait_for_completion_timeout(&compl, time_left);
   if (!time_left)
     return -ETIMEDOUT;
}

...specifically completions are additive, so just wait "count" times and then the reader doesn't need to learn your new wait_count abstraction and try to reason about it.

...and, actually, I argue in other replies that this should't use a timeout, so even cleaner:

for (i = 0; i < count; i++)
wait_for_completion(&compl);

Once you do that, you can also get rid of the need to pre-count "n", so all your loops turn into:

for (i = 0; n[i]; i++)

I suppose you might want to get rid of "RPMH_MAX_REQ_IN_BATCH" and dynamically allocate your array too, but that seems sane. As per above it seems like you should just dynamically allocate a whole array of "struct rpmh_request" items at once anyway.

---

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

Quoted 4 lines

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

+       return (ret > 0) ? 0 : -ETIMEDOUT;
+
+}
+EXPORT_SYMBOL(rpmh_write_batch);

Perhaps an even simpler thing than taking all my advice above: can't you just add a optional completion to rpmh_write_async()? That would just be stuffed into rpm_msg.

Now your batch code would just be a bunch of calls to rpmh_write_async() with an equal number of wait_for_completion() calls at the end. Is there a reason that wouldn't work? You'd get rid of _a lot_ of code.

-Doug

Quoted 5 lines

Quoted 1 line

+ break;

"break" seems wrong here. You'll end up waiting for the completion, then I guess timing out, then returning -ETIMEDOUT?

As the loop above break runs for remaining count, completion will be notified so there will not be waiting.

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]>

Reviewed-by Matthias Kaehlcke <[email protected]>

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

drivers/soc/qcom/rpmh-rsc.c +4 -1

index 68c25ebbbe09..369b9b3eedc5 100644
@@ -153,6 +153,7 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                                         const struct tcs_request *msg)
 {
        int type;
+       struct tcs_group *tcs;
 
        switch (msg->state) {
        case RPMH_ACTIVE_ONLY_STATE:

@@ -168,7 +169,22 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                return ERR_PTR(-EINVAL);
        }
 
-       return get_tcs_of_type(drv, type);
+       /*
+        * 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

Hi,

Quoted 40 lines · Lina Iyer

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]>

Reviewed-by Matthias Kaehlcke <[email protected]>

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

drivers/soc/qcom/rpmh-rsc.c +9 -1

index 68c25ebbbe09..369b9b3eedc5 100644
@@ -153,6 +153,7 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                                          const struct tcs_request *msg)
  {
         int type;
+       struct tcs_group *tcs;
 
         switch (msg->state) {
         case RPMH_ACTIVE_ONLY_STATE:

@@ -168,7 +169,22 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
                 return ERR_PTR(-EINVAL);
         }
 
-       return get_tcs_of_type(drv, type);
+       /*
+        * 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.
+        */

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

Quoted 5 lines

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

+       tcs = get_tcs_of_type(drv, type);
+       if (msg->state == RPMH_ACTIVE_ONLY_STATE && IS_ERR(tcs)) {
+               tcs = get_tcs_of_type(drv, WAKE_TCS);
+               if (!IS_ERR(tcs))
+                       rpmh_rsc_invalidate(drv);

I noticed that rpmh_rsc_invalidate() can return -EAGAIN. Do you need to deal with that here?

-Doug