Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Marek Szyprowski]

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@xxxxxxxxxx>
>>> Cc: Bjorn Andersson <andersson@xxxxxxxxxx>
>>> Cc: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>>> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
>>
>> 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland