[RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments

[RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments

[Douglas Anderson]

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.

With all that, enjoy.


Douglas Anderson (9):
  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: Only use "tcs_in_use" for ACTIVE_ONLY
  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/soc/qcom/rpmh-internal.h |  45 ++--
 drivers/soc/qcom/rpmh-rsc.c      | 390 +++++++++++++++++++++++--------
 2 files changed, 313 insertions(+), 122 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog

[RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds

[Douglas Anderson]

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

 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 e278fc11fe5c..5c88b8cd5bf8 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

Re: [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds

[Maulik Shah]

Hi,

On 3/7/2020 5:29 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().

i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)

can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
this way we have only one read and one write function.

so at the end we will two function as,

static u32 read_tcs_reg(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 +
                             RSC_DRV_CMD_OFFSET * cmd_id);
}

static void write_tcs_reg(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 +
                       RSC_DRV_CMD_OFFSET * cmd_id);
}

>
> 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.
With above change, i don't think you need to re-order this.
specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.

Thanks,
Maulik

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  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 e278fc11fe5c..5c88b8cd5bf8 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

Re: [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds

[Doug Anderson]

Hi,

On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Hi,
>
> On 3/7/2020 5:29 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().
>
> i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
>
> can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> this way we have only one read and one write function.
>
> so at the end we will two function as,
>
> static u32 read_tcs_reg(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 +
>                              RSC_DRV_CMD_OFFSET * cmd_id);
> }
>
> static void write_tcs_reg(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 +
>                        RSC_DRV_CMD_OFFSET * cmd_id);
> }

I can if you insist and this is still better than the existing
(inconsistent) code.

...but I still feel that having two functions adds value here.


Anyone else who is CCed want to weigh in and tie break?


> > 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.
> With above change, i don't think you need to re-order this.
> specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.

There was never any "need" to re-order.  That math works out to be the
same.  This is just clearer.

As an example, let's look at this:

struct point {
  int x;
  int y;
};
struct point points[10];

Let's say you have:
  void *points_base = &(points[0]);

...and now you want to find &(points[5].y).  What does your math look like?

a) points_base + (sizeof(struct point) * 5) + 4 ;

...or...

b) points_base + 4 + (sizeof(struct point) * 5);


Both calculations give the same result, but I am arguring that "a)" is
more intuitive.  Specifically you deal with the array access first and
then deal with the offset within the structure that you found.


-Doug

Re: [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds

[Matthias Kaehlcke]

Hi,

On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> >
> > Hi,
> >
> > On 3/7/2020 5:29 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().
> >
> > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
> >
> > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> > this way we have only one read and one write function.
> >
> > so at the end we will two function as,
> >
> > static u32 read_tcs_reg(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 +
> >                              RSC_DRV_CMD_OFFSET * cmd_id);
> > }
> >
> > static void write_tcs_reg(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 +
> >                        RSC_DRV_CMD_OFFSET * cmd_id);
> > }
> 
> I can if you insist and this is still better than the existing
> (inconsistent) code.
> 
> ...but I still feel that having two functions adds value here.
> 
> 
> Anyone else who is CCed want to weigh in and tie break?

I agree with Doug, having two functions makes the code that calls them
clearer. It makes it evident when a command is read/written and doesn't require
a useless extra parameter when accessing a non-command register.

> > > 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.
> > With above change, i don't think you need to re-order this.
> > specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.
> 
> There was never any "need" to re-order.  That math works out to be the
> same.  This is just clearer.
> 
> As an example, let's look at this:
> 
> struct point {
>   int x;
>   int y;
> };
> struct point points[10];
> 
> Let's say you have:
>   void *points_base = &(points[0]);
> 
> ...and now you want to find &(points[5].y).  What does your math look like?
> 
> a) points_base + (sizeof(struct point) * 5) + 4 ;
> 
> ...or...
> 
> b) points_base + 4 + (sizeof(struct point) * 5);
> 
> 
> Both calculations give the same result, but I am arguring that "a)" is
> more intuitive.  Specifically you deal with the array access first and
> then deal with the offset within the structure that you found.

