Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments
@ 2020-03-11 23:13 Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
                   ` (9 more replies)
  0 siblings, 10 replies; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

In order to review Maulik's latest "rpmh_flush for non OSI targets"
patch series I've found myself trying to understand rpmh-rsc better.
To make it easier for others to do this in the future, add a whole lot
of comments / documentation.

As part of this there are a very small number of functional changes.
- We'll get a tiny performance boost by getting rid of the "cmd_cache"
  which I believe was unnecessary (though just to be sure, best to try
  this atop Maulik's patches where it should be super obvious that we
  always invalidate before writing sleep/wake TCSs.
- I think I've eliminated a possible deadlock on "nosmp" systems,
  though it was mostly theoretical.
- Possibly we could get a warning in some cases if I misunderstood how
  tcs_is_free() works.  It'd be easy to remove the warning, though.

These changes touch a lot of code in rpmh-rsc, so hopefully someone at
Qualcomm can test them out better than I did (I don't have every last
client of RPMH in my tree) and review them soon-ish so they can land
and future patches can be based on them.

I've tried to structure the patches so that simpler / less
controversial patches are first.  Those could certainly land on their
own without later patches.  Many of the patches could also be dropped
and the others would still apply if they are controversial.  If you
need help doing this then please yell.

At the end of this patch series there is still one TODO left in the
code.  Specifically I think we still have some problems when we try to
borrow a wakeup TCS for an active-only transfer.  The problems I think
are there aren't new, just the comment.  It's unclear if we should
land with the TODO in place or wait till that gets resolved first.

With all that, enjoy.

Changes in v2:
- Now prose in comments instead of struct definitions.
- Pretty ASCII art from Stephen.
- More clear that active-only xfers can happen on wake TCS sometimes.
- Document locks for updating "tcs_in_use" more.
- Document tcs_is_free() without drv->lock OK for tcs_invalidate().
- Document bug of tcs_write() not handling -EAGAIN.
- Document get_tcs_for_msg() => -EAGAIN only for ACTIVE_ONLY.
- Reword tcs_write() doc a bit.
- Document two get_tcs_for_msg() issues if zero-active TCS.
- Document that rpmh_rsc_send_data() can be an implicit invalidate.
- Fixed documentation of "tcs" param in find_slots().
- Comment tcs_is_free() new for v2; replaces old patch 6.
- Got rid of useless "if (x) continue" at end of for loop.
- ("Always use -EAGAIN, never -EBUSY") new for v2.

Douglas Anderson (10):
  drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds
  drivers: qcom: rpmh-rsc: Document the register layout better
  drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
  drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction
  drivers: qcom: rpmh-rsc: A lot of comments
  drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state
    mismatch
  drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active
  drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()
  drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
  drivers: qcom: rpmh-rsc: Always use -EAGAIN, never -EBUSY

 drivers/soc/qcom/rpmh-internal.h |  52 ++--
 drivers/soc/qcom/rpmh-rsc.c      | 443 +++++++++++++++++++++++--------
 2 files changed, 358 insertions(+), 137 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01  8:12   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

This patch makes two changes, both of which should be no-ops:

1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() /
   write_tcs_cmd().

2. Change the order of operations in the above functions to make it
   more obvious to me what the math is doing.  Specifically first you
   want to find the right TCS, then the right register, and then
   multiply by the command ID if necessary.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index b71822131f59..b87b79f0347d 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -61,28 +61,33 @@
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
-static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
+static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
 {
-	return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
 			     RSC_DRV_CMD_OFFSET * cmd_id);
 }
 
+static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id)
+{
+	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
+}
+
 static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id,
 			  u32 data)
 {
-	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+	writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
 		       RSC_DRV_CMD_OFFSET * cmd_id);
 }
 
 static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data)
 {
-	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+	writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
 }
 
 static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
 			       u32 data)
 {
-	writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+	writel(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
 	for (;;) {
 		if (data == readl(drv->tcs_base + reg +
 				  RSC_DRV_TCS_OFFSET * tcs_id))
@@ -94,7 +99,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
 static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 {
 	return !test_bit(tcs_id, drv->tcs_in_use) &&
-	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
 }
 
 static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
@@ -212,7 +217,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 	const struct tcs_request *req;
 	struct tcs_cmd *cmd;
 
-	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
+	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0);
 
 	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
 		req = get_req_from_tcs(drv, i);
@@ -226,7 +231,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 			u32 sts;
 
 			cmd = &req->cmds[j];
-			sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, 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))) {
@@ -265,7 +270,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 	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, 0);
+	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];
@@ -281,7 +286,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 	}
 
 	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, 0);
+	cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
 	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
 }
 
@@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
 	 * While clearing ensure that the AMC mode trigger is cleared
 	 * and then the mode enable is cleared.
 	 */
-	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0);
+	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id);
 	enable &= ~TCS_AMC_MODE_TRIGGER;
 	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
 	enable &= ~TCS_AMC_MODE_ENABLE;
@@ -319,10 +324,10 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 		if (tcs_is_free(drv, tcs_id))
 			continue;
 
-		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
 
 		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
-			addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
+			addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
 			for (k = 0; k < msg->num_cmds; k++) {
 				if (addr == msg->cmds[k].addr)
 					return -EBUSY;
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01  8:14   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

Perhaps it's just me, it took a really long time to understand what
the register layout of rpmh-rsc was just from the #defines.  Let's add
a bunch of comments describing which blocks are part of other blocks.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Now prose in comments instead of struct definitions.
- Pretty ASCII art from Stephen.

 drivers/soc/qcom/rpmh-rsc.c | 78 ++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index b87b79f0347d..02c8e0ffbbe4 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -37,14 +37,24 @@
 #define DRV_NCPT_MASK			0x1F
 #define DRV_NCPT_SHIFT			27
 
-/* Register offsets */
+/*
+ * Register offsets within a TCS.
+ *
+ * TCSs are stored one after another; multiply tcs_id by RSC_DRV_TCS_OFFSET
+ * to find a given TCS and add one of the below to find a register.
+ */
 #define RSC_DRV_IRQ_ENABLE		0x00
 #define RSC_DRV_IRQ_STATUS		0x04
-#define RSC_DRV_IRQ_CLEAR		0x08
-#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10
+#define RSC_DRV_IRQ_CLEAR		0x08	/* w/o; write 1 to clear */
+#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10	/* 1 bit per command */
 #define RSC_DRV_CONTROL			0x14
-#define RSC_DRV_STATUS			0x18
-#define RSC_DRV_CMD_ENABLE		0x1C
+#define RSC_DRV_STATUS			0x18	/* zero if tcs is busy */
+#define RSC_DRV_CMD_ENABLE		0x1C	/* 1 bit per command */
+
+/*
+ * Commands (up to 16) start at 0x30 in a TCS; multiply command index
+ * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
+ */
 #define RSC_DRV_CMD_MSGID		0x30
 #define RSC_DRV_CMD_ADDR		0x34
 #define RSC_DRV_CMD_DATA		0x38
@@ -61,6 +71,64 @@
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
+/*
+ * Here's a high level overview of how all the registers in RPMH work
+ * together:
+ *
+ * - The main rpmh-rsc address is the base of a register space that can
+ *   be used to find overall configuration of the hardware
+ *   (DRV_PRNT_CHLD_CONFIG).  Also found within the rpmh-rsc register
+ *   space are all the TCS blocks.  The offset of the TCS blocks is
+ *   specified in the device tree by "qcom,tcs-offset" and used to
+ *   compute tcs_base.
+ * - TCS blocks come one after another.  Type, count, and order are
+ *   specified by the device tree as "qcom,tcs-config".
+ * - Each TCS block has some registers, then space for up to 16 commands.
+ *   Note that though address space is reserved for 16 commands, fewer
+ *   might be present.  See ncpt (num cmds per TCS).
+ * - The first TCS block is special in that it has registers to control
+ *   interrupts (RSC_DRV_IRQ_XXX).  Space for these registers is reserved
+ *   in all TCS blocks to make the math easier, but only the first one
+ *   matters.
+ *
+ * Here's a picture:
+ *
+ *  +---------------------------------------------------+
+ *  |RSC                                                |
+ *  | ctrl                                              |
+ *  |                                                   |
+ *  | Drvs:                                             |
+ *  | +-----------------------------------------------+ |
+ *  | |DRV0                                           | |
+ *  | | ctrl                                          | |
+ *  | |                                               | |
+ *  | | TCSs:                                         | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | | IRQ  | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS1  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | | +------------------------------------------+  | |
+ *  | | |TCS2  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
+ *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
+ *  | | +------------------------------------------+  | |
+ *  | |                    ......                     | |
+ *  | +-----------------------------------------------+ |
+ *  | +-----------------------------------------------+ |
+ *  | |DRV1                                           | |
+ *  | | (same as DRV0)                                | |
+ *  | +-----------------------------------------------+ |
+ *  |                      ......                       |
+ *  +---------------------------------------------------+
+ */
+
+
 static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
 {
 	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01  8:17   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

I was trying to write documentation for the functions in rpmh-rsc and
I got to tcs_ctrl_write().  The documentation for the function would
have been: "This is the core of rpmh_rsc_write_ctrl_data(); all the
caller does is error-check and then call this".

Having the error checks in a separate function doesn't help for
anything since:
- There are no other callers that need to bypass the error checks.
- It's less documenting.  When I read tcs_ctrl_write() I kept
  wondering if I need to handle cases other than ACTIVE_ONLY or cases
  with more commands than could fit in a TCS.  This is obvious when
  the error checks and code are together.
- The function just isn't that long, so there's no problem
  understanding the combined function.

Things were even more confusing because the two functions names didn't
make obvious (at least to me) their relationship.

Simplify by folding one function into the other.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
---

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 39 ++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 02c8e0ffbbe4..799847b08038 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -550,27 +550,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 	return 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 TCS id and the command in the TCS to write to */
-	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
-	if (!ret)
-		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
-	spin_unlock_irqrestore(&tcs->lock, flags);
-
-	return ret;
-}
-
 /**
  * rpmh_rsc_write_ctrl_data: Write request to the controller
  *
@@ -581,6 +560,11 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
  */
 int rpmh_rsc_write_ctrl_data(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;
+
 	if (!msg || !msg->cmds || !msg->num_cmds ||
 	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
 		pr_err("Payload error\n");
@@ -591,7 +575,18 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	if (msg->state == RPMH_ACTIVE_ONLY_STATE)
 		return -EINVAL;
 
-	return tcs_ctrl_write(drv, msg);
+	tcs = get_tcs_for_msg(drv, msg);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock_irqsave(&tcs->lock, flags);
+	/* find the TCS id and the command in the TCS to write to */
+	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
+	if (!ret)
+		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
+	spin_unlock_irqrestore(&tcs->lock, flags);
+
+	return ret;
 }
 
 static int rpmh_probe_tcs_config(struct platform_device *pdev,
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (2 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01  8:18   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

The get_tcs_of_type() function doesn't provide any value.  It's not
conceptually difficult to access a value in an array, even if that
value is in a structure and we want a pointer to the value.  Having
the function in there makes me feel like it's doing something fancier
like looping or searching.  Remove it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
---

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 799847b08038..c9f29cbd5ee5 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -170,17 +170,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
 }
 
-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);
+	struct tcs_group *tcs = &drv->tcs[type];
 
 	spin_lock(&tcs->lock);
 	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
@@ -246,9 +239,9 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 	 * dedicated AMC, and therefore would invalidate the sleep and wake
 	 * TCSes before making an active state request.
 	 */
-	tcs = get_tcs_of_type(drv, type);
+	tcs = &drv->tcs[type];
 	if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs) {
-		tcs = get_tcs_of_type(drv, WAKE_TCS);
+		tcs = &drv->tcs[WAKE_TCS];
 		if (tcs->num_tcs) {
 			ret = rpmh_rsc_invalidate(drv);
 			if (ret)
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (3 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01 11:29   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch Douglas Anderson
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

I've been pouring through the rpmh-rsc code and trying to understand
it.  Document everything to the best of my ability.  All documentation
here is strictly from code analysis--no actual knowledge of the
hardware was used.  If something is wrong in here I either
misunderstood the code, had a typo, or the code has a bug in it
leading to my incorrect understanding.

In a few places here I have documented things that don't make tons of
sense.  A future patch will try to address this.  While this means I'm
adding comments / todos and then later fixing them in the series, it
seemed more urgent to get things documented first so that people could
understand the later patches.

This should be a no-op.  It's just comment changes.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- More clear that active-only xfers can happen on wake TCS sometimes.
- Document locks for updating "tcs_in_use" more.
- Document tcs_is_free() without drv->lock OK for tcs_invalidate().
- Document bug of tcs_write() not handling -EAGAIN.
- Document get_tcs_for_msg() => -EAGAIN only for ACTIVE_ONLY.
- Reword tcs_write() doc a bit.
- Document two get_tcs_for_msg() issues if zero-active TCS.
- Document that rpmh_rsc_send_data() can be an implicit invalidate.
- Fixed documentation of "tcs" param in find_slots().

 drivers/soc/qcom/rpmh-internal.h |  52 +++---
 drivers/soc/qcom/rpmh-rsc.c      | 264 +++++++++++++++++++++++++++++--
 2 files changed, 281 insertions(+), 35 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 6eec32b97f83..b756d3036e96 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -22,16 +22,25 @@ struct rsc_drv;
  * struct tcs_group: group of Trigger Command Sets (TCS) to send state requests
  * to the controller
  *
- * @drv:       the controller
- * @type:      type of the TCS in this group - active, sleep, wake
- * @mask:      mask of the TCSes relative to all the TCSes in the RSC
- * @offset:    start of the TCS group relative to the TCSes in the RSC
- * @num_tcs:   number of TCSes in this type
- * @ncpt:      number of commands in each TCS
- * @lock:      lock for synchronizing this TCS writes
- * @req:       requests that are sent from the TCS
- * @cmd_cache: flattened cache of cmds in sleep/wake TCS
- * @slots:     indicates which of @cmd_addr are occupied
+ * @drv:       The controller.
+ * @type:      Type of the TCS in this group - active, sleep, wake.
+ * @mask:      Mask of the TCSes relative to all the TCSes in the RSC.
+ * @offset:    Start of the TCS group relative to the TCSes in the RSC.
+ * @num_tcs:   Number of TCSes in this type.
+ * @ncpt:      Number of commands in each TCS.
+ * @lock:      Lock for synchronizing this TCS writes.
+ * @req:       Requests that are sent from the TCS; only used for ACTIVE_ONLY
+ *             transfers (could be on a wake/sleep TCS if we are borrowing for
+ *             an ACTIVE_ONLY transfer).
+ *             Start: grab drv->lock, set req, set tcs_in_use, drop drv->lock,
+ *                    trigger
+ *             End: get irq, access req,
+ *                  grab drv->lock, clear tcs_in_use, drop drv->lock
+ * @cmd_cache: Flattened cache of cmds in sleep/wake TCS; num_tcs * ncpt big.
+ * @slots:     Indicates which of @cmd_addr are occupied; only used for
+ *             SLEEP / WAKE TCSs.  Things are tightly packed in the
+ *             case that (ncpt < MAX_CMDS_PER_TCS).  That is if ncpt = 2 and
+ *             MAX_CMDS_PER_TCS = 16 then bit[2] = the first bit in 2nd TCS.
  */
 struct tcs_group {
 	struct rsc_drv *drv;
@@ -84,14 +93,21 @@ struct rpmh_ctrlr {
  * struct rsc_drv: the Direct Resource Voter (DRV) of the
  * Resource State Coordinator controller (RSC)
  *
- * @name:       controller identifier
- * @tcs_base:   start address of the TCS registers in this controller
- * @id:         instance id in the controller (Direct Resource Voter)
- * @num_tcs:    number of TCSes in this DRV
- * @tcs:        TCS groups
- * @tcs_in_use: s/w state of the TCS
- * @lock:       synchronize state of the controller
- * @client:     handle to the DRV's client.
+ * @name:       Controller identifier.
+ * @tcs_base:   Start address of the TCS registers in this controller.
+ * @id:         Instance id in the controller (Direct Resource Voter).
+ * @num_tcs:    Number of TCSes in this DRV.
+ * @tcs:        TCS groups.
+ * @tcs_in_use: s/w state of the TCS; only set for ACTIVE_ONLY transfers, but
+ *              might show a sleep/wake TCS in use if it was borrowed for an
+ *              active_only transfer.  You must hold both the lock in this
+ *              struct and the tcs_lock for the TCS in order to mark a TCS as
+ *              in-use, but you only need the lock in this structure to mark
+ *              one freed.
+ * @lock:       Synchronize state of the controller.  If you will be grabbing
+ *              this lock and a tcs_lock at the same time, grab the tcs_lock
+ *              first so we always have a consistent lock ordering.
+ * @client:     Handle to the DRV's client.
  */
 struct rsc_drv {
 	const char *name;
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index c9f29cbd5ee5..9d2669cbd994 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -164,12 +164,38 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
 	}
 }
 
+/**
+ * tcs_is_free() - Return if a TCS is totally free.
+ * @drv:    The RSC controller.
+ * @tcs_id: The global ID of this TCS.
+ *
+ * Returns true if nobody has claimed this TCS (by setting tcs_in_use).
+ * If the TCS looks free, checks that the hardware agrees.
+ *
+ * Must be called with the drv->lock held or the tcs_lock for the TCS being
+ * tested.  If only the tcs_lock is held then it is possible that this
+ * function will return that a tcs is still busy when it has been recently
+ * been freed but it will never return free when a TCS is actually in use.
+ *
+ * Return: true if the given TCS is free.
+ */
 static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 {
 	return !test_bit(tcs_id, drv->tcs_in_use) &&
 	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
 }
 
+/**
+ * tcs_invalidate() - Invalidate all TCSs of the given type (sleep or wake).
+ * @drv:  The RSC controller.
+ * @type: SLEEP_TCS or WAKE_TCS
+ *
+ * This will clear the "slots" variable of the given tcs_group and also
+ * tell the hardware to forget about all entries.
+ *
+ * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
+ *         bit.  Caller should make sure to enable interrupts between tries.
+ */
 static int tcs_invalidate(struct rsc_drv *drv, int type)
 {
 	int m;
@@ -196,9 +222,11 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
 }
 
 /**
- * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
+ * rpmh_rsc_invalidate() - Invalidate sleep and wake TCSes.
+ * @drv: The RSC controller.
  *
- * @drv: the RSC controller
+ * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
+ *         bit.  Caller should make sure to enable interrupts between tries.
  */
 int rpmh_rsc_invalidate(struct rsc_drv *drv)
 {
@@ -211,6 +239,20 @@ int rpmh_rsc_invalidate(struct rsc_drv *drv)
 	return ret;
 }
 
+/**
+ * get_tcs_for_msg() - Get the tcs_group used to send the given message.
+ * @drv: The RSC controller.
+ * @msg: The message we want to send.
+ *
+ * This is normally pretty straightforward except if we are trying to send
+ * an ACTIVE_ONLY message but don't have any active_only TCSs.
+ *
+ * Called without drv->lock held and with no tcs_lock locks held.
+ *
+ * Return: 0 if no problem, or -EAGAIN if the caller should try again in a bit.
+ *         Caller should make sure to enable interrupts between tries.
+ *         Only ever returns -EAGAIN for ACTIVE_ONLY transfers.
+ */
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 					 const struct tcs_request *msg)
 {
@@ -246,12 +288,35 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 			ret = rpmh_rsc_invalidate(drv);
 			if (ret)
 				return ERR_PTR(ret);
+
+			/*
+			 * TODO:
+			 * - Temporarily enable IRQs on the wake tcs.
+			 * - Make sure nobody else race with us and re-write
+			 *   to this TCS; document how this works.
+			 */
 		}
 	}
 
 	return tcs;
 }
 
+/**
+ * get_req_from_tcs() - Get a stashed request that was xfering on the given tcs.
+ * @drv:    The RSC controller.
+ * @tcs_id: The global ID of this TCS.
+ *
+ * For ACTIVE_ONLY transfers we want to call back into the client when the
+ * transfer finishes.  To do this we need the "request" that the client
+ * originally provided us.  This function grabs the request that we stashed
+ * when we started the transfer.
+ *
+ * This only makes sense for ACTIVE_ONLY transfers since those are the only
+ * ones we track sending (the only ones we enable interrupts for and the only
+ * ones we call back to the client for).
+ *
+ * Return: The stashed request.
+ */
 static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
 						  int tcs_id)
 {
@@ -268,7 +333,14 @@ static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
 }
 
 /**
- * tcs_tx_done: TX Done interrupt handler
+ * tcs_tx_done() - TX Done interrupt handler.
+ * @irq: The IRQ number (ignored).
+ * @p:   Pointer to "struct rsc_drv".
+ *
+ * Called for ACTIVE_ONLY TCSs (those are the only ones we enable the IRQ for)
+ * when a transfer is done.
+ *
+ * Return: IRQ_HANDLED
  */
 static irqreturn_t tcs_tx_done(int irq, void *p)
 {
@@ -278,6 +350,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 	const struct tcs_request *req;
 	struct tcs_cmd *cmd;
 
+	/* NOTE: interrupt status for all TCSs are found in TCS 0 */
 	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0);
 
 	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
@@ -318,6 +391,16 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+/**
+ * __tcs_buffer_write() - Write to TCS hardware from a request; don't trigger.
+ * @drv:    The controller.
+ * @tcs_id: The global ID of this TCS.
+ * @cmd_id: The index within the TCS to start writing.
+ * @msg:    The message we want to send, which will contain several addr/data
+ *          pairs to program (but few enough that they all fit in one TCS).
+ *
+ * This is used for all types of TCSs (active, sleep, and wake).
+ */
 static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 			       const struct tcs_request *msg)
 {
@@ -351,6 +434,15 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
 }
 
+/**
+ * __tcs_trigger() - Start transferring on the given TCS.
+ *
+ * The TCS given is probably the active-only one, but could be a wake one
+ * that we borrowed if there are zero active-only TCSs.
+ *
+ * @drv:    The controller.
+ * @tcs_id: The global ID of this TCS.
+ */
 static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
 {
 	u32 enable;
@@ -373,6 +465,27 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
 	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
 }
 
+/**
+ * check_for_req_inflight() - Look to see if conflicting cmds are in flight.
+ * @drv: The controller.
+ * @tcs: A pointer to the tcs_group used for ACTIVE_ONLY transfers.
+ * @msg: The message we want to send, which will contain several addr/data
+ *       pairs to program (but few enough that they all fit in one TCS).
+ *
+ * Only for use for ACTIVE_ONLY tcs_group, since those are the only ones
+ * that might be actively sending.
+ *
+ * This will walk through the TCSs in the group and check if any of them
+ * appear to be sending to addresses referenced in the message.  If it finds
+ * one it'll return -EBUSY.
+ *
+ * Must be called with the drv->lock held since that protects tcs_in_use.
+ *
+ * Return: 0 if nothing in flight or -EBUSY if we should try again later.
+ *         The caller must re-enable interrupts between tries since that's
+ *         the only way tcs_is_free() will ever return true and the only way
+ *         RSC_DRV_CMD_ENABLE will ever be cleared.
+ */
 static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 				  const struct tcs_request *msg)
 {
@@ -399,6 +512,15 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 	return 0;
 }
 
+/**
+ * find_free_tcs() - Find free tcs in the given tcs_group; only for ACTIVE_ONLY.
+ * @tcs: A pointer to the ACTIVE_ONLY tcs_group (or the wake tcs_group if
+ *       we borrowed it because there are zero active-only ones).
+ *
+ * Must be called with the drv->lock held since that protects tcs_in_use.
+ *
+ * Return: The first tcs that's free.
+ */
 static int find_free_tcs(struct tcs_group *tcs)
 {
 	int i;
@@ -411,6 +533,20 @@ static int find_free_tcs(struct tcs_group *tcs)
 	return -EBUSY;
 }
 
+/**
+ * tcs_write() - Store messages into a TCS right now, or return -EBUSY.
+ * @drv: The controller.
+ * @msg: The data to be sent.
+ *
+ * Grabs a TCS for ACTIVE_ONLY transfers and writes the messages to it.
+ *
+ * If there are no free ACTIVE_ONLY TCSs or if a command for the same address
+ * is already transferring returns -EBUSY which means the client should retry
+ * shortly.
+ *
+ * Return: 0 on success, -EBUSY if client should retry, or an error.
+ *         Client should have interrupts enabled for a bit before retrying.
+ */
 static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 {
 	struct tcs_group *tcs;
@@ -418,16 +554,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	unsigned long flags;
 	int ret;
 
+	/* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);
 
 	spin_lock_irqsave(&tcs->lock, flags);
+
 	spin_lock(&drv->lock);
-	/*
-	 * The h/w does not like if we send a request to the same address,
-	 * when one is already in-flight or being processed.
-	 */
 	ret = check_for_req_inflight(drv, tcs, msg);
 	if (ret) {
 		spin_unlock(&drv->lock);
@@ -454,14 +588,30 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 }
 
 /**
- * rpmh_rsc_send_data: Validate the incoming message and write to the
- * appropriate TCS block.
+ * rpmh_rsc_send_data() - Validate the incoming message + write to TCS block.
+ * @drv: The controller.
+ * @msg: The data to be sent.
  *
- * @drv: the controller
- * @msg: the data to be sent
+ * NOTES:
+ * - This is only used for "ACTIVE_ONLY" since the limitations of this
+ *   function don't make sense for sleep/wake cases.
+ * - To do the transfer, we will grab a whole TCS for ourselves--we don't
+ *   try to share.  If there are none available we'll wait indefinitely
+ *   for a free one.
+ * - This function will not wait for the commands to be finished, only for
+ *   data to be programmed into the RPMh.  See rpmh_tx_done() which will
+ *   be called when the transfer is fully complete.
+ * - This function must be called with interrupts enabled.  If the hardware
+ *   is busy doing someone else's transfer we need that transfer to fully
+ *   finish so that we can have the hardware, and to fully finish it needs
+ *   the interrupt handler to run.  If the interrupts is set to run on the
+ *   active CPU this can never happen if interrupts are disabled.
+ * - If there are no active TCS calling this function can cause an implicit
+ *   call to rpmh_rsc_invalidate().  Unless you know for sure that you have
+ *   an active TCS you should assume that you need to re-write sleep/wake
+ *   values after calling this function.
  *
  * Return: 0 on success, -EINVAL on error.
- * Note: This call blocks until a valid data is written to the TCS.
  */
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 {
@@ -485,6 +635,63 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return ret;
 }
 
+/**
+ * find_match() - Find if the cmd sequence is in the tcs_group
+ * @tcs: The tcs_group to search.  Either sleep or wake.
+ * @cmd: The command sequence to search for; only addr is looked at.
+ * @len: The number of commands in the sequence.
+ *
+ * Searches through the given tcs_group to see if a given command sequence
+ * is in there.
+ *
+ * Two sequences are matches if they modify the same set of addresses in
+ * the same order.  The value of the data is not considered when deciding if
+ * two things are matches.
+ *
+ * How this function works is best understood by example.  For our example,
+ * we'll imagine our tcs group contains these (cmd, data) tuples:
+ *   [(a, A), (b, B), (c, C), (d, D), (e, E), (f, F), (g, G), (h, H)]
+ * ...in other words it has an element where (addr=a, data=A), etc.
+ * ...we'll assume that there is one TCS in the group that can store 8 commands.
+ *
+ * - find_match([(a, X)]) => 0
+ * - find_match([(c, X), (d, X)]) => 2
+ * - find_match([(c, X), (d, X), (e, X)]) => 2
+ * - find_match([(z, X)]) => -ENODATA
+ * - find_match([(a, X), (y, X)]) => -EINVAL (and warning printed)
+ * - find_match([(g, X), (h, X), (i, X)]) => -EINVAL (and warning printed)
+ * - find_match([(y, X), (a, X)]) => -ENODATA
+ *
+ * NOTE: This function overall seems like it has questionable value.
+ * - It can be used to update a message in the TCS with new data, but I
+ *   don't believe we actually do that--we always fully invalidate and
+ *   re-write everything.  Specifically it would be too limiting to force
+ *   someone not to change the set of addresses written to each time.
+ * - This function could be attempting to avoid writing different data to
+ *   the same address twice in a tcs_group.  If that's the goal, it doesn't
+ *   do a great job since find_match([(y, X), (a, X)]) return -ENODATA in my
+ *   above example.
+ * - If you originally wrote [(a, A), (b, B), (c, C)] and later tried to
+ *   write [(a, A), (b, B)] it'd look like a match and we wouldn't consider
+ *   it an error that the size got shorter.
+ * - If two clients wrote sequences that happened to be placed in slots next
+ *   to each other then a later check could match a sequence that was the
+ *   size of both together.
+ *
+ * TODO: in light of the above, prehaps we can just remove this function?
+ * If we later come up with fancy algorithms for updating everything without
+ * full invalidations we can come up with something then.
+ *
+ * Only for use on sleep/wake TCSs since those are the only ones we maintain
+ * tcs->slots and tcs->cmd_cache for.
+ *
+ * Must be called with the tcs_lock for the group held.
+ *
+ * Return: If the given command sequence wasn't in the tcs_group: -ENODATA.
+ *         If the given command sequence was in the tcs_group: the index of
+ *         the slot in the tcs_group where the first command is.
+ *         In some error cases (see above), -EINVAL.
+ */
 static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
 		      int len)
 {
@@ -497,6 +704,11 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
 		if (i + len >= tcs->num_tcs * tcs->ncpt)
 			goto seq_err;
 		for (j = 0; j < len; j++) {
+			/*
+			 * TODO: it's actually not valid to look at
+			 * "cmd_cache[x]" if "slots[x]" doesn't have a bit
+			 * set.  Should add a check.
+			 */
 			if (tcs->cmd_cache[i + j] != cmd[j].addr)
 				goto seq_err;
 		}
@@ -510,6 +722,23 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
 	return -EINVAL;
 }
 
+/**
+ * find_slots() - Find a place to write the given message.
+ * @tcs:    The tcs group to search.
+ * @msg:    The message we want to find room for.
+ * @tcs_id: If we return 0 from the function, we return the global ID of the
+ *          TCS to write to here.
+ * @cmd_id: If we return 0 from the function, we return the index of
+ *          the command array of the returned TCS where the client should
+ *          start writing the message.
+ *
+ * Only for use on sleep/wake TCSs since those are the only ones we maintain
+ * tcs->slots and tcs->cmd_cache for.
+ *
+ * Must be called with the tcs_lock for the group held.
+ *
+ * Return: -ENOMEM if there was no room, else 0.
+ */
 static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 		      int *tcs_id, int *cmd_id)
 {
@@ -521,7 +750,7 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 	if (slot >= 0)
 		goto copy_data;
 
-	/* Do over, until we can fit the full payload in a TCS */
+	/* Do over, until we can fit the full payload in a single TCS */
 	do {
 		slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
 						  i, msg->num_cmds, 0);
@@ -544,12 +773,13 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 }
 
 /**
- * rpmh_rsc_write_ctrl_data: Write request to the controller
- *
- * @drv: the controller
- * @msg: the data to be written to the controller
+ * rpmh_rsc_write_ctrl_data() - Write request to controller but don't trigger.
+ * @drv: The controller.
+ * @msg: The data to be written to the controller.
  *
  * There is no response returned for writing the request to the controller.
+ *
+ * Return: 0 if no error; else -error.
  */
 int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 {
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (4 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01 11:38   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

tcs_is_free() had two checks in it: does the software think that the
TCS is free and does the hardware think that the TCS is free.  Let's
comment this and also add a warning in the case that software and
hardware disagree, at least for ACTIVE_ONLY TCS.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Comment tcs_is_free() new for v2; replaces old patch 6.

 drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 9d2669cbd994..93f5d1fb71ca 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -181,8 +181,27 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
  */
 static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 {
-	return !test_bit(tcs_id, drv->tcs_in_use) &&
-	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
+	/* If software thinks it's in use then it's definitely in use */
+	if (test_bit(tcs_id, drv->tcs_in_use))
+		return false;
+
+	/* If hardware agrees it's free then it's definitely free */
+	if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0)
+		return true;
+
+	/*
+	 * If we're here then software and hardware disagree about whether
+	 * the TCS is free.  Software thinks it is free and hardware thinks
+	 * it is not.
+	 *
+	 * Maybe this should be a warning in all cases, but it's almost
+	 * certainly a warning for the ACTIVE_TCS where nobody else should
+	 * be doing anything else behind our backs.  For now we'll just
+	 * warn there and then still return that we're in use.
+	 */
+	WARN(drv->tcs[tcs_id].type == ACTIVE_TCS,
+	     "Driver thought TCS was free but HW reported busy\n");
+	return false;
 }
 
 /**
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (5 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-04-01 12:40   ` Maulik Shah
  2020-03-11 23:13 ` [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

tcs_write() is documented to only be useful for writing
RPMH_ACTIVE_ONLY_STATE.  Let's be loud if someone messes up.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2: None

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

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 93f5d1fb71ca..ba489d18c20e 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -573,6 +573,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	unsigned long flags;
 	int ret;
 
+	/*
+	 * It only makes sense to grab a whole TCS for ourselves if we're
+	 * triggering right away, which we only do for ACTIVE_ONLY.
+	 */
+	WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE);
+
 	/* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (6 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-03-26 21:44   ` Doug Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 09/10] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 10/10] drivers: qcom: rpmh-rsc: Always use -EAGAIN, never -EBUSY Douglas Anderson
  9 siblings, 1 reply; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

