[PATCHv4 0/3] Fix hardware handshake on SAM9x5 platforms

[PATCHv4 0/3] Fix hardware handshake on SAM9x5 platforms

[Richard Genoud]

Since commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled"), hardware handshake is not working
anymore on SAM9x5/SAMA5D3/SAM9 platforms.

The first two patches fix the hardware handshake when CTS/RTS pins are
handled by GPIOs.

The last patch fixes hardware handshake when CTS/RTS pins are not GPIOs.

Changes since v3:
 - remove superfuous #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.

NB: patch 2 NEEDS patch 1 to compile.

Richard Genoud (3):
  serial: mctrl_gpio: implement mctrl_gpio_use_rtscts
  tty/serial: at91: fix hardware handshake with GPIOs
  tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs)

 drivers/tty/serial/atmel_serial.c      | 26 +++++++++++++++++---------
 drivers/tty/serial/serial_mctrl_gpio.c |  7 +++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 3 files changed, 34 insertions(+), 9 deletions(-)

[PATCHv4 0/3] Fix hardware handshake on SAM9x5 platforms

[Richard Genoud]

Since commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled"), hardware handshake is not working
anymore on SAM9x5/SAMA5D3/SAM9 platforms.

The first two patches fix the hardware handshake when CTS/RTS pins are
handled by GPIOs.

The last patch fixes hardware handshake when CTS/RTS pins are not GPIOs.

Changes since v3:
 - remove superfuous #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.

NB: patch 2 NEEDS patch 1 to compile.

Richard Genoud (3):
  serial: mctrl_gpio: implement mctrl_gpio_use_rtscts
  tty/serial: at91: fix hardware handshake with GPIOs
  tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs)

 drivers/tty/serial/atmel_serial.c      | 26 +++++++++++++++++---------
 drivers/tty/serial/serial_mctrl_gpio.c |  7 +++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 3 files changed, 34 insertions(+), 9 deletions(-)

[PATCHv4 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts

[Richard Genoud]

This function returns true if CTS and RTS are used as GPIOs.
Some drivers (like atmel_serial) needs to know if the flow control is
handled by the controller or by GPIOs.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/serial_mctrl_gpio.c |  7 +++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index d2da6aa7f27d..38e6e784faa2 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -72,6 +72,13 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
 }
 EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod);
 
+bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+	return mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS) &&
+		mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS);
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_use_rtscts);
+
 unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
 {
 	enum mctrl_gpio_idx i;
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
index fa000bcff217..c34269733c62 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.h
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -101,6 +101,11 @@ void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
  */
 void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
 
+/*
+ * Return true if both CTS and RTS are used with GPIOs
+ */
+bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios);
+
 #else /* GPIOLIB */
 
 static inline
@@ -152,6 +157,11 @@ static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
 {
 }
 
+static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+	return false;
+}
+
 #endif /* GPIOLIB */
 
 #endif

[PATCHv4 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts

[Richard Genoud]

This function returns true if CTS and RTS are used as GPIOs.
Some drivers (like atmel_serial) needs to know if the flow control is
handled by the controller or by GPIOs.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/tty/serial/serial_mctrl_gpio.c |  7 +++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index d2da6aa7f27d..38e6e784faa2 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -72,6 +72,13 @@ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios,
 }
 EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod);
 
+bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+	return mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS) &&
+		mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS);
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_use_rtscts);
+
 unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
 {
 	enum mctrl_gpio_idx i;
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
index fa000bcff217..c34269733c62 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.h
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -101,6 +101,11 @@ void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios);
  */
 void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios);
 
+/*
+ * Return true if both CTS and RTS are used with GPIOs
+ */
+bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios);
+
 #else /* GPIOLIB */
 
 static inline
@@ -152,6 +157,11 @@ static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
 {
 }
 
+static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+	return false;
+}
+
 #endif /* GPIOLIB */
 
 #endif

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled") broke the hardware handshake when GPIOs
were used.

Hardware handshake with GPIOs used to work before this commit because
the CRTSCTS flag (termios->c_cflag) was set, but not the
ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
enabled, but not handled by the controller.

This commit restores this behaviour.

NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
and it will also need previous commit:
"serial: mctrl_gpio: implement mctrl_gpio_use_rtscts"

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
---
 drivers/tty/serial/atmel_serial.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index fd8aa1f4ba78..b01b68ece35c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,8 +2130,12 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		atmel_uart_writel(port, ATMEL_US_TTGR,
 				  port->rs485.delay_rts_after_send);
 		mode |= ATMEL_US_USMODE_RS485;
