[PATCH v2 0/3] Even moar rpmh cleanups

[PATCH v2 0/3] Even moar rpmh cleanups

[Stephen Boyd]

Patches based on Doug's latest series[1] on top of linux-next. We remove
the tcs_is_free() API and then do super micro optimizations on the irq
handler. I haven't tested anything here so most likely there's a bug
(again)!

Changes from v1
 * First patch became even moar complicated because it combines
   find_free_tcs() with the check for a request in flight
 * Fixed subject in patch 2
 * Put back unsigned long for bitmap operation to silence compiler
   warning
 * Picked up review tags

Stephen Boyd (3):
  soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs
  soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler
  soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition

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

Cc: Maulik Shah <mkshah@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>

[1] https://lore.kernel.org/r/20200424094610.v5.1.Ic7096b3b9b7828cdd41cd5469a6dee5eb6abf549@changeid

base-commit: 02d8ecc18b8f392389ac9e7b785b0230ecb80833
prerequisite-patch-id: 0d383ea46ef52ab4044886a7d88d85c3c506f4ed
prerequisite-patch-id: a02b0b018404d1a0c79270ab567051656f123b23
prerequisite-patch-id: e59d990462b004a9f8335e87c2d0d747afec49ea
prerequisite-patch-id: 20cd04c9dbed8937de5d61f486616c5961b8ef99
prerequisite-patch-id: bb479b9adbe28c58b3ac8f363a306de80b6dcb74
-- 
Sent by a computer, using git, on the internet

[PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs

[Stephen Boyd]

These APIs do very little. Let's replace all the callsites with the
normal operations that would be done on top of the tcs_in_use bitmap
This simplifies and reduces the code size.

Cc: Maulik Shah <mkshah@codeaurora.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/soc/qcom/rpmh-rsc.c | 109 ++++++++++++------------------------
 1 file changed, 37 insertions(+), 72 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 571aa1012f23..3f4951840365 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -172,22 +172,6 @@ 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).
- *
- * Context: Must be called with the drv->lock held.
- *
- * 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);
-}
-
 /**
  * tcs_invalidate() - Invalidate all TCSes of the given type (sleep or wake).
  * @drv:  The RSC controller.
@@ -484,7 +468,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 }
 
 /**
- * check_for_req_inflight() - Look to see if conflicting cmds are in flight.
+ * check_for_req_inflight_and_find_free() - Find an available tcs for a req
  * @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
@@ -492,33 +476,37 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
  *
  * This will walk through the TCSes in the group and check if any of them
  * appear to be sending to addresses referenced in the message. If it finds
- * one it'll return -EBUSY.
+ * one it'll return -EBUSY because the hardware can't handle more than
+ * one of the same address being changed at the same time.
  *
- * Only for use for active-only transfers.
+ * Only for use with active-only transfers.
  *
  * Must be called with the drv->lock held since that protects tcs_in_use.
  *
- * Return: 0 if nothing in flight or -EBUSY if we should try again later.
+ * Return: offset` of free slot if nothing in flight and a free slot is found
+ *         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
+ *         the only way tcs_in_use will ever be updated 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)
+static int check_for_req_inflight_and_find_free(struct rsc_drv *drv,
+	const struct tcs_group *tcs, const struct tcs_request *msg)
 {
 	unsigned long curr_enabled;
 	u32 addr;
-	int i, j, k;
-	int tcs_id = tcs->offset;
-
-	for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
-		if (tcs_is_free(drv, tcs_id))
-			continue;
+	int j, k;
+	int i = tcs->offset;
+	unsigned long max = tcs->offset + tcs->num_tcs;
+	int first_free = i;
 
-		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
+	for_each_set_bit_from(i, drv->tcs_in_use, max) {
+		/* Find a free tcs to use in this group */
+		if (first_free == i)
+			first_free = i + 1; /* Maybe the next one is free? */
 
+		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i);
 		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
-			addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
+			addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, i, j);
 			for (k = 0; k < msg->num_cmds; k++) {
 				if (addr == msg->cmds[k].addr)
 					return -EBUSY;
@@ -526,28 +514,11 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 		}
 	}
 
-	return 0;
-}
+	if (first_free >= max)
+		return -EBUSY;
 