Auditing tcs_invalidate() made me worried.  Specifically I saw that it
used spin_lock(), not spin_lock_irqsave().  That always worries me.

As I understand it, using spin_lock() is only valid in these
situations:

a) You know you are running in the interrupt handler (and all other
   users of the lock use the "irqsave" variant).
b) You know that nobody using the lock is ever running in the
   interrupt handler.
c) You know that someone else has always disabled interrupts before
   your code runs and thus the "irqsave" variant is pointless.

From auditing the driver we look OK.  ...except that there is one
further corner case.  If sometimes your code is called with IRQs
disabled and sometimes it's not you will get in trouble if someone
ever boots your board with "nosmp" (AKA in uniprocessor mode).  In
such a case if someone else has the lock (without disabling
interrupts) and they get swapped out then your code (with interrupts
disabled) might loop forever waiting for the spinlock.

It's just safer to use the irqsave version, so let's do that.  In
future patches I believe tcs_invalidate() will always be called with
interrupts off anyway.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index ba489d18c20e..c82c734788b1 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -218,9 +218,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
 static int tcs_invalidate(struct rsc_drv *drv, int type)
 {
 	int m;
+	unsigned long flags;
 	struct tcs_group *tcs = &drv->tcs[type];
 
-	spin_lock(&tcs->lock);
+	spin_lock_irqsave(&tcs->lock, flags);
 	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
 		spin_unlock(&tcs->lock);
 		return 0;
@@ -235,7 +236,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
 	}
 	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
