| Message ID | 20250603153022.39434-3-akhilrajeev@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] dt-bindings: i2c: nvidia,tegra20-i2c: Specify the required properties | expand |
On Tue, Jun 03, 2025 at 09:00:22PM +0530, Akhil R wrote: > Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless. > The driver should not be doing its own bounce-buffering if the buffer is > allocated through dma_alloc_coherent() > > Suggested-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > v3->v4: No change > v2->v3: No change > v1->v2: No change > > drivers/i2c/busses/i2c-tegra.c | 20 +------------------- > 1 file changed, 1 insertion(+), 19 deletions(-) I've had a patch like this in my local tree for a while and never got around to send it out. It turns out that this is actually not just unnecessary but can also cause issues. We were seeing really strange crashes in the QSPI driver that seem related to this. I don't know exactly what causes it, but removing the dma_sync_*() calls (for memory that is also dma_alloc_coherent()-allocated) fixes things. For QSPI I'm thinking it would probably be better to move to streaming DMA mappings because the buffers in DMA mode are sufficiently large (at least 2 cache lines) that we might see some benefit from the caching. I don't know if this would apply for I2C as well? I vaguely recall that certain transfers can be quite large and I wonder if cached mappings would be advantageous here as well. Do you have good data on what the usage patterns are? That said, maybe the extra amount of work isn't worth it because both I2C and QSPI are fairly slow busses, so the impact of caches is probably minimal in comparison. Anyway: Reviewed-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 22ddbae9d847..b10a4bc9cb34 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -1292,17 +1292,9 @@ 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->dma_dev, - i2c_dev->dma_phys, - xfer_size, DMA_FROM_DEVICE); - err = tegra_i2c_dma_submit(i2c_dev, xfer_size); if (err) return err; - } else { - dma_sync_single_for_cpu(i2c_dev->dma_dev, - i2c_dev->dma_phys, - xfer_size, DMA_TO_DEVICE); } } @@ -1312,11 +1304,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, if (i2c_dev->dma_mode) { memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE, msg->buf, i2c_dev->msg_len); - - dma_sync_single_for_device(i2c_dev->dma_dev, - i2c_dev->dma_phys, - xfer_size, DMA_TO_DEVICE); - err = tegra_i2c_dma_submit(i2c_dev, xfer_size); if (err) return err; @@ -1357,13 +1344,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, return -ETIMEDOUT; } - if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE) { - dma_sync_single_for_cpu(i2c_dev->dma_dev, - i2c_dev->dma_phys, - xfer_size, DMA_FROM_DEVICE); - + if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE) memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, i2c_dev->msg_len); - } } time_left = tegra_i2c_wait_completion(i2c_dev, &i2c_dev->msg_complete,
Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless. The driver should not be doing its own bounce-buffering if the buffer is allocated through dma_alloc_coherent() Suggested-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- v3->v4: No change v2->v3: No change v1->v2: No change drivers/i2c/busses/i2c-tegra.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)