RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, April 18, 2012 9:41 AM, Rafal Prylowski wrote:
> On 2012-04-17 22:51, H Hartley Sweeten wrote:
>> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>>
>>> Rafal,
>>>
>>> Here's the output:
>>>
>>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>>> M2M: 20c3
>>> M2M: 20c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>>
>>> The "M2M: 21c3" keeps getting output until the system is turned off.
>> 
>> Rafal,
>> 
>> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
>> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
>> 
>> I added a prink in msm_hw_submit():
>> 
>> 	if (ep93xx_dma_advance_active(edmac)) {
>> 		m2m_fill_desc(edmac);
>> 		control |= M2M_CONTROL_NFBINT;
>> 		printk("%s: NFB enabled\n", __func__);
>> 	}
>> 
>> This message is never displayed.
>> 
>> And for some reason the txd.cookie is not getting set correctly to detect
>> the last entry in m2m_hw_interrupt.
>> 
>> 	/*
>> 	 * Check whether we are done with descriptors or not. This, together
>> 	 * with DMA channel state, determines action to take in interrupt.
>> 	 */
>> 	last = list_first_entry(edmac->active.next,
>> 		struct ep93xx_dma_desc, node)->txd.cookie;
>> 
>> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
>> even though a "next" buffer does not exist.
>> 
>
> Well, I thought this is correct even if we have only one descriptor in active
> queue (when NFBINT bit is not set). edmac->active.next should point to the same
> element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
> this.
>
> Following is a double buffering patch, which should solve your issues with interrupt
> storm. It's similar to your hack, but hopefully more optimal, and (the most
> important thing for me) it never tries to disable the channel without ensuring
> that it is in proper state. If it happens that we are done with descriptors
> but Buffer and Control FSM informs that the channel is still operating, we just
> wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
> always issues next interrupt, and sets DONE bit again).
> Patch applies to current mainline.
>
>
> Implement double buffering for M2M DMA channels.
>
>  Signed-off-by: Rafal Prylowski <prylowski@xxxxxxxxxxx>
>
> Index: linux-2.6/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/dma/ep93xx_dma.c
> +++ linux-2.6/drivers/dma/ep93xx_dma.c
> @@ -71,6 +71,7 @@
>  #define M2M_CONTROL_TM_SHIFT		13
>  #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
>  #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
> +#define M2M_CONTROL_NFBINT		BIT(21)
>  #define M2M_CONTROL_RSS_SHIFT		22
>  #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
>  #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
> @@ -79,7 +80,23 @@
>  #define M2M_CONTROL_PWSC_SHIFT		25
>  
>  #define M2M_INTERRUPT			0x0004
> -#define M2M_INTERRUPT_DONEINT		BIT(1)
> +#define M2M_INTERRUPT_MASK		6
> +
> +#define M2M_STATUS			0x000c
> +#define M2M_STATUS_CTL_SHIFT		1
> +#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_BUF_SHIFT		4
> +#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_DONE			BIT(6)
> +

Nitpick.. Unnecessary whitespace.

