On Tue, Apr 16, 2024 at 07:28:56PM +0300, Serge Semin wrote: > Currently in case of the DEV_TO_MEM or MEM_TO_DEV DMA transfers the memory > data width (single transfer width) is determined based on the buffer > length, buffer base address or DMA master-channel max address width > capability. It isn't enough in case of the channel disabling prior the > block transfer is finished. Here is what DW AHB DMA IP-core databook says > regarding the port suspension (DMA-transfer pause) implementation in the > controller: > > "When CTLx.SRC_TR_WIDTH < CTLx.DST_TR_WIDTH and the CFGx.CH_SUSP bit is > high, the CFGx.FIFO_EMPTY is asserted once the contents of the FIFO do not > permit a single word of CTLx.DST_TR_WIDTH to be formed. However, there may > still be data in the channel FIFO, but not enough to form a single > transfer of CTLx.DST_TR_WIDTH. In this scenario, once the channel is > disabled, the remaining data in the channel FIFO is not transferred to the > destination peripheral." > > So in case if the port gets to be suspended and then disabled it's > possible to have the data silently discarded even though the controller > reported that FIFO is empty and the CTLx.BLOCK_TS indicated the dropped > data already received from the source device. This looks as if the data > somehow got lost on a way from the peripheral device to memory and causes > problems for instance in the DW APB UART driver, which pauses and disables > the DMA-transfer as soon as the recv data timeout happens. Here is the way > it looks: > > Memory <------- DMA FIFO <------ UART FIFO <---------------- UART > DST_TR_WIDTH -+--------| | | > | | | | No more data > Current lvl -+--------| |---------+- DMA-burst lvl > | | |---------+- Leftover data > | | |---------+- SRC_TR_WIDTH > -+--------+-------+---------+ > > In the example above: no more data is getting received over the UART port > and BLOCK_TS is not even close to be fully received; some data is left in > the UART FIFO, but not enough to perform a bursted DMA-xfer to the DMA > FIFO; some data is left in the DMA FIFO, but not enough to be passed > further to the system memory in a single transfer. In this situation the > 8250 UART driver catches the recv timeout interrupt, pauses the > DMA-transfer and terminates it completely, after which the IRQ handler > manually fetches the leftover data from the UART FIFO into the > recv-buffer. But since the DMA-channel has been disabled with the data > left in the DMA FIFO, that data will be just discarded and the recv-buffer > will have a gap of the "current lvl" size in the recv-buffer at the tail > of the lately received data portion. So the data will be lost just due to > the misconfigured DMA transfer. > > Note this is only relevant for the case of the transfer suspension and > _disabling_. No problem will happen if the transfer will be re-enabled > afterwards or the block transfer is fully completed. In the later case the > "FIFO flush mode" will be executed at the transfer final stage in order to > push out the data left in the DMA FIFO. > > In order to fix the denoted problem the DW AHB DMA-engine driver needs to > make sure that the _bursted_ source transfer width is greater or equal to > the single destination transfer (note the HW databook describes more > strict constraint than actually required). Since the peripheral-device > side is prescribed by the client driver logic, the memory-side can be only > used for that. The solution can be easily implemented for the DEV_TO_MEM > transfers just by adjusting the memory-channel address width. Sadly it's > not that easy for the MEM_TO_DEV transfers since the mem-to-dma burst size > is normally dynamically determined by the controller. So the only thing > that can be done is to make sure that memory-side address width can be > greater than the peripheral device address width. ... > +static int dwc_verify_m_buswidth(struct dma_chan *chan) > +{ > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > + struct dw_dma *dw = to_dw_dma(chan->device); > + u32 reg_width, reg_burst, mem_width; > + > + mem_width = dw->pdata->data_width[dwc->dws.m_master]; > + > + /* Make sure src and dst word widths are coherent */ > + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV) { > + reg_width = dwc->dma_sconfig.dst_addr_width; > + if (mem_width < reg_width) > + return -EINVAL; > + > + dwc->dma_sconfig.src_addr_width = mem_width; > + } else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM) { > + reg_width = dwc->dma_sconfig.src_addr_width; > + reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst); > + > + dwc->dma_sconfig.dst_addr_width = min(mem_width, reg_width * reg_burst); I understand the desire to go this way, but wouldn't be better to have a symmetrical check and return an error? > + } > + > + return 0; > +} IIRC MEM side of the DMA channel will ignore those in HW, so basically you are (re-)using them purely for the __ffs() corrections. ... > dwc->dma_sconfig.src_maxburst = > - clamp(dwc->dma_sconfig.src_maxburst, 0U, dwc->max_burst); > + clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst); > dwc->dma_sconfig.dst_maxburst = > - clamp(dwc->dma_sconfig.dst_maxburst, 0U, dwc->max_burst); > + clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst); -- With Best Regards, Andy Shevchenko