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

[Doug Anderson <dianders@chromium.org>]

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