All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Genoud <richard.genoud@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Nicolas Ferre" <nicolas.ferre@atmel.com>,
	"Alexandre Belloni" <alexandre.belloni@free-electrons.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Cyrille Pitchen" <cyrille.pitchen@atmel.com>
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Richard Genoud <richard.genoud@gmail.com>
Subject: [PATCH] tty/serial: at91: fix hardware handshake on Atmel platforms
Date: Tue, 25 Oct 2016 18:11:35 +0200	[thread overview]
Message-ID: <20161025161135.7316-1-richard.genoud@gmail.com> (raw)

commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled"), despite its title, broke hardware
handshake on *every* Atmel platforms.

The only one partially working is the SAMA5D2.

To understand why, one has to understand the flag ATMEL_US_USMODE_HWHS
first:
Before commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management
when hardware handshake is enabled"), this flag was never set.
Thus, the CTS/RTS where only handled by serial_core (and everything
worked just fine).

This commit introduced the use of the ATMEL_US_USMODE_HWHS flag,
enabling it for all boards when the user space enables flow control.

When the ATMEL_US_USMODE_HWHS is set, the Atmel USART controller
handles a part of the flow control job:
- disable the transmitter when the CTS pin gets high.
- drive the RTS pin high when the DMA buffer transfer is completed or
  PDC RX buffer full or RX FIFO is beyond threshold. (depending on the
  controller version).

NB: This feature is *not* mandatory for the flow control to work.

Now, the specifics of the ATMEL_US_USMODE_HWHS flag:

