| Message ID | 20160930085801.21497-3-richard.genoud@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Richard, On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote: > Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when > hardware handshake is enabled") broke the hardware handshake when GPIOs > were 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. What does the HWHS flag control? What if only RTS is a gpio and CTS is not? Or the other way round? What is the problematic setup? I guess it's RTS and CTS are gpios and with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that happens? Best regards Uwe
2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hello Richard, > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote: >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when >> hardware handshake is enabled") broke the hardware handshake when GPIOs >> were 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. > > What does the HWHS flag control? What if only RTS is a gpio and CTS is > not? Or the other way round? First, HWHS flag is used only in SAMA5D2. (if I correctly understood Atmel HW guys, all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it is unusable, because they don't have Fifos nor PDC). So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS pin according to the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf). The controller will also start/stop the transmission on CTS changes. But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess). With this flag set, It's mandatory to have CTS and RTS not handled via GPIO, because if they were, the controller couldn't, well, control them. The NORMAL flag, on the contrary, just tell the controller not to mess with RTS/CTS, and in this case, the driver will handle CTS changes and drive the RTS pin, via GPIO or via the CR register. It's not a problem to have CTS as a GPIO and RTS controlled via the CR register (or CTS changes read in CSR register and RTS as a GPIO). I just gave it a quick try, works the same. ( But I don't know if it will work with FIFOs > What is the problematic setup? I guess it's RTS and CTS are gpios and > with that setting ATMEL_US_USMODE_HWHS is wrong? What happens if that > happens? Yes, CTS/RTS as GPIOs + HWHS flag is clearly wrong. If that happens, well, the controller will try to drive an RTS pin that won't have been muxed as RTS. It does not seems to be a problem, but instructing the controller do drive pins it doesn't have access doesn't really make sense. And in the case of the SAMA5D2, well I don't know what the result will be, but it could stop transmission from the "ghost" CTS signal I guess. Anyway, the problematics setups are all the setups with USMODE_HWHS enabled on platform without Fifos or PDC, i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong). For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and RTS/CTS NOT muxed as GPIOS, it's like there was no flow control at all (the CTS pin doesn't disable the transmitter). Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2, (cfhttps://lkml.org/lkml/2016/9/7/598 ), I honestly didn't dig any further into that flag. Regards, Richard
On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote : > Anyway, the problematics setups are all the setups with USMODE_HWHS > enabled on platform without Fifos or PDC, > i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong). > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc. Please, don't break those platforms. The only affected platforms are sam9x5, sama5d3 and sama5d4. > For instance, on sam9x5, if DMA is used, USMODE_HWHS enabled and > RTS/CTS NOT muxed as GPIOS, > it's like there was no flow control at all (the CTS pin doesn't > disable the transmitter). > > Since atmel HW guys said that USMODE_HWHS is broken for platforms !sama5d2, > (cfhttps://lkml.org/lkml/2016/9/7/598 ), I honestly didn't dig any > further into that flag. > > > Regards, > Richard
2016-09-30 13:16 GMT+02:00 Alexandre Belloni <alexandre.belloni@free-electrons.com>: > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote : >> Anyway, the problematics setups are all the setups with USMODE_HWHS >> enabled on platform without Fifos or PDC, >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong). >> > > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc. > Please, don't break those platforms. > > The only affected platforms are sam9x5, sama5d3 and sama5d4. Have you tested them ? And why are you saying that rm9200 and g45 will be broken with this patch ? If they have a pdc, they will fall in the case: (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) and thus use: mode |= ATMEL_US_USMODE_HWHS; won't they ?
On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote : > 2016-09-30 13:16 GMT+02:00 Alexandre Belloni > <alexandre.belloni@free-electrons.com>: > > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote : > >> Anyway, the problematics setups are all the setups with USMODE_HWHS > >> enabled on platform without Fifos or PDC, > >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong). > >> > > > > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc. > > Please, don't break those platforms. > > > > The only affected platforms are sam9x5, sama5d3 and sama5d4. > Have you tested them ? > > And why are you saying that rm9200 and g45 will be broken with this patch ? > If they have a pdc, they will fall in the case: > (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) > and thus use: > mode |= ATMEL_US_USMODE_HWHS; > > won't they ? Well, this patch wouldn't have my ack if it was breaking them. However, I'm still not sure about 3/3.
2016-09-30 13:54 GMT+02:00 Alexandre Belloni <alexandre.belloni@free-electrons.com>: > On 30/09/2016 at 13:45:47 +0200, Richard Genoud wrote : >> 2016-09-30 13:16 GMT+02:00 Alexandre Belloni >> <alexandre.belloni@free-electrons.com>: >> > On 30/09/2016 at 13:04:28 +0200, Richard Genoud wrote : >> >> Anyway, the problematics setups are all the setups with USMODE_HWHS >> >> enabled on platform without Fifos or PDC, >> >> i.e. all platforms but sama5d2 (Cyrille, correct me if I'm wrong). >> >> >> > >> > This is a wrong assumption, at91rm9200 to at91sam9g45 all have a pdc. >> > Please, don't break those platforms. >> > >> > The only affected platforms are sam9x5, sama5d3 and sama5d4. >> Have you tested them ? >> >> And why are you saying that rm9200 and g45 will be broken with this patch ? >> If they have a pdc, they will fall in the case: >> (atmel_use_pdc_rx(port) || atmel_use_fifo(port)) >> and thus use: >> mode |= ATMEL_US_USMODE_HWHS; >> >> won't they ? > > Well, this patch wouldn't have my ack if it was breaking them. It's not. Read the code. >However, I'm still not sure about 3/3. As I said 2 weeks from now ( https://lkml.org/lkml/2016/9/14/263 ) the only case where we would want to drop the CRTSCTS flag is when there's no pin muxed for CTS/RTS nor GPIOs. Fell free to give a patch for that. To be clear : this patch (2/3) unbreaks ALL platforms with GPIOs as CTS/RTS and 3/3 unbreaks sam9x5 / sama5d3 / sama5d4 platform with CTS/RTS as !GPIOs.
On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote: > 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > Hello Richard, > > > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote: > >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when > >> hardware handshake is enabled") broke the hardware handshake when GPIOs > >> were 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. > > > > What does the HWHS flag control? What if only RTS is a gpio and CTS is > > not? Or the other way round? > First, HWHS flag is used only in SAMA5D2. (if I correctly understood > Atmel HW guys, > all other platforms (sam9, sam9x5, sama5d3...) have this flag, but it > is unusable, > because they don't have Fifos nor PDC). > So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS > pin according to > the number of char present in the rx fifo (cf Figure 44-29 §44.7.3.15 p.1438 of > http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf). > The controller will also start/stop the transmission on CTS changes. > But, as I haven't got this hard, I couldn't test it. (but Cyrille did I guess). > With this flag set, It's mandatory to have CTS and RTS not handled via > GPIO, because if > they were, the controller couldn't, well, control them. Assuming the respective pin doesn't reach the hardware both are no problem though. And it would keep the driver simpler to just ignore this. There would be no need for patch 1 and also this patch could be dropped. So I guess there is really something to fix, otherwise you wouldn't start patching the driver. But I don't understand the issue, so I'd like to have a better picture. Best regards Uwe
Le Tue, 4 Oct 2016 09:25:25 +0200, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> a écrit : > On Fri, Sep 30, 2016 at 01:04:28PM +0200, Richard Genoud wrote: > > 2016-09-30 11:12 GMT+02:00 Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de>: > > > Hello Richard, > > > > > > On Fri, Sep 30, 2016 at 10:58:00AM +0200, Richard Genoud wrote: > > >> Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management > > >> when hardware handshake is enabled") broke the hardware > > >> handshake when GPIOs were 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. > > > > > > What does the HWHS flag control? What if only RTS is a gpio and > > > CTS is not? Or the other way round? > > First, HWHS flag is used only in SAMA5D2. (if I correctly understood > > Atmel HW guys, > > all other platforms (sam9, sam9x5, sama5d3...) have this flag, but > > it is unusable, > > because they don't have Fifos nor PDC). > > So, on SAMA5D2, the HWHS flag tells the controller to drive the RTS > > pin according to > > the number of char present in the rx fifo (cf Figure 44-29 > > §44.7.3.15 p.1438 of > > http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf). > > The controller will also start/stop the transmission on CTS > > changes. But, as I haven't got this hard, I couldn't test it. (but > > Cyrille did I guess). With this flag set, It's mandatory to have > > CTS and RTS not handled via GPIO, because if they were, the > > controller couldn't, well, control them. > > Assuming the respective pin doesn't reach the hardware both are no > problem though. And it would keep the driver simpler to just ignore > this. There would be no need for patch 1 and also this patch could be > dropped. So I guess there is really something to fix, otherwise you > wouldn't start patching the driver. But I don't understand the issue, > so I'd like to have a better picture. > > Best regards > Uwe > Ok, so I'll try to explain what's going on. I'm sure you're familiar with hardware handshaking (aka flow control), but anyway, it doesn't hurt to explain it for the big picture understanding. On RS232, flow control is handled by 2 pins: RTS and CTS. CTS is an input. When it's high (ttl), it indicates that we should stop transmitting. RTS is an output, we drive it at high level (ttl) to indicate that we can't receive anymore data for now (because the rx buffer is full, or the device is closed for instance). The RTS and CTS management is done in serial_core.c which calls atmel_set/get_mctrl to set/get RTS/CTS pin state. And CTS changes are detected via an interrupt in atmel_interrupt(). Now, the atmel USART controller has a feature that can handle part of the flow control job: - disabling 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). This feature is enabled by setting the flag ATMEL_US_USMODE_HWHS. And to be clear, this feature is *not* mandatory for the flow control to work ! [ The fun part is that according to atmel designers, this feature is broken for platforms with no PDC and no FIFO. (source: https://lkml.org/lkml/2016/9/7/598 ) ] 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 (among other things). The logic introduced was to set the flag ATMEL_US_USMODE_HWHS for all boards when the user space enables flow control. This was clearly wrong because this feature can only be used when PDC or FIFO are in use. If ATMEL_US_USMODE_HWHS is enabled and PDC/FIFO/DMA are not used, the RTS pin stays at high level no matter what, preventing any data to be received. Then, commit 5be605ac9af9 ("tty/serial: atmel: fix hardware handshake selection") did something completely wrong: Instead of removing the flag ATMEL_US_USMODE_HWHS for platforms with no PDC nor FIFOs, and let serial_core happily handle the CTS/RTS, it refused to set the flow control when the user space asked for it. This was because of a misunderstanding: It's not that platforms without PDC and FIFO can't do flow control at all, they just can't enable the USART controller feature relative to flag ATMEL_US_USMODE_HWHS. And, again, this feature is *not* mandatory for the flow control to work. So, now, all atmel platforms with no PDC and no FIFO (SAM9G35, SAMA5D3, etc.) can't use flow control anymore. That's the reason of this patch series. And as the controller flow control feature only works with PDC and FIFOs, the logic would be to set the ATMEL_US_USMODE_HWHS like that: if ((termios->c_cflag & CRTSCTS) && !mctrl_gpio_use_rtscts(atmel_port->gpios) && (atmel_use_pdc_rx(port) || atmel_use_fifo(port))) { mode |= ATMEL_US_USMODE_HWHS; } I added the mctrl_gpio_use_rtscts() test, because: - There's no point using the ATMEL_US_USMODE_HWHS feature when CTS/RTS are driven by GPIOs, it doesn't make any sense. - And it simply doesn't work because the transmitter is shut down. (and that's the other reason of this patch series) For the mctrl_gpio_use_rtscts() function, I agree that it may be atmel specific, and I could implement as something like atmel_use_{c,r}ts_gpio(). I'll have soon other hardware to test this on, and I'll resend a series after that. regards, Richard
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index fd8aa1f4ba78..b01b68ece35c 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -2130,8 +2130,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; @@ -2139,7 +2143,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 hardware handshake or controlled by GPIOs */ mode |= ATMEL_US_USMODE_NORMAL; }
Commit 1cf6e8fc8341 ("tty/serial: at91: fix RTS line management when hardware handshake is enabled") broke the hardware handshake when GPIOs were 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 | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)