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

[PATCH 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 | 9 +++++++++
 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..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..5d423b7f1e1c 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,10 @@ static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
 {
 }
 
+static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+}
+
 #endif /* GPIOLIB */
 
 #endif

[PATCH 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 | 9 +++++++++
 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..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..5d423b7f1e1c 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,10 @@ static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
 {
 }
 
+static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
+{
+}
+
 #endif /* GPIOLIB */
 
 #endif

[PATCH 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>
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;
 	}
 

[PATCH 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>
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;
 	}
 

[PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Richard Genoud]

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

So, here's a summary:
If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
controller can't drive the RTS pin (there's no way for it to know when
we can't receive data anymore).

If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
because when there's a FIFO, it can know when it's full and then drive
the RTS pin accordingly.
NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
FIFO mechanism.

If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
can't control the RTS pin, and we can't control it either by hand. So,
the hardware handshake is disabled. (that's what commit 5be605ac9af9
("tty/serial: atmel: fix hardware handshake selection") did).

There's still something obscure to me:
In the SAM9G35 datasheet, it's said that
"Setting the USART to operate with hardware handshaking is performed by
writing the USART_MODE field in US_MR to the value 0x2.
[..] Using this mode requires using the DMA channel for reception."

So, according to this, it *should* be possible to use automatic hardware
handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
samad controllers)

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

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, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e9b4fbf88c2d..503fbc623227 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,14 +2130,26 @@ 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
+		 * DMA enabled and FIFOs.
+		 * TODO: sam9x5 controllers don't have FIFOs like samad
+		 * controllers, and yet, the datasheet says that the
+		 * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
+		 * channel for reception.
 		 */
-		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;
+		if (atmel_use_dma_rx(port)) {
+			if (atmel_use_fifo(port)) {
+				mode |= ATMEL_US_USMODE_HWHS;
+			} else {
+				dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
+				termios->c_cflag &= ~CRTSCTS;
+			}
 		} else {
-			mode |= ATMEL_US_USMODE_HWHS;
+			/*
+			 * if DMA is not used, tell the controller that it
+			 * should not handle the RTS pin automatically
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 		}
 	} else {
 		/* RS232 without hadware handshake or controlled by GPIOs */

[PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Richard Genoud]

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

So, here's a summary:
If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
controller can't drive the RTS pin (there's no way for it to know when
we can't receive data anymore).

If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
because when there's a FIFO, it can know when it's full and then drive
the RTS pin accordingly.
NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
FIFO mechanism.

If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
can't control the RTS pin, and we can't control it either by hand. So,
the hardware handshake is disabled. (that's what commit 5be605ac9af9
("tty/serial: atmel: fix hardware handshake selection") did).

There's still something obscure to me:
In the SAM9G35 datasheet, it's said that
"Setting the USART to operate with hardware handshaking is performed by
writing the USART_MODE field in US_MR to the value 0x2.
[..] Using this mode requires using the DMA channel for reception."

So, according to this, it *should* be possible to use automatic hardware
handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
samad controllers)

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

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, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e9b4fbf88c2d..503fbc623227 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2130,14 +2130,26 @@ 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
+		 * DMA enabled and FIFOs.
+		 * TODO: sam9x5 controllers don't have FIFOs like samad
+		 * controllers, and yet, the datasheet says that the
+		 * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
+		 * channel for reception.
 		 */