- For platforms with DMAC and no FIFOs (sam9x25, sam9x35, sama5D3,
sama5D4, sam9g15, sam9g25, sam9g35)* this feature simply doesn't work.
( source: https://lkml.org/lkml/2016/9/7/598 )
Tested it on sam9g35, the RTS pins always stays up, even when RXEN=1
or a new DMA transfer descriptor is set.
=> ATMEL_US_USMODE_HWHS should not be used for those platforms

- For platforms with a PDC (sam926{0,1,3}, sam9g10, sam9g20, sam9g45,
sam9g46)*, there's another kind of problem. Once the flag
ATMEL_US_USMODE_HWHS is set, the RTS pin can't be driven anymore via
RTSEN/RTSDIS in USART Control Register. The RTS pin can only be driven
by enabling/disabling the receiver or setting RCR=RNCR=0 in the PDC
(Receive (Next) Counter Register).
=> Doing this is beyond the scope of this patch and could add other
bugs, so the original (and working) behaviour should be set for those
platforms (meaning ATMEL_US_USMODE_HWHS flag should be unset).

- For platforms with a FIFO (sama5d2)*, the RTS pin is driven according
to the RX FIFO thresholds, and can be also driven by RTSEN/RTSDIS in
USART Control Register. No problem here.
(This was the use case of commit 1cf6e8fc8341 ("tty/serial: at91: fix
RTS line management when hardware handshake is enabled"))
NB: If the CTS pin declared as a GPIO in the DTS, (for instance
cts-gpios = <&pioA PIN_PB31 GPIO_ACTIVE_LOW>), the transmitter will be
disabled.
=> ATMEL_US_USMODE_HWHS flag can be set for this platform ONLY IF the
CTS pin is not a GPIO.

So, the only case when ATMEL_US_USMODE_HWHS can be enabled is when
(atmel_use_fifo(port) &&
 !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS))

Tested on all Atmel USART controller flavours:
 AT91SAM9G35-CM, AT91SAM9G20-EK and SAMA5D2xplained
      ^^^^           ^^^^               ^^^^
  (DMAC flavour), (PDC flavour) and (FIFO flavour)

Changes since v4:
 - the mctrl_gpio_use_rtscts() is gone since it was atmel_serial
 specific. (so patch 1 is gone)
 - patches 2 and 3 have been merged together since it didn't make
 a lot of sense to correct the GPIO case in one separate patch.
 - ATMEL_US_USMODE_HWHS is now unset for platform with PDC

Changes since v3:
 - remove superfluous #include <linux/err.h> (thanks to Uwe)
 - rebase on next-20160930

Changes since v2:
 - remove IS_ERR_OR_NULL() test in patch 1/3 as Uwe suggested.
 - fix typos in patch 2/3
 - rebase on next-20160927
 - simplify the logic in patch 3/3.

Changes since v1:
 - Correct patch 1 with the error found by kbuild.
 - Add Alexandre's Acked-by on patch 2
 - Rewrite patch 3 logic in the light of the on-going discussion
   with Cyrille and Alexandre.

* the list may not be exhaustive

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

 I think this should go in the stable tree since it fixes the flow
 control broken since v4.0.
 But It won't compile on versions before 4.9rc1 because:
 function atmel_use_fifo was introduced in 4.4.12 / 4.7
 variable atmel_port was introduced in 4.9rc1

 That's why I didn't add the Cc stable in the email body.


diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index fd8aa1f4ba78..2c7c45904ba7 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2132,11 +2132,28 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		mode |= ATMEL_US_USMODE_RS485;
 	} else if (termios->c_cflag & CRTSCTS) {
 		/* RS232 with hardware handshake (RTS/CTS) */
-		if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) {
-			dev_info(port->dev, "not enabling hardware flow control because DMA is used");
-			termios->c_cflag &= ~CRTSCTS;
-		} else {
+		if (atmel_use_fifo(port) &&
+		    !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) {
+			/*
+			 * with ATMEL_US_USMODE_HWHS set, the controller will
+			 * be able to drive the RTS pin high/low when the RX
+			 * FIFO is above RXFTHRES/below RXFTHRES2.
+			 * It will also disable the transmitter when the CTS
+			 * pin is high.
+			 * This mode is not activated if CTS pin is a GPIO
+			 * because in this case, the transmitter is always
+			 * disabled.
+			 * If the RTS pin is a GPIO, the controller won't be
+			 * able to drive it according to the FIFO thresholds,
+			 * but it will be handled by the driver.
+			 */
 			mode |= ATMEL_US_USMODE_HWHS;
+		} else {
+			/*
+			 * For platforms without FIFO, the flow control is
+			 * handled by the driver.
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 		}
 	} else {
 		/* RS232 without hadware handshake */

WARNING: multiple messages have this Message-ID (diff)
From: richard.genoud@gmail.com (Richard Genoud)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] tty/serial: at91: fix hardware handshake on Atmel platforms
Date: Tue, 25 Oct 2016 18:11:35 +0200	[thread overview]
Message-ID: <20161025161135.7316-1-richard.genoud@gmail.com> (raw)

commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled"), despite its title, broke hardware
handshake on *every* Atmel platforms.

The only one partially working is the SAMA5D2.

To understand why, one has to understand the flag ATMEL_US_USMODE_HWHS
first:
Before commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management
when hardware handshake is enabled"), this flag was never set.
Thus, the CTS/RTS where only handled by serial_core (and everything
worked just fine).

This commit introduced the use of the ATMEL_US_USMODE_HWHS flag,
enabling it for all boards when the user space enables flow control.

When the ATMEL_US_USMODE_HWHS is set, the Atmel USART controller
handles a part of the flow control job:
- disable the transmitter when the CTS pin gets high.
- drive the RTS pin high when the DMA buffer transfer is completed or
  PDC RX buffer full or RX FIFO is beyond threshold. (depending on the
  controller version).

NB: This feature is *not* mandatory for the flow control to work.

Now, the specifics of the ATMEL_US_USMODE_HWHS flag:

- For platforms with DMAC and no FIFOs (sam9x25, sam9x35, sama5D3,
sama5D4, sam9g15, sam9g25, sam9g35)* this feature simply doesn't work.
( source: https://lkml.org/lkml/2016/9/7/598 )
Tested it on sam9g35, the RTS pins always stays up, even when RXEN=1
or a new DMA transfer descriptor is set.
=> ATMEL_US_USMODE_HWHS should not be used for those platforms

- For platforms with a PDC (sam926{0,1,3}, sam9g10, sam9g20, sam9g45,
sam9g46)*, there's another kind of problem. Once the flag
ATMEL_US_USMODE_HWHS is set, the RTS pin can't be driven anymore via
RTSEN/RTSDIS in USART Control Register. The RTS pin can only be driven
by enabling/disabling the receiver or setting RCR=RNCR=0 in the PDC
(Receive (Next) Counter Register).
=> Doing this is beyond the scope of this patch and could add other
bugs, so the original (and working) behaviour should be set for those
platforms (meaning ATMEL_US_USMODE_HWHS flag should be unset).

- For platforms with a FIFO (sama5d2)*, the RTS pin is driven according
to the RX FIFO thresholds, and can be also driven by RTSEN/RTSDIS in
USART Control Register. No problem here.
(This was the use case of commit 1cf6e8fc8341 ("tty/serial: at91: fix
RTS line management when hardware handshake is enabled"))
NB: If the CTS pin declared as a GPIO in the DTS, (for instance
cts-gpios = <&pioA PIN_PB31 GPIO_ACTIVE_LOW>), the transmitter will be
disabled.
=> ATMEL_US_USMODE_HWHS flag can be set for this platform ONLY IF the
CTS pin is not a GPIO.

So, the only case when ATMEL_US_USMODE_HWHS can be enabled is when
(atmel_use_fifo(port) &&
 !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS))

Tested on all Atmel USART controller flavours:
 AT91SAM9G35-CM, AT91SAM9G20-EK and SAMA5D2xplained
      ^^^^           ^^^^               ^^^^
  (DMAC flavour), (PDC flavour) and (FIFO flavour)

Changes since v4:
 - the mctrl_gpio_use_rtscts() is gone since it was atmel_serial
 specific. (so patch 1 is gone)
 - patches 2 and 3 have been merged together since it didn't make
 a lot of sense to correct the GPIO case in one separate patch.
 - ATMEL_US_USMODE_HWHS is now unset for platform with PDC

Changes since v3:
 - remove superfluous #include <linux/err.h> (thanks to Uwe)
 - rebase on next-20160930

Changes since v2:
 - remove IS_ERR_OR_NULL() test in patch 1/3 as Uwe suggested.
 - fix typos in patch 2/3
 - rebase on next-20160927
 - simplify the logic in patch 3/3.

Changes since v1:
 - Correct patch 1 with the error found by kbuild.
 - Add Alexandre's Acked-by on patch 2
 - Rewrite patch 3 logic in the light of the on-going discussion
   with Cyrille and Alexandre.

* the list may not be exhaustive

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/atmel_serial.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

 I think this should go in the stable tree since it fixes the flow
 control broken since v4.0.
 But It won't compile on versions before 4.9rc1 because:
 function atmel_use_fifo was introduced in 4.4.12 / 4.7
 variable atmel_port was introduced in 4.9rc1

 That's why I didn't add the Cc stable in the email body.


diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index fd8aa1f4ba78..2c7c45904ba7 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2132,11 +2132,28 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		mode |= ATMEL_US_USMODE_RS485;
 	} else if (termios->c_cflag & CRTSCTS) {
 		/* RS232 with hardware handshake (RTS/CTS) */
-		if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) {
-			dev_info(port->dev, "not enabling hardware flow control because DMA is used");
-			termios->c_cflag &= ~CRTSCTS;
-		} else {
+		if (atmel_use_fifo(port) &&
+		    !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) {
+			/*
+			 * with ATMEL_US_USMODE_HWHS set, the controller will
+			 * be able to drive the RTS pin high/low when the RX
+			 * FIFO is above RXFTHRES/below RXFTHRES2.
+			 * It will also disable the transmitter when the CTS
+			 * pin is high.
+			 * This mode is not activated if CTS pin is a GPIO
+			 * because in this case, the transmitter is always
+			 * disabled.
+			 * If the RTS pin is a GPIO, the controller won't be
+			 * able to drive it according to the FIFO thresholds,
+			 * but it will be handled by the driver.
+			 */
 			mode |= ATMEL_US_USMODE_HWHS;
+		} else {
+			/*
+			 * For platforms without FIFO, the flow control is
+			 * handled by the driver.
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 		}
 	} else {
 		/* RS232 without hadware handshake */

             reply	other threads:[~2016-10-25 16:11 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-25 16:11 Richard Genoud [this message]
2016-10-25 16:11 ` [PATCH] tty/serial: at91: fix hardware handshake on Atmel platforms Richard Genoud
2016-10-25 16:22 ` Alexandre Belloni
2016-10-25 16:22   ` Alexandre Belloni
2016-10-26  6:52   ` Richard Genoud
2016-10-26  6:52     ` Richard Genoud
2016-10-25 17:17 ` Uwe Kleine-König
2016-10-25 17:17   ` Uwe Kleine-König
2016-10-26 14:55   ` Richard Genoud
2016-10-26 14:55     ` Richard Genoud
2016-10-26 15:35     ` Alexandre Belloni
2016-10-26 15:35       ` Alexandre Belloni
2016-10-26 15:35       ` Alexandre Belloni
2016-10-26 15:51       ` Richard Genoud
2016-10-26 15:51         ` Richard Genoud
2016-10-26 16:09         ` Alexandre Belloni
2016-10-26 16:09           ` Alexandre Belloni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161025161135.7316-1-richard.genoud@gmail.com \
    --to=richard.genoud@gmail.com \
    --cc=alexandre.belloni@free-electrons.com \
    --cc=cyrille.pitchen@atmel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=nicolas.ferre@atmel.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.