[PATCH 1/2] serial: atmel: cleanup atmel_start+stop_tx()

[PATCH 1/2] serial: atmel: cleanup atmel_start+stop_tx()

[Jiri Slaby (SUSE)]

Define local variables holding information about whether pdc or dma is
used in the HW. These are retested several times by calls to
atmel_use_pdc_tx() and atmel_use_dma_tx(). So to make the code more
readable, simply cache the values.

This is also a preparatory patch for the next one (where is_pdc is used
once more in atmel_stop_tx()).

Cc: Richard Genoud <richard.genoud@gmail.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: linux-arm-kernel@lists.infradead.org
Reported-by: Michael Walle <michael@walle.cc>
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/atmel_serial.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 4ca04676c406..65f63dccfd72 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -552,8 +552,9 @@ static u_int atmel_get_mctrl(struct uart_port *port)
 static void atmel_stop_tx(struct uart_port *port)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	bool is_pdc = atmel_use_pdc_tx(port);
 
-	if (atmel_use_pdc_tx(port)) {
+	if (is_pdc) {
 		/* disable PDC transmit */
 		atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
 	}
@@ -572,7 +573,6 @@ static void atmel_stop_tx(struct uart_port *port)
 	if (atmel_uart_is_half_duplex(port))
 		if (!atomic_read(&atmel_port->tasklet_shutdown))
 			atmel_start_rx(port);
-
 }
 
 /*
@@ -581,20 +581,22 @@ static void atmel_stop_tx(struct uart_port *port)
 static void atmel_start_tx(struct uart_port *port)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+	bool is_pdc = atmel_use_pdc_tx(port);
+	bool is_dma = is_pdc || atmel_use_dma_tx(port);
 
-	if (atmel_use_pdc_tx(port) && (atmel_uart_readl(port, ATMEL_PDC_PTSR)
+	if (is_pdc && (atmel_uart_readl(port, ATMEL_PDC_PTSR)
 				       & ATMEL_PDC_TXTEN))
 		/* The transmitter is already running.  Yes, we
 		   really need this.*/
 		return;
 
-	if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
-		if (atmel_uart_is_half_duplex(port))
-			atmel_stop_rx(port);
+	if (is_dma && atmel_uart_is_half_duplex(port))
+		atmel_stop_rx(port);
 
-	if (atmel_use_pdc_tx(port))
+	if (is_pdc) {
 		/* re-enable PDC transmit */
 		atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTEN);
+	}
 
 	/* Enable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
-- 
2.38.1

[PATCH 2/2] serial: atmel: don't stop the transmitter when doing PIO

[Jiri Slaby (SUSE)]

Writing ATMEL_US_TXDIS to ATMEL_US_CR makes the transmitter NOT to send
the just queued character. This means when the character is last and
uart calls ops->stop_tx(), the character is not sent at all.

The usart datasheet is not much specific on this, it just says the
transmitter is stopped. But apparently, the character is dropped. So
we should stop the transmitter only for DMA and PDC transfers to not
send any more characters. For PIO, this is unexpected and deviates from
other drivers. In particular, the below referenced commit broke TX as it
added a call to ->stop_tx() after the very last character written to the
transmitter.

So fix this by limiting the write of ATMEL_US_TXDIS to DMA transfers
only.

Even there, I don't know if it is correctly implemented. Are all the
queued characters sent once ->start_tx() is called? Anyone tested flow
control -- be it hard (RTSCTS) or the soft (XOFF/XON) one?

Fixes: 2d141e683e9a ("tty: serial: use uart_port_tx() helper")
Cc: Richard Genoud <richard.genoud@gmail.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: linux-arm-kernel@lists.infradead.org
Reported-by: Michael Walle <michael@walle.cc>
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/atmel_serial.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 65f63dccfd72..f1c06e12efa0 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -553,19 +553,22 @@ static void atmel_stop_tx(struct uart_port *port)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	bool is_pdc = atmel_use_pdc_tx(port);
+	bool is_dma = is_pdc || atmel_use_dma_tx(port);
 
 	if (is_pdc) {
 		/* disable PDC transmit */
 		atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
 	}
 