+1

Re: [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds

[Stephen Boyd]

Quoting Matthias Kaehlcke (2020-03-11 09:17:26)
> Hi,
> 
> On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> > >
> > > Hi,
> > >
> > > On 3/7/2020 5:29 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().
> > >
> > > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
> > >
> > > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> > > this way we have only one read and one write function.
> > >
> > > so at the end we will two function as,
> > >
> > > static u32 read_tcs_reg(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 +
> > >                              RSC_DRV_CMD_OFFSET * cmd_id);
> > > }
> > >
> > > static void write_tcs_reg(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 +
> > >                        RSC_DRV_CMD_OFFSET * cmd_id);
> > > }
> > 
> > I can if you insist and this is still better than the existing
> > (inconsistent) code.
> > 
> > ...but I still feel that having two functions adds value here.
> > 
> > 
> > Anyone else who is CCed want to weigh in and tie break?
> 
> I agree with Doug, having two functions makes the code that calls them
> clearer. It makes it evident when a command is read/written and doesn't require
> a useless extra parameter when accessing a non-command register.

Me too! In fact, I asked for this when this driver was introduced and I
was half-ignored[1]. Making sure we never have to pass 0 to one of these
functions should be a goal.

From two years ago:
>
> Is m the type of TCS (sleep, active, wake) and n is just an offset?
> Maybe you can replace m with 'tcs_type' and n with 'index' or 'i' or
> 'offset'. And then don't use this function to write the random TCS
> registers that don't have to do with the TCS command slots? I see
> various places where there are things like:
> 
> > +               write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
> > +       write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
> > +       write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
> 
> And 'n' is 0, meaning you rely on that 0 killing that last part of the
> equation (RSC_DRV_CMD_OFFSET * n). But if we had a write_tcs_reg(drv,
> reg, m, data) and a write_tcs_cmd(drv, reg, m, n, data) then it would be
> clearer.
> 
> Even better, add a void *base to a 'struct tcs' and then pass that
> struct to the tcs_read/write APIs and then have that pull out a
> tcs->base + reg or tcs->base + reg + RSC_DRV_CMD_OFFSET * index.
> 

[1] https://lore.kernel.org/linux-arm-msm/152364140661.51482.261490347611407195@swboyd.mtv.corp.google.com/

[RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Douglas Anderson]

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.  It's much
easier to understand this if we define some structures.  At the moment
these structures aren't used at all (so think of them as
documentation), but to me they really help in understanding.

These structures were all figured out from the #defines and
reading/writing functions.  Anything that wasn't used in the driver is
marked as "opaque".

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

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

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 5c88b8cd5bf8..0a409988d103 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -61,6 +61,73 @@
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
+/*
+ * The following structures aren't used in the code anywhere (right now), but
+ * help to document how the register space is laid out.  In other words it's
+ * another way to visualize the "Register offsets".
+ *
+ * Couch this in a bogus #ifdef instead of comments to allow the embedded
+ * comments to work.
+ */
+#ifdef STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP
+
+/* 0x14 = 20 bytes big (see RSC_DRV_CMD_OFFSET) */
+struct tcs_cmd_hw {
+	u32 msgid;
+	u32 addr;
+	u32 data;
+	u32 status;
+	u32 resp_data;
+};
+
+/* 0x2a0 = 672 bytes big (see RSC_DRV_TCS_OFFSET) */
+struct tcs_hw {
+	/*
+	 * These are only valid on TCS 0 but are present everywhere.
+	 * Contains 1 bit per TCS.
+	 */
+	u32 irq_enable;
+	u32 irq_status;
+	u32 irq_clear;			/* Write only; write 1 to clear */
+
+	char opaque_00c[0x4];
+
+	u32 cmd_wait_for_cmpl;		/* Bit field, 1 bit per command */
+	u32 control;
+	u32 status;			/* status is 0 if tcs is busy */
+	u32 cmd_enable;			/* Bit field, 1 bit per command */
+
+	char opaque_01c[0x10];
+
+	struct tcs_cmd_hw tcs_cmd_hw[MAX_CMDS_PER_TCS];
+
+	char opaque_170[0x130];
+};
+
+/* Example for sc7180 based on current dts */
+struct rpmh_rsc_hw_sc7180 {
+	char opaque_000[0xc];
+
+	u32 prnt_chld_config;
+
+	char opaque_010[0xcf0];
+
+	/*
+	 * Offset 0xd00 aka qcom,tcs-offset from device tree.  Presumably
+	 * could be different for different SoCs?  Currently driver stores
+	 * a pointer to the first tcs in tcs_base.
+	 *
+	 * Count of various TCS entries also comes from dts.
+	 */
+	struct tcs_hw active[2];
+	struct tcs_hw sleep[3];
+	struct tcs_hw wake[3];
+	struct tcs_hw control[1];
+};
+
+#endif /* STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP */
+
+
 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

Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Maulik Shah]