-		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;
+		if (atmel_use_dma_rx(port)) {
+			if (atmel_use_fifo(port)) {
+				mode |= ATMEL_US_USMODE_HWHS;
+			} else {
+				dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
+				termios->c_cflag &= ~CRTSCTS;
+			}
 		} else {
-			mode |= ATMEL_US_USMODE_HWHS;
+			/*
+			 * if DMA is not used, tell the controller that it
+			 * should not handle the RTS pin automatically
+			 */
+			mode |= ATMEL_US_USMODE_NORMAL;
 		}
 	} else {
 		/* RS232 without hadware handshake or controlled by GPIOs */

Re: [PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Alexandre Belloni]

Hi,

On 07/09/2016 at 18:13:24 +0200, Richard Genoud wrote :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
> 
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
> 

You forgot that the PDC can properly drive the RTS pin so all the
platforms that are using the PDC will break after your patch.

Also, I believe the controller is able to drive the RTS and CTS pins
when simply using PIOs (it knows when one complete character has been
received and US_RHR has not yet been read). However, I didn't test.
Maybe someone at Atmel can confirm.

I think that 5be605ac9af9 is doing the right thing but I still can be
convince otherwise ;).

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

[PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Alexandre Belloni]

Hi,

On 07/09/2016 at 18:13:24 +0200, Richard Genoud wrote :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
> 
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
> 

You forgot that the PDC can properly drive the RTS pin so all the
platforms that are using the PDC will break after your patch.

Also, I believe the controller is able to drive the RTS and CTS pins
when simply using PIOs (it knows when one complete character has been
received and US_RHR has not yet been read). However, I didn't test.
Maybe someone at Atmel can confirm.

I think that 5be605ac9af9 is doing the right thing but I still can be
convince otherwise ;).

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

Re: [PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Cyrille Pitchen]

Hi Richard,

For usart without FIFOs (hence before sama5d2), according to our designers, the
RTS line could only been controlled by an internal PDC signal which doesn't
exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
DMA controllers. So if you enable the hardware handshaking feature on some
usart, its RTS line won't be monitored at all. The hardware handshaking is
broken on all SoCs using DMA controllers instead of PDC.

With the sama5d2, our latest MPU, we fixed this issue by introducing an
alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
FIFOs are available, those thresholds can be used to control the level of RTS
line.

Indeed I think a better test should be:
if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
	dev_info(port->dev, "not enabling hardware flow control because neither the PDC nor the FIFO are available");
	termios->c_cflags &= ~CRTSCTS;
}

We can double check once again with our designers to confirm that without FIFO
the only way for the hardware handshaking to work is to use a PDC.
So IMHO, your patch should not be applied, sorry!

Best regards,

Cyrille

Le 07/09/2016 à 18:13, Richard Genoud a écrit :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
> 
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
> 
> If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
> because when there's a FIFO, it can know when it's full and then drive
> the RTS pin accordingly.
> NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
> FIFO mechanism.
> 
> If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
> can't control the RTS pin, and we can't control it either by hand. So,
> the hardware handshake is disabled. (that's what commit 5be605ac9af9
> ("tty/serial: atmel: fix hardware handshake selection") did).
> 
> There's still something obscure to me:
> In the SAM9G35 datasheet, it's said that
> "Setting the USART to operate with hardware handshaking is performed by
> writing the USART_MODE field in US_MR to the value 0x2.
> [..] Using this mode requires using the DMA channel for reception."
> 
> So, according to this, it *should* be possible to use automatic hardware
> handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
> samad controllers)
> 
> NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
> 
> 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, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..503fbc623227 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,14 +2130,26 @@ 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
> +		 * DMA enabled and FIFOs.
> +		 * TODO: sam9x5 controllers don't have FIFOs like samad
> +		 * controllers, and yet, the datasheet says that the
> +		 * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
> +		 * channel for reception.
>  		 */
> -		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;
> +		if (atmel_use_dma_rx(port)) {
> +			if (atmel_use_fifo(port)) {
> +				mode |= ATMEL_US_USMODE_HWHS;
> +			} else {
> +				dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
> +				termios->c_cflag &= ~CRTSCTS;
> +			}
>  		} else {
> -			mode |= ATMEL_US_USMODE_HWHS;
> +			/*
> +			 * if DMA is not used, tell the controller that it
> +			 * should not handle the RTS pin automatically
> +			 */
> +			mode |= ATMEL_US_USMODE_NORMAL;
>  		}
>  	} else {
>  		/* RS232 without hadware handshake or controlled by GPIOs */
> 

Re: [PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Cyrille Pitchen]

Hi Richard,

For usart without FIFOs (hence before sama5d2), according to our designers, the
RTS line could only been controlled by an internal PDC signal which doesn't
exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
DMA controllers. So if you enable the hardware handshaking feature on some
usart, its RTS line won't be monitored at all. The hardware handshaking is
broken on all SoCs using DMA controllers instead of PDC.

With the sama5d2, our latest MPU, we fixed this issue by introducing an
alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
FIFOs are available, those thresholds can be used to control the level of RTS
line.

Indeed I think a better test should be:
if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
	dev_info(port->dev, "not enabling hardware flow control because neither the PDC nor the FIFO are available");
	termios->c_cflags &= ~CRTSCTS;
}

