В Fri, 1 Feb 2019 01:11:06 +0000
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> пишет:
> > > > + 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
>
I tried to take a look at it again and now thinking that your variant
is more correct. Still it's a bit difficult to judge because this case
is no-op.
> - 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
Correct.
> During Receiving from Device:
> - before submitting RX DMA to give buffer access to DMAengine
> dma_sync_single_for_Device(DMA_FROM_DEVICE)
Correct.
> - 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
>
Correct.
