[v2] soc: qcom: rpmh: Remove serialization of TCS commands

Commit Message

Maulik Shah Nov. 24, 2020, 7:31 a.m. UTC

From: Lina Iyer <[e-mail]>

Requests sent to RPMH can be sent as fire-n-forget or response required,
with the latter ensuring the command has been completed by the hardware
accelerator. Commands in a request with tcs_cmd::wait set, would ensure
that those select commands are sent as response required, even though
the actual TCS request may be fire-n-forget.

Also, commands with .wait flag were also guaranteed to be complete
before the following command in the TCS is sent. This means that the
next command of the same request blocked until the current request is
completed. This could mean waiting for a voltage to settle or series of
NOCs be configured before the next command is sent. But drivers using
this feature have never cared about the serialization aspect. By not
enforcing the serialization we can allow the hardware to run in parallel
improving the performance.

Let's clarify the usage of this member in the tcs_cmd structure to mean
only completion and not serialization. This should also improve the
performance of bus requests where changes could happen in parallel.
Also, CPU resume from deep idle may see benefits from certain wake
requests.

Signed-off-by: Lina Iyer <[e-mail]>
Signed-off-by: Maulik Shah <[e-mail]>
---
Changes in v2:
- Add SoB of self
- Fix typo in comment
- Update comment as Doug suggested
- Remove write to RSC_DRV_CMD_WAIT_FOR_CMPL in tcs_write() and tcs_invalidate()
---
 drivers/soc/qcom/rpmh-rsc.c | 25 ++++++++++---------------
 include/soc/qcom/tcs.h      |  3 ++-
 2 files changed, 12 insertions(+), 16 deletions(-)

Comments

Doug Anderson Dec. 3, 2020, 9:44 p.m. UTC | #1

Hi,

On Mon, Nov 23, 2020 at 11:32 PM Maulik Shah <[e-mail]> wrote:
>
> @@ -423,8 +422,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>                         cmd = &req->cmds[j];
>                         sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
>                         if (!(sts & CMD_STATUS_ISSUED) ||
> -                          ((req->wait_for_compl || cmd->wait) &&
> -                          !(sts & CMD_STATUS_COMPL))) {
> +                          (cmd->wait && !(sts & CMD_STATUS_COMPL))) {
>                                 pr_err("Incomplete request: %s: addr=%#x data=%#x",
>                                        drv->name, cmd->addr, cmd->data);
>                                 err = -EIO;

In my review of v1 all those months ago, the way we left things was
that I disagreed with this part of the patch, and I still do.  I think
you should leave things the way they were in tcs_tx_done().  Copying
my un-responded-to comments from v1 here for you:

In your patch in __tcs_buffer_write(), if "wait_for_compl" is set then
"CMD_MSGID_RESP_REQ" will be added for every message in the request,
right?  That's because you have this bit of code:

/* Convert all commands to RR when the request has wait_for_compl set */
cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;

That means that if _either_ "cmd->wait" or "req->wait_for_compl" is
set then you expect the "sts" to have "CMD_STATUS_COMPL", right?
That's exactly the code that used to be there.

Said another way, if "req->wait_for_compl" was set then it's an error
if any of our commands are missing the "CMD_STATUS_COMPL" bit, right?


> @@ -30,7 +30,7 @@ enum rpmh_state {
>   *
>   * @addr: the address of the resource slv_id:18:16 | offset:0:15
>   * @data: the resource state request
> - * @wait: wait for this request to be complete before sending the next
> + * @wait: ensure that this command is complete before returning

In my response to v1 I suggested that a comment would be nice here.
Something akin to:

Setting "wait" here only makes sense in the batch case for active-only
transfers.

This is because:
* rpmh_write_async() - There's no callback and rpmh_write_async()
doesn't set the "completion" to anything so there's nobody that cares
at all

* DEFINE_RPMH_MSG_ONSTACK - always sets wait_for_compl.

-Doug

Maulik Shah Jan. 7, 2021, 8:37 a.m. UTC | #2

Hi Doug,

On 12/4/2020 3:14 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 23, 2020 at 11:32 PM Maulik Shah <[e-mail]> wrote:
>> @@ -423,8 +422,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>>                          cmd = &req->cmds[j];
>>                          sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
>>                          if (!(sts & CMD_STATUS_ISSUED) ||
>> -                          ((req->wait_for_compl || cmd->wait) &&
>> -                          !(sts & CMD_STATUS_COMPL))) {
>> +                          (cmd->wait && !(sts & CMD_STATUS_COMPL))) {
>>                                  pr_err("Incomplete request: %s: addr=%#x data=%#x",
>>                                         drv->name, cmd->addr, cmd->data);
>>                                  err = -EIO;
> In my review of v1 all those months ago, the way we left things was
> that I disagreed with this part of the patch, and I still do.  I think
> you should leave things the way they were in tcs_tx_done().  Copying
> my un-responded-to comments from v1 here for you:
>
> In your patch in __tcs_buffer_write(), if "wait_for_compl" is set then
> "CMD_MSGID_RESP_REQ" will be added for every message in the request,
> right?  That's because you have this bit of code:
>
> /* Convert all commands to RR when the request has wait_for_compl set */
> cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
>
> That means that if _either_ "cmd->wait" or "req->wait_for_compl" is
> set then you expect the "sts" to have "CMD_STATUS_COMPL", right?
> That's exactly the code that used to be there.
>
> Said another way, if "req->wait_for_compl" was set then it's an error
> if any of our commands are missing the "CMD_STATUS_COMPL" bit, right?
>
>
>> @@ -30,7 +30,7 @@ enum rpmh_state {
>>    *
>>    * @addr: the address of the resource slv_id:18:16 | offset:0:15
>>    * @data: the resource state request
>> - * @wait: wait for this request to be complete before sending the next
>> + * @wait: ensure that this command is complete before returning
> In my response to v1 I suggested that a comment would be nice here.
> Something akin to:
>
> Setting "wait" here only makes sense in the batch case for active-only
> transfers.
>
> This is because:
> * rpmh_write_async() - There's no callback and rpmh_write_async()
> doesn't set the "completion" to anything so there's nobody that cares
> at all
>
> * DEFINE_RPMH_MSG_ONSTACK - always sets wait_for_compl.
>
> -Doug

Addressed both comments and sent v3.

Thanks,
Maulik

Patch

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 37969dc..9a06099 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -231,10 +231,9 @@  static void tcs_invalidate(struct rsc_drv *drv, int type)
 	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
 		return;
 
-	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++)
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
-		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
-	}
+
 	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
 }
 
@@ -423,8 +422,7 @@  static irqreturn_t tcs_tx_done(int irq, void *p)
 			cmd = &req->cmds[j];
 			sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
 			if (!(sts & CMD_STATUS_ISSUED) ||
-			   ((req->wait_for_compl || cmd->wait) &&
-			   !(sts & CMD_STATUS_COMPL))) {
+			   (cmd->wait && !(sts & CMD_STATUS_COMPL))) {
 				pr_err("Incomplete request: %s: addr=%#x data=%#x",
 				       drv->name, cmd->addr, cmd->data);
 				err = -EIO;
@@ -443,7 +441,6 @@  static irqreturn_t tcs_tx_done(int irq, void *p)
 skip:
 		/* Reclaim the TCS */
 		write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
-		write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0);
 		writel_relaxed(BIT(i), drv->tcs_base + RSC_DRV_IRQ_CLEAR);
 		spin_lock(&drv->lock);
 		clear_bit(i, drv->tcs_in_use);
@@ -476,23 +473,23 @@  static irqreturn_t tcs_tx_done(int irq, void *p)
 static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 			       const struct tcs_request *msg)
 {
-	u32 msgid, cmd_msgid;
+	u32 msgid;
+	u32 cmd_msgid = CMD_MSGID_LEN | CMD_MSGID_WRITE;
 	u32 cmd_enable = 0;
-	u32 cmd_complete;
 	struct tcs_cmd *cmd;
 	int i, j;
 
-	cmd_msgid = CMD_MSGID_LEN;
+	/* Convert all commands to RR when the request has wait_for_compl set */
 	cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
-	cmd_msgid |= CMD_MSGID_WRITE;
-
-	cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id);
 
 	for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
 		cmd = &msg->cmds[i];
 		cmd_enable |= BIT(j);
