Re: [PATCH 4/4] Add DMA engine driver for Freescale MPC8xxx processors.

[Scott Wood <scottwood@freescale.com>]

Zhang Wei-r63237 wrote:
>>It'd be much simpler to allocate the entire ring at once.  No need for
>>linked lists, DMA pools, etc.  Just a single dma_alloc_coherent.
>>
> 
> 
> I use a flexible ld ring size here. If there is no more free ring, the
> driver can add new ld to the ring.

I don't think that's worth all the complexity that it adds.  Most 
drivers make do just fine with a fixed-size ring.

>>What benefit do we get out of using extended mode?  If the 
>>driver can do
>>everything it needs to with basic, with no performance 
>>penalty, why not
>>always use basic?
>>
> 
> Why there are extended mode in silicon? :) 

I've been wondering that myself. :-)

> Since there are here, we use it.

"Because it's there" really isn't a good reason for adding complexity to 
the code.

>>It'd be nice if we didn't have to stop the DMA in order to insert new
>>descriptors.
> 
> 
> Since there is only happen at no more free ld in the ring. We need to
> break the ld-ring and add new ld to it. 

Or you could just block until a previous descriptor completes.

>>Why not just use the index into the ring as the cookie?
> 
> 
> The cookie will record all transfer by increased number. The ring index
> is less for all transfer.

I'm not sure I follow.  All you need is a way to identify a currently 
used descriptor, right?

>>>+						fsl_chan->id, stat);
>>>+	if (!stat)
>>>+		return IRQ_NONE;
>>>+	busy = stat & (FSL_DMA_SR_CB);
>>>+	stat &= ~(FSL_DMA_SR_CB | FSL_DMA_SR_CH);
>>
>>This masking must happen *before* the IRQ_NONE check.
> 
> Really? FSL_DMA_SR_CH is also a stat of event. 

Does the IRQ handler always clear the bit?  If not, then it doesn't count.

> And I need the FSL_DMA_SR_CB status.

The busy assignment can still happen before the mask...  just move the 
IRQ_NONE check down.

-Scott