| Message ID | 1480682111-9299-2-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Geert, Thank you for the patch. On Friday 02 Dec 2016 13:35:10 Geert Uytterhoeven wrote: > If a UART has dedicated RTS/CTS pins, there are some issues: > > 1. When changing hardware control flow, the new AUTORTS state is not > immediately reflected in the hardware, but only when RTS is raised. > However, the serial core doesn't call .set_mctrl() after > .set_termios(), hence AUTORTS may only become effective when the port > is closed, and reopened later. > Note that this problem does not happen when manually using stty to > change CRTSCTS, as AUTORTS will work fine on next open. > > 2. When hardware control flow is disabled (or AUTORTS is not yet > effective), changing any serial port configuration deasserts RTS, as > .set_termios() calls sci_init_pins(). Isn't this still a problem with this patch applied ? Calling sci_set_mctrl() should reconfigure the pins properly, but won't there be a short window during which the configuration will be wrong ? > To fix both issues, call .set_mctrl() from .set_termios() when dedicated > RTS/CTS pins are present, to refresh the AUTORTS or RTS state. > This is similar to what other drivers supporting AUTORTS do (e.g. > omap-serial). > > Reported-by: Baumann, Christoph (C.) <cbaumann@visteon.com> (issue 1) > Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted > RTS/CTS") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Tested on r8a7791/koelsch with HSCIF1 (GPIO hardware flow control), > and HSCIF2 and SCIFB0 (dedicated hardware flow control). > > A simple test program (basically "cat" with CRTSCTS configuration > capability) can be found at https://github.com/geertu/sercat > > Without this patch the following will fail: > > 1. sercat -f /dev/hscif2 & > seq 100 120 | sercat -f -w /dev/hscif1 # hangs > > 2. seq 200 220 | sercat -f -w /dev/hscif2 & > sercat -f /dev/hscif1 # no data received > --- > drivers/tty/serial/sh-sci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index 91e7dddbf72cd3de..c503db1900f003ed 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2340,6 +2340,10 @@ static void sci_set_termios(struct uart_port *port, > struct ktermios *termios, > > serial_port_out(port, SCFCR, ctrl); > } > + if (port->flags & UPF_HARD_FLOW) { > + /* Refresh (Auto) RTS */ > + sci_set_mctrl(port, port->mctrl); > + } > > scr_val |= s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0); > dev_dbg(port->dev, "SCSCR 0x%x\n", scr_val);
Hi Laurent, On Fri, Jan 6, 2017 at 1:30 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Friday 02 Dec 2016 13:35:10 Geert Uytterhoeven wrote: >> If a UART has dedicated RTS/CTS pins, there are some issues: >> 1. When changing hardware control flow, the new AUTORTS state is not >> immediately reflected in the hardware, but only when RTS is raised. >> However, the serial core doesn't call .set_mctrl() after >> .set_termios(), hence AUTORTS may only become effective when the port >> is closed, and reopened later. >> Note that this problem does not happen when manually using stty to >> change CRTSCTS, as AUTORTS will work fine on next open. >> >> 2. When hardware control flow is disabled (or AUTORTS is not yet >> effective), changing any serial port configuration deasserts RTS, as >> .set_termios() calls sci_init_pins(). > > Isn't this still a problem with this patch applied ? Calling sci_set_mctrl() > should reconfigure the pins properly, but won't there be a short window during > which the configuration will be wrong ? You mean in between the calls to sci_init_pins() and sci_set_mctrl()? I don't think _de_asserting RTS for a few microseconds matters much, would it? Asserting RTS wrongly would be worse. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Monday 09 Jan 2017 10:53:55 Geert Uytterhoeven wrote: > On Fri, Jan 6, 2017 at 1:30 PM, Laurent Pinchart wrote: > > On Friday 02 Dec 2016 13:35:10 Geert Uytterhoeven wrote: > >> If a UART has dedicated RTS/CTS pins, there are some issues: > >> 1. When changing hardware control flow, the new AUTORTS state is not > >> immediately reflected in the hardware, but only when RTS is raised. > >> However, the serial core doesn't call .set_mctrl() after > >> .set_termios(), hence AUTORTS may only become effective when the port > >> is closed, and reopened later. > >> Note that this problem does not happen when manually using stty to > >> change CRTSCTS, as AUTORTS will work fine on next open. > >> > >> 2. When hardware control flow is disabled (or AUTORTS is not yet > >> effective), changing any serial port configuration deasserts RTS, as > >> .set_termios() calls sci_init_pins(). > > > > Isn't this still a problem with this patch applied ? Calling > > sci_set_mctrl() should reconfigure the pins properly, but won't there be > > a short window during which the configuration will be wrong ? > > You mean in between the calls to sci_init_pins() and sci_set_mctrl()? That's what I mean, yes. > I don't think _de_asserting RTS for a few microseconds matters much, would > it? Asserting RTS wrongly would be worse. It's not the end of the world, but as you're trying to fix this issue, maybe it would be a good time to refactor the code properly and fix the core problem (.set_termios() calling sci_init_pins()) rather than working around it :-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 91e7dddbf72cd3de..c503db1900f003ed 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2340,6 +2340,10 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, serial_port_out(port, SCFCR, ctrl); } + if (port->flags & UPF_HARD_FLOW) { + /* Refresh (Auto) RTS */ + sci_set_mctrl(port, port->mctrl); + } scr_val |= s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0); dev_dbg(port->dev, "SCSCR 0x%x\n", scr_val);
If a UART has dedicated RTS/CTS pins, there are some issues: 1. When changing hardware control flow, the new AUTORTS state is not immediately reflected in the hardware, but only when RTS is raised. However, the serial core doesn't call .set_mctrl() after .set_termios(), hence AUTORTS may only become effective when the port is closed, and reopened later. Note that this problem does not happen when manually using stty to change CRTSCTS, as AUTORTS will work fine on next open. 2. When hardware control flow is disabled (or AUTORTS is not yet effective), changing any serial port configuration deasserts RTS, as .set_termios() calls sci_init_pins(). To fix both issues, call .set_mctrl() from .set_termios() when dedicated RTS/CTS pins are present, to refresh the AUTORTS or RTS state. This is similar to what other drivers supporting AUTORTS do (e.g. omap-serial). Reported-by: Baumann, Christoph (C.) <cbaumann@visteon.com> (issue 1) Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted RTS/CTS") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Tested on r8a7791/koelsch with HSCIF1 (GPIO hardware flow control), and HSCIF2 and SCIFB0 (dedicated hardware flow control). A simple test program (basically "cat" with CRTSCTS configuration capability) can be found at https://github.com/geertu/sercat Without this patch the following will fail: 1. sercat -f /dev/hscif2 & seq 100 120 | sercat -f -w /dev/hscif1 # hangs 2. seq 200 220 | sercat -f -w /dev/hscif2 & sercat -f /dev/hscif1 # no data received --- drivers/tty/serial/sh-sci.c | 4 ++++ 1 file changed, 4 insertions(+)