[PATCH v2 0/4] serial: sh-sci: FIFO initialization fixes

[PATCH v2 0/4] serial: sh-sci: FIFO initialization fixes

[Geert Uytterhoeven]

	Hi Greg, Jiri,

When opening a Renesas SCIF serial port after it has been used before,
stale data may be read.  This has been observed on R-Car Gen2 and R-Car
Gen3, with all SCIF variants present (SCIF, SCIFA, SCIFB, and HSCIF). It
is much more likely to happen when DMA is enabled, although it has been
seen with PIO, too.

There are actually two reasons why stale data is received:
  1. Transfers are started, or are still activated, before the FIFO is
     reset, causing one or more (up to the RX FIFO size) stale bytes
     (fixed by patches 1 and 2),
  2. FIFO reset lacked clearing the RDF flag, causing one byte of stale
     data (fixed by patch 3).
While at it, patch 4 adds the missing clearing of two other flags in the
initialization sequence on (H)SCIF.

While the issue can be reproduced using subsequent runs of sertest[1],
I wrote a new test program, fifotest[2], to trigger it more easily.
More detailed test information can be found on the eLinux wiki[3].

I have verified that this series fixes the issue on SCIF, SCIFA, SCIFB,
and HSCIF, on R-Car Gen2 (r8a7791/koelsch) and R-Car Gen3
(r8a7795/salvator-x).
Basic regression testing has been done with SCIFA on sh73a0/kzm9g.
Regression testing on other variants (notably SCI) would be appreciated.

Changes compared to v1:
  - Do not clear clock source bits, since the serial console relies on
    them, as reported by Shimoda-san.

This series applies against both v4.6 and next-20160526.
For testing, it's also available in a topic branch at
https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/scif-fifo-v2.

Please apply, thanks!

[1] https://github.com/geertu/sertest
[2] https://github.com/geertu/fifotest
[3] http://elinux.org/Tests:SCIF-FIFO

Geert Uytterhoeven (4):
  serial: sh-sci: Do not start transfers from sci_startup()
  serial: sh-sci: Stop transfers in sci_shutdown()
  serial: sh-sci: Clear RX, error, and break flags during reset
  serial: sh-sci: Clear (H)SCIF timeout and overrun during reset

 drivers/tty/serial/sh-sci.c | 20 ++++++++++++++------
 drivers/tty/serial/sh-sci.h |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

-- 
1.9.1

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

[PATCH v2 1/4] serial: sh-sci: Do not start transfers from sci_startup()

[Geert Uytterhoeven]

FIFO reset is done in sci_reset(), called from sci_set_termios(), while
sci_start_tx() and sci_start_rx() are called before, from sci_startup().
However, starting transfers before the UART's FIFOs have been reset may
cause reading of stale data.

Remove the calls to sci_start_tx() and sci_start_rx() from sci_startup()
to fix this.

Transfers are still started when needed:
  - sci_start_rx() is called from sci_set_termios() after FIFO reset, if
    the CREAD flag is set,
  - sci_start_tx() is called from uart_change_speed() immediately
    thereafter, if transmission is enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - No changes.
---
 drivers/tty/serial/sh-sci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 0130feb069aee02f..d6ba90c572f7475c 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1873,7 +1873,6 @@ static void sci_break_ctl(struct uart_port *port, int break_state)
 static int sci_startup(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
-	unsigned long flags;
 	int ret;
 
 	dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
@@ -1884,11 +1883,6 @@ static int sci_startup(struct uart_port *port)
 
 	sci_request_dma(port);
 
-	spin_lock_irqsave(&port->lock, flags);
-	sci_start_tx(port);
-	sci_start_rx(port);
-	spin_unlock_irqrestore(&port->lock, flags);
-
 	return 0;
 }
 
-- 
1.9.1

[PATCH v2 2/4] serial: sh-sci: Stop transfers in sci_shutdown()

[Geert Uytterhoeven]

Make sure the transmitter and receiver are stopped when shutting down
the port, and related interrupts are disabled.

Without this:
  - New input data may be received into the RX FIFO, possibly
    triggering a new RX DMA completion,
  - Transfers will still be enabled on a subsequent startup of the UART,
    before the UART's FIFOs have been reset, causing reading of stale
    data.

Inspired by a patch in the BSP by Koji Matsuoka
<koji.matsuoka.xm@renesas.com>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Do not clear clock source bits, since the serial console relies on
    them, as reported by Shimoda-san.