-	/*
-	 * Disable the transmitter.
-	 * This is mandatory when DMA is used, otherwise the DMA buffer
-	 * is fully transmitted.
-	 */
-	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
-	atmel_port->tx_stopped = true;
+	if (is_dma) {
+		/*
+		 * Disable the transmitter.
+		 * This is mandatory when DMA is used, otherwise the DMA buffer
+		 * is fully transmitted.
+		 */
+		atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
+		atmel_port->tx_stopped = true;
+	}
 
 	/* Disable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
@@ -601,9 +604,11 @@ static void atmel_start_tx(struct uart_port *port)
 	/* Enable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
 
-	/* re-enable the transmitter */
-	atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
-	atmel_port->tx_stopped = false;
+	if (is_dma) {
+		/* re-enable the transmitter */
+		atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+		atmel_port->tx_stopped = false;
+	}
 }
 
 /*
-- 
2.38.1

Re: [PATCH 1/2] serial: atmel: cleanup atmel_start+stop_tx()

[Michael Walle]

Am 2022-11-23 09:27, schrieb Jiri Slaby (SUSE):
> Define local variables holding information about whether pdc or dma is
> used in the HW. These are retested several times by calls to
> atmel_use_pdc_tx() and atmel_use_dma_tx(). So to make the code more
> readable, simply cache the values.
> 
> This is also a preparatory patch for the next one (where is_pdc is used
> once more in atmel_stop_tx()).
> 
> Cc: Richard Genoud <richard.genoud@gmail.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Reported-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Already merged, but:
Tested-by: Michael Walle <michael@walle.cc>

Thanks,
-michael

Re: [PATCH 2/2] serial: atmel: don't stop the transmitter when doing PIO

[Michael Walle]

Am 2022-11-23 09:27, schrieb Jiri Slaby (SUSE):
> Writing ATMEL_US_TXDIS to ATMEL_US_CR makes the transmitter NOT to send
> the just queued character. This means when the character is last and
> uart calls ops->stop_tx(), the character is not sent at all.
> 
> The usart datasheet is not much specific on this, it just says the
> transmitter is stopped. But apparently, the character is dropped. So
> we should stop the transmitter only for DMA and PDC transfers to not
> send any more characters. For PIO, this is unexpected and deviates from
> other drivers. In particular, the below referenced commit broke TX as 
> it
> added a call to ->stop_tx() after the very last character written to 
> the
> transmitter.
> 
> So fix this by limiting the write of ATMEL_US_TXDIS to DMA transfers
> only.
> 
> Even there, I don't know if it is correctly implemented. Are all the
> queued characters sent once ->start_tx() is called? Anyone tested flow
> control -- be it hard (RTSCTS) or the soft (XOFF/XON) one?
> 
> Fixes: 2d141e683e9a ("tty: serial: use uart_port_tx() helper")
> Cc: Richard Genoud <richard.genoud@gmail.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Reported-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Already merged, but:
Tested-by: Michael Walle <michael@walle.cc>

Thanks,
-michael

Re: [PATCH 2/2] serial: atmel: don't stop the transmitter when doing PIO

[Richard Genoud]

Le 23/11/2022 à 09:50, Michael Walle a écrit :
> Am 2022-11-23 09:27, schrieb Jiri Slaby (SUSE):
>> Writing ATMEL_US_TXDIS to ATMEL_US_CR makes the transmitter NOT to send
>> the just queued character. This means when the character is last and
>> uart calls ops->stop_tx(), the character is not sent at all.
>>
>> The usart datasheet is not much specific on this, it just says the
>> transmitter is stopped. But apparently, the character is dropped. So
>> we should stop the transmitter only for DMA and PDC transfers to not
>> send any more characters. For PIO, this is unexpected and deviates from
>> other drivers. In particular, the below referenced commit broke TX as it
>> added a call to ->stop_tx() after the very last character written to the
>> transmitter.
>>
>> So fix this by limiting the write of ATMEL_US_TXDIS to DMA transfers
>> only.
>>
>> Even there, I don't know if it is correctly implemented. Are all the
>> queued characters sent once ->start_tx() is called? Anyone tested flow
>> control -- be it hard (RTSCTS) or the soft (XOFF/XON) one?
>>
>> Fixes: 2d141e683e9a ("tty: serial: use uart_port_tx() helper")
>> Cc: Richard Genoud <richard.genoud@gmail.com>
>> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Reported-by: Michael Walle <michael@walle.cc>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> 
> Already merged, but:
> Tested-by: Michael Walle <michael@walle.cc>
Acked-by: Richard Genoud <richard.genoud@gmail.com>

> 
> Thanks,
> -michael
Thanks !

Re: [PATCH 1/2] serial: atmel: cleanup atmel_start+stop_tx()

[Richard Genoud]

Le 23/11/2022 à 09:50, Michael Walle a écrit :
> Am 2022-11-23 09:27, schrieb Jiri Slaby (SUSE):
>> Define local variables holding information about whether pdc or dma is
>> used in the HW. These are retested several times by calls to
>> atmel_use_pdc_tx() and atmel_use_dma_tx(). So to make the code more
>> readable, simply cache the values.
>>
>> This is also a preparatory patch for the next one (where is_pdc is used
>> once more in atmel_stop_tx()).
>>
>> Cc: Richard Genoud <richard.genoud@gmail.com>
>> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Reported-by: Michael Walle <michael@walle.cc>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> 
> Already merged, but:
> Tested-by: Michael Walle <michael@walle.cc>
Acked-by: Richard Genoud <richard.genoud@gmail.com>
> 
> Thanks,
> -michael
Thanks !