We can double check once again with our designers to confirm that without FIFO
the only way for the hardware handshaking to work is to use a PDC.
So IMHO, your patch should not be applied, sorry!

Best regards,

Cyrille

Le 07/09/2016 à 18:13, Richard Genoud a écrit :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
> 
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
> 
> If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
> because when there's a FIFO, it can know when it's full and then drive
> the RTS pin accordingly.
> NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
> FIFO mechanism.
> 
> If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
> can't control the RTS pin, and we can't control it either by hand. So,
> the hardware handshake is disabled. (that's what commit 5be605ac9af9
> ("tty/serial: atmel: fix hardware handshake selection") did).
> 
> There's still something obscure to me:
> In the SAM9G35 datasheet, it's said that
> "Setting the USART to operate with hardware handshaking is performed by
> writing the USART_MODE field in US_MR to the value 0x2.
> [..] Using this mode requires using the DMA channel for reception."
> 
> So, according to this, it *should* be possible to use automatic hardware
> handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
> samad controllers)
> 
> NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
> 
> 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, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..503fbc623227 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,14 +2130,26 @@ 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
> +		 * DMA enabled and FIFOs.
> +		 * TODO: sam9x5 controllers don't have FIFOs like samad
> +		 * controllers, and yet, the datasheet says that the
> +		 * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
> +		 * channel for reception.
>  		 */
> -		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;
> +		if (atmel_use_dma_rx(port)) {
> +			if (atmel_use_fifo(port)) {
> +				mode |= ATMEL_US_USMODE_HWHS;
> +			} else {
> +				dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
> +				termios->c_cflag &= ~CRTSCTS;
> +			}
>  		} else {
> -			mode |= ATMEL_US_USMODE_HWHS;
> +			/*
> +			 * if DMA is not used, tell the controller that it
> +			 * should not handle the RTS pin automatically
> +			 */
> +			mode |= ATMEL_US_USMODE_NORMAL;
>  		}
>  	} else {
>  		/* RS232 without hadware handshake or controlled by GPIOs */
> 

[PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Cyrille Pitchen]

Hi Richard,

For usart without FIFOs (hence before sama5d2), according to our designers, the
RTS line could only been controlled by an internal PDC signal which doesn't
exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
DMA controllers. So if you enable the hardware handshaking feature on some
usart, its RTS line won't be monitored at all. The hardware handshaking is
broken on all SoCs using DMA controllers instead of PDC.

With the sama5d2, our latest MPU, we fixed this issue by introducing an
alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
FIFOs are available, those thresholds can be used to control the level of RTS
line.

Indeed I think a better test should be:
if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
	dev_info(port->dev, "not enabling hardware flow control because neither the PDC nor the FIFO are available");
	termios->c_cflags &= ~CRTSCTS;
}

We can double check once again with our designers to confirm that without FIFO
the only way for the hardware handshaking to work is to use a PDC.
So IMHO, your patch should not be applied, sorry!

Best regards,

Cyrille