-/**
- * find_free_tcs() - Find free tcs in the given tcs_group; only for active.
- * @tcs: A pointer to the active-only tcs_group (or the wake tcs_group if
- *       we borrowed it because there are zero active-only ones).
- *
- * Must be called with the drv->lock held since that protects tcs_in_use.
- *
- * Return: The first tcs that's free.
- */
-static int find_free_tcs(struct tcs_group *tcs)
-{
-	int i;
-
-	for (i = 0; i < tcs->num_tcs; i++) {
-		if (tcs_is_free(tcs->drv, tcs->offset + i))
-			return tcs->offset + i;
-	}
-
-	return -EBUSY;
+	set_bit(first_free, drv->tcs_in_use);
+	return first_free;
 }
 
 /**
@@ -580,17 +551,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	 * 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)
-		goto unlock;
-
-	ret = find_free_tcs(tcs);
-	if (ret < 0)
+	tcs_id = check_for_req_inflight_and_find_free(drv, tcs, msg);
+	if (tcs_id < 0) {
+		ret = tcs_id;
 		goto unlock;
-	tcs_id = ret;
+	}
 
+	ret = 0;
 	tcs->req[tcs_id - tcs->offset] = msg;
-	set_bit(tcs_id, drv->tcs_in_use);
 	if (msg->state == RPMH_ACTIVE_ONLY_STATE && tcs->type != ACTIVE_TCS) {
 		/*
 		 * Clear previously programmed WAKE commands in selected
@@ -601,6 +569,7 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
 		enable_tcs_irq(drv, tcs_id, true);
 	}
+unlock:
 	spin_unlock_irqrestore(&drv->lock, flags);
 
 	/*
@@ -611,12 +580,11 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 	 * - The interrupt can't go off until we trigger w/ the last line
 	 *   of __tcs_set_trigger() below.
 	 */
-	__tcs_buffer_write(drv, tcs_id, 0, msg);
-	__tcs_set_trigger(drv, tcs_id, true);
+	if (!ret) {
+		__tcs_buffer_write(drv, tcs_id, 0, msg);
+		__tcs_set_trigger(drv, tcs_id, true);
+	}
 
-	return 0;
-unlock:
-	spin_unlock_irqrestore(&drv->lock, flags);
 	return ret;
 }
 
