* [PATCH] i2c: tegra: Allocate DMA memory for DMA engine @ 2022-10-20 14:39 Thierry Reding 2022-10-20 15:14 ` Robin Murphy 2022-11-01 12:38 ` Wolfram Sang 0 siblings, 2 replies; 4+ messages in thread From: Thierry Reding @ 2022-10-20 14:39 UTC (permalink / raw) To: Wolfram Sang Cc: Dmitry Osipenko, Jon Hunter, linux-i2c, linux-tegra, Robin Murphy 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"); @@ -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); -- 2.37.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] i2c: tegra: Allocate DMA memory for DMA engine 2022-10-20 14:39 [PATCH] i2c: tegra: Allocate DMA memory for DMA engine Thierry Reding @ 2022-10-20 15:14 ` Robin Murphy 2022-10-20 16:07 ` Thierry Reding 2022-11-01 12:38 ` Wolfram Sang 1 sibling, 1 reply; 4+ messages in thread From: Robin Murphy @ 2022-10-20 15:14 UTC (permalink / raw) To: Thierry Reding, Wolfram Sang Cc: Dmitry Osipenko, Jon Hunter, linux-i2c, linux-tegra 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); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i2c: tegra: Allocate DMA memory for DMA engine 2022-10-20 15:14 ` Robin Murphy @ 2022-10-20 16:07 ` Thierry Reding 0 siblings, 0 replies; 4+ messages in thread From: Thierry Reding @ 2022-10-20 16:07 UTC (permalink / raw) To: Robin Murphy Cc: Wolfram Sang, Dmitry Osipenko, Jon Hunter, linux-i2c, linux-tegra [-- Attachment #1: Type: text/plain, Size: 4906 bytes --] On Thu, Oct 20, 2022 at 04:14:20PM +0100, Robin Murphy wrote: > 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. Yes, I've had those comments in the back of my mind and wanted to get this out separately because this is needed to redress an issue that we've introduced in the DT files for these devices, where now all I2C controllers are sorted into the same IOMMU group as the DMA controller, which doesn't make any sense as they are not themselves performing any DMA accesses. I'll look at the synchronization bits next. I suspect that you're right about the sync calls all being redundant. If dma_alloc_coherent() memory is always guaranteed to be uncached (unless perhaps if dma-coherent is specified for the DMA device), then I don't see why they should be needed. Thierry > > 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); [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i2c: tegra: Allocate DMA memory for DMA engine 2022-10-20 14:39 [PATCH] i2c: tegra: Allocate DMA memory for DMA engine Thierry Reding 2022-10-20 15:14 ` Robin Murphy @ 2022-11-01 12:38 ` Wolfram Sang 1 sibling, 0 replies; 4+ messages in thread From: Wolfram Sang @ 2022-11-01 12:38 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Osipenko, Jon Hunter, linux-i2c, linux-tegra, Robin Murphy [-- Attachment #1: Type: text/plain, Size: 713 bytes --] On Thu, Oct 20, 2022 at 04:39:33PM +0200, 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> Applied to for-current, so you can work on the next steps on top of this soon. Thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-01 12:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-20 14:39 [PATCH] i2c: tegra: Allocate DMA memory for DMA engine Thierry Reding 2022-10-20 15:14 ` Robin Murphy 2022-10-20 16:07 ` Thierry Reding 2022-11-01 12:38 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox