| Message ID | 20200306155707.RFT.1.I1b754137e8089e46cf33fc2ea270734ec3847ec4@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, On 3/7/2020 5:29 AM, Douglas Anderson wrote: > This patch makes two changes, both of which should be no-ops: > > 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() / > write_tcs_cmd(). i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id) can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg(). this way we have only one read and one write function. so at the end we will two function as, static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) { return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + RSC_DRV_CMD_OFFSET * cmd_id); } static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, u32 data) { writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + RSC_DRV_CMD_OFFSET * cmd_id); } > > 2. Change the order of operations in the above functions to make it > more obvious to me what the math is doing. Specifically first you > want to find the right TCS, then the right register, and then > multiply by the command ID if necessary. With above change, i don't think you need to re-order this. specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs. Thanks, Maulik > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index e278fc11fe5c..5c88b8cd5bf8 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -61,28 +61,33 @@ > #define CMD_STATUS_ISSUED BIT(8) > #define CMD_STATUS_COMPL BIT(16) > > -static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) > +static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) > { > - return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > + return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg + > RSC_DRV_CMD_OFFSET * cmd_id); > } > > +static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id) > +{ > + return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); > +} > + > static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, > u32 data) > { > - writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > + writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg + > RSC_DRV_CMD_OFFSET * cmd_id); > } > > static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data) > { > - writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id); > + writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); > } > > static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, > u32 data) > { > - writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id); > + writel(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); > for (;;) { > if (data == readl(drv->tcs_base + reg + > RSC_DRV_TCS_OFFSET * tcs_id)) > @@ -94,7 +99,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, > static bool tcs_is_free(struct rsc_drv *drv, int tcs_id) > { > return !test_bit(tcs_id, drv->tcs_in_use) && > - read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0); > + read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id); > } > > static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) > @@ -212,7 +217,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) > const struct tcs_request *req; > struct tcs_cmd *cmd; > > - irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0); > + irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0); > > for_each_set_bit(i, &irq_status, BITS_PER_LONG) { > req = get_req_from_tcs(drv, i); > @@ -226,7 +231,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) > u32 sts; > > cmd = &req->cmds[j]; > - sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, j); > + sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j); > if (!(sts & CMD_STATUS_ISSUED) || > ((req->wait_for_compl || cmd->wait) && > !(sts & CMD_STATUS_COMPL))) { > @@ -265,7 +270,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id, > cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0; > cmd_msgid |= CMD_MSGID_WRITE; > > - cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0); > + cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id); > > for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) { > cmd = &msg->cmds[i]; > @@ -281,7 +286,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id, > } > > write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete); > - cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0); > + cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id); > write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable); > } > > @@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id) > * While clearing ensure that the AMC mode trigger is cleared > * and then the mode enable is cleared. > */ > - enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0); > + enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id); > enable &= ~TCS_AMC_MODE_TRIGGER; > write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); > enable &= ~TCS_AMC_MODE_ENABLE; > @@ -319,10 +324,10 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs, > if (tcs_is_free(drv, tcs_id)) > continue; > > - curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0); > + curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id); > > for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) { > - addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j); > + addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j); > for (k = 0; k < msg->num_cmds; k++) { > if (addr == msg->cmds[k].addr) > return -EBUSY;
Hi, On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > Hi, > > On 3/7/2020 5:29 AM, Douglas Anderson wrote: > > This patch makes two changes, both of which should be no-ops: > > > > 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() / > > write_tcs_cmd(). > > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id) > > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg(). > this way we have only one read and one write function. > > so at the end we will two function as, > > static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) > { > return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > RSC_DRV_CMD_OFFSET * cmd_id); > } > > static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, > u32 data) > { > writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > RSC_DRV_CMD_OFFSET * cmd_id); > } I can if you insist and this is still better than the existing (inconsistent) code. ...but I still feel that having two functions adds value here. Anyone else who is CCed want to weigh in and tie break? > > 2. Change the order of operations in the above functions to make it > > more obvious to me what the math is doing. Specifically first you > > want to find the right TCS, then the right register, and then > > multiply by the command ID if necessary. > With above change, i don't think you need to re-order this. > specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs. There was never any "need" to re-order. That math works out to be the same. This is just clearer. As an example, let's look at this: struct point { int x; int y; }; struct point points[10]; Let's say you have: void *points_base = &(points[0]); ...and now you want to find &(points[5].y). What does your math look like? a) points_base + (sizeof(struct point) * 5) + 4 ; ...or... b) points_base + 4 + (sizeof(struct point) * 5); Both calculations give the same result, but I am arguring that "a)" is more intuitive. Specifically you deal with the array access first and then deal with the offset within the structure that you found. -Doug
Hi, On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote: > Hi, > > On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > > > Hi, > > > > On 3/7/2020 5:29 AM, Douglas Anderson wrote: > > > This patch makes two changes, both of which should be no-ops: > > > > > > 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() / > > > write_tcs_cmd(). > > > > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id) > > > > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg(). > > this way we have only one read and one write function. > > > > so at the end we will two function as, > > > > static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) > > { > > return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > > RSC_DRV_CMD_OFFSET * cmd_id); > > } > > > > static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, > > u32 data) > > { > > writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > > RSC_DRV_CMD_OFFSET * cmd_id); > > } > > I can if you insist and this is still better than the existing > (inconsistent) code. > > ...but I still feel that having two functions adds value here. > > > Anyone else who is CCed want to weigh in and tie break? I agree with Doug, having two functions makes the code that calls them clearer. It makes it evident when a command is read/written and doesn't require a useless extra parameter when accessing a non-command register. > > > 2. Change the order of operations in the above functions to make it > > > more obvious to me what the math is doing. Specifically first you > > > want to find the right TCS, then the right register, and then > > > multiply by the command ID if necessary. > > With above change, i don't think you need to re-order this. > > specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs. > > There was never any "need" to re-order. That math works out to be the > same. This is just clearer. > > As an example, let's look at this: > > struct point { > int x; > int y; > }; > struct point points[10]; > > Let's say you have: > void *points_base = &(points[0]); > > ...and now you want to find &(points[5].y). What does your math look like? > > a) points_base + (sizeof(struct point) * 5) + 4 ; > > ...or... > > b) points_base + 4 + (sizeof(struct point) * 5); > > > Both calculations give the same result, but I am arguring that "a)" is > more intuitive. Specifically you deal with the array access first and > then deal with the offset within the structure that you found. +1
Quoting Matthias Kaehlcke (2020-03-11 09:17:26) > Hi, > > On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > > > > > Hi, > > > > > > On 3/7/2020 5:29 AM, Douglas Anderson wrote: > > > > This patch makes two changes, both of which should be no-ops: > > > > > > > > 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() / > > > > write_tcs_cmd(). > > > > > > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id) > > > > > > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg(). > > > this way we have only one read and one write function. > > > > > > so at the end we will two function as, > > > > > > static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) > > > { > > > return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > > > RSC_DRV_CMD_OFFSET * cmd_id); > > > } > > > > > > static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, > > > u32 data) > > > { > > > writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + > > > RSC_DRV_CMD_OFFSET * cmd_id); > > > } > > > > I can if you insist and this is still better than the existing > > (inconsistent) code. > > > > ...but I still feel that having two functions adds value here. > > > > > > Anyone else who is CCed want to weigh in and tie break? > > I agree with Doug, having two functions makes the code that calls them > clearer. It makes it evident when a command is read/written and doesn't require > a useless extra parameter when accessing a non-command register. Me too! In fact, I asked for this when this driver was introduced and I was half-ignored[1]. Making sure we never have to pass 0 to one of these functions should be a goal. From two years ago: > > Is m the type of TCS (sleep, active, wake) and n is just an offset? > Maybe you can replace m with 'tcs_type' and n with 'index' or 'i' or > 'offset'. And then don't use this function to write the random TCS > registers that don't have to do with the TCS command slots? I see > various places where there are things like: > > > + write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0); > > + write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete); > > + write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable); > > And 'n' is 0, meaning you rely on that 0 killing that last part of the > equation (RSC_DRV_CMD_OFFSET * n). But if we had a write_tcs_reg(drv, > reg, m, data) and a write_tcs_cmd(drv, reg, m, n, data) then it would be > clearer. > > Even better, add a void *base to a 'struct tcs' and then pass that > struct to the tcs_read/write APIs and then have that pull out a > tcs->base + reg or tcs->base + reg + RSC_DRV_CMD_OFFSET * index. > [1] https://lore.kernel.org/linux-arm-msm/152364140661.51482.261490347611407195@swboyd.mtv.corp.google.com/
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index e278fc11fe5c..5c88b8cd5bf8 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -61,28 +61,33 @@ #define CMD_STATUS_ISSUED BIT(8) #define CMD_STATUS_COMPL BIT(16) -static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) +static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id) { - return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + + return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg + RSC_DRV_CMD_OFFSET * cmd_id); } +static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id) +{ + return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); +} + static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id, u32 data) { - writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id + + writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg + RSC_DRV_CMD_OFFSET * cmd_id); } static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data) { - writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id); + writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); } static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, u32 data) { - writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id); + writel(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg); for (;;) { if (data == readl(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id)) @@ -94,7 +99,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id, static bool tcs_is_free(struct rsc_drv *drv, int tcs_id) { return !test_bit(tcs_id, drv->tcs_in_use) && - read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0); + read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id); } static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) @@ -212,7 +217,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) const struct tcs_request *req; struct tcs_cmd *cmd; - irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0); + irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0); for_each_set_bit(i, &irq_status, BITS_PER_LONG) { req = get_req_from_tcs(drv, i); @@ -226,7 +231,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) u32 sts; cmd = &req->cmds[j]; - sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, j); + sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j); if (!(sts & CMD_STATUS_ISSUED) || ((req->wait_for_compl || cmd->wait) && !(sts & CMD_STATUS_COMPL))) { @@ -265,7 +270,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id, cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0; cmd_msgid |= CMD_MSGID_WRITE; - cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0); + cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id); for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) { cmd = &msg->cmds[i]; @@ -281,7 +286,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id, } write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete); - cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0); + cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id); write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable); } @@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id) * While clearing ensure that the AMC mode trigger is cleared * and then the mode enable is cleared. */ - enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0); + enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id); enable &= ~TCS_AMC_MODE_TRIGGER; write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); enable &= ~TCS_AMC_MODE_ENABLE; @@ -319,10 +324,10 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs, if (tcs_is_free(drv, tcs_id)) continue; - curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0); + curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id); for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) { - addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j); + addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j); for (k = 0; k < msg->num_cmds; k++) { if (addr == msg->cmds[k].addr) return -EBUSY;
This patch makes two changes, both of which should be no-ops: 1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() / write_tcs_cmd(). 2. Change the order of operations in the above functions to make it more obvious to me what the math is doing. Specifically first you want to find the right TCS, then the right register, and then multiply by the command ID if necessary. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)