[PATCHv2 0/3] Fix handware handshake on SAM9x5 platforms

[PATCHv2 0/3] Fix handware 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 platforms.

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

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

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 when DMA is not used

 drivers/tty/serial/atmel_serial.c      | 25 +++++++++++++++++--------
 drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 3 files changed, 35 insertions(+), 8 deletions(-)

[PATCHv2 0/3] Fix handware 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 platforms.

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

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

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 when DMA is not used

 drivers/tty/serial/atmel_serial.c      | 25 +++++++++++++++++--------
 drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 3 files changed, 35 insertions(+), 8 deletions(-)

[PATCHv2 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 |  8 ++++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index d2da6aa7f27d..93bed8c99796 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/irq.h>
+#include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/termios.h>
 #include <linux/serial_core.h>
@@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
+		!IS_ERR_OR_NULL(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

[PATCHv2 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 |  8 ++++++++
 drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
index d2da6aa7f27d..93bed8c99796 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/irq.h>
+#include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/termios.h>
 #include <linux/serial_core.h>
@@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
+		!IS_ERR_OR_NULL(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

[PATCHv2 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
where 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2eaa18ddef61..e9b4fbf88c2d 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 			      struct ktermios *old)
 {
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	unsigned long flags;
 	unsigned int old_mode, mode, imr, quot, baud;
 
@@ -2126,8 +2127,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;
@@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

[PATCHv2 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
where 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 2eaa18ddef61..e9b4fbf88c2d 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 			      struct ktermios *old)
 {
+	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	unsigned long flags;
 	unsigned int old_mode, mode, imr, quot, baud;
 
@@ -2126,8 +2127,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;
@@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;
 	}
 

[PATCHv2 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e9b4fbf88c2d..32154e7231ce 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	} else if ((termios->c_cflag & CRTSCTS) &&
 		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
 		/*
-		 * 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 {
+		if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
 			mode |= ATMEL_US_USMODE_HWHS;
-		}
+		else
+			/*
+			 * The hardware handshake won't be handle by the
+			 * controller but by the driver.
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 	} else {
 		/* RS232 without hadware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;

[PATCHv2 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e9b4fbf88c2d..32154e7231ce 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	} else if ((termios->c_cflag & CRTSCTS) &&
 		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
 		/*
-		 * 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 {
+		if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
 			mode |= ATMEL_US_USMODE_HWHS;
-		}
+		else
+			/*
+			 * The hardware handshake won't be handle by the
+			 * controller but by the driver.
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 	} else {
 		/* RS232 without hadware handshake or controlled by GPIOs */
 		mode |= ATMEL_US_USMODE_NORMAL;

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

[Alexandre Belloni]

On 12/09/2016 at 11:47:33 +0200, Richard Genoud wrote :
> 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..32154e7231ce 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  	} else if ((termios->c_cflag & CRTSCTS) &&
>  		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>  		/*
> -		 * 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 {
> +		if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>  			mode |= ATMEL_US_USMODE_HWHS;
> -		}
> +		else
> +			/*
> +			 * The hardware handshake won't be handle by the
> +			 * controller but by the driver.
> +			 */
> +			mode |= ATMEL_US_USMODE_NORMAL;

You still need the case where HWHS is impossible and there are no gpio
configured. You need to inform userspace that the configuration was not
applied instead of silently ignoring the error.

>  	} else {
>  		/* RS232 without hadware handshake or controlled by GPIOs */
>  		mode |= ATMEL_US_USMODE_NORMAL;

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

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

[Alexandre Belloni]

On 12/09/2016 at 11:47:33 +0200, Richard Genoud wrote :
> 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..32154e7231ce 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  	} else if ((termios->c_cflag & CRTSCTS) &&
>  		   !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>  		/*
> -		 * 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 {
> +		if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>  			mode |= ATMEL_US_USMODE_HWHS;
> -		}
> +		else
> +			/*
> +			 * The hardware handshake won't be handle by the
> +			 * controller but by the driver.
> +			 */
> +			mode |= ATMEL_US_USMODE_NORMAL;

You still need the case where HWHS is impossible and there are no gpio
configured. You need to inform userspace that the configuration was not
applied instead of silently ignoring the error.

>  	} else {
>  		/* RS232 without hadware handshake or controlled by GPIOs */
>  		mode |= ATMEL_US_USMODE_NORMAL;

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

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

[Richard Genoud]

2016-09-12 11:53 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 11:47:33 +0200, Richard Genoud wrote :
>> 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 | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index e9b4fbf88c2d..32154e7231ce 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>       } else if ((termios->c_cflag & CRTSCTS) &&
>>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>>               /*
>> -              * 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 {
>> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>>                       mode |= ATMEL_US_USMODE_HWHS;
>> -             }
>> +             else
>> +                     /*
>> +                      * The hardware handshake won't be handle by the
>> +                      * controller but by the driver.
>> +                      */
>> +                     mode |= ATMEL_US_USMODE_NORMAL;
>
> You still need the case where HWHS is impossible and there are no gpio
> configured. You need to inform userspace that the configuration was not
> applied instead of silently ignoring the error.

Could you explain which case it is ?
The only one I can see is when there's no GPIO declared for RTS/CTS
AND, there's no pin muxed for RTS/CTS either.

Have you got another example in mind ?


>
>>       } else {
>>               /* RS232 without hadware handshake or controlled by GPIOs */
>>               mode |= ATMEL_US_USMODE_NORMAL;
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

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

[Richard Genoud]

2016-09-12 11:53 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 11:47:33 +0200, Richard Genoud wrote :
>> 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 | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index e9b4fbf88c2d..32154e7231ce 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>       } else if ((termios->c_cflag & CRTSCTS) &&
>>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>>               /*
>> -              * 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 {
>> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>>                       mode |= ATMEL_US_USMODE_HWHS;
>> -             }
>> +             else
>> +                     /*
>> +                      * The hardware handshake won't be handle by the
>> +                      * controller but by the driver.
>> +                      */
>> +                     mode |= ATMEL_US_USMODE_NORMAL;
>
> You still need the case where HWHS is impossible and there are no gpio
> configured. You need to inform userspace that the configuration was not
> applied instead of silently ignoring the error.

Could you explain which case it is ?
The only one I can see is when there's no GPIO declared for RTS/CTS
AND, there's no pin muxed for RTS/CTS either.

Have you got another example in mind ?


>
>>       } else {
>>               /* RS232 without hadware handshake or controlled by GPIOs */
>>               mode |= ATMEL_US_USMODE_NORMAL;
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

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

[Richard Genoud]

2016-09-12 11:53 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 11:47:33 +0200, Richard Genoud wrote :
>> 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 | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index e9b4fbf88c2d..32154e7231ce 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>       } else if ((termios->c_cflag & CRTSCTS) &&
>>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>>               /*
>> -              * 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 {
>> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>>                       mode |= ATMEL_US_USMODE_HWHS;
>> -             }
>> +             else
>> +                     /*
>> +                      * The hardware handshake won't be handle by the
>> +                      * controller but by the driver.
>> +                      */
>> +                     mode |= ATMEL_US_USMODE_NORMAL;
>
> You still need the case where HWHS is impossible and there are no gpio
> configured. You need to inform userspace that the configuration was not
> applied instead of silently ignoring the error.

Could you explain which case it is ?
The only one I can see is when there's no GPIO declared for RTS/CTS
AND, there's no pin muxed for RTS/CTS either.

Have you got another example in mind ?


>
>>       } else {
>>               /* RS232 without hadware handshake or controlled by GPIOs */
>>               mode |= ATMEL_US_USMODE_NORMAL;
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

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

[Alexandre Belloni]

On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> >> index e9b4fbf88c2d..32154e7231ce 100644
> >> --- a/drivers/tty/serial/atmel_serial.c
> >> +++ b/drivers/tty/serial/atmel_serial.c
> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> >>       } else if ((termios->c_cflag & CRTSCTS) &&
> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
> >>               /*
> >> -              * 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 {
> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
> >>                       mode |= ATMEL_US_USMODE_HWHS;
> >> -             }
> >> +             else
> >> +                     /*
> >> +                      * The hardware handshake won't be handle by the
> >> +                      * controller but by the driver.
> >> +                      */
> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
> >
> > You still need the case where HWHS is impossible and there are no gpio
> > configured. You need to inform userspace that the configuration was not
> > applied instead of silently ignoring the error.
> 
> Could you explain which case it is ?
> The only one I can see is when there's no GPIO declared for RTS/CTS
> AND, there's no pin muxed for RTS/CTS either.
> 
> Have you got another example in mind ?
> 

Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
handled in the else below. I think you need to keep the termios->c_cflag
&= ~CRTSCTS; here so that userspace knows configuring it failed.

> >
> >>       } else {
> >>               /* RS232 without hadware handshake or controlled by GPIOs */
> >>               mode |= ATMEL_US_USMODE_NORMAL;
> >

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

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

[Alexandre Belloni]

On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> >> index e9b4fbf88c2d..32154e7231ce 100644
> >> --- a/drivers/tty/serial/atmel_serial.c
> >> +++ b/drivers/tty/serial/atmel_serial.c
> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> >>       } else if ((termios->c_cflag & CRTSCTS) &&
> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
> >>               /*
> >> -              * 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 {
> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
> >>                       mode |= ATMEL_US_USMODE_HWHS;
> >> -             }
> >> +             else
> >> +                     /*
> >> +                      * The hardware handshake won't be handle by the
> >> +                      * controller but by the driver.
> >> +                      */
> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
> >
> > You still need the case where HWHS is impossible and there are no gpio
> > configured. You need to inform userspace that the configuration was not
> > applied instead of silently ignoring the error.
> 
> Could you explain which case it is ?
> The only one I can see is when there's no GPIO declared for RTS/CTS
> AND, there's no pin muxed for RTS/CTS either.
> 
> Have you got another example in mind ?
> 

Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
handled in the else below. I think you need to keep the termios->c_cflag
&= ~CRTSCTS; here so that userspace knows configuring it failed.

> >
> >>       } else {
> >>               /* RS232 without hadware handshake or controlled by GPIOs */
> >>               mode |= ATMEL_US_USMODE_NORMAL;
> >

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

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

[Alexandre Belloni]

On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> >> index e9b4fbf88c2d..32154e7231ce 100644
> >> --- a/drivers/tty/serial/atmel_serial.c
> >> +++ b/drivers/tty/serial/atmel_serial.c
> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> >>       } else if ((termios->c_cflag & CRTSCTS) &&
> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
> >>               /*
> >> -              * 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 {
> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
> >>                       mode |= ATMEL_US_USMODE_HWHS;
> >> -             }
> >> +             else
> >> +                     /*
> >> +                      * The hardware handshake won't be handle by the
> >> +                      * controller but by the driver.
> >> +                      */
> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
> >
> > You still need the case where HWHS is impossible and there are no gpio
> > configured. You need to inform userspace that the configuration was not
> > applied instead of silently ignoring the error.
> 
> Could you explain which case it is ?
> The only one I can see is when there's no GPIO declared for RTS/CTS
> AND, there's no pin muxed for RTS/CTS either.
> 
> Have you got another example in mind ?
> 

Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
handled in the else below. I think you need to keep the termios->c_cflag
&= ~CRTSCTS; here so that userspace knows configuring it failed.

> >
> >>       } else {
> >>               /* RS232 without hadware handshake or controlled by GPIOs */
> >>               mode |= ATMEL_US_USMODE_NORMAL;
> >

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

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

[Richard Genoud]

2016-09-13 15:45 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
>> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> >> index e9b4fbf88c2d..32154e7231ce 100644
>> >> --- a/drivers/tty/serial/atmel_serial.c
>> >> +++ b/drivers/tty/serial/atmel_serial.c
>> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> >>       } else if ((termios->c_cflag & CRTSCTS) &&
>> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>> >>               /*
>> >> -              * 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 {
>> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>> >>                       mode |= ATMEL_US_USMODE_HWHS;
>> >> -             }
>> >> +             else
>> >> +                     /*
>> >> +                      * The hardware handshake won't be handle by the
>> >> +                      * controller but by the driver.
>> >> +                      */
>> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
>> >
>> > You still need the case where HWHS is impossible and there are no gpio
>> > configured. You need to inform userspace that the configuration was not
>> > applied instead of silently ignoring the error.
>>
>> Could you explain which case it is ?
>> The only one I can see is when there's no GPIO declared for RTS/CTS
>> AND, there's no pin muxed for RTS/CTS either.
>>
>> Have you got another example in mind ?
>>
>
> Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
> handled in the else below. I think you need to keep the termios->c_cflag
> &= ~CRTSCTS; here so that userspace knows configuring it failed.

So, could you explain how you intend to detect that there's no pin
muxed for RTS/CTS ?
Because IHMO, that's not possible with what is described in the DTS.
To be sure that we really understand each other, I want you to tell me
how you intend to make the difference between:
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
AND
pinctrl-0 = <&pinctrl_usart1>;
in the atmel_serial driver ?

and then  your test would be:
if (rts_cts_not_muxed()) {
dev_info(port->dev, "hey ! there's no pin muxed for RTS/CTS, I won't
enable HWHS");
}


Meanwhile, I did quite some testing with a sama5d3 xplained I have.
Same results as the SAM9x5. (Unfortunately, I don't have a sama5d2,
the only ones with fifos).

Here are those results:
With RTS/CTS NOT handled by GPIOs (pinctrl-0 = <&pinctrl_usart1
&pinctrl_usart1_rts_cts>;) :
 - if USMODE==HW : Fails in every case. (DMA/PDC/PIO)
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

With RTS/CTS handled by GPIOs
(pinctrl-0 = <&pinctrl_usart1;rts-gpios = <&pioB 27
GPIO_ACTIVE_LOW>;cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;) :
 - if USMODE==HW : Nonsense
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

So, again, commit 5be605ac9af9 is wrong because we don't want to
remove the CRTSCTS flag if rts_cts are muxed !!!

If someone does some testing on a sama5d2, it would help, but from the
informations I have, I stick to this logic:
                /*
                 * 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_pdc_rx(port) || atmel_use_fifo(port))
                        mode |= ATMEL_US_USMODE_HWHS;
                else
                        /*
                         * The hardware handshake won't be handle by the
                         * controller but by the driver.
                         */
                        mode |= ATMEL_US_USMODE_NORMAL;



Test configuration on 3xplained :

DTS:
            usart1: serial@f0020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf0020000 0x100>;
                interrupts = <13 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx; // comment to switch to PIO
dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(5)>, <&dma0 2
(AT91_DMA_CFG_PER_ID(6) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";

                clocks = <&usart1_clk>;
                clock-names = "usart";
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
/*
//or
rts-gpios = <&pioB 27 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;
pinctrl-0 = <&pinctrl_usart1>;
*/
                status = "okay";
            };

            usart2: serial@f8020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf8020000 0x100>;
                interrupts = <14 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx;
dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(7)>, <&dma1 2
(AT91_DMA_CFG_PER_ID(8) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
rts-gpios = <&pioA 20 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioA 21 GPIO_ACTIVE_LOW>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_usart2>;
                clocks = <&usart2_clk>;
                clock-names = "usart";
                status = "okay";
            };


On board:

stty -F /dev/ttyS2 115200 raw -echo crtscts -opost clocal cread
stty -F /dev/ttyS3 115200 raw -echo crtscts -opost clocal cread

On one terminal :
cat MAINTAINERS > /dev/ttyS3

On another:
exec 4</dev/ttyS2
cat <&4 >> /tmp/rcv  # ctrl-c and again several times
exec 4<&-
diff -u MAINTAINERS /tmp/rcv

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

[Richard Genoud]

2016-09-13 15:45 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
>> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> >> index e9b4fbf88c2d..32154e7231ce 100644
>> >> --- a/drivers/tty/serial/atmel_serial.c
>> >> +++ b/drivers/tty/serial/atmel_serial.c
>> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> >>       } else if ((termios->c_cflag & CRTSCTS) &&
>> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>> >>               /*
>> >> -              * 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 {
>> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>> >>                       mode |= ATMEL_US_USMODE_HWHS;
>> >> -             }
>> >> +             else
>> >> +                     /*
>> >> +                      * The hardware handshake won't be handle by the
>> >> +                      * controller but by the driver.
>> >> +                      */
>> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
>> >
>> > You still need the case where HWHS is impossible and there are no gpio
>> > configured. You need to inform userspace that the configuration was not
>> > applied instead of silently ignoring the error.
>>
>> Could you explain which case it is ?
>> The only one I can see is when there's no GPIO declared for RTS/CTS
>> AND, there's no pin muxed for RTS/CTS either.
>>
>> Have you got another example in mind ?
>>
>
> Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
> handled in the else below. I think you need to keep the termios->c_cflag
> &= ~CRTSCTS; here so that userspace knows configuring it failed.

So, could you explain how you intend to detect that there's no pin
muxed for RTS/CTS ?
Because IHMO, that's not possible with what is described in the DTS.
To be sure that we really understand each other, I want you to tell me
how you intend to make the difference between:
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
AND
pinctrl-0 = <&pinctrl_usart1>;
in the atmel_serial driver ?

and then  your test would be:
if (rts_cts_not_muxed()) {
dev_info(port->dev, "hey ! there's no pin muxed for RTS/CTS, I won't
enable HWHS");
}


Meanwhile, I did quite some testing with a sama5d3 xplained I have.
Same results as the SAM9x5. (Unfortunately, I don't have a sama5d2,
the only ones with fifos).

Here are those results:
With RTS/CTS NOT handled by GPIOs (pinctrl-0 = <&pinctrl_usart1
&pinctrl_usart1_rts_cts>;) :
 - if USMODE==HW : Fails in every case. (DMA/PDC/PIO)
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

With RTS/CTS handled by GPIOs
(pinctrl-0 = <&pinctrl_usart1;rts-gpios = <&pioB 27
GPIO_ACTIVE_LOW>;cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;) :
 - if USMODE==HW : Nonsense
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

So, again, commit 5be605ac9af9 is wrong because we don't want to
remove the CRTSCTS flag if rts_cts are muxed !!!

If someone does some testing on a sama5d2, it would help, but from the
informations I have, I stick to this logic:
                /*
                 * 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_pdc_rx(port) || atmel_use_fifo(port))
                        mode |= ATMEL_US_USMODE_HWHS;
                else
                        /*
                         * The hardware handshake won't be handle by the
                         * controller but by the driver.
                         */
                        mode |= ATMEL_US_USMODE_NORMAL;



Test configuration on 3xplained :

DTS:
            usart1: serial@f0020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf0020000 0x100>;
                interrupts = <13 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx; // comment to switch to PIO
dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(5)>, <&dma0 2
(AT91_DMA_CFG_PER_ID(6) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";

                clocks = <&usart1_clk>;
                clock-names = "usart";
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
/*
//or
rts-gpios = <&pioB 27 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;
pinctrl-0 = <&pinctrl_usart1>;
*/
                status = "okay";
            };

            usart2: serial@f8020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf8020000 0x100>;
                interrupts = <14 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx;
dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(7)>, <&dma1 2
(AT91_DMA_CFG_PER_ID(8) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
rts-gpios = <&pioA 20 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioA 21 GPIO_ACTIVE_LOW>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_usart2>;
                clocks = <&usart2_clk>;
                clock-names = "usart";
                status = "okay";
            };


On board:

stty -F /dev/ttyS2 115200 raw -echo crtscts -opost clocal cread
stty -F /dev/ttyS3 115200 raw -echo crtscts -opost clocal cread

On one terminal :
cat MAINTAINERS > /dev/ttyS3

On another:
exec 4</dev/ttyS2
cat <&4 >> /tmp/rcv  # ctrl-c and again several times
exec 4<&-
diff -u MAINTAINERS /tmp/rcv

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

[Richard Genoud]

2016-09-13 15:45 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/09/2016 at 12:50:38 +0200, Richard Genoud wrote :
>> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> >> index e9b4fbf88c2d..32154e7231ce 100644
>> >> --- a/drivers/tty/serial/atmel_serial.c
>> >> +++ b/drivers/tty/serial/atmel_serial.c
>> >> @@ -2130,15 +2130,19 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> >>       } else if ((termios->c_cflag & CRTSCTS) &&
>> >>                  !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
>> >>               /*
>> >> -              * 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 {
>> >> +             if (atmel_use_pdc_rx(port) || atmel_use_fifo(port))
>> >>                       mode |= ATMEL_US_USMODE_HWHS;
>> >> -             }
>> >> +             else
>> >> +                     /*
>> >> +                      * The hardware handshake won't be handle by the
>> >> +                      * controller but by the driver.
>> >> +                      */
>> >> +                     mode |= ATMEL_US_USMODE_NORMAL;
>> >
>> > You still need the case where HWHS is impossible and there are no gpio
>> > configured. You need to inform userspace that the configuration was not
>> > applied instead of silently ignoring the error.
>>
>> Could you explain which case it is ?
>> The only one I can see is when there's no GPIO declared for RTS/CTS
>> AND, there's no pin muxed for RTS/CTS either.
>>
>> Have you got another example in mind ?
>>
>
> Hum, actually, it is that case. The other one (CRTSCTS and gpios) is
> handled in the else below. I think you need to keep the termios->c_cflag
> &= ~CRTSCTS; here so that userspace knows configuring it failed.

So, could you explain how you intend to detect that there's no pin
muxed for RTS/CTS ?
Because IHMO, that's not possible with what is described in the DTS.
To be sure that we really understand each other, I want you to tell me
how you intend to make the difference between:
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
AND
pinctrl-0 = <&pinctrl_usart1>;
in the atmel_serial driver ?

and then  your test would be:
if (rts_cts_not_muxed()) {
dev_info(port->dev, "hey ! there's no pin muxed for RTS/CTS, I won't
enable HWHS");
}


Meanwhile, I did quite some testing with a sama5d3 xplained I have.
Same results as the SAM9x5. (Unfortunately, I don't have a sama5d2,
the only ones with fifos).

Here are those results:
With RTS/CTS NOT handled by GPIOs (pinctrl-0 = <&pinctrl_usart1
&pinctrl_usart1_rts_cts>;) :
 - if USMODE==HW : Fails in every case. (DMA/PDC/PIO)
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

With RTS/CTS handled by GPIOs
(pinctrl-0 = <&pinctrl_usart1;rts-gpios = <&pioB 27
GPIO_ACTIVE_LOW>;cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;) :
 - if USMODE==HW : Nonsense
 - if USMODE==NORMAL + PDC : crask kernel (quite normal since there's no PDC)
 - if USMODE==NORMAL + DMA : works great, CTS and RTS respond OK
 - if USMODE==NORMAL + PIO : works fine, but some chars gets eaten.

So, again, commit 5be605ac9af9 is wrong because we don't want to
remove the CRTSCTS flag if rts_cts are muxed !!!

If someone does some testing on a sama5d2, it would help, but from the
informations I have, I stick to this logic:
                /*
                 * 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_pdc_rx(port) || atmel_use_fifo(port))
                        mode |= ATMEL_US_USMODE_HWHS;
                else
                        /*
                         * The hardware handshake won't be handle by the
                         * controller but by the driver.
                         */
                        mode |= ATMEL_US_USMODE_NORMAL;



Test configuration on 3xplained :

DTS:
            usart1: serial at f0020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf0020000 0x100>;
                interrupts = <13 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx; // comment to switch to PIO
dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(5)>, <&dma0 2
(AT91_DMA_CFG_PER_ID(6) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";

                clocks = <&usart1_clk>;
                clock-names = "usart";
pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
/*
//or
rts-gpios = <&pioB 27 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioB 26 GPIO_ACTIVE_LOW>;
pinctrl-0 = <&pinctrl_usart1>;
*/
                status = "okay";
            };

            usart2: serial at f8020000 {
                compatible = "atmel,at91sam9260-usart";
                reg = <0xf8020000 0x100>;
                interrupts = <14 IRQ_TYPE_LEVEL_HIGH 5>;
atmel,use-dma-rx;
dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(7)>, <&dma1 2
(AT91_DMA_CFG_PER_ID(8) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
rts-gpios = <&pioA 20 GPIO_ACTIVE_LOW>;
cts-gpios = <&pioA 21 GPIO_ACTIVE_LOW>;
                dma-names = "tx", "rx";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_usart2>;
                clocks = <&usart2_clk>;
                clock-names = "usart";
                status = "okay";
            };


On board:

stty -F /dev/ttyS2 115200 raw -echo crtscts -opost clocal cread
stty -F /dev/ttyS3 115200 raw -echo crtscts -opost clocal cread

On one terminal :
cat MAINTAINERS > /dev/ttyS3

On another:
exec 4</dev/ttyS2
cat <&4 >> /tmp/rcv  # ctrl-c and again several times
exec 4<&-
diff -u MAINTAINERS /tmp/rcv

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

[Greg Kroah-Hartman]

On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

This patch doesn't apply to my tree, are you sure it's up to date?

Can you please refresh this whole series against linux-next and resend?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

This patch doesn't apply to my tree, are you sure it's up to date?

Can you please refresh this whole series against linux-next and resend?

thanks,

greg k-h

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

[Uwe Kleine-König]

On Mon, Sep 12, 2016 at 11:47:31AM +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.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
>  drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index d2da6aa7f27d..93bed8c99796 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -17,6 +17,7 @@
>  #include <linux/err.h>
>  #include <linux/device.h>
>  #include <linux/irq.h>
> +#include <linux/err.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/termios.h>
>  #include <linux/serial_core.h>
> @@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
> +		!IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));

mctrl_gpio_to_gpiod cannot return an error pointer. So this is
(surprise!) a wrong usage of IS_ERR_OR_NULL.

Best regards
Uwe


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

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

[Uwe Kleine-König]

On Mon, Sep 12, 2016 at 11:47:31AM +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.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
>  drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> index d2da6aa7f27d..93bed8c99796 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -17,6 +17,7 @@
>  #include <linux/err.h>
>  #include <linux/device.h>
>  #include <linux/irq.h>
> +#include <linux/err.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/termios.h>
>  #include <linux/serial_core.h>
> @@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
> +		!IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));

mctrl_gpio_to_gpiod cannot return an error pointer. So this is
(surprise!) a wrong usage of IS_ERR_OR_NULL.

Best regards
Uwe


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

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

[Uwe Kleine-König]

On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where used.

s/where/were/

> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 2eaa18ddef61..e9b4fbf88c2d 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  			      struct ktermios *old)
>  {
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
>  	unsigned int old_mode, mode, imr, quot, baud;
>  
> @@ -2126,8 +2127,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)) {

IMHO the behaviour of the hw controlled pins shouldn't change when
mctrl_gpio is in use (if possible). But I don't understand the issue so
I guess you need a better changelog.


> +		/*
> +		 * 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;
> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */

When touching this line, please also do s/hadware/hardware/.

>  		mode |= ATMEL_US_USMODE_NORMAL;
>  	}

Best regards
Uwe

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

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

[Uwe Kleine-König]

On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where used.

s/where/were/

> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 2eaa18ddef61..e9b4fbf88c2d 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  			      struct ktermios *old)
>  {
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
>  	unsigned int old_mode, mode, imr, quot, baud;
>  
> @@ -2126,8 +2127,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)) {

IMHO the behaviour of the hw controlled pins shouldn't change when
mctrl_gpio is in use (if possible). But I don't understand the issue so
I guess you need a better changelog.


> +		/*
> +		 * 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;
> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */

When touching this line, please also do s/hadware/hardware/.

>  		mode |= ATMEL_US_USMODE_NORMAL;
>  	}

Best regards
Uwe

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

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

[Uwe Kleine-König]

On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when GPIOs
> where used.

s/where/were/

> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 2eaa18ddef61..e9b4fbf88c2d 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  			      struct ktermios *old)
>  {
> +	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
>  	unsigned int old_mode, mode, imr, quot, baud;
>  
> @@ -2126,8 +2127,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)) {

IMHO the behaviour of the hw controlled pins shouldn't change when
mctrl_gpio is in use (if possible). But I don't understand the issue so
I guess you need a better changelog.


> +		/*
> +		 * 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;
> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */

When touching this line, please also do s/hadware/hardware/.

>  		mode |= ATMEL_US_USMODE_NORMAL;
>  	}

Best regards
Uwe

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

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

[Richard Genoud]

2016-09-22 15:14 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:31AM +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.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
>>  drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>> index d2da6aa7f27d..93bed8c99796 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/err.h>
>>  #include <linux/device.h>
>>  #include <linux/irq.h>
>> +#include <linux/err.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/termios.h>
>>  #include <linux/serial_core.h>
>> @@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
>> +             !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));
>
> mctrl_gpio_to_gpiod cannot return an error pointer. So this is
> (surprise!) a wrong usage of IS_ERR_OR_NULL.

