Re: [PATCH 4/4] dmaengine: xilinx: xdma: Support cyclic transfers

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Lizhi,

> > + * xdma_prep_dma_cyclic - prepare for cyclic DMA transactions
> > + * @chan: DMA channel pointer
> > + * @address: Device DMA address to access
> > + * @size: Total length to transfer
> > + * @period_size: Period size to use for each transfer
> > + * @dir: Transfer direction
> > + * @flags: Transfer ack flags
> > + */
> > +static struct dma_async_tx_descriptor *
> > +xdma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t address,
> > +		     size_t size, size_t period_size,
> > +		     enum dma_transfer_direction dir,
> > +		     unsigned long flags)
> > +{
> > +	struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> > +	struct xdma_device *xdev = xdma_chan->xdev_hdl;
> > +	unsigned int periods = size / period_size;  

> What if size is not multiple of period_size?

Can this really happen? I would have expected a bug if size was not a
multiple of period_size, because the headers explicitly tell that we
should expect an interrupt after each period and we should loop over
when we reach the last one. This makes it very impractical to handle
the situation you mention.

> > +	struct dma_async_tx_descriptor *tx_desc;
> > +	u64 addr, dev_addr, *src, *dst;
> > +	struct xdma_desc_block *dblk;
> > +	struct xdma_hw_desc *desc;
> > +	struct xdma_desc *sw_desc;
> > +	unsigned int i;
> > +
> > +	/*
> > +	 * Simplify the whole logic by preventing an abnormally high number of
> > +	 * periods and periods size.
> > +	 */
> > +	if (period_size > XDMA_DESC_BLEN_MAX) {
> > +		xdma_err(xdev, "period size limited to %lu bytes\n", XDMA_DESC_BLEN_MAX);
> > +		return NULL;
> > +	}
> > +
> > +	if (periods > XDMA_DESC_ADJACENT) {
> > +		xdma_err(xdev, "number of periods limited to %u\n", XDMA_DESC_ADJACENT);
> > +		return NULL;
> > +	}
> > +
> > +	sw_desc = xdma_alloc_desc(xdma_chan, periods, true);
> > +	if (!sw_desc)
> > +		return NULL;
> > +
> > +	sw_desc->periods = periods;
> > +	sw_desc->period_size = period_size;
> > +	sw_desc->dir = dir;
> > +
> > +	if (dir == DMA_MEM_TO_DEV) {
> > +		dev_addr = xdma_chan->cfg.dst_addr;
> > +		src = &addr;
> > +		dst = &dev_addr;
> > +	} else {
> > +		dev_addr = xdma_chan->cfg.src_addr;
> > +		src = &dev_addr;
> > +		dst = &addr;
> > +	}
> > +
> > +	dblk = sw_desc->desc_blocks;
> > +	desc = dblk->virt_addr;
> > +	for (i = 0; i < periods; i++) {
> > +		addr = address;
> > +
> > +		/* fill hardware descriptor */
> > +		desc->bytes = cpu_to_le32(period_size);
> > +		desc->src_addr = cpu_to_le64(*src);
> > +		desc->dst_addr = cpu_to_le64(*dst);
> > +
> > +		desc++;
> > +		address += period_size;
> > +	}
> > +
> > +	tx_desc = vchan_tx_prep(&xdma_chan->vchan, &sw_desc->vdesc, flags);
> > +	if (!tx_desc)
> > +		goto failed;
> > +
> > +	return tx_desc;
> > +
> > +failed:
> > +	xdma_free_desc(&sw_desc->vdesc);
> > +
> > +	return NULL;
> > +}
> > +
> >   /**
> >    * xdma_device_config - Configure the DMA channel
> >    * @chan: DMA channel
> > @@ -583,7 +698,36 @@ static int xdma_alloc_chan_resources(struct dma_chan *chan)
> >   static enum dma_status xdma_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> >   				      struct dma_tx_state *state)
> >   {
> > -	return dma_cookie_status(chan, cookie, state);
> > +	struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> > +	struct virt_dma_desc *vd;
> > +	struct xdma_desc *desc;
> > +	enum dma_status ret;
> > +	unsigned long flags;
> > +	unsigned int period_idx;
> > +	u32 residue = 0;
> > +
> > +	ret = dma_cookie_status(chan, cookie, state);
> > +	if (ret == DMA_COMPLETE || !state)  
>
> probably do not need to check state. Or at least check before calling dma_cookie_status.

Good idea.

> > +		return ret;
> > +
> > +	spin_lock_irqsave(&xdma_chan->vchan.lock, flags);
> > +
> > +	vd = vchan_find_desc(&xdma_chan->vchan, cookie);
> > +	if (vd)
> > +		desc = to_xdma_desc(vd);
> > +	if (!desc || !desc->cyclic) {  
>
> desc is un-initialized if vd is NULL.

Yes, handled in v2.

> > +		spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags);
> > +		return ret;
> > +	}
> > +
> > +	period_idx = desc->completed_desc_num % desc->periods;
> > +	residue = (desc->periods - period_idx) * desc->period_size;
> > +
> > +	spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags);
> > +
> > +	dma_set_residue(state, residue);
> > +
> > +	return ret;
> >   }  
> >   >   /**  
> > @@ -599,6 +743,7 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
> >   	struct virt_dma_desc *vd;
> >   	struct xdma_desc *desc;
> >   	int ret;
> > +	u32 st;  
> >   >   	spin_lock(&xchan->vchan.lock);
> >   > @@ -617,6 +762,19 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)  
> >   		goto out;  
> >   >   	desc->completed_desc_num += complete_desc_num;  
> > +
> > +	if (desc->cyclic) {
> > +		ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS,
> > +				  &st);
> > +		if (ret)
> > +			goto out;
> > +
> > +		regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_STATUS, st);  
> 
> What does reading/writing channel status register do here?

It clears the status register to allow the next interrupt to trigger.

Thanks,
Miquèl