@@ -745,8 +713,8 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
  */
 static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
 {
-	int m;
-	struct tcs_group *tcs = &drv->tcs[ACTIVE_TCS];
+	const struct tcs_group *tcs = &drv->tcs[ACTIVE_TCS];
+	unsigned long max;
 
 	/*
 	 * If we made an active request on a RSC that does not have a
@@ -757,12 +725,9 @@ static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
 	if (!tcs->num_tcs)
 		tcs = &drv->tcs[WAKE_TCS];
 
-	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
-		if (!tcs_is_free(drv, m))
-			return true;
-	}
+	max = tcs->offset + tcs->num_tcs;
 
-	return false;
+	return find_next_bit(drv->tcs_in_use, max, tcs->offset) < max;
 }
 
 /**
-- 
Sent by a computer, using git, on the internet

[PATCH v2 2/3] soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler

[Stephen Boyd]

readl() returns a u32, and BITS_PER_LONG is different on 32-bit vs.
64-bit architectures. Let's loop over the possible bits set in that type
instead of looping over more bits than we ever may need to.

Cc: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/soc/qcom/rpmh-rsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 3f4951840365..28121a32a434 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -371,7 +371,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 
 	irq_status = readl_relaxed(drv->tcs_base + RSC_DRV_IRQ_STATUS);
 
-	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
+	for_each_set_bit(i, &irq_status, BITS_PER_TYPE(u32)) {
 		req = get_req_from_tcs(drv, i);
 		if (!req) {
 			WARN_ON(1);
-- 
Sent by a computer, using git, on the internet

[PATCH v2 3/3] soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition

[Stephen Boyd]

Move the WARN_ON() into the if condition so the compiler can see that
the branch is unlikely() and possibly optimize it better.

Cc: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/soc/qcom/rpmh-rsc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 28121a32a434..4accfeb191de 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -373,10 +373,8 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 
 	for_each_set_bit(i, &irq_status, BITS_PER_TYPE(u32)) {
 		req = get_req_from_tcs(drv, i);
-		if (!req) {
-			WARN_ON(1);
+		if (WARN_ON(!req))
 			goto skip;
-		}
 
 		err = 0;
 		for (j = 0; j < req->num_cmds; j++) {
-- 
Sent by a computer, using git, on the internet

Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs

[Doug Anderson]

Hi,

On Sat, Apr 25, 2020 at 10:53 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> These APIs do very little. Let's replace all the callsites with the
> normal operations that would be done on top of the tcs_in_use bitmap
> This simplifies and reduces the code size.
>
> Cc: Maulik Shah <mkshah@codeaurora.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/soc/qcom/rpmh-rsc.c | 109 ++++++++++++------------------------
>  1 file changed, 37 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 571aa1012f23..3f4951840365 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -172,22 +172,6 @@ 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).
> - *
> - * Context: Must be called with the drv->lock held.
> - *
> - * 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);
> -}
> -
>  /**
>   * tcs_invalidate() - Invalidate all TCSes of the given type (sleep or wake).
>   * @drv:  The RSC controller.
> @@ -484,7 +468,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>  }
>
>  /**
> - * check_for_req_inflight() - Look to see if conflicting cmds are in flight.
> + * check_for_req_inflight_and_find_free() - Find an available tcs for a req
>   * @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
> @@ -492,33 +476,37 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   *
>   * This will walk through the TCSes in the group and check if any of them
>   * appear to be sending to addresses referenced in the message. If it finds
> - * one it'll return -EBUSY.
> + * one it'll return -EBUSY because the hardware can't handle more than
> + * one of the same address being changed at the same time.
>   *
> - * Only for use for active-only transfers.
> + * Only for use with active-only transfers.
>   *
>   * Must be called with the drv->lock held since that protects tcs_in_use.
>   *
> - * Return: 0 if nothing in flight or -EBUSY if we should try again later.
> + * Return: offset` of free slot if nothing in flight and a free slot is found

Why the back tick after "offset"?


> + *         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
> + *         the only way tcs_in_use will ever be updated 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)
> +static int check_for_req_inflight_and_find_free(struct rsc_drv *drv,
> +       const struct tcs_group *tcs, const struct tcs_request *msg)
>  {
>         unsigned long curr_enabled;
>         u32 addr;
> -       int i, j, k;
> -       int tcs_id = tcs->offset;
> -
> -       for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
> -               if (tcs_is_free(drv, tcs_id))
> -                       continue;
> +       int j, k;
> +       int i = tcs->offset;
> +       unsigned long max = tcs->offset + tcs->num_tcs;
> +       int first_free = i;

The way "first_free" is calculated definitely adds complexity to this
function.  Are we sure it's justified compared to just calling
find_next_zero_bit() if the function doesn't return -EBUSY?  If you
really like it this way I won't object too strongly, but I'm not
convinced that it makes the code size smaller (vs. jumping to a common
implementation in the kernel) and it seems unlikely to have any
real-world speed impact.


> -               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
> +       for_each_set_bit_from(i, drv->tcs_in_use, max) {
> +               /* Find a free tcs to use in this group */
> +               if (first_free == i)
> +                       first_free = i + 1; /* Maybe the next one is free? */
>
> +               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i);
>                 for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> -                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
> +                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, i, j);
>                         for (k = 0; k < msg->num_cmds; k++) {
>                                 if (addr == msg->cmds[k].addr)
>                                         return -EBUSY;
> @@ -526,28 +514,11 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>                 }
>         }
>
> -       return 0;
> -}
> +       if (first_free >= max)
> +               return -EBUSY;
>
> -/**
> - * find_free_tcs() - Find free tcs in the given tcs_group; only for active.
> - * @tcs: A pointer to the active-only tcs_group (or the wake tcs_group if
> - *       we borrowed it because there are zero active-only ones).
> - *
> - * Must be called with the drv->lock held since that protects tcs_in_use.
> - *
> - * Return: The first tcs that's free.
> - */
> -static int find_free_tcs(struct tcs_group *tcs)
> -{
> -       int i;
> -
> -       for (i = 0; i < tcs->num_tcs; i++) {
> -               if (tcs_is_free(tcs->drv, tcs->offset + i))
> -                       return tcs->offset + i;
> -       }
> -
> -       return -EBUSY;
> +       set_bit(first_free, drv->tcs_in_use);

Function is not documented to also set the bit.  Do we really gain
anything by setting it in this function, or can it just stay with the
caller?  I'd hate to call this function
check_for_req_inflight_and_find_free_and_claim_it().


> +       return first_free;
>  }
>
>  /**
> @@ -580,17 +551,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>          * 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)
> -               goto unlock;
> -
> -       ret = find_free_tcs(tcs);
> -       if (ret < 0)
> +       tcs_id = check_for_req_inflight_and_find_free(drv, tcs, msg);
> +       if (tcs_id < 0) {
> +               ret = tcs_id;
>                 goto unlock;
> -       tcs_id = ret;
> +       }
>
> +       ret = 0;

nit: Cleaner to just init to 0 when the function is declared now?


-Doug

Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs

[Stephen Boyd]

Quoting Doug Anderson (2020-04-27 17:13:04)
> On Sat, Apr 25, 2020 at 10:53 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index 571aa1012f23..3f4951840365 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -492,33 +476,37 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
> >   *
> >   * This will walk through the TCSes in the group and check if any of them
> >   * appear to be sending to addresses referenced in the message. If it finds
> > - * one it'll return -EBUSY.
> > + * one it'll return -EBUSY because the hardware can't handle more than
> > + * one of the same address being changed at the same time.
> >   *
> > - * Only for use for active-only transfers.
> > + * Only for use with active-only transfers.
> >   *
> >   * Must be called with the drv->lock held since that protects tcs_in_use.
> >   *
> > - * Return: 0 if nothing in flight or -EBUSY if we should try again later.
> > + * Return: offset` of free slot if nothing in flight and a free slot is found
> 
> Why the back tick after "offset"?

Must be some copy/paste bug!

> 
> 
> > + *         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
> > + *         the only way tcs_in_use will ever be updated 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)
> > +static int check_for_req_inflight_and_find_free(struct rsc_drv *drv,
> > +       const struct tcs_group *tcs, const struct tcs_request *msg)
> >  {
> >         unsigned long curr_enabled;
> >         u32 addr;
> > -       int i, j, k;
> > -       int tcs_id = tcs->offset;
> > -
> > -       for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
> > -               if (tcs_is_free(drv, tcs_id))
> > -                       continue;
> > +       int j, k;
> > +       int i = tcs->offset;
> > +       unsigned long max = tcs->offset + tcs->num_tcs;
> > +       int first_free = i;
> 
> The way "first_free" is calculated definitely adds complexity to this
> function.  Are we sure it's justified compared to just calling
> find_next_zero_bit() if the function doesn't return -EBUSY?  If you
> really like it this way I won't object too strongly, but I'm not
> convinced that it makes the code size smaller (vs. jumping to a common
> implementation in the kernel) and it seems unlikely to have any
> real-world speed impact.

I was trying to coalesce the double loop over the free bits here by
adding a couple more lines to keep track of the first free bit and to
set the bit when it's found. It almost feels like it would be better to
inline this whole function into the one call site too.

> 
> 
> > -               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
> > +       for_each_set_bit_from(i, drv->tcs_in_use, max) {
> > +               /* Find a free tcs to use in this group */
> > +               if (first_free == i)
> > +                       first_free = i + 1; /* Maybe the next one is free? */
> >
> > +               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i);
> >                 for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> > -                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
> > +                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, i, j);
> >                         for (k = 0; k < msg->num_cmds; k++) {
> >                                 if (addr == msg->cmds[k].addr)
> >                                         return -EBUSY;
> > @@ -526,28 +514,11 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> >                 }
> >         }
> >
> > -       return 0;
> > -}
> > +       if (first_free >= max)
> > +               return -EBUSY;
> >
> > -/**
> > - * find_free_tcs() - Find free tcs in the given tcs_group; only for active.
> > - * @tcs: A pointer to the active-only tcs_group (or the wake tcs_group if
> > - *       we borrowed it because there are zero active-only ones).
> > - *
> > - * Must be called with the drv->lock held since that protects tcs_in_use.
> > - *
> > - * Return: The first tcs that's free.
> > - */
> > -static int find_free_tcs(struct tcs_group *tcs)
> > -{
> > -       int i;
> > -
> > -       for (i = 0; i < tcs->num_tcs; i++) {
> > -               if (tcs_is_free(tcs->drv, tcs->offset + i))
> > -                       return tcs->offset + i;
> > -       }
> > -
> > -       return -EBUSY;
> > +       set_bit(first_free, drv->tcs_in_use);
> 
> Function is not documented to also set the bit.  Do we really gain
> anything by setting it in this function, or can it just stay with the
> caller?  I'd hate to call this function
> check_for_req_inflight_and_find_free_and_claim_it().

Maybe the function can be named claim_tcs_for_req() or something like
that. Anything to make it shorter would be good!

> 
> 
> > +       return first_free;
> >  }
> >
> >  /**
> > @@ -580,17 +551,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >          * 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)
> > -               goto unlock;
> > -
> > -       ret = find_free_tcs(tcs);
> > -       if (ret < 0)
> > +       tcs_id = check_for_req_inflight_and_find_free(drv, tcs, msg);
> > +       if (tcs_id < 0) {
> > +               ret = tcs_id;
> >                 goto unlock;
> > -       tcs_id = ret;
> > +       }
> >
> > +       ret = 0;
> 
> nit: Cleaner to just init to 0 when the function is declared now?
> 

Ok.

Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs

[Doug Anderson]

Hi,

On Mon, May 4, 2020 at 11:36 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > > -static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> > > -                                 const struct tcs_request *msg)
> > > +static int check_for_req_inflight_and_find_free(struct rsc_drv *drv,
> > > +       const struct tcs_group *tcs, const struct tcs_request *msg)
> > >  {
> > >         unsigned long curr_enabled;
> > >         u32 addr;
> > > -       int i, j, k;
> > > -       int tcs_id = tcs->offset;
> > > -
> > > -       for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
> > > -               if (tcs_is_free(drv, tcs_id))
> > > -                       continue;
> > > +       int j, k;
> > > +       int i = tcs->offset;
> > > +       unsigned long max = tcs->offset + tcs->num_tcs;
> > > +       int first_free = i;
> >
> > The way "first_free" is calculated definitely adds complexity to this
> > function.  Are we sure it's justified compared to just calling
> > find_next_zero_bit() if the function doesn't return -EBUSY?  If you
> > really like it this way I won't object too strongly, but I'm not
> > convinced that it makes the code size smaller (vs. jumping to a common
> > implementation in the kernel) and it seems unlikely to have any
> > real-world speed impact.
>
> I was trying to coalesce the double loop over the free bits here by
> adding a couple more lines to keep track of the first free bit and to
> set the bit when it's found. It almost feels like it would be better to
> inline this whole function into the one call site too.

Definitely a bike shed color issue.  I know it was double-looping before, but:

* Neither loop was very long, a few bits at most.

* The 2nd loop was in common code.  That means the "number of
instructions" to implement this loop is small--just a function call.
For code that isn't a hot spot it can be better to optimize for code
size rather than speed since it means you're taking up fewer cache
lines and thus less likely to kick out other code.  ;-)  ...but we're
getting into micro optimization.

In any case, I probably haven't convinced you.  I'm fine with your
code and I'll shut up now.


> > > -               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
> > > +       for_each_set_bit_from(i, drv->tcs_in_use, max) {
> > > +               /* Find a free tcs to use in this group */
> > > +               if (first_free == i)
> > > +                       first_free = i + 1; /* Maybe the next one is free? */
> > >
> > > +               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i);
> > >                 for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> > > -                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
> > > +                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, i, j);
> > >                         for (k = 0; k < msg->num_cmds; k++) {
> > >                                 if (addr == msg->cmds[k].addr)
> > >                                         return -EBUSY;
> > > @@ -526,28 +514,11 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> > >                 }
> > >         }
> > >
> > > -       return 0;
> > > -}
> > > +       if (first_free >= max)
> > > +               return -EBUSY;
> > >
> > > -/**
> > > - * find_free_tcs() - Find free tcs in the given tcs_group; only for active.
> > > - * @tcs: A pointer to the active-only tcs_group (or the wake tcs_group if
> > > - *       we borrowed it because there are zero active-only ones).
> > > - *
> > > - * Must be called with the drv->lock held since that protects tcs_in_use.
> > > - *
> > > - * Return: The first tcs that's free.
> > > - */
> > > -static int find_free_tcs(struct tcs_group *tcs)
> > > -{
> > > -       int i;
> > > -
> > > -       for (i = 0; i < tcs->num_tcs; i++) {
> > > -               if (tcs_is_free(tcs->drv, tcs->offset + i))
> > > -                       return tcs->offset + i;
> > > -       }
> > > -
> > > -       return -EBUSY;
> > > +       set_bit(first_free, drv->tcs_in_use);
> >
> > Function is not documented to also set the bit.  Do we really gain
> > anything by setting it in this function, or can it just stay with the
> > caller?  I'd hate to call this function
> > check_for_req_inflight_and_find_free_and_claim_it().
>
> Maybe the function can be named claim_tcs_for_req() or something like
> that. Anything to make it shorter would be good!

Sure.  ...though moving the set_bit() here is just pure churn, right?
It can be here or it can be in the calling function and there's really
no advantage either way.  If you really like it here then fine.  I
just see no benefit and it's just an extra line to change.


-Doug