Hello Ilpo, > On Wed, 22 Nov 2023, Gregory CLEMENT wrote: > >> From: Arnd Bergmann <arnd@xxxxxxxx> >> >> Since there is no guarantee that the memory returned by >> dma_alloc_coherent() is associated with a 'struct page', using the >> architecture specific phys_to_page() is wrong, but using >> virt_to_page() would be as well. >> >> Stop using sg lists altogether and just use the *_single() functions >> instead. This also simplifies the code a bit since the scatterlists in >> this driver always have only one entry anyway. > > Is that actually a HW limitation? Because it would be nice to get rid of > those memcpy()s and use sg with two entries which is the general > direction serial doing DMA Tx should be moving towards (IMO). Actually I can't answer this. On our platform we don't use DMA. The need for this fix came initially because it triggered a compilation error when doing an an ARM specific call on a MIPS architecture (see the link below). With this patch, I managed to build a kernel image without any error and I didn't see any regression at runtime. > > -- > i. > >> gcl: Add a commit log from the initial thread: For the contecte, all came from the following thread: >> https://lore.kernel.org/lkml/86db0fe5-930d-4cbb-bd7d-03367da38951@xxxxxxxxxxxxxxxx/ >> Regards, Gregory >> Fixes: cb06ff102e2d7 ("ARM: PL011: Add support for Rx DMA buffer polling.") >> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx> >> Tested-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx> >> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx> >> --- >> drivers/tty/serial/amba-pl011.c | 62 +++++++++++++++------------------ >> 1 file changed, 29 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c >> index 61cc24cd90e4b..73a1c40148c25 100644 >> --- a/drivers/tty/serial/amba-pl011.c >> +++ b/drivers/tty/serial/amba-pl011.c >> @@ -219,8 +219,9 @@ static struct vendor_data vendor_st = { >> /* Deals with DMA transactions */ >> >> struct pl011_sgbuf { >> - struct scatterlist sg; >> - char *buf; >> + dma_addr_t dma; >> + size_t len; >> + char *buf; >> }; >> >> struct pl011_dmarx_data { >> @@ -241,7 +242,8 @@ struct pl011_dmarx_data { >> >> struct pl011_dmatx_data { >> struct dma_chan *chan; >> - struct scatterlist sg; >> + dma_addr_t dma; >> + size_t len; >> char *buf; >> bool queued; >> }; >> @@ -369,18 +371,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) >> static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg, >> enum dma_data_direction dir) >> { >> - dma_addr_t dma_addr; >> - >> - sg->buf = dma_alloc_coherent(chan->device->dev, >> - PL011_DMA_BUFFER_SIZE, &dma_addr, GFP_KERNEL); >> + sg->buf = dma_alloc_coherent(chan->device->dev, PL011_DMA_BUFFER_SIZE, >> + &sg->dma, GFP_KERNEL); >> if (!sg->buf) >> return -ENOMEM; >> - >> - sg_init_table(&sg->sg, 1); >> - sg_set_page(&sg->sg, phys_to_page(dma_addr), >> - PL011_DMA_BUFFER_SIZE, offset_in_page(dma_addr)); >> - sg_dma_address(&sg->sg) = dma_addr; >> - sg_dma_len(&sg->sg) = PL011_DMA_BUFFER_SIZE; >> + sg->len = PL011_DMA_BUFFER_SIZE; >> >> return 0; >> } >> @@ -390,8 +385,7 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg, >> { >> if (sg->buf) { >> dma_free_coherent(chan->device->dev, >> - PL011_DMA_BUFFER_SIZE, sg->buf, >> - sg_dma_address(&sg->sg)); >> + PL011_DMA_BUFFER_SIZE, sg->buf, sg->dma); >> } >> } >> >> @@ -552,8 +546,8 @@ static void pl011_dma_tx_callback(void *data) >> >> uart_port_lock_irqsave(&uap->port, &flags); >> if (uap->dmatx.queued) >> - dma_unmap_sg(dmatx->chan->device->dev, &dmatx->sg, 1, >> - DMA_TO_DEVICE); >> + dma_unmap_single(dmatx->chan->device->dev, dmatx->dma, >> + dmatx->len, DMA_TO_DEVICE); >> >> dmacr = uap->dmacr; >> uap->dmacr = dmacr & ~UART011_TXDMAE; >> @@ -639,18 +633,19 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap) >> memcpy(&dmatx->buf[first], &xmit->buf[0], second); >> } >> >> - dmatx->sg.length = count; >> - >> - if (dma_map_sg(dma_dev->dev, &dmatx->sg, 1, DMA_TO_DEVICE) != 1) { >> + dmatx->len = count; >> + dmatx->dma = dma_map_single(dma_dev->dev, dmatx->buf, count, >> + DMA_TO_DEVICE); >> + if (dmatx->dma == DMA_MAPPING_ERROR) { >> uap->dmatx.queued = false; >> dev_dbg(uap->port.dev, "unable to map TX DMA\n"); >> return -EBUSY; >> } >> >> - desc = dmaengine_prep_slave_sg(chan, &dmatx->sg, 1, DMA_MEM_TO_DEV, >> + desc = dmaengine_prep_slave_single(chan, dmatx->dma, dmatx->len, DMA_MEM_TO_DEV, >> DMA_PREP_INTERRUPT | DMA_CTRL_ACK); >> if (!desc) { >> - dma_unmap_sg(dma_dev->dev, &dmatx->sg, 1, DMA_TO_DEVICE); >> + dma_unmap_single(dma_dev->dev, dmatx->dma, dmatx->len, DMA_TO_DEVICE); >> uap->dmatx.queued = false; >> /* >> * If DMA cannot be used right now, we complete this >> @@ -813,8 +808,8 @@ __acquires(&uap->port.lock) >> dmaengine_terminate_async(uap->dmatx.chan); >> >> if (uap->dmatx.queued) { >> - dma_unmap_sg(uap->dmatx.chan->device->dev, &uap->dmatx.sg, 1, >> - DMA_TO_DEVICE); >> + dma_unmap_single(uap->dmatx.chan->device->dev, uap->dmatx.dma, >> + uap->dmatx.len, DMA_TO_DEVICE); >> uap->dmatx.queued = false; >> uap->dmacr &= ~UART011_TXDMAE; >> pl011_write(uap->dmacr, uap, REG_DMACR); >> @@ -836,7 +831,7 @@ static int pl011_dma_rx_trigger_dma(struct uart_amba_port *uap) >> /* Start the RX DMA job */ >> sgbuf = uap->dmarx.use_buf_b ? >> &uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a; >> - desc = dmaengine_prep_slave_sg(rxchan, &sgbuf->sg, 1, >> + desc = dmaengine_prep_slave_single(rxchan, sgbuf->dma, sgbuf->len, >> DMA_DEV_TO_MEM, >> DMA_PREP_INTERRUPT | DMA_CTRL_ACK); >> /* >> @@ -886,7 +881,7 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, >> >> if (uap->dmarx.poll_rate) { >> /* The data can be taken by polling */ >> - dmataken = sgbuf->sg.length - dmarx->last_residue; >> + dmataken = sgbuf->len - dmarx->last_residue; >> /* Recalculate the pending size */ >> if (pending >= dmataken) >> pending -= dmataken; >> @@ -911,7 +906,7 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, >> >> /* Reset the last_residue for Rx DMA poll */ >> if (uap->dmarx.poll_rate) >> - dmarx->last_residue = sgbuf->sg.length; >> + dmarx->last_residue = sgbuf->len; >> >> /* >> * Only continue with trying to read the FIFO if all DMA chars have >> @@ -969,7 +964,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap) >> pl011_write(uap->dmacr, uap, REG_DMACR); >> uap->dmarx.running = false; >> >> - pending = sgbuf->sg.length - state.residue; >> + pending = sgbuf->len - state.residue; >> BUG_ON(pending > PL011_DMA_BUFFER_SIZE); >> /* Then we terminate the transfer - we now know our residue */ >> dmaengine_terminate_all(rxchan); >> @@ -1015,7 +1010,7 @@ static void pl011_dma_rx_callback(void *data) >> * the DMA irq handler. So we check the residue here. >> */ >> rxchan->device->device_tx_status(rxchan, dmarx->cookie, &state); >> - pending = sgbuf->sg.length - state.residue; >> + pending = sgbuf->len - state.residue; >> BUG_ON(pending > PL011_DMA_BUFFER_SIZE); >> /* Then we terminate the transfer - we now know our residue */ >> dmaengine_terminate_all(rxchan); >> @@ -1074,7 +1069,7 @@ static void pl011_dma_rx_poll(struct timer_list *t) >> sgbuf = dmarx->use_buf_b ? &uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a; >> rxchan->device->device_tx_status(rxchan, dmarx->cookie, &state); >> if (likely(state.residue < dmarx->last_residue)) { >> - dmataken = sgbuf->sg.length - dmarx->last_residue; >> + dmataken = sgbuf->len - dmarx->last_residue; >> size = dmarx->last_residue - state.residue; >> dma_count = tty_insert_flip_string(port, sgbuf->buf + dmataken, >> size); >> @@ -1123,7 +1118,7 @@ static void pl011_dma_startup(struct uart_amba_port *uap) >> return; >> } >> >> - sg_init_one(&uap->dmatx.sg, uap->dmatx.buf, PL011_DMA_BUFFER_SIZE); >> + uap->dmatx.len = PL011_DMA_BUFFER_SIZE; >> >> /* The DMA buffer is now the FIFO the TTY subsystem can use */ >> uap->port.fifosize = PL011_DMA_BUFFER_SIZE; >> @@ -1200,8 +1195,9 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap) >> /* In theory, this should already be done by pl011_dma_flush_buffer */ >> dmaengine_terminate_all(uap->dmatx.chan); >> if (uap->dmatx.queued) { >> - dma_unmap_sg(uap->dmatx.chan->device->dev, &uap->dmatx.sg, 1, >> - DMA_TO_DEVICE); >> + dma_unmap_single(uap->dmatx.chan->device->dev, >> + uap->dmatx.dma, uap->dmatx.len, >> + DMA_TO_DEVICE); >> uap->dmatx.queued = false; >> } >> >> -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com