I see, your commit 1d267ea6539f266352f  ("serial: mctrl-gpio: simplify
init routine") removed the need of IS_ERR_OR_NULL().
I'll change that.

Thanks !

Richard.

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

[Richard Genoud]

2016-09-22 15:14 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:31AM +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.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
>>  drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>> index d2da6aa7f27d..93bed8c99796 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/err.h>
>>  #include <linux/device.h>
>>  #include <linux/irq.h>
>> +#include <linux/err.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/termios.h>
>>  #include <linux/serial_core.h>
>> @@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
>> +             !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));
>
> mctrl_gpio_to_gpiod cannot return an error pointer. So this is
> (surprise!) a wrong usage of IS_ERR_OR_NULL.

I see, your commit 1d267ea6539f266352f  ("serial: mctrl-gpio: simplify
init routine") removed the need of IS_ERR_OR_NULL().
I'll change that.

Thanks !

Richard.

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

[Richard Genoud]

2016-09-22 15:14 GMT+02:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:31AM +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.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/tty/serial/serial_mctrl_gpio.c |  8 ++++++++
>>  drivers/tty/serial/serial_mctrl_gpio.h | 10 ++++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>> index d2da6aa7f27d..93bed8c99796 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/err.h>
>>  #include <linux/device.h>
>>  #include <linux/irq.h>
>> +#include <linux/err.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/termios.h>
>>  #include <linux/serial_core.h>
>> @@ -72,6 +73,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 !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_CTS)) &&
>> +             !IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(gpios, UART_GPIO_RTS));
>
> mctrl_gpio_to_gpiod cannot return an error pointer. So this is
> (surprise!) a wrong usage of IS_ERR_OR_NULL.

