Re: [PATCH 1/2] serial: sh-sci: Fix early deassertion of dedicated RTS

[Laurent Pinchart]

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@xxxxxxxxxxx> (issue 1)
> Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted
> RTS/CTS")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
> 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);

-- 
Regards,

Laurent Pinchart