-	} else if (termios->c_cflag & CRTSCTS) {
-		/* RS232 with hardware handshake (RTS/CTS) */
+	} else if ((termios->c_cflag & CRTSCTS) &&
+		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
+		/*
+		 * RS232 with hardware handshake (RTS/CTS)
+		 * handled by the controller.
+		 */
 		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;
@@ -2139,7 +2143,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 			mode |= ATMEL_US_USMODE_HWHS;
 		}
 	} else {
-		/* RS232 without hadware handshake */
+		/* RS232 without hardware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled") broke the hardware handshake when GPIOs
were used.

Hardware handshake with GPIOs used to work before this commit because
the CRTSCTS flag (termios->c_cflag) was set, but not the
ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
enabled, but not handled by the controller.

This commit restores this behaviour.

NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
and it will also need previous commit:
"serial: mctrl_gpio: implement mctrl_gpio_use_rtscts"

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
---
 drivers/tty/serial/atmel_serial.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index fd8aa1f4ba78..b01b68ece35c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,8 +2130,12 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		atmel_uart_writel(port, ATMEL_US_TTGR,
 				  port->rs485.delay_rts_after_send);
 		mode |= ATMEL_US_USMODE_RS485;
-	} else if (termios->c_cflag & CRTSCTS) {
-		/* RS232 with hardware handshake (RTS/CTS) */
+	} else if ((termios->c_cflag & CRTSCTS) &&
+		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
+		/*
+		 * RS232 with hardware handshake (RTS/CTS)
+		 * handled by the controller.
+		 */
 		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;
@@ -2139,7 +2143,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 			mode |= ATMEL_US_USMODE_HWHS;
 		}
 	} else {
-		/* RS232 without hadware handshake */
+		/* RS232 without hardware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

[PATCHv4 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs)

[Richard Genoud]

Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled") broke the hardware handshake on SAM9x5
platforms.

On Atmel platforms, the USART can only handle the handware handshake
(ATMEL_US_USMODE_HWHS) if FIFOs or PDC are used.

Thus, ATMEL_US_USMODE_HWHS mode should only be used in this case.

For SAM9x5, there's no FIFOs nor PDC for the USART, so the mode should
be ATMEL_US_USMODE_NORMAL and the RTS pin should be controlled by the
driver.

NB: -stable is not Cced because it doesn't cleanly apply on 4.1+

Tested on SAM9G35-CM with and without DMA

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
---
 drivers/tty/serial/atmel_serial.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index b01b68ece35c..4d033e6af44a 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2131,19 +2131,23 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 				  port->rs485.delay_rts_after_send);
 		mode |= ATMEL_US_USMODE_RS485;
 	} else if ((termios->c_cflag & CRTSCTS) &&
-		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
+		   !mctrl_gpio_use_rtscts(atmel_port->gpios) &&
+		   (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) {
 		/*
-		 * RS232 with hardware handshake (RTS/CTS)
-		 * handled by the controller.
+		 * Automatic hardware handshake (RTS/CTS) only work with
+		 * FIFOs or PDC.
+		 * Meaning that on SAM9x5 the controller can't handle
+		 * the hardware handshake (no FIFOs nor PDC on these platforms).
 		 */
-		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 {
-			mode |= ATMEL_US_USMODE_HWHS;
-		}
+		mode |= ATMEL_US_USMODE_HWHS;
 	} else {
-		/* RS232 without hardware handshake or controlled by GPIOs */
+		/*
+		 * Other cases are:
+		 * - RS232 without hardware handshake
+		 * - RS232 with hardware handshake and:
+		 *   - controller unable to handle CTS/RTS by itself
+		 *   - or CTS/RTS handled by GPIOs
+		 */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

[PATCHv4 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs)

[Richard Genoud]

Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
hardware handshake is enabled") broke the hardware handshake on SAM9x5
platforms.

On Atmel platforms, the USART can only handle the handware handshake
(ATMEL_US_USMODE_HWHS) if FIFOs or PDC are used.

Thus, ATMEL_US_USMODE_HWHS mode should only be used in this case.

For SAM9x5, there's no FIFOs nor PDC for the USART, so the mode should
be ATMEL_US_USMODE_NORMAL and the RTS pin should be controlled by the
driver.

NB: -stable is not Cced because it doesn't cleanly apply on 4.1+

Tested on SAM9G35-CM with and without DMA

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
---
 drivers/tty/serial/atmel_serial.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index b01b68ece35c..4d033e6af44a 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2131,19 +2131,23 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 				  port->rs485.delay_rts_after_send);
 		mode |= ATMEL_US_USMODE_RS485;
 	} else if ((termios->c_cflag & CRTSCTS) &&
-		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
+		   !mctrl_gpio_use_rtscts(atmel_port->gpios) &&
+		   (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) {
 		/*
-		 * RS232 with hardware handshake (RTS/CTS)
-		 * handled by the controller.
+		 * Automatic hardware handshake (RTS/CTS) only work with
+		 * FIFOs or PDC.
+		 * Meaning that on SAM9x5 the controller can't handle
+		 * the hardware handshake (no FIFOs nor PDC on these platforms).
 		 */
-		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 {
-			mode |= ATMEL_US_USMODE_HWHS;
-		}
+		mode |= ATMEL_US_USMODE_HWHS;
 	} else {
-		/* RS232 without hardware handshake or controlled by GPIOs */
+		/*
+		 * Other cases are:
+		 * - RS232 without hardware handshake
+		 * - RS232 with hardware handshake and:
+		 *   - controller unable to handle CTS/RTS by itself
+		 *   - or CTS/RTS handled by GPIOs
+		 */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Uwe Kleine-König]

Hello Richard,

On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> were used.
> 
> Hardware handshake with GPIOs used to work before this commit because
> the CRTSCTS flag (termios->c_cflag) was set, but not the
> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> enabled, but not handled by the controller.

What does the HWHS flag control? What if only RTS is a gpio and CTS is
not? Or the other way round?

What is the problematic setup? I guess it's RTS and CTS are gpios and
with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that
happens?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Uwe Kleine-König]

Hello Richard,

On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> were used.
> 
> Hardware handshake with GPIOs used to work before this commit because
> the CRTSCTS flag (termios->c_cflag) was set, but not the
> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> enabled, but not handled by the controller.

