From: Geert Uytterhoeven <geert@linux-m68k.org>
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
Date: Fri, 14 Aug 2020 15:25:23 +0000 [thread overview]
Message-ID: <CAMuHMdU0EiQuk_bWx1yrmbBTXg8mL-PeN2=P61xQ5Ucb5QmYYg@mail.gmail.com> (raw)
In-Reply-To: <CAMuHMdUyV58t3eihBJv2xex5gW1Oef37Jo3FHoJstU=SspmpHA@mail.gmail.com>
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
next prev parent reply other threads:[~2020-08-14 15:25 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1585333048-31828-1-git-send-email-kazuhiro.fujita.jg@renesas.com>
[not found] ` <CAMuHMdW+u5r6zyxFJsVzj21BYDrKCr=Q6Ojk5VeN+mkhvXX9Jw@mail.gmail.com>
[not found] ` <OSBPR01MB3590E3D12546BC6711CEB542AAC80@OSBPR01MB3590.jpnprd01.prod.outlook.com>
[not found] ` <CAMuHMdXmfQ0x7mCZ-E7OPQFv2z-=mFDT20hJ2_JKax=OePB8eA@mail.gmail.com>
[not found] ` <CA+V-a8vPn_z_j1Vwr_1F=dCw8H=g5UMWvWxgRqBeVR7dzHPz8Q@mail.gmail.com>
2020-04-02 15:24 ` [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct sequence Geert Uytterhoeven
2020-04-15 12:36 ` Geert Uytterhoeven
2020-04-15 23:21 ` Rob Landley
2020-08-14 15:25 ` Geert Uytterhoeven [this message]
2020-08-16 16:22 ` Prabhakar Mahadev Lad
2020-08-17 3:17 ` Rob Landley
2020-08-18 15:11 ` SCI on R2D+ (was: Re: [PATCH] serial: sh-sci: Make sure status register SCxSR is read in correct seq Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMuHMdU0EiQuk_bWx1yrmbBTXg8mL-PeN2=P61xQ5Ucb5QmYYg@mail.gmail.com' \
--to=geert@linux-m68k.org \
--cc=Chris.Brandt@renesas.com \
--cc=glaubitz@physik.fu-berlin.de \
--cc=gregkh@linuxfoundation.org \
--cc=hao.bui.yg@renesas.com \
--cc=jslaby@suse.com \
--cc=kazuhiro.fujita.jg@renesas.com \
--cc=kazumi.harada.rh@renesas.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=prabhakar.csengg@gmail.com \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=rob@landley.net \
--cc=sashal@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).