> > > + if (dma) {
> > > + if (i2c_dev->msg_read) {
> > > + chan = i2c_dev->rx_dma_chan;
> > > + tegra_i2c_config_fifo_trig(i2c_dev,
> > > xfer_size,
> > > +
> > > DATA_DMA_DIR_RX);
> > > + dma_sync_single_for_device(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > + xfer_size,
> > > +
> > > DMA_FROM_DEVICE);
> >
> > Do we really need this? We're not actually passing the device any
> > data, so no caches to flush here. I we're cautious about flushing
> > caches when we do write to the buffer (and I think we do that properly
> > already), then there should be no need to do it here again.
> >
>
> IIUC, DMA API has a concept of buffer handing which tells to use
dma_sync_single_for_device() before issuing hardware job that touches the buffer and to use dma_sync_single_for_cpu() after hardware done the execution. In fact the CPU caches are getting flushed or invalidated as appropriate in a result.
>
> dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in the CPU cache, probably to avoid CPU evicting data from cache to DRAM while hardware writes to the buffer. Hence this hunk is correct.
>
> > > + err = tegra_i2c_dma_submit(i2c_dev,
> > > xfer_size);
> > > + if (err < 0) {
> > > + dev_err(i2c_dev->dev,
> > > + "starting RX DMA failed,
> > > err %d\n",
> > > + err);
> > > + goto unlock;
> > > + }
> > > + } else {
> > > + chan = i2c_dev->tx_dma_chan;
> > > + tegra_i2c_config_fifo_trig(i2c_dev,
> > > xfer_size,
> > > +
> > > DATA_DMA_DIR_TX);
> > > + dma_sync_single_for_cpu(i2c_dev->dev,
> > > + i2c_dev->dma_phys,
> > > + xfer_size,
> > > + DMA_TO_DEVICE);
> >
> > This, on the other hand seems correct because we need to invalidate
> > the caches for this buffer to make sure the data that we put there
> > doesn't get overwritten.
>
> As I stated before in a comment to v6, this particular case of
> dma_sync_single_for_cpu() usage is incorrect because CPU should take ownership of the buffer after completion of hardwate job. But in fact
> dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because CPU doesn't need to flush or invalidate anything to take ownership of the buffer if hardware did a read-only access.
>
> >
> > > + if (!i2c_dev->msg_read) {
> > > + if (dma) {
> > > + memcpy(buffer, msg->buf, msg->len);
> > > + dma_sync_single_for_device(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > + xfer_size,
> > > +
> > > DMA_TO_DEVICE);
> >
> > Again, here we properly flush the caches to make sure the data that
> > we've written to the DMA buffer is visible to the DMA engine.
> >
>
> +1 this is correct
>
>
>
> > > +
> > > + if (i2c_dev->msg_read) {
> > > + if (likely(i2c_dev->msg_err ==
> > > I2C_ERR_NONE)) {
> > > +
> > > dma_sync_single_for_cpu(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > + xfer_size,
> > > +
> > > DMA_FROM_DEVICE);
> >
> > Here we invalidate the caches to make sure we don't get stale data
> > that may be in the caches for data that we're copying out of the DMA
> > buffer. I think that's about all the cache maintenance that we real
> > need.
>
> Correct.
>
> And technically here should be dma_sync_single_for_cpu(DMA_TO_DEVICE)
> for the TX. But again, it's a NO-OP.
Is my below understanding correct? Can you please confirm?
During Transmit to device:
- Before writing msg data into dma buf by CPU, giving DMA ownership to CPU
dma_sync_single_for_cpu with dir DMA_TO_DEVICE
- After writing to dma buf by CPU, giving back the ownership to device to access buffer to send during DMA transmit
dma_sync_single_for_device with dir DMA_TO_DEVICE
During Receiving from Device:
- before submitting RX DMA to give buffer access to DMAengine
dma_sync_single_for_Device(DMA_FROM_DEVICE)
- after DMA RX completion, giving dma ownership to CPU for reading dmabuf data written by DMA from device
dma_sync_single_for_cpu with dir DMA_FROM_DEVICE
