| Message ID | 20200424045414.133381-2-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Even moar rpmh cleanups | expand |
On Thu 23 Apr 21:54 PDT 2020, Stephen Boyd wrote: > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c [..] > 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); > + if (i > tcs->offset + tcs->num_tcs) Afaict this should be >= Regards, Bjorn
Hi, On Thu, Apr 23, 2020 at 9:54 PM Stephen Boyd <swboyd@chromium.org> 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@codeaurora.org> > Cc: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > 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?
Quoting Bjorn Andersson (2020-04-23 22:36:26) > On Thu 23 Apr 21:54 PDT 2020, Stephen Boyd wrote: > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > [..] > > 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); > > + if (i > tcs->offset + tcs->num_tcs) > > Afaict this should be >= > Thanks!
Quoting Doug Anderson (2020-04-24 10:12:04) > On Thu, Apr 23, 2020 at 9:54 PM Stephen Boyd <swboyd@chromium.org> wrote: > > + 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(). Yeah I think that should work. I'll roll it in.
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); + if (i > tcs->offset + tcs->num_tcs) + 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); - return false; + return set < tcs->offset + tcs->num_tcs; } /**
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@codeaurora.org> Cc: Douglas Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/soc/qcom/rpmh-rsc.c | 56 +++++++++++-------------------------- 1 file changed, 17 insertions(+), 39 deletions(-)