diff mbox series

[v4,01/10] drivers: qcom: rpmh-rsc: Clean code reading/writing TCS regs/cmds

Message ID 20200413100321.v4.1.I1b754137e8089e46cf33fc2ea270734ec3847ec4@changeid (mailing list archive)
State New, archived
Headers show
Series drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand

Commit Message

Doug Anderson April 13, 2020, 5:04 p.m. UTC 
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>
Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>
---

Changes in v4: None
Changes in v3:
- Add "TCS" in title (Maulik).
- Rebased atop v16 ('Invoke rpmh_flush...') series.

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Joe Perches April 13, 2020, 6:19 p.m. UTC | #1 
On Mon, 2020-04-13 at 10:04 -0700, 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().
> 
> 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.

Though these operations are only used a couple times, perhaps
it'd be useful to have static inlines for the calcs.

> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
[]
> @@ -67,28 +67,33 @@
>  #define CMD_STATUS_ISSUED		BIT(8)
>  #define CMD_STATUS_COMPL		BIT(16)

Maybe something like:

static inline void __iomem *
tcs_reg_addr(struct rsc_drv drv, int reg, int tcs_id)
{
	return drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg;
}

static inline void __iomem *
tcs_cmd_addr(struct rsc_drv drv, int reg, int tcs_id, int cmd_id)
{
	return tcs_reg_addr(drv, reg, tcs_id) + RSC_DRV_CMD_OFFSET * cmd_id;
}

> -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);

	return readl_relaxed(tcs_cmd_addr(drv, reg, tcs_id, cmd_id));

etc...
Doug Anderson April 13, 2020, 9:18 p.m. UTC | #2 
Hi,

On Mon, Apr 13, 2020 at 11:21 AM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-04-13 at 10:04 -0700, 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().
> >
> > 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.
>
> Though these operations are only used a couple times, perhaps
> it'd be useful to have static inlines for the calcs.
>
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> []
> > @@ -67,28 +67,33 @@
> >  #define CMD_STATUS_ISSUED            BIT(8)
> >  #define CMD_STATUS_COMPL             BIT(16)
>
> Maybe something like:
>
> static inline void __iomem *
> tcs_reg_addr(struct rsc_drv drv, int reg, int tcs_id)
> {
>         return drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg;
> }
>
> static inline void __iomem *
> tcs_cmd_addr(struct rsc_drv drv, int reg, int tcs_id, int cmd_id)
> {
>         return tcs_reg_addr(drv, reg, tcs_id) + RSC_DRV_CMD_OFFSET * cmd_id;
> }
>
> > -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);
>
>         return readl_relaxed(tcs_cmd_addr(drv, reg, tcs_id, cmd_id));
>
> etc...

I won't object if you really feel passionately about making that
change but at this point it doesn't add tons of extra readability for
me personally.  I was kinda hoping that Maulik and my series could
land in the next few days since having 16 patches outstanding gets a
bit unwieldy.  I'd rather not send out another spin of my series at
this point since it's just a bunch more churn in everyone's inboxes.
Maybe after they land you can post that as a follow-up cleanup?

-Doug
Stephen Boyd April 13, 2020, 9:20 p.m. UTC | #3 
Quoting Douglas Anderson (2020-04-13 10:04:06)
> 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>
> Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
> Tested-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Joe Perches April 13, 2020, 9:33 p.m. UTC | #4 
On Mon, 2020-04-13 at 14:18 -0700, Doug Anderson wrote:
> Hi,

Rehi.

