From: Robin Murphy <robin.murphy@arm.com>
To: Thierry Reding <thierry.reding@gmail.com>, Wolfram Sang <wsa@kernel.org>
Cc: Dmitry Osipenko <digetx@gmail.com>,
Jon Hunter <jonathanh@nvidia.com>,
linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH] i2c: tegra: Allocate DMA memory for DMA engine
Date: Thu, 20 Oct 2022 16:14:20 +0100 [thread overview]
Message-ID: <b258387c-365e-e18d-7b7c-e38105786193@arm.com> (raw)
In-Reply-To: <20221020143933.1951609-1-thierry.reding@gmail.com>
On 2022-10-20 15:39, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> When the I2C controllers are running in DMA mode, it is the DMA engine
> that performs the memory accesses rather than the I2C controller. Pass
> the DMA engine's struct device pointer to the DMA API to make sure the
> correct DMA operations are used.
>
> This fixes an issue where the DMA engine's SMMU stream ID needs to be
> misleadingly set for the I2C controllers in device tree.
>
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> drivers/i2c/busses/i2c-tegra.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 954022c04cc4..3869c258a529 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -284,6 +284,7 @@ struct tegra_i2c_dev {
> struct dma_chan *tx_dma_chan;
> struct dma_chan *rx_dma_chan;
> unsigned int dma_buf_size;
> + struct device *dma_dev;
> dma_addr_t dma_phys;
> void *dma_buf;
>
> @@ -420,7 +421,7 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
> static void tegra_i2c_release_dma(struct tegra_i2c_dev *i2c_dev)
> {
> if (i2c_dev->dma_buf) {
> - dma_free_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
> + dma_free_coherent(i2c_dev->dma_dev, i2c_dev->dma_buf_size,
> i2c_dev->dma_buf, i2c_dev->dma_phys);
> i2c_dev->dma_buf = NULL;
> }
> @@ -472,10 +473,13 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>
> i2c_dev->tx_dma_chan = chan;
>
> + WARN_ON(i2c_dev->tx_dma_chan->device != i2c_dev->rx_dma_chan->device);
> + i2c_dev->dma_dev = chan->device->dev;
> +
> i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len +
> I2C_PACKET_HEADER_SIZE;
>
> - dma_buf = dma_alloc_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
> + dma_buf = dma_alloc_coherent(i2c_dev->dma_dev, i2c_dev->dma_buf_size,
> &dma_phys, GFP_KERNEL | __GFP_NOWARN);
> if (!dma_buf) {
> dev_err(i2c_dev->dev, "failed to allocate DMA buffer\n");
That much is definitely better, but as mentioned previously all the
syncs below look completely bogus either way, so should really be
removed rather than "fixed". If it's necessary to ensure ordering of CPU
accesses to the buffer relative to the DMA transfer itself, where that
isn't already implicit in some readl()/writel() involved in starting and
stopping the DMA channel, then dma_wmb()/dma_rmb() are the tools for
that job.
Thanks,
Robin.
> @@ -1272,7 +1276,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>
> if (i2c_dev->dma_mode) {
> if (i2c_dev->msg_read) {
> - dma_sync_single_for_device(i2c_dev->dev,
> + dma_sync_single_for_device(i2c_dev->dma_dev,
> i2c_dev->dma_phys,
> xfer_size, DMA_FROM_DEVICE);
>
> @@ -1280,7 +1284,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> if (err)
> return err;
> } else {
> - dma_sync_single_for_cpu(i2c_dev->dev,
> + dma_sync_single_for_cpu(i2c_dev->dma_dev,
> i2c_dev->dma_phys,
> xfer_size, DMA_TO_DEVICE);
> }
> @@ -1293,7 +1297,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE,
> msg->buf, msg->len);
>
> - dma_sync_single_for_device(i2c_dev->dev,
> + dma_sync_single_for_device(i2c_dev->dma_dev,
> i2c_dev->dma_phys,
> xfer_size, DMA_TO_DEVICE);
>
> @@ -1344,7 +1348,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> }
>
> if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE) {
> - dma_sync_single_for_cpu(i2c_dev->dev,
> + dma_sync_single_for_cpu(i2c_dev->dma_dev,
> i2c_dev->dma_phys,
> xfer_size, DMA_FROM_DEVICE);
>
next prev parent reply other threads:[~2022-10-20 15:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-20 14:39 [PATCH] i2c: tegra: Allocate DMA memory for DMA engine Thierry Reding
2022-10-20 15:14 ` Robin Murphy [this message]
2022-10-20 16:07 ` Thierry Reding
2022-11-01 12:38 ` Wolfram Sang
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=b258387c-365e-e18d-7b7c-e38105786193@arm.com \
--to=robin.murphy@arm.com \
--cc=digetx@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=thierry.reding@gmail.com \
--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