Hi Doug,

On 3/7/2020 5:29 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.  
i don't understand why register layout is so important for you to understand?

besides, i think all required registers are properly named with #define

for e.g.
/* Register offsets */
#define RSC_DRV_IRQ_ENABLE              0x00
#define RSC_DRV_IRQ_STATUS              0x04
#define RSC_DRV_IRQ_CLEAR               0x08

now when you want to enable/disable irq in driver code, its pretty simple to figure out
that we need to read/write at RSC_DRV_IRQ_ENABLE offset.

this seems unnecessary change to me, can you please drop this when you spin v2?

Thanks,
Maulik
> It's much
> easier to understand this if we define some structures.  At the moment
> these structures aren't used at all (so think of them as
> documentation), but to me they really help in understanding.
>
> These structures were all figured out from the #defines and
> reading/writing functions.  Anything that wasn't used in the driver is
> marked as "opaque".
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/soc/qcom/rpmh-rsc.c | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 5c88b8cd5bf8..0a409988d103 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -61,6 +61,73 @@
>  #define CMD_STATUS_ISSUED		BIT(8)
>  #define CMD_STATUS_COMPL		BIT(16)
>  
> +/*
> + * The following structures aren't used in the code anywhere (right now), but
> + * help to document how the register space is laid out.  In other words it's
> + * another way to visualize the "Register offsets".
> + *
> + * Couch this in a bogus #ifdef instead of comments to allow the embedded
> + * comments to work.
> + */
> +#ifdef STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP
> +
> +/* 0x14 = 20 bytes big (see RSC_DRV_CMD_OFFSET) */
> +struct tcs_cmd_hw {
> +	u32 msgid;
> +	u32 addr;
> +	u32 data;
> +	u32 status;
> +	u32 resp_data;
> +};
> +
> +/* 0x2a0 = 672 bytes big (see RSC_DRV_TCS_OFFSET) */
> +struct tcs_hw {
> +	/*
> +	 * These are only valid on TCS 0 but are present everywhere.
> +	 * Contains 1 bit per TCS.
> +	 */
> +	u32 irq_enable;
> +	u32 irq_status;
> +	u32 irq_clear;			/* Write only; write 1 to clear */
> +
> +	char opaque_00c[0x4];
> +
> +	u32 cmd_wait_for_cmpl;		/* Bit field, 1 bit per command */
> +	u32 control;
> +	u32 status;			/* status is 0 if tcs is busy */
> +	u32 cmd_enable;			/* Bit field, 1 bit per command */
> +
> +	char opaque_01c[0x10];
> +
> +	struct tcs_cmd_hw tcs_cmd_hw[MAX_CMDS_PER_TCS];
> +
> +	char opaque_170[0x130];
> +};
> +
> +/* Example for sc7180 based on current dts */
> +struct rpmh_rsc_hw_sc7180 {
> +	char opaque_000[0xc];
> +
> +	u32 prnt_chld_config;
> +
> +	char opaque_010[0xcf0];
> +
> +	/*
> +	 * Offset 0xd00 aka qcom,tcs-offset from device tree.  Presumably
> +	 * could be different for different SoCs?  Currently driver stores
> +	 * a pointer to the first tcs in tcs_base.
> +	 *
> +	 * Count of various TCS entries also comes from dts.
> +	 */
> +	struct tcs_hw active[2];
> +	struct tcs_hw sleep[3];
> +	struct tcs_hw wake[3];
> +	struct tcs_hw control[1];
> +};
> +
> +#endif /* STRUCTS_TO_DOCUMENT_HW_REGISTER_MAP */
> +
> +
>  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

Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Doug Anderson]

