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

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

[Kazuhiro Fujita]

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>
---
 drivers/tty/serial/sh-sci.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 0641a72..d646bc4 100644
--- 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;
-- 
2.7.4

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

[Geert Uytterhoeven]

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

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

[Prabhakar Mahadev Lad]

Hi Geert,

Thank you for the review.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2020 16:18
> 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>;
> Prabhakar Mahadev Lad <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
>
> 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.
>
I'll dig out some documentation wrt RZ/A2 & SuperH. Also H8300 needs to be considered. By any chance do you have RZ/A2 to test 😊.

> Nevertheless, this patch will need some testing on various hardware.
> Do you have a test case to verify the broken/fixed behavior?
>
Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.

Cheers,
--Prabhakar

> 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


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

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

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: 31 March 2020 16:18
> > 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>;
> > Prabhakar Mahadev Lad <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
>> > 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.
> >
> I'll dig out some documentation wrt RZ/A2 & SuperH. Also H8300 needs to be considered.

AFAIK, H8/300 has SCI only, so is not affected.

> By any chance do you have RZ/A2 to test .

Actually I do.

> > Nevertheless, this patch will need some testing on various hardware.
> > Do you have a test case to verify the broken/fixed behavior?
> >
> Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.

Thanks, that's good to know!

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

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

[Prabhakar Mahadev Lad]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2020 18:38
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Kazuhiro Fujita <kazuhiro.fujita.jg@renesas.com>; 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>; 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
>
> Hi Prabhakar,
>
> On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: 31 March 2020 16:18
> > > 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>;
> > > Prabhakar Mahadev Lad <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
> >> > 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.
> > >
> > I'll dig out some documentation wrt RZ/A2 & SuperH. Also H8300 needs to be considered.
>
For SuperH,

tango@tango:~/work/mainline/linux-next/arch/sh$ grep -nr PORT_SCIFA
kernel/cpu/sh4a/setup-sh7724.c:355:.type           = PORT_SCIFA,
kernel/cpu/sh4a/setup-sh7724.c:375:.type           = PORT_SCIFA,
kernel/cpu/sh4a/setup-sh7724.c:395:.type           = PORT_SCIFA,
kernel/cpu/sh4a/setup-sh7723.c:88:.type           = PORT_SCIFA,
kernel/cpu/sh4a/setup-sh7723.c:108:.type           = PORT_SCIFA,
kernel/cpu/sh4a/setup-sh7723.c:128:.type           = PORT_SCIFA,

And rest use PORT_SCIF, referring to [1] page 564 this behaves similar to SCIFA/B atleast sh7760.

But I couldn’t find datasheet for rest of the family ☹

> AFAIK, H8/300 has SCI only, so is not affected.
>
That’s one less thing to worry.

> > By any chance do you have RZ/A2 to test .
>
> Actually I do.
>
Great, could you please do the needy 😃

[1] https://datasheet.octopart.com/D17760BP200AD-Renesas-datasheet-124679.pdf

Cheers,
--Prabhakar

> > > Nevertheless, this patch will need some testing on various hardware.
> > > Do you have a test case to verify the broken/fixed behavior?
> > >
> > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and
> other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
>
> Thanks, that's good to know!
>
> 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


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

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

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > 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>

> > Nevertheless, this patch will need some testing on various hardware.
> > Do you have a test case to verify the broken/fixed behavior?
> >
> Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.

This can easily be tested on the console.  Basic testing can even be
done with an unmodified kernel, as there is already a "parity error"
notice message in the driver.

Enable even parity on the console:

$ stty evenp

(use "oddp" for odd parity, and invert all below)

Typing e.g. a single "p" should trigger a parity error.
Typing "o" shouldn't.
Without this patch, no parity error is detected on SCIF.

Likewise, pasting a sequence of "p" characters should trigger a lot of
parity errors, "o" shouldn't.
Without this patch, parity errors are detected on SCIF, except for the
first character.

For more advanced testing, make the following change to the driver:

- dev_notice(port->dev, "parity error\n");
+ dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
c, hweight8(c));

Pasting an alternating sequence of "p" and "o" characters should trigger
parity errors for the "p" characters.
Without this patch, they are triggered for the "o" characters instead.

With this patch, the issues above are fixed on SCIF.
This has been verified on:
  1. SCIF on R-Car Gen 2,
  2. SCIF on R-Car Gen3
  3. SCIF on RZ/A1H,
  4. SCIF on RZ/A2M.

However, I also tried this on HSCIF on R-Car Gen3, where I cannot
trigger parity errors at all.
Parabhakar: have you tried HSCIF on RZ/G1 and RZ/G2? Can you trigger
parity errors on HSCIF?

This has been regression-tested on:
  1. SCIFA on SH-Mobile AG5, R-Mobile A1, and R-Mobile APE6.

I haven't tested it yet on:
  1. SCIFB on SH/R-Mobile (needs wiring up),
  2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
  3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
  4. SuperH (only remote Migo-R available, but unaccessible).

I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
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

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

[Lad, Prabhakar]

Hi Geert,

On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > 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>
>
> > > Nevertheless, this patch will need some testing on various hardware.
> > > Do you have a test case to verify the broken/fixed behavior?
> > >
> > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
>
> This can easily be tested on the console.  Basic testing can even be
> done with an unmodified kernel, as there is already a "parity error"
> notice message in the driver.
>
> Enable even parity on the console:
>
> $ stty evenp
>
> (use "oddp" for odd parity, and invert all below)
>
> Typing e.g. a single "p" should trigger a parity error.
> Typing "o" shouldn't.
> Without this patch, no parity error is detected on SCIF.
>
> Likewise, pasting a sequence of "p" characters should trigger a lot of
> parity errors, "o" shouldn't.
> Without this patch, parity errors are detected on SCIF, except for the
> first character.
>
> For more advanced testing, make the following change to the driver:
>
> - dev_notice(port->dev, "parity error\n");
> + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> c, hweight8(c));
>
> Pasting an alternating sequence of "p" and "o" characters should trigger
> parity errors for the "p" characters.
> Without this patch, they are triggered for the "o" characters instead.
>
Thank you that makes life easier.

> With this patch, the issues above are fixed on SCIF.
> This has been verified on:
>   1. SCIF on R-Car Gen 2,
>   2. SCIF on R-Car Gen3
>   3. SCIF on RZ/A1H,
>   4. SCIF on RZ/A2M.
>
Thank you for the testing.

> However, I also tried this on HSCIF on R-Car Gen3, where I cannot
> trigger parity errors at all.
> Parabhakar: have you tried HSCIF on RZ/G1 and RZ/G2? Can you trigger
> parity errors on HSCIF?
>
I couldnt test this in RZ/Gx as the hscif interface is connected to
wifi, but I did manage to trigger this
on M3N following are the steps:
1: Set console as ttySC1 in booatargs
2: The login console will come up on both SC0&1
3: Login through SC0
4: Append securetty file: echo ttySC1 >> /etc/securetty
5: Login through SC1 (CN26 on M3N)
6:  And finally I repeated your steps using stty on SC1 (CN26) to
introduce parity error.

> This has been regression-tested on:
>   1. SCIFA on SH-Mobile AG5, R-Mobile A1, and R-Mobile APE6.
>
> I haven't tested it yet on:
>   1. SCIFB on SH/R-Mobile (needs wiring up),
>   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
>   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
>   4. SuperH (only remote Migo-R available, but unaccessible).
>
> I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
probably testing this on SuperH is gonna be a pain due to lack of
hardware availability,
(it needs to be tested on 19 platforms)
how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?

Cheers,
--Prabhakar

> 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

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

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > 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>
> >
> > > > Nevertheless, this patch will need some testing on various hardware.
> > > > Do you have a test case to verify the broken/fixed behavior?
> > > >
> > > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
> >
> > This can easily be tested on the console.  Basic testing can even be
> > done with an unmodified kernel, as there is already a "parity error"
> > notice message in the driver.
> >
> > Enable even parity on the console:
> >
> > $ stty evenp
> >
> > (use "oddp" for odd parity, and invert all below)
> >
> > Typing e.g. a single "p" should trigger a parity error.
> > Typing "o" shouldn't.
> > Without this patch, no parity error is detected on SCIF.
> >
> > Likewise, pasting a sequence of "p" characters should trigger a lot of
> > parity errors, "o" shouldn't.
> > Without this patch, parity errors are detected on SCIF, except for the
> > first character.
> >
> > For more advanced testing, make the following change to the driver:
> >
> > - dev_notice(port->dev, "parity error\n");
> > + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> > c, hweight8(c));
> >
> > Pasting an alternating sequence of "p" and "o" characters should trigger
> > parity errors for the "p" characters.
> > Without this patch, they are triggered for the "o" characters instead.
> >
> Thank you that makes life easier.
>
> > With this patch, the issues above are fixed on SCIF.
> > This has been verified on:
> >   1. SCIF on R-Car Gen 2,
> >   2. SCIF on R-Car Gen3
> >   3. SCIF on RZ/A1H,
> >   4. SCIF on RZ/A2M.
> >
> Thank you for the testing.
>
> > However, I also tried this on HSCIF on R-Car Gen3, where I cannot
> > trigger parity errors at all.
> > Parabhakar: have you tried HSCIF on RZ/G1 and RZ/G2? Can you trigger
> > parity errors on HSCIF?
> >
> I couldnt test this in RZ/Gx as the hscif interface is connected to
> wifi, but I did manage to trigger this
> on M3N following are the steps:
> 1: Set console as ttySC1 in booatargs
> 2: The login console will come up on both SC0&1
> 3: Login through SC0
> 4: Append securetty file: echo ttySC1 >> /etc/securetty
> 5: Login through SC1 (CN26 on M3N)
> 6:  And finally I repeated your steps using stty on SC1 (CN26) to
> introduce parity error.

Interesting, as I decided to pick up an M3-N target, too ;-)

Then I realized you used ttySC1 as the console, so DMA is disabled.
I just used the existing getty I have running on the second serial port,
which had DMA enabled, and no parity errors were triggered, as
sci_receive_chars() is never called.
If I disable DMA for HSCIF1 in the .dtsi, parity errors are detected
as expected.

Hence the driver does not support parity checking when DMA is enabled.
I also think it's not easy to add support for that, if possible at all.

> > I haven't tested it yet on:
> >   1. SCIFB on SH/R-Mobile (needs wiring up),
> >   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
> >   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
> >   4. SuperH (only remote Migo-R available, but unaccessible).
> >
> > I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
> probably testing this on SuperH is gonna be a pain due to lack of
> hardware availability,
> (it needs to be tested on 19 platforms)
> how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?

I had a look at a few SuperH docs w.r.t. framing/parity error behavior:
  - SCIF(A) on SH7724: similar to R-Car Gen2,
  - H(SCIF) on SH7734: same as on R-Car Gen2,
  - SCIF on SH7751: conflict between status register ("to be read next")
    and flowchart ("read from").

Let's wait a bit, we're in the middle of the merge window anyway.
Probably we can get it tested on SuperH during the coming weeks.

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

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

[Geert Uytterhoeven]

On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > > -----Original Message-----
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > 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>
> > >
> > > > > Nevertheless, this patch will need some testing on various hardware.
> > > > > Do you have a test case to verify the broken/fixed behavior?
> > > > >
> > > > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
> > >
> > > This can easily be tested on the console.  Basic testing can even be
> > > done with an unmodified kernel, as there is already a "parity error"
> > > notice message in the driver.
> > >
> > > Enable even parity on the console:
> > >
> > > $ stty evenp
> > >
> > > (use "oddp" for odd parity, and invert all below)
> > >
> > > Typing e.g. a single "p" should trigger a parity error.
> > > Typing "o" shouldn't.
> > > Without this patch, no parity error is detected on SCIF.
> > >
> > > Likewise, pasting a sequence of "p" characters should trigger a lot of
> > > parity errors, "o" shouldn't.
> > > Without this patch, parity errors are detected on SCIF, except for the
> > > first character.
> > >
> > > For more advanced testing, make the following change to the driver:
> > >
> > > - dev_notice(port->dev, "parity error\n");
> > > + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> > > c, hweight8(c));
> > >
> > > Pasting an alternating sequence of "p" and "o" characters should trigger
> > > parity errors for the "p" characters.
> > > Without this patch, they are triggered for the "o" characters instead.
> > >
> > Thank you that makes life easier.
> >
> > > With this patch, the issues above are fixed on SCIF.
> > > This has been verified on:
> > >   1. SCIF on R-Car Gen 2,
> > >   2. SCIF on R-Car Gen3
> > >   3. SCIF on RZ/A1H,
> > >   4. SCIF on RZ/A2M.

> If I disable DMA for HSCIF1 in the .dtsi, parity errors are detected
> as expected.

So HSCIF on R-Car Gen3 is affected, and fixed by this patch.

> Hence the driver does not support parity checking when DMA is enabled.
> I also think it's not easy to add support for that, if possible at all.
>
> > > I haven't tested it yet on:
> > >   1. SCIFB on SH/R-Mobile (needs wiring up),

SCIFB on R-Mobile A1 is not affected, and works before/after, as expected.

> > >   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),

Same for SCIFA, SCIFB on R-Car M2-W.

HSCIF on R-Car M2-W is affected, and fixed by this patch.

This means the modern platforms we care about are handled fine, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

That leaves us with testing on a few legacy platforms...

> > >   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
> > >   4. SuperH (only remote Migo-R available, but unaccessible).
> > >
> > > I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
> > probably testing this on SuperH is gonna be a pain due to lack of
> > hardware availability,
> > (it needs to be tested on 19 platforms)
> > how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?
>
> I had a look at a few SuperH docs w.r.t. framing/parity error behavior:
>   - SCIF(A) on SH7724: similar to R-Car Gen2,
>   - H(SCIF) on SH7734: same as on R-Car Gen2,
>   - SCIF on SH7751: conflict between status register ("to be read next")
>     and flowchart ("read from").
>
> Let's wait a bit, we're in the middle of the merge window anyway.
> Probably we can get it tested on SuperH during the coming weeks.

Anyone with a real (not qemu) SuperH system who can do the basic "stty evenp"
tests above, and report back to us?
Thanks a lot!

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

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

[Rob Landley]

On 4/15/20 7:36 AM, Geert Uytterhoeven wrote:
>> Let's wait a bit, we're in the middle of the merge window anyway.
>> Probably we can get it tested on SuperH during the coming weeks.
> 
> Anyone with a real (not qemu) SuperH system who can do the basic "stty evenp"
> tests above, and report back to us?
> Thanks a lot!
The j-core boards use either uartlite or 16550a for serial, and neither of my
legacy sh4 boxes is easily accessible right now. But if nobody manages to test
this before next merge window poke me and I can set one up.

Rob

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

[Geert Uytterhoeven]

On Wed, Apr 15, 2020 at 2:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > 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>
> > > >
> > > > > > Nevertheless, this patch will need some testing on various hardware.
> > > > > > Do you have a test case to verify the broken/fixed behavior?
> > > > > >
> > > > > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity) and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
> > > >
> > > > This can easily be tested on the console.  Basic testing can even be
> > > > done with an unmodified kernel, as there is already a "parity error"
> > > > notice message in the driver.
> > > >
> > > > Enable even parity on the console:
> > > >
> > > > $ stty evenp
> > > >
> > > > (use "oddp" for odd parity, and invert all below)
> > > >
> > > > Typing e.g. a single "p" should trigger a parity error.
> > > > Typing "o" shouldn't.
> > > > Without this patch, no parity error is detected on SCIF.
> > > >
> > > > Likewise, pasting a sequence of "p" characters should trigger a lot of
> > > > parity errors, "o" shouldn't.
> > > > Without this patch, parity errors are detected on SCIF, except for the
> > > > first character.
> > > >
> > > > For more advanced testing, make the following change to the driver:
> > > >
> > > > - dev_notice(port->dev, "parity error\n");
> > > > + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> > > > c, hweight8(c));
> > > >
> > > > Pasting an alternating sequence of "p" and "o" characters should trigger
> > > > parity errors for the "p" characters.
> > > > Without this patch, they are triggered for the "o" characters instead.
> > > >
> > > Thank you that makes life easier.
> > >
> > > > With this patch, the issues above are fixed on SCIF.
> > > > This has been verified on:
> > > >   1. SCIF on R-Car Gen 2,
> > > >   2. SCIF on R-Car Gen3
> > > >   3. SCIF on RZ/A1H,
> > > >   4. SCIF on RZ/A2M.
>
> > If I disable DMA for HSCIF1 in the .dtsi, parity errors are detected
> > as expected.
>
> So HSCIF on R-Car Gen3 is affected, and fixed by this patch.
>
> > Hence the driver does not support parity checking when DMA is enabled.
> > I also think it's not easy to add support for that, if possible at all.
> >
> > > > I haven't tested it yet on:
> > > >   1. SCIFB on SH/R-Mobile (needs wiring up),
>
> SCIFB on R-Mobile A1 is not affected, and works before/after, as expected.
>
> > > >   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
>
> Same for SCIFA, SCIFB on R-Car M2-W.
>
> HSCIF on R-Car M2-W is affected, and fixed by this patch.
>
> This means the modern platforms we care about are handled fine, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> That leaves us with testing on a few legacy platforms...
>
> > > >   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
> > > >   4. SuperH (only remote Migo-R available, but unaccessible).
> > > >
> > > > I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
> > > probably testing this on SuperH is gonna be a pain due to lack of
> > > hardware availability,
> > > (it needs to be tested on 19 platforms)
> > > how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?
> >
> > I had a look at a few SuperH docs w.r.t. framing/parity error behavior:
> >   - SCIF(A) on SH7724: similar to R-Car Gen2,
> >   - H(SCIF) on SH7734: same as on R-Car Gen2,
> >   - SCIF on SH7751: conflict between status register ("to be read next")
> >     and flowchart ("read from").

FTR, I gave it a try on the SH7751R-based I-O DATA USL-5P aka Landisk:
SCIF is affected, and fixed by commit 3dc4db3662366306 ("serial: sh-sci:
Make sure status register SCxSR is read in correct sequence").

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

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

[Prabhakar Mahadev Lad]

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 14 August 2020 16:25
> To: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; Kazuhiro Fujita <kazuhiro.fujita.jg@renesas.com>; 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>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Hao Bui
> <hao.bui.yg@renesas.com>; KAZUMI HARADA <kazumi.harada.rh@renesas.com>; Sasha Levin <sashal@kernel.org>; Chris Brandt
> <Chris.Brandt@renesas.com>; Magnus Damm <magnus.damm@gmail.com>; Linux-sh list <linux-sh@vger.kernel.org>; Rob Landley
> <rob@landley.net>; John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Subject: Re: [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct sequence
>
> On Wed, Apr 15, 2020 at 2:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> > > <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > 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>
> > > > >
> > > > > > > Nevertheless, this patch will need some testing on various hardware.
> > > > > > > Do you have a test case to verify the broken/fixed behavior?
> > > > > > >
> > > > > > Agreed, its been tested on RZ/G2x & RZ/G1x  by doing a loopback test, configure one interface as CS8 mode(8-bits data, No parity)
> and other as CS7 mode (7-bits data, 1-bit Parity) and parity errors should be detected.
> > > > >
> > > > > This can easily be tested on the console.  Basic testing can even be
> > > > > done with an unmodified kernel, as there is already a "parity error"
> > > > > notice message in the driver.
> > > > >
> > > > > Enable even parity on the console:
> > > > >
> > > > > $ stty evenp
> > > > >
> > > > > (use "oddp" for odd parity, and invert all below)
> > > > >
> > > > > Typing e.g. a single "p" should trigger a parity error.
> > > > > Typing "o" shouldn't.
> > > > > Without this patch, no parity error is detected on SCIF.
> > > > >
> > > > > Likewise, pasting a sequence of "p" characters should trigger a lot of
> > > > > parity errors, "o" shouldn't.
> > > > > Without this patch, parity errors are detected on SCIF, except for the
> > > > > first character.
> > > > >
> > > > > For more advanced testing, make the following change to the driver:
> > > > >
> > > > > - dev_notice(port->dev, "parity error\n");
> > > > > + dev_notice(port->dev, "parity error for char 0x%02x hweight %u\n",
> > > > > c, hweight8(c));
> > > > >
> > > > > Pasting an alternating sequence of "p" and "o" characters should trigger
> > > > > parity errors for the "p" characters.
> > > > > Without this patch, they are triggered for the "o" characters instead.
> > > > >
> > > > Thank you that makes life easier.
> > > >
> > > > > With this patch, the issues above are fixed on SCIF.
> > > > > This has been verified on:
> > > > >   1. SCIF on R-Car Gen 2,
> > > > >   2. SCIF on R-Car Gen3
> > > > >   3. SCIF on RZ/A1H,
> > > > >   4. SCIF on RZ/A2M.
> >
> > > If I disable DMA for HSCIF1 in the .dtsi, parity errors are detected
> > > as expected.
> >
> > So HSCIF on R-Car Gen3 is affected, and fixed by this patch.
> >
> > > Hence the driver does not support parity checking when DMA is enabled.
> > > I also think it's not easy to add support for that, if possible at all.
> > >
> > > > > I haven't tested it yet on:
> > > > >   1. SCIFB on SH/R-Mobile (needs wiring up),
> >
> > SCIFB on R-Mobile A1 is not affected, and works before/after, as expected.
> >
> > > > >   2. SCIFA, SCIFB, and HSCIF on R-Car Gen2 (needs wiring up),
> >
> > Same for SCIFA, SCIFB on R-Car M2-W.
> >
> > HSCIF on R-Car M2-W is affected, and fixed by this patch.
> >
> > This means the modern platforms we care about are handled fine, so
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > That leaves us with testing on a few legacy platforms...
> >
> > > > >   3. (H)SCIF on R-Car Gen1 (remote boards unaccessible at the moment),
> > > > >   4. SuperH (only remote Migo-R available, but unaccessible).
> > > > >
> > > > > I can test 1 and 2 (and perhaps 3 and 4) later, if needed.
> > > > probably testing this on SuperH is gonna be a pain due to lack of
> > > > hardware availability,
> > > > (it needs to be tested on 19 platforms)
> > > > how about #ifdef CONFIG_ARCH_RENESAS || CONFIG_H8300 and the fix ?
> > >
> > > I had a look at a few SuperH docs w.r.t. framing/parity error behavior:
> > >   - SCIF(A) on SH7724: similar to R-Car Gen2,
> > >   - H(SCIF) on SH7734: same as on R-Car Gen2,
> > >   - SCIF on SH7751: conflict between status register ("to be read next")
> > >     and flowchart ("read from").
>
> FTR, I gave it a try on the SH7751R-based I-O DATA USL-5P aka Landisk:
> SCIF is affected, and fixed by commit 3dc4db3662366306 ("serial: sh-sci:
> Make sure status register SCxSR is read in correct sequence").
>
Thank you Geert.

Cheers,
Prabhakar


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

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

[Rob Landley]

On 8/16/20 11:22 AM, Prabhakar Mahadev Lad wrote:
>> FTR, I gave it a try on the SH7751R-based I-O DATA USL-5P aka Landisk:
>> SCIF is affected, and fixed by commit 3dc4db3662366306 ("serial: sh-sci:
>> Make sure status register SCxSR is read in correct sequence").
>>
> Thank you Geert.
> 
> Cheers,
> Prabhakar

Did we ever figure out how to get linux to talk to the _first_ serial port on
the qemu-system-sh4 r2d board? I'm still doing:

  qemu-system-sh4 -M r2d -serial null -serial mon:stdio

Because I can only get a working console on the _second_ serial port. (SCI vs
SCIF I think?)

Rob

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

[Chris Brandt]

Hi Geert,

On Tue, Mar 31, 2020 1, Geert Uytterhoeven wrote:
> 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.

For the RZ/A2M, it is NOT a "SCIFA"...even though it says that in the 
hardware manual.

And honestly, I could not trace back where that IP came from. It was 
from somewhere in the MCU design section (not the SoC design section). 
Someone modified the IP so they put an "A" at the end to show it was 
different. Regardless, it has a different history than all the other IP 
supported by the SCI driver.

Chris