From: Dmitry Osipenko <digetx@gmail.com>
To: Sowjanya Komatineni <skomatineni@nvidia.com>,
"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Mantravadi Karthik <mkarthik@nvidia.com>,
Shardar Mohammed <smohammed@nvidia.com>,
Timo Alho <talho@nvidia.com>
Cc: "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>
Subject: Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support
Date: Thu, 31 Jan 2019 05:53:15 +0300 [thread overview]
Message-ID: <725aece7-d3f2-c26f-4af4-200d8b507123@gmail.com> (raw)
In-Reply-To: <BYAPR12MB3398CE89D24AC71BF7DAF43CC2910@BYAPR12MB3398.namprd12.prod.outlook.com>
31.01.2019 5:24, Sowjanya Komatineni пишет:
>>>>> time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
>>>>> TEGRA_I2C_TIMEOUT);
>>>>> tegra_i2c_mask_irq(i2c_dev, int_mask);
>>>>>
>>>>> if (time_left == 0) {
>>>>> dev_err(i2c_dev->dev, "i2c transfer timed out\n");
>>>>> + if (dma) {
>>>>> + dmaengine_terminate_all(chan);
>>>>> + complete(&i2c_dev->dma_complete);
>>>>> + }
>>>>
>>>> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.
>>>
>>> DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
>>> So still need to check for msg_complete timeout.
>>
>> You're waiting for DMA completion and then for the I2C message completion.
>>
>> Hence your code is structured like this:
>>
>> 1. Issue DMA transfer
>> 2. Wait for DMA completion
>> 3. Wait for message completion
>>
>> Why do you need to abort DMA in 3 if it was already completed in 2?
>
> Ok, thought you are referring to msg complete timeout check in dma mode. Yes no need for terminating DMA when msg timeout. Will fix it.
>
>>>>> @@ -740,6 +925,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>>>> u32 int_mask;
>>>>> unsigned long time_left;
>>>>> unsigned long flags;
>>>>> + size_t xfer_size;
>>>>> + u32 *buffer = 0;
>>>>> + int ret = 0;
>>>>> + bool dma = false;
>>>>> +
>>>>> + if (msg->flags & I2C_M_RD)
>>>>> + xfer_size = msg->len;
>>>>> + else
>>>>> + xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
>>>>> +
>>>>> + xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
>>>>> + dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
>>>>> + if (dma) {
>>>>> + if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
>>>>> + ret = tegra_i2c_init_dma_param(i2c_dev, true);
>>>>> + else if (!i2c_dev->tx_dma_chan)
>>>>> + ret = tegra_i2c_init_dma_param(i2c_dev, false);
>>>>
>>>> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
>>>>
>>>> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
>>>>
>>>>> + if (ret < 0) {
>>>>> + dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
>>>>> + dma = false;
>>>>> + ret = 0;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + i2c_dev->is_curr_dma_xfer = dma;
>>>>
>>>>
>>> Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
>>> So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.
>>>
>>>
>>
>> Yes, I made that comment, but then corrected myself. Seems you missed the correction: https://lkml.org/lkml/2019/1/26/217
>>
>> If you're having troubles with the corporate email, maybe you could try to switch to something else like gmail.
>>
>> I've tried to apply this series locally, but again it fails to apply. What's the kernel base you're using? You should make your patches on top linux-next (preferably) or mainline.
>
> Yeah somehow missed that. Will move it back to probe..
> I am using 5.0-rc1
>
Managed to apply patches this time, was my bad. Please take a look and reply to the rest of my comments before sending a new version, thanks.
next prev parent reply other threads:[~2019-01-31 2:53 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-30 16:01 [PATCH V7 1/5] i2c: tegra: Sort all the include headers alphabetically Sowjanya Komatineni
2019-01-30 16:01 ` [PATCH V7 2/5] i2c: tegra: Add Bus Clear Master Support Sowjanya Komatineni
2019-01-31 11:31 ` Thierry Reding
2019-01-30 16:01 ` [PATCH V7 3/5] i2c: tegra: Add DMA Support Sowjanya Komatineni
2019-01-31 0:05 ` Dmitry Osipenko
2019-01-31 1:19 ` Sowjanya Komatineni
2019-01-31 2:13 ` Dmitry Osipenko
2019-01-31 2:24 ` Sowjanya Komatineni
2019-01-31 2:53 ` Dmitry Osipenko [this message]
2019-01-31 3:34 ` Sowjanya Komatineni
2019-01-31 4:01 ` Dmitry Osipenko
2019-01-31 12:06 ` Thierry Reding
2019-01-31 14:06 ` Dmitry Osipenko
2019-01-31 14:43 ` Thierry Reding
2019-01-31 15:02 ` Dmitry Osipenko
2019-01-31 16:01 ` Thierry Reding
2019-01-31 16:27 ` Dmitry Osipenko
2019-01-31 16:31 ` Dmitry Osipenko
2019-01-31 19:02 ` Dmitry Osipenko
2019-01-31 16:55 ` Dmitry Osipenko
2019-01-31 18:08 ` Dmitry Osipenko
2019-01-31 18:15 ` Dmitry Osipenko
2019-01-30 16:01 ` [PATCH V7 4/5] i2c: tegra: Update transfer timeout Sowjanya Komatineni
2019-01-31 1:38 ` Dmitry Osipenko
2019-01-30 16:01 ` [PATCH V7 5/5] i2c: tegra: Add I2C interface timing support Sowjanya Komatineni
2019-01-31 4:30 ` Dmitry Osipenko
2019-01-31 11:28 ` [PATCH V7 1/5] i2c: tegra: Sort all the include headers alphabetically Thierry Reding
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=725aece7-d3f2-c26f-4af4-200d8b507123@gmail.com \
--to=digetx@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=mkarthik@nvidia.com \
--cc=skomatineni@nvidia.com \
--cc=smohammed@nvidia.com \
--cc=talho@nvidia.com \
--cc=thierry.reding@gmail.com \
/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;
as well as URLs for NNTP newsgroup(s).