Hi,

On Wed, Mar 11, 2020 at 2:35 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Hi Doug,
>
> On 3/7/2020 5:29 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.
> i don't understand why register layout is so important for you to understand?
>
> besides, i think all required registers are properly named with #define
>
> for e.g.
> /* Register offsets */
> #define RSC_DRV_IRQ_ENABLE              0x00
> #define RSC_DRV_IRQ_STATUS              0x04
> #define RSC_DRV_IRQ_CLEAR               0x08
>
> now when you want to enable/disable irq in driver code, its pretty simple to figure out
> that we need to read/write at RSC_DRV_IRQ_ENABLE offset.

It's not the specific layout that's important to me.  It's the
relationships between everything.  In other words:

a) one rpmh-rsc contains some registers and then a whole bunch of TCS blocks

b) one TCS block contains some registers and space for up to 16 commands.

c) each command has a handful of registers

Nothing describes this in the existing code--you've gotta read through
all the code and figure out how it's reading/writing registers to
figure it out.


> this seems unnecessary change to me, can you please drop this when you spin v2?

In v2 I'll replace this with the prose below.  Personally I find this
inferior to the struct definitions which are easier to read
at-a-glance, but it seems like it would be less controversial...

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

I'll also move the documentation of "irq_clear", "cmd_wait_for_cmpl",
"status", and "cmd_enable" next to the respective #defines.

Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Evan Green]

On Wed, Mar 11, 2020 at 8:28 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Mar 11, 2020 at 2:35 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> >
> > Hi Doug,
> >
> > On 3/7/2020 5:29 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.
> > i don't understand why register layout is so important for you to understand?
> >
> > besides, i think all required registers are properly named with #define
> >
> > for e.g.
> > /* Register offsets */
> > #define RSC_DRV_IRQ_ENABLE              0x00
> > #define RSC_DRV_IRQ_STATUS              0x04
> > #define RSC_DRV_IRQ_CLEAR               0x08
> >
> > now when you want to enable/disable irq in driver code, its pretty simple to figure out
> > that we need to read/write at RSC_DRV_IRQ_ENABLE offset.
>
> It's not the specific layout that's important to me.  It's the
> relationships between everything.  In other words:
>
> a) one rpmh-rsc contains some registers and then a whole bunch of TCS blocks
>
> b) one TCS block contains some registers and space for up to 16 commands.
>
> c) each command has a handful of registers
>
> Nothing describes this in the existing code--you've gotta read through
> all the code and figure out how it's reading/writing registers to
> figure it out.
>
>
> > this seems unnecessary change to me, can you please drop this when you spin v2?
>
> In v2 I'll replace this with the prose below.  Personally I find this
> inferior to the struct definitions which are easier to read
> at-a-glance, but it seems like it would be less controversial...
>
> /*
>  * 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.
>  */
>
> I'll also move the documentation of "irq_clear", "cmd_wait_for_cmpl",
> "status", and "cmd_enable" next to the respective #defines.

Perhaps, I'm biased in the same way Doug is, but reviewing RPMh
patches is especially difficult because exactly this kind of
documentation is missing. The relationship and inheritance between
TCS, DRV, RSC, and how many of each are inside of what is never really
spelled out. This makes it difficult to reason about what new patches
are doing and whether or not it works. Having the register layout and
documentation helps convey these relationships, and helps others to
ensure that the code is correctly doing what it's intended to do.

-Evan

Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Stephen Boyd]