Le 07/09/2016 ? 18:13, Richard Genoud a ?crit :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
> 
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
> 
> If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
> because when there's a FIFO, it can know when it's full and then drive
> the RTS pin accordingly.
> NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
> FIFO mechanism.
> 
> If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
> can't control the RTS pin, and we can't control it either by hand. So,
> the hardware handshake is disabled. (that's what commit 5be605ac9af9
> ("tty/serial: atmel: fix hardware handshake selection") did).
> 
> There's still something obscure to me:
> In the SAM9G35 datasheet, it's said that
> "Setting the USART to operate with hardware handshaking is performed by
> writing the USART_MODE field in US_MR to the value 0x2.
> [..] Using this mode requires using the DMA channel for reception."
> 
> So, according to this, it *should* be possible to use automatic hardware
> handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
> samad controllers)
> 
> NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
> 
> 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, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..503fbc623227 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,14 +2130,26 @@ 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
> +		 * DMA enabled and FIFOs.
> +		 * TODO: sam9x5 controllers don't have FIFOs like samad
> +		 * controllers, and yet, the datasheet says that the
> +		 * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
> +		 * channel for reception.
>  		 */
> -		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;
> +		if (atmel_use_dma_rx(port)) {
> +			if (atmel_use_fifo(port)) {
> +				mode |= ATMEL_US_USMODE_HWHS;
> +			} else {
> +				dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
> +				termios->c_cflag &= ~CRTSCTS;
> +			}
>  		} else {
> -			mode |= ATMEL_US_USMODE_HWHS;
> +			/*
> +			 * if DMA is not used, tell the controller that it
> +			 * should not handle the RTS pin automatically
> +			 */
> +			mode |= ATMEL_US_USMODE_NORMAL;
>  		}
>  	} else {
>  		/* RS232 without hadware handshake or controlled by GPIOs */
> 

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

[Alexandre Belloni]

On 07/09/2016 at 18:13:23 +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>
> Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

> ---
>  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;
>  	}
>  

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

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

[Alexandre Belloni]

On 07/09/2016 at 18:13:23 +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>
> Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

> ---
>  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;
>  	}
>  

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

Hi Richard,

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v4.8-rc5 next-20160907]
[cannot apply to tty/tty-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Richard-Genoud/serial-mctrl_gpio-implement-mctrl_gpio_use_rtscts/20160908-002703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   In file included from drivers/tty/serial/sh-sci.c:60:0:
   drivers/tty/serial/serial_mctrl_gpio.h: In function 'mctrl_gpio_use_rtscts':
>> drivers/tty/serial/serial_mctrl_gpio.h:162:1: warning: no return statement in function returning non-void [-Wreturn-type]
    }
    ^

vim +162 drivers/tty/serial/serial_mctrl_gpio.h

   146	
   147	static inline
   148	void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
   149	{
   150	}
   151	
   152	static inline void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
   153	{
   154	}
   155	
   156	static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
   157	{
   158	}
   159	
   160	static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
   161	{
 > 162	}
   163	
   164	#endif /* GPIOLIB */
   165	
   166	#endif

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 15819 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

Hi Richard,

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v4.8-rc5 next-20160907]
[cannot apply to tty/tty-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Richard-Genoud/serial-mctrl_gpio-implement-mctrl_gpio_use_rtscts/20160908-002703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   In file included from drivers/tty/serial/sh-sci.c:60:0:
   drivers/tty/serial/serial_mctrl_gpio.h: In function 'mctrl_gpio_use_rtscts':
>> drivers/tty/serial/serial_mctrl_gpio.h:162:1: warning: no return statement in function returning non-void [-Wreturn-type]
    }
    ^

vim +162 drivers/tty/serial/serial_mctrl_gpio.h

   146	
   147	static inline
   148	void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
   149	{
   150	}
   151	
   152	static inline void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
   153	{
   154	}
   155	
   156	static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
   157	{
   158	}
   159	
   160	static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
   161	{
 > 162	}
   163	
   164	#endif /* GPIOLIB */
   165	
   166	#endif

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 15819 bytes --]

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

[kbuild test robot]

Hi Richard,

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v4.8-rc5 next-20160907]
[cannot apply to tty/tty-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Richard-Genoud/serial-mctrl_gpio-implement-mctrl_gpio_use_rtscts/20160908-002703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   In file included from drivers/tty/serial/sh-sci.c:60:0:
   drivers/tty/serial/serial_mctrl_gpio.h: In function 'mctrl_gpio_use_rtscts':
>> drivers/tty/serial/serial_mctrl_gpio.h:162:1: warning: no return statement in function returning non-void [-Wreturn-type]
    }
    ^

vim +162 drivers/tty/serial/serial_mctrl_gpio.h

   146	
   147	static inline
   148	void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
   149	{
   150	}
   151	
   152	static inline void mctrl_gpio_enable_ms(struct mctrl_gpios *gpios)
   153	{
   154	}
   155	
   156	static inline void mctrl_gpio_disable_ms(struct mctrl_gpios *gpios)
   157	{
   158	}
   159	
   160	static inline bool mctrl_gpio_use_rtscts(struct mctrl_gpios *gpios)
   161	{
 > 162	}
   163	
   164	#endif /* GPIOLIB */
   165	
   166	#endif

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 15819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160908/0ef0b669/attachment-0001.obj>

Re: [PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Richard Genoud]

Hi Alex, Cyrille,


I did some tests to clear this HW handcheck history out.
NB: all those tests are with a 4.8-rc5 kernel on a AT91SAM9G35-CM board.

I couldn't test the PDC since, as Cyrille said, there's no such thing on
SAM9G35 USARTs.