-	spin_unlock(&tcs->lock);
+	spin_unlock_irqrestore(&tcs->lock, flags);
 
 	return 0;
 }
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 09/10] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (7 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  2020-03-11 23:13 ` [RFT PATCH v2 10/10] drivers: qcom: rpmh-rsc: Always use -EAGAIN, never -EBUSY Douglas Anderson
  9 siblings, 0 replies; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

As talked about in the comments introduced by the patch ("drivers:
qcom: rpmh-rsc: A lot of comments"), the find_match() function()
didn't seem very sensible.  Remove it and the cmd_cache array that it
needed.

Nothing should stop us from exploring some fancy ways to avoid fully
invalidating the whole sleep/wake TCSs all the time in the future, but
find_match() doesn't seem well enough thought out to keep while we
wait for something better to arrive.

This patch isn't quite a no-op.  Specifically:

* It should be a slight performance boost of not searching through so
  many arrays.
* There is slightly less self-checking of commands written to the
  sleep/wake sets.  If it truly is an error to write to the same
  address more than once in a tcs_group then some cases (but not all)
  would have been caught before.

[1] https://lore.kernel.org/r/1583428023-19559-1-git-send-email-mkshah@codeaurora.org

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
It's possible that this might need the latest version of Maulik's
rpmh.c patches to work properly so we can really be sure that we
always invalidate before we flush.

Changes in v2:
- Got rid of useless "if (x) continue" at end of for loop.

 drivers/soc/qcom/rpmh-internal.h |   2 -
 drivers/soc/qcom/rpmh-rsc.c      | 111 +------------------------------
 2 files changed, 1 insertion(+), 112 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index b756d3036e96..07dfa393bb34 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -36,7 +36,6 @@ struct rsc_drv;
  *                    trigger
  *             End: get irq, access req,
  *                  grab drv->lock, clear tcs_in_use, drop drv->lock
- * @cmd_cache: Flattened cache of cmds in sleep/wake TCS; num_tcs * ncpt big.
  * @slots:     Indicates which of @cmd_addr are occupied; only used for
  *             SLEEP / WAKE TCSs.  Things are tightly packed in the
  *             case that (ncpt < MAX_CMDS_PER_TCS).  That is if ncpt = 2 and
@@ -51,7 +50,6 @@ struct tcs_group {
 	int ncpt;
 	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
-	u32 *cmd_cache;
 	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
 };
 
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index c82c734788b1..abbd8b158a63 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -661,93 +661,6 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return ret;
 }
 
-/**
- * find_match() - Find if the cmd sequence is in the tcs_group
- * @tcs: The tcs_group to search.  Either sleep or wake.
- * @cmd: The command sequence to search for; only addr is looked at.
- * @len: The number of commands in the sequence.
- *
- * Searches through the given tcs_group to see if a given command sequence
- * is in there.
- *
- * Two sequences are matches if they modify the same set of addresses in
- * the same order.  The value of the data is not considered when deciding if
- * two things are matches.
- *
- * How this function works is best understood by example.  For our example,
- * we'll imagine our tcs group contains these (cmd, data) tuples:
- *   [(a, A), (b, B), (c, C), (d, D), (e, E), (f, F), (g, G), (h, H)]
- * ...in other words it has an element where (addr=a, data=A), etc.
- * ...we'll assume that there is one TCS in the group that can store 8 commands.
- *
- * - find_match([(a, X)]) => 0
- * - find_match([(c, X), (d, X)]) => 2
- * - find_match([(c, X), (d, X), (e, X)]) => 2
- * - find_match([(z, X)]) => -ENODATA
- * - find_match([(a, X), (y, X)]) => -EINVAL (and warning printed)
- * - find_match([(g, X), (h, X), (i, X)]) => -EINVAL (and warning printed)
- * - find_match([(y, X), (a, X)]) => -ENODATA
- *
- * NOTE: This function overall seems like it has questionable value.
- * - It can be used to update a message in the TCS with new data, but I
- *   don't believe we actually do that--we always fully invalidate and
- *   re-write everything.  Specifically it would be too limiting to force
- *   someone not to change the set of addresses written to each time.
- * - This function could be attempting to avoid writing different data to
- *   the same address twice in a tcs_group.  If that's the goal, it doesn't
- *   do a great job since find_match([(y, X), (a, X)]) return -ENODATA in my
- *   above example.
- * - If you originally wrote [(a, A), (b, B), (c, C)] and later tried to
- *   write [(a, A), (b, B)] it'd look like a match and we wouldn't consider
- *   it an error that the size got shorter.
- * - If two clients wrote sequences that happened to be placed in slots next
- *   to each other then a later check could match a sequence that was the
- *   size of both together.
- *
- * TODO: in light of the above, prehaps we can just remove this function?
- * If we later come up with fancy algorithms for updating everything without
- * full invalidations we can come up with something then.
- *
- * Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
- *
- * Must be called with the tcs_lock for the group held.
- *
- * Return: If the given command sequence wasn't in the tcs_group: -ENODATA.
- *         If the given command sequence was in the tcs_group: the index of
- *         the slot in the tcs_group where the first command is.
- *         In some error cases (see above), -EINVAL.
- */
-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 >= tcs->num_tcs * tcs->ncpt)
-			goto seq_err;
-		for (j = 0; j < len; j++) {
-			/*
-			 * TODO: it's actually not valid to look at
-			 * "cmd_cache[x]" if "slots[x]" doesn't have a bit
-			 * set.  Should add a check.
-			 */
-			if (tcs->cmd_cache[i + j] != cmd[j].addr)
-				goto seq_err;
-		}
-		return i;
-	}
-
-	return -ENODATA;
-
-seq_err:
-	WARN(1, "Message does not match previous sequence.\n");
-	return -EINVAL;
-}
-
 /**
  * find_slots() - Find a place to write the given message.
  * @tcs:    The tcs group to search.
@@ -759,7 +672,7 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
  *          start writing the message.
  *
  * Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
+ * tcs->slots for.
  *
  * Must be called with the tcs_lock for the group held.
  *
@@ -771,11 +684,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 	int slot, offset;
 	int i = 0;
 
-	/* Find if we already have the msg in our TCS */
-	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 single TCS */
 	do {
 		slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
@@ -785,11 +693,7 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
 		i += tcs->ncpt;
 	} while (slot + msg->num_cmds - 1 >= i);
 
-copy_data:
 	bitmap_set(tcs->slots, slot, msg->num_cmds);
-	/* Copy the addresses of the resources over to the slots */
-	for (i = 0; i < msg->num_cmds; i++)
-		tcs->cmd_cache[slot + i] = msg->cmds[i].addr;
 
 	offset = slot / tcs->ncpt;
 	*tcs_id = offset + tcs->offset;
@@ -913,19 +817,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 		tcs->mask = ((1 << tcs->num_tcs) - 1) << st;
 		tcs->offset = st;
 		st += tcs->num_tcs;
-
-		/*
-		 * Allocate memory to cache sleep and wake requests to
-		 * avoid reading TCS register memory.
-		 */
-		if (tcs->type == ACTIVE_TCS)
-			continue;
-
-		tcs->cmd_cache = devm_kcalloc(&pdev->dev,
-					      tcs->num_tcs * ncpt, sizeof(u32),
-					      GFP_KERNEL);
-		if (!tcs->cmd_cache)
-			return -ENOMEM;
 	}
 
 	drv->num_tcs = st;
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [RFT PATCH v2 10/10] drivers: qcom: rpmh-rsc: Always use -EAGAIN, never -EBUSY
  2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
                   ` (8 preceding siblings ...)
  2020-03-11 23:13 ` [RFT PATCH v2 09/10] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
@ 2020-03-11 23:13 ` Douglas Anderson
  9 siblings, 0 replies; 19+ messages in thread
From: Douglas Anderson @ 2020-03-11 23:13 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd,
	Douglas Anderson, linux-arm-msm, linux-kernel

Some parts of rpmh-rsc returned -EAGAIN when the controller was busy
and you should try again.  Other parts returned -EBUSY when the
controller was busy and you should try again.  Typically -EAGAIN was
used when dealing with sleep/wake TCSs and -EBUSY was used when
dealing with the active TCS.

Let's standardize and just have one return code.

If we don't do this then the crossover case where we need to use a
sleep/wake TCS for an active only transfer (when there are zero active
TCSs) we need to either adapt one code to the other test for both.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("Always use -EAGAIN, never -EBUSY") new for v2.

 drivers/soc/qcom/rpmh-rsc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index abbd8b158a63..8b59d07ef94e 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -497,11 +497,11 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
  *
  * This will walk through the TCSs in the group and check if any of them
  * appear to be sending to addresses referenced in the message.  If it finds
- * one it'll return -EBUSY.
+ * one it'll return -EAGAIN.
  *
  * Must be called with the drv->lock held since that protects tcs_in_use.
  *
- * Return: 0 if nothing in flight or -EBUSY if we should try again later.
+ * Return: 0 if nothing in flight or -EAGAIN if we should try again later.
  *         The caller must re-enable interrupts between tries since that's
  *         the only way tcs_is_free() will ever return true and the only way
  *         RSC_DRV_CMD_ENABLE will ever be cleared.
@@ -524,7 +524,7 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 			addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
 			for (k = 0; k < msg->num_cmds; k++) {
 				if (addr == msg->cmds[k].addr)
-					return -EBUSY;
+					return -EAGAIN;
 			}
 		}
 	}