Quoting Doug Anderson (2020-03-11 08:27:44)
> Hi,
> 
> On Wed, Mar 11, 2020 at 2:35 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> >
> > Hi Doug,
> >
> > On 3/7/2020 5:29 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.
> > i don't understand why register layout is so important for you to understand?
> >
> > besides, i think all required registers are properly named with #define
> >
> > for e.g.
> > /* Register offsets */
> > #define RSC_DRV_IRQ_ENABLE              0x00
> > #define RSC_DRV_IRQ_STATUS              0x04
> > #define RSC_DRV_IRQ_CLEAR               0x08
> >
> > now when you want to enable/disable irq in driver code, its pretty simple to figure out
> > that we need to read/write at RSC_DRV_IRQ_ENABLE offset.
> 
> It's not the specific layout that's important to me.  It's the
> relationships between everything.  In other words:
> 
> a) one rpmh-rsc contains some registers and then a whole bunch of TCS blocks
> 
> b) one TCS block contains some registers and space for up to 16 commands.
> 
> c) each command has a handful of registers
> 
> Nothing describes this in the existing code--you've gotta read through
> all the code and figure out how it's reading/writing registers to
> figure it out.
> 
> 
> > this seems unnecessary change to me, can you please drop this when you spin v2?
> 
> In v2 I'll replace this with the prose below.  Personally I find this
> inferior to the struct definitions which are easier to read
> at-a-glance, but it seems like it would be less controversial...
> 
> /*
>  * 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.

I never liked the qcom,tcs-offset property. Why can't that be hardcoded
in the driver based on the compatible string? Why does it need to be
specified in DT at all?

>  * - TCS blocks come one after another.  Type, count, and order are
>  *   specified by the device tree as "qcom,tcs-config".

This one too. Is the idea that some boards are going to change how the TCS
FIFOs are used?

>  * - 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.

I suspect an ascii block diagram would be useful to understand how it is
all done up. But given that the first TCS block has some sort of irq
control registers maybe it isn't correct to draw it split out of TCS0. I
think the first TCS is always the "active" TCS so it has these registers
for "irq control". The other two are the sleep and wake ones. But
sometimes there isn't an active TCS or software doesn't have a use for
it? Can probably indicate this in the diagram too.

Oh and maybe there should be CMD registers inside each TCS in this
diagram. It'd be great to have something like this so visual learners
like me can understand it all.

  +--------------------------------------------------+
  |RSC                                               |
  | +----------------------------------------------+ |
  | |DRV0                                          | |
  | | +-----------------------------------------+  | |
  | | |IRQ CONTROL                              |  | |
  | | |                                         |  | |
  | | +-----------------------------------------+  | |
  | | +-----------------------------------------+  | |
  | | |TCS0 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
  | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | +-----------------------------------------+  | |
  | | +-----------------------------------------+  | |
  | | |TCS1 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
  | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | +-----------------------------------------+  | |
  | | +-----------------------------------------+  | |
  | | |TCS2 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
  | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
  | | +-----------------------------------------+  | |
  | |                   ......                     | |
  | +----------------------------------------------+ |
  | +----------------------------------------------+ |
  | |DRV1                                          | |
  | | (same as DRV0)                               | |
  | +----------------------------------------------+ |
  +--------------------------------------------------+

Re: [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better

[Doug Anderson]

Hi,

On Wed, Mar 11, 2020 at 1:08 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-03-11 08:27:44)
> > Hi,
> >
> > On Wed, Mar 11, 2020 at 2:35 AM Maulik Shah <mkshah@codeaurora.org> wrote:
> > >
> > > Hi Doug,
> > >
> > > On 3/7/2020 5:29 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.
> > > i don't understand why register layout is so important for you to understand?
> > >
> > > besides, i think all required registers are properly named with #define
> > >
> > > for e.g.
> > > /* Register offsets */
> > > #define RSC_DRV_IRQ_ENABLE              0x00
> > > #define RSC_DRV_IRQ_STATUS              0x04
> > > #define RSC_DRV_IRQ_CLEAR               0x08
> > >
> > > now when you want to enable/disable irq in driver code, its pretty simple to figure out
> > > that we need to read/write at RSC_DRV_IRQ_ENABLE offset.
> >
> > It's not the specific layout that's important to me.  It's the
> > relationships between everything.  In other words:
> >
> > a) one rpmh-rsc contains some registers and then a whole bunch of TCS blocks
> >
> > b) one TCS block contains some registers and space for up to 16 commands.
> >
> > c) each command has a handful of registers
> >
> > Nothing describes this in the existing code--you've gotta read through
> > all the code and figure out how it's reading/writing registers to
> > figure it out.
> >
> >
> > > this seems unnecessary change to me, can you please drop this when you spin v2?
> >
> > In v2 I'll replace this with the prose below.  Personally I find this
> > inferior to the struct definitions which are easier to read
> > at-a-glance, but it seems like it would be less controversial...
> >
> > /*
> >  * 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.
>
> I never liked the qcom,tcs-offset property. Why can't that be hardcoded
> in the driver based on the compatible string? Why does it need to be
> specified in DT at all?
>
> >  * - TCS blocks come one after another.  Type, count, and order are
> >  *   specified by the device tree as "qcom,tcs-config".
>
> This one too. Is the idea that some boards are going to change how the TCS
> FIFOs are used?

Yeah, those thoughts occurred to me as well.  ...but right now there
actually _is_ no SoC-specific compatible string.  From sc7180:

  apps_rsc: rsc@18200000 {
    compatible = "qcom,rpmh-rsc";

I wouldn't be opposed to deprecating those properties and adding an
SoC-specific compatible string, but I probably won't take that up in
my series.  It's already getting a little unwieldy.


> >  * - 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.
>
> I suspect an ascii block diagram would be useful to understand how it is
> all done up. But given that the first TCS block has some sort of irq
> control registers maybe it isn't correct to draw it split out of TCS0. I
> think the first TCS is always the "active" TCS so it has these registers
> for "irq control". The other two are the sleep and wake ones. But
> sometimes there isn't an active TCS or software doesn't have a use for
> it? Can probably indicate this in the diagram too.

One thing that indicates that the first TCS is special and it's not
just that the first TCS happens to be the used for active-only is that
there are actually two active-only TCSs but we check the IRQ registers
in TCS 0 to tell us about both of them.


> Oh and maybe there should be CMD registers inside each TCS in this
> diagram. It'd be great to have something like this so visual learners
> like me can understand it all.
>
>   +--------------------------------------------------+
>   |RSC                                               |
>   | +----------------------------------------------+ |
>   | |DRV0                                          | |
>   | | +-----------------------------------------+  | |
>   | | |IRQ CONTROL                              |  | |
>   | | |                                         |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS0 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS1 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | | +-----------------------------------------+  | |
>   | | |TCS2 |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | |     | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15|  | |
>   | | |     |  |  |  |  |  |  |  |  |  |  |  |  |  | |
>   | | +-----------------------------------------+  | |
>   | |                   ......                     | |
>   | +----------------------------------------------+ |
>   | +----------------------------------------------+ |
>   | |DRV1                                          | |
>   | | (same as DRV0)                               | |
>   | +----------------------------------------------+ |
>   +--------------------------------------------------+

Thanks!  I was trying to figure out some sort of ASCII art at one
point but I gave up.  I was thinking that the struct was as good was
we were going to get in terms of "visual".  What you've got looks
useful though and I'll paste something like this in.  I'm going to
leave out the "IRQ control" parts though since it (to me) makes it
feel like it's a whole separate memory region.  In fact the IRQ
control registers seem (from the math) to be present in all the TCSs.
I'm going to go with this:

 *  +---------------------------------------------------+
 *  |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)                                | |
 *  | +-----------------------------------------------+ |
 *  |                      ......                       |
 *  +---------------------------------------------------+

So I'll end up skipping the fake struct definitions and go with the
prose + diagram.  If nothing else it's less controversial than
bogus/unused struct definitions.

-Doug

[RFT PATCH 3/9] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller

[Douglas Anderson]

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 it obvious (at least to me) their relationship.

Simplify by folding one function into the other.

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

 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 0a409988d103..099603bf14bf 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -549,27 +549,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
  *
@@ -580,6 +559,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");
@@ -590,7 +574,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

Re: [RFT PATCH 3/9] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller

[Maulik Shah]

Hi,

On 3/7/2020 5:29 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 it obvious (at least to me) their relationship.
>
> Simplify by folding one function into the other.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  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 0a409988d103..099603bf14bf 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -549,27 +549,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
>   *
> @@ -580,6 +559,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");
> @@ -590,7 +574,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,
i am ok with this change.

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

Thanks,
Maulik

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

[RFT PATCH 4/9] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction

[Douglas Anderson]

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

 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 099603bf14bf..a1298035bcd2 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -169,17 +169,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)) {
@@ -245,9 +238,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

Re: [RFT PATCH 4/9] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction

[Maulik Shah]

Hi,

On 3/7/2020 5:29 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>
> ---
>
>  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 099603bf14bf..a1298035bcd2 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -169,17 +169,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)) {
> @@ -245,9 +238,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)
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

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

[RFT PATCH 5/9] drivers: qcom: rpmh-rsc: A lot of comments

[Douglas Anderson]

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.

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

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

 drivers/soc/qcom/rpmh-internal.h |  45 +++---
 drivers/soc/qcom/rpmh-rsc.c      | 238 ++++++++++++++++++++++++++++---
 2 files changed, 248 insertions(+), 35 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 6eec32b97f83..49df01af7701 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -22,16 +22,23 @@ 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.
+ *             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 +91,16 @@ 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 for ACTIVE_ONLY TCSs.
+ * @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 a1298035bcd2..190226151029 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -163,12 +163,35 @@ 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 since that protects tcs_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;
@@ -195,9 +218,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)
 {
@@ -210,6 +235,17 @@ 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.
+ *
+ * Return: 0 if no problem, or -EGAIN if the caller should try again in a bit.
+ *         Caller should make sure to enable interrupts between tries.
+ */
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 					 const struct tcs_request *msg)
 {
@@ -251,6 +287,22 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 	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)
 {
@@ -267,7 +319,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)
 {
@@ -277,6 +336,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) {
@@ -317,6 +377,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)
 {
@@ -350,6 +420,11 @@ 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 ACTIVE_ONLY TCS.
+ * @drv:    The controller.
+ * @tcs_id: The global ID of this TCS.
+ */
 static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
 {
 	u32 enable;
@@ -372,6 +447,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)
 {
@@ -398,6 +494,14 @@ 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.
+ *
+ * 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;
@@ -410,6 +514,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 of type ACTIVE_ONLY 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;
@@ -422,11 +540,8 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 		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);
@@ -453,14 +568,23 @@ 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 one of the "ACTIVE_ONLY" tcs for
+ *   ourselves--we don't try to share.  If there are none available we'll
+ *   wait indefinitely for a free one.  It's important that we _aren't_ being
+ *   called with IRQs disabled because of this (we might need the interrupt to
+ *   fire to free up a busy TCS)
+ * - This function will not wait for the commands to be finished, only for
+ *   that data to be programmed into the RPMh.  See rpmh_tx_done() which will
+ *   be called when the transfer is complete.
  *
  * 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)
 {
@@ -484,6 +608,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)
 {
@@ -496,6 +677,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;
 		}