I see, your commit 1d267ea6539f266352f  ("serial: mctrl-gpio: simplify
init routine") removed the need of IS_ERR_OR_NULL().
I'll change that.

Thanks !

Richard.

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

[Richard Genoud]

2016-09-22 15:20 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> where used.
>
> s/where/were/
ok.

>> 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 | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 2eaa18ddef61..e9b4fbf88c2d 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>                             struct ktermios *old)
>>  {
>> +     struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>       unsigned long flags;
>>       unsigned int old_mode, mode, imr, quot, baud;
>>
>> @@ -2126,8 +2127,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)) {
>
> IMHO the behaviour of the hw controlled pins shouldn't change when
> mctrl_gpio is in use (if possible). But I don't understand the issue so
> I guess you need a better changelog.
Yes, the behavior of the CTS/RTS pins should the same whenever they
are GPIOs or not.
That was the case before commit 1cf6e8fc8341 ("tty/serial: at91: fix
RTS line management when
hardware handshake is enabled")
BUT, this commit (1cf6e8fc834) introduced the actual use of the
ATMEL_US_USMODE_HWHS flag.
This flags changes the way RTS/CTS pins are driven, and that's the
discussion we are having in patch 3/3.

>> +             /*
>> +              * 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;
>> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */
>
> When touching this line, please also do s/hadware/hardware/.
ok.
>
>>               mode |= ATMEL_US_USMODE_NORMAL;
>>       }
>
> Best regards
> Uwe

Thanks.

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

[Richard Genoud]

2016-09-22 15:20 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> where used.
>
> s/where/were/
ok.

>> 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 | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 2eaa18ddef61..e9b4fbf88c2d 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>                             struct ktermios *old)
>>  {
>> +     struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>       unsigned long flags;
>>       unsigned int old_mode, mode, imr, quot, baud;
>>
>> @@ -2126,8 +2127,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)) {
>
> IMHO the behaviour of the hw controlled pins shouldn't change when
> mctrl_gpio is in use (if possible). But I don't understand the issue so
> I guess you need a better changelog.
Yes, the behavior of the CTS/RTS pins should the same whenever they
are GPIOs or not.
That was the case before commit 1cf6e8fc8341 ("tty/serial: at91: fix
RTS line management when
hardware handshake is enabled")
BUT, this commit (1cf6e8fc834) introduced the actual use of the
ATMEL_US_USMODE_HWHS flag.
This flags changes the way RTS/CTS pins are driven, and that's the
discussion we are having in patch 3/3.

>> +             /*
>> +              * 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;
>> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */
>
> When touching this line, please also do s/hadware/hardware/.
ok.
>
>>               mode |= ATMEL_US_USMODE_NORMAL;
>>       }
>
> Best regards
> Uwe

Thanks.

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

[Richard Genoud]

2016-09-22 15:20 GMT+02:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> On Mon, Sep 12, 2016 at 11:47:32AM +0200, Richard Genoud wrote:
>> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
>> hardware handshake is enabled") broke the hardware handshake when GPIOs
>> where used.
>
> s/where/were/
ok.

>> 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 | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 2eaa18ddef61..e9b4fbf88c2d 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2025,6 +2025,7 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>                             struct ktermios *old)
>>  {
>> +     struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>       unsigned long flags;
>>       unsigned int old_mode, mode, imr, quot, baud;
>>
>> @@ -2126,8 +2127,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)) {
>
> IMHO the behaviour of the hw controlled pins shouldn't change when
> mctrl_gpio is in use (if possible). But I don't understand the issue so
> I guess you need a better changelog.
Yes, the behavior of the CTS/RTS pins should the same whenever they
are GPIOs or not.
That was the case before commit 1cf6e8fc8341 ("tty/serial: at91: fix
RTS line management when
hardware handshake is enabled")
BUT, this commit (1cf6e8fc834) introduced the actual use of the
ATMEL_US_USMODE_HWHS flag.
This flags changes the way RTS/CTS pins are driven, and that's the
discussion we are having in patch 3/3.

>> +             /*
>> +              * 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;
>> @@ -2135,7 +2140,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 hadware handshake or controlled by GPIOs */
>
> When touching this line, please also do s/hadware/hardware/.
ok.
>
>>               mode |= ATMEL_US_USMODE_NORMAL;
>>       }
>
> Best regards
> Uwe

Thanks.