| Message ID | 20240405060826.2521-12-jirislaby@kernel.org |
|---|---|
| State | Accepted |
| Commit | f8fef2fa419febbfed2d04f0518111565df2673d |
| Headers | show |
| Series | None | expand |
On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: > This is a preparatory for the serial-to-kfifo switch. kfifo understands > only scatter-gatter approach, so switch to that. > > No functional change intended, it's just dmaengine_prep_slave_single() > inline expanded. > > And in this case, switch from dma_map_single() to dma_map_sg() too. This > needs struct msm_dma changes. I split the rx and tx parts into an union. > TX is now struct scatterlist, RX remains the old good phys-virt-count > triple. > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > Cc: linux-arm-msm@vger.kernel.org I've just found that this patch broke UART operation on DragonBoard 410c. I briefly checked and didn't notice anything obviously wrong here, but the board stops transmitting any data from its serial port after the first message. I will try to analyze this issue a bit more tomorrow. > --- > drivers/tty/serial/msm_serial.c | 86 +++++++++++++++++++-------------- > 1 file changed, 49 insertions(+), 37 deletions(-) > > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c > index d27c4c8c84e1..7bf30e632313 100644 > --- a/drivers/tty/serial/msm_serial.c > +++ b/drivers/tty/serial/msm_serial.c > @@ -161,11 +161,16 @@ enum { > struct msm_dma { > struct dma_chan *chan; > enum dma_data_direction dir; > - dma_addr_t phys; > - unsigned char *virt; > + union { > + struct { > + dma_addr_t phys; > + unsigned char *virt; > + unsigned int count; > + } rx; > + struct scatterlist tx_sg; > + }; > dma_cookie_t cookie; > u32 enable_bit; > - unsigned int count; > struct dma_async_tx_descriptor *desc; > }; > > @@ -249,8 +254,12 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma) > unsigned int mapped; > u32 val; > > - mapped = dma->count; > - dma->count = 0; > + if (dma->dir == DMA_TO_DEVICE) { > + mapped = sg_dma_len(&dma->tx_sg); > + } else { > + mapped = dma->rx.count; > + dma->rx.count = 0; > + } > > dmaengine_terminate_all(dma->chan); > > @@ -265,8 +274,13 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma) > val &= ~dma->enable_bit; > msm_write(port, val, UARTDM_DMEN); > > - if (mapped) > - dma_unmap_single(dev, dma->phys, mapped, dma->dir); > + if (mapped) { > + if (dma->dir == DMA_TO_DEVICE) { > + dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir); > + sg_init_table(&dma->tx_sg, 1); > + } else > + dma_unmap_single(dev, dma->rx.phys, mapped, dma->dir); > + } > } > > static void msm_release_dma(struct msm_port *msm_port) > @@ -285,7 +299,7 @@ static void msm_release_dma(struct msm_port *msm_port) > if (dma->chan) { > msm_stop_dma(&msm_port->uart, dma); > dma_release_channel(dma->chan); > - kfree(dma->virt); > + kfree(dma->rx.virt); > } > > memset(dma, 0, sizeof(*dma)); > @@ -357,8 +371,8 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) > > of_property_read_u32(dev->of_node, "qcom,rx-crci", &crci); > > - dma->virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL); > - if (!dma->virt) > + dma->rx.virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL); > + if (!dma->rx.virt) > goto rel_rx; > > memset(&conf, 0, sizeof(conf)); > @@ -385,7 +399,7 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) > > return; > err: > - kfree(dma->virt); > + kfree(dma->rx.virt); > rel_rx: > dma_release_channel(dma->chan); > no_rx: > @@ -420,7 +434,7 @@ static void msm_start_tx(struct uart_port *port) > struct msm_dma *dma = &msm_port->tx_dma; > > /* Already started in DMA mode */ > - if (dma->count) > + if (sg_dma_len(&dma->tx_sg)) > return; > > msm_port->imr |= MSM_UART_IMR_TXLEV; > @@ -448,12 +462,12 @@ static void msm_complete_tx_dma(void *args) > uart_port_lock_irqsave(port, &flags); > > /* Already stopped */ > - if (!dma->count) > + if (!sg_dma_len(&dma->tx_sg)) > goto done; > > dmaengine_tx_status(dma->chan, dma->cookie, &state); > > - dma_unmap_single(port->dev, dma->phys, dma->count, dma->dir); > + dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); > > val = msm_read(port, UARTDM_DMEN); > val &= ~dma->enable_bit; > @@ -464,9 +478,9 @@ static void msm_complete_tx_dma(void *args) > msm_write(port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR); > } > > - count = dma->count - state.residue; > + count = sg_dma_len(&dma->tx_sg) - state.residue; > uart_xmit_advance(port, count); > - dma->count = 0; > + sg_init_table(&dma->tx_sg, 1); > > /* Restore "Tx FIFO below watermark" interrupt */ > msm_port->imr |= MSM_UART_IMR_TXLEV; > @@ -485,19 +499,18 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) > struct circ_buf *xmit = &msm_port->uart.state->xmit; > struct uart_port *port = &msm_port->uart; > struct msm_dma *dma = &msm_port->tx_dma; > - void *cpu_addr; > int ret; > u32 val; > > - cpu_addr = &xmit->buf[xmit->tail]; > + sg_init_table(&dma->tx_sg, 1); > + sg_set_buf(&dma->tx_sg, &xmit->buf[xmit->tail], count); > > - dma->phys = dma_map_single(port->dev, cpu_addr, count, dma->dir); > - ret = dma_mapping_error(port->dev, dma->phys); > + ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); > if (ret) > return ret; > > - dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys, > - count, DMA_MEM_TO_DEV, > + dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1, > + DMA_MEM_TO_DEV, > DMA_PREP_INTERRUPT | > DMA_PREP_FENCE); > if (!dma->desc) { > @@ -520,8 +533,6 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) > msm_port->imr &= ~MSM_UART_IMR_TXLEV; > msm_write(port, msm_port->imr, MSM_UART_IMR); > > - dma->count = count; > - > val = msm_read(port, UARTDM_DMEN); > val |= dma->enable_bit; > > @@ -536,7 +547,8 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) > dma_async_issue_pending(dma->chan); > return 0; > unmap: > - dma_unmap_single(port->dev, dma->phys, count, dma->dir); > + dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); > + sg_init_table(&dma->tx_sg, 1); > return ret; > } > > @@ -553,7 +565,7 @@ static void msm_complete_rx_dma(void *args) > uart_port_lock_irqsave(port, &flags); > > /* Already stopped */ > - if (!dma->count) > + if (!dma->rx.count) > goto done; > > val = msm_read(port, UARTDM_DMEN); > @@ -570,14 +582,14 @@ static void msm_complete_rx_dma(void *args) > > port->icount.rx += count; > > - dma->count = 0; > + dma->rx.count = 0; > > - dma_unmap_single(port->dev, dma->phys, UARTDM_RX_SIZE, dma->dir); > + dma_unmap_single(port->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir); > > for (i = 0; i < count; i++) { > char flag = TTY_NORMAL; > > - if (msm_port->break_detected && dma->virt[i] == 0) { > + if (msm_port->break_detected && dma->rx.virt[i] == 0) { > port->icount.brk++; > flag = TTY_BREAK; > msm_port->break_detected = false; > @@ -588,9 +600,9 @@ static void msm_complete_rx_dma(void *args) > if (!(port->read_status_mask & MSM_UART_SR_RX_BREAK)) > flag = TTY_NORMAL; > > - sysrq = uart_prepare_sysrq_char(port, dma->virt[i]); > + sysrq = uart_prepare_sysrq_char(port, dma->rx.virt[i]); > if (!sysrq) > - tty_insert_flip_char(tport, dma->virt[i], flag); > + tty_insert_flip_char(tport, dma->rx.virt[i], flag); > } > > msm_start_rx_dma(msm_port); > @@ -614,13 +626,13 @@ static void msm_start_rx_dma(struct msm_port *msm_port) > if (!dma->chan) > return; > > - dma->phys = dma_map_single(uart->dev, dma->virt, > + dma->rx.phys = dma_map_single(uart->dev, dma->rx.virt, > UARTDM_RX_SIZE, dma->dir); > - ret = dma_mapping_error(uart->dev, dma->phys); > + ret = dma_mapping_error(uart->dev, dma->rx.phys); > if (ret) > goto sw_mode; > > - dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys, > + dma->desc = dmaengine_prep_slave_single(dma->chan, dma->rx.phys, > UARTDM_RX_SIZE, DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT); > if (!dma->desc) > @@ -648,7 +660,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port) > > msm_write(uart, msm_port->imr, MSM_UART_IMR); > > - dma->count = UARTDM_RX_SIZE; > + dma->rx.count = UARTDM_RX_SIZE; > > dma_async_issue_pending(dma->chan); > > @@ -668,7 +680,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port) > > return; > unmap: > - dma_unmap_single(uart->dev, dma->phys, UARTDM_RX_SIZE, dma->dir); > + dma_unmap_single(uart->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir); > > sw_mode: > /* > @@ -955,7 +967,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id) > } > > if (misr & (MSM_UART_IMR_RXLEV | MSM_UART_IMR_RXSTALE)) { > - if (dma->count) { > + if (dma->rx.count) { > val = MSM_UART_CR_CMD_STALE_EVENT_DISABLE; > msm_write(port, val, MSM_UART_CR); > val = MSM_UART_CR_CMD_RESET_STALE_INT; Best regards
On 15. 04. 24, 23:17, Marek Szyprowski wrote: > On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: >> This is a preparatory for the serial-to-kfifo switch. kfifo understands >> only scatter-gatter approach, so switch to that. >> >> No functional change intended, it's just dmaengine_prep_slave_single() >> inline expanded. >> >> And in this case, switch from dma_map_single() to dma_map_sg() too. This >> needs struct msm_dma changes. I split the rx and tx parts into an union. >> TX is now struct scatterlist, RX remains the old good phys-virt-count >> triple. >> >> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> >> Cc: Bjorn Andersson <andersson@kernel.org> >> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> >> Cc: linux-arm-msm@vger.kernel.org > > I've just found that this patch broke UART operation on DragonBoard > 410c. I briefly checked and didn't notice anything obviously wrong here, > but the board stops transmitting any data from its serial port after the > first message. I will try to analyze this issue a bit more tomorrow. I double checked, but I see no immediate issues in the patch too. So please, if you can analyze this more… thanks,
Hi Jiri, On 16.04.2024 12:23, Jiri Slaby wrote: > On 15. 04. 24, 23:17, Marek Szyprowski wrote: >> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: >>> This is a preparatory for the serial-to-kfifo switch. kfifo understands >>> only scatter-gatter approach, so switch to that. >>> >>> No functional change intended, it's just dmaengine_prep_slave_single() >>> inline expanded. >>> >>> And in this case, switch from dma_map_single() to dma_map_sg() too. >>> This >>> needs struct msm_dma changes. I split the rx and tx parts into an >>> union. >>> TX is now struct scatterlist, RX remains the old good phys-virt-count >>> triple. >>> >>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> >>> Cc: Bjorn Andersson <andersson@kernel.org> >>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> >>> Cc: linux-arm-msm@vger.kernel.org >> >> I've just found that this patch broke UART operation on DragonBoard >> 410c. I briefly checked and didn't notice anything obviously wrong here, >> but the board stops transmitting any data from its serial port after the >> first message. I will try to analyze this issue a bit more tomorrow. > > I double checked, but I see no immediate issues in the patch too. So > please, if you can analyze this more… I've spent some time digging into this issue and frankly speaking I still have no idea WHY it doesn't work (or I seriously mixed something in the scatterlist principles). However I found a workaround to make it working. Maybe it will help a bit guessing what happens there. Here is a workaround: diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index ae7a8e3cf467..fd3f3bf03f33 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -169,6 +169,7 @@ struct msm_dma { } rx; struct scatterlist tx_sg; }; + int mapped; dma_cookie_t cookie; u32 enable_bit; struct dma_async_tx_descriptor *desc; @@ -278,6 +279,7 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma) if (dma->dir == DMA_TO_DEVICE) { dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir); sg_init_table(&dma->tx_sg, 1); + dma->mapped = 0; } else dma_unmap_single(dev, dma->rx.phys, mapped, dma->dir); } @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port) struct msm_dma *dma = &msm_port->tx_dma; /* Already started in DMA mode */ - if (sg_dma_len(&dma->tx_sg)) + if (dma->mapped) return; msm_port->imr |= MSM_UART_IMR_TXLEV; @@ -462,7 +464,7 @@ static void msm_complete_tx_dma(void *args) uart_port_lock_irqsave(port, &flags); /* Already stopped */ - if (!sg_dma_len(&dma->tx_sg)) + if (!dma->mapped) goto done; dmaengine_tx_status(dma->chan, dma->cookie, &state); @@ -481,6 +483,7 @@ static void msm_complete_tx_dma(void *args) count = sg_dma_len(&dma->tx_sg) - state.residue; uart_xmit_advance(port, count); sg_init_table(&dma->tx_sg, 1); + dma->mapped = 0; /* Restore "Tx FIFO below watermark" interrupt */ msm_port->imr |= MSM_UART_IMR_TXLEV; @@ -522,6 +525,7 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) dma->desc->callback_param = msm_port; dma->cookie = dmaengine_submit(dma->desc); + dma->mapped = 1; ret = dma_submit_error(dma->cookie); if (ret) goto unmap; @@ -549,6 +553,7 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) unmap: dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); sg_init_table(&dma->tx_sg, 1); + dma->mapped = 0; return ret; } Best regards
On 17. 04. 24, 12:15, Marek Szyprowski wrote: > Hi Jiri, > > On 16.04.2024 12:23, Jiri Slaby wrote: >> On 15. 04. 24, 23:17, Marek Szyprowski wrote: >>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: >>>> This is a preparatory for the serial-to-kfifo switch. kfifo understands >>>> only scatter-gatter approach, so switch to that. >>>> >>>> No functional change intended, it's just dmaengine_prep_slave_single() >>>> inline expanded. >>>> >>>> And in this case, switch from dma_map_single() to dma_map_sg() too. >>>> This >>>> needs struct msm_dma changes. I split the rx and tx parts into an >>>> union. >>>> TX is now struct scatterlist, RX remains the old good phys-virt-count >>>> triple. >>>> >>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> >>>> Cc: Bjorn Andersson <andersson@kernel.org> >>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> Cc: linux-arm-msm@vger.kernel.org >>> >>> I've just found that this patch broke UART operation on DragonBoard >>> 410c. I briefly checked and didn't notice anything obviously wrong here, >>> but the board stops transmitting any data from its serial port after the >>> first message. I will try to analyze this issue a bit more tomorrow. >> >> I double checked, but I see no immediate issues in the patch too. So >> please, if you can analyze this more… > > I've spent some time digging into this issue and frankly speaking I > still have no idea WHY it doesn't work (or I seriously mixed something > in the scatterlist principles). However I found a workaround to make it > working. Maybe it will help a bit guessing what happens there. ... > @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port) > struct msm_dma *dma = &msm_port->tx_dma; > > /* Already started in DMA mode */ > - if (sg_dma_len(&dma->tx_sg)) > + if (dma->mapped) Thanks for looking into this. I was hesitant if I should use a flag. I should have, apparently. Quick question: What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config? thanks,
On 17.04.2024 12:50, Jiri Slaby wrote: > On 17. 04. 24, 12:15, Marek Szyprowski wrote: >> On 16.04.2024 12:23, Jiri Slaby wrote: >>> On 15. 04. 24, 23:17, Marek Szyprowski wrote: >>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: >>>>> This is a preparatory for the serial-to-kfifo switch. kfifo >>>>> understands >>>>> only scatter-gatter approach, so switch to that. >>>>> >>>>> No functional change intended, it's just >>>>> dmaengine_prep_slave_single() >>>>> inline expanded. >>>>> >>>>> And in this case, switch from dma_map_single() to dma_map_sg() too. >>>>> This >>>>> needs struct msm_dma changes. I split the rx and tx parts into an >>>>> union. >>>>> TX is now struct scatterlist, RX remains the old good phys-virt-count >>>>> triple. >>>>> >>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> >>>>> Cc: Bjorn Andersson <andersson@kernel.org> >>>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>> Cc: linux-arm-msm@vger.kernel.org >>>> >>>> I've just found that this patch broke UART operation on DragonBoard >>>> 410c. I briefly checked and didn't notice anything obviously wrong >>>> here, >>>> but the board stops transmitting any data from its serial port >>>> after the >>>> first message. I will try to analyze this issue a bit more tomorrow. >>> >>> I double checked, but I see no immediate issues in the patch too. So >>> please, if you can analyze this more… >> >> I've spent some time digging into this issue and frankly speaking I >> still have no idea WHY it doesn't work (or I seriously mixed something >> in the scatterlist principles). However I found a workaround to make it >> working. Maybe it will help a bit guessing what happens there. > ... >> @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port) >> struct msm_dma *dma = &msm_port->tx_dma; >> >> /* Already started in DMA mode */ >> - if (sg_dma_len(&dma->tx_sg)) >> + if (dma->mapped) > > Thanks for looking into this. > > I was hesitant if I should use a flag. I should have, apparently. > > Quick question: > What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config? CONFIG_NEED_SG_DMA_LENGTH=y I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to: 1. if (dma->tx_sg.length) 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) but none of the above worked what is really strange and incomprehensible for me. Best regards
On 17. 04. 24, 14:45, Marek Szyprowski wrote: > On 17.04.2024 12:50, Jiri Slaby wrote: >> On 17. 04. 24, 12:15, Marek Szyprowski wrote: >>> On 16.04.2024 12:23, Jiri Slaby wrote: >>>> On 15. 04. 24, 23:17, Marek Szyprowski wrote: >>>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote: >>>>>> This is a preparatory for the serial-to-kfifo switch. kfifo >>>>>> understands >>>>>> only scatter-gatter approach, so switch to that. >>>>>> >>>>>> No functional change intended, it's just >>>>>> dmaengine_prep_slave_single() >>>>>> inline expanded. >>>>>> >>>>>> And in this case, switch from dma_map_single() to dma_map_sg() too. >>>>>> This >>>>>> needs struct msm_dma changes. I split the rx and tx parts into an >>>>>> union. >>>>>> TX is now struct scatterlist, RX remains the old good phys-virt-count >>>>>> triple. >>>>>> >>>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> >>>>>> Cc: Bjorn Andersson <andersson@kernel.org> >>>>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>> Cc: linux-arm-msm@vger.kernel.org >>>>> >>>>> I've just found that this patch broke UART operation on DragonBoard >>>>> 410c. I briefly checked and didn't notice anything obviously wrong >>>>> here, >>>>> but the board stops transmitting any data from its serial port >>>>> after the >>>>> first message. I will try to analyze this issue a bit more tomorrow. >>>> >>>> I double checked, but I see no immediate issues in the patch too. So >>>> please, if you can analyze this more… >>> >>> I've spent some time digging into this issue and frankly speaking I >>> still have no idea WHY it doesn't work (or I seriously mixed something >>> in the scatterlist principles). However I found a workaround to make it >>> working. Maybe it will help a bit guessing what happens there. >> ... >>> @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port) >>> struct msm_dma *dma = &msm_port->tx_dma; >>> >>> /* Already started in DMA mode */ >>> - if (sg_dma_len(&dma->tx_sg)) >>> + if (dma->mapped) >> >> Thanks for looking into this. >> >> I was hesitant if I should use a flag. I should have, apparently. >> >> Quick question: >> What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config? > > > CONFIG_NEED_SG_DMA_LENGTH=y > > > I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to: > > 1. if (dma->tx_sg.length) > > 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) > > but none of the above worked what is really strange and incomprehensible > for me. Thanks. Neither for me. Could you add: { static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); if (dma->mapped != !!sg_dma_len(&dma->tx_sg) && __ratelimit(&rs)) printk_deferred(KERN_DEBUG "%s (%d): mapped=%u dma_len=%u\n", __func__, __LINE__, dma->mapped, sg_dma_len(&dma->tx_sg)); } before each of your 'if (dma->mapped)' to see where sg_dma_len() is wrong and what is its value in the bad case. I hope I did the logic right. thanks,
On 17. 04. 24, 14:45, Marek Szyprowski wrote: > I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to: > > 1. if (dma->tx_sg.length) > > 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) > > but none of the above worked what is really strange and incomprehensible > for me. > Aaaah, nevermind, what about this? Two bugs: 1) dma_map_sg() returns the number of mapped entries. Not zero! 2) And I forgot to zero tx_sg in case of error. --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, count); ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); - if (ret) - return ret; + if (!ret) + goto zero_out; dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1, DMA_MEM_TO_DEV, @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) return 0; unmap: dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); +zero_out: sg_init_table(&dma->tx_sg, 1); return ret; } thanks,
On 19. 04. 24, 9:43, Jiri Slaby wrote: > On 17. 04. 24, 14:45, Marek Szyprowski wrote: >> I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to: >> >> 1. if (dma->tx_sg.length) >> >> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) >> >> but none of the above worked what is really strange and incomprehensible >> for me. >> > > Aaaah, nevermind, what about this? > > Two bugs: > 1) dma_map_sg() returns the number of mapped entries. Not zero! > 2) And I forgot to zero tx_sg in case of error. > > --- a/drivers/tty/serial/msm_serial.c > +++ b/drivers/tty/serial/msm_serial.c > @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port > *msm_port, unsigned int count) > kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, count); > > ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); > - if (ret) > - return ret; > + if (!ret) Still wrong, ret = -EIO missing in here. > + goto zero_out; > > dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1, > DMA_MEM_TO_DEV, > @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port > *msm_port, unsigned int count) > return 0; > unmap: > dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); > +zero_out: > sg_init_table(&dma->tx_sg, 1); > return ret; > } > > > thanks,
On 19.04.2024 09:53, Jiri Slaby wrote: > On 19. 04. 24, 9:43, Jiri Slaby wrote: >> On 17. 04. 24, 14:45, Marek Szyprowski wrote: >>> I alse tried to change the "if (dma->mapped)" check in >>> msm_start_tx() to: >>> >>> 1. if (dma->tx_sg.length) >>> >>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) >>> >>> but none of the above worked what is really strange and >>> incomprehensible >>> for me. >>> >> >> Aaaah, nevermind, what about this? >> >> Two bugs: >> 1) dma_map_sg() returns the number of mapped entries. Not zero! >> 2) And I forgot to zero tx_sg in case of error. >> >> --- a/drivers/tty/serial/msm_serial.c >> +++ b/drivers/tty/serial/msm_serial.c >> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port >> *msm_port, unsigned int count) >> kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, >> count); >> >> ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); >> - if (ret) >> - return ret; >> + if (!ret) > > Still wrong, ret = -EIO missing in here. "if (ret <= 0)" seems to be better here. Indeed this fixed the issue. I checked the code many times, but I missed this dma_map_sg() return value issue. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> + goto zero_out; >> >> dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1, >> DMA_MEM_TO_DEV, >> @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port >> *msm_port, unsigned int count) >> return 0; >> unmap: >> dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); >> +zero_out: >> sg_init_table(&dma->tx_sg, 1); >> return ret; >> } >> >> >> thanks, > Best regards
On 19. 04. 24, 10:00, Marek Szyprowski wrote: > On 19.04.2024 09:53, Jiri Slaby wrote: >> On 19. 04. 24, 9:43, Jiri Slaby wrote: >>> On 17. 04. 24, 14:45, Marek Szyprowski wrote: >>>> I alse tried to change the "if (dma->mapped)" check in >>>> msm_start_tx() to: >>>> >>>> 1. if (dma->tx_sg.length) >>>> >>>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK) >>>> >>>> but none of the above worked what is really strange and >>>> incomprehensible >>>> for me. >>>> >>> >>> Aaaah, nevermind, what about this? >>> >>> Two bugs: >>> 1) dma_map_sg() returns the number of mapped entries. Not zero! >>> 2) And I forgot to zero tx_sg in case of error. >>> >>> --- a/drivers/tty/serial/msm_serial.c >>> +++ b/drivers/tty/serial/msm_serial.c >>> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port >>> *msm_port, unsigned int count) >>> kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, >>> count); >>> >>> ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); >>> - if (ret) >>> - return ret; >>> + if (!ret) >> >> Still wrong, ret = -EIO missing in here. > > "if (ret <= 0)" seems to be better here. It returns unsigned, so I have a better patch. Will send in a second. > Indeed this fixed the issue. I checked the code many times, but I missed > this dma_map_sg() return value issue. Perfect! > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Could you test that one too :)? thanks,
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index d27c4c8c84e1..7bf30e632313 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -161,11 +161,16 @@ enum { struct msm_dma { struct dma_chan *chan; enum dma_data_direction dir; - dma_addr_t phys; - unsigned char *virt; + union { + struct { + dma_addr_t phys; + unsigned char *virt; + unsigned int count; + } rx; + struct scatterlist tx_sg; + }; dma_cookie_t cookie; u32 enable_bit; - unsigned int count; struct dma_async_tx_descriptor *desc; }; @@ -249,8 +254,12 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma) unsigned int mapped; u32 val; - mapped = dma->count; - dma->count = 0; + if (dma->dir == DMA_TO_DEVICE) { + mapped = sg_dma_len(&dma->tx_sg); + } else { + mapped = dma->rx.count; + dma->rx.count = 0; + } dmaengine_terminate_all(dma->chan); @@ -265,8 +274,13 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma) val &= ~dma->enable_bit; msm_write(port, val, UARTDM_DMEN); - if (mapped) - dma_unmap_single(dev, dma->phys, mapped, dma->dir); + if (mapped) { + if (dma->dir == DMA_TO_DEVICE) { + dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir); + sg_init_table(&dma->tx_sg, 1); + } else + dma_unmap_single(dev, dma->rx.phys, mapped, dma->dir); + } } static void msm_release_dma(struct msm_port *msm_port) @@ -285,7 +299,7 @@ static void msm_release_dma(struct msm_port *msm_port) if (dma->chan) { msm_stop_dma(&msm_port->uart, dma); dma_release_channel(dma->chan); - kfree(dma->virt); + kfree(dma->rx.virt); } memset(dma, 0, sizeof(*dma)); @@ -357,8 +371,8 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) of_property_read_u32(dev->of_node, "qcom,rx-crci", &crci); - dma->virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL); - if (!dma->virt) + dma->rx.virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL); + if (!dma->rx.virt) goto rel_rx; memset(&conf, 0, sizeof(conf)); @@ -385,7 +399,7 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) return; err: - kfree(dma->virt); + kfree(dma->rx.virt); rel_rx: dma_release_channel(dma->chan); no_rx: @@ -420,7 +434,7 @@ static void msm_start_tx(struct uart_port *port) struct msm_dma *dma = &msm_port->tx_dma; /* Already started in DMA mode */ - if (dma->count) + if (sg_dma_len(&dma->tx_sg)) return; msm_port->imr |= MSM_UART_IMR_TXLEV; @@ -448,12 +462,12 @@ static void msm_complete_tx_dma(void *args) uart_port_lock_irqsave(port, &flags); /* Already stopped */ - if (!dma->count) + if (!sg_dma_len(&dma->tx_sg)) goto done; dmaengine_tx_status(dma->chan, dma->cookie, &state); - dma_unmap_single(port->dev, dma->phys, dma->count, dma->dir); + dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); val = msm_read(port, UARTDM_DMEN); val &= ~dma->enable_bit; @@ -464,9 +478,9 @@ static void msm_complete_tx_dma(void *args) msm_write(port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR); } - count = dma->count - state.residue; + count = sg_dma_len(&dma->tx_sg) - state.residue; uart_xmit_advance(port, count); - dma->count = 0; + sg_init_table(&dma->tx_sg, 1); /* Restore "Tx FIFO below watermark" interrupt */ msm_port->imr |= MSM_UART_IMR_TXLEV; @@ -485,19 +499,18 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) struct circ_buf *xmit = &msm_port->uart.state->xmit; struct uart_port *port = &msm_port->uart; struct msm_dma *dma = &msm_port->tx_dma; - void *cpu_addr; int ret; u32 val; - cpu_addr = &xmit->buf[xmit->tail]; + sg_init_table(&dma->tx_sg, 1); + sg_set_buf(&dma->tx_sg, &xmit->buf[xmit->tail], count); - dma->phys = dma_map_single(port->dev, cpu_addr, count, dma->dir); - ret = dma_mapping_error(port->dev, dma->phys); + ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir); if (ret) return ret; - dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys, - count, DMA_MEM_TO_DEV, + dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1, + DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_PREP_FENCE); if (!dma->desc) { @@ -520,8 +533,6 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) msm_port->imr &= ~MSM_UART_IMR_TXLEV; msm_write(port, msm_port->imr, MSM_UART_IMR); - dma->count = count; - val = msm_read(port, UARTDM_DMEN); val |= dma->enable_bit; @@ -536,7 +547,8 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count) dma_async_issue_pending(dma->chan); return 0; unmap: - dma_unmap_single(port->dev, dma->phys, count, dma->dir); + dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir); + sg_init_table(&dma->tx_sg, 1); return ret; } @@ -553,7 +565,7 @@ static void msm_complete_rx_dma(void *args) uart_port_lock_irqsave(port, &flags); /* Already stopped */ - if (!dma->count) + if (!dma->rx.count) goto done; val = msm_read(port, UARTDM_DMEN); @@ -570,14 +582,14 @@ static void msm_complete_rx_dma(void *args) port->icount.rx += count; - dma->count = 0; + dma->rx.count = 0; - dma_unmap_single(port->dev, dma->phys, UARTDM_RX_SIZE, dma->dir); + dma_unmap_single(port->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir); for (i = 0; i < count; i++) { char flag = TTY_NORMAL; - if (msm_port->break_detected && dma->virt[i] == 0) { + if (msm_port->break_detected && dma->rx.virt[i] == 0) { port->icount.brk++; flag = TTY_BREAK; msm_port->break_detected = false; @@ -588,9 +600,9 @@ static void msm_complete_rx_dma(void *args) if (!(port->read_status_mask & MSM_UART_SR_RX_BREAK)) flag = TTY_NORMAL; - sysrq = uart_prepare_sysrq_char(port, dma->virt[i]); + sysrq = uart_prepare_sysrq_char(port, dma->rx.virt[i]); if (!sysrq) - tty_insert_flip_char(tport, dma->virt[i], flag); + tty_insert_flip_char(tport, dma->rx.virt[i], flag); } msm_start_rx_dma(msm_port); @@ -614,13 +626,13 @@ static void msm_start_rx_dma(struct msm_port *msm_port) if (!dma->chan) return; - dma->phys = dma_map_single(uart->dev, dma->virt, + dma->rx.phys = dma_map_single(uart->dev, dma->rx.virt, UARTDM_RX_SIZE, dma->dir); - ret = dma_mapping_error(uart->dev, dma->phys); + ret = dma_mapping_error(uart->dev, dma->rx.phys); if (ret) goto sw_mode; - dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys, + dma->desc = dmaengine_prep_slave_single(dma->chan, dma->rx.phys, UARTDM_RX_SIZE, DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT); if (!dma->desc) @@ -648,7 +660,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port) msm_write(uart, msm_port->imr, MSM_UART_IMR); - dma->count = UARTDM_RX_SIZE; + dma->rx.count = UARTDM_RX_SIZE; dma_async_issue_pending(dma->chan); @@ -668,7 +680,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port) return; unmap: - dma_unmap_single(uart->dev, dma->phys, UARTDM_RX_SIZE, dma->dir); + dma_unmap_single(uart->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir); sw_mode: /* @@ -955,7 +967,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id) } if (misr & (MSM_UART_IMR_RXLEV | MSM_UART_IMR_RXSTALE)) { - if (dma->count) { + if (dma->rx.count) { val = MSM_UART_CR_CMD_STALE_EVENT_DISABLE; msm_write(port, val, MSM_UART_CR); val = MSM_UART_CR_CMD_RESET_STALE_INT;
This is a preparatory for the serial-to-kfifo switch. kfifo understands only scatter-gatter approach, so switch to that. No functional change intended, it's just dmaengine_prep_slave_single() inline expanded. And in this case, switch from dma_map_single() to dma_map_sg() too. This needs struct msm_dma changes. I split the rx and tx parts into an union. TX is now struct scatterlist, RX remains the old good phys-virt-count triple. Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> Cc: Bjorn Andersson <andersson@kernel.org> Cc: Konrad Dybcio <konrad.dybcio@linaro.org> Cc: linux-arm-msm@vger.kernel.org --- drivers/tty/serial/msm_serial.c | 86 +++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 37 deletions(-)