* [PATCH 0/4] serial: sh-sci: FIFO initialization fixes
@ 2016-05-26 18:18 Geert Uytterhoeven
2016-05-26 18:18 ` [PATCH 1/4] serial: sh-sci: Do not start transfers from sci_startup() Geert Uytterhoeven
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2016-05-26 18:18 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Magnus Damm
Cc: linux-serial, linux-renesas-soc, Geert Uytterhoeven
Hi Greg, Jiri, Magnus,
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.
This series applies against both v4.6 and next-20160526. 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-v1.
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 | 17 +++++++++++------
drivers/tty/serial/sh-sci.h | 1 +
2 files changed, 12 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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/4] serial: sh-sci: Do not start transfers from sci_startup() 2016-05-26 18:18 [PATCH 0/4] serial: sh-sci: FIFO initialization fixes Geert Uytterhoeven @ 2016-05-26 18:18 ` Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() Geert Uytterhoeven ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Geert Uytterhoeven @ 2016-05-26 18:18 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Cc: linux-serial, linux-renesas-soc, 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> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() 2016-05-26 18:18 [PATCH 0/4] serial: sh-sci: FIFO initialization fixes Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 1/4] serial: sh-sci: Do not start transfers from sci_startup() Geert Uytterhoeven @ 2016-05-26 18:18 ` Geert Uytterhoeven 2016-06-21 7:35 ` Yoshihiro Shimoda 2016-05-26 18:18 ` [PATCH 3/4] serial: sh-sci: Clear RX, error, and break flags during reset Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 4/4] serial: sh-sci: Clear (H)SCIF timeout and overrun " Geert Uytterhoeven 3 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2016-05-26 18:18 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Cc: linux-serial, linux-renesas-soc, 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> --- 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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index d6ba90c572f7475c..4d2f916f45277e20 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1896,6 +1896,8 @@ static void sci_shutdown(struct uart_port *port) spin_lock_irqsave(&port->lock, flags); sci_stop_rx(port); sci_stop_tx(port); + /* Stop RX and TX, disable related interrupts */ + serial_port_out(port, SCSCR, 0); spin_unlock_irqrestore(&port->lock, flags); #ifdef CONFIG_SERIAL_SH_SCI_DMA -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() 2016-05-26 18:18 ` [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() Geert Uytterhoeven @ 2016-06-21 7:35 ` Yoshihiro Shimoda 2016-06-21 14:52 ` Geert Uytterhoeven 0 siblings, 1 reply; 9+ messages in thread From: Yoshihiro Shimoda @ 2016-06-21 7:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-serial, linux-renesas-soc, Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Hi, Geert-san, > From: Geert Uytterhoeven > Sent: Friday, May 27, 2016 3:19 AM > > 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> > --- > 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, If we write zero to the register, we cannot use the port as a console after it is called. In fact, I have an issue while rc scripts are running on my root filesystem. When rc scripts is running, "shutdown" is called a lot. After the "shutdown", if the kernel will put strings using a console, it cannot put strings because the register is zero (TE and CKE are 0). So, we have to consider it. FYI, I made a patch to fix this issue. (Perhaps, both the CKE and TE should be set in the serial_console_write(), but I don't know how to set the CKE for now :) ) Best regards, Yoshihiro Shimoda --- diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index afa25ec..b5b1b38 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1989,6 +1989,7 @@ static void sci_shutdown(struct uart_port *port) { struct sci_port *s = to_sci_port(port); unsigned long flags; + unsigned int ctrl; dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); @@ -1998,8 +1999,12 @@ static void sci_shutdown(struct uart_port *port) spin_lock_irqsave(&port->lock, flags); sci_stop_rx(port); sci_stop_tx(port); - /* Stop RX and TX, disable related interrupts */ - serial_port_out(port, SCSCR, 0); + /* Stop RX and TX, disable related interrupts, keep clock source */ + ctrl = serial_port_in(port, SCSCR); + ctrl = (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | + (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); + serial_port_out(port, SCSCR, ctrl); + spin_unlock_irqrestore(&port->lock, flags); #ifdef CONFIG_SERIAL_SH_SCI_DMA @@ -2801,6 +2806,8 @@ static void serial_console_write(struct console *co, const char *s, ctrl = serial_port_in(port, SCSCR); ctrl_temp = (sci_port->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); + ctrl_temp |= SCSCR_TE; /* FIXME: while "break ctl" is on */ + serial_port_out(port, SCSCR, ctrl_temp); uart_console_write(port, s, count, serial_console_putchar); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() 2016-06-21 7:35 ` Yoshihiro Shimoda @ 2016-06-21 14:52 ` Geert Uytterhoeven 2016-06-23 10:42 ` Yoshihiro Shimoda 0 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2016-06-21 14:52 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Geert Uytterhoeven, linux-serial, linux-renesas-soc, Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Hi Shimoda-san, On Tue, Jun 21, 2016 at 9:35 AM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: >> From: Geert Uytterhoeven >> Sent: Friday, May 27, 2016 3:19 AM >> >> 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> >> --- >> 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, > > If we write zero to the register, we cannot use the port as a console after it is called. > In fact, I have an issue while rc scripts are running on my root filesystem. > When rc scripts is running, "shutdown" is called a lot. > After the "shutdown", if the kernel will put strings using a console, it cannot put strings > because the register is zero (TE and CKE are 0). So, we have to consider it. > > FYI, I made a patch to fix this issue. > (Perhaps, both the CKE and TE should be set in the serial_console_write(), but I don't know how to set the CKE for now :) ) > > Best regards, > Yoshihiro Shimoda > --- > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index afa25ec..b5b1b38 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -1989,6 +1989,7 @@ static void sci_shutdown(struct uart_port *port) > { > struct sci_port *s = to_sci_port(port); > unsigned long flags; > + unsigned int ctrl; > > dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); > > @@ -1998,8 +1999,12 @@ static void sci_shutdown(struct uart_port *port) > spin_lock_irqsave(&port->lock, flags); > sci_stop_rx(port); > sci_stop_tx(port); > - /* Stop RX and TX, disable related interrupts */ > - serial_port_out(port, SCSCR, 0); > + /* Stop RX and TX, disable related interrupts, keep clock source */ > + ctrl = serial_port_in(port, SCSCR); > + ctrl = (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | > + (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); My bad. We should indeed keep CKE, as the serial console relies on that. I'm just wondering why I didn't notice this, as at least on Koelsch, the external SCIF clock is used, implying a non-zero CKEx setting. > + serial_port_out(port, SCSCR, ctrl); > + > spin_unlock_irqrestore(&port->lock, flags); > > #ifdef CONFIG_SERIAL_SH_SCI_DMA > @@ -2801,6 +2806,8 @@ static void serial_console_write(struct console *co, const char *s, > ctrl = serial_port_in(port, SCSCR); > ctrl_temp = (sci_port->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | > (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); > + ctrl_temp |= SCSCR_TE; /* FIXME: while "break ctl" is on */ This shouldn't be needed, as SCSCR_TE should be set in sci_port->cfg->scscr (look in all places where it's initialized). Can you please double check? > + > serial_port_out(port, SCSCR, ctrl_temp); > > uart_console_write(port, s, count, serial_console_putchar); Thanks for your report! 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() 2016-06-21 14:52 ` Geert Uytterhoeven @ 2016-06-23 10:42 ` Yoshihiro Shimoda 2016-06-23 11:20 ` Geert Uytterhoeven 0 siblings, 1 reply; 9+ messages in thread From: Yoshihiro Shimoda @ 2016-06-23 10:42 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, linux-serial, linux-renesas-soc, Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Hi Geert-san, > From: Geert Uytterhoeven > Sent: Tuesday, June 21, 2016 11:52 PM > > Hi Shimoda-san, > > On Tue, Jun 21, 2016 at 9:35 AM, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > >> From: Geert Uytterhoeven > >> Sent: Friday, May 27, 2016 3:19 AM > >> > >> 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> > >> --- > >> 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, > > > > If we write zero to the register, we cannot use the port as a console after it is called. > > In fact, I have an issue while rc scripts are running on my root filesystem. > > When rc scripts is running, "shutdown" is called a lot. > > After the "shutdown", if the kernel will put strings using a console, it cannot put strings > > because the register is zero (TE and CKE are 0). So, we have to consider it. > > > > FYI, I made a patch to fix this issue. > > (Perhaps, both the CKE and TE should be set in the serial_console_write(), but I don't know how to set the CKE for now :) ) > > > > Best regards, > > Yoshihiro Shimoda > > --- > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > > index afa25ec..b5b1b38 100644 > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -1989,6 +1989,7 @@ static void sci_shutdown(struct uart_port *port) > > { > > struct sci_port *s = to_sci_port(port); > > unsigned long flags; > > + unsigned int ctrl; > > > > dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); > > > > @@ -1998,8 +1999,12 @@ static void sci_shutdown(struct uart_port *port) > > spin_lock_irqsave(&port->lock, flags); > > sci_stop_rx(port); > > sci_stop_tx(port); > > - /* Stop RX and TX, disable related interrupts */ > > - serial_port_out(port, SCSCR, 0); > > + /* Stop RX and TX, disable related interrupts, keep clock source */ > > + ctrl = serial_port_in(port, SCSCR); > > + ctrl = (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | > > + (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); > > My bad. We should indeed keep CKE, as the serial console relies on that. > I'm just wondering why I didn't notice this, as at least on Koelsch, the > external SCIF clock is used, implying a non-zero CKEx setting. > > > + serial_port_out(port, SCSCR, ctrl); > > + > > spin_unlock_irqrestore(&port->lock, flags); > > > > #ifdef CONFIG_SERIAL_SH_SCI_DMA > > @@ -2801,6 +2806,8 @@ static void serial_console_write(struct console *co, const char *s, > > ctrl = serial_port_in(port, SCSCR); > > ctrl_temp = (sci_port->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | > > (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); > > + ctrl_temp |= SCSCR_TE; /* FIXME: while "break ctl" is on */ > > This shouldn't be needed, as SCSCR_TE should be set in sci_port->cfg->scscr > (look in all places where it's initialized). > Can you please double check? Sorry for the check (because I took a day off yesterday). As you mentioned it, this is not needed. (I should have tested on such a code before I sent this report...) Best regards, Yoshihiro Shimoda > > + > > serial_port_out(port, SCSCR, ctrl_temp); > > > > uart_console_write(port, s, count, serial_console_putchar); > > Thanks for your report! > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() 2016-06-23 10:42 ` Yoshihiro Shimoda @ 2016-06-23 11:20 ` Geert Uytterhoeven 0 siblings, 0 replies; 9+ messages in thread From: Geert Uytterhoeven @ 2016-06-23 11:20 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Geert Uytterhoeven, linux-serial, linux-renesas-soc, Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Hi Shimoda-san, On Thu, Jun 23, 2016 at 12:42 PM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: >> From: Geert Uytterhoeven >> Sent: Tuesday, June 21, 2016 11:52 PM >> On Tue, Jun 21, 2016 at 9:35 AM, Yoshihiro Shimoda >> <yoshihiro.shimoda.uh@renesas.com> wrote: >> >> From: Geert Uytterhoeven >> >> Sent: Friday, May 27, 2016 3:19 AM >> >> >> >> 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> >> >> --- >> >> 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, >> > >> > If we write zero to the register, we cannot use the port as a console after it is called. >> > In fact, I have an issue while rc scripts are running on my root filesystem. >> > When rc scripts is running, "shutdown" is called a lot. >> > After the "shutdown", if the kernel will put strings using a console, it cannot put strings >> > because the register is zero (TE and CKE are 0). So, we have to consider it. >> > >> > FYI, I made a patch to fix this issue. >> > (Perhaps, both the CKE and TE should be set in the serial_console_write(), but I don't know how to set the CKE for now :) ) >> > >> > Best regards, >> > Yoshihiro Shimoda >> > --- >> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> > index afa25ec..b5b1b38 100644 >> > --- a/drivers/tty/serial/sh-sci.c >> > +++ b/drivers/tty/serial/sh-sci.c >> > @@ -1989,6 +1989,7 @@ static void sci_shutdown(struct uart_port *port) >> > { >> > struct sci_port *s = to_sci_port(port); >> > unsigned long flags; >> > + unsigned int ctrl; >> > >> > dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); >> > >> > @@ -1998,8 +1999,12 @@ static void sci_shutdown(struct uart_port *port) >> > spin_lock_irqsave(&port->lock, flags); >> > sci_stop_rx(port); >> > sci_stop_tx(port); >> > - /* Stop RX and TX, disable related interrupts */ >> > - serial_port_out(port, SCSCR, 0); >> > + /* Stop RX and TX, disable related interrupts, keep clock source */ >> > + ctrl = serial_port_in(port, SCSCR); >> > + ctrl = (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | >> > + (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); >> >> My bad. We should indeed keep CKE, as the serial console relies on that. >> I'm just wondering why I didn't notice this, as at least on Koelsch, the >> external SCIF clock is used, implying a non-zero CKEx setting. >> >> > + serial_port_out(port, SCSCR, ctrl); >> > + >> > spin_unlock_irqrestore(&port->lock, flags); >> > >> > #ifdef CONFIG_SERIAL_SH_SCI_DMA >> > @@ -2801,6 +2806,8 @@ static void serial_console_write(struct console *co, const char *s, >> > ctrl = serial_port_in(port, SCSCR); >> > ctrl_temp = (sci_port->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0)) | >> > (ctrl & (SCSCR_CKE1 | SCSCR_CKE0)); >> > + ctrl_temp |= SCSCR_TE; /* FIXME: while "break ctl" is on */ >> >> This shouldn't be needed, as SCSCR_TE should be set in sci_port->cfg->scscr >> (look in all places where it's initialized). >> Can you please double check? > > Sorry for the check (because I took a day off yesterday). > As you mentioned it, this is not needed. > (I should have tested on such a code before I sent this report...) Thanks, I will send an updated patch shortly. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] serial: sh-sci: Clear RX, error, and break flags during reset 2016-05-26 18:18 [PATCH 0/4] serial: sh-sci: FIFO initialization fixes Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 1/4] serial: sh-sci: Do not start transfers from sci_startup() Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() Geert Uytterhoeven @ 2016-05-26 18:18 ` Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 4/4] serial: sh-sci: Clear (H)SCIF timeout and overrun " Geert Uytterhoeven 3 siblings, 0 replies; 9+ messages in thread From: Geert Uytterhoeven @ 2016-05-26 18:18 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Cc: linux-serial, linux-renesas-soc, 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> --- 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 4d2f916f45277e20..549d799cb18fafe7 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2052,6 +2052,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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] serial: sh-sci: Clear (H)SCIF timeout and overrun during reset 2016-05-26 18:18 [PATCH 0/4] serial: sh-sci: FIFO initialization fixes Geert Uytterhoeven ` (2 preceding siblings ...) 2016-05-26 18:18 ` [PATCH 3/4] serial: sh-sci: Clear RX, error, and break flags during reset Geert Uytterhoeven @ 2016-05-26 18:18 ` Geert Uytterhoeven 3 siblings, 0 replies; 9+ messages in thread From: Geert Uytterhoeven @ 2016-05-26 18:18 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Magnus Damm Cc: linux-serial, linux-renesas-soc, 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> --- 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 549d799cb18fafe7..fe9ad1b56420876a 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2056,6 +2056,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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-23 11:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-26 18:18 [PATCH 0/4] serial: sh-sci: FIFO initialization fixes Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 1/4] serial: sh-sci: Do not start transfers from sci_startup() Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 2/4] serial: sh-sci: Stop transfers in sci_shutdown() Geert Uytterhoeven 2016-06-21 7:35 ` Yoshihiro Shimoda 2016-06-21 14:52 ` Geert Uytterhoeven 2016-06-23 10:42 ` Yoshihiro Shimoda 2016-06-23 11:20 ` Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 3/4] serial: sh-sci: Clear RX, error, and break flags during reset Geert Uytterhoeven 2016-05-26 18:18 ` [PATCH 4/4] serial: sh-sci: Clear (H)SCIF timeout and overrun " Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.