> > + dma_sync_single_for_device(i2c_dev->dev,
> > + i2c_dev->dma_phys,
> > + xfer_size,
> > + DMA_FROM_DEVICE);
> > + ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> > + if (ret < 0) {
> > + dev_err(i2c_dev->dev,
> > + "Starting RX DMA failed, err %d\n",
> > + ret);
> > + goto exit;
> > + }
> > + } else {
> > + chan = i2c_dev->tx_dma_chan;
> > + tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> > + DATA_DMA_DIR_TX);
> > + /* Make the dma buffer to read by cpu */> +
> > dma_sync_single_for_cpu(i2c_dev->dev,
> > + i2c_dev->dma_phys,
> > + xfer_size,
> > + DMA_TO_DEVICE);
>
> This is not correct, you need to call dma_sync_single_for_cpu() after
> completion of the transfer to give back dma buffer ownership to CPU, see
> below. This dma_sync_single_for_cpu() invocation should be removed.
Transfer is not done yet. Dma buffer ownership is given to CPU here before
copying header bytes and msg bytes into dma buffer before actually transmitting
> > +
> > + 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);
> > +
> > + memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
> > + msg->len);
> > + }
> > + }
>
> Here you should give back dma buffer ownership to CPU:
After transfer is done, for msg reads, dma buffer ownership is given to CPU and
read data from dma buffer is copied to msg buffer
For msg writes, no need of ownership to CPU.
> >
> > else {
> > dma_sync_single_for_cpu(i2c_dev->dev,
> > i2c_dev->dma_phys,
> > xfer_size,
> > DMA_TO_DEVICE);
> > }
>
> > time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> > TEGRA_I2C_TIMEOUT);
> > tegra_i2c_mask_irq(i2c_dev, int_mask);
> >
> > if (time_left == 0) {
> > dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> > + if (dma) {
> > + dmaengine_terminate_all(chan);
> > + complete(&i2c_dev->dma_complete);
> > + }
>
> DMA transfer has been completed at this point, hence this hunk isn't needed.
> Please remove it.
DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer
interrupt from I2C confirms complete transaction along with dma complete check.
So still need to check for msg_complete timeout.
> > @@ -740,6 +925,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> > *i2c_dev,
> > u32 int_mask;
> > unsigned long time_left;
> > unsigned long flags;
> > + size_t xfer_size;
> > + u32 *buffer = 0;
> > + int ret = 0;
> > + bool dma = false;
> > +
> > + if (msg->flags & I2C_M_RD)
> > + xfer_size = msg->len;
> > + else
> > + xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> > +
> > + xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> > + dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> > + if (dma) {
> > + if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> > + ret = tegra_i2c_init_dma_param(i2c_dev, true);
> > + else if (!i2c_dev->tx_dma_chan)
> > + ret = tegra_i2c_init_dma_param(i2c_dev, false);
>
> In the comment to V3 I mentioned that it's not a good idea to request
> channels dynamically because suspend-resume order is based on devices
> registration order, in this case APB DMA must be probed before I2C. Please
> move channels allocation into the probe.
>
> This also raises the question about the need to register I2C driver from the
> subsys-init level because APB driver is getting registered from the
> module-init level and hence I2C probing will be deferred until APB DMA driver
> is registered. It looks to me that the subsys-init is a relict of the past
> and it should be fine to move I2C driver registration into the module-init
> level, of course it's not strictly necessary and could be done later on if
> desired.
>
> > + if (ret < 0) {
> > + dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> > + dma = false;
> > + ret = 0;
> > + }
> > + }
> > +
> > + i2c_dev->is_curr_dma_xfer = dma;
>
>
Since your previous feedback suggest "let's postpone channels requesting and
dma_buf allocation until they are really needed", I thought it make sense to
not request channels and allocate till DMA is needed.
So moved from probe to xfer_msg function. By the time it gets to xfer msg
function, devices registration should be done already along with apb dma probe.
