| Message ID | 20240918-z_cnt-v1-1-2c58fbfb07d6@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA transfers | expand |
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,
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
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);