From: rojay@codeaurora.org
To: Stephen Boyd <swboyd@chromium.org>
Cc: wsa@kernel.org, dianders@chromium.org,
saiprakash.ranjan@codeaurora.org, gregkh@linuxfoundation.org,
mka@chromium.org, akashast@codeaurora.org,
msavaliy@qti.qualcomm.com, skakit@codeaurora.org,
vkaur@codeaurora.org, pyarlaga@codeaurora.org,
rnayak@codeaurora.org, agross@kernel.org,
bjorn.andersson@linaro.org, linux-arm-msm@vger.kernel.org,
linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
sumit.semwal@linaro.org, linux-media@vger.kernel.org
Subject: Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Date: Mon, 21 Sep 2020 16:51:04 +0530 [thread overview]
Message-ID: <f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org> (raw)
In-Reply-To: <160037421089.4188128.9425314091585708436@swboyd.mtv.corp.google.com>
Hi Stephen,
On 2020-09-18 01:53, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index dead5db3315a..b0d8043c8cb2 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -86,6 +86,10 @@ struct geni_i2c_dev {
>> u32 clk_freq_out;
>> const struct geni_i2c_clk_fld *clk_fld;
>> int suspended;
>> + void *dma_buf;
>> + size_t xfer_len;
>> + dma_addr_t tx_dma;
>> + dma_addr_t rx_dma;
>
> Do we need both tx_dma and rx_dma? Seems that we use cur->flags to
> figure out if the transfer is tx or rx so we could have juat dma_buf
> and
> dma_addr here?
>
Okay.
>> };
>>
>> struct geni_i2c_err_log {
>> @@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct
>> geni_i2c_dev *gi2c)
>>
>> spin_lock_irqsave(&gi2c->lock, flags);
>> geni_i2c_err(gi2c, GENI_TIMEOUT);
>> - gi2c->cur = NULL;
>
> This looks concerning. We're moving this out from under the spinlock.
> The irq handler in this driver seems to hold the spinlock all the time
> while processing and this function grabs it here to keep cur consistent
> when aborting the transfer due to a timeout. Otherwise it looks like
> the
> irqhandler can race with this and try to complete the transfer while
> it's being torn down here.
>
>> geni_se_abort_m_cmd(&gi2c->se);
>> spin_unlock_irqrestore(&gi2c->lock, flags);
>> do {
>> @@ -349,10 +352,62 @@ static void geni_i2c_tx_fsm_rst(struct
>> geni_i2c_dev *gi2c)
>> dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
>> }
>>
>> +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c)
>
> So maybe pass cur to this function?
>
Sorry, i did not understand why to pass cur to this function?
>> +{
>> + struct geni_se *se = &gi2c->se;
>> +
>> + gi2c->cur_rd = 0;
>> + if (gi2c->dma_buf) {
>> + if (gi2c->err)
>> + geni_i2c_rx_fsm_rst(gi2c);
>> + geni_se_rx_dma_unprep(se, gi2c->rx_dma,
>> gi2c->xfer_len);
>> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur,
>> !gi2c->err);
>> + }
>> +}
>> +
>> +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c)
>
> And this one?
>
>> +{
>> + struct geni_se *se = &gi2c->se;
>> +
>> + gi2c->cur_wr = 0;
>> + if (gi2c->dma_buf) {
>> + if (gi2c->err)
>> + geni_i2c_tx_fsm_rst(gi2c);
>> + geni_se_tx_dma_unprep(se, gi2c->tx_dma,
>> gi2c->xfer_len);
>> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur,
>> !gi2c->err);
>> + }
>> +}
>> +
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + int ret;
>> + u32 geni_status;
>> +
>> + /* Resume device, as runtime suspend can happen anytime during
>> transfer */
>> + ret = pm_runtime_get_sync(gi2c->se.dev);
>> + if (ret < 0) {
>> + dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
>> ret);
>> + return;
>> + }
>> +
>> + geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
>
> And this probably needs to hold the lock?
>
>> + if (!(geni_status & M_GENI_CMD_ACTIVE))
>> + goto out;
>> +
>> + geni_i2c_abort_xfer(gi2c);
>> + if (gi2c->cur->flags & I2C_M_RD)
>> + geni_i2c_rx_msg_cleanup(gi2c);
>> + else
>> + geni_i2c_tx_msg_cleanup(gi2c);
>> + gi2c->cur = NULL;
>
> until here?
>
Okay.
>> +out:
>> + pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
>> i2c_msg *msg,
>> u32 m_param)
>> {
>> - dma_addr_t rx_dma;
>> + dma_addr_t rx_dma = 0;
>> unsigned long time_left;
>> void *dma_buf = NULL;
>> struct geni_se *se = &gi2c->se;
>> @@ -372,6 +427,10 @@ static int geni_i2c_rx_one_msg(struct
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> geni_se_select_mode(se, GENI_SE_FIFO);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> dma_buf = NULL;
>> + } else {
>> + gi2c->xfer_len = len;
>> + gi2c->rx_dma = rx_dma;
>> + gi2c->dma_buf = dma_buf;
>> }
>>
>> geni_se_setup_m_cmd(se, I2C_READ, m_param);
>> @@ -380,13 +439,7 @@ static int geni_i2c_rx_one_msg(struct
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> if (!time_left)
>> geni_i2c_abort_xfer(gi2c);
>>
>> - gi2c->cur_rd = 0;
>> - if (dma_buf) {
>> - if (gi2c->err)
>> - geni_i2c_rx_fsm_rst(gi2c);
>> - geni_se_rx_dma_unprep(se, rx_dma, len);
>> - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> - }
>> + geni_i2c_rx_msg_cleanup(gi2c);
>>
>> return gi2c->err;
>> }
>
> It may make sense to extract the cleanup stuff into another patch. Then
> have a patch after that which does the shutdown hook. So three patches
> total.
>
Okay, I will make separate patches, one for cleanup and another for
shutdown hook.
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c
>> b/drivers/soc/qcom/qcom-geni-se.c
>> index d0e4f520cff8..0216b38c1e9a 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -705,7 +705,7 @@ void geni_se_tx_dma_unprep(struct geni_se *se,
>> dma_addr_t iova, size_t len)
>> {
>> struct geni_wrapper *wrapper = se->wrapper;
>>
>> - if (iova && !dma_mapping_error(wrapper->dev, iova))
>> + if (!dma_mapping_error(wrapper->dev, iova))
>> dma_unmap_single(wrapper->dev, iova, len,
>> DMA_TO_DEVICE);
>> }
>> EXPORT_SYMBOL(geni_se_tx_dma_unprep);
>> @@ -722,7 +722,7 @@ void geni_se_rx_dma_unprep(struct geni_se *se,
>> dma_addr_t iova, size_t len)
>> {
>> struct geni_wrapper *wrapper = se->wrapper;
>>
>> - if (iova && !dma_mapping_error(wrapper->dev, iova))
>> + if (!dma_mapping_error(wrapper->dev, iova))
>> dma_unmap_single(wrapper->dev, iova, len,
>> DMA_FROM_DEVICE);
>> }
>> EXPORT_SYMBOL(geni_se_rx_dma_unprep);
>
> I'd make this a different patch. Nothing depends on this change, right?
Yes this is independent patch. I will make this as separate patch which
will be the third one.
Thanks,
Roja
next prev parent reply other threads:[~2020-09-21 11:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-17 12:25 [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
2020-09-17 20:23 ` Stephen Boyd
2020-09-21 11:21 ` rojay [this message]
2020-09-21 20:12 ` Stephen Boyd
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=f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org \
--to=rojay@codeaurora.org \
--cc=agross@kernel.org \
--cc=akashast@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mka@chromium.org \
--cc=msavaliy@qti.qualcomm.com \
--cc=pyarlaga@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=skakit@codeaurora.org \
--cc=sumit.semwal@linaro.org \
--cc=swboyd@chromium.org \
--cc=vkaur@codeaurora.org \
--cc=wsa@kernel.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.