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@xxxxxxxxx> wrote:
> On Wed, Apr 1, 2020 at 1:43 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Tue, Mar 31, 2020 at 5:58 PM Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > On Fri, Mar 27, 2020 at 7:17 PM Kazuhiro Fujita
> > > > <kazuhiro.fujita.jg@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Kazuhiro Fujita <kazuhiro.fujita.jg@xxxxxxxxxxx>
> > > > > Signed-off-by: Hao Bui <hao.bui.yg@xxxxxxxxxxx>
> > > > > Signed-off-by: KAZUMI HARADA <kazumi.harada.rh@xxxxxxxxxxx>
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > > > 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@xxxxxxxxxxxxxx

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