v1 was extracted from "[PATCH/RFC v3 0/4] serial: sh-sci: Add DT DMA
support".  The issues with the serial console seen before on
r8a7740/armadillo and sh73a0/kzm9g seem to be gone.  Changes after
resurrection:
  - Write zero to also disable related interrupts, as suggested by
    Laurent Pinchart,
  - Enhanced patch description.
---
 drivers/tty/serial/sh-sci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index d6ba90c572f7475c..eecace576c3b758f 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1890,12 +1890,17 @@ static void sci_shutdown(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
 	unsigned long flags;
+	u16 scr;
 
 	dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
 
 	spin_lock_irqsave(&port->lock, flags);
 	sci_stop_rx(port);
 	sci_stop_tx(port);
+	/* Stop RX and TX, disable related interrupts, keep clock source */
+	scr = serial_port_in(port, SCSCR);
+	serial_port_out(port, SCSCR, scr & (SCSCR_CKE1 | SCSCR_CKE0));
+
 	spin_unlock_irqrestore(&port->lock, flags);
 
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
-- 
1.9.1

Re: [PATCH v2 2/4] serial: sh-sci: Stop transfers in sci_shutdown()

[Greg Kroah-Hartman]

On Fri, Jun 24, 2016 at 04:59:14PM +0200, Geert Uytterhoeven wrote:
> Make sure the transmitter and receiver are stopped when shutting down
> the port, and related interrupts are disabled.
> 
> Without this:
>   - New input data may be received into the RX FIFO, possibly
>     triggering a new RX DMA completion,
>   - Transfers will still be enabled on a subsequent startup of the UART,
>     before the UART's FIFOs have been reset, causing reading of stale
>     data.
> 
> Inspired by a patch in the BSP by Koji Matsuoka
> <koji.matsuoka.xm@renesas.com>.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Do not clear clock source bits, since the serial console relies on
>     them, as reported by Shimoda-san.
> 
> v1 was extracted from "[PATCH/RFC v3 0/4] serial: sh-sci: Add DT DMA
> support".  The issues with the serial console seen before on
> r8a7740/armadillo and sh73a0/kzm9g seem to be gone.  Changes after
> resurrection:
>   - Write zero to also disable related interrupts, as suggested by
>     Laurent Pinchart,
>   - Enhanced patch description.
> ---
>  drivers/tty/serial/sh-sci.c | 5 +++++
>  1 file changed, 5 insertions(+)

This patch does not apply to my tree :(

Re: [PATCH v2 2/4] serial: sh-sci: Stop transfers in sci_shutdown()

[Greg Kroah-Hartman]

On Sat, Jun 25, 2016 at 09:09:39AM -0700, Greg Kroah-Hartman wrote:
> On Fri, Jun 24, 2016 at 04:59:14PM +0200, Geert Uytterhoeven wrote:
> > Make sure the transmitter and receiver are stopped when shutting down
> > the port, and related interrupts are disabled.
> > 
> > Without this:
> >   - New input data may be received into the RX FIFO, possibly
> >     triggering a new RX DMA completion,
> >   - Transfers will still be enabled on a subsequent startup of the UART,
> >     before the UART's FIFOs have been reset, causing reading of stale
> >     data.
> > 
> > Inspired by a patch in the BSP by Koji Matsuoka
> > <koji.matsuoka.xm@renesas.com>.
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v2:
> >   - Do not clear clock source bits, since the serial console relies on
> >     them, as reported by Shimoda-san.
> > 
> > v1 was extracted from "[PATCH/RFC v3 0/4] serial: sh-sci: Add DT DMA
> > support".  The issues with the serial console seen before on
> > r8a7740/armadillo and sh73a0/kzm9g seem to be gone.  Changes after
> > resurrection:
> >   - Write zero to also disable related interrupts, as suggested by
> >     Laurent Pinchart,
> >   - Enhanced patch description.
> > ---
> >  drivers/tty/serial/sh-sci.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> This patch does not apply to my tree :(

But I don't have access to my 2-factor token for about 24 hours, so my
tree will not be public until after then, sorry about that, I'll email
you when it gets synced up to kernel.org...

greg k-h

Re: [PATCH v2 2/4] serial: sh-sci: Stop transfers in sci_shutdown()

[Greg Kroah-Hartman]

On Sat, Jun 25, 2016 at 09:10:51AM -0700, Greg Kroah-Hartman wrote:
> On Sat, Jun 25, 2016 at 09:09:39AM -0700, Greg Kroah-Hartman wrote:
> > On Fri, Jun 24, 2016 at 04:59:14PM +0200, Geert Uytterhoeven wrote:
> > > Make sure the transmitter and receiver are stopped when shutting down
> > > the port, and related interrupts are disabled.
> > > 
> > > Without this:
> > >   - New input data may be received into the RX FIFO, possibly
> > >     triggering a new RX DMA completion,
> > >   - Transfers will still be enabled on a subsequent startup of the UART,
> > >     before the UART's FIFOs have been reset, causing reading of stale
> > >     data.
> > > 
> > > Inspired by a patch in the BSP by Koji Matsuoka
> > > <koji.matsuoka.xm@renesas.com>.
> > > 
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > v2:
> > >   - Do not clear clock source bits, since the serial console relies on
> > >     them, as reported by Shimoda-san.
> > > 
> > > v1 was extracted from "[PATCH/RFC v3 0/4] serial: sh-sci: Add DT DMA
> > > support".  The issues with the serial console seen before on
> > > r8a7740/armadillo and sh73a0/kzm9g seem to be gone.  Changes after
> > > resurrection:
> > >   - Write zero to also disable related interrupts, as suggested by
> > >     Laurent Pinchart,
> > >   - Enhanced patch description.
> > > ---
> > >  drivers/tty/serial/sh-sci.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > 
> > This patch does not apply to my tree :(
> 
> But I don't have access to my 2-factor token for about 24 hours, so my
> tree will not be public until after then, sorry about that, I'll email
> you when it gets synced up to kernel.org...

It's synced now, sorry for the noise.

greg k-h

[PATCH v2 3/4] serial: sh-sci: Clear RX, error, and break flags during reset

[Geert Uytterhoeven]

Setting the FIFO reset bits is not sufficient to reset the RX FIFO.
After this the status register's RDF flag bit may still be set, causing
the reception of one stale byte of data.

To fix this, clear all status flag bits related to reception, error, and
break handling, cfr. the initialization flowchart in the datasheet.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - No changes.
---
 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 eecace576c3b758f..0a2be5d293220e08 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2055,6 +2055,10 @@ static void sci_reset(struct uart_port *port)
 	reg = sci_getreg(port, SCFCR);
 	if (reg->size)
 		serial_port_out(port, SCFCR, SCFCR_RFRST | SCFCR_TFRST);
+
+	sci_clear_SCxSR(port,
+			SCxSR_RDxF_CLEAR(port) & SCxSR_ERROR_CLEAR(port) &
+			SCxSR_BREAK_CLEAR(port));
 }
 
 static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
-- 
1.9.1

[PATCH v2 4/4] serial: sh-sci: Clear (H)SCIF timeout and overrun during reset

[Geert Uytterhoeven]

Add the missing timeout bit definition for (H)SCIF.
Clear the timeout and overrun flag bits during UART reset, cfr. the
initialization flowchart in the datasheet.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - No changes.
---
 drivers/tty/serial/sh-sci.c | 5 +++++
 drivers/tty/serial/sh-sci.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 0a2be5d293220e08..cf2a7903d2f0270b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2059,6 +2059,11 @@ static void sci_reset(struct uart_port *port)
 	sci_clear_SCxSR(port,
 			SCxSR_RDxF_CLEAR(port) & SCxSR_ERROR_CLEAR(port) &
 			SCxSR_BREAK_CLEAR(port));
+	if (sci_getreg(port, SCLSR)->size) {
+		status = serial_port_in(port, SCLSR);
+		status &= ~(SCLSR_TO | SCLSR_ORER);
+		serial_port_out(port, SCLSR, status);
+	}
 }
 
 static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
diff --git a/drivers/tty/serial/sh-sci.h b/drivers/tty/serial/sh-sci.h
index 7a4fa185b93ef307..c590418d2a40d78b 100644
--- a/drivers/tty/serial/sh-sci.h
+++ b/drivers/tty/serial/sh-sci.h
@@ -105,6 +105,7 @@ enum {
 #define SCFCR_LOOP	BIT(0)	/* Loopback Test */
 
 /* SCLSR (Line Status Register) on (H)SCIF */
+#define SCLSR_TO	BIT(2)	/* Timeout */
 #define SCLSR_ORER	BIT(0)	/* Overrun Error */
 
 /* SCSPTR (Serial Port Register), optional */
-- 
1.9.1