-----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 <redacted>; Greg Kroah-
Hartman [off-list ref]; Jiri Slaby [off-list ref]; open list:SERIAL DRIVERS [off-list ref]; Linux-
Renesas [off-list ref]; Linux Kernel Mailing List [off-list ref]; Hao Bui
[off-list ref]; KAZUMI HARADA [off-list ref]; Sasha Levin [off-list ref]; Chris Brandt
[off-list ref]; Magnus Damm [off-list ref]; Linux-sh list [off-list ref]; Rob Landley
[off-list ref]; John Paul Adrian Glaubitz [off-list ref]
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 [off-list ref] wrote:

quoted

On Thu, Apr 2, 2020 at 5:24 PM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Thu, Apr 2, 2020 at 1:28 PM Lad, Prabhakar
[off-list ref] wrote:

quoted

On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven [off-list ref] wrote:

quoted

On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
[off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Geert Uytterhoeven <geert@linux-m68k.org>
On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
[off-list ref] wrote:

quoted

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: <redacted>
Signed-off-by: Kazuhiro Fujita <redacted>
Signed-off-by: Hao Bui <redacted>
Signed-off-by: KAZUMI HARADA <redacted>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

quoted

  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