* [PATCH v2 0/3] Even moar rpmh cleanups
@ 2020-04-25 17:53 Stephen Boyd
2020-04-25 17:53 ` [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs Stephen Boyd
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Stephen Boyd @ 2020-04-25 17:53 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson
Cc: linux-kernel, linux-arm-msm, Maulik Shah, Douglas Anderson
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs 2020-04-25 17:53 [PATCH v2 0/3] Even moar rpmh cleanups Stephen Boyd @ 2020-04-25 17:53 ` Stephen Boyd 2020-04-28 0:13 ` Doug Anderson 2020-04-25 17:53 ` [PATCH v2 2/3] soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 3/3] soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition Stephen Boyd 2 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2020-04-25 17:53 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson Cc: linux-kernel, linux-arm-msm, Maulik Shah, Douglas Anderson 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs 2020-04-25 17:53 ` [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs Stephen Boyd @ 2020-04-28 0:13 ` Doug Anderson 2020-05-05 6:36 ` Stephen Boyd 0 siblings, 1 reply; 7+ messages in thread From: Doug Anderson @ 2020-04-28 0:13 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Bjorn Andersson, LKML, linux-arm-msm, Maulik Shah 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs 2020-04-28 0:13 ` Doug Anderson @ 2020-05-05 6:36 ` Stephen Boyd 2020-05-05 20:04 ` Doug Anderson 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2020-05-05 6:36 UTC (permalink / raw) To: Doug Anderson Cc: Andy Gross, Bjorn Andersson, LKML, linux-arm-msm, Maulik Shah 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs 2020-05-05 6:36 ` Stephen Boyd @ 2020-05-05 20:04 ` Doug Anderson 0 siblings, 0 replies; 7+ messages in thread From: Doug Anderson @ 2020-05-05 20:04 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Gross, Bjorn Andersson, LKML, linux-arm-msm, Maulik Shah 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler 2020-04-25 17:53 [PATCH v2 0/3] Even moar rpmh cleanups Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs Stephen Boyd @ 2020-04-25 17:53 ` Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 3/3] soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition Stephen Boyd 2 siblings, 0 replies; 7+ messages in thread From: Stephen Boyd @ 2020-04-25 17:53 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson Cc: linux-kernel, linux-arm-msm, Maulik Shah, Douglas Anderson 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition 2020-04-25 17:53 [PATCH v2 0/3] Even moar rpmh cleanups Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 2/3] soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler Stephen Boyd @ 2020-04-25 17:53 ` Stephen Boyd 2 siblings, 0 replies; 7+ messages in thread From: Stephen Boyd @ 2020-04-25 17:53 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson Cc: linux-kernel, linux-arm-msm, Maulik Shah, Douglas Anderson 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-05 20:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-25 17:53 [PATCH v2 0/3] Even moar rpmh cleanups Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs Stephen Boyd 2020-04-28 0:13 ` Doug Anderson 2020-05-05 6:36 ` Stephen Boyd 2020-05-05 20:04 ` Doug Anderson 2020-04-25 17:53 ` [PATCH v2 2/3] soc: qcom: rpmh-rsc: Loop over fewer bits in irq handler Stephen Boyd 2020-04-25 17:53 ` [PATCH v2 3/3] soc: qcom: rpmh-rsc: Fold WARN_ON() into if condition Stephen Boyd
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).