> -----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