>  
>  #define M2M_BCR0			0x0010
>  #define M2M_BCR1			0x0014
> @@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
>  
>  /*
>   * M2M DMA implementation
> - *
> - * For the M2M transfers we don't use NFB at all. This is because it simply
> - * doesn't work well with memcpy transfers. When you submit both buffers it is
> - * extremely unlikely that you get an NFB interrupt, but it instead reports
> - * DONE interrupt and both buffers are already transferred which means that we
> - * weren't able to update the next buffer.
> - *
> - * So for now we "simulate" NFB by just submitting buffer after buffer
> - * without double buffering.
>   */
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
> @@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
>  	m2m_fill_desc(edmac);
>  	control |= M2M_CONTROL_DONEINT;
>  
> +	if (ep93xx_dma_advance_active(edmac)) {
> +		m2m_fill_desc(edmac);
> +		control |= M2M_CONTROL_NFBINT;
> +	}
> +
>  	/*
>  	 * Now we can finally enable the channel. For M2M channel this must be
>  	 * done _after_ the BCRx registers are programmed.
> @@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
>  	}
>  }
>  
> +/*
> + * According to EP93xx User's Guide, we should receive DONE interrupt when all
> + * M2M DMA controller transactions complete normally. This is not always the
> + * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
> + * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
> + * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
> + * In effect, disabling the channel when only DONE bit is set could stop
> + * currently running DMA transfer. To avoid this, we use Buffer FSM and
> + * Control FSM to check current state of DMA channel.
> + */
>  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  {
> +	u32 status = readl(edmac->regs + M2M_STATUS);
> +	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
> +	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
> +	bool done = status & M2M_STATUS_DONE;
> +	bool last_done;
>  	u32 control;
> +	struct ep93xx_dma_desc *desc;
>  
> -	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
> +	/* Accept only DONE and NFB interrupts */
> +	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
>  		return INTERRUPT_UNKNOWN;
>  
> -	/* Clear the DONE bit */
> -	writel(0, edmac->regs + M2M_INTERRUPT);
> +	if (done)
> +		/* Clear the DONE bit */
> +		writel(0, edmac->regs + M2M_INTERRUPT);

Another nitpick. Either move the comment above the if (done) line or enclose
the two lines in { }.

>  
> -	/* Disable interrupts and the channel */
> -	control = readl(edmac->regs + M2M_CONTROL);
> -	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
> -	writel(control, edmac->regs + M2M_CONTROL);
> +	/*
> +	 * Check whether we are done with descriptors or not. This, together
> +	 * with DMA channel state, determines action to take in interrupt.
> +	 */
> +	desc = ep93xx_dma_get_active(edmac);
> +	last_done = !desc || desc->txd.cookie;
>  
>  	/*
> -	 * Since we only get DONE interrupt we have to find out ourselves
> -	 * whether there still is something to process. So we try to advance
> -	 * the chain an see whether it succeeds.
> +	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
> +	 * DMA channel. Using DONE and NFB bits from channel status register
> +	 * or bits from channel interrupt register is not reliable.
>  	 */
> -	if (ep93xx_dma_advance_active(edmac)) {
> -		edmac->edma->hw_submit(edmac);
> -		return INTERRUPT_NEXT_BUFFER;
> +	if (!last_done &&
> +	    (buf_fsm == M2M_STATUS_BUF_NO ||
> +	     buf_fsm == M2M_STATUS_BUF_ON)) {
> +		/*
> +		 * Two buffers are ready for update when Buffer FSM is in
> +		 * DMA_NO_BUF state. Only one buffer can be prepared without
> +		 * disabling the channel or polling the DONE bit.
> +		 * To simplify things, always prepare only one buffer.
> +		 */
> +		if (ep93xx_dma_advance_active(edmac)) {
> +			m2m_fill_desc(edmac);
> +			if (done && !edmac->chan.private) {
> +				/* Software trigger for memcpy channel */
> +				control = readl(edmac->regs + M2M_CONTROL);
> +				control |= M2M_CONTROL_START;
> +				writel(control, edmac->regs + M2M_CONTROL);
> +			}
> +			return INTERRUPT_NEXT_BUFFER;
> +		} else
> +			last_done = true;

One more nitpick. The else branch should also be enclosed with { }. See CodingStyle.

> +	}
> +
> +	/*
> +	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
> +	 * and Control FSM is in DMA_STALL state.
> +	 */
> +	if (last_done &&
> +	    buf_fsm == M2M_STATUS_BUF_NO &&
> +	    ctl_fsm == M2M_STATUS_CTL_STALL) {
> +		/* Disable interrupts and the channel */
> +		control = readl(edmac->regs + M2M_CONTROL);
> +		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
> +			    | M2M_CONTROL_ENABLE);
> +		writel(control, edmac->regs + M2M_CONTROL);
> +		return INTERRUPT_DONE;
>  	}
>  
> -	return INTERRUPT_DONE;
> +	/*
> +	 * Nothing to do this time.
> +	 */
> +	return INTERRUPT_NEXT_BUFFER;
>  }
>  
>  /*

Other than the nitpicks, I'm now happy with this patch. It works fine on
my ep93xx board with the mmc_spi and sst25l drivers.

Tested-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Acked-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>


--
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/