| Message ID | 20220222103437.194779-6-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | RZN1 DMA support | expand |
On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > Pausing a partial transfer only causes data to be written to memory that > is a multiple of the memory width setting. > > However, when a DMA client driver finishes DMA early, e.g. due to UART > char timeout interrupt, all data read from the device must be written to > memory. > > Therefore, allow the slave to limit the memory width to ensure all data > read from the device is written to memory when DMA is paused. (I have only 2.17d and 2.21a datasheets, so below based on the latter) It seems you are referring to the chapter 7.7 "Disabling a Channel Prior to Transfer Completion" of the data sheet where it stays that it does not guarantee to have last burst to be completed in case of SRC_TR_WIDTH < DST_TR_WIDTH and the CH_SUSP bit is high, when the FIFO_EMPTY is asserted. Okay, in iDMA 32-bit we have a specific bit (seems like a fix) that drains FIFO, but still it doesn't drain the FIFO fully (in case of misalignment) and the last piece of data (which is less than TR width) is lost when channel gets disabled. Now, if we look at the implementation of the serial8250_rx_dma_flush() we may see that it does 1. Pause channel without draining FIFO 2. Moves data to TTY buffer 3. Terminates channel. During termination it does pause channel again (with draining enabled), followed by disabling channel and resuming it again. According to the 7.7 the resuming channel allows to finish the transfer normally. It seems the logic in the ->terminate_all() is broken and we actually need to resume channel first (possibly conditionally, if it was suspended), then pause it and disable and resume again. The problem with ->terminate_all() is that it has no knowledge if it has been called on paused channel (that's why it has to pause channel itself). The pause on termination is required due to some issues in early steppings of iDMA 32-bit hardware implementations. If my theory is correct, the above change should fix the issues you see.
Hi Andy, Phil, andriy.shevchenko@linux.intel.com wrote on Wed, 23 Feb 2022 15:35:58 +0200: > On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote: > > From: Phil Edworthy <phil.edworthy@renesas.com> > > > > Pausing a partial transfer only causes data to be written to memory that > > is a multiple of the memory width setting. > > > > However, when a DMA client driver finishes DMA early, e.g. due to UART > > char timeout interrupt, all data read from the device must be written to > > memory. > > > > Therefore, allow the slave to limit the memory width to ensure all data > > read from the device is written to memory when DMA is paused. > > (I have only 2.17d and 2.21a datasheets, so below based on the latter) > > It seems you are referring to the chapter 7.7 "Disabling a Channel Prior > to Transfer Completion" of the data sheet where it stays that it does not > guarantee to have last burst to be completed in case of > SRC_TR_WIDTH < DST_TR_WIDTH and the CH_SUSP bit is high, when the FIFO_EMPTY > is asserted. > > Okay, in iDMA 32-bit we have a specific bit (seems like a fix) that drains > FIFO, but still it doesn't drain the FIFO fully (in case of misalignment) > and the last piece of data (which is less than TR width) is lost when channel > gets disabled. > > Now, if we look at the implementation of the serial8250_rx_dma_flush() we > may see that it does > 1. Pause channel without draining FIFO > 2. Moves data to TTY buffer > 3. Terminates channel. > > During termination it does pause channel again (with draining enabled), > followed by disabling channel and resuming it again. > > According to the 7.7 the resuming channel allows to finish the transfer > normally. > > It seems the logic in the ->terminate_all() is broken and we actually need > to resume channel first (possibly conditionally, if it was suspended), then > pause it and disable and resume again. > > The problem with ->terminate_all() is that it has no knowledge if it has > been called on paused channel (that's why it has to pause channel itself). > The pause on termination is required due to some issues in early steppings > of iDMA 32-bit hardware implementations. > > If my theory is correct, the above change should fix the issues you see. I don't have access to these datasheets so I will believe your words and try to apply Andy's solution. I ended up with the following fix, hopefully I got it right: diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 48cdefe997f1..59822664d8ec 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -865,6 +865,10 @@ static int dwc_terminate_all(struct dma_chan *chan) clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags); + /* Ensure the last byte(s) are drained before disabling the channel */ + if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags)) + dwc_chan_resume(dwc, true); + dwc_chan_pause(dwc, true); dwc_chan_disable(dw, dwc); Phil, I know it's been 3 years since you investigated this issue, but do you still have access to the script reproducing the issue? Even better, do you still have the hardware to test? Thanks, Miquèl
On Thu, Feb 24, 2022 at 05:30:09PM +0100, Miquel Raynal wrote: > andriy.shevchenko@linux.intel.com wrote on Wed, 23 Feb 2022 15:35:58 > +0200: > > On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote: ... > > It seems the logic in the ->terminate_all() is broken and we actually need > > to resume channel first (possibly conditionally, if it was suspended), then > > pause it and disable and resume again. > > > > The problem with ->terminate_all() is that it has no knowledge if it has > > been called on paused channel (that's why it has to pause channel itself). > > The pause on termination is required due to some issues in early steppings > > of iDMA 32-bit hardware implementations. > > > > If my theory is correct, the above change should fix the issues you see. > > I don't have access to these datasheets so I will believe your words > and try to apply Andy's solution. I ended up with the following fix, > hopefully I got it right: > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > index 48cdefe997f1..59822664d8ec 100644 > --- a/drivers/dma/dw/core.c > +++ b/drivers/dma/dw/core.c > @@ -865,6 +865,10 @@ static int dwc_terminate_all(struct dma_chan *chan) > > clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags); > > + /* Ensure the last byte(s) are drained before disabling the channel */ > + if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags)) > + dwc_chan_resume(dwc, true); > + > dwc_chan_pause(dwc, true); > > dwc_chan_disable(dw, dwc); Yes, this is good enough PoC. Needs to be tested, thanks! > Phil, I know it's been 3 years since you investigated this issue, but > do you still have access to the script reproducing the issue? Even > better, do you still have the hardware to test?
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 7ab83fe601ed..48cdefe997f1 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -705,6 +705,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) : DWC_CTLL_FC(DW_DMA_FC_D_P2M); + if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width) + data_width = sconfig->dst_addr_width; + for_each_sg(sgl, sg, sg_len, i) { struct dw_desc *desc; u32 len, mem;