What does the HWHS flag control? What if only RTS is a gpio and CTS is
not? Or the other way round?

What is the problematic setup? I guess it's RTS and CTS are gpios and
with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that
happens?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Richard,
>
> On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> were used.
>>
>> Hardware handshake with GPIOs used to work before this commit because
>> the CRTSCTS flag (termios->c_cflag) was set, but not the
>> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
>> enabled, but not handled by the controller.
>
> What does the HWHS flag control? What if only RTS is a gpio and CTS is
> not? Or the other way round?
First, HWHS flag is used only in SAMA5D2. (if I correctly understood
Atmel HW guys,
all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
is unusable,
because they don't have Fifos nor PDC).
So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
pin according to
the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of
 http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
The controller will also start/stop the transmission on CTS changes.
But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
With this flag set, It's mandatory to have CTS and RTS not handled via
GPIO, because if
they were, the controller couldn't, well, control them.

The NORMAL flag, on the contrary, just tell the controller not to mess
with RTS/CTS,
and in this case, the driver will handle CTS changes and drive the RTS
pin, via GPIO or
via the CR register.

It's not a problem to have CTS as a GPIO and RTS controlled via the CR register
(or CTS changes read in CSR register and RTS as a GPIO).
I just gave it a quick try, works the same.
( But I don't know if it will work with FIFOs

> What is the problematic setup? I guess it's RTS and CTS are gpios and
> with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that
> happens?

Yes, CTS/RTS as GPIOs + HWHS flag is clearly wrong.
If that happens, well, the controller will try to drive an RTS pin
that won't have been muxed
as RTS. It does not seems to be a problem, but instructing the
controller do drive pins it doesn't
have access doesn't really make sense.
And in the case of the SAMA5D2, well I don't know what the result will
be, but it could stop transmission
from the "ghost" CTS signal I guess.

Anyway, the problematics setups are all the setups with USMODE_HWHS
enabled on platform without Fifos or PDC,
i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).

For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and
RTS/CTS NOT muxed as GPIOS,
it's like there was no flow control at all (the CTS pin doesn't
disable the transmitter).

Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2,
(cfhttps://lkml.org/lkml/2016/9/7/598 ),  I honestly didn't dig any
further into that flag.


Regards,
Richard

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Richard,
>
> On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> were used.
>>
>> Hardware handshake with GPIOs used to work before this commit because
>> the CRTSCTS flag (termios->c_cflag) was set, but not the
>> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
>> enabled, but not handled by the controller.
>
> What does the HWHS flag control? What if only RTS is a gpio and CTS is
> not? Or the other way round?
First, HWHS flag is used only in SAMA5D2. (if I correctly understood
Atmel HW guys,
all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
is unusable,
because they don't have Fifos nor PDC).
So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
pin according to
the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of
 http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
The controller will also start/stop the transmission on CTS changes.
But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
With this flag set, It's mandatory to have CTS and RTS not handled via
GPIO, because if
they were, the controller couldn't, well, control them.

The NORMAL flag, on the contrary, just tell the controller not to mess
with RTS/CTS,
and in this case, the driver will handle CTS changes and drive the RTS
pin, via GPIO or
via the CR register.

It's not a problem to have CTS as a GPIO and RTS controlled via the CR register
(or CTS changes read in CSR register and RTS as a GPIO).
I just gave it a quick try, works the same.
( But I don't know if it will work with FIFOs

> What is the problematic setup? I guess it's RTS and CTS are gpios and
> with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that
> happens?

Yes, CTS/RTS as GPIOs + HWHS flag is clearly wrong.
If that happens, well, the controller will try to drive an RTS pin
that won't have been muxed
as RTS. It does not seems to be a problem, but instructing the
controller do drive pins it doesn't
have access doesn't really make sense.
And in the case of the SAMA5D2, well I don't know what the result will
be, but it could stop transmission
from the "ghost" CTS signal I guess.

Anyway, the problematics setups are all the setups with USMODE_HWHS
enabled on platform without Fifos or PDC,
i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).

For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and
RTS/CTS NOT muxed as GPIOS,
it's like there was no flow control at all (the CTS pin doesn't
disable the transmitter).

Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2,
(cfhttps://lkml.org/lkml/2016/9/7/598 ),  I honestly didn't dig any
further into that flag.


Regards,
Richard

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 11:12 GMT+02:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> Hello Richard,
>
> On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> were used.
>>
>> Hardware handshake with GPIOs used to work before this commit because
>> the CRTSCTS flag (termios->c_cflag) was set, but not the
>> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
>> enabled, but not handled by the controller.
>
> What does the HWHS flag control? What if only RTS is a gpio and CTS is
> not? Or the other way round?
First, HWHS flag is used only in SAMA5D2. (if I correctly understood
Atmel HW guys,
all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
is unusable,
because they don't have Fifos nor PDC).
So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
pin according to
the number of char present in the rx fifo (cf Figure 44-29 ?44.7.3.15 p.1438 of
 http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
The controller will also start/stop the transmission on CTS changes.
But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
With this flag set, It's mandatory to have CTS and RTS not handled via
GPIO, because if
they were, the controller couldn't, well, control them.

The NORMAL flag, on the contrary, just tell the controller not to mess
with RTS/CTS,
and in this case, the driver will handle CTS changes and drive the RTS
pin, via GPIO or
via the CR register.

It's not a problem to have CTS as a GPIO and RTS controlled via the CR register
(or CTS changes read in CSR register and RTS as a GPIO).
I just gave it a quick try, works the same.
( But I don't know if it will work with FIFOs

> What is the problematic setup? I guess it's RTS and CTS are gpios and
> with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that
> happens?

Yes, CTS/RTS as GPIOs + HWHS flag is clearly wrong.
If that happens, well, the controller will try to drive an RTS pin
that won't have been muxed
as RTS. It does not seems to be a problem, but instructing the
controller do drive pins it doesn't
have access doesn't really make sense.
And in the case of the SAMA5D2, well I don't know what the result will
be, but it could stop transmission
from the "ghost" CTS signal I guess.

Anyway, the problematics setups are all the setups with USMODE_HWHS
enabled on platform without Fifos or PDC,
i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).

For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and
RTS/CTS NOT muxed as GPIOS,
it's like there was no flow control at all (the CTS pin doesn't
disable the transmitter).

Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2,
(cfhttps://lkml.org/lkml/2016/9/7/598 ),  I honestly didn't dig any
further into that flag.


Regards,
Richard

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Alexandre Belloni]

On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
> Anyway, the problematics setups are all the setups with USMODE_HWHS
> enabled on platform without Fifos or PDC,
> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
> 

This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
Please, don't break those platforms.

The only affected platforms are sam9x5, sama5d3 and sama5d4.

> For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and
> RTS/CTS NOT muxed as GPIOS,
> it's like there was no flow control at all (the CTS pin doesn't
> disable the transmitter).
> 
> Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2,
> (cfhttps://lkml.org/lkml/2016/9/7/598 ),  I honestly didn't dig any
> further into that flag.
> 
> 
> Regards,
> Richard

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Alexandre Belloni]

On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
> Anyway, the problematics setups are all the setups with USMODE_HWHS
> enabled on platform without Fifos or PDC,
> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
> 

This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
Please, don't break those platforms.

The only affected platforms are sam9x5, sama5d3 and sama5d4.

> For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and
> RTS/CTS NOT muxed as GPIOS,
> it's like there was no flow control at all (the CTS pin doesn't
> disable the transmitter).
> 
> Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2,
> (cfhttps://lkml.org/lkml/2016/9/7/598 ),  I honestly didn't dig any
> further into that flag.
> 
> 
> Regards,
> Richard

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 13:16 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
>> Anyway, the problematics setups are all the setups with USMODE_HWHS
>> enabled on platform without Fifos or PDC,
>> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
>>
>
> This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
> Please, don't break those platforms.
>
> The only affected platforms are sam9x5, sama5d3 and sama5d4.
Have you tested them ?

And why are you saying that rm9200 and g45 will be broken with this patch ?
If they have a pdc, they will fall in the case:
(atmel_use_pdc_rx(port) || atmel_use_fifo(port))
and thus use:
mode |= ATMEL_US_USMODE_HWHS;

won't they ?

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 13:16 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
>> Anyway, the problematics setups are all the setups with USMODE_HWHS
>> enabled on platform without Fifos or PDC,
>> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
>>
>
> This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
> Please, don't break those platforms.
>
> The only affected platforms are sam9x5, sama5d3 and sama5d4.
Have you tested them ?

And why are you saying that rm9200 and g45 will be broken with this patch ?
If they have a pdc, they will fall in the case:
(atmel_use_pdc_rx(port) || atmel_use_fifo(port))
and thus use:
mode |= ATMEL_US_USMODE_HWHS;

won't they ?

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Alexandre Belloni]

On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote :
> 2016-09-30 13:16 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
> > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
> >> Anyway, the problematics setups are all the setups with USMODE_HWHS
> >> enabled on platform without Fifos or PDC,
> >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
> >>
> >
> > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
> > Please, don't break those platforms.
> >
> > The only affected platforms are sam9x5, sama5d3 and sama5d4.
> Have you tested them ?
> 
> And why are you saying that rm9200 and g45 will be broken with this patch ?
> If they have a pdc, they will fall in the case:
> (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
> and thus use:
> mode |= ATMEL_US_USMODE_HWHS;
> 
> won't they ?

Well, this patch wouldn't have my ack if it was breaking them. However,
I'm still not sure about 3/3.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Alexandre Belloni]

On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote :
> 2016-09-30 13:16 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
> > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
> >> Anyway, the problematics setups are all the setups with USMODE_HWHS
> >> enabled on platform without Fifos or PDC,
> >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
> >>
> >
> > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
> > Please, don't break those platforms.
> >
> > The only affected platforms are sam9x5, sama5d3 and sama5d4.
> Have you tested them ?
> 
> And why are you saying that rm9200 and g45 will be broken with this patch ?
> If they have a pdc, they will fall in the case:
> (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
> and thus use:
> mode |= ATMEL_US_USMODE_HWHS;
> 
> won't they ?

Well, this patch wouldn't have my ack if it was breaking them. However,
I'm still not sure about 3/3.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 13:54 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote :
>> 2016-09-30 13:16 GMT+02:00 Alexandre Belloni
>> <alexandre.belloni@free-electrons.com>:
>> > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
>> >> Anyway, the problematics setups are all the setups with USMODE_HWHS
>> >> enabled on platform without Fifos or PDC,
>> >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
>> >>
>> >
>> > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
>> > Please, don't break those platforms.
>> >
>> > The only affected platforms are sam9x5, sama5d3 and sama5d4.
>> Have you tested them ?
>>
>> And why are you saying that rm9200 and g45 will be broken with this patch ?
>> If they have a pdc, they will fall in the case:
>> (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>> and thus use:
>> mode |= ATMEL_US_USMODE_HWHS;
>>
>> won't they ?
>
> Well, this patch wouldn't have my ack if it was breaking them.
It's not.
Read the code.

>However, I'm still not sure about 3/3.
As I said 2 weeks from now ( https://lkml.org/lkml/2016/9/14/263 ) the
only case where
we would want to drop the CRTSCTS flag is when there's no pin muxed
for CTS/RTS nor GPIOs.
Fell free to give a patch for that.

To be clear : this patch (2/3) unbreaks ALL platforms with GPIOs as CTS/RTS
and 3/3 unbreaks sam9x5 / sama5d3 / sama5d4 platform with CTS/RTS as !GPIOs.

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

2016-09-30 13:54 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote :
>> 2016-09-30 13:16 GMT+02:00 Alexandre Belloni
>> <alexandre.belloni@free-electrons.com>:
>> > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote :
>> >> Anyway, the problematics setups are all the setups with USMODE_HWHS
>> >> enabled on platform without Fifos or PDC,
>> >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong).
>> >>
>> >
>> > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc.
>> > Please, don't break those platforms.
>> >
>> > The only affected platforms are sam9x5, sama5d3 and sama5d4.
>> Have you tested them ?
>>
>> And why are you saying that rm9200 and g45 will be broken with this patch ?
>> If they have a pdc, they will fall in the case:
>> (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>> and thus use:
>> mode |= ATMEL_US_USMODE_HWHS;
>>
>> won't they ?
>
> Well, this patch wouldn't have my ack if it was breaking them.
It's not.
Read the code.

>However, I'm still not sure about 3/3.
As I said 2 weeks from now ( https://lkml.org/lkml/2016/9/14/263 ) the
only case where
we would want to drop the CRTSCTS flag is when there's no pin muxed
for CTS/RTS nor GPIOs.
Fell free to give a patch for that.

To be clear : this patch (2/3) unbreaks ALL platforms with GPIOs as CTS/RTS
and 3/3 unbreaks sam9x5 / sama5d3 / sama5d4 platform with CTS/RTS as !GPIOs.

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Uwe Kleine-König]

On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Richard,
> >
> > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> >> hardware handshake is enabled") broke the hardware handshake when GPIOs
> >> were used.
> >>
> >> Hardware handshake with GPIOs used to work before this commit because
> >> the CRTSCTS flag (termios->c_cflag) was set, but not the
> >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> >> enabled, but not handled by the controller.
> >
> > What does the HWHS flag control? What if only RTS is a gpio and CTS is
> > not? Or the other way round?
> First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> Atmel HW guys,
> all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
> is unusable,
> because they don't have Fifos nor PDC).
> So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> pin according to
> the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of
>  http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> The controller will also start/stop the transmission on CTS changes.
> But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
> With this flag set, It's mandatory to have CTS and RTS not handled via
> GPIO, because if
> they were, the controller couldn't, well, control them.

Assuming the respective pin doesn't reach the hardware both are no
problem though. And it would keep the driver simpler to just ignore
this. There would be no need for patch 1 and also this patch could be
dropped. So I guess there is really something to fix, otherwise you
wouldn't start patching the driver. But I don't understand the issue, so
I'd like to have a better picture.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Uwe Kleine-König]

On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Richard,
> >
> > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> >> hardware handshake is enabled") broke the hardware handshake when GPIOs
> >> were used.
> >>
> >> Hardware handshake with GPIOs used to work before this commit because
> >> the CRTSCTS flag (termios->c_cflag) was set, but not the
> >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> >> enabled, but not handled by the controller.
> >
> > What does the HWHS flag control? What if only RTS is a gpio and CTS is
> > not? Or the other way round?
> First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> Atmel HW guys,
> all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
> is unusable,
> because they don't have Fifos nor PDC).
> So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> pin according to
> the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of
>  http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> The controller will also start/stop the transmission on CTS changes.
> But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
> With this flag set, It's mandatory to have CTS and RTS not handled via
> GPIO, because if
> they were, the controller couldn't, well, control them.

Assuming the respective pin doesn't reach the hardware both are no
problem though. And it would keep the driver simpler to just ignore
this. There would be no need for patch 1 and also this patch could be
dropped. So I guess there is really something to fix, otherwise you
wouldn't start patching the driver. But I don't understand the issue, so
I'd like to have a better picture.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Uwe Kleine-König]

On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> 2016-09-30 11:12 GMT+02:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > Hello Richard,
> >
> > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> >> hardware handshake is enabled") broke the hardware handshake when GPIOs
> >> were used.
> >>
> >> Hardware handshake with GPIOs used to work before this commit because
> >> the CRTSCTS flag (termios->c_cflag) was set, but not the
> >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware handshake
> >> enabled, but not handled by the controller.
> >
> > What does the HWHS flag control? What if only RTS is a gpio and CTS is
> > not? Or the other way round?
> First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> Atmel HW guys,
> all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it
> is unusable,
> because they don't have Fifos nor PDC).
> So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> pin according to
> the number of char present in the rx fifo (cf Figure 44-29 ?44.7.3.15 p.1438 of
>  http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> The controller will also start/stop the transmission on CTS changes.
> But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess).
> With this flag set, It's mandatory to have CTS and RTS not handled via
> GPIO, because if
> they were, the controller couldn't, well, control them.

Assuming the respective pin doesn't reach the hardware both are no
problem though. And it would keep the driver simpler to just ignore
this. There would be no need for patch 1 and also this patch could be
dropped. So I guess there is really something to fix, otherwise you
wouldn't start patching the driver. But I don't understand the issue, so
I'd like to have a better picture.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCHv4 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts

[Uwe Kleine-König]

Hello Greg,

On Fri, Sep 30, 2016 at 10:57:59AM +0200, Richard Genoud wrote:
> This function returns true if CTS and RTS are used as GPIOs.
> Some drivers (like atmel_serial) needs to know if the flow control is
> handled by the controller or by GPIOs.

just for the record: I don't like this patch because I think it's highly
at91 specific and could so well live in that driver. Moreover I'm not
conviced yet that it's really the correct thing to do even for this
driver. So please don't apply, at least until we're done with the
discussion of patch 2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCHv4 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts

[Uwe Kleine-König]

Hello Greg,

On Fri, Sep 30, 2016 at 10:57:59AM +0200, Richard Genoud wrote:
> This function returns true if CTS and RTS are used as GPIOs.
> Some drivers (like atmel_serial) needs to know if the flow control is
> handled by the controller or by GPIOs.

just for the record: I don't like this patch because I think it's highly
at91 specific and could so well live in that driver. Moreover I'm not
conviced yet that it's really the correct thing to do even for this
driver. So please don't apply, at least until we're done with the
discussion of patch 2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

Le Tue, 4 Oct 2016 09:25:25 +0200,
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> a écrit :

> On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> > 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de>:
> > > Hello Richard,
> > >
> > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> > >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management
> > >> when hardware handshake is enabled") broke the hardware
> > >> handshake when GPIOs were used.
> > >>
> > >> Hardware handshake with GPIOs used to work before this commit
> > >> because the CRTSCTS flag (termios->c_cflag) was set, but not the
> > >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware
> > >> handshake enabled, but not handled by the controller.
> > >
> > > What does the HWHS flag control? What if only RTS is a gpio and
> > > CTS is not? Or the other way round?
> > First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> > Atmel HW guys,
> > all other platforms (sam9, sam9x5, sama5d3...) have this flag, but
> > it is unusable,
> > because they don't have Fifos nor PDC).
> > So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> > pin according to
> > the number of char present in the rx fifo (cf Figure 44-29
> > §44.7.3.15 p.1438 of
> > http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> > The controller will also start/stop the transmission on CTS
> > changes. But, as I haven't got this hard, I couldn't test it. (but
> > Cyrille did I guess). With this flag set, It's mandatory to have
> > CTS and RTS not handled via GPIO, because if they were, the
> > controller couldn't, well, control them.
> 
> Assuming the respective pin doesn't reach the hardware both are no
> problem though. And it would keep the driver simpler to just ignore
> this. There would be no need for patch 1 and also this patch could be
> dropped. So I guess there is really something to fix, otherwise you
> wouldn't start patching the driver. But I don't understand the issue,
> so I'd like to have a better picture.
> 
> Best regards
> Uwe
> 
Ok, so I'll try to explain what's going on.

I'm sure you're familiar with hardware handshaking (aka flow control),
but anyway, it doesn't hurt to explain it for the big picture
understanding.

On RS232, flow control is handled by 2 pins: RTS and CTS.
CTS is an input. When it's high (ttl), it indicates that we should stop
transmitting.
RTS is an output, we drive it at high level (ttl) to indicate that we
can't receive anymore data for now (because the rx buffer is full, or
the device is closed for instance).

The RTS and CTS management is done in serial_core.c which calls
atmel_set/get_mctrl to set/get RTS/CTS pin state.
And CTS changes are detected via an interrupt in atmel_interrupt().

Now, the atmel USART controller has a feature that can handle part of
the flow control job:
- disabling 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).
This feature is enabled by setting the flag ATMEL_US_USMODE_HWHS.
And to be clear, this feature is *not* mandatory for the flow control to
work !

[ The fun part is that according to atmel designers, this feature is
broken for platforms with no PDC and no FIFO.
(source: https://lkml.org/lkml/2016/9/7/598 ) ]

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 (among other
things).
The logic introduced was to set the flag ATMEL_US_USMODE_HWHS for all
boards when the user space enables flow control.
This was clearly wrong because this feature can only be used when PDC
or FIFO are in use.

If ATMEL_US_USMODE_HWHS is enabled and PDC/FIFO/DMA are not used, the
RTS pin stays at high level no matter what, preventing any data to be
received.

Then, commit 5be605ac9af9 ("tty/serial: atmel: fix hardware handshake
selection") did something completely wrong:
Instead of removing the flag ATMEL_US_USMODE_HWHS for platforms with
no PDC nor FIFOs, and let serial_core happily handle the CTS/RTS, it
refused to set the flow control when the user space asked for it.
This was because of a misunderstanding:
It's not that platforms without PDC and FIFO can't do flow control at
all, they just can't enable the USART controller feature relative to
flag ATMEL_US_USMODE_HWHS.
And, again, this feature is *not* mandatory for the flow control to
work.

So, now, all atmel platforms with no PDC and no FIFO (SAM9G35, SAMA5D3,
etc.) can't use flow control anymore.


That's the reason of this patch series.


And as the controller flow control feature only works with PDC and
FIFOs, the logic would be to set the ATMEL_US_USMODE_HWHS like that:
if ((termios->c_cflag & CRTSCTS) &&
    !mctrl_gpio_use_rtscts(atmel_port->gpios) &&
    (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) {
    mode |= ATMEL_US_USMODE_HWHS;
}

I added the mctrl_gpio_use_rtscts() test, because:
- There's no point using the ATMEL_US_USMODE_HWHS feature when CTS/RTS
  are driven by GPIOs, it doesn't make any sense.
- And it simply doesn't work because the transmitter is shut down.
(and that's the other reason of this patch series)

For the mctrl_gpio_use_rtscts() function, I agree that it may be atmel
specific, and I could implement as something like
atmel_use_{c,r}ts_gpio().

I'll have soon other hardware to test this on, and I'll resend a series
after that.

regards,
Richard

Re: [PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

Le Tue, 4 Oct 2016 09:25:25 +0200,
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> a écrit :

> On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> > 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de>:
> > > Hello Richard,
> > >
> > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> > >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management
> > >> when hardware handshake is enabled") broke the hardware
> > >> handshake when GPIOs were used.
> > >>
> > >> Hardware handshake with GPIOs used to work before this commit
> > >> because the CRTSCTS flag (termios->c_cflag) was set, but not the
> > >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware
> > >> handshake enabled, but not handled by the controller.
> > >
> > > What does the HWHS flag control? What if only RTS is a gpio and
> > > CTS is not? Or the other way round?
> > First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> > Atmel HW guys,
> > all other platforms (sam9, sam9x5, sama5d3...) have this flag, but
> > it is unusable,
> > because they don't have Fifos nor PDC).
> > So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> > pin according to
> > the number of char present in the rx fifo (cf Figure 44-29
> > §44.7.3.15 p.1438 of
> > http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> > The controller will also start/stop the transmission on CTS
> > changes. But, as I haven't got this hard, I couldn't test it. (but
> > Cyrille did I guess). With this flag set, It's mandatory to have
> > CTS and RTS not handled via GPIO, because if they were, the
> > controller couldn't, well, control them.
> 
> Assuming the respective pin doesn't reach the hardware both are no
> problem though. And it would keep the driver simpler to just ignore
> this. There would be no need for patch 1 and also this patch could be
> dropped. So I guess there is really something to fix, otherwise you
> wouldn't start patching the driver. But I don't understand the issue,
> so I'd like to have a better picture.
> 
> Best regards
> Uwe
> 
Ok, so I'll try to explain what's going on.

I'm sure you're familiar with hardware handshaking (aka flow control),
but anyway, it doesn't hurt to explain it for the big picture
understanding.

On RS232, flow control is handled by 2 pins: RTS and CTS.
CTS is an input. When it's high (ttl), it indicates that we should stop
transmitting.
RTS is an output, we drive it at high level (ttl) to indicate that we
can't receive anymore data for now (because the rx buffer is full, or
the device is closed for instance).

The RTS and CTS management is done in serial_core.c which calls
atmel_set/get_mctrl to set/get RTS/CTS pin state.
And CTS changes are detected via an interrupt in atmel_interrupt().

Now, the atmel USART controller has a feature that can handle part of
the flow control job:
- disabling 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).
This feature is enabled by setting the flag ATMEL_US_USMODE_HWHS.
And to be clear, this feature is *not* mandatory for the flow control to
work !

[ The fun part is that according to atmel designers, this feature is
broken for platforms with no PDC and no FIFO.
(source: https://lkml.org/lkml/2016/9/7/598 ) ]

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 (among other
things).
The logic introduced was to set the flag ATMEL_US_USMODE_HWHS for all
boards when the user space enables flow control.
This was clearly wrong because this feature can only be used when PDC
or FIFO are in use.

If ATMEL_US_USMODE_HWHS is enabled and PDC/FIFO/DMA are not used, the
RTS pin stays at high level no matter what, preventing any data to be
received.

Then, commit 5be605ac9af9 ("tty/serial: atmel: fix hardware handshake
selection") did something completely wrong:
Instead of removing the flag ATMEL_US_USMODE_HWHS for platforms with
no PDC nor FIFOs, and let serial_core happily handle the CTS/RTS, it
refused to set the flow control when the user space asked for it.
This was because of a misunderstanding:
It's not that platforms without PDC and FIFO can't do flow control at
all, they just can't enable the USART controller feature relative to
flag ATMEL_US_USMODE_HWHS.
And, again, this feature is *not* mandatory for the flow control to
work.

So, now, all atmel platforms with no PDC and no FIFO (SAM9G35, SAMA5D3,
etc.) can't use flow control anymore.


That's the reason of this patch series.


And as the controller flow control feature only works with PDC and
FIFOs, the logic would be to set the ATMEL_US_USMODE_HWHS like that:
if ((termios->c_cflag & CRTSCTS) &&
    !mctrl_gpio_use_rtscts(atmel_port->gpios) &&
    (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) {
    mode |= ATMEL_US_USMODE_HWHS;
}

I added the mctrl_gpio_use_rtscts() test, because:
- There's no point using the ATMEL_US_USMODE_HWHS feature when CTS/RTS
  are driven by GPIOs, it doesn't make any sense.
- And it simply doesn't work because the transmitter is shut down.
(and that's the other reason of this patch series)

For the mctrl_gpio_use_rtscts() function, I agree that it may be atmel
specific, and I could implement as something like
atmel_use_{c,r}ts_gpio().

I'll have soon other hardware to test this on, and I'll resend a series
after that.

regards,
Richard

[PATCHv4 2/3] tty/serial: at91: fix hardware handshake with GPIOs

[Richard Genoud]

Le Tue, 4 Oct 2016 09:25:25 +0200,
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> a ?crit :

> On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote:
> > 2016-09-30 11:12 GMT+02:00 Uwe Kleine-K?nig
> > <u.kleine-koenig@pengutronix.de>:
> > > Hello Richard,
> > >
> > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote:
> > >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management
> > >> when hardware handshake is enabled") broke the hardware
> > >> handshake when GPIOs were used.
> > >>
> > >> Hardware handshake with GPIOs used to work before this commit
> > >> because the CRTSCTS flag (termios->c_cflag) was set, but not the
> > >> ATMEL_US_USMODE_HWHS flag (controller register) ; so hardware
> > >> handshake enabled, but not handled by the controller.
> > >
> > > What does the HWHS flag control? What if only RTS is a gpio and
> > > CTS is not? Or the other way round?
> > First, HWHS flag is used only in SAMA5D2. (if I correctly understood
> > Atmel HW guys,
> > all other platforms (sam9, sam9x5, sama5d3...) have this flag, but
> > it is unusable,
> > because they don't have Fifos nor PDC).
> > So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS
> > pin according to
> > the number of char present in the rx fifo (cf Figure 44-29
> > ?44.7.3.15 p.1438 of
> > http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf).
> > The controller will also start/stop the transmission on CTS
> > changes. But, as I haven't got this hard, I couldn't test it. (but
> > Cyrille did I guess). With this flag set, It's mandatory to have
> > CTS and RTS not handled via GPIO, because if they were, the
> > controller couldn't, well, control them.
> 
> Assuming the respective pin doesn't reach the hardware both are no
> problem though. And it would keep the driver simpler to just ignore
> this. There would be no need for patch 1 and also this patch could be
> dropped. So I guess there is really something to fix, otherwise you
> wouldn't start patching the driver. But I don't understand the issue,
> so I'd like to have a better picture.
> 
> Best regards
> Uwe
> 
Ok, so I'll try to explain what's going on.

I'm sure you're familiar with hardware handshaking (aka flow control),
but anyway, it doesn't hurt to explain it for the big picture
understanding.

On RS232, flow control is handled by 2 pins: RTS and CTS.
CTS is an input. When it's high (ttl), it indicates that we should stop
transmitting.
RTS is an output, we drive it at high level (ttl) to indicate that we
can't receive anymore data for now (because the rx buffer is full, or
the device is closed for instance).

The RTS and CTS management is done in serial_core.c which calls
atmel_set/get_mctrl to set/get RTS/CTS pin state.
And CTS changes are detected via an interrupt in atmel_interrupt().

Now, the atmel USART controller has a feature that can handle part of
the flow control job:
- disabling 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).
This feature is enabled by setting the flag ATMEL_US_USMODE_HWHS.
And to be clear, this feature is *not* mandatory for the flow control to
work !

[ The fun part is that according to atmel designers, this feature is
broken for platforms with no PDC and no FIFO.
(source: https://lkml.org/lkml/2016/9/7/598 ) ]

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 (among other
things).
The logic introduced was to set the flag ATMEL_US_USMODE_HWHS for all
boards when the user space enables flow control.
This was clearly wrong because this feature can only be used when PDC
or FIFO are in use.

If ATMEL_US_USMODE_HWHS is enabled and PDC/FIFO/DMA are not used, the
RTS pin stays at high level no matter what, preventing any data to be
received.

Then, commit 5be605ac9af9 ("tty/serial: atmel: fix hardware handshake
selection") did something completely wrong:
Instead of removing the flag ATMEL_US_USMODE_HWHS for platforms with
no PDC nor FIFOs, and let serial_core happily handle the CTS/RTS, it
refused to set the flow control when the user space asked for it.
This was because of a misunderstanding:
It's not that platforms without PDC and FIFO can't do flow control at
all, they just can't enable the USART controller feature relative to
flag ATMEL_US_USMODE_HWHS.
And, again, this feature is *not* mandatory for the flow control to
work.

So, now, all atmel platforms with no PDC and no FIFO (SAM9G35, SAMA5D3,
etc.) can't use flow control anymore.


That's the reason of this patch series.


And as the controller flow control feature only works with PDC and
FIFOs, the logic would be to set the ATMEL_US_USMODE_HWHS like that:
if ((termios->c_cflag & CRTSCTS) &&
    !mctrl_gpio_use_rtscts(atmel_port->gpios) &&
    (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) {
    mode |= ATMEL_US_USMODE_HWHS;
}

I added the mctrl_gpio_use_rtscts() test, because:
- There's no point using the ATMEL_US_USMODE_HWHS feature when CTS/RTS
  are driven by GPIOs, it doesn't make any sense.
- And it simply doesn't work because the transmitter is shut down.
(and that's the other reason of this patch series)

For the mctrl_gpio_use_rtscts() function, I agree that it may be atmel
specific, and I could implement as something like
atmel_use_{c,r}ts_gpio().

I'll have soon other hardware to test this on, and I'll resend a series
after that.

regards,
Richard