Re: [PATCH v5 2/3] tty: serial: use uart_port_tx() helper

From: Jiri Slaby
Date: Tue Nov 22 2022 - 03:23:22 EST

On 22. 11. 22, 9:18, Michael Walle wrote:
Am 2022-11-22 09:09, schrieb Michael Walle:
Hi,

Am 2022-11-22 08:02, schrieb Jiri Slaby:
On 21. 11. 22, 21:27, Michael Walle wrote:
This will break serial output for the userspace on my board
(arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt*dts). The uart_port_tx()
helper will call __port->ops->stop_tx(__port) if uart_circ_chars_pending()
returns 0. But the code above, doesn't do that. In fact, removing the
stop_tx() call in the helper macro, will fix the console output.

Any ideas how to fix that?

Hm, so ATMEL_US_TXRDY is removed from tx_done_mask in stop_tx, but not
added back in start_tx. So the tx interrupt is never handled (the tx
tasklet is not scheduled to send the queue chars) in
atmel_handle_transmit().

Any chance, the below fixes it?

diff --git a/drivers/tty/serial/atmel_serial.c
b/drivers/tty/serial/atmel_serial.c
index 11bf2466390e..395370e0c77b 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -596,6 +596,8 @@ static void atmel_start_tx(struct uart_port *port)
                /* re-enable PDC transmit */
                atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTEN);

+       atmel_port->tx_done_mask |= ATMEL_US_TXRDY;
+
        /* Enable interrupts */
        atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);


thanks,

Unfortunately, that doesn't help. Btw, some characters are transmitted:


[    6.219356] Key type dns_resolver registered
[    6.223679] Registering SWP/SWPB emulation handler
[    6.247530] Loading compiled-in X.509 certificates
[    6.288467] Freeing unused kernel image (initmem) memory: 1024K
[    6.297789] Run /init as init process
WbSOROSOSOSOSOStarting linuxptp system clock synchronization: O

-michael

But you made me look at atmel_stop_tx() and there is this:

       /*
    * 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);

Removing that write, will also fix the problem. Could it be, that
the transmit is still active (via DMA) but the driver will call
tx_stop() and then stop the transmission in the background?

Yes, that was exactly the next step to try. The datasheet doesn't tell much what happens when TXDIS is written while the characters are transmitted. So can you give the attached patch a try?

thanks,
--
js
From 2dcc79384b97a2176bbf84ea634aae4a26efd0b4 Mon Sep 17 00:00:00 2001
From: "Jiri Slaby (SUSE)" <jirislaby@xxxxxxxxxx>
Date: Tue, 22 Nov 2022 09:05:51 +0100
Subject: [PATCH] atmel: fix

---
drivers/tty/serial/atmel_serial.c | 40 ++++++++++++++++++-------------
1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 11bf2466390e..bb44f297a4fc 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -552,19 +552,23 @@ 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);
+ bool is_dma = is_pdc || atmel_use_dma_tx(port);

- if (atmel_use_pdc_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);
@@ -581,27 +585,31 @@ 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 (id_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);

- /* 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