Re: [PATCH 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() API

[Doug Anderson]

Hi,

On Thu, Apr 23, 2020 at 9:54 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> This API does very little. Let's replace all the callsites with the
> normal operations that would be done on top of the bitmap that
> tcs_in_use is. This simplifies and reduces the code size.
>
> Cc: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>  drivers/soc/qcom/rpmh-rsc.c | 56 +++++++++++--------------------------
>  1 file changed, 17 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 060be10bc491..76e0294a672c 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.
> @@ -500,7 +484,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
>   *
>   * 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
> + *         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,
> @@ -508,17 +492,14 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>  {
>         unsigned long curr_enabled;
>         u32 addr;
> -       int i, j, k;
> -       int tcs_id = tcs->offset;
> +       int j, k;
> +       int i = tcs->offset;
>
> -       for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
> -               if (tcs_is_free(drv, tcs_id))
> -                       continue;
> -
> -               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
> +       for_each_set_bit_from(i, drv->tcs_in_use, tcs->offset + tcs->num_tcs) {
> +               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;
> @@ -536,18 +517,18 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>   *
>   * Must be called with the drv->lock held since that protects tcs_in_use.
>   *
> - * Return: The first tcs that's free.
> + * Return: The first tcs that's free or -EBUSY if all in use.
>   */
>  static int find_free_tcs(struct tcs_group *tcs)
>  {
> -       int i;
> +       const struct rsc_drv *drv = tcs->drv;
> +       unsigned long i;
>
> -       for (i = 0; i < tcs->num_tcs; i++) {
> -               if (tcs_is_free(tcs->drv, tcs->offset + i))
> -                       return tcs->offset + i;
> -       }
> +       i = find_next_zero_bit(drv->tcs_in_use, MAX_TCS_NR, tcs->offset);

Rather than passing MAX_TCS_NR would it be legit to pass "tcs->offset
+ tcs->num_tcs"?  You are passing that as "size" above in
check_for_req_inflight().


> +       if (i > tcs->offset + tcs->num_tcs)

Should the above be ">=" ?  Oh, I guess Bjorn found the same thing.  ;-)


> +               return -EBUSY;
>
> -       return -EBUSY;
> +       return i;
>  }
>
>  /**
> @@ -744,8 +725,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];
> +       unsigned long set;
> +       const struct tcs_group *tcs = &drv->tcs[ACTIVE_TCS];
>
>         /*
>          * If we made an active request on a RSC that does not have a
> @@ -756,12 +737,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;
> -       }
> +       set = find_next_bit(drv->tcs_in_use, MAX_TCS_NR, tcs->offset);

Similar to above, can you pass "tcs->offset + tcs->num_tcs" instead of
MAX_TCS_NR?