Re: [PATCH V4 2/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 05/04/16 22:36, Vinod Koul wrote:
> On Tue, Mar 15, 2016 at 03:56:29PM +0000, Jon Hunter wrote:
> 
>> +static void tegra_adma_request_free(struct tegra_adma_chan *tdc)
>> +{
>> +	struct tegra_adma *tdma = tdc->tdma;
>> +
>> +	if (!tdc->sreq_reserved)
>> +		return;
>> +
>> +	switch (tdc->sreq_dir) {
>> +	case DMA_MEM_TO_DEV:
>> +		clear_bit(tdc->sreq_index, &tdma->tx_requests_reserved);
>> +		break;
> 
> empty line here woould be nicer

OK.

>> +	ret = dma_cookie_status(dc, cookie, txstate);
>> +	if (ret == DMA_COMPLETE || !txstate)
>> +		return ret;
>> +
>> +	spin_lock_irqsave(&tdc->vc.lock, flags);
>> +
>> +	vd = vchan_find_desc(&tdc->vc, cookie);
>> +	if (vd) {
>> +		desc = to_tegra_adma_desc(&vd->tx);
>> +		residual = desc->ch_regs.tc;
> 
> Here we are filling up residue for desc found in issued list
> 
>> +	} else if (tdc->desc && tdc->desc->vd.tx.cookie == cookie) {
>> +		residual = tegra_adma_get_residue(tdc);
> 
> Well if it is not issued then why we we need to caluclate, its full size of
> descriptor

This is the current/active descriptor which has been removed from the
issued list and so it needs to be calculated.

>> +	} else {
>> +		residual = 0;
> 
> why this?

So either the descriptor is still in the submitted list or it has
completed but it has not been marked complete yet. In both cases, we
don't have access to the descriptor and so the residue is marked as 0
and we return the current status. Please note this is based upon the
omap dma driver (drivers/dma/omap-dma.c) which does the same. Other dma
driver do very similar things here as well.

>> +static struct dma_async_tx_descriptor *tegra_adma_prep_slave_sg(
>> +	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>> +	enum dma_transfer_direction direction, unsigned long flags,
>> +	void *context)
>> +{
>> +	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
>> +
>> +	dev_warn(tdc2dev(tdc), "scatter-gather transfers are not supported\n");
>> +
>> +	return NULL;
>> +}
> 
> Why do we need this placeholder, If you dont support slave_sg dont add this
> as capability

So, AFAICT, dma_async_device_register() does not check to see if
device_prep_slave_sg() is valid AND none of the functions
dmaengine_prep_slave_single(), dmaengine_prep_slave_single() and
dmaengine_prep_rio_sg() check to see if the function pointer is valid
before calling chan->device->device_prep_slave_sg(). So it seems that we
always expect this function pointer to be valid.

So should the inline functions ensure the function pointer is valid
before attempting to call them? If so I can add a patch for this.
Otherwise it seems the driver needs a stub. It would be a massive change
to add a new capability, say SLAVE_SG, and populate this for all
existing drivers.

>> +static const struct tegra_adma_chip_data tegra210_chip_data = {
>> +	.nr_channels = 22,
>> +};
> 
> why should this be hard coded in kernel and not queried from something like
> DT? This case seems to be hardware property

Originally, I did have this in DT, however, the Tegra maintainers prefer
this and this is consistent with the other Tegra DMA driver (see
driver/dma/tegra20-apb-dma.c) [0].

>> +	dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
>> +	dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
>> +	dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
> 
> I think you should not set DMA_SLAVE, do you need caps to be exported. I
> think that should be exported for cyclic too, let me know if that was the
> issue?
> 

Why should I not be setting DMA_SLAVE? Should I not be calling
dma_get_any_slave_channel() in the xlate?

I think that I do want to set DMA_CYCLIC as well to ensure that we check
that the device->device_prep_dma_cyclic() function pointer is populated
when registering the DMA controller.

Cheers
Jon

[0] http://marc.info/?l=linux-tegra&m=144423393825053&w=2