| Message ID | 20200425175312.124892-2-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Even moar rpmh cleanups | expand |
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
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.
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
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; } /**
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(-)