Re: [PATCHv10 2/2] dma: Add Freescale eDMA engine driver support

[Vinod Koul]

On Mon, Jan 20, 2014 at 11:05:03AM +0000, Jingchang Lu wrote:
> > > > > +	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > > > > +	struct dma_slave_config *cfg = (void *)arg;
> > > > > +	unsigned long flags;
> > > > > +	LIST_HEAD(head);
> > > > > +
> > > > > +	switch (cmd) {
> > > > > +	case DMA_TERMINATE_ALL:
> > > > > +		spin_lock_irqsave(&fsl_chan->vchan.lock, flags);
> > > > > +		fsl_edma_disable_request(fsl_chan);
> > > > > +		fsl_chan->edesc = NULL;
> > > > > +		vchan_get_all_descriptors(&fsl_chan->vchan, &head);
> > > > > +		spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
> > > > > +		vchan_dma_desc_free_list(&fsl_chan->vchan, &head);
> > > > > +		return 0;
> > > > well what happens to the current ongoing transactions, i don't see
> > those
> > > > getting
> > > > terminated?
> > > The fsl_edma_disable_request(fsl_chan) would end the channel's ongoing
> > transaction, then
> > > the eDMA would not response to device dma request, and the
> > vchan_dma_desc_free_list()
> > > will release all associate memory. Thanks.
> > Can you explain a bit more how terminate will happen, given taht you are
> > using
> > same thing for pause?
> It works just like an interrupt controller on irq enable and disable. It has a register called
> set/clear enable request register(SERQ/CERQ) to enable or disable the DMA request for a given
> channel. It won't transfer data any more with the enable request register cleared. So for the
> pause and terminate the pause is the same. And there is no other way to stop the channels.
well then it is not pause! If you jave no way to stop the channel, you cant
claim to support pause and resume!

Also, for terminate this will be problematic. Assuming you are doing transfers
and terminate is invoked. Then you will disable irq. The transfers may get stuck
and periphral clock may go away after transfer so current transaction never gets
completed, how do we recovery from this?


Alstly, have you looked at edma driver already existing, any similarties in teh
controller with that?


> > > > > +static struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
> > > > > +		struct dma_chan *chan, dma_addr_t dma_addr, size_t
> > buf_len,
> > > > > +		size_t period_len, enum dma_transfer_direction
> > direction,
> > > > > +		unsigned long flags, void *context)
> > > > > +{
> > > > you may want to implement the capablities api subsequently for audio
> > > > usage.
> > > Do you mean the device_slave_caps function? If it is, I will add it.
> > Yes, that can be incrementally added..
> I have send the v11 patch out with a basic device_slave_caps definition. I notice
> that there is some new update of the dma capabilities, so if possible and others ok,
> could you please merge this patch first and leave me improving the driver in subsequent
> patches. Thanks.
Sure, as I said this can be incremental but atm am worried about pause and
terminate behaviour.

--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/