| Message ID | 20161027160406.25738-1-richard.genoud@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le 27/10/2016 à 18:04, Richard Genoud a écrit : > After commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management > when hardware handshake is enabled"), the hardware handshake wasn't > functional anymore on Atmel platforms (beside SAMA5D2). > > To understand why, one has to understand the flag ATMEL_US_USMODE_HWHS > first: > Before commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management > when hardware handshake is enabled"), this flag was never set. > Thus, the CTS/RTS where only handled by serial_core (and everything > worked just fine). > > This commit introduced the use of the ATMEL_US_USMODE_HWHS flag, > enabling it for all boards when the user space enables flow control. > > When the ATMEL_US_USMODE_HWHS is set, the Atmel USART controller > handles a part of the flow control job: > - disable the transmitter when the CTS pin gets high. > - drive the RTS pin high when the DMA buffer transfer is completed or > PDC RX buffer full or RX FIFO is beyond threshold. (depending on the > controller version). > > NB: This feature is *not* mandatory for the flow control to work. > (Nevertheless, it's very useful if low latencies are needed.) > > Now, the specifics of the ATMEL_US_USMODE_HWHS flag: > > - For platforms with DMAC and no FIFOs (sam9x25, sam9x35, sama5D3, > sama5D4, sam9g15, sam9g25, sam9g35)* this feature simply doesn't work. > ( source: https://lkml.org/lkml/2016/9/7/598 ) > Tested it on sam9g35, the RTS pins always stays up, even when RXEN=1 > or a new DMA transfer descriptor is set. > => ATMEL_US_USMODE_HWHS must not be used for those platforms > > - For platforms with a PDC (sam926{0,1,3}, sam9g10, sam9g20, sam9g45, > sam9g46)*, there's another kind of problem. Once the flag > ATMEL_US_USMODE_HWHS is set, the RTS pin can't be driven anymore via > RTSEN/RTSDIS in USART Control Register. The RTS pin can only be driven > by enabling/disabling the receiver or setting RCR=RNCR=0 in the PDC > (Receive (Next) Counter Register). > => Doing this is beyond the scope of this patch and could add other > bugs, so the original (and working) behaviour should be set for those > platforms (meaning ATMEL_US_USMODE_HWHS flag should be unset). > > - For platforms with a FIFO (sama5d2)*, the RTS pin is driven according > to the RX FIFO thresholds, and can be also driven by RTSEN/RTSDIS in > USART Control Register. No problem here. > (This was the use case of commit 1cf6e8fc8341 ("tty/serial: at91: fix > RTS line management when hardware handshake is enabled")) > NB: If the CTS pin declared as a GPIO in the DTS, (for instance > cts-gpios = <&pioA PIN_PB31 GPIO_ACTIVE_LOW>), the transmitter will be > disabled. > => ATMEL_US_USMODE_HWHS flag can be set for this platform ONLY IF the > CTS pin is not a GPIO. > > So, the only case when ATMEL_US_USMODE_HWHS can be enabled is when > (atmel_use_fifo(port) && > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) > > Tested on all Atmel USART controller flavours: > AT91SAM9G35-CM (DMAC flavour), AT91SAM9G20-EK (PDC flavour), > SAMA5D2xplained (FIFO flavour). > > * the list may not be exhaustive > > Cc: <stable@vger.kernel.org> #4.4+ (beware, missing atmel_port variable) > Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Thanks > --- > drivers/tty/serial/atmel_serial.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > Note for -stable: > This patch will apply on 4.4.x/4.8.x but compilation will fail due to > a missing variable atmel_port (introduced in 4.9-rc1): > > 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; > > Changes since v5: > - fix typos > - increase commentary > > Changes since v4: > - the mctrl_gpio_use_rtscts() is gone since it was atmel_serial > specific. (so patch 1 is gone) > - patches 2 and 3 have been merged together since it didn't make > a lot of sense to correct the GPIO case in one separate patch. > - ATMEL_US_USMODE_HWHS is now unset for platform with PDC > > Changes since v3: > - remove superfluous #include <linux/err.h> (thanks to Uwe) > - rebase on next-20160930 > > Changes since v2: > - remove IS_ERR_OR_NULL() test in patch 1/3 as Uwe suggested. > - fix typos in patch 2/3 > - rebase on next-20160927 > - simplify the logic in patch 3/3. > > Changes since v1: > - Correct patch 1 with the error found by kbuild. > - Add Alexandre's Acked-by on patch 2 > - Rewrite patch 3 logic in the light of the on-going discussion > with Cyrille and Alexandre. > > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index fd8aa1f4ba78..168b10cad47b 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > mode |= ATMEL_US_USMODE_RS485; > } else if (termios->c_cflag & CRTSCTS) { > /* RS232 with hardware handshake (RTS/CTS) */ > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > - termios->c_cflag &= ~CRTSCTS; > - } else { > + if (atmel_use_fifo(port) && > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > + /* > + * with ATMEL_US_USMODE_HWHS set, the controller will > + * be able to drive the RTS pin high/low when the RX > + * FIFO is above RXFTHRES/below RXFTHRES2. > + * It will also disable the transmitter when the CTS > + * pin is high. > + * This mode is not activated if CTS pin is a GPIO > + * because in this case, the transmitter is always > + * disabled (there must be an internal pull-up > + * responsible for this behaviour). > + * If the RTS pin is a GPIO, the controller won't be > + * able to drive it according to the FIFO thresholds, > + * but it will be handled by the driver. > + */ > mode |= ATMEL_US_USMODE_HWHS; > + } else { > + /* > + * For platforms without FIFO, the flow control is > + * handled by the driver. > + */ > + mode |= ATMEL_US_USMODE_NORMAL; > } > } else { > /* RS232 without hadware handshake */ >
Hello Richard, On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index fd8aa1f4ba78..168b10cad47b 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > mode |= ATMEL_US_USMODE_RS485; > } else if (termios->c_cflag & CRTSCTS) { > /* RS232 with hardware handshake (RTS/CTS) */ > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > - termios->c_cflag &= ~CRTSCTS; > - } else { > + if (atmel_use_fifo(port) && > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > + /* > + * with ATMEL_US_USMODE_HWHS set, the controller will > + * be able to drive the RTS pin high/low when the RX > + * FIFO is above RXFTHRES/below RXFTHRES2. > + * It will also disable the transmitter when the CTS > + * pin is high. > + * This mode is not activated if CTS pin is a GPIO > + * because in this case, the transmitter is always > + * disabled (there must be an internal pull-up > + * responsible for this behaviour). > + * If the RTS pin is a GPIO, the controller won't be > + * able to drive it according to the FIFO thresholds, > + * but it will be handled by the driver. > + */ > mode |= ATMEL_US_USMODE_HWHS; You use !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) as indicator that the cts mode of the respective pin is used. Is this reliable? (It's not if there are machines that don't use CTS, neither as gpio nor using the hardware function.) Maybe this needs a dt property to indicate that there is no (hw)handshaking available? > + } else { > + ... > } Best regards Uwe
On 27/10/2016 at 20:02:29 +0200, Uwe Kleine-König wrote : > Hello Richard, > > On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > > index fd8aa1f4ba78..168b10cad47b 100644 > > --- a/drivers/tty/serial/atmel_serial.c > > +++ b/drivers/tty/serial/atmel_serial.c > > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > > mode |= ATMEL_US_USMODE_RS485; > > } else if (termios->c_cflag & CRTSCTS) { > > /* RS232 with hardware handshake (RTS/CTS) */ > > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > > - termios->c_cflag &= ~CRTSCTS; > > - } else { > > + if (atmel_use_fifo(port) && > > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > > + /* > > + * with ATMEL_US_USMODE_HWHS set, the controller will > > + * be able to drive the RTS pin high/low when the RX > > + * FIFO is above RXFTHRES/below RXFTHRES2. > > + * It will also disable the transmitter when the CTS > > + * pin is high. > > + * This mode is not activated if CTS pin is a GPIO > > + * because in this case, the transmitter is always > > + * disabled (there must be an internal pull-up > > + * responsible for this behaviour). > > + * If the RTS pin is a GPIO, the controller won't be > > + * able to drive it according to the FIFO thresholds, > > + * but it will be handled by the driver. > > + */ > > mode |= ATMEL_US_USMODE_HWHS; > > You use > > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) > > as indicator that the cts mode of the respective pin is used. Is this > reliable? (It's not if there are machines that don't use CTS, neither as > gpio nor using the hardware function.) Maybe this needs a dt property to > indicate that there is no (hw)handshaking available? > We had a call today were we agreed that this should be added in a future patch. Let's fix the regression for now.
2016-10-27 20:02 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hello Richard, > > On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index fd8aa1f4ba78..168b10cad47b 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, >> mode |= ATMEL_US_USMODE_RS485; >> } else if (termios->c_cflag & CRTSCTS) { >> /* RS232 with hardware handshake (RTS/CTS) */ >> - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { >> - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); >> - termios->c_cflag &= ~CRTSCTS; >> - } else { >> + if (atmel_use_fifo(port) && >> + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { >> + /* >> + * with ATMEL_US_USMODE_HWHS set, the controller will >> + * be able to drive the RTS pin high/low when the RX >> + * FIFO is above RXFTHRES/below RXFTHRES2. >> + * It will also disable the transmitter when the CTS >> + * pin is high. >> + * This mode is not activated if CTS pin is a GPIO >> + * because in this case, the transmitter is always >> + * disabled (there must be an internal pull-up >> + * responsible for this behaviour). >> + * If the RTS pin is a GPIO, the controller won't be >> + * able to drive it according to the FIFO thresholds, >> + * but it will be handled by the driver. >> + */ >> mode |= ATMEL_US_USMODE_HWHS; > > You use > > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) > > as indicator that the cts mode of the respective pin is used. Is this > reliable? (It's not if there are machines that don't use CTS, neither as > gpio nor using the hardware function.) Maybe this needs a dt property to > indicate that there is no (hw)handshaking available? I used that to filter-out the case where CTS is a GPIO. Now, for the machines that don't use CTS neither as GPIO nor using the hardware function, it's a whole different story, beyond the scope of this patch. And like you said, a DT property could be useful to handle this case. ( It's a little bit like if there was an RS232 cable with just TX/RX/GND, but this will be way harder to detect :) ) Anyway, patches are welcome to handle that, but I don't think it belongs in this one. >> + } else { >> + ... >> } > > Best regards > Uwe > regards, Richard
Le 27/10/2016 à 18:04, Richard Genoud a écrit : > After commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management > when hardware handshake is enabled"), the hardware handshake wasn't > functional anymore on Atmel platforms (beside SAMA5D2). > > To understand why, one has to understand the flag ATMEL_US_USMODE_HWHS > first: > Before commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management > when hardware handshake is enabled"), this flag was never set. > Thus, the CTS/RTS where only handled by serial_core (and everything > worked just fine). > > This commit introduced the use of the ATMEL_US_USMODE_HWHS flag, > enabling it for all boards when the user space enables flow control. > > When the ATMEL_US_USMODE_HWHS is set, the Atmel USART controller > handles a part of the flow control job: > - disable the transmitter when the CTS pin gets high. > - drive the RTS pin high when the DMA buffer transfer is completed or > PDC RX buffer full or RX FIFO is beyond threshold. (depending on the > controller version). > > NB: This feature is *not* mandatory for the flow control to work. > (Nevertheless, it's very useful if low latencies are needed.) > > Now, the specifics of the ATMEL_US_USMODE_HWHS flag: > > - For platforms with DMAC and no FIFOs (sam9x25, sam9x35, sama5D3, > sama5D4, sam9g15, sam9g25, sam9g35)* this feature simply doesn't work. > ( source: https://lkml.org/lkml/2016/9/7/598 ) > Tested it on sam9g35, the RTS pins always stays up, even when RXEN=1 > or a new DMA transfer descriptor is set. > => ATMEL_US_USMODE_HWHS must not be used for those platforms > > - For platforms with a PDC (sam926{0,1,3}, sam9g10, sam9g20, sam9g45, > sam9g46)*, there's another kind of problem. Once the flag > ATMEL_US_USMODE_HWHS is set, the RTS pin can't be driven anymore via > RTSEN/RTSDIS in USART Control Register. The RTS pin can only be driven > by enabling/disabling the receiver or setting RCR=RNCR=0 in the PDC > (Receive (Next) Counter Register). > => Doing this is beyond the scope of this patch and could add other > bugs, so the original (and working) behaviour should be set for those > platforms (meaning ATMEL_US_USMODE_HWHS flag should be unset). > > - For platforms with a FIFO (sama5d2)*, the RTS pin is driven according > to the RX FIFO thresholds, and can be also driven by RTSEN/RTSDIS in > USART Control Register. No problem here. > (This was the use case of commit 1cf6e8fc8341 ("tty/serial: at91: fix > RTS line management when hardware handshake is enabled")) > NB: If the CTS pin declared as a GPIO in the DTS, (for instance > cts-gpios = <&pioA PIN_PB31 GPIO_ACTIVE_LOW>), the transmitter will be > disabled. > => ATMEL_US_USMODE_HWHS flag can be set for this platform ONLY IF the > CTS pin is not a GPIO. > > So, the only case when ATMEL_US_USMODE_HWHS can be enabled is when > (atmel_use_fifo(port) && > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) > > Tested on all Atmel USART controller flavours: > AT91SAM9G35-CM (DMAC flavour), AT91SAM9G20-EK (PDC flavour), > SAMA5D2xplained (FIFO flavour). > > * the list may not be exhaustive > > Cc: <stable@vger.kernel.org> #4.4+ (beware, missing atmel_port variable) > Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> Acked-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> thanks :) > --- > drivers/tty/serial/atmel_serial.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > Note for -stable: > This patch will apply on 4.4.x/4.8.x but compilation will fail due to > a missing variable atmel_port (introduced in 4.9-rc1): > > 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; > > Changes since v5: > - fix typos > - increase commentary > > Changes since v4: > - the mctrl_gpio_use_rtscts() is gone since it was atmel_serial > specific. (so patch 1 is gone) > - patches 2 and 3 have been merged together since it didn't make > a lot of sense to correct the GPIO case in one separate patch. > - ATMEL_US_USMODE_HWHS is now unset for platform with PDC > > Changes since v3: > - remove superfluous #include <linux/err.h> (thanks to Uwe) > - rebase on next-20160930 > > Changes since v2: > - remove IS_ERR_OR_NULL() test in patch 1/3 as Uwe suggested. > - fix typos in patch 2/3 > - rebase on next-20160927 > - simplify the logic in patch 3/3. > > Changes since v1: > - Correct patch 1 with the error found by kbuild. > - Add Alexandre's Acked-by on patch 2 > - Rewrite patch 3 logic in the light of the on-going discussion > with Cyrille and Alexandre. > > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index fd8aa1f4ba78..168b10cad47b 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > mode |= ATMEL_US_USMODE_RS485; > } else if (termios->c_cflag & CRTSCTS) { > /* RS232 with hardware handshake (RTS/CTS) */ > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > - termios->c_cflag &= ~CRTSCTS; > - } else { > + if (atmel_use_fifo(port) && > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > + /* > + * with ATMEL_US_USMODE_HWHS set, the controller will > + * be able to drive the RTS pin high/low when the RX > + * FIFO is above RXFTHRES/below RXFTHRES2. > + * It will also disable the transmitter when the CTS > + * pin is high. > + * This mode is not activated if CTS pin is a GPIO > + * because in this case, the transmitter is always > + * disabled (there must be an internal pull-up > + * responsible for this behaviour). > + * If the RTS pin is a GPIO, the controller won't be > + * able to drive it according to the FIFO thresholds, > + * but it will be handled by the driver. > + */ > mode |= ATMEL_US_USMODE_HWHS; > + } else { > + /* > + * For platforms without FIFO, the flow control is > + * handled by the driver. > + */ > + mode |= ATMEL_US_USMODE_NORMAL; > } > } else { > /* RS232 without hadware handshake */ >
On Fri, Oct 28, 2016 at 01:13:31AM +0200, Alexandre Belloni wrote: > On 27/10/2016 at 20:02:29 +0200, Uwe Kleine-König wrote : > > Hello Richard, > > > > On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: > > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > > > index fd8aa1f4ba78..168b10cad47b 100644 > > > --- a/drivers/tty/serial/atmel_serial.c > > > +++ b/drivers/tty/serial/atmel_serial.c > > > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > > > mode |= ATMEL_US_USMODE_RS485; > > > } else if (termios->c_cflag & CRTSCTS) { > > > /* RS232 with hardware handshake (RTS/CTS) */ > > > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > > > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > > > - termios->c_cflag &= ~CRTSCTS; > > > - } else { > > > + if (atmel_use_fifo(port) && > > > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > > > + /* > > > + * with ATMEL_US_USMODE_HWHS set, the controller will > > > + * be able to drive the RTS pin high/low when the RX > > > + * FIFO is above RXFTHRES/below RXFTHRES2. > > > + * It will also disable the transmitter when the CTS > > > + * pin is high. > > > + * This mode is not activated if CTS pin is a GPIO > > > + * because in this case, the transmitter is always > > > + * disabled (there must be an internal pull-up > > > + * responsible for this behaviour). > > > + * If the RTS pin is a GPIO, the controller won't be > > > + * able to drive it according to the FIFO thresholds, > > > + * but it will be handled by the driver. > > > + */ > > > mode |= ATMEL_US_USMODE_HWHS; > > > > You use > > > > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) > > > > as indicator that the cts mode of the respective pin is used. Is this > > reliable? (It's not if there are machines that don't use CTS, neither as > > gpio nor using the hardware function.) Maybe this needs a dt property to > > indicate that there is no (hw)handshaking available? > > > > We had a call today were we agreed that this should be added in a future > patch. Let's fix the regression for now. A machine without CTS (neither gpio nor hw function) used to work fine before the breaking commit, right? So this case is part of the regression and needs a fix? Anyhow, this probably shouldn't stop the commit entering mainline because there are probably very few such machines (if any). So: Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
2016-10-28 11:51 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > On Fri, Oct 28, 2016 at 01:13:31AM +0200, Alexandre Belloni wrote: >> On 27/10/2016 at 20:02:29 +0200, Uwe Kleine-König wrote : >> > Hello Richard, >> > >> > On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: >> > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> > > index fd8aa1f4ba78..168b10cad47b 100644 >> > > --- a/drivers/tty/serial/atmel_serial.c >> > > +++ b/drivers/tty/serial/atmel_serial.c >> > > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, >> > > mode |= ATMEL_US_USMODE_RS485; >> > > } else if (termios->c_cflag & CRTSCTS) { >> > > /* RS232 with hardware handshake (RTS/CTS) */ >> > > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { >> > > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); >> > > - termios->c_cflag &= ~CRTSCTS; >> > > - } else { >> > > + if (atmel_use_fifo(port) && >> > > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { >> > > + /* >> > > + * with ATMEL_US_USMODE_HWHS set, the controller will >> > > + * be able to drive the RTS pin high/low when the RX >> > > + * FIFO is above RXFTHRES/below RXFTHRES2. >> > > + * It will also disable the transmitter when the CTS >> > > + * pin is high. >> > > + * This mode is not activated if CTS pin is a GPIO >> > > + * because in this case, the transmitter is always >> > > + * disabled (there must be an internal pull-up >> > > + * responsible for this behaviour). >> > > + * If the RTS pin is a GPIO, the controller won't be >> > > + * able to drive it according to the FIFO thresholds, >> > > + * but it will be handled by the driver. >> > > + */ >> > > mode |= ATMEL_US_USMODE_HWHS; >> > >> > You use >> > >> > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) >> > >> > as indicator that the cts mode of the respective pin is used. Is this >> > reliable? (It's not if there are machines that don't use CTS, neither as >> > gpio nor using the hardware function.) Maybe this needs a dt property to >> > indicate that there is no (hw)handshaking available? >> > >> >> We had a call today were we agreed that this should be added in a future >> patch. Let's fix the regression for now. > > A machine without CTS (neither gpio nor hw function) used to work fine > before the breaking commit, right? So this case is part of the > regression and needs a fix? Actually, a machine with a FIFO and without CTS didn't even exist at the time of the breaking commit (v4.0), the FIFO handling was introduced later, so it's not even a regression ! > Anyhow, this probably shouldn't stop the commit entering mainline > because there are probably very few such machines (if any). > > So: > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Best regards > Uwe Thanks ! Greg, could you take this in your tree ? regards, Richard.
On Fri, Oct 28, 2016 at 12:56:09PM +0200, Richard Genoud wrote: > 2016-10-28 11:51 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > On Fri, Oct 28, 2016 at 01:13:31AM +0200, Alexandre Belloni wrote: > >> On 27/10/2016 at 20:02:29 +0200, Uwe Kleine-König wrote : > >> > Hello Richard, > >> > > >> > On Thu, Oct 27, 2016 at 06:04:06PM +0200, Richard Genoud wrote: > >> > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > >> > > index fd8aa1f4ba78..168b10cad47b 100644 > >> > > --- a/drivers/tty/serial/atmel_serial.c > >> > > +++ b/drivers/tty/serial/atmel_serial.c > >> > > @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > >> > > mode |= ATMEL_US_USMODE_RS485; > >> > > } else if (termios->c_cflag & CRTSCTS) { > >> > > /* RS232 with hardware handshake (RTS/CTS) */ > >> > > - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { > >> > > - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); > >> > > - termios->c_cflag &= ~CRTSCTS; > >> > > - } else { > >> > > + if (atmel_use_fifo(port) && > >> > > + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { > >> > > + /* > >> > > + * with ATMEL_US_USMODE_HWHS set, the controller will > >> > > + * be able to drive the RTS pin high/low when the RX > >> > > + * FIFO is above RXFTHRES/below RXFTHRES2. > >> > > + * It will also disable the transmitter when the CTS > >> > > + * pin is high. > >> > > + * This mode is not activated if CTS pin is a GPIO > >> > > + * because in this case, the transmitter is always > >> > > + * disabled (there must be an internal pull-up > >> > > + * responsible for this behaviour). > >> > > + * If the RTS pin is a GPIO, the controller won't be > >> > > + * able to drive it according to the FIFO thresholds, > >> > > + * but it will be handled by the driver. > >> > > + */ > >> > > mode |= ATMEL_US_USMODE_HWHS; > >> > > >> > You use > >> > > >> > !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS) > >> > > >> > as indicator that the cts mode of the respective pin is used. Is this > >> > reliable? (It's not if there are machines that don't use CTS, neither as > >> > gpio nor using the hardware function.) Maybe this needs a dt property to > >> > indicate that there is no (hw)handshaking available? > >> > > >> > >> We had a call today were we agreed that this should be added in a future > >> patch. Let's fix the regression for now. > > > > A machine without CTS (neither gpio nor hw function) used to work fine > > before the breaking commit, right? So this case is part of the > > regression and needs a fix? > Actually, a machine with a FIFO and without CTS didn't even exist at the > time of the breaking commit (v4.0), the FIFO handling was introduced later, > so it's not even a regression ! > > > Anyhow, this probably shouldn't stop the commit entering mainline > > because there are probably very few such machines (if any). > > > > So: > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > Best regards > > Uwe > > > Thanks ! > > Greg, could you take this in your tree ? Now applied, thanks for working through all of this. greg k-h
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index fd8aa1f4ba78..168b10cad47b 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -2132,11 +2132,29 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, mode |= ATMEL_US_USMODE_RS485; } else if (termios->c_cflag & CRTSCTS) { /* RS232 with hardware handshake (RTS/CTS) */ - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); - termios->c_cflag &= ~CRTSCTS; - } else { + if (atmel_use_fifo(port) && + !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) { + /* + * with ATMEL_US_USMODE_HWHS set, the controller will + * be able to drive the RTS pin high/low when the RX + * FIFO is above RXFTHRES/below RXFTHRES2. + * It will also disable the transmitter when the CTS + * pin is high. + * This mode is not activated if CTS pin is a GPIO + * because in this case, the transmitter is always + * disabled (there must be an internal pull-up + * responsible for this behaviour). + * If the RTS pin is a GPIO, the controller won't be + * able to drive it according to the FIFO thresholds, + * but it will be handled by the driver. + */ mode |= ATMEL_US_USMODE_HWHS; + } else { + /* + * For platforms without FIFO, the flow control is + * handled by the driver. + */ + mode |= ATMEL_US_USMODE_NORMAL; } } else { /* RS232 without hadware handshake */
After commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled"), the hardware handshake wasn't functional anymore on Atmel platforms (beside SAMA5D2). To understand why, one has to understand the flag ATMEL_US_USMODE_HWHS first: Before commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled"), this flag was never set. Thus, the CTS/RTS where only handled by serial_core (and everything worked just fine). This commit introduced the use of the ATMEL_US_USMODE_HWHS flag, enabling it for all boards when the user space enables flow control. When the ATMEL_US_USMODE_HWHS is set, the Atmel USART controller handles a part of the flow control job: - disable the transmitter when the CTS pin gets high. - drive the RTS pin high when the DMA buffer transfer is completed or PDC RX buffer full or RX FIFO is beyond threshold. (depending on the controller version). NB: This feature is *not* mandatory for the flow control to work. (Nevertheless, it's very useful if low latencies are needed.) Now, the specifics of the ATMEL_US_USMODE_HWHS flag: - For platforms with DMAC and no FIFOs (sam9x25, sam9x35, sama5D3, sama5D4, sam9g15, sam9g25, sam9g35)* this feature simply doesn't work. ( source: https://lkml.org/lkml/2016/9/7/598 ) Tested it on sam9g35, the RTS pins always stays up, even when RXEN=1 or a new DMA transfer descriptor is set. => ATMEL_US_USMODE_HWHS must not be used for those platforms - For platforms with a PDC (sam926{0,1,3}, sam9g10, sam9g20, sam9g45, sam9g46)*, there's another kind of problem. Once the flag ATMEL_US_USMODE_HWHS is set, the RTS pin can't be driven anymore via RTSEN/RTSDIS in USART Control Register. The RTS pin can only be driven by enabling/disabling the receiver or setting RCR=RNCR=0 in the PDC (Receive (Next) Counter Register). => Doing this is beyond the scope of this patch and could add other bugs, so the original (and working) behaviour should be set for those platforms (meaning ATMEL_US_USMODE_HWHS flag should be unset). - For platforms with a FIFO (sama5d2)*, the RTS pin is driven according to the RX FIFO thresholds, and can be also driven by RTSEN/RTSDIS in USART Control Register. No problem here. (This was the use case of commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")) NB: If the CTS pin declared as a GPIO in the DTS, (for instance cts-gpios = <&pioA PIN_PB31 GPIO_ACTIVE_LOW>), the transmitter will be disabled. => ATMEL_US_USMODE_HWHS flag can be set for this platform ONLY IF the CTS pin is not a GPIO. So, the only case when ATMEL_US_USMODE_HWHS can be enabled is when (atmel_use_fifo(port) && !mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS)) Tested on all Atmel USART controller flavours: AT91SAM9G35-CM (DMAC flavour), AT91SAM9G20-EK (PDC flavour), SAMA5D2xplained (FIFO flavour). * the list may not be exhaustive Cc: <stable@vger.kernel.org> #4.4+ (beware, missing atmel_port variable) Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") Signed-off-by: Richard Genoud <richard.genoud@gmail.com> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/tty/serial/atmel_serial.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) Note for -stable: This patch will apply on 4.4.x/4.8.x but compilation will fail due to a missing variable atmel_port (introduced in 4.9-rc1): 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; Changes since v5: - fix typos - increase commentary Changes since v4: - the mctrl_gpio_use_rtscts() is gone since it was atmel_serial specific. (so patch 1 is gone) - patches 2 and 3 have been merged together since it didn't make a lot of sense to correct the GPIO case in one separate patch. - ATMEL_US_USMODE_HWHS is now unset for platform with PDC Changes since v3: - remove superfluous #include <linux/err.h> (thanks to Uwe) - rebase on next-20160930 Changes since v2: - remove IS_ERR_OR_NULL() test in patch 1/3 as Uwe suggested. - fix typos in patch 2/3 - rebase on next-20160927 - simplify the logic in patch 3/3. Changes since v1: - Correct patch 1 with the error found by kbuild. - Add Alexandre's Acked-by on patch 2 - Rewrite patch 3 logic in the light of the on-going discussion with Cyrille and Alexandre.