@@ -550,21 +550,21 @@ static int find_free_tcs(struct tcs_group *tcs)
 			return tcs->offset + i;
 	}
 
-	return -EBUSY;
+	return -EAGAIN;
 }
 
 /**
- * tcs_write() - Store messages into a TCS right now, or return -EBUSY.
+ * tcs_write() - Store messages into a TCS right now, or return -EAGAIN.
  * @drv: The controller.
  * @msg: The data to be sent.
  *
  * Grabs a TCS for ACTIVE_ONLY transfers and writes the messages to it.
  *
  * If there are no free ACTIVE_ONLY TCSs or if a command for the same address
- * is already transferring returns -EBUSY which means the client should retry
+ * is already transferring returns -EAGAIN which means the client should retry
  * shortly.
  *
- * Return: 0 on success, -EBUSY if client should retry, or an error.
+ * Return: 0 on success, -EAGAIN if client should retry, or an error.
  *         Client should have interrupts enabled for a bit before retrying.
  */
 static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
@@ -580,7 +580,6 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	 */
 	WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE);
 
-	/* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);
@@ -651,12 +650,12 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 
 	do {
 		ret = tcs_write(drv, msg);
-		if (ret == -EBUSY) {
+		if (ret == -EAGAIN) {
 			pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=%#x\n",
 					    msg->cmds[0].addr);
 			udelay(10);
 		}
-	} while (ret == -EBUSY);
+	} while (ret == -EAGAIN);
 
 	return ret;
 }
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()
  2020-03-11 23:13 ` [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
@ 2020-03-26 21:44   ` Doug Anderson
  0 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2020-03-26 21:44 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Maulik Shah
  Cc: Matthias Kaehlcke, Rajendra Nayak, Evan Green, Lina Iyer,
	Stephen Boyd, linux-arm-msm, LKML

Hi,

On Wed, Mar 11, 2020 at 4:14 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Auditing tcs_invalidate() made me worried.  Specifically I saw that it
> used spin_lock(), not spin_lock_irqsave().  That always worries me.
>
> As I understand it, using spin_lock() is only valid in these
> situations:
>
> a) You know you are running in the interrupt handler (and all other
>    users of the lock use the "irqsave" variant).
> b) You know that nobody using the lock is ever running in the
>    interrupt handler.
> c) You know that someone else has always disabled interrupts before
>    your code runs and thus the "irqsave" variant is pointless.
>
> From auditing the driver we look OK.  ...except that there is one
> further corner case.  If sometimes your code is called with IRQs
> disabled and sometimes it's not you will get in trouble if someone
> ever boots your board with "nosmp" (AKA in uniprocessor mode).  In
> such a case if someone else has the lock (without disabling
> interrupts) and they get swapped out then your code (with interrupts
> disabled) might loop forever waiting for the spinlock.
>
> It's just safer to use the irqsave version, so let's do that.  In
> future patches I believe tcs_invalidate() will always be called with
> interrupts off anyway.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2: None
>
>  drivers/soc/qcom/rpmh-rsc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index ba489d18c20e..c82c734788b1 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -218,9 +218,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>  static int tcs_invalidate(struct rsc_drv *drv, int type)
>  {
>         int m;
> +       unsigned long flags;
>         struct tcs_group *tcs = &drv->tcs[type];
>
> -       spin_lock(&tcs->lock);
> +       spin_lock_irqsave(&tcs->lock, flags);
>         if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>                 spin_unlock(&tcs->lock);

Noticed a bug while doing a code review of:

https://lkml.kernel.org/r/1585244270-637-7-git-send-email-mkshah@codeaurora.org

...specifically my patch forgets to change the error case to
spin_unlock_irqrestore().  ...but perhaps if that other patch lands
when we can just remove the spinlocks from this function...  I'll post
more in my reply to that other patch.


-Doug

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds
  2020-03-11 23:13 ` [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
@ 2020-04-01  8:12   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01  8:12 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

nit: can you please change as below, to mention its for TCS
drivers: qcom: rpmh-rsc: Clean code reading/writing TCS regs/cmds

Reviewed and tested.

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> This patch makes two changes, both of which should be no-ops:
>
> 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() /
>     write_tcs_cmd().
>
> 2. Change the order of operations in the above functions to make it
>     more obvious to me what the math is doing.  Specifically first you
>     want to find the right TCS, then the right register, and then
>     multiply by the command ID if necessary.
>
> Signed-off-by: Douglas Anderson<dianders@chromium.org>
> ---
>
> Changes in v2: None
>
>   drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index b71822131f59..b87b79f0347d 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -61,28 +61,33 @@
>   #define CMD_STATUS_ISSUED		BIT(8)
>   #define CMD_STATUS_COMPL		BIT(16)
>   
> -static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
> +static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
>   {
> -	return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
> +	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
>   			     RSC_DRV_CMD_OFFSET * cmd_id);
>   }
>   
> +static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id)
> +{
> +	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
> +}
> +
>   static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id,
>   			  u32 data)
>   {
> -	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
> +	writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
>   		       RSC_DRV_CMD_OFFSET * cmd_id);
>   }
>   
>   static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data)
>   {
> -	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
> +	writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
>   }
>   
>   static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
>   			       u32 data)
>   {
> -	writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
> +	writel(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
>   	for (;;) {
>   		if (data == readl(drv->tcs_base + reg +
>   				  RSC_DRV_TCS_OFFSET * tcs_id))
> @@ -94,7 +99,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
>   static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>   {
>   	return !test_bit(tcs_id, drv->tcs_in_use) &&
> -	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
> +	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
>   }
>   
>   static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
> @@ -212,7 +217,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>   	const struct tcs_request *req;
>   	struct tcs_cmd *cmd;
>   
> -	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
> +	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0);
>   
>   	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
>   		req = get_req_from_tcs(drv, i);
> @@ -226,7 +231,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>   			u32 sts;
>   
>   			cmd = &req->cmds[j];
> -			sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, 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))) {
> @@ -265,7 +270,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   	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, 0);
> +	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];
> @@ -281,7 +286,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   	}
>   
>   	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, 0);
> +	cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
>   	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
>   }
>   
> @@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
>   	 * While clearing ensure that the AMC mode trigger is cleared
>   	 * and then the mode enable is cleared.
>   	 */
> -	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0);
> +	enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id);
>   	enable &= ~TCS_AMC_MODE_TRIGGER;
>   	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
>   	enable &= ~TCS_AMC_MODE_ENABLE;
> @@ -319,10 +324,10 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>   		if (tcs_is_free(drv, tcs_id))
>   			continue;
>   
> -		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
> +		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
>   
>   		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> -			addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
> +			addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
>   			for (k = 0; k < msg->num_cmds; k++) {
>   				if (addr == msg->cmds[k].addr)
>   					return -EBUSY;

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better
  2020-03-11 23:13 ` [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
@ 2020-04-01  8:14   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01  8:14 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> Perhaps it's just me, it took a really long time to understand what
> the register layout of rpmh-rsc was just from the #defines.  Let's add
> a bunch of comments describing which blocks are part of other blocks.
>
> Signed-off-by: Douglas Anderson<dianders@chromium.org>
> ---
>
> Changes in v2:
> - Now prose in comments instead of struct definitions.
> - Pretty ASCII art from Stephen.
>
>   drivers/soc/qcom/rpmh-rsc.c | 78 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index b87b79f0347d..02c8e0ffbbe4 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -37,14 +37,24 @@
>   #define DRV_NCPT_MASK			0x1F
>   #define DRV_NCPT_SHIFT			27
>   
> -/* Register offsets */
> +/*
> + * Register offsets within a TCS.

Change this to

/* Offsets for common TCS Registers, one bit per TCS */

> + *
> + * TCSs are stored one after another; multiply tcs_id by RSC_DRV_TCS_OFFSET
> + * to find a given TCS and add one of the below to find a register.
> + */
Move above comment after these 3 common IRQ registers.
>   #define RSC_DRV_IRQ_ENABLE		0x00
>   #define RSC_DRV_IRQ_STATUS		0x04
> -#define RSC_DRV_IRQ_CLEAR		0x08

please add line break between RSC_DRV_IRQ_CLEAR and 
RSC_DRV_CMD_WAIT_FOR_CMPL

you may want to add one more comment inbetween saying

/* Offsets for per TCS Registers */

> -#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10
> +#define RSC_DRV_IRQ_CLEAR		0x08	/* w/o; write 1 to clear */
> +#define RSC_DRV_CMD_WAIT_FOR_CMPL	0x10	/* 1 bit per command */
>   #define RSC_DRV_CONTROL			0x14
> -#define RSC_DRV_STATUS			0x18
> -#define RSC_DRV_CMD_ENABLE		0x1C
> +#define RSC_DRV_STATUS			0x18	/* zero if tcs is busy */
> +#define RSC_DRV_CMD_ENABLE		0x1C	/* 1 bit per command */
> +
> +/*
> + * Commands (up to 16) start at 0x30 in a TCS; multiply command index
> + * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
> + */
you may also add /* Offsets for per command in a TCS */
>   #define RSC_DRV_CMD_MSGID		0x30
>   #define RSC_DRV_CMD_ADDR		0x34
>   #define RSC_DRV_CMD_DATA		0x38
> @@ -61,6 +71,64 @@
>   #define CMD_STATUS_ISSUED		BIT(8)
>   #define CMD_STATUS_COMPL		BIT(16)
>   
> +/*
> + * Here's a high level overview of how all the registers in RPMH work
> + * together:
> + *
> + * - The main rpmh-rsc address is the base of a register space that can
> + *   be used to find overall configuration of the hardware
> + *   (DRV_PRNT_CHLD_CONFIG).  Also found within the rpmh-rsc register
> + *   space are all the TCS blocks.  The offset of the TCS blocks is
> + *   specified in the device tree by "qcom,tcs-offset" and used to
> + *   compute tcs_base.
> + * - TCS blocks come one after another.  Type, count, and order are
> + *   specified by the device tree as "qcom,tcs-config".
> + * - Each TCS block has some registers, then space for up to 16 commands.
> + *   Note that though address space is reserved for 16 commands, fewer
> + *   might be present.  See ncpt (num cmds per TCS).
> + * - The first TCS block is special in that it has registers to control
> + *   interrupts (RSC_DRV_IRQ_XXX).  Space for these registers is reserved
> + *   in all TCS blocks to make the math easier, but only the first one
> + *   matters.

First TCS block is not special, the RSC_DRV_IRQ_XXX registers are common 
for all

TCSes.  can you please drop this last paragraph and then add one more 
block in

ASCII diagram to have TCS common IRQ registers like below to represent 
it more clear.

+----------------------------------------------+
|TCS                                           |
| IRQ                                          |
|                                              |
| +------------------------------------------+ |
| |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  | |
| |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| |
| | ctrl |  |  |  |  |  |  |  |  |  |  |  |  | |
| +------------------------------------------+ |
+----------------------------------------------+


> + *
> + * Here's a picture:
> + *
> + *  +---------------------------------------------------+
> + *  |RSC                                                |
> + *  | ctrl                                              |
> + *  |                                                   |
> + *  | Drvs:                                             |
> + *  | +-----------------------------------------------+ |
> + *  | |DRV0                                           | |
> + *  | | ctrl                                          | |
> + *  | |                                               | |
> + *  | | TCSs:                                         | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS0  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | | IRQ  | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS1  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | | +------------------------------------------+  | |
> + *  | | |TCS2  |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | |      | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
> + *  | | | ctrl |  |  |  |  |  |  |  |  |  |  |  |  |  | |
> + *  | | +------------------------------------------+  | |
> + *  | |                    ......                     | |
> + *  | +-----------------------------------------------+ |
> + *  | +-----------------------------------------------+ |
> + *  | |DRV1                                           | |
> + *  | | (same as DRV0)                                | |
> + *  | +-----------------------------------------------+ |
> + *  |                      ......                       |
> + *  +---------------------------------------------------+
> + */
> +
> +

nit: two blank lines at the end, you can drop one.

Thanks,
Maulik
>   static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
>   {
>   	return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
  2020-03-11 23:13 ` [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
@ 2020-04-01  8:17   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01  8:17 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> I was trying to write documentation for the functions in rpmh-rsc and
> I got to tcs_ctrl_write().  The documentation for the function would
> have been: "This is the core of rpmh_rsc_write_ctrl_data(); all the
> caller does is error-check and then call this".
>
> Having the error checks in a separate function doesn't help for
> anything since:
> - There are no other callers that need to bypass the error checks.
> - It's less documenting.  When I read tcs_ctrl_write() I kept
>    wondering if I need to handle cases other than ACTIVE_ONLY or cases
>    with more commands than could fit in a TCS.  This is obvious when
>    the error checks and code are together.
> - The function just isn't that long, so there's no problem
>    understanding the combined function.
>
> Things were even more confusing because the two functions names didn't
> make obvious (at least to me) their relationship.
>
> Simplify by folding one function into the other.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>
> Changes in v2: None
>
>   drivers/soc/qcom/rpmh-rsc.c | 39 ++++++++++++++++---------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 02c8e0ffbbe4..799847b08038 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -550,27 +550,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
>   	return 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 TCS id and the command in the TCS to write to */
> -	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
> -	if (!ret)
> -		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> -	spin_unlock_irqrestore(&tcs->lock, flags);
> -
> -	return ret;
> -}
> -
>   /**
>    * rpmh_rsc_write_ctrl_data: Write request to the controller
>    *
> @@ -581,6 +560,11 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>    */
>   int rpmh_rsc_write_ctrl_data(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;
> +
>   	if (!msg || !msg->cmds || !msg->num_cmds ||
>   	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
>   		pr_err("Payload error\n");
> @@ -591,7 +575,18 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   	if (msg->state == RPMH_ACTIVE_ONLY_STATE)
>   		return -EINVAL;
>   
> -	return tcs_ctrl_write(drv, msg);
> +	tcs = get_tcs_for_msg(drv, msg);
> +	if (IS_ERR(tcs))
> +		return PTR_ERR(tcs);
> +
> +	spin_lock_irqsave(&tcs->lock, flags);
> +	/* find the TCS id and the command in the TCS to write to */
> +	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
> +	if (!ret)
> +		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> +	spin_unlock_irqrestore(&tcs->lock, flags);
> +
> +	return ret;
>   }
>   
>   static int rpmh_probe_tcs_config(struct platform_device *pdev,

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction
  2020-03-11 23:13 ` [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
@ 2020-04-01  8:18   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01  8:18 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> The get_tcs_of_type() function doesn't provide any value.  It's not
> conceptually difficult to access a value in an array, even if that
> value is in a structure and we want a pointer to the value.  Having
> the function in there makes me feel like it's doing something fancier
> like looping or searching.  Remove it.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>
> Changes in v2: None
>
>   drivers/soc/qcom/rpmh-rsc.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 799847b08038..c9f29cbd5ee5 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -170,17 +170,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>   	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
>   }
>   
> -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);
> +	struct tcs_group *tcs = &drv->tcs[type];
>   
>   	spin_lock(&tcs->lock);
>   	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> @@ -246,9 +239,9 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
>   	 * dedicated AMC, and therefore would invalidate the sleep and wake
>   	 * TCSes before making an active state request.
>   	 */
> -	tcs = get_tcs_of_type(drv, type);
> +	tcs = &drv->tcs[type];
>   	if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs) {
> -		tcs = get_tcs_of_type(drv, WAKE_TCS);
> +		tcs = &drv->tcs[WAKE_TCS];
>   		if (tcs->num_tcs) {
>   			ret = rpmh_rsc_invalidate(drv);
>   			if (ret)

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments
  2020-03-11 23:13 ` [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
@ 2020-04-01 11:29   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01 11:29 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> I've been pouring through the rpmh-rsc code and trying to understand
> it.  Document everything to the best of my ability.  All documentation
> here is strictly from code analysis--no actual knowledge of the
> hardware was used.  If something is wrong in here I either
> misunderstood the code, had a typo, or the code has a bug in it
> leading to my incorrect understanding.
>
> In a few places here I have documented things that don't make tons of
> sense.  A future patch will try to address this.  While this means I'm
> adding comments / todos and then later fixing them in the series, it
> seemed more urgent to get things documented first so that people could
> understand the later patches.
>
> This should be a no-op.  It's just comment changes.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - More clear that active-only xfers can happen on wake TCS sometimes.
> - Document locks for updating "tcs_in_use" more.
> - Document tcs_is_free() without drv->lock OK for tcs_invalidate().
> - Document bug of tcs_write() not handling -EAGAIN.
> - Document get_tcs_for_msg() => -EAGAIN only for ACTIVE_ONLY.
> - Reword tcs_write() doc a bit.
> - Document two get_tcs_for_msg() issues if zero-active TCS.
> - Document that rpmh_rsc_send_data() can be an implicit invalidate.
> - Fixed documentation of "tcs" param in find_slots().
>
>   drivers/soc/qcom/rpmh-internal.h |  52 +++---
>   drivers/soc/qcom/rpmh-rsc.c      | 264 +++++++++++++++++++++++++++++--
>   2 files changed, 281 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index 6eec32b97f83..b756d3036e96 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -22,16 +22,25 @@ struct rsc_drv;
>    * struct tcs_group: group of Trigger Command Sets (TCS) to send state requests
>    * to the controller
>    *
> - * @drv:       the controller
> - * @type:      type of the TCS in this group - active, sleep, wake
> - * @mask:      mask of the TCSes relative to all the TCSes in the RSC
> - * @offset:    start of the TCS group relative to the TCSes in the RSC
> - * @num_tcs:   number of TCSes in this type
> - * @ncpt:      number of commands in each TCS
> - * @lock:      lock for synchronizing this TCS writes
> - * @req:       requests that are sent from the TCS
> - * @cmd_cache: flattened cache of cmds in sleep/wake TCS
> - * @slots:     indicates which of @cmd_addr are occupied
> + * @drv:       The controller.
> + * @type:      Type of the TCS in this group - active, sleep, wake.
> + * @mask:      Mask of the TCSes relative to all the TCSes in the RSC.
> + * @offset:    Start of the TCS group relative to the TCSes in the RSC.
> + * @num_tcs:   Number of TCSes in this type.
> + * @ncpt:      Number of commands in each TCS.
> + * @lock:      Lock for synchronizing this TCS writes.
> + * @req:       Requests that are sent from the TCS; only used for ACTIVE_ONLY
> + *             transfers (could be on a wake/sleep TCS if we are borrowing for
> + *             an ACTIVE_ONLY transfer).
> + *             Start: grab drv->lock, set req, set tcs_in_use, drop drv->lock,
> + *                    trigger
> + *             End: get irq, access req,
> + *                  grab drv->lock, clear tcs_in_use, drop drv->lock
> + * @cmd_cache: Flattened cache of cmds in sleep/wake TCS; num_tcs * ncpt big.
> + * @slots:     Indicates which of @cmd_addr are occupied; only used for
> + *             SLEEP / WAKE TCSs.  Things are tightly packed in the
> + *             case that (ncpt < MAX_CMDS_PER_TCS).  That is if ncpt = 2 and
> + *             MAX_CMDS_PER_TCS = 16 then bit[2] = the first bit in 2nd TCS.
>    */
>   struct tcs_group {
>   	struct rsc_drv *drv;
> @@ -84,14 +93,21 @@ struct rpmh_ctrlr {
>    * struct rsc_drv: the Direct Resource Voter (DRV) of the
>    * Resource State Coordinator controller (RSC)
>    *
> - * @name:       controller identifier
> - * @tcs_base:   start address of the TCS registers in this controller
> - * @id:         instance id in the controller (Direct Resource Voter)
> - * @num_tcs:    number of TCSes in this DRV
> - * @tcs:        TCS groups
> - * @tcs_in_use: s/w state of the TCS
> - * @lock:       synchronize state of the controller
> - * @client:     handle to the DRV's client.
> + * @name:       Controller identifier.
> + * @tcs_base:   Start address of the TCS registers in this controller.
> + * @id:         Instance id in the controller (Direct Resource Voter).
> + * @num_tcs:    Number of TCSes in this DRV.
> + * @tcs:        TCS groups.
> + * @tcs_in_use: s/w state of the TCS; only set for ACTIVE_ONLY transfers, but
> + *              might show a sleep/wake TCS in use if it was borrowed for an
> + *              active_only transfer.  You must hold both the lock in this
> + *              struct and the tcs_lock for the TCS in order to mark a TCS as
> + *              in-use, but you only need the lock in this structure to mark
> + *              one freed.
> + * @lock:       Synchronize state of the controller.  If you will be grabbing
> + *              this lock and a tcs_lock at the same time, grab the tcs_lock
> + *              first so we always have a consistent lock ordering.
> + * @client:     Handle to the DRV's client.
>    */
>   struct rsc_drv {
>   	const char *name;
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index c9f29cbd5ee5..9d2669cbd994 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -164,12 +164,38 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
>   	}
>   }
>   
> +/**
> + * tcs_is_free() - Return if a TCS is totally free.
> + * @drv:    The RSC controller.
> + * @tcs_id: The global ID of this TCS.
> + *
> + * Returns true if nobody has claimed this TCS (by setting tcs_in_use).
> + * If the TCS looks free, checks that the hardware agrees.
> + *
> + * Must be called with the drv->lock held or the tcs_lock for the TCS being
> + * tested.  If only the tcs_lock is held then it is possible that this
> + * function will return that a tcs is still busy when it has been recently
> + * been freed but it will never return free when a TCS is actually in use.
> + *
> + * Return: true if the given TCS is free.
> + */
>   static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>   {
>   	return !test_bit(tcs_id, drv->tcs_in_use) &&
>   	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
>   }
>   
> +/**
> + * tcs_invalidate() - Invalidate all TCSs of the given type (sleep or wake).
> + * @drv:  The RSC controller.
> + * @type: SLEEP_TCS or WAKE_TCS
> + *
> + * This will clear the "slots" variable of the given tcs_group and also
> + * tell the hardware to forget about all entries.
> + *
> + * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
> + *         bit.  Caller should make sure to enable interrupts between tries.
> + */
>   static int tcs_invalidate(struct rsc_drv *drv, int type)
>   {
>   	int m;
> @@ -196,9 +222,11 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
>   }
>   
>   /**
> - * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
> + * rpmh_rsc_invalidate() - Invalidate sleep and wake TCSes.
> + * @drv: The RSC controller.
>    *
> - * @drv: the RSC controller
> + * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
> + *         bit.  Caller should make sure to enable interrupts between tries.
>    */
>   int rpmh_rsc_invalidate(struct rsc_drv *drv)
>   {
> @@ -211,6 +239,20 @@ int rpmh_rsc_invalidate(struct rsc_drv *drv)
>   	return ret;
>   }
>   
> +/**
> + * get_tcs_for_msg() - Get the tcs_group used to send the given message.
> + * @drv: The RSC controller.
> + * @msg: The message we want to send.
> + *
> + * This is normally pretty straightforward except if we are trying to send
> + * an ACTIVE_ONLY message but don't have any active_only TCSs.
> + *
> + * Called without drv->lock held and with no tcs_lock locks held.
> + *
> + * Return: 0 if no problem, or -EAGAIN if the caller should try again in a bit.
> + *         Caller should make sure to enable interrupts between tries.
> + *         Only ever returns -EAGAIN for ACTIVE_ONLY transfers.
with [2] it will not return -EAGAIN, can you please remove this part.
> + */
>   static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
>   					 const struct tcs_request *msg)
>   {
> @@ -246,12 +288,35 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
>   			ret = rpmh_rsc_invalidate(drv);
>   			if (ret)
>   				return ERR_PTR(ret);
> +
> +			/*
> +			 * TODO:
> +			 * - Temporarily enable IRQs on the wake tcs.
> +			 * - Make sure nobody else race with us and re-write
> +			 *   to this TCS; document how this works.
You can remove above comment, i already included change to enable IRQs 
on wake TCS in my series.
> +			 */
>   		}
>   	}
>   
>   	return tcs;
>   }
>   
> +/**
> + * get_req_from_tcs() - Get a stashed request that was xfering on the given tcs.
> + * @drv:    The RSC controller.
> + * @tcs_id: The global ID of this TCS.
> + *
> + * For ACTIVE_ONLY transfers we want to call back into the client when the
> + * transfer finishes.  To do this we need the "request" that the client
> + * originally provided us.  This function grabs the request that we stashed
> + * when we started the transfer.
> + *
> + * This only makes sense for ACTIVE_ONLY transfers since those are the only
> + * ones we track sending (the only ones we enable interrupts for and the only
> + * ones we call back to the client for).
> + *
> + * Return: The stashed request.
> + */
>   static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
>   						  int tcs_id)
>   {
> @@ -268,7 +333,14 @@ static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
>   }
>   
>   /**
> - * tcs_tx_done: TX Done interrupt handler
> + * tcs_tx_done() - TX Done interrupt handler.
> + * @irq: The IRQ number (ignored).
> + * @p:   Pointer to "struct rsc_drv".
> + *
> + * Called for ACTIVE_ONLY TCSs (those are the only ones we enable the IRQ for)
> + * when a transfer is done.
> + *
> + * Return: IRQ_HANDLED
>    */
>   static irqreturn_t tcs_tx_done(int irq, void *p)
>   {
> @@ -278,6 +350,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>   	const struct tcs_request *req;
>   	struct tcs_cmd *cmd;
>   
> +	/* NOTE: interrupt status for all TCSs are found in TCS 0 */
>   	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0);
>   
>   	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
> @@ -318,6 +391,16 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>   	return IRQ_HANDLED;
>   }
>   
> +/**
> + * __tcs_buffer_write() - Write to TCS hardware from a request; don't trigger.
> + * @drv:    The controller.
> + * @tcs_id: The global ID of this TCS.
> + * @cmd_id: The index within the TCS to start writing.
> + * @msg:    The message we want to send, which will contain several addr/data
> + *          pairs to program (but few enough that they all fit in one TCS).
> + *
> + * This is used for all types of TCSs (active, sleep, and wake).
> + */
>   static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   			       const struct tcs_request *msg)
>   {
> @@ -351,6 +434,15 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
>   }
>   
> +/**
> + * __tcs_trigger() - Start transferring on the given TCS.
> + *
> + * The TCS given is probably the active-only one, but could be a wake one
> + * that we borrowed if there are zero active-only TCSs.
> + *
> + * @drv:    The controller.
> + * @tcs_id: The global ID of this TCS.
> + */
>   static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
>   {
>   	u32 enable;
> @@ -373,6 +465,27 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
>   	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
>   }
>   
> +/**
> + * check_for_req_inflight() - Look to see if conflicting cmds are in flight.
> + * @drv: The controller.
> + * @tcs: A pointer to the tcs_group used for ACTIVE_ONLY transfers.
> + * @msg: The message we want to send, which will contain several addr/data
> + *       pairs to program (but few enough that they all fit in one TCS).
> + *
> + * Only for use for ACTIVE_ONLY tcs_group, since those are the only ones
> + * that might be actively sending.

This comment will need to modify/removed after we use this in place of 
find_match().

see below for more details.

> + *
> + * This will walk through the TCSs in the group and check if any of them
> + * appear to be sending to addresses referenced in the message.  If it finds
> + * one it'll return -EBUSY.
> + *
> + * Must be called with the drv->lock held since that protects tcs_in_use.
> + *
> + * Return: 0 if nothing in flight or -EBUSY if we should try again later.
> + *         The caller must re-enable interrupts between tries since that's
> + *         the only way tcs_is_free() will ever return true and the only way
> + *         RSC_DRV_CMD_ENABLE will ever be cleared.
> + */
>   static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>   				  const struct tcs_request *msg)
>   {
> @@ -399,6 +512,15 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>   	return 0;
>   }
>   
> +/**
> + * find_free_tcs() - Find free tcs in the given tcs_group; only for ACTIVE_ONLY.
> + * @tcs: A pointer to the ACTIVE_ONLY tcs_group (or the wake tcs_group if
> + *       we borrowed it because there are zero active-only ones).
> + *
> + * Must be called with the drv->lock held since that protects tcs_in_use.
> + *
> + * Return: The first tcs that's free.
> + */
>   static int find_free_tcs(struct tcs_group *tcs)
>   {
>   	int i;
> @@ -411,6 +533,20 @@ static int find_free_tcs(struct tcs_group *tcs)
>   	return -EBUSY;
>   }
>   
> +/**
> + * tcs_write() - Store messages into a TCS right now, or return -EBUSY.
> + * @drv: The controller.
> + * @msg: The data to be sent.
> + *
> + * Grabs a TCS for ACTIVE_ONLY transfers and writes the messages to it.
> + *
> + * If there are no free ACTIVE_ONLY TCSs or if a command for the same address
> + * is already transferring returns -EBUSY which means the client should retry
> + * shortly.
> + *
> + * Return: 0 on success, -EBUSY if client should retry, or an error.
> + *         Client should have interrupts enabled for a bit before retrying.
> + */
>   static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>   {
>   	struct tcs_group *tcs;
> @@ -418,16 +554,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>   	unsigned long flags;
>   	int ret;
>   
> +	/* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */
with [2] it will not return -EAGAIN, can you please remove this comment.
>   	tcs = get_tcs_for_msg(drv, msg);
>   	if (IS_ERR(tcs))
>   		return PTR_ERR(tcs);
>   
>   	spin_lock_irqsave(&tcs->lock, flags);
> +
>   	spin_lock(&drv->lock);
> -	/*
> -	 * The h/w does not like if we send a request to the same address,
> -	 * when one is already in-flight or being processed.
> -	 */
please keep above comment as it is.
>   	ret = check_for_req_inflight(drv, tcs, msg);
>   	if (ret) {
>   		spin_unlock(&drv->lock);
> @@ -454,14 +588,30 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>   }
>   
>   /**
> - * rpmh_rsc_send_data: Validate the incoming message and write to the
> - * appropriate TCS block.
> + * rpmh_rsc_send_data() - Validate the incoming message + write to TCS block.
> + * @drv: The controller.
> + * @msg: The data to be sent.
>    *
> - * @drv: the controller
> - * @msg: the data to be sent
> + * NOTES:
> + * - This is only used for "ACTIVE_ONLY" since the limitations of this
> + *   function don't make sense for sleep/wake cases.
> + * - To do the transfer, we will grab a whole TCS for ourselves--we don't
> + *   try to share.  If there are none available we'll wait indefinitely
> + *   for a free one.
> + * - This function will not wait for the commands to be finished, only for
> + *   data to be programmed into the RPMh.  See rpmh_tx_done() which will
> + *   be called when the transfer is fully complete.
> + * - This function must be called with interrupts enabled.  If the hardware
> + *   is busy doing someone else's transfer we need that transfer to fully
> + *   finish so that we can have the hardware, and to fully finish it needs
> + *   the interrupt handler to run.  If the interrupts is set to run on the
> + *   active CPU this can never happen if interrupts are disabled.
> + * - If there are no active TCS calling this function can cause an implicit
> + *   call to rpmh_rsc_invalidate().  Unless you know for sure that you have
> + *   an active TCS you should assume that you need to re-write sleep/wake
> + *   values after calling this function.
>    *
>    * Return: 0 on success, -EINVAL on error.
> - * Note: This call blocks until a valid data is written to the TCS.
>    */
>   int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   {
> @@ -485,6 +635,63 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   	return ret;
>   }
>   
> +/**
> + * find_match() - Find if the cmd sequence is in the tcs_group
> + * @tcs: The tcs_group to search.  Either sleep or wake.
> + * @cmd: The command sequence to search for; only addr is looked at.
> + * @len: The number of commands in the sequence.
> + *
> + * Searches through the given tcs_group to see if a given command sequence
> + * is in there.
> + *
> + * Two sequences are matches if they modify the same set of addresses in
> + * the same order.  The value of the data is not considered when deciding if
> + * two things are matches.
> + *
> + * How this function works is best understood by example.  For our example,
> + * we'll imagine our tcs group contains these (cmd, data) tuples:
> + *   [(a, A), (b, B), (c, C), (d, D), (e, E), (f, F), (g, G), (h, H)]
> + * ...in other words it has an element where (addr=a, data=A), etc.
> + * ...we'll assume that there is one TCS in the group that can store 8 commands.
> + *
> + * - find_match([(a, X)]) => 0
> + * - find_match([(c, X), (d, X)]) => 2
> + * - find_match([(c, X), (d, X), (e, X)]) => 2
> + * - find_match([(z, X)]) => -ENODATA
> + * - find_match([(a, X), (y, X)]) => -EINVAL (and warning printed)
> + * - find_match([(g, X), (h, X), (i, X)]) => -EINVAL (and warning printed)
> + * - find_match([(y, X), (a, X)]) => -ENODATA
> + *
> + * NOTE: This function overall seems like it has questionable value.
> + * - It can be used to update a message in the TCS with new data, but I
> + *   don't believe we actually do that--we always fully invalidate and
> + *   re-write everything.
we are doing this from [1] onwards.
> Specifically it would be too limiting to force
> + *   someone not to change the set of addresses written to each time.
> + * - This function could be attempting to avoid writing different data to
> + *   the same address twice in a tcs_group.  If that's the goal, it doesn't
> + *   do a great job since find_match([(y, X), (a, X)]) return -ENODATA in my
> + *   above example.
> + * - If you originally wrote [(a, A), (b, B), (c, C)] and later tried to
> + *   write [(a, A), (b, B)] it'd look like a match and we wouldn't consider
> + *   it an error that the size got shorter.
> + * - If two clients wrote sequences that happened to be placed in slots next
> + *   to each other then a later check could match a sequence that was the
> + *   size of both together.
> + *
> + * TODO: in light of the above, prehaps we can just remove this function?

We still need to check there is no duplicate request in a TCS for SLEEP 
and WAKE as well.

find_match() checks if the request already exist for a resource then 
update with new value, in a way preventing new request to go in

when one already exists. I am ok to remove this function since after [1] 
we always fully invalidate and then re-write and little point in

finding a match now. However we need to use check_for_req_inflight() 
from tcs_ctrl_write() with little modification to ignore tcs_is_free()

check if is called from tcs_ctrlr_write().

After this change on 9th change in your series,  please move it before 
current patch in series.

please also keep dependency on [1] for 9th change.

> + * If we later come up with fancy algorithms for updating everything without
> + * full invalidations we can come up with something then.
> + *
> + * Only for use on sleep/wake TCSs since those are the only ones we maintain
> + * tcs->slots and tcs->cmd_cache for.
> + *
> + * Must be called with the tcs_lock for the group held.
> + *
> + * Return: If the given command sequence wasn't in the tcs_group: -ENODATA.
> + *         If the given command sequence was in the tcs_group: the index of
> + *         the slot in the tcs_group where the first command is.
> + *         In some error cases (see above), -EINVAL.
> + */
>   static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
>   		      int len)
>   {
> @@ -497,6 +704,11 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
>   		if (i + len >= tcs->num_tcs * tcs->ncpt)
>   			goto seq_err;
>   		for (j = 0; j < len; j++) {
> +			/*
> +			 * TODO: it's actually not valid to look at
> +			 * "cmd_cache[x]" if "slots[x]" doesn't have a bit
> +			 * set.  Should add a check.
> +			 */
>   			if (tcs->cmd_cache[i + j] != cmd[j].addr)
>   				goto seq_err;
>   		}
> @@ -510,6 +722,23 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
>   	return -EINVAL;
>   }
>   
> +/**
> + * find_slots() - Find a place to write the given message.
> + * @tcs:    The tcs group to search.
> + * @msg:    The message we want to find room for.
> + * @tcs_id: If we return 0 from the function, we return the global ID of the
> + *          TCS to write to here.
> + * @cmd_id: If we return 0 from the function, we return the index of
> + *          the command array of the returned TCS where the client should
> + *          start writing the message.
> + *
> + * Only for use on sleep/wake TCSs since those are the only ones we maintain
> + * tcs->slots and tcs->cmd_cache for.
> + *
> + * Must be called with the tcs_lock for the group held.
> + *
> + * Return: -ENOMEM if there was no room, else 0.
> + */
>   static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
>   		      int *tcs_id, int *cmd_id)
>   {
> @@ -521,7 +750,7 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
>   	if (slot >= 0)
>   		goto copy_data;
>   
> -	/* Do over, until we can fit the full payload in a TCS */
> +	/* Do over, until we can fit the full payload in a single TCS */
>   	do {
>   		slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
>   						  i, msg->num_cmds, 0);
> @@ -544,12 +773,13 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
>   }
>   
>   /**
> - * rpmh_rsc_write_ctrl_data: Write request to the controller
> - *
> - * @drv: the controller
> - * @msg: the data to be written to the controller
> + * rpmh_rsc_write_ctrl_data() - Write request to controller but don't trigger.
> + * @drv: The controller.
> + * @msg: The data to be written to the controller.
>    *
>    * There is no response returned for writing the request to the controller.

you can remove above line since responce is returned from this function.

> + *
> + * Return: 0 if no error; else -error.
>    */
>   int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   {

Thanks,
Maulik

[1] https://patchwork.kernel.org/patch/11467811/

[2] https://patchwork.kernel.org/patch/11467821/

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch
  2020-03-11 23:13 ` [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch Douglas Anderson
@ 2020-04-01 11:38   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01 11:38 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> tcs_is_free() had two checks in it: does the software think that the
> TCS is free and does the hardware think that the TCS is free.  Let's
> comment this and also add a warning in the case that software and
> hardware disagree, at least for ACTIVE_ONLY TCS.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Comment tcs_is_free() new for v2; replaces old patch 6.
>
>   drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 9d2669cbd994..93f5d1fb71ca 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -181,8 +181,27 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
>    */
>   static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>   {
> -	return !test_bit(tcs_id, drv->tcs_in_use) &&
> -	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
> +	/* If software thinks it's in use then it's definitely in use */
> +	if (test_bit(tcs_id, drv->tcs_in_use))
> +		return false;
> +
> +	/* If hardware agrees it's free then it's definitely free */
> +	if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0)
> +		return true;
> +
> +	/*
> +	 * If we're here then software and hardware disagree about whether
> +	 * the TCS is free.  Software thinks it is free and hardware thinks
> +	 * it is not.
> +	 *
> +	 * Maybe this should be a warning in all cases, but it's almost
> +	 * certainly a warning for the ACTIVE_TCS where nobody else should
> +	 * be doing anything else behind our backs.  For now we'll just
> +	 * warn there and then still return that we're in use.
> +	 */
> +	WARN(drv->tcs[tcs_id].type == ACTIVE_TCS,
> +	     "Driver thought TCS was free but HW reported busy\n");
This warning can come for borrowed WAKE_TCS as well.
> +	return false;
>   }

We have a patch on downstream variant to optimize this by only checking 
tcs_in_use flag (SW check) and HW check is removed.

  static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
  {
-       return !test_bit(tcs_id, drv->tcs_in_use) &&
-              read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+       return !test_bit(tcs_id, drv->tcs_in_use);
  }

With this we are good and don't require to put above warning as well.

if you want me to upload, i can post it and then you can drop this 
change from your series.

Or if you want to modify it as above and keep in this series i am ok.

Thanks,
Maulik

>   
>   /**

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active
  2020-03-11 23:13 ` [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
@ 2020-04-01 12:40   ` Maulik Shah
  0 siblings, 0 replies; 19+ messages in thread
From: Maulik Shah @ 2020-04-01 12:40 UTC (permalink / raw)
  To: Douglas Anderson, Andy Gross, Bjorn Andersson
  Cc: mka, Rajendra Nayak, evgreen, Lina Iyer, swboyd, linux-arm-msm,
	linux-kernel

Hi,

On 3/12/2020 4:43 AM, Douglas Anderson wrote:
> tcs_write() is documented to only be useful for writing
> RPMH_ACTIVE_ONLY_STATE.  Let's be loud if someone messes up.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2: None
>
>   drivers/soc/qcom/rpmh-rsc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 93f5d1fb71ca..ba489d18c20e 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -573,6 +573,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>   	unsigned long flags;
>   	int ret;
>   
> +	/*
> +	 * It only makes sense to grab a whole TCS for ourselves if we're
> +	 * triggering right away, which we only do for ACTIVE_ONLY.
> +	 */
> +	WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE);
> +

Unnecessary check, we will never hit this warning. Lets not add such check.

Saying that you can modify this change to drop below check  from 
rpmh_rsc_write_ctrl_data() as that never hit.

         /* Data sent to this API will not be sent immediately */
         if (msg->state == RPMH_ACTIVE_ONLY_STATE)
                 return -EINVAL;

we always call rpmh_rsc_write_ctrl_data() for RPMH_SLEEP_STATE and 
RPMH_WAKE_ONLY_STATE.

Thanks,
Maulik
>   	/* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */
>   	tcs = get_tcs_for_msg(drv, msg);
>   	if (IS_ERR(tcs))

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 23:13 [RFT PATCH v2 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
2020-03-11 23:13 ` [RFT PATCH v2 01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
2020-04-01  8:12   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 02/10] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
2020-04-01  8:14   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
2020-04-01  8:17   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 04/10] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
2020-04-01  8:18   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 05/10] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
2020-04-01 11:29   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 06/10] drivers: qcom: rpmh-rsc: Comment tcs_is_free() + warn if state mismatch Douglas Anderson
2020-04-01 11:38   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 07/10] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
2020-04-01 12:40   ` Maulik Shah
2020-03-11 23:13 ` [RFT PATCH v2 08/10] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
2020-03-26 21:44   ` Doug Anderson
2020-03-11 23:13 ` [RFT PATCH v2 09/10] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
2020-03-11 23:13 ` [RFT PATCH v2 10/10] drivers: qcom: rpmh-rsc: Always use -EAGAIN, never -EBUSY Douglas Anderson

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git