Re: [PATCH] dmaengine: ti: k3-udma: Fix teardown timeout for cyclic mode

[Péter Ujfalusi]

On 21/08/2023 13:40, Vignesh Raghavendra wrote:
> In cyclic mode, last descriptor needs to have EOP flag set so that
> teardown flushes data towards PDMA in case of MEM_TO_DMA.  Else,

MEM_TO_DEV ?

> operation will not complete successfully leading to spurious timeout on
> channel terminate.
> 
> Without this terminating aplay cmd outputs false error msg like:
> [116.402800] ti-bcdma 485c0100.dma-controller: chan1 teardown timeout!
> 
> This doesn't seem to be problem with UDMA-P on J7xx devices (although is
> a requirement as per spec) but shows up easily on BCDMA + McASP. Fix
> this by setting the appropriate flag

If I recall the teardown flow was in case of TR mode (does not matter if
cyclic or not) is when the TD bit is set the currently executed TR is
finished at the next iteration boundary.
The EOP and tdown (and SOP) is sent to the destination in a zero length
data package as part of the teardown sequence.

In cyclic TR mode EOP is a strange concept as it is a stream w/o
packetization..

> 
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Suggested-by: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxx>
> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> ---
> 
> This complete reimplementation based on learning of HW behavior for problems
> reported at
> https://lore.kernel.org/linux-arm-kernel/20220215044112.161634-1-vigneshr@xxxxxx/
> 
>  drivers/dma/ti/k3-udma.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 30fd2f386f36..02aac7be8d28 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -3476,6 +3476,10 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
>  	u16 tr0_cnt0, tr0_cnt1, tr1_cnt0;
>  	unsigned int i;
>  	int num_tr;
> +	u32 period_csf = 0;
> +
> +	if (uc->config.ep_type == PSIL_EP_PDMA_XY && dir == DMA_MEM_TO_DEV)
> +		period_csf = CPPI5_TR_CSF_EOP;
>  
>  	num_tr = udma_get_tr_counters(period_len, __ffs(buf_addr), &tr0_cnt0,
>  				      &tr0_cnt1, &tr1_cnt0);
> @@ -3525,8 +3529,10 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
>  		}
>  
>  		if (!(flags & DMA_PREP_INTERRUPT))
> -			cppi5_tr_csf_set(&tr_req[tr_idx].flags,
> -					 CPPI5_TR_CSF_SUPR_EVT);
> +			period_csf |= CPPI5_TR_CSF_SUPR_EVT;
> +
> +		if (period_csf)
> +			cppi5_tr_csf_set(&tr_req[tr_idx].flags, period_csf);

This is not what the commit message was saying...
You set EOP (End Of Packet) for each period, if the period size is <
SZ_64K then you set the EOP for each descriptor, otherwise to every
second one.

>  
>  		period_addr += period_len;
>  	}

-- 
Péter