Re: [PATCH] dma: mdc: Correct terminate_all handling

Re: [PATCH] dma: mdc: Correct terminate_all handling

[Vinod Koul]

On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote:
> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
> 
> Use of the CANCEL bit in mdc_terminate_all creates an
> additional 'command done' to appear in the registers (in
> addition to an interrupt).
> 
> In addition, there is a potential race between
> mdc_terminate_all and the irq handler if a transfer
> completes at the same time as the terminate all (presently
> this results in an inappropriate warning).
> 
> To handle these issues, any outstanding 'command done'
> events are cleared during mdc_terminate_all and the irq
> handler takes no action when there are no new 'command done'
> events.

SO what does 'command done' event represent, and what would be the behaviour
of ignoring those

> 
> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com>
> ---
>  drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
> index 9ca5683..aa09590 100644
> --- a/drivers/dma/img-mdc-dma.c
> +++ b/drivers/dma/img-mdc-dma.c
> @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan,
>  	return ret;
>  }
>  
> +static unsigned int mdc_get_new_events(struct mdc_chan *mchan)
> +{
> +	u32 val, processed, done1, done2;
> +	unsigned int ret;
> +
> +	val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
> +	processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) &
> +		MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK;

This should be right justfied

> +	/*
> +	 * CMDS_DONE may have incremented between reading CMDS_PROCESSED
> +	 * and clearing INT_ACTIVE.  Re-read CMDS_PROCESSED to ensure we
> +	 * didn't miss a command completion.
> +	 */
> +	do {
> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
> +		done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;
> +		val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK <<
> +			  MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) |
> +			 MDC_CMDS_PROCESSED_INT_ACTIVE);
> +		val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT;
> +		mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED);
> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
> +		done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;

this is very hard to read, can you give some empty lines on logical blocks

> +	} while (done1 != done2);
> +
> +	if (done1 >= processed)
> +		ret = done1 - processed;
> +	else
> +		ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) -
> +			processed) + done1;
> +
> +	return ret;
> +}
> +
>  static int mdc_terminate_all(struct dma_chan *chan)
>  {
>  	struct mdc_chan *mchan = to_mdc_chan(chan);
> @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan)
>  	mchan->desc = NULL;
>  	vchan_get_all_descriptors(&mchan->vc, &head);
>  
> +	mdc_get_new_events(mchan);
> +

Only getting?

-- 
~Vinod

Re: [PATCH] dma: mdc: Correct terminate_all handling

[Damien Horsley]

On 05/12/15 08:43, Vinod Koul wrote:
> On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote:
>> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
>>
>> Use of the CANCEL bit in mdc_terminate_all creates an
>> additional 'command done' to appear in the registers (in
>> addition to an interrupt).
>>
>> In addition, there is a potential race between
>> mdc_terminate_all and the irq handler if a transfer
>> completes at the same time as the terminate all (presently
>> this results in an inappropriate warning).
>>
>> To handle these issues, any outstanding 'command done'
>> events are cleared during mdc_terminate_all and the irq
>> handler takes no action when there are no new 'command done'
>> events.
> 
> SO what does 'command done' event represent, and what would be the behaviour
> of ignoring those
> 

The 'command done' event occurs whenever a new descriptor is loaded from
memory, and once when the whole transfer is completed. Use of the CANCEL
bit can cause additional 'command done' events to occur, which need to
be cleared.

>>
>> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com>
>> ---
>>  drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
>> index 9ca5683..aa09590 100644
>> --- a/drivers/dma/img-mdc-dma.c
>> +++ b/drivers/dma/img-mdc-dma.c
>> @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan,
>>  	return ret;
>>  }
>>  
>> +static unsigned int mdc_get_new_events(struct mdc_chan *mchan)
>> +{
>> +	u32 val, processed, done1, done2;
>> +	unsigned int ret;
>> +
>> +	val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +	processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) &
>> +		MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK;
> 
> This should be right justfied
> 

Ok

>> +	/*
>> +	 * CMDS_DONE may have incremented between reading CMDS_PROCESSED
>> +	 * and clearing INT_ACTIVE.  Re-read CMDS_PROCESSED to ensure we
>> +	 * didn't miss a command completion.
>> +	 */
>> +	do {
>> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +		done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
>> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;
>> +		val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK <<
>> +			  MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) |
>> +			 MDC_CMDS_PROCESSED_INT_ACTIVE);
>> +		val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT;
>> +		mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED);
>> +		val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED);
>> +		done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) &
>> +			MDC_CMDS_PROCESSED_CMDS_DONE_MASK;
> 
> this is very hard to read, can you give some empty lines on logical blocks
>

Ok

>> +	} while (done1 != done2);
>> +
>> +	if (done1 >= processed)
>> +		ret = done1 - processed;
>> +	else
>> +		ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) -
>> +			processed) + done1;
>> +
>> +	return ret;
>> +}
>> +
>>  static int mdc_terminate_all(struct dma_chan *chan)
>>  {
>>  	struct mdc_chan *mchan = to_mdc_chan(chan);
>> @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan)
>>  	mchan->desc = NULL;
>>  	vchan_get_all_descriptors(&mchan->vc, &head);
>>  
>> +	mdc_get_new_events(mchan);
>> +
> 
> Only getting?
> 

Yes, the extra 'command done' events need to be ignored.

Re: [PATCH] dma: mdc: Correct terminate_all handling

[Vinod Koul]

On Mon, Dec 07, 2015 at 04:59:41PM +0000, Damien Horsley wrote:
> On 05/12/15 08:43, Vinod Koul wrote:
> > On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote:
> >> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
> >>
> >> Use of the CANCEL bit in mdc_terminate_all creates an
> >> additional 'command done' to appear in the registers (in
> >> addition to an interrupt).
> >>
> >> In addition, there is a potential race between
> >> mdc_terminate_all and the irq handler if a transfer
> >> completes at the same time as the terminate all (presently
> >> this results in an inappropriate warning).
> >>
> >> To handle these issues, any outstanding 'command done'
> >> events are cleared during mdc_terminate_all and the irq
> >> handler takes no action when there are no new 'command done'
> >> events.
> > 
> > SO what does 'command done' event represent, and what would be the behaviour
> > of ignoring those
> > 
> 
> The 'command done' event occurs whenever a new descriptor is loaded from
> memory, and once when the whole transfer is completed. Use of the CANCEL
> bit can cause additional 'command done' events to occur, which need to
> be cleared.

Okay, btw please fix the subsystem name. I'ts dmaengine

-- 
~Vinod