> On Mon, Apr 13, 2020 at 11:21 AM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-04-13 at 10:04 -0700, 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().
> > > 
> > > 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.
> > 
> > Though these operations are only used a couple times, perhaps
> > it'd be useful to have static inlines for the calcs.
> > 
> > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > []
> > > @@ -67,28 +67,33 @@
> > >  #define CMD_STATUS_ISSUED            BIT(8)
> > >  #define CMD_STATUS_COMPL             BIT(16)
> > 
> > Maybe something like:
> > 
> > static inline void __iomem *
> > tcs_reg_addr(struct rsc_drv drv, int reg, int tcs_id)
> > {
> >         return drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg;
> > }
> > 
> > static inline void __iomem *
> > tcs_cmd_addr(struct rsc_drv drv, int reg, int tcs_id, int cmd_id)
> > {
> >         return tcs_reg_addr(drv, reg, tcs_id) + RSC_DRV_CMD_OFFSET * cmd_id;
> > }
> > 
> > > -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);
> > 
> >         return readl_relaxed(tcs_cmd_addr(drv, reg, tcs_id, cmd_id));
> > 
> > etc...
> 
> I won't object if you really feel passionately about making that
> change but at this point it doesn't add tons of extra readability for
> me personally.

Just a suggestion.

> I was kinda hoping that Maulik and my series could
> land in the next few days since having 16 patches outstanding gets a
> bit unwieldy.  I'd rather not send out another spin of my series at
> this point since it's just a bunch more churn in everyone's inboxes.
> Maybe after they land you can post that as a follow-up cleanup?

If I remember...

cheers, Joe
Doug Anderson April 14, 2020, 5:43 p.m. UTC | #5 
Hi,

On Mon, Apr 13, 2020 at 2:35 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-04-13 at 14:18 -0700, Doug Anderson wrote:
> > Hi,
>
> Rehi.
>
> > On Mon, Apr 13, 2020 at 11:21 AM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-04-13 at 10:04 -0700, 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().
> > > >
> > > > 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.
> > >
> > > Though these operations are only used a couple times, perhaps
> > > it'd be useful to have static inlines for the calcs.
> > >
> > > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > > []
> > > > @@ -67,28 +67,33 @@
> > > >  #define CMD_STATUS_ISSUED            BIT(8)
> > > >  #define CMD_STATUS_COMPL             BIT(16)
> > >
> > > Maybe something like:
> > >
> > > static inline void __iomem *
> > > tcs_reg_addr(struct rsc_drv drv, int reg, int tcs_id)
> > > {
> > >         return drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg;
> > > }
> > >
> > > static inline void __iomem *
> > > tcs_cmd_addr(struct rsc_drv drv, int reg, int tcs_id, int cmd_id)
> > > {
> > >         return tcs_reg_addr(drv, reg, tcs_id) + RSC_DRV_CMD_OFFSET * cmd_id;
> > > }
> > >
> > > > -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);
> > >
> > >         return readl_relaxed(tcs_cmd_addr(drv, reg, tcs_id, cmd_id));
> > >
> > > etc...
> >
> > I won't object if you really feel passionately about making that
> > change but at this point it doesn't add tons of extra readability for
> > me personally.
>
> Just a suggestion.

I tried it and after looking at it again, it definitely does make it cleaner.


> > I was kinda hoping that Maulik and my series could
> > land in the next few days since having 16 patches outstanding gets a
> > bit unwieldy.  I'd rather not send out another spin of my series at
> > this point since it's just a bunch more churn in everyone's inboxes.
> > Maybe after they land you can post that as a follow-up cleanup?
>
> If I remember...

http://lore.kernel.org/r/20200414104120.1.Ic70288f256ff0be65cac6a600367212dfe39f6c9@changeid

-Doug
diff mbox series

Patch

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index cc1293cb15a5..91fb5a6d68a2 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -67,28 +67,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))
@@ -100,7 +105,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)
@@ -207,7 +212,7 @@  static void __tcs_set_trigger(struct rsc_drv *drv, int tcs_id, bool trigger)
 	 * 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;
@@ -226,7 +231,7 @@  static void enable_tcs_irq(struct rsc_drv *drv, int tcs_id, bool enable)
 {
 	u32 data;
 
-	data = read_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0);
+	data = read_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0);
 	if (enable)
 		data |= BIT(tcs_id);
 	else
@@ -245,7 +250,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);
@@ -259,7 +264,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))) {
@@ -313,7 +318,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];
@@ -329,7 +334,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);
 }
 
@@ -345,10 +350,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;