Re: [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct sequence

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Kazuhiro Fujita <kazuhiro.fujita.jg@renesas.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Prabhakar <prabhakar.csengg@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Hao Bui <hao.bui.yg@renesas.com>,
	KAZUMI HARADA <kazumi.harada.rh@renesas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Sasha Levin <sashal@kernel.org>,
	Chris Brandt <Chris.Brandt@renesas.com>
Subject: Re: [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct sequence
Date: Tue, 31 Mar 2020 17:17:39 +0200	[thread overview]
Message-ID: <CAMuHMdW+u5r6zyxFJsVzj21BYDrKCr=Q6Ojk5VeN+mkhvXX9Jw@mail.gmail.com> (raw)
In-Reply-To: <1585333048-31828-1-git-send-email-kazuhiro.fujita.jg@renesas.com>

Hi Fujita-san,

CC -stable, +sasha, +seebe

On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
<kazuhiro.fujita.jg@renesas.com> wrote:
> For SCIF and HSCIF interfaces the SCxSR register holds the status of
> data that is to be read next from SCxRDR register, But where as for
> SCIFA and SCIFB interfaces SCxSR register holds status of data that is
> previously read from SCxRDR register.
>
> This patch makes sure the status register is read depending on the port
> types so that errors are caught accordingly.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Kazuhiro Fujita <kazuhiro.fujita.jg@renesas.com>
> Signed-off-by: Hao Bui <hao.bui.yg@renesas.com>
> Signed-off-by: KAZUMI HARADA <kazumi.harada.rh@renesas.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -870,9 +870,16 @@ static void sci_receive_chars(struct uart_port *port)
>                                 tty_insert_flip_char(tport, c, TTY_NORMAL);
>                 } else {
>                         for (i = 0; i < count; i++) {
> -                               char c = serial_port_in(port, SCxRDR);
> -
> -                               status = serial_port_in(port, SCxSR);
> +                               char c;
> +
> +                               if (port->type == PORT_SCIF ||
> +                                   port->type == PORT_HSCIF) {
> +                                       status = serial_port_in(port, SCxSR);
> +                                       c = serial_port_in(port, SCxRDR);
> +                               } else {
> +                                       c = serial_port_in(port, SCxRDR);
> +                                       status = serial_port_in(port, SCxSR);
> +                               }
>                                 if (uart_handle_sysrq_char(port, c)) {
>                                         count--; i--;
>                                         continue;

I can confirm that the documentation for the Serial Status Register on
  1. (H)SCIF on R-Car Gen1/2/3 says the framing/error flag applies to
     the data that is "to be read next" from the FIFO., and that the
     "Sample Flowchart for Serial Reception (2)" confirms this,
  2. SCIF[AB] on R-Car Gen2, SH-Mobile AG5, R-Mobile A1 and APE6 says
     the framing/error flag applies to the receive data that is "read"
     from the FIFO, and that the "Example of Flow for Serial Reception
     (2)" confirms this,
  3. SCIF on RZ/A1H says something similar as for (H)SCIF above, using
     slightly different wording, also confirmed by the "Sample Flowchart
     for Receiving Serial Data (2)".

However, the documentation for "SCIFA" on RZ/A2 (for which we use
PORT_SCIF, not PORT_SCIFA, in the driver) has conflicting information:
  1. Section 17.2.7 "Serial Status Register (FSR)" says:
       - A receive framing/parity error occurred in the "next receive
         data read" from the FIFO,
       - Indicates whether there is a framing/parity error in the data
         "read" from the FIFO.
  2. Figure 17.8 "Sample Flowchart for Receiving Serial Data in
     Asynchronous Mode (2)".
       - Whether a framing error or parity error has occurred in the
         received data that is "read" from the FIFO.

So while the change looks OK for most Renesas ARM SoCs, the situation
for RZ/A2 is unclear.
Note that the above does not take into account variants used on SuperH
SoCs.

Nevertheless, this patch will need some testing on various hardware.
Do you have a test case to verify the broken/fixed behavior?

Thanks!

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