1. Patchew
  2. linux
  3. View series

[PATCH] dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA transfers

Jai Luthra posted 1 patch 2 months, 1 week ago 
drivers/dma/ti/k3-udma.c | 61 ++++++++++++++++++++++++++++++++++++------------
1 file changed, 46 insertions(+), 15 deletions(-)
[PATCH] dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA transfers
Posted by Jai Luthra 2 months, 1 week ago 
From: Jai Luthra <j-luthra@ti.com>

When receiving data in cyclic mode from PDMA peripherals, where reload
count is set to infinite, any TR in the set can potentially be the last
one of the overall transfer. In such cases, the EOP flag needs to be set
in each TR and PDMA's Static TR "Z" parameter should be set, matching
the size of the TR.

This is required for the teardown to function properly and cleanup the
internal state memory. This only affects platforms using BCDMA and not
those using UDMA-P, which could set EOP flag in the teardown TR
automatically.

Similarly when transmitting data in cyclic mode to PDMA peripherals, the
EOP flag needs to be set to get the teardown completion signal
correctly.

Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
Signed-off-by: Jai Luthra <j-luthra@ti.com>
Signed-off-by: Jai Luthra <jai.luthra@linux.dev>
---
 drivers/dma/ti/k3-udma.c | 61 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 406ee199c2ac..5a900b63dae5 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -3185,27 +3185,39 @@ static int udma_configure_statictr(struct udma_chan *uc, struct udma_desc *d,
 
 	d->static_tr.elcnt = elcnt;
 
-	/*
-	 * PDMA must to close the packet when the channel is in packet mode.
-	 * For TR mode when the channel is not cyclic we also need PDMA to close
-	 * the packet otherwise the transfer will stall because PDMA holds on
-	 * the data it has received from the peripheral.
-	 */
 	if (uc->config.pkt_mode || !uc->cyclic) {
+		/*
+		 * PDMA must close the packet when the channel is in packet mode.
+		 * For TR mode when the channel is not cyclic we also need PDMA
+		 * to close the packet otherwise the transfer will stall because
+		 * PDMA holds on the data it has received from the peripheral.
+		 */
 		unsigned int div = dev_width * elcnt;
 
 		if (uc->cyclic)
 			d->static_tr.bstcnt = d->residue / d->sglen / div;
 		else
 			d->static_tr.bstcnt = d->residue / div;
+	} else if (uc->ud->match_data->type == DMA_TYPE_BCDMA &&
+		   uc->config.dir == DMA_DEV_TO_MEM && !uc->config.pkt_mode &&
+		   uc->cyclic) {
+		/*
+		 * For cyclic TR mode PDMA must close the packet after every TR
+		 * transfer, as we have to set EOP in each TR to prevent short
+		 * packet errors seen on channel teardown.
+		 */
+		struct cppi5_tr_type1_t *tr_req = d->hwdesc[0].tr_req_base;
 
-		if (uc->config.dir == DMA_DEV_TO_MEM &&
-		    d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
-			return -EINVAL;
+		d->static_tr.bstcnt =
+			(tr_req->icnt0 * tr_req->icnt1) / dev_width;
 	} else {
 		d->static_tr.bstcnt = 0;
 	}
 
+	if (uc->config.dir == DMA_DEV_TO_MEM &&
+	    d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -3450,8 +3462,9 @@ udma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	/* static TR for remote PDMA */
 	if (udma_configure_statictr(uc, d, dev_width, burst)) {
 		dev_err(uc->ud->dev,
-			"%s: StaticTR Z is limited to maximum 4095 (%u)\n",
-			__func__, d->static_tr.bstcnt);
+			"%s: StaticTR Z is limited to maximum %u (%u)\n",
+			__func__, uc->ud->match_data->statictr_z_mask,
+			d->static_tr.bstcnt);
 
 		udma_free_hwdesc(uc, d);
 		kfree(d);
@@ -3476,6 +3489,7 @@ 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;
 
 	num_tr = udma_get_tr_counters(period_len, __ffs(buf_addr), &tr0_cnt0,
 				      &tr0_cnt1, &tr1_cnt0);
@@ -3498,6 +3512,20 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
 		period_addr = buf_addr |
 			((u64)uc->config.asel << K3_ADDRESS_ASEL_SHIFT);
 
+	/*
+	 * For BCDMA <-> PDMA transfers, the EOP flag needs to be set on the
+	 * last TR of a descriptor, to mark the packet as complete.
+	 * This is required for getting the teardown completion message in case
+	 * of TX, and to avoid short-packet error in case of RX.
+	 *
+	 * As we are in cyclic mode, we do not know which period might be the
+	 * last one, so set the flag for each period.
+	 */
+	if (uc->config.ep_type == PSIL_EP_PDMA_XY &&
+	    uc->ud->match_data->type == DMA_TYPE_BCDMA) {
+		period_csf = CPPI5_TR_CSF_EOP;
+	}
+
 	for (i = 0; i < periods; i++) {
 		int tr_idx = i * num_tr;
 
@@ -3525,8 +3553,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);
 
 		period_addr += period_len;
 	}
@@ -3655,8 +3685,9 @@ udma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 	/* static TR for remote PDMA */
 	if (udma_configure_statictr(uc, d, dev_width, burst)) {
 		dev_err(uc->ud->dev,
-			"%s: StaticTR Z is limited to maximum 4095 (%u)\n",
-			__func__, d->static_tr.bstcnt);
+			"%s: StaticTR Z is limited to maximum %u (%u)\n",
+			__func__, uc->ud->match_data->statictr_z_mask,
+			d->static_tr.bstcnt);
 
 		udma_free_hwdesc(uc, d);
 		kfree(d);

---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240918-z_cnt-3ca5daec76bf

Best regards,
-- 
Jai Luthra <jai.luthra@linux.dev>
Re: [PATCH] dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA transfers
Posted by Francesco Dolcini 2 months, 1 week ago 
Hello Jai,

On Wed, Sep 18, 2024 at 06:46:55PM +0530, Jai Luthra wrote:
> From: Jai Luthra <j-luthra@ti.com>
> 
> When receiving data in cyclic mode from PDMA peripherals, where reload
> count is set to infinite, any TR in the set can potentially be the last
> one of the overall transfer. In such cases, the EOP flag needs to be set
> in each TR and PDMA's Static TR "Z" parameter should be set, matching
> the size of the TR.
> 
> This is required for the teardown to function properly and cleanup the
> internal state memory. This only affects platforms using BCDMA and not
> those using UDMA-P, which could set EOP flag in the teardown TR
> automatically.
> 
> Similarly when transmitting data in cyclic mode to PDMA peripherals, the
> EOP flag needs to be set to get the teardown completion signal
> correctly.
> 
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> Signed-off-by: Jai Luthra <jai.luthra@linux.dev>

Thanks for this, really appreciated!

I did test this patch on top of v6.11, before I had errors every
time I was doing some recording, (e.g. `arecord -D hw:0,0 -c 2 -f S16_LE
-r 44100 -t wav -d 16 /tmp/a.wav`)

[   63.906602] ti-udma 485c0100.dma-controller: chan2 teardown timeout!
[   64.090472] davinci-mcasp 2b00000.audio-controller: Receive buffer overflow
[   65.409909] ti-udma 485c0100.dma-controller: chan2 teardown timeout!

In addition to that I used to have system crashes afterward, but today
it seems that this is not happening with v6.11.

I think that this should go explicitly to stable, so I would add
Cc:stable in your v2 (that I assume you need to send to address some
review comment from Péter).

With all of that said

Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com # Toradex Verdin AM62

Francesco
Re: [PATCH] dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA transfers
Posted by Péter Ujfalusi 2 months, 1 week ago 
Hi Jai,

I only have couple of suggestions, mainly for the comments and commit
message, otherwise looks good.

On 18/09/2024 16:16, Jai Luthra wrote:
> From: Jai Luthra <j-luthra@ti.com>
> 
> When receiving data in cyclic mode from PDMA peripherals, where reload
> count is set to infinite, any TR in the set can potentially be the last
> one of the overall transfer. In such cases, the EOP flag needs to be set
> in each TR and PDMA's Static TR "Z" parameter should be set, matching
> the size of the TR.
> 
> This is required for the teardown to function properly and cleanup the
> internal state memory. This only affects platforms using BCDMA and not
> those using UDMA-P, which could set EOP flag in the teardown TR
> automatically.

Since this only affects BCDMA, I would clarify the commit title to
something:
dmaengine: ti: k3-udma: Set EOP flag for all TRs in cyclic BCDMA transfer

And adjust the commit message accordingly.

Basically we kind of emulate PacketMode with this on BCDMA, right?

> Similarly when transmitting data in cyclic mode to PDMA peripherals, the
> EOP flag needs to be set to get the teardown completion signal
> correctly.
> 
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> Signed-off-by: Jai Luthra <jai.luthra@linux.dev>
> ---
>  drivers/dma/ti/k3-udma.c | 61 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 406ee199c2ac..5a900b63dae5 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -3185,27 +3185,39 @@ static int udma_configure_statictr(struct udma_chan *uc, struct udma_desc *d,
>  
>  	d->static_tr.elcnt = elcnt;
>  
> -	/*
> -	 * PDMA must to close the packet when the channel is in packet mode.
> -	 * For TR mode when the channel is not cyclic we also need PDMA to close
> -	 * the packet otherwise the transfer will stall because PDMA holds on
> -	 * the data it has received from the peripheral.
> -	 */
>  	if (uc->config.pkt_mode || !uc->cyclic) {
> +		/*
> +		 * PDMA must close the packet when the channel is in packet mode.
> +		 * For TR mode when the channel is not cyclic we also need PDMA
> +		 * to close the packet otherwise the transfer will stall because
> +		 * PDMA holds on the data it has received from the peripheral.
> +		 */
>  		unsigned int div = dev_width * elcnt;
>  
>  		if (uc->cyclic)
>  			d->static_tr.bstcnt = d->residue / d->sglen / div;
>  		else
>  			d->static_tr.bstcnt = d->residue / div;
> +	} else if (uc->ud->match_data->type == DMA_TYPE_BCDMA &&
> +		   uc->config.dir == DMA_DEV_TO_MEM && !uc->config.pkt_mode &&
> +		   uc->cyclic) {

BCDMA and pkt_mode cannot be true at the same time, the check for
!uc->config.pkt_mode can be dropped.

> +		/*
> +		 * For cyclic TR mode PDMA must close the packet after every TR

This is only valid for BCDMA, the comment should mention this.

> +		 * transfer, as we have to set EOP in each TR to prevent short
> +		 * packet errors seen on channel teardown.
> +		 */
> +		struct cppi5_tr_type1_t *tr_req = d->hwdesc[0].tr_req_base;
>  
> -		if (uc->config.dir == DMA_DEV_TO_MEM &&
> -		    d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
> -			return -EINVAL;
> +		d->static_tr.bstcnt =
> +			(tr_req->icnt0 * tr_req->icnt1) / dev_width;
>  	} else {
>  		d->static_tr.bstcnt = 0;
>  	}
>  
> +	if (uc->config.dir == DMA_DEV_TO_MEM &&
> +	    d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> @@ -3450,8 +3462,9 @@ udma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	/* static TR for remote PDMA */
>  	if (udma_configure_statictr(uc, d, dev_width, burst)) {
>  		dev_err(uc->ud->dev,
> -			"%s: StaticTR Z is limited to maximum 4095 (%u)\n",
> -			__func__, d->static_tr.bstcnt);
> +			"%s: StaticTR Z is limited to maximum %u (%u)\n",
> +			__func__, uc->ud->match_data->statictr_z_mask,
> +			d->static_tr.bstcnt);
>  
>  		udma_free_hwdesc(uc, d);
>  		kfree(d);
> @@ -3476,6 +3489,7 @@ 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;
>  
>  	num_tr = udma_get_tr_counters(period_len, __ffs(buf_addr), &tr0_cnt0,
>  				      &tr0_cnt1, &tr1_cnt0);
> @@ -3498,6 +3512,20 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
>  		period_addr = buf_addr |
>  			((u64)uc->config.asel << K3_ADDRESS_ASEL_SHIFT);
>  
> +	/*
> +	 * For BCDMA <-> PDMA transfers, the EOP flag needs to be set on the
> +	 * last TR of a descriptor, to mark the packet as complete.
> +	 * This is required for getting the teardown completion message in case
> +	 * of TX, and to avoid short-packet error in case of RX.
> +	 *
> +	 * As we are in cyclic mode, we do not know which period might be the
> +	 * last one, so set the flag for each period.
> +	 */
> +	if (uc->config.ep_type == PSIL_EP_PDMA_XY &&
> +	    uc->ud->match_data->type == DMA_TYPE_BCDMA) {
> +		period_csf = CPPI5_TR_CSF_EOP;
> +	}
> +
>  	for (i = 0; i < periods; i++) {
>  		int tr_idx = i * num_tr;
>  
> @@ -3525,8 +3553,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);
>  
>  		period_addr += period_len;
>  	}
> @@ -3655,8 +3685,9 @@ udma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  	/* static TR for remote PDMA */
>  	if (udma_configure_statictr(uc, d, dev_width, burst)) {
>  		dev_err(uc->ud->dev,
> -			"%s: StaticTR Z is limited to maximum 4095 (%u)\n",
> -			__func__, d->static_tr.bstcnt);
> +			"%s: StaticTR Z is limited to maximum %u (%u)\n",
> +			__func__, uc->ud->match_data->statictr_z_mask,
> +			d->static_tr.bstcnt);
>  
>  		udma_free_hwdesc(uc, d);
>  		kfree(d);
> 
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240918-z_cnt-3ca5daec76bf
> 
> Best regards,

-- 
Péter

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.