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

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

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()?
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 -- ge...@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

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

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

[Geert Uytterhoeven]

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