[PATCH 6/6] dmaengine: imx-sdma: avoid stopping channel in the middle of a BD transfer

[Shawn Guo]

Hi Sascha,

On Thu, Jan 20, 2011 at 11:27:01AM +0100, Sascha Hauer wrote:
> On Thu, Jan 20, 2011 at 06:43:13PM +0800, Shawn Guo wrote:
> > On Thu, Jan 20, 2011 at 05:50:40AM +0800, Shawn Guo wrote:
> > > When STOP register bit gets set, SDMA hardware will immediately stop
> > > the channel once the current burst other than buffer descriptor
> > > transfer is done.
> > > 
> > > * Change sdma_disable_channel() to only set STOP register bit after
> > >   polling the current BD done, so that the current BD transfer
> > >   corruption could be avoided.
> > > 
> > > * Increment buf_tail in mxc_sdma_handle_channel_normal() as well as
> > >   sdma_handle_channel_loop(), since buf_tail now is used in
> > >   sdma_disable_channel() which could be called in both sg and cyclic
> > >   cases.

As the commit message tells, buf_tail is now handled by non-cyclic too.

> > > 
> > > * Return DMA_SUCCESS other than DMA_ERROR in sdma_disable_channel().
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo at freescale.com>
> > > ---
> > >  drivers/dma/imx-sdma.c |    9 ++++++++-
> > >  1 files changed, 8 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > index fa63ace..ae87287 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -481,6 +481,8 @@ static void mxc_sdma_handle_channel_normal(struct sdma_channel *sdmac)
> > >  	else
> > >  		sdmac->status = DMA_SUCCESS;
> > >  
> > > +	sdmac->buf_tail++;
> > > +

ditto

> > >  	if (sdmac->desc.callback)
> > >  		sdmac->desc.callback(sdmac->desc.callback_param);
> > >  	sdmac->last_completed = sdmac->desc.cookie;
> > > @@ -655,9 +657,14 @@ static void sdma_disable_channel(struct sdma_channel *sdmac)
> > >  {
> > >  	struct sdma_engine *sdma = sdmac->sdma;
> > >  	int channel = sdmac->channel;
> > > +	struct sdma_buffer_descriptor *bd = &sdmac->bd[sdmac->buf_tail];
> > > +
> > > +	while (bd->mode.status & BD_DONE)
> > > +		cpu_relax();
> > >  
> > >  	__raw_writel(1 << channel, sdma->regs + SDMA_H_STATSTOP);
> > > -	sdmac->status = DMA_ERROR;
> > > +
> > > +	sdmac->status = DMA_SUCCESS;
> > >  }
> > 
> > Sorry. The patch lost the change as below.  Will pick it up in v2.
> > 
> > @@ -658,11 +658,15 @@ static void sdma_disable_channel(struct sdma_channel *sdmac)
> >         struct sdma_engine *sdma = sdmac->sdma;
> >         int channel = sdmac->channel;
> >         struct sdma_buffer_descriptor *bd = &sdmac->bd[sdmac->buf_tail];
> > +       u32 stat = __raw_readl(sdma->regs + SDMA_H_STATSTOP);
> > 
> > -       while (bd->mode.status & BD_DONE)
> > -               cpu_relax();
> > +       /* stop the channel if it's running */
> > +       if (stat & (1 << channel)) {
> > +               while (bd->mode.status & BD_DONE)
> > +                       cpu_relax();
> > 
> > -       __raw_writel(1 << channel, sdma->regs + SDMA_H_STATSTOP);
> > +               __raw_writel(1 << channel, sdma->regs + SDMA_H_STATSTOP);
> > +       }
> > 
> >         sdmac->status = DMA_SUCCESS;
> 
> NAK.
> 
> The patches has several problems.
> 
> - buf_tail is only used for cyclic transfers, so in case of non cyclic
>   transfers you poll for an arbitrary descriptor being ready.
> 
See above.

> - Even in cyclic transfers when buf_tail points to the correct
>   descriptor the hardware will immediatly start the next descriptor
>   before you have a chance to set the STATSTOP bit. So you probably
>   will corrupt the next descriptor instead of the current one
>   which makes this patch useless.
> 
You are right that the buf_tail will not stop immediately until next
time it gets looped on.  But in any case, polling BD_DONE will not
corrupt any descriptor.

> - buf_tail is increased in the interrupt handler, so if you
>   happen to disable the channel after the bd is done but before
>   the interrupt handler has increased buf_tail you poll for the
>   wrong bd being ready which again makes this patch useless.
> 
> If in non cyclic transfers we want to disable a channel we have our
> reasons (device error or timeout) and then the data is corrupted anyway,
> so no reason to poll for a descriptor getting done. Even worse, in case
> of an device error the descriptor might not get ready at all and
Hmm, we should polling (BD_DONE | BD_ERROR).

> we will poll for ever in the loop above.
> Cyclic transfers are designed for audio and disabling a channel
> basically means pausing the stream. When the SDMA engine does not
I thought pausing channel is different from disabling channel, or we
do not need DMA_PAUSE and DMA_TERMINATE_ALL for dma_ctrl_cmd.

> support pausing the stream in hardware, we have to live with the
What about on the fly setting bit "L" of the next descriptor that's
not been running?  So that SDMA can stop the channel when it gets
this descriptor done.

> fact that we pick up the stream on the next descriptor resulting
> in some bytes missing in the resulting audio stream.
> 

-- 
Regards,
Shawn