RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support

[Sowjanya Komatineni]

> > > +	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