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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox