* [PATCHv2 0/3] Fix handware handshake on SAM9x5 platforms @ 2016-09-12 9:47 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman Cc: linux-arm-kernel, linux-serial, linux-kernel, 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(-) ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 0/3] Fix handware handshake on SAM9x5 platforms @ 2016-09-12 9:47 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: linux-arm-kernel 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(-) ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts 2016-09-12 9:47 ` Richard Genoud @ 2016-09-12 9:47 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman Cc: linux-arm-kernel, linux-serial, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts @ 2016-09-12 9:47 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts 2016-09-12 9:47 ` Richard Genoud @ 2016-09-22 13:14 ` Uwe Kleine-König -1 siblings, 0 replies; 32+ messages in thread From: Uwe Kleine-König @ 2016-09-22 13:14 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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/ | ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts @ 2016-09-22 13:14 ` Uwe Kleine-König 0 siblings, 0 replies; 32+ messages in thread From: Uwe Kleine-König @ 2016-09-22 13:14 UTC (permalink / raw) To: linux-arm-kernel 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/ | ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts 2016-09-22 13:14 ` Uwe Kleine-König (?) @ 2016-09-26 11:57 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-26 11:57 UTC (permalink / raw) To: Uwe Kleine-König Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts @ 2016-09-26 11:57 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-26 11:57 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts @ 2016-09-26 11:57 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-26 11:57 UTC (permalink / raw) To: Uwe Kleine-König Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs 2016-09-12 9:47 ` Richard Genoud @ 2016-09-12 9:47 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman Cc: linux-arm-kernel, linux-serial, linux-kernel, 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; } ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-12 9:47 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: linux-arm-kernel 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; } ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs 2016-09-12 9:47 ` Richard Genoud @ 2016-09-22 9:47 ` Greg Kroah-Hartman -1 siblings, 0 replies; 32+ messages in thread From: Greg Kroah-Hartman @ 2016-09-22 9:47 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-22 9:47 ` Greg Kroah-Hartman 0 siblings, 0 replies; 32+ messages in thread From: Greg Kroah-Hartman @ 2016-09-22 9:47 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs 2016-09-12 9:47 ` Richard Genoud (?) @ 2016-09-22 13:20 ` Uwe Kleine-König -1 siblings, 0 replies; 32+ messages in thread From: Uwe Kleine-König @ 2016-09-22 13:20 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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/ | ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-22 13:20 ` Uwe Kleine-König 0 siblings, 0 replies; 32+ messages in thread From: Uwe Kleine-König @ 2016-09-22 13:20 UTC (permalink / raw) To: linux-arm-kernel 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/ | ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-22 13:20 ` Uwe Kleine-König 0 siblings, 0 replies; 32+ messages in thread From: Uwe Kleine-König @ 2016-09-22 13:20 UTC (permalink / raw) To: Richard Genoud Cc: Greg Kroah-Hartman, Nicolas Ferre, linux-kernel, Alexandre Belloni, linux-serial, Cyrille Pitchen, linux-arm-kernel 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/ | ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs 2016-09-22 13:20 ` Uwe Kleine-König (?) @ 2016-09-27 7:45 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-27 7:45 UTC (permalink / raw) To: Uwe Kleine-König Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-27 7:45 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-27 7:45 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs @ 2016-09-27 7:45 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-27 7:45 UTC (permalink / raw) To: Uwe Kleine-König Cc: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) 2016-09-12 9:47 ` Richard Genoud @ 2016-09-12 9:47 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: Cyrille Pitchen, Alexandre Belloni, Nicolas Ferre, Greg Kroah-Hartman Cc: linux-arm-kernel, linux-serial, linux-kernel, 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; ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-12 9:47 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 9:47 UTC (permalink / raw) To: linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) 2016-09-12 9:47 ` Richard Genoud @ 2016-09-12 9:53 ` Alexandre Belloni -1 siblings, 0 replies; 32+ messages in thread From: Alexandre Belloni @ 2016-09-12 9:53 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-12 9:53 ` Alexandre Belloni 0 siblings, 0 replies; 32+ messages in thread From: Alexandre Belloni @ 2016-09-12 9:53 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) 2016-09-12 9:53 ` Alexandre Belloni (?) @ 2016-09-12 10:50 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 10:50 UTC (permalink / raw) To: Alexandre Belloni Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ? ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-12 10:50 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 10:50 UTC (permalink / raw) To: linux-arm-kernel 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 ? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-12 10:50 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-12 10:50 UTC (permalink / raw) To: Alexandre Belloni Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) 2016-09-12 10:50 ` Richard Genoud (?) @ 2016-09-13 13:45 ` Alexandre Belloni -1 siblings, 0 replies; 32+ messages in thread From: Alexandre Belloni @ 2016-09-13 13:45 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-13 13:45 ` Alexandre Belloni 0 siblings, 0 replies; 32+ messages in thread From: Alexandre Belloni @ 2016-09-13 13:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-13 13:45 ` Alexandre Belloni 0 siblings, 0 replies; 32+ messages in thread From: Alexandre Belloni @ 2016-09-13 13:45 UTC (permalink / raw) To: Richard Genoud Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) 2016-09-13 13:45 ` Alexandre Belloni (?) @ 2016-09-14 10:31 ` Richard Genoud -1 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-14 10:31 UTC (permalink / raw) To: Alexandre Belloni Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-14 10:31 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-14 10:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) @ 2016-09-14 10:31 ` Richard Genoud 0 siblings, 0 replies; 32+ messages in thread From: Richard Genoud @ 2016-09-14 10:31 UTC (permalink / raw) To: Alexandre Belloni Cc: Cyrille Pitchen, Nicolas Ferre, Greg Kroah-Hartman, linux-arm-kernel, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2016-09-27 7:46 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-12 9:47 [PATCHv2 0/3] Fix handware handshake on SAM9x5 platforms Richard Genoud 2016-09-12 9:47 ` Richard Genoud 2016-09-12 9:47 ` [PATCHv2 1/3] serial: mctrl_gpio: implement mctrl_gpio_use_rtscts Richard Genoud 2016-09-12 9:47 ` Richard Genoud 2016-09-22 13:14 ` Uwe Kleine-König 2016-09-22 13:14 ` Uwe Kleine-König 2016-09-26 11:57 ` Richard Genoud 2016-09-26 11:57 ` Richard Genoud 2016-09-26 11:57 ` Richard Genoud 2016-09-12 9:47 ` [PATCHv2 2/3] tty/serial: at91: fix hardware handshake with GPIOs Richard Genoud 2016-09-12 9:47 ` Richard Genoud 2016-09-22 9:47 ` Greg Kroah-Hartman 2016-09-22 9:47 ` Greg Kroah-Hartman 2016-09-22 13:20 ` Uwe Kleine-König 2016-09-22 13:20 ` Uwe Kleine-König 2016-09-22 13:20 ` Uwe Kleine-König 2016-09-27 7:45 ` Richard Genoud 2016-09-27 7:45 ` Richard Genoud 2016-09-27 7:45 ` Richard Genoud 2016-09-12 9:47 ` [PATCHv2 3/3] tty/serial: at91: fix hardware handshake on SAM9x5 (without GPIOs) Richard Genoud 2016-09-12 9:47 ` Richard Genoud 2016-09-12 9:53 ` Alexandre Belloni 2016-09-12 9:53 ` Alexandre Belloni 2016-09-12 10:50 ` Richard Genoud 2016-09-12 10:50 ` Richard Genoud 2016-09-12 10:50 ` Richard Genoud 2016-09-13 13:45 ` Alexandre Belloni 2016-09-13 13:45 ` Alexandre Belloni 2016-09-13 13:45 ` Alexandre Belloni 2016-09-14 10:31 ` Richard Genoud 2016-09-14 10:31 ` Richard Genoud 2016-09-14 10:31 ` Richard Genoud
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.