-		cmd_complete |= cmd->wait << j;
 		msgid = cmd_msgid;
+		/*
+		 * Additionally, if the cmd->wait is set, make the command
+		 * response reqd even if the overall request was fire-n-forget.
+		 */
 		msgid |= cmd->wait ? CMD_MSGID_RESP_REQ : 0;
 
 		write_tcs_cmd(drv, RSC_DRV_CMD_MSGID, tcs_id, j, msgid);
@@ -501,7 +498,6 @@  static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 		trace_rpmh_send_msg(drv, tcs_id, j, msgid, cmd);
 	}
 
-	write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete);
 	cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
 	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
 }
@@ -652,7 +648,6 @@  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 		 * cleaned from rpmh_flush() by invoking rpmh_rsc_invalidate()
 		 */
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
-		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
 		enable_tcs_irq(drv, tcs_id, true);
 	}
 	spin_unlock_irqrestore(&drv->lock, flags);
diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
index 7a2a055..eb5cb35 100644
--- a/include/soc/qcom/tcs.h
+++ b/include/soc/qcom/tcs.h
@@ -30,7 +30,7 @@  enum rpmh_state {
  *
  * @addr: the address of the resource slv_id:18:16 | offset:0:15
  * @data: the resource state request
- * @wait: wait for this request to be complete before sending the next
+ * @wait: ensure that this command is complete before returning
  */
 struct tcs_cmd {
 	u32 addr;
@@ -43,6 +43,7 @@  struct tcs_cmd {
  *
  * @state:          state for the request.
  * @wait_for_compl: wait until we get a response from the h/w accelerator
+ *                  (same as setting cmd->wait for all commands in the request)
  * @num_cmds:       the number of @cmds in this request
  * @cmds:           an array of tcs_cmds
  */