From: Maulik Shah <mkshah@codeaurora.org>
To: Douglas Anderson <dianders@chromium.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Rajendra Nayak <rnayak@codeaurora.org>,
mka@chromium.org, evgreen@chromium.org, swboyd@chromium.org,
Lina Iyer <ilina@codeaurora.org>,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds
Date: Wed, 11 Mar 2020 14:17:17 +0530 [thread overview]
Message-ID: <85758e97-8c0c-5c4e-24ad-d3e8b2b01d3c@codeaurora.org> (raw)
In-Reply-To: <20200306155707.RFT.1.I1b754137e8089e46cf33fc2ea270734ec3847ec4@changeid>
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;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2020-03-11 8:47 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-06 23:59 [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
2020-03-11 8:47 ` Maulik Shah [this message]
2020-03-11 15:03 ` Doug Anderson
2020-03-11 16:17 ` Matthias Kaehlcke
2020-03-11 19:30 ` Stephen Boyd
2020-03-06 23:59 ` [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
2020-03-11 9:35 ` Maulik Shah
2020-03-11 15:27 ` Doug Anderson
2020-03-11 18:49 ` Evan Green
2020-03-11 20:08 ` Stephen Boyd
2020-03-11 22:35 ` Doug Anderson
2020-03-06 23:59 ` [RFT PATCH 3/9] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
2020-03-11 9:50 ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 4/9] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
2020-03-11 12:02 ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 5/9] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY Douglas Anderson
2020-03-11 0:33 ` Doug Anderson
2020-03-06 23:59 ` [RFT PATCH 7/9] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
2020-03-11 0:35 ` Doug Anderson
2020-03-11 9:48 ` [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Maulik Shah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85758e97-8c0c-5c4e-24ad-d3e8b2b01d3c@codeaurora.org \
--to=mkshah@codeaurora.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=ilina@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mka@chromium.org \
--cc=rnayak@codeaurora.org \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.