On Tue, Oct 10, 2023 at 07:08:42AM +0000, Biju Das wrote: > Hi Paul, > > Thanks for the patch. > > > Subject: [PATCH v2 11/19] serial: sh: Fix error handling > > > > The current SCIF error handling is broken for the RZ/G2L. After a break > > condition has been triggered, the current code is unable to clear the error > > and serial port output never resumes. > > > > The RZ/G2L datasheet says that most error conditions are cleared by > > resetting the relevant error bits in the FSR & LSR registers to zero. > > To clear framing errors, the invalid data also needs to be read out of the > > receive FIFO. > > > > After reviewing datasheets for RZ/G2{H,M,N,E}, R-Car Gen4, R-Car Gen3 and > > even SH7751 SoCs, it's clear that this is the way to clear errors for all > > of these SoCs. > > > > While we're here, annotate the handle_error() function with a couple of > > comments as the reads and writes themselves don't immediately make it clear > > what we're doing. > > > > Signed-off-by: Paul Barker <paul.barker...@bp.renesas.com> > > --- > > v1->v2: > > * New patch after discussion with Marek & further investigation. > > > > I'm not going to put a `Fixes` tag on this, the error handling has been > > writing ones to clear errors since at least 2007 so maybe it was originally > > right for some board. It doesn't seem to apply to any of the current users > > of the SCIF driver though. > > > > drivers/serial/serial_sh.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/serial/serial_sh.c b/drivers/serial/serial_sh.c index > > 5e543dbf3d58..2b650d458e71 100644 > > --- a/drivers/serial/serial_sh.c > > +++ b/drivers/serial/serial_sh.c > > @@ -79,10 +79,22 @@ sh_serial_setbrg_generic(struct uart_port *port, int > > clk, int baudrate) > > > > static void handle_error(struct uart_port *port) { > > - sci_in(port, SCxSR); > > - sci_out(port, SCxSR, SCxSR_ERROR_CLEAR(port)); > > + /* > > + * Most errors are cleared by resetting the relevant error bits to > > zero > > + * in the FSR & LSR registers. For each register, a read followed by > > a > > + * write is needed according to the relevant datasheets. > > + */ > > + unsigned short status = sci_in(port, SCxSR); > > + sci_out(port, SCxSR, status & ~SCIF_ERRORS); > > sci_in(port, SCLSR); > > sci_out(port, SCLSR, 0x00); > > + > > + /* > > + * To clear framing errors, we also need to read and discard a > > + * character. > > + */ > > + if (status & SCIF_FER) > > + sci_in(port, SCxRDR); > > Do we need to read status again to make sure framing error > is cleared?
I've not seen anything to suggest that we need to do that and this implementation is clearing the framing errors successfully. Thanks, Paul
signature.asc
Description: PGP signature