Re: [RESEND,2/5] dmaengine: Add ADM driver

[Andy Gross <andy.gross@linaro.org>]

On Fri, Dec 23, 2016 at 08:53:09PM +0100, Neil Armstrong wrote:
> 
> 
> Le 22/12/2016 18:55, Zoran Markovic a écrit :
> >> Add the DMA engine driver for the QCOM Application Data Mover (ADM) DMA
> >> controller found in the MSM8x60 and IPQ/APQ8064 platforms.
> > 
> > With minor changes I got this working on MDM9615, using qcom_nand. Below are the changes I had to make, please consider them. Patches for MDM9615 NAND are pending.
> > 
> >> +static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
> >> +	struct scatterlist *sgl, unsigned int sg_len,
> >> +	enum dma_transfer_direction direction, unsigned long flags,
> >> +	void *context)
> >> +{
> > ...
> > 
> >> +	/* if using flow control, validate burst and crci values */
> >> +	if (achan->slave.device_fc) {
> >> +
> >> +		blk_size = adm_get_blksize(burst);
> >> +		if (blk_size < 0) {
> >> +			dev_err(adev->dev, "invalid burst value: %d\n",
> >> +				burst);
> >> +			return ERR_PTR(-EINVAL);
> > Return NULL here, most DMA clients (including qcom_nand) expect NULL if prep_slave_sg() fails.
> >> +		}
> >> +
> >> +		crci = achan->slave.slave_id & 0xf;
> >> +		if (!crci || achan->slave.slave_id > 0x1f) {
> >> +			dev_err(adev->dev, "invalid crci value\n");
> >> +			return ERR_PTR(-EINVAL);
> > Ditto above.
> >> +		}
> >> +	}
> >> +
> >> +	/* iterate through sgs and compute allocation size of structures */
> >> +	for_each_sg(sgl, sg, sg_len, i) {
> >> +		if (achan->slave.device_fc) {
> >> +			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
> >> +						  ADM_MAX_ROWS);
> >> +			if (sg_dma_len(sg) % burst)
> >> +				single_count++;
> >> +		} else {
> >> +			single_count += DIV_ROUND_UP(sg_dma_len(sg),
> >> +						     ADM_MAX_XFER);
> >> +		}
> >> +	}
> >> +
> >> +	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
> >> +	if (!async_desc)
> >> +		return ERR_PTR(-ENOMEM);
> > Ditto above.
> > 
> >> +
> >> +	if (crci)
> >> +		async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
> >> +					ADM_CRCI_CTL_MUX_SEL : 0;
> >> +	async_desc->crci = crci;
> >> +	async_desc->blk_size = blk_size;
> >> +	async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
> >> +				box_count * sizeof(struct adm_desc_hw_box) +
> >> +				sizeof(*cple) + 2 * ADM_DESC_ALIGN;
> >> +
> >> +	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
> >> +				&async_desc->dma_addr, GFP_NOWAIT);
> > Under pressure this allocation might fail, resulting in NAND errors. I handled it using wait_event_timeout() to wait until buffers are available. Either that, or clients such as qcom_nand need to handle this failure.

prep_slave_sg calls can be made from atomic context.  Which is why we do the
GFP_NOWAIT.  You can't sleep in that function.  Instead, the client needs to see
that it got ENOMEM and deal with the memory pressure itself.  Either that or we
need to relax the dma_alloc to a dma_alloc_coherent (I suspect you'll still have
the same problem).  Or you need to increase the static dma pool.

I've seen this issue with people using ADM w/ SPI.  They had other dma pool
pressure and the ADM transfers would fail to get buffers.

> > 
> >> +
> >> +	if (!async_desc->cpl) {
> >> +		kfree(async_desc);
> >> +		return ERR_PTR(-ENOMEM);
> > Return NULL.
> >> +	}
> > ...
> >> +}
> > ...
> > 
> >> +static void adm_dma_free_desc(struct virt_dma_desc *vd) {
> >> +	struct adm_async_desc *async_desc = container_of(vd,
> >> +			struct adm_async_desc, vd);
> >> +
> >> +	dma_free_writecombine(async_desc->adev->dev, async_desc->dma_len,
> >> +		async_desc->cpl, async_desc->dma_addr);
> >> +	kfree(async_desc);
> > Do wake_up() here to signal buffer availability.
> >> +}
> > 
> > Regards, Zoran
> > 
> 
> Hi Zoran,
> 
> These are also the fixes we needed to make the ADM work on MDM9615.
> 
> Andy, do you plan to resend this driver ?

The main outstanding comments I had were specific to the crci encoding and the
use of a virtual channel to encapsulate the crci information instead of using
the slave_config.  I don't see that being done.

I haven't had time to address that and I was hoping that Thomas would do it.
But it seems like he moved on to other things, so I need to put this on my list.


Andy