The modes that DON'T WORK are:
ATMEL_US_USMODE_HWHS + DMA (RTS won't go up when the data are not read anymore)
ATMEL_US_USMODE_HWHS + PIO (RTS won't even go down when the port is open)

The modes that WORK are:
ATMEL_US_USMODE_NORMAL + DMA (no error, perfect transfer)
ATMEL_US_USMODE_NORMAL + PIO (some chars are eaten or nullified)


So, it seems that Atmel HW's guys where right when they said that the
HW handshake is completely broken on SAM9x5 (updating the manual with a
note saying that ATMEL_US_USMODE_HWHS shouldn't be use would be a good
idea !).

2016-09-07 18:59 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> Hi,
>
> You forgot that the PDC can properly drive the RTS pin so all the
> platforms that are using the PDC will break after your patch.
You're right, I missed the platforms with PDC.
But right know, all SAM9x5 platforms are broken (and I guess SAM92xx
also), so I guess we'll find a way to correct that.

> Also, I believe the controller is able to drive the RTS and CTS pins
> when simply using PIOs (it knows when one complete character has been
> received and US_RHR has not yet been read). However, I didn't test.
> Maybe someone at Atmel can confirm.
I did see anything like that in SAM9G35 datasheet, and the tests I've done show
the contrary.
Actually, in SAM9G35 datasheet:
"When a character reception is completed, it is transferred to the
Receive Holding register (US_RHR) and the RXRDY bit in US_CSR rises.
If a character is completed while the RXRDY is set, the OVRE
(Overrun Error) bit is set.
The last character is transferred into US_RHR and overwrites the previous one."

> I think that 5be605ac9af9 is doing the right thing but I still can be
> convince otherwise ;).
I'll get to that.

2016-09-07 19:28 GMT+02:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>:
> Hi Richard,
>
> For usart without FIFOs (hence before sama5d2), according to our designers, the
> RTS line could only been controlled by an internal PDC signal which doesn't
> exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
> DMA controllers. So if you enable the hardware handshaking feature on some
> usart, its RTS line won't be monitored at all. The hardware handshaking is
> broken on all SoCs using DMA controllers instead of PDC.
>
> With the sama5d2, our latest MPU, we fixed this issue by introducing an
> alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
> FIFOs are available, those thresholds can be used to control the level of RTS
> line.
>
> Indeed I think a better test should be:
> if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
>         dev_info(port->dev, "not enabling hardware flow control
>         because neither the PDC nor the FIFO are available");
>         termios->c_cflags &= ~CRTSCTS;
> }
Maybe that's ok for SAMAD2+ platform, but not for older ones.
Actually, from all the tests I've done, the only mode that works for
SAM9x5 is:
mode |= ATMEL_US_USMODE_NORMAL;
and the CRTSCTS flag should *NOT* be removed.

So, IHMO, test should be:

if (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) {
    mode |= ATMEL_US_USMODE_HWHS;
} else {
    mode |= ATMEL_US_USMODE_NORMAL;
}

Because nothing prevents the driver to drive the RTS pin itself, as it
is done in atmel_set_mctrl().
So, IHMO, commit 5be605ac9af9 is all wrong.
It basically says "If the controller can't handle RTS/CTS, then we don't
do HW handshake."
NO ! If the controller can't do it, the driver can do it for him, can't it ?!
(Well, from here, it truly seems it can !)


If you want to grab an atmel board and a scope to do some testing,
here's what I've done:

On one side:
stty -F /dev/ttyS2 115200 crtscts -opost clocal cread
exec 3>/dev/ttyS2
dd if=/home/rgenoud/dev/linux/MAINTAINERS bs=1M count=1 > /dev/ttyS2
exec 3>&-

On the board:
stty -F /dev/ttyS1 115200 crtscts -opost clocal cread
exec 4</dev/ttyS1
rm -f /tmp/rcv
cat <&4 >> /tmp/rcv
# then ctrl-C, and "cat <&4 >> /tmp/rcv" again
# at the ctrl-C, the RTS pin should go up

To enable dma in sam9x5 dts, I added
atmel,use-dma-rx;
in usart0: serial@f801c000


>
> We can double check once again with our designers to confirm that without FIFO
> the only way for the hardware handshaking to work is to use a PDC.
> So IMHO, your patch should not be applied, sorry!
>
> Best regards,
>
> Cyrille
>

Best regards.
Richard.

Re: [PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Richard Genoud]

Hi Alex, Cyrille,


I did some tests to clear this HW handcheck history out.
NB: all those tests are with a 4.8-rc5 kernel on a AT91SAM9G35-CM board.

I couldn't test the PDC since, as Cyrille said, there's no such thing on
SAM9G35 USARTs.

The modes that DON'T WORK are:
ATMEL_US_USMODE_HWHS + DMA (RTS won't go up when the data are not read anymore)
ATMEL_US_USMODE_HWHS + PIO (RTS won't even go down when the port is open)

The modes that WORK are:
ATMEL_US_USMODE_NORMAL + DMA (no error, perfect transfer)
ATMEL_US_USMODE_NORMAL + PIO (some chars are eaten or nullified)


So, it seems that Atmel HW's guys where right when they said that the
HW handshake is completely broken on SAM9x5 (updating the manual with a
note saying that ATMEL_US_USMODE_HWHS shouldn't be use would be a good
idea !).

2016-09-07 18:59 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> Hi,
>
> You forgot that the PDC can properly drive the RTS pin so all the
> platforms that are using the PDC will break after your patch.
You're right, I missed the platforms with PDC.
But right know, all SAM9x5 platforms are broken (and I guess SAM92xx
also), so I guess we'll find a way to correct that.

> Also, I believe the controller is able to drive the RTS and CTS pins
> when simply using PIOs (it knows when one complete character has been
> received and US_RHR has not yet been read). However, I didn't test.
> Maybe someone at Atmel can confirm.
I did see anything like that in SAM9G35 datasheet, and the tests I've done show
the contrary.
Actually, in SAM9G35 datasheet:
"When a character reception is completed, it is transferred to the
Receive Holding register (US_RHR) and the RXRDY bit in US_CSR rises.
If a character is completed while the RXRDY is set, the OVRE
(Overrun Error) bit is set.
The last character is transferred into US_RHR and overwrites the previous one."

> I think that 5be605ac9af9 is doing the right thing but I still can be
> convince otherwise ;).
I'll get to that.

2016-09-07 19:28 GMT+02:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>:
> Hi Richard,
>
> For usart without FIFOs (hence before sama5d2), according to our designers, the
> RTS line could only been controlled by an internal PDC signal which doesn't
> exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
> DMA controllers. So if you enable the hardware handshaking feature on some
> usart, its RTS line won't be monitored at all. The hardware handshaking is
> broken on all SoCs using DMA controllers instead of PDC.
>
> With the sama5d2, our latest MPU, we fixed this issue by introducing an
> alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
> FIFOs are available, those thresholds can be used to control the level of RTS
> line.
>
> Indeed I think a better test should be:
> if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
>         dev_info(port->dev, "not enabling hardware flow control
>         because neither the PDC nor the FIFO are available");
>         termios->c_cflags &= ~CRTSCTS;
> }
Maybe that's ok for SAMAD2+ platform, but not for older ones.
Actually, from all the tests I've done, the only mode that works for
SAM9x5 is:
mode |= ATMEL_US_USMODE_NORMAL;
and the CRTSCTS flag should *NOT* be removed.

So, IHMO, test should be:

if (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) {
    mode |= ATMEL_US_USMODE_HWHS;
} else {
    mode |= ATMEL_US_USMODE_NORMAL;
}

Because nothing prevents the driver to drive the RTS pin itself, as it
is done in atmel_set_mctrl().
So, IHMO, commit 5be605ac9af9 is all wrong.
It basically says "If the controller can't handle RTS/CTS, then we don't
do HW handshake."
NO ! If the controller can't do it, the driver can do it for him, can't it ?!
(Well, from here, it truly seems it can !)


If you want to grab an atmel board and a scope to do some testing,
here's what I've done:

On one side:
stty -F /dev/ttyS2 115200 crtscts -opost clocal cread
exec 3>/dev/ttyS2
dd if=/home/rgenoud/dev/linux/MAINTAINERS bs=1M count=1 > /dev/ttyS2
exec 3>&-

On the board:
stty -F /dev/ttyS1 115200 crtscts -opost clocal cread
exec 4</dev/ttyS1
rm -f /tmp/rcv
cat <&4 >> /tmp/rcv
# then ctrl-C, and "cat <&4 >> /tmp/rcv" again
# at the ctrl-C, the RTS pin should go up

To enable dma in sam9x5 dts, I added
atmel,use-dma-rx;
in usart0: serial@f801c000


>
> We can double check once again with our designers to confirm that without FIFO
> the only way for the hardware handshaking to work is to use a PDC.
> So IMHO, your patch should not be applied, sorry!
>
> Best regards,
>
> Cyrille
>

Best regards.
Richard.

[PATCH 3/3] tty/serial: at91: fix hardware handshake when DMA is not used

[Richard Genoud]

Hi Alex, Cyrille,


I did some tests to clear this HW handcheck history out.
NB: all those tests are with a 4.8-rc5 kernel on a AT91SAM9G35-CM board.

I couldn't test the PDC since, as Cyrille said, there's no such thing on
SAM9G35 USARTs.

The modes that DON'T WORK are:
ATMEL_US_USMODE_HWHS + DMA (RTS won't go up when the data are not read anymore)
ATMEL_US_USMODE_HWHS + PIO (RTS won't even go down when the port is open)

The modes that WORK are:
ATMEL_US_USMODE_NORMAL + DMA (no error, perfect transfer)
ATMEL_US_USMODE_NORMAL + PIO (some chars are eaten or nullified)


So, it seems that Atmel HW's guys where right when they said that the
HW handshake is completely broken on SAM9x5 (updating the manual with a
note saying that ATMEL_US_USMODE_HWHS shouldn't be use would be a good
idea !).

2016-09-07 18:59 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> Hi,
>
> You forgot that the PDC can properly drive the RTS pin so all the
> platforms that are using the PDC will break after your patch.
You're right, I missed the platforms with PDC.
But right know, all SAM9x5 platforms are broken (and I guess SAM92xx
also), so I guess we'll find a way to correct that.

> Also, I believe the controller is able to drive the RTS and CTS pins
> when simply using PIOs (it knows when one complete character has been
> received and US_RHR has not yet been read). However, I didn't test.
> Maybe someone at Atmel can confirm.
I did see anything like that in SAM9G35 datasheet, and the tests I've done show
the contrary.
Actually, in SAM9G35 datasheet:
"When a character reception is completed, it is transferred to the
Receive Holding register (US_RHR) and the RXRDY bit in US_CSR rises.
If a character is completed while the RXRDY is set, the OVRE
(Overrun Error) bit is set.
The last character is transferred into US_RHR and overwrites the previous one."

> I think that 5be605ac9af9 is doing the right thing but I still can be
> convince otherwise ;).
I'll get to that.

2016-09-07 19:28 GMT+02:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>:
> Hi Richard,
>
> For usart without FIFOs (hence before sama5d2), according to our designers, the
> RTS line could only been controlled by an internal PDC signal which doesn't
> exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
> DMA controllers. So if you enable the hardware handshaking feature on some
> usart, its RTS line won't be monitored at all. The hardware handshaking is
> broken on all SoCs using DMA controllers instead of PDC.
>
> With the sama5d2, our latest MPU, we fixed this issue by introducing an
> alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
> FIFOs are available, those thresholds can be used to control the level of RTS
> line.
>
> Indeed I think a better test should be:
> if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
>         dev_info(port->dev, "not enabling hardware flow control
>         because neither the PDC nor the FIFO are available");
>         termios->c_cflags &= ~CRTSCTS;
> }
Maybe that's ok for SAMAD2+ platform, but not for older ones.
Actually, from all the tests I've done, the only mode that works for
SAM9x5 is:
mode |= ATMEL_US_USMODE_NORMAL;
and the CRTSCTS flag should *NOT* be removed.

So, IHMO, test should be:

if (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) {
    mode |= ATMEL_US_USMODE_HWHS;
} else {
    mode |= ATMEL_US_USMODE_NORMAL;
}

Because nothing prevents the driver to drive the RTS pin itself, as it
is done in atmel_set_mctrl().
So, IHMO, commit 5be605ac9af9 is all wrong.
It basically says "If the controller can't handle RTS/CTS, then we don't
do HW handshake."
NO ! If the controller can't do it, the driver can do it for him, can't it ?!
(Well, from here, it truly seems it can !)


If you want to grab an atmel board and a scope to do some testing,
here's what I've done:

On one side:
stty -F /dev/ttyS2 115200 crtscts -opost clocal cread
exec 3>/dev/ttyS2
dd if=/home/rgenoud/dev/linux/MAINTAINERS bs=1M count=1 > /dev/ttyS2
exec 3>&-

On the board:
stty -F /dev/ttyS1 115200 crtscts -opost clocal cread
exec 4</dev/ttyS1
rm -f /tmp/rcv
cat <&4 >> /tmp/rcv
# then ctrl-C, and "cat <&4 >> /tmp/rcv" again
# at the ctrl-C, the RTS pin should go up

To enable dma in sam9x5 dts, I added
atmel,use-dma-rx;
in usart0: serial at f801c000


>
> We can double check once again with our designers to confirm that without FIFO
> the only way for the hardware handshaking to work is to use a PDC.
> So IMHO, your patch should not be applied, sorry!
>
> Best regards,
>
> Cyrille
>

Best regards.
Richard.