| Message ID | 20160907161324.27774-3-richard.genoud@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 07/09/2016 at 18:13:24 +0200, Richard Genoud wrote : > Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when > hardware handshake is enabled") broke the hardware handshake when > DMA is not used. > > So, here's a summary: > If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the > controller can't drive the RTS pin (there's no way for it to know when > we can't receive data anymore). > You forgot that the PDC can properly drive the RTS pin so all the platforms that are using the PDC will break after your patch. Also, I believe the controller is able to drive the RTS and CTS pins when simply using PIOs (it knows when one complete character has been received and US_RHR has not yet been read). However, I didn't test. Maybe someone at Atmel can confirm. I think that 5be605ac9af9 is doing the right thing but I still can be convince otherwise ;).
Hi Richard,
For usart without FIFOs (hence before sama5d2), according to our designers, the
RTS line could only been controlled by an internal PDC signal which doesn't
exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds
DMA controllers. So if you enable the hardware handshaking feature on some
usart, its RTS line won't be monitored at all. The hardware handshaking is
broken on all SoCs using DMA controllers instead of PDC.
With the sama5d2, our latest MPU, we fixed this issue by introducing an
alternative mechanism which relies on 2 thresholds on the RX FIFO. So when
FIFOs are available, those thresholds can be used to control the level of RTS
line.
Indeed I think a better test should be:
if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) {
dev_info(port->dev, "not enabling hardware flow control because neither the PDC nor the FIFO are available");
termios->c_cflags &= ~CRTSCTS;
}
We can double check once again with our designers to confirm that without FIFO
the only way for the hardware handshaking to work is to use a PDC.
So IMHO, your patch should not be applied, sorry!
Best regards,
Cyrille
Le 07/09/2016 à 18:13, Richard Genoud a écrit :
> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when
> hardware handshake is enabled") broke the hardware handshake when
> DMA is not used.
>
> So, here's a summary:
> If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the
> controller can't drive the RTS pin (there's no way for it to know when
> we can't receive data anymore).
>
> If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS
> because when there's a FIFO, it can know when it's full and then drive
> the RTS pin accordingly.
> NB: I didn't test this mode since my board (at91sam9g35-cm) has not the
> FIFO mechanism.
>
> If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller
> can't control the RTS pin, and we can't control it either by hand. So,
> the hardware handshake is disabled. (that's what commit 5be605ac9af9
> ("tty/serial: atmel: fix hardware handshake selection") did).
>
> There's still something obscure to me:
> In the SAM9G35 datasheet, it's said that
> "Setting the USART to operate with hardware handshaking is performed by
> writing the USART_MODE field in US_MR to the value 0x2.
> [..] Using this mode requires using the DMA channel for reception."
>
> So, according to this, it *should* be possible to use automatic hardware
> handshake with the DMA and without FIFO (sam9x5 have no FIFOs like
> samad controllers)
>
> NB: -stable is not Cced because it doesn't cleanly apply on 4.1+
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled")
> ---
> drivers/tty/serial/atmel_serial.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e9b4fbf88c2d..503fbc623227 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2130,14 +2130,26 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> } else if ((termios->c_cflag & CRTSCTS) &&
> !mctrl_gpio_use_rtscts(atmel_port->gpios)) {
> /*
> - * RS232 with hardware handshake (RTS/CTS)
> - * handled by the controller.
> + * Automatic hardware handshake (RTS/CTS) only work with
> + * DMA enabled and FIFOs.
> + * TODO: sam9x5 controllers don't have FIFOs like samad
> + * controllers, and yet, the datasheet says that the
> + * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA
> + * channel for reception.
> */
> - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) {
> - dev_info(port->dev, "not enabling hardware flow control because DMA is used");
> - termios->c_cflag &= ~CRTSCTS;
> + if (atmel_use_dma_rx(port)) {
> + if (atmel_use_fifo(port)) {
> + mode |= ATMEL_US_USMODE_HWHS;
> + } else {
> + dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo");
> + termios->c_cflag &= ~CRTSCTS;
> + }
> } else {
> - mode |= ATMEL_US_USMODE_HWHS;
> + /*
> + * if DMA is not used, tell the controller that it
> + * should not handle the RTS pin automatically
> + */
> + mode |= ATMEL_US_USMODE_NORMAL;
> }
> } else {
> /* RS232 without hadware handshake or controlled by GPIOs */
>
Hi Alex, Cyrille, I did some tests to clear this HW handcheck history out. NB: all those tests are with a 4.8-rc5 kernel on a AT91SAM9G35-CM board. I couldn't test the PDC since, as Cyrille said, there's no such thing on SAM9G35 USARTs. The modes that DON'T WORK are: ATMEL_US_USMODE_HWHS + DMA (RTS won't go up when the data are not read anymore) ATMEL_US_USMODE_HWHS + PIO (RTS won't even go down when the port is open) The modes that WORK are: ATMEL_US_USMODE_NORMAL + DMA (no error, perfect transfer) ATMEL_US_USMODE_NORMAL + PIO (some chars are eaten or nullified) So, it seems that Atmel HW's guys where right when they said that the HW handshake is completely broken on SAM9x5 (updating the manual with a note saying that ATMEL_US_USMODE_HWHS shouldn't be use would be a good idea !). 2016-09-07 18:59 GMT+02:00 Alexandre Belloni <alexandre.belloni@free-electrons.com>: > Hi, > > You forgot that the PDC can properly drive the RTS pin so all the > platforms that are using the PDC will break after your patch. You're right, I missed the platforms with PDC. But right know, all SAM9x5 platforms are broken (and I guess SAM92xx also), so I guess we'll find a way to correct that. > Also, I believe the controller is able to drive the RTS and CTS pins > when simply using PIOs (it knows when one complete character has been > received and US_RHR has not yet been read). However, I didn't test. > Maybe someone at Atmel can confirm. I did see anything like that in SAM9G35 datasheet, and the tests I've done show the contrary. Actually, in SAM9G35 datasheet: "When a character reception is completed, it is transferred to the Receive Holding register (US_RHR) and the RXRDY bit in US_CSR rises. If a character is completed while the RXRDY is set, the OVRE (Overrun Error) bit is set. The last character is transferred into US_RHR and overwrites the previous one." > I think that 5be605ac9af9 is doing the right thing but I still can be > convince otherwise ;). I'll get to that. 2016-09-07 19:28 GMT+02:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>: > Hi Richard, > > For usart without FIFOs (hence before sama5d2), according to our designers, the > RTS line could only been controlled by an internal PDC signal which doesn't > exist with the DMA controller. Referring to its datasheet, the sam9g35 embeds > DMA controllers. So if you enable the hardware handshaking feature on some > usart, its RTS line won't be monitored at all. The hardware handshaking is > broken on all SoCs using DMA controllers instead of PDC. > > With the sama5d2, our latest MPU, we fixed this issue by introducing an > alternative mechanism which relies on 2 thresholds on the RX FIFO. So when > FIFOs are available, those thresholds can be used to control the level of RTS > line. > > Indeed I think a better test should be: > if (!atmel_use_pdc_rx(port) && !atmel_use_fifo(port)) { > dev_info(port->dev, "not enabling hardware flow control > because neither the PDC nor the FIFO are available"); > termios->c_cflags &= ~CRTSCTS; > } Maybe that's ok for SAMAD2+ platform, but not for older ones. Actually, from all the tests I've done, the only mode that works for SAM9x5 is: mode |= ATMEL_US_USMODE_NORMAL; and the CRTSCTS flag should *NOT* be removed. So, IHMO, test should be: if (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) { mode |= ATMEL_US_USMODE_HWHS; } else { mode |= ATMEL_US_USMODE_NORMAL; } Because nothing prevents the driver to drive the RTS pin itself, as it is done in atmel_set_mctrl(). So, IHMO, commit 5be605ac9af9 is all wrong. It basically says "If the controller can't handle RTS/CTS, then we don't do HW handshake." NO ! If the controller can't do it, the driver can do it for him, can't it ?! (Well, from here, it truly seems it can !) If you want to grab an atmel board and a scope to do some testing, here's what I've done: On one side: stty -F /dev/ttyS2 115200 crtscts -opost clocal cread exec 3>/dev/ttyS2 dd if=/home/rgenoud/dev/linux/MAINTAINERS bs=1M count=1 > /dev/ttyS2 exec 3>&- On the board: stty -F /dev/ttyS1 115200 crtscts -opost clocal cread exec 4</dev/ttyS1 rm -f /tmp/rcv cat <&4 >> /tmp/rcv # then ctrl-C, and "cat <&4 >> /tmp/rcv" again # at the ctrl-C, the RTS pin should go up To enable dma in sam9x5 dts, I added atmel,use-dma-rx; in usart0: serial@f801c000 > > We can double check once again with our designers to confirm that without FIFO > the only way for the hardware handshaking to work is to use a PDC. > So IMHO, your patch should not be applied, sorry! > > Best regards, > > Cyrille > Best regards. Richard.
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index e9b4fbf88c2d..503fbc623227 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -2130,14 +2130,26 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, } else if ((termios->c_cflag & CRTSCTS) && !mctrl_gpio_use_rtscts(atmel_port->gpios)) { /* - * RS232 with hardware handshake (RTS/CTS) - * handled by the controller. + * Automatic hardware handshake (RTS/CTS) only work with + * DMA enabled and FIFOs. + * TODO: sam9x5 controllers don't have FIFOs like samad + * controllers, and yet, the datasheet says that the + * ATMEL_US_USMODE_HWHS can be used with (and only with) a DMA + * channel for reception. */ - if (atmel_use_dma_rx(port) && !atmel_use_fifo(port)) { - dev_info(port->dev, "not enabling hardware flow control because DMA is used"); - termios->c_cflag &= ~CRTSCTS; + if (atmel_use_dma_rx(port)) { + if (atmel_use_fifo(port)) { + mode |= ATMEL_US_USMODE_HWHS; + } else { + dev_info(port->dev, "not enabling hardware flow control because DMA is used without fifo"); + termios->c_cflag &= ~CRTSCTS; + } } else { - mode |= ATMEL_US_USMODE_HWHS; + /* + * if DMA is not used, tell the controller that it + * should not handle the RTS pin automatically + */ + mode |= ATMEL_US_USMODE_NORMAL; } } else { /* RS232 without hadware handshake or controlled by GPIOs */
Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") broke the hardware handshake when DMA is not used. So, here's a summary: If DMA is NOT USED the mode should be ATMEL_US_USMODE_NORMAL because the controller can't drive the RTS pin (there's no way for it to know when we can't receive data anymore). If DMA is USED along with FIFOs, the mode should be ATMEL_US_USMODE_HWHS because when there's a FIFO, it can know when it's full and then drive the RTS pin accordingly. NB: I didn't test this mode since my board (at91sam9g35-cm) has not the FIFO mechanism. If DMA is USED WITHOUT FIFOs, then this is the bad case: the controller can't control the RTS pin, and we can't control it either by hand. So, the hardware handshake is disabled. (that's what commit 5be605ac9af9 ("tty/serial: atmel: fix hardware handshake selection") did). There's still something obscure to me: In the SAM9G35 datasheet, it's said that "Setting the USART to operate with hardware handshaking is performed by writing the USART_MODE field in US_MR to the value 0x2. [..] Using this mode requires using the DMA channel for reception." So, according to this, it *should* be possible to use automatic hardware handshake with the DMA and without FIFO (sam9x5 have no FIFOs like samad controllers) NB: -stable is not Cced because it doesn't cleanly apply on 4.1+ Signed-off-by: Richard Genoud <richard.genoud@gmail.com> Fixes: 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") --- drivers/tty/serial/atmel_serial.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)