@@ -509,6 +695,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 controller.
+ * @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)
 {
@@ -520,7 +723,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);
@@ -543,12 +746,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

[RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY

[Douglas Anderson]

From trawling through the code (see the "A lot of comments" change) I
found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs.
...yet tcs_is_free() was checking the variable called from
tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY
TCSs.

Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS"
register, which was presumably the important part.

It also feels like for ACTIVE_ONLY TCSs that it probably wasn't
important to check the "RSC_DRV_STATUS".  We'll keep doing it just in
case but we'll add a warning if it ever actually mattered.

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

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

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 190226151029..c63441182358 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -164,7 +164,7 @@ 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.
+ * tcs_is_free() - Return if an ACTIVE_ONLY TCS is totally free.
  * @drv:    The RSC controller.
  * @tcs_id: The global ID of this TCS.
  *
@@ -177,8 +177,23 @@ 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 (test_bit(tcs_id, drv->tcs_in_use))
+		return false;
+
+	if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0)
+		return true;
+
+	/*
+	 * If this warning never ever hits then we can change this function
+	 * to just look at "tcs_in_use" and skip the read of the
+	 * RSC_DRV_STATUS register.
+	 *
+	 * If this warning _does_ hit, we should figure out if this is just
+	 * the way the hardware works or if there is some bug being pointed
+	 * out.
+	 */
+	WARN(1, "Driver thought TCS was free but HW reported busy\n");
+	return false;
 }
 
 /**
@@ -204,7 +219,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
 	}
 
 	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
-		if (!tcs_is_free(drv, m)) {
+		if (read_tcs_reg(drv, RSC_DRV_STATUS, m) == 0) {
 			spin_unlock(&tcs->lock);
 			return -EAGAIN;
 		}
-- 
2.25.1.481.gfbce0eb801-goog

Re: [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY

[Doug Anderson]

Hi,

On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> From trawling through the code (see the "A lot of comments" change) I
> found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs.
> ...yet tcs_is_free() was checking the variable called from
> tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY
> TCSs.
>
> Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS"
> register, which was presumably the important part.
>
> It also feels like for ACTIVE_ONLY TCSs that it probably wasn't
> important to check the "RSC_DRV_STATUS".  We'll keep doing it just in
> case but we'll add a warning if it ever actually mattered.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)

After other RPMH email threads, it's possible that this patch isn't
quite right.  ...but also the code wasn't quite right before.

Specifically if we have 0 active TCSs then it's possible that we've
used a wake TCS to send an active-only request.  That would be a case
where "tcs_in_use" might be set and we'd need to make sure that
tcs_invalidate() checks it.  However:

1. We need to add locking to tcs_invalidate() since "tcs_in_use" is
protected by drv->lock and tcs_invalidate() didn't grab that lock.

2. We should add documentation explaining the situation.


I'm still waiting on overall review / testing of my series, but I'll
put it on my list to address this for v2.

-Doug

[RFT PATCH 7/9] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active

[Douglas Anderson]

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

 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 c63441182358..693873fce895 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -550,6 +550,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);
+
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);
-- 
2.25.1.481.gfbce0eb801-goog

[RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()

[Douglas Anderson]

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

 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 693873fce895..0297da5ceeaa 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -210,9 +210,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;
@@ -227,7 +228,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

[RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire

[Douglas Anderson]

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.

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

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 49df01af7701..7206a1ac97e8 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -34,7 +34,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
@@ -49,7 +48,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 0297da5ceeaa..733373ed56bd 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -630,93 +630,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 controller.
@@ -728,7 +641,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.
  *
@@ -740,11 +653,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,
@@ -754,11 +662,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;
@@ -889,12 +793,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 		 */
 		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

Re: [RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire

[Doug Anderson]

Hi,

On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> @@ -889,12 +793,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
>                  */
>                 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;

During later code inspection I happened to notice that the "if" test
above the code I removed can also be removed.  I'll do that in v2.
The code after the v1 patch doesn't hurt, it's just silly to have the
"if (blah) continue" at the end of the loop.

I'll wait on sending a v2 until I get some testing / review feedback
on v1 or enough time passes.

-Doug

Re: [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments

[Maulik Shah]

Hi,

On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> 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) 

i can help these get tested.

Thanks,
Maulik

> 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.
>
> With all that, enjoy.
>
>
> Douglas Anderson (9):
>   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: Only use "tcs_in_use" for ACTIVE_ONLY
>   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/soc/qcom/rpmh-internal.h |  45 ++--
>  drivers/soc/qcom/rpmh-rsc.c      | 390 +++++++++++++++++++++++--------
>  2 files changed, 313 insertions(+), 122 deletions(-)
>
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation