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: <[email protected]>
Signed-off-by: Kazuhiro Fujita <[email protected]>
Signed-off-by: Hao Bui <[email protected]>
Signed-off-by: KAZUMI HARADA <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
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
Hi Fujita-san,
CC -stable, +sasha, +seebe
On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
<[email protected]> 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: <[email protected]>
> Signed-off-by: Kazuhiro Fujita <[email protected]>
> Signed-off-by: Hao Bui <[email protected]>
> Signed-off-by: KAZUMI HARADA <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
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 -- [email protected]
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
Hi Geert,
Thank you for the review.
> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: 31 March 2020 16:18
> To: Kazuhiro Fujita <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; open list:SERIAL DRIVERS <linux-
> [email protected]>; Linux-Renesas <[email protected]>; Prabhakar <[email protected]>; Linux Kernel
> Mailing List <[email protected]>; Hao Bui <[email protected]>; KAZUMI HARADA <[email protected]>;
> Prabhakar Mahadev Lad <[email protected]>; Sasha Levin <[email protected]>; Chris Brandt
> <[email protected]>
> 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
> <[email protected]> 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: <[email protected]>
> > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > Signed-off-by: Hao Bui <[email protected]>
> > Signed-off-by: KAZUMI HARADA <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> 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 -- [email protected]
>
> 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
Hi Prabhakar,
On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
<[email protected]> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <[email protected]>
> > Sent: 31 March 2020 16:18
> > To: Kazuhiro Fujita <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; open list:SERIAL DRIVERS <linux-
> > [email protected]>; Linux-Renesas <[email protected]>; Prabhakar <[email protected]>; Linux Kernel
> > Mailing List <[email protected]>; Hao Bui <[email protected]>; KAZUMI HARADA <[email protected]>;
> > Prabhakar Mahadev Lad <[email protected]>; Sasha Levin <[email protected]>; Chris Brandt
> > <[email protected]>
> > 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
> > <[email protected]> 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: <[email protected]>
> > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > Signed-off-by: Hao Bui <[email protected]>
> > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > 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 -- [email protected]
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
Hi Geert,
> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: 31 March 2020 18:38
> To: Prabhakar Mahadev Lad <[email protected]>
> Cc: Kazuhiro Fujita <[email protected]>; Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>;
> open list:SERIAL DRIVERS <[email protected]>; Linux-Renesas <[email protected]>; Prabhakar
> <[email protected]>; Linux Kernel Mailing List <[email protected]>; Hao Bui <[email protected]>; KAZUMI
> HARADA <[email protected]>; Sasha Levin <[email protected]>; Chris Brandt <[email protected]>
> 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
> <[email protected]> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <[email protected]>
> > > Sent: 31 March 2020 16:18
> > > To: Kazuhiro Fujita <[email protected]>
> > > Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; open list:SERIAL DRIVERS <linux-
> > > [email protected]>; Linux-Renesas <[email protected]>; Prabhakar <[email protected]>; Linux Kernel
> > > Mailing List <[email protected]>; Hao Bui <[email protected]>; KAZUMI HARADA <[email protected]>;
> > > Prabhakar Mahadev Lad <[email protected]>; Sasha Levin <[email protected]>; Chris Brandt
> > > <[email protected]>
> > > 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
> > > <[email protected]> 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: <[email protected]>
> > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > Signed-off-by: Hao Bui <[email protected]>
> > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > 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 -- [email protected]
>
> 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
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
Hi Prabhakar,
On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
<[email protected]> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <[email protected]>
> > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > <[email protected]> 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: <[email protected]>
> > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > Signed-off-by: Hao Bui <[email protected]>
> > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > 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 -- [email protected]
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
Hi Geert,
On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> <[email protected]> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <[email protected]>
> > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > <[email protected]> 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: <[email protected]>
> > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > Signed-off-by: Hao Bui <[email protected]>
> > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
>
> > > 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 -- [email protected]
>
> 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
Hi Prabhakar,
On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
<[email protected]> wrote:
> On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > <[email protected]> wrote:
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <[email protected]>
> > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > <[email protected]> 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: <[email protected]>
> > > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > > Signed-off-by: Hao Bui <[email protected]>
> > > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > > > 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 -- [email protected]
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
On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > <[email protected]> wrote:
> > > > > -----Original Message-----
> > > > > From: Geert Uytterhoeven <[email protected]>
> > > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > > <[email protected]> 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: <[email protected]>
> > > > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > > > Signed-off-by: Hao Bui <[email protected]>
> > > > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > > > 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 <[email protected]>
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 -- [email protected]
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
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
On Wed, Apr 15, 2020 at 2:36 PM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> > <[email protected]> wrote:
> > > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > > <[email protected]> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Geert Uytterhoeven <[email protected]>
> > > > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > > > <[email protected]> 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: <[email protected]>
> > > > > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > > > > Signed-off-by: Hao Bui <[email protected]>
> > > > > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > >
> > > > > > 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 <[email protected]>
>
> 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 -- [email protected]
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
> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: 14 August 2020 16:25
> To: Lad, Prabhakar <[email protected]>
> Cc: Prabhakar Mahadev Lad <[email protected]>; Kazuhiro Fujita <[email protected]>; Greg Kroah-
> Hartman <[email protected]>; Jiri Slaby <[email protected]>; open list:SERIAL DRIVERS <[email protected]>; Linux-
> Renesas <[email protected]>; Linux Kernel Mailing List <[email protected]>; Hao Bui
> <[email protected]>; KAZUMI HARADA <[email protected]>; Sasha Levin <[email protected]>; Chris Brandt
> <[email protected]>; Magnus Damm <[email protected]>; Linux-sh list <[email protected]>; Rob Landley
> <[email protected]>; John Paul Adrian Glaubitz <[email protected]>
> 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 <[email protected]> wrote:
> > On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
> > > <[email protected]> wrote:
> > > > On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > > > > <[email protected]> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Geert Uytterhoeven <[email protected]>
> > > > > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > > > > <[email protected]> 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: <[email protected]>
> > > > > > > > Signed-off-by: Kazuhiro Fujita <[email protected]>
> > > > > > > > Signed-off-by: Hao Bui <[email protected]>
> > > > > > > > Signed-off-by: KAZUMI HARADA <[email protected]>
> > > > > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > >
> > > > > > > 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 <[email protected